Skip to content

fix: allow skill owner to move skill to an org acct while publishing existing skill #1562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pushmatrix wants to merge 1 commit into
base: main
Choose a base branch
from
+332 −6
Open

fix: allow skill owner to move skill to an org acct while publishing existing skill #1562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
301 changes: 301 additions & 0 deletions convex/skills.rateLimit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1357,4 +1357,305 @@ describe("skills anti-spam guards", () => {
}),
);
});

it("allows owner to move skill from personal publisher to org publisher", async () => {
const patch = vi.fn(async () => {});
const insert = vi.fn(async (table: string) => {
if (table === "skillVersions") return "skillVersions:1";
if (table === "skillEmbeddings") return "skillEmbeddings:1";
if (table === "embeddingSkillMap") return "embeddingSkillMap:1";
if (table === "skillVersionFingerprints") return "skillVersionFingerprints:1";
throw new Error(`unexpected insert table ${table}`);
});
const existingSkill = {
_id: "skills:1",
slug: "shop",
displayName: "Shop",
summary: "A skill",
ownerUserId: "users:owner",
ownerPublisherId: "publishers:personal",
latestVersionId: undefined,
tags: {},
softDeletedAt: undefined,
badges: { redactionApproved: undefined, highlighted: undefined, official: undefined, deprecated: undefined },
moderationStatus: "active",
moderationReason: "pending.scan",
moderationNotes: undefined,
moderationVerdict: "clean",
moderationReasonCodes: undefined,
moderationEvidence: undefined,
moderationSummary: "Clean",
moderationEngineVersion: "test",
moderationEvaluatedAt: 1,
moderationSourceVersionId: undefined,
quality: undefined,
moderationFlags: undefined,
isSuspicious: false,
reportCount: 0,
lastReportedAt: undefined,
statsDownloads: 0,
statsStars: 0,
statsInstallsCurrent: 0,
statsInstallsAllTime: 0,
stats: { downloads: 0, installsCurrent: 0, installsAllTime: 0, stars: 0, versions: 0, comments: 0 },
createdAt: 1,
updatedAt: 1,
manualOverride: undefined,
};
const db = {
get: vi.fn(async (id: string) => {
if (id === "users:owner") {
return { _id: "users:owner", handle: "pushmatrix", deletedAt: undefined, deactivatedAt: undefined, trustedPublisher: false, role: "user" };
}
if (id === "publishers:personal") return { _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" };
if (id === "publishers:org") return { _id: "publishers:org", kind: "org", deletedAt: undefined, deactivatedAt: undefined };
return null;
Comment on lines +1406 to +1412
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 db.get mock missing "publishers:org" — positive test never exercises new code

With ownerPublisherId: "publishers:org" and personalPublisher._id = "publishers:personal", the handler calls requirePublisherRole(ctx, { publisherId: "publishers:org", ... }) at skills.ts:5788. That function calls ctx.db.get("publishers:org"), which returns null here, so isPublisherActive(null) → false → throws ConvexError("Publisher not found"). patch is never called and the expect(patch).toHaveBeenCalledWith(...) assertion fails.

Add the missing publisher to the get mock:

Suggested change
get: vi.fn(async (id: string) => {
if (id === "users:owner") {
return {
_id: "users:owner",
handle: "pushmatrix",
deletedAt: undefined,
deactivatedAt: undefined,
};
}
if (id === "publishers:personal") {
return { _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" };
}
return null;
get: vi.fn(async (id: string) => {
if (id === "users:owner") {
return {
_id: "users:owner",
handle: "pushmatrix",
deletedAt: undefined,
deactivatedAt: undefined,
};
}
if (id === "publishers:personal") {
return { _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" };
}
if (id === "publishers:org") {
return { _id: "publishers:org", kind: "org", deletedAt: undefined, deactivatedAt: undefined };
}
return null;
}),

The same gap exists in the third test ("updates slug aliases", line 1501–1508) — add the identical "publishers:org" branch there as well.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: convex/skills.rateLimit.test.ts
Line: 1364-1376

Comment:
**`db.get` mock missing `"publishers:org"` — positive test never exercises new code**

With `ownerPublisherId: "publishers:org"` and `personalPublisher._id = "publishers:personal"`, the handler calls `requirePublisherRole(ctx, { publisherId: "publishers:org", ... })` at `skills.ts:5788`. That function calls `ctx.db.get("publishers:org")`, which returns `null` here, so `isPublisherActive(null)` → false → throws `ConvexError("Publisher not found")`. `patch` is never called and the `expect(patch).toHaveBeenCalledWith(...)` assertion fails.

Add the missing publisher to the `get` mock:

```suggestion
      get: vi.fn(async (id: string) => {
        if (id === "users:owner") {
          return {
            _id: "users:owner",
            handle: "pushmatrix",
            deletedAt: undefined,
            deactivatedAt: undefined,
          };
        }
        if (id === "publishers:personal") {
          return { _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" };
        }
        if (id === "publishers:org") {
          return { _id: "publishers:org", kind: "org", deletedAt: undefined, deactivatedAt: undefined };
        }
        return null;
      }),
```

The same gap exists in the third test ("updates slug aliases", line 1501–1508) — add the identical `"publishers:org"` branch there as well.

How can I resolve this? If you propose a fix, please make it concise.

}),
query: vi.fn((table: string) => {
if (table === "skills") return { withIndex: () => ({ unique: async () => existingSkill }) };
if (table === "publishers") return { withIndex: () => ({ unique: async () => ({ _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" }) }) };
if (table === "publisherMembers") return { withIndex: () => ({ unique: async () => ({ role: "publisher", publisherId: "publishers:org" }) }) };
if (table === "skillSlugAliases") return { withIndex: (name: string) => name === "by_skill" ? { collect: async () => [] } : { unique: async () => null } };
if (table === "skillVersions") return { withIndex: () => ({ unique: async () => null }) };
if (table === "skillBadges") return { withIndex: () => ({ take: async () => [] }) };
if (table === "skillEmbeddings") return { withIndex: () => ({ unique: async () => null }) };
throw new Error(`unexpected table ${table}`);
}),
patch,
insert,
normalizeId: vi.fn(),
};

const result = await insertVersionHandler(
{ db } as never,
createPublishArgs({ userId: "users:owner", slug: "shop", ownerPublisherId: "publishers:org" }) as never,
);

expect(patch).toHaveBeenCalledWith("skills:1", expect.objectContaining({ ownerPublisherId: "publishers:org" }));
expect(result).toEqual({ skillId: "skills:1", versionId: "skillVersions:1", embeddingId: "skillEmbeddings:1" });
});

it("blocks non-owner from claiming a skill with a different publisher", async () => {
// Stranger publishes under their own personal publisher (skips requirePublisherRole),
// but the slug belongs to a different user — should throw slug-taken.
const db = {
get: vi.fn(async (id: string) => {
if (id === "users:stranger") return { _id: "users:stranger", handle: "stranger", personalPublisherId: "publishers:stranger-personal", deletedAt: undefined, deactivatedAt: undefined };
if (id === "users:owner") return { _id: "users:owner", handle: "alice", deletedAt: undefined, deactivatedAt: undefined };
if (id === "publishers:stranger-personal") return { _id: "publishers:stranger-personal", handle: "stranger", kind: "user", linkedUserId: "users:stranger", deletedAt: undefined, deactivatedAt: undefined };
return null;
}),
query: vi.fn((table: string) => {
if (table === "skills") {
return {
withIndex: () => ({
unique: async () => ({
_id: "skills:1",
slug: "shop",
ownerUserId: "users:owner",
ownerPublisherId: "publishers:other",
softDeletedAt: undefined,
moderationStatus: "active",
moderationFlags: undefined,
}),
}),
};
}
if (table === "publishers") {
return {
withIndex: (name: string) => {
if (name === "by_handle") return { unique: async () => ({ _id: "publishers:stranger-personal", handle: "stranger", kind: "user", linkedUserId: "users:stranger", deletedAt: undefined, deactivatedAt: undefined }) };
return { unique: async () => ({ _id: "publishers:stranger-personal", handle: "stranger", kind: "user", linkedUserId: "users:stranger", deletedAt: undefined, deactivatedAt: undefined }) };
},
};
}
if (table === "publisherMembers") return { withIndex: () => ({ unique: async () => ({ role: "owner", publisherId: "publishers:stranger-personal", userId: "users:stranger" }) }) };
throw new Error(`unexpected table ${table}`);
}),
patch: vi.fn(async () => {}),
insert: vi.fn(async () => "publisherMembers:1"),
normalizeId: vi.fn(),
};

await expect(
insertVersionHandler(
{ db } as never,
createPublishArgs({ userId: "users:stranger", slug: "shop", ownerPublisherId: "publishers:stranger-personal" }) as never,
),
).rejects.toThrow(/slug is already taken/i);
});

it("updates slug aliases when moving skill to a different publisher", async () => {
const patch = vi.fn(async () => {});
const insert = vi.fn(async (table: string) => {
if (table === "skillVersions") return "skillVersions:1";
if (table === "skillEmbeddings") return "skillEmbeddings:1";
if (table === "embeddingSkillMap") return "embeddingSkillMap:1";
if (table === "skillVersionFingerprints") return "skillVersionFingerprints:1";
throw new Error(`unexpected insert table ${table}`);
});
const existingSkill = {
_id: "skills:1",
slug: "shop",
displayName: "Shop",
summary: "A skill",
ownerUserId: "users:owner",
ownerPublisherId: "publishers:personal",
latestVersionId: undefined,
tags: {},
softDeletedAt: undefined,
badges: { redactionApproved: undefined, highlighted: undefined, official: undefined, deprecated: undefined },
moderationStatus: "active",
moderationReason: "pending.scan",
moderationNotes: undefined,
moderationVerdict: "clean",
moderationReasonCodes: undefined,
moderationEvidence: undefined,
moderationSummary: "Clean",
moderationEngineVersion: "test",
moderationEvaluatedAt: 1,
moderationSourceVersionId: undefined,
quality: undefined,
moderationFlags: undefined,
isSuspicious: false,
reportCount: 0,
lastReportedAt: undefined,
statsDownloads: 0,
statsStars: 0,
statsInstallsCurrent: 0,
statsInstallsAllTime: 0,
stats: { downloads: 0, installsCurrent: 0, installsAllTime: 0, stars: 0, versions: 0, comments: 0 },
createdAt: 1,
updatedAt: 1,
manualOverride: undefined,
};
const db = {
get: vi.fn(async (id: string) => {
if (id === "users:owner") {
return { _id: "users:owner", handle: "pushmatrix", deletedAt: undefined, deactivatedAt: undefined, trustedPublisher: false, role: "user" };
}
if (id === "publishers:personal") return { _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" };
if (id === "publishers:org") return { _id: "publishers:org", kind: "org", deletedAt: undefined, deactivatedAt: undefined };
return null;
}),
query: vi.fn((table: string) => {
if (table === "skills") return { withIndex: () => ({ unique: async () => existingSkill }) };
if (table === "publishers") return { withIndex: () => ({ unique: async () => ({ _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" }) }) };
if (table === "publisherMembers") return { withIndex: () => ({ unique: async () => ({ role: "publisher", publisherId: "publishers:org" }) }) };
if (table === "skillSlugAliases") {
return {
withIndex: (name: string) => {
if (name === "by_skill") return { collect: async () => [{ _id: "skillSlugAliases:1", skillId: "skills:1", ownerPublisherId: "publishers:personal" }] };
return { unique: async () => null };
},
};
}
if (table === "skillVersions") return { withIndex: () => ({ unique: async () => null }) };
if (table === "skillBadges") return { withIndex: () => ({ take: async () => [] }) };
if (table === "skillEmbeddings") return { withIndex: () => ({ unique: async () => null }) };
throw new Error(`unexpected table ${table}`);
}),
patch,
insert,
normalizeId: vi.fn(),
};

const result = await insertVersionHandler(
{ db } as never,
createPublishArgs({ userId: "users:owner", slug: "shop", ownerPublisherId: "publishers:org" }) as never,
);

expect(result).toEqual({ skillId: "skills:1", versionId: "skillVersions:1", embeddingId: "skillEmbeddings:1" });
expect(patch).toHaveBeenCalledWith("skills:1", expect.objectContaining({ ownerPublisherId: "publishers:org" }));
expect(patch).toHaveBeenCalledWith("skillSlugAliases:1", expect.objectContaining({ ownerPublisherId: "publishers:org" }));
});

it("preserves org publisher when owner publishes via legacy path without ownerPublisherId", async () => {
const patch = vi.fn(async () => {});
const insert = vi.fn(async (table: string) => {
if (table === "skillVersions") return "skillVersions:1";
if (table === "skillEmbeddings") return "skillEmbeddings:1";
if (table === "embeddingSkillMap") return "embeddingSkillMap:1";
if (table === "skillVersionFingerprints") return "skillVersionFingerprints:1";
throw new Error(`unexpected insert table ${table}`);
});
const existingSkill = {
_id: "skills:1",
slug: "shop",
displayName: "Shop",
summary: "A skill",
ownerUserId: "users:owner",
ownerPublisherId: "publishers:org",
latestVersionId: undefined,
tags: {},
softDeletedAt: undefined,
badges: { redactionApproved: undefined, highlighted: undefined, official: undefined, deprecated: undefined },
moderationStatus: "active",
moderationReason: "pending.scan",
moderationNotes: undefined,
moderationVerdict: "clean",
moderationReasonCodes: undefined,
moderationEvidence: undefined,
moderationSummary: "Clean",
moderationEngineVersion: "test",
moderationEvaluatedAt: 1,
moderationSourceVersionId: undefined,
quality: undefined,
moderationFlags: undefined,
isSuspicious: false,
reportCount: 0,
lastReportedAt: undefined,
statsDownloads: 0,
statsStars: 0,
statsInstallsCurrent: 0,
statsInstallsAllTime: 0,
stats: { downloads: 0, installsCurrent: 0, installsAllTime: 0, stars: 0, versions: 0, comments: 0 },
createdAt: 1,
updatedAt: 1,
manualOverride: undefined,
};
const db = {
get: vi.fn(async (id: string) => {
if (id === "users:owner") {
return { _id: "users:owner", handle: "pushmatrix", deletedAt: undefined, deactivatedAt: undefined, trustedPublisher: false, role: "user" };
}
if (id === "publishers:personal") return { _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" };
if (id === "publishers:org") return { _id: "publishers:org", kind: "org", deletedAt: undefined, deactivatedAt: undefined };
return null;
}),
query: vi.fn((table: string) => {
if (table === "skills") return { withIndex: () => ({ unique: async () => existingSkill }) };
if (table === "publishers") return { withIndex: () => ({ unique: async () => ({ _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" }) }) };
if (table === "publisherMembers") return { withIndex: () => ({ unique: async () => ({ role: "publisher", publisherId: "publishers:org" }) }) };
if (table === "skillSlugAliases") {
return {
withIndex: (name: string) => name === "by_skill" ? { collect: async () => [] } : { unique: async () => null },
};
}
if (table === "skillVersions") return { withIndex: () => ({ unique: async () => null }) };
if (table === "skillBadges") return { withIndex: () => ({ take: async () => [] }) };
if (table === "skillEmbeddings") return { withIndex: () => ({ unique: async () => null }) };
throw new Error(`unexpected table ${table}`);
}),
patch,
insert,
normalizeId: vi.fn(),
};

// No ownerPublisherId passed — simulates legacy CLI publish
const result = await insertVersionHandler(
{ db } as never,
createPublishArgs({ userId: "users:owner", slug: "shop" }) as never,
);

expect(result).toEqual({ skillId: "skills:1", versionId: "skillVersions:1", embeddingId: "skillEmbeddings:1" });
// ownerPublisherId should stay as publishers:org, never changed to publishers:personal
// ownerPublisherId in every skill patch should be the org, never personal
for (const call of patch.mock.calls) {
const [id, fields] = call as never as [string, Record<string, unknown>];
if (id === "skills:1" && "ownerPublisherId" in fields) {
expect(fields.ownerPublisherId).toBe("publishers:org");
}
}
});
});
37 changes: 31 additions & 6 deletions convex/skills.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6169,7 +6169,7 @@ export const insertVersion = internalMutation({
if (!user || user.deletedAt || user.deactivatedAt) throw new Error("User not found");
const personalPublisher = await ensurePersonalPublisherForUser(ctx, user);
if (!personalPublisher) throw new ConvexError("Personal publisher not found");
const ownerPublisherId = args.ownerPublisherId ?? personalPublisher._id;
let ownerPublisherId = args.ownerPublisherId ?? personalPublisher._id;
if (ownerPublisherId !== personalPublisher._id) {
await requirePublisherRole(ctx, {
publisherId: ownerPublisherId,
Expand Down Expand Up @@ -6204,11 +6204,36 @@ export const insertVersion = internalMutation({
}

if (skill && skill.ownerPublisherId && skill.ownerPublisherId !== ownerPublisherId) {
const owner = await getOwnerPublisher(ctx, {
ownerPublisherId: skill.ownerPublisherId,
ownerUserId: skill.ownerUserId,
});
throw new ConvexError(buildSlugTakenErrorMessage(skill, owner));
// Only move the skill when the caller explicitly requested a different publisher.
// When ownerPublisherId was defaulted from the personal publisher (legacy CLI paths),
// preserve the existing publisher to avoid silently moving org skills to personal.
const explicitPublisherRequested = Boolean(args.ownerPublisherId);
if (skill.ownerUserId === userId && explicitPublisherRequested) {
await ctx.db.patch(skill._id, { ownerPublisherId, updatedAt: now });
Comment on lines +6210 to +6212
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require explicit move signal before changing owner publisher

This branch assumes args.ownerPublisherId means the caller explicitly asked to transfer ownership, but the normal web publish path always supplies an ownerPublisherId (via publishVersion target resolution) and the publish form defaults ownerHandle to the personal publisher for updates. In that flow, republishing an org-owned skill can now hit this branch and patch ownerPublisherId to personal even when the user only intended to publish a new version, creating an unintended ownership transfer regression from the previous slug-taken guard.

Useful? React with 👍 / 👎.

Comment on lines +6210 to +6212
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require explicit transfer signal before patching ownerPublisherId

Using Boolean(args.ownerPublisherId) as the transfer signal is unsafe because this field is auto-populated in normal publish flows rather than being a true user-intent flag. publishVersion always passes target.publisherId, and the web update flow auto-fills owner handle to the personal publisher (src/routes/publish-skill.tsx:204-210, reached from src/routes/dashboard.tsx:328), so republishing an org-owned skill can hit this branch and silently move ownership to personal via ctx.db.patch(skill._id, ...). Fresh evidence is that the new branch now performs ownership moves on that defaulted path, making this an introduced behavior regression rather than a theoretical caller misuse.

Useful? React with 👍 / 👎.

const aliases = await ctx.db
.query("skillSlugAliases")
.withIndex("by_skill", (q) => q.eq("skillId", skill!._id))
.collect();
for (const alias of aliases) {
await ctx.db.patch(alias._id, { ownerPublisherId, updatedAt: now });
}
skill = { ...skill, ownerPublisherId };
} else if (skill.ownerUserId === userId && !explicitPublisherRequested) {
// Owner publishing via a legacy path that doesn't pass ownerPublisherId.
// Keep the existing publisher, but verify the caller still has access.
await requirePublisherRole(ctx, {
publisherId: skill.ownerPublisherId!,
userId,
allowed: ["publisher"],
});
ownerPublisherId = skill.ownerPublisherId;
Comment on lines +6221 to +6229
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Re-validate org access before preserving ownerPublisherId

In the legacy-path branch (skill.ownerUserId === userId with no explicit ownerPublisherId), this code rewrites ownerPublisherId to skill.ownerPublisherId after the only authorization check has already run against the caller’s personal publisher. That means legacy publish endpoints that call publishVersionForUser without an ownerPublisherId (e.g. convex/httpApi.ts and convex/httpApiV1/skillsV1.ts) now allow publishing under an org-owned skill even when the caller no longer has org membership (or the org publisher is inactive), because no requirePublisherRole check is performed for the preserved org publisher.

Useful? React with 👍 / 👎.

} else {
const owner = await getOwnerPublisher(ctx, {
ownerPublisherId: skill.ownerPublisherId,
ownerUserId: skill.ownerUserId,
});
throw new ConvexError(buildSlugTakenErrorMessage(skill, owner));
}
}

if (skill && !skill.ownerPublisherId && skill.ownerUserId !== userId) {
Expand Down