fix: allow skill owner to move skill to an org acct while publishing existing skill#1562
fix: allow skill owner to move skill to an org acct while publishing existing skill#1562pushmatrix wants to merge 2 commits intoopenclaw:mainfrom
Conversation
When a skill already has an ownerPublisherId, the insertVersion mutation treated any publisher mismatch as a slug conflict — even when the caller is the skill's ownerUserId. This made it impossible to move your own skill from a personal publisher to an org publisher (or vice versa). Now, when ownerPublisherId differs but the caller is the ownerUserId, we update the skill's publisher instead of throwing. Slug aliases are also updated to stay in sync, matching the pattern used by the transfer flow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@pushmatrix is attempting to deploy a commit to the 0xBuns Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThe production change in All three new tests, however, share a mock gap that causes them to fail in CI before the new code path is reached. Confidence Score: 4/5Production logic is correct, but all three new tests fail before exercising the new code path — the feature is effectively untested. Two P1 findings: the positive tests never call convex/skills.rateLimit.test.ts — all three new tests need Prompt To Fix All With AIThis 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.
---
This is a comment left during a code review.
Path: convex/skills.rateLimit.test.ts
Line: 1440-1495
Comment:
**Wrong error asserted — test fails for the wrong reason**
The test expects `rejects.toThrow(/slug is already taken/i)`, but execution never reaches the slug-conflict check. Because `ownerPublisherId: "publishers:strangerOrg"` ≠ `personalPublisher._id` (`"publishers:personal"`), `requirePublisherRole` is called. `ctx.db.get("publishers:strangerOrg")` returns `null` (not in the mock), so the function throws `ConvexError("Publisher not found")` — not "slug is already taken".
For this test to validate that a non-owner is blocked at the slug level, the mock needs `"publishers:strangerOrg"` to resolve as a valid publisher, and the stranger must appear as a member. Without those, the test only confirms that publishing to a non-existent publisher is blocked, which is already covered elsewhere.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: allow skill owner to move skill to ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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; |
There was a problem hiding this 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:
| 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.| ); | ||
| }); | ||
|
|
||
| it("blocks non-owner from claiming a skill with a different publisher", async () => { | ||
| const db = { | ||
| get: vi.fn(async (id: string) => { | ||
| if (id === "users:stranger") { | ||
| return { _id: "users:stranger", handle: "stranger", deletedAt: undefined, deactivatedAt: undefined }; | ||
| } | ||
| if (id === "users:owner") { | ||
| return { _id: "users:owner", handle: "alice", deletedAt: undefined, deactivatedAt: undefined }; | ||
| } | ||
| if (id === "publishers:personal") { | ||
| return { _id: "publishers:personal", kind: "user", linkedUserId: "users:stranger" }; | ||
| } | ||
| return null; | ||
| }), | ||
| query: vi.fn((table: string) => { | ||
| if (table === "skills") { | ||
| return { | ||
| withIndex: (name: string) => { | ||
| if (name !== "by_slug") throw new Error(`unexpected skills index ${name}`); | ||
| return { | ||
| unique: async () => ({ | ||
| _id: "skills:1", | ||
| slug: "shop", | ||
| ownerUserId: "users:owner", | ||
| ownerPublisherId: "publishers:other", | ||
| softDeletedAt: undefined, | ||
| moderationStatus: "active", | ||
| moderationFlags: undefined, | ||
| }), | ||
| }; | ||
| }, | ||
| }; | ||
| } | ||
| if (table === "publishers") { | ||
| return { | ||
| withIndex: () => ({ unique: async () => ({ _id: "publishers:personal", kind: "user", linkedUserId: "users:stranger" }) }), | ||
| }; | ||
| } | ||
| throw new Error(`unexpected table ${table}`); | ||
| }), | ||
| normalizeId: vi.fn(), | ||
| }; | ||
|
|
||
| await expect( | ||
| insertVersionHandler( | ||
| { db } as never, | ||
| createPublishArgs({ | ||
| userId: "users:stranger", | ||
| slug: "shop", | ||
| ownerPublisherId: "publishers:strangerOrg", | ||
| }) as never, | ||
| ), | ||
| ).rejects.toThrow(/slug is already taken/i); |
There was a problem hiding this comment.
The test expects rejects.toThrow(/slug is already taken/i), but execution never reaches the slug-conflict check. Because ownerPublisherId: "publishers:strangerOrg" ≠ personalPublisher._id ("publishers:personal"), requirePublisherRole is called. ctx.db.get("publishers:strangerOrg") returns null (not in the mock), so the function throws ConvexError("Publisher not found") — not "slug is already taken".
For this test to validate that a non-owner is blocked at the slug level, the mock needs "publishers:strangerOrg" to resolve as a valid publisher, and the stranger must appear as a member. Without those, the test only confirms that publishing to a non-existent publisher is blocked, which is already covered elsewhere.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skills.rateLimit.test.ts
Line: 1440-1495
Comment:
**Wrong error asserted — test fails for the wrong reason**
The test expects `rejects.toThrow(/slug is already taken/i)`, but execution never reaches the slug-conflict check. Because `ownerPublisherId: "publishers:strangerOrg"` ≠ `personalPublisher._id` (`"publishers:personal"`), `requirePublisherRole` is called. `ctx.db.get("publishers:strangerOrg")` returns `null` (not in the mock), so the function throws `ConvexError("Publisher not found")` — not "slug is already taken".
For this test to validate that a non-owner is blocked at the slug level, the mock needs `"publishers:strangerOrg"` to resolve as a valid publisher, and the stranger must appear as a member. Without those, the test only confirms that publishing to a non-existent publisher is blocked, which is already covered elsewhere.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 895f811bcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (id === "publishers:personal") { | ||
| return { _id: "publishers:personal", kind: "user", linkedUserId: "users:owner" }; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
These new tests pass ownerPublisherId values like publishers:org, but the db.get mock only returns publishers:personal and otherwise returns null. In insertVersion, that causes requirePublisherRole to fail with Publisher not found before the owner/non-owner slug logic runs, so the tests do not actually exercise the behavior they claim to validate and are likely to fail for the wrong reason.
Useful? React with 👍 / 👎.
The db.get mock was missing the org publisher entry, so requirePublisherRole would throw "Publisher not found" before reaching the code under test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pushmatrix
changed the title
pushmatrix
changed the title
|
This is useful for me! Thank you! |
|
Please note that CI / build (pull_request)Failing after 1m. All test cases must be successful before merge. @pushmatrix |
3 participants
Fixes: #1431
Summary
insertVersionincorrectly threw "Slug is already taken" even when the caller owns the skillconvex/skills.ts:5820treated anyownerPublisherIdmismatch as a conflict with no escape hatch for the actual ownerownerUserId, the skill's publisher is updated instead of throwingTest plan
🤖 Generated with Claude Code