feat: add package ownership transfers#1604
feat: add package ownership transfers#1604TommYDeeee wants to merge 13 commits intoopenclaw:mainfrom
Conversation
|
@TommYDeeee is attempting to deploy a commit to the 0xBuns Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds a full package ownership transfer lifecycle (request → accept / reject / cancel) mirroring the skill transfer system from #1603, with 7-day expiry, one-pending-at-a-time enforcement, ownership re-validation at accept time, and org-admin fallback for all decision endpoints. The implementation is solid and consistent with its skill-transfer counterpart. Confidence Score: 5/5Safe to merge — all findings are P2 style/convention issues that do not affect correctness or security. Both findings are P2: a convex/packageTransfers.ts (filter queries) and convex/httpApiV1/packagesV1.ts (error status mapping) have minor style concerns, but neither blocks merge.
|
| export const getPendingTransferByPackageAndUserInternal = internalQuery({ | ||
| args: { | ||
| packageId: v.id("packages"), | ||
| toUserId: v.id("users"), | ||
| }, | ||
| handler: async (ctx, args) => { | ||
| const now = Date.now(); | ||
| const transfer = await ctx.db | ||
| .query("packageOwnershipTransfers") | ||
| .withIndex("by_package_status", (q) => | ||
| q.eq("packageId", args.packageId).eq("status", "pending"), | ||
| ) | ||
| .filter((q) => q.eq(q.field("toUserId"), args.toUserId)) | ||
| .first(); | ||
|
|
||
| if (!transfer || isTransferExpired(transfer, now)) return null; | ||
| return transfer; | ||
| }, | ||
| }); | ||
|
|
||
| export const getPendingTransferByPackageAndFromUserInternal = internalQuery({ | ||
| args: { | ||
| packageId: v.id("packages"), | ||
| fromUserId: v.id("users"), | ||
| }, | ||
| handler: async (ctx, args) => { | ||
| const now = Date.now(); | ||
| const transfer = await ctx.db | ||
| .query("packageOwnershipTransfers") | ||
| .withIndex("by_package_status", (q) => | ||
| q.eq("packageId", args.packageId).eq("status", "pending"), | ||
| ) | ||
| .filter((q) => q.eq(q.field("fromUserId"), args.fromUserId)) | ||
| .first(); | ||
|
|
||
| if (!transfer || isTransferExpired(transfer, now)) return null; | ||
| return transfer; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
.filter() after index violates AGENTS.md query rule
Both getPendingTransferByPackageAndUserInternal and getPendingTransferByPackageAndFromUserInternal use by_package_status then chain .filter() to narrow by toUserId/fromUserId. AGENTS.md states: "Always use .withIndex() instead of .filter() for fields that can be indexed." In practice the scan is trivially small (at most one pending transfer per package), but adding compound indexes would align with the project rule:
// in schema.ts
.index("by_package_status_to_user", ["packageId", "status", "toUserId"])
.index("by_package_status_from_user", ["packageId", "status", "fromUserId"])Then the queries become:
// getPendingTransferByPackageAndUserInternal
.withIndex("by_package_status_to_user", (q) =>
q.eq("packageId", args.packageId).eq("status", "pending").eq("toUserId", args.toUserId)
)
.first()
// getPendingTransferByPackageAndFromUserInternal
.withIndex("by_package_status_from_user", (q) =>
q.eq("packageId", args.packageId).eq("status", "pending").eq("fromUserId", args.fromUserId)
)
.first()Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/packageTransfers.ts
Line: 472-510
Comment:
**`.filter()` after index violates AGENTS.md query rule**
Both `getPendingTransferByPackageAndUserInternal` and `getPendingTransferByPackageAndFromUserInternal` use `by_package_status` then chain `.filter()` to narrow by `toUserId`/`fromUserId`. AGENTS.md states: "Always use `.withIndex()` instead of `.filter()` for fields that can be indexed." In practice the scan is trivially small (at most one pending transfer per package), but adding compound indexes would align with the project rule:
```ts
// in schema.ts
.index("by_package_status_to_user", ["packageId", "status", "toUserId"])
.index("by_package_status_from_user", ["packageId", "status", "fromUserId"])
```
Then the queries become:
```ts
// getPendingTransferByPackageAndUserInternal
.withIndex("by_package_status_to_user", (q) =>
q.eq("packageId", args.packageId).eq("status", "pending").eq("toUserId", args.toUserId)
)
.first()
// getPendingTransferByPackageAndFromUserInternal
.withIndex("by_package_status_from_user", (q) =>
q.eq("packageId", args.packageId).eq("status", "pending").eq("fromUserId", args.fromUserId)
)
.first()
```
**Context Used:** AGENTS.md ([source](https://app.greptile.com/review/custom-context?memory=a1d58d20-b4dd-4cbb-973a-9fd7824e1921))
How can I resolve this? If you propose a fix, please make it concise.| function packageTransferErrorToResponse(error: unknown, headers: HeadersInit) { | ||
| const message = error instanceof Error ? error.message : "Transfer failed"; | ||
| const lower = message.toLowerCase(); | ||
| if (lower.includes("unauthorized")) return text("Unauthorized", 401, headers); | ||
| if (lower.includes("forbidden")) return text("Forbidden", 403, headers); | ||
| if (lower.includes("not found")) return text(message, 404, headers); | ||
| if (lower.includes("required") || lower.includes("invalid") || lower.includes("pending")) { | ||
| return text(message, 400, headers); | ||
| } | ||
| return text(message, 400, headers); | ||
| } |
There was a problem hiding this comment.
Stale-transfer error returns 400 instead of 409
acceptTransferInternal throws "Transfer is no longer valid" when package ownership changed since the transfer was requested. That string doesn't match any keyword in packageTransferErrorToResponse, so it falls through to the default 400. This is misleading — the client's request was well-formed; it's the resource state that changed. A 409 Conflict would be more semantically correct and help callers distinguish "bad input" from "concurrent modification".
| function packageTransferErrorToResponse(error: unknown, headers: HeadersInit) { | |
| const message = error instanceof Error ? error.message : "Transfer failed"; | |
| const lower = message.toLowerCase(); | |
| if (lower.includes("unauthorized")) return text("Unauthorized", 401, headers); | |
| if (lower.includes("forbidden")) return text("Forbidden", 403, headers); | |
| if (lower.includes("not found")) return text(message, 404, headers); | |
| if (lower.includes("required") || lower.includes("invalid") || lower.includes("pending")) { | |
| return text(message, 400, headers); | |
| } | |
| return text(message, 400, headers); | |
| } | |
| function packageTransferErrorToResponse(error: unknown, headers: HeadersInit) { | |
| const message = error instanceof Error ? error.message : "Transfer failed"; | |
| const lower = message.toLowerCase(); | |
| if (lower.includes("unauthorized")) return text("Unauthorized", 401, headers); | |
| if (lower.includes("forbidden")) return text("Forbidden", 403, headers); | |
| if (lower.includes("not found")) return text(message, 404, headers); | |
| if (lower.includes("no longer valid") || lower.includes("expired")) { | |
| return text(message, 409, headers); | |
| } | |
| if (lower.includes("required") || lower.includes("invalid") || lower.includes("pending")) { | |
| return text(message, 400, headers); | |
| } | |
| return text(message, 400, headers); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/httpApiV1/packagesV1.ts
Line: 1469-1479
Comment:
**Stale-transfer error returns 400 instead of 409**
`acceptTransferInternal` throws `"Transfer is no longer valid"` when package ownership changed since the transfer was requested. That string doesn't match any keyword in `packageTransferErrorToResponse`, so it falls through to the default 400. This is misleading — the client's request was well-formed; it's the resource state that changed. A 409 Conflict would be more semantically correct and help callers distinguish "bad input" from "concurrent modification".
```suggestion
function packageTransferErrorToResponse(error: unknown, headers: HeadersInit) {
const message = error instanceof Error ? error.message : "Transfer failed";
const lower = message.toLowerCase();
if (lower.includes("unauthorized")) return text("Unauthorized", 401, headers);
if (lower.includes("forbidden")) return text("Forbidden", 403, headers);
if (lower.includes("not found")) return text(message, 404, headers);
if (lower.includes("no longer valid") || lower.includes("expired")) {
return text(message, 409, headers);
}
if (lower.includes("required") || lower.includes("invalid") || lower.includes("pending")) {
return text(message, 400, headers);
}
return text(message, 400, headers);
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e19aae2ce9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path: `${ApiRoutes.skills}/${encodeURIComponent(slug)}/transfer/${spec.action}`, | ||
| path: `${resolveApiPath(name, itemType)}/transfer/${spec.action}`, | ||
| token, | ||
| ...(body ? { body: JSON.stringify(body) } : {}), |
There was a problem hiding this comment.
Send transfer accept payload as object, not pre-stringified JSON
When transfer accept is run with --publisher, this code stringifies the body before calling apiRequest, but apiRequest already does JSON.stringify(args.body) internally. That double-encodes the payload into a JSON string literal, so the server reads a string instead of an object and never sees publisherHandle; the accept call still succeeds, but ownership is assigned to the default publisher (typically personal) instead of the requested org.
Useful? React with 👍 / 👎.
498c370 to
04df985
Compare
|
Review comments here are noted — thanks for the thorough feedback. Since this PR is stacked on #1603, I'll address these after PR 1 is merged and this branch is rebased as there might be changes in the first #1603 and I want to avoid duplicating everything:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04df985fb0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
convex/httpApiV1/packagesV1.ts
Outdated
| const hasBody = contentType.includes("json"); | ||
| const parsed = hasBody ? await parseJsonPayload(request, headers) : null; | ||
| if (parsed && !parsed.ok) return parsed.response; |
There was a problem hiding this comment.
Treat empty JSON accept payload as optional
When decision === "accept", this path parses JSON whenever the Content-Type header contains json. A client that sends POST .../transfer/accept with Content-Type: application/json but no body (common for optional-body endpoints) will make request.json() throw and get 400 Invalid JSON, even though publisherHandle is optional and the request should succeed. This is a behavioral regression for empty-body accept calls; parse only when a body is actually present (and apply the same fix to the analogous branch in skillsV1.ts).
Useful? React with 👍 / 👎.
04df985 to
88045f5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88045f54ed
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const body = await r.json(); | ||
| return Boolean(body?.package); | ||
| } catch { |
There was a problem hiding this comment.
Distinguish skill fallback from real package in auto-detection
resolveItemType treats any successful /api/v1/packages/{name} response with a package field as a real package, but that endpoint also returns skill fallbacks with a top-level package object. As a result, a normal skill name makes both skillRes and pkgRes true, and the CLI incorrectly aborts with the "Found both a skill and package" error unless users add --type skill, which regresses the default transfer workflow.
Useful? React with 👍 / 👎.
| if (!pendingTransfer) { | ||
| pendingTransfer = await runQueryRef<PendingTransferLike | null>( | ||
| ctx, | ||
| internalRefs.packageTransfers.getPendingTransferByPackageInternal, | ||
| { packageId: transferContext.pkg._id }, |
There was a problem hiding this comment.
Keep unauthorized transfer decisions indistinguishable from not found
This fallback fetches any pending package transfer before checking whether the caller is an allowed recipient. On personal transfers, non-recipient users can now hit the mutation path and receive a 400 No pending transfer found (via error mapping) instead of the 404 returned when no transfer exists, which changes API semantics and leaks whether a pending transfer exists for that package. Restrict this fallback to org-targeted transfers or map that specific error back to 404.
Useful? React with 👍 / 👎.
88045f5 to
13f31a5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13f31a5b42
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pendingTransfer = await ctx.runQuery( | ||
| internal.skillTransfers.getPendingTransferBySkillInternal, | ||
| { skillId: transferContext.skill._id }, |
There was a problem hiding this comment.
Keep unauthorized skill decisions indistinguishable from missing transfer
This fallback pulls any pending transfer for the skill even when the caller is not the intended recipient. On personal transfers, a non-recipient can now hit the mutation path and receive 400 No pending transfer found (via transferErrorToResponse) instead of the prior 404 when no transfer exists, which leaks whether a pending transfer exists for that skill. Restrict the fallback to org-targeted transfers (or map that specific error back to 404) so unauthorized callers cannot distinguish existence.
Useful? React with 👍 / 👎.
13f31a5 to
87cf19f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87cf19f875
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const parsed = await parseJsonPayload(request, headers); | ||
| if (parsed.ok) { |
There was a problem hiding this comment.
Reject invalid JSON in package transfer accept requests
The accept branch silently ignores JSON parse failures because it only handles the parsed.ok path. If a client sends malformed JSON while trying to set publisherHandle, the transfer still succeeds and falls back to the default ownership target, which can assign the package to the wrong publisher without surfacing an error. This should return parsed.response when payload parsing fails.
Useful? React with 👍 / 👎.
| fail( | ||
| `No skill or package named "${name}" found. If the item is private, use --type skill or --type package to skip auto-detection.`, | ||
| ); |
There was a problem hiding this comment.
Propagate probe auth/server errors in transfer type detection
Auto-detection currently treats any non-2xx probe result as "not found" and falls through to this message, so 401/403/429/5xx responses are misreported as missing resources. In practice, an expired token or transient registry failure becomes a misleading "No skill or package named" error instead of surfacing the real failure mode. Only 404-style misses should map to this branch; other statuses should be reported explicitly.
Useful? React with 👍 / 👎.
87cf19f to
5729f12
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5729f12b9b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const parsed = await parseJsonPayload(request, headers); | ||
| if (parsed.ok) { | ||
| const publisherHandleRaw = |
There was a problem hiding this comment.
Reject malformed JSON in skill transfer accept requests
In the accept branch, JSON parse failures are silently ignored because only the parsed.ok path is handled. If a client sends malformed JSON while trying to provide publisherHandle, the transfer still succeeds and defaults to the non-override ownership path instead of returning a client error, which can move a skill to the wrong publisher without warning. Return parsed.response when parsing fails.
Useful? React with 👍 / 👎.
5729f12 to
02cfa90
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02cfa90d98
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (args.toPublisherId) { | ||
| const toPublisher = await ctx.db.get(args.toPublisherId); | ||
| if (!toPublisher || (toPublisher as Record<string, unknown>).deletedAt || (toPublisher as Record<string, unknown>).deactivatedAt) { | ||
| throw new Error("Target publisher not found"); | ||
| } |
There was a problem hiding this comment.
Enforce target-org admin eligibility for chosen recipient
When toPublisherId is supplied, this branch only checks that the publisher exists, so transfer requests can target a user who is not an admin/owner of that org. The request succeeds, but later acceptTransferInternal rejects that same recipient via validateTransferAcceptPermission, yielding an effectively non-actionable transfer for the intended recipient. Validate recipient membership/role at request time (or reject non-admin recipients) to avoid creating transfers that the designated recipient can never accept.
Useful? React with 👍 / 👎.
| if (args.toPublisherId) { | ||
| const toPublisher = await ctx.db.get(args.toPublisherId); | ||
| if (!toPublisher || toPublisher.deletedAt || toPublisher.deactivatedAt) { | ||
| throw new Error("Target publisher not found"); | ||
| } |
There was a problem hiding this comment.
Validate org recipient role before creating skill transfer
For org-targeted skill transfers, this code only verifies that the target publisher is active; it does not verify that toUserHandle has admin/owner rights on that publisher. As a result, the request can be created for a recipient who later cannot accept it because validateTransferAcceptPermission requires admin-level membership, which breaks the expected request→accept flow for that recipient.
Useful? React with 👍 / 👎.
Add fromPublisherId/toPublisherId fields to skillOwnershipTransfers schema and make toUserId optional to support org-to-org, user-to-org, and org-to-user transfer flows. - Extract shared transfer helpers into convex/lib/transfers.ts - Update skillTransfers.ts with org-aware request/accept/reject/cancel - Add publisher handle resolution and org admin fallback in skillsV1.ts - Add type:"skill" field to transfer listing responses in transfersV1.ts - Update CLI with --type and --publisher flags for transfer commands - Update response schemas, docs (http-api, cli, orgs)
- Add clarifying comment that ownerUserId reflects the accepting admin, not the original toUserId, for org-targeted transfers - Remove redundant .map() type remapping in transfer listing — the query already returns type: "skill" - Fix double JSON encoding in CLI transfer accept body
Remove package auto-detection, --type flag, and package-specific response schema fields. PR 1 only adds skill org support — package transfer CLI support belongs in PR 2 alongside the backend endpoints.
…JSON The existing test expected body as a JSON string, but apiRequest already calls JSON.stringify internally — passing a pre-stringified body causes double-encoding. Updated test to verify the body object directly, matching the corrected CLI behavior.
…n cancel - Accept handler no longer rejects empty JSON bodies. If parsing fails (empty body, no content-type), publisherId stays undefined and the transfer proceeds with the default publisher. Fixes clients that send Content-Type: application/json with no body. - Cancel always validates org membership when fromPublisherId is set, even for the original requester. Prevents cancelled-after-removal where a user who lost org admin role could still cancel.
requestTransferInternal now checks that toPublisherId is not deleted/deactivated before persisting the transfer. Prevents creating dead transfers that can never be accepted.
When the sender specified a target publisher (toPublisherId), the acceptor's publisherId arg is now ignored. The publisher override only applies to user-targeted transfers where the recipient chooses where to assign ownership.
Personal publishers (kind: "user") now require direct ownerUserId match, same as the original pre-PR check. Only org publishers (kind: "org") use membership-based authorization. Prevents a theoretical privilege escalation where a member added to a personal publisher could request transfers for skills they don't own.
validateTransferAcceptPermission now checks publisher.kind and rejects personal publishers (kind: "user") as transfer targets, consistent with the validateTransferOwnership fix. Prevents accepting transfers into a personal publisher via membership.
requestTransferInternal now rejects toPublisherId with kind: "user", preventing creation of dead transfers that can never be accepted.
02cfa90 to
45d9027
Compare
45d9027 to
e8e596c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8e596c316
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const transfers = await ctx.db | ||
| .query("packageOwnershipTransfers") | ||
| .withIndex("by_to_user_status", (q) => q.eq("toUserId", args.userId).eq("status", "pending")) | ||
| .collect(); |
There was a problem hiding this comment.
Include org-targeted package transfers in incoming list
listIncomingInternal only loads transfers via by_to_user_status, so a package transfer sent to an org (toPublisherId set) is invisible to other admins of that org unless they are the specific toUserId. This diverges from the skill transfer behavior and from the accept/reject permission model (which allows any org admin), and it makes pending org package transfers effectively undiscoverable in GET /api/v1/transfers/incoming / clawhub transfer list for those admins.
Useful? React with 👍 / 👎.
Summary
Adds the ability to transfer ownership of packages between users and organizations, completing the ownership transfer system alongside the skill org support in #1603.
This is PR 2 of 2, chained on top of #1603 (skill transfer org support). Please merge #1603 first — this PR includes those changes plus the package-specific additions. The incremental diff (what this PR adds beyond #1603) is 8 files / +1229 lines, focused purely on the package transfer system.
This separation keeps the review surface manageable: #1603 introduces the shared infrastructure and extends skills, while this PR applies those same patterns to packages as a clean additive layer.
Resolves #1431
Motivation
Once a package was published under a personal account, there was no way to reassign it to an organization. Users who published packages under their personal handle and later wanted to move them under their org were stuck. This PR introduces package ownership transfers mirroring the skill transfer system, supporting user→user, user→org, org→user, and org→org flows.
What's new (beyond #1603)
Package transfers backend (
convex/packageTransfers.ts)fromPublisherIdfor org transfers to correctly handle org admin vsownerUserIdmismatchpublisherIdoverride on accept validated against org membershippackage.transfer.request/accept/reject/cancel)Schema changes
packageOwnershipTransferstable mirroringskillOwnershipTransfersshape with indexes:by_package,by_from_user,by_to_user,by_to_user_status,by_from_user_status,by_package_statusHTTP endpoints
/api/v1/packages/{name}/transfertoUserHandle, optionaltoPublisherHandle,message)/api/v1/packages/{name}/transfer/acceptpublisherHandleto assign to org)/api/v1/packages/{name}/transfer/reject/api/v1/packages/{name}/transfer/cancelUnified transfer listing
GET /api/v1/transfers/incomingandGET /api/v1/transfers/outgoingnow return both skill and package transferstype: "skill" | "package"discriminator, sorted byrequestedAtdescendingTransfer rules
adminorownerrole on the source/target publisherIncremental files changed (on top of #1603: 8 files, +1229 / -67)
convex/packageTransfers.tsconvex/packageTransfers.test.tsconvex/schema.tspackageOwnershipTransferstableconvex/httpApiV1/packagesV1.tsconvex/httpApiV1/transfersV1.tsconvex/httpApiV1.handlers.test.tsdocs/http-api.mddocs/api.mdTest plan