feat: add org support to skill ownership transfers#1603
feat: add org support to skill ownership transfers#1603TommYDeeee wants to merge 12 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 extends skill ownership transfers to support org-targeted flows (user→org, org→user, org→org) by extracting shared helpers into
Confidence Score: 4/5Safe to merge with one behavioral regression in the outgoing transfer list to address first. One clear P1 regression:
|
![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: f4d7528980
ℹ️ 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".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
|
Thanks for the contribution! Just a quick note: there's a failing test case that needs to be fixed. That would be the last step before merge. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 475809be79
ℹ️ 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".
|
Thanks for the quick response! Really appreciate it. @TommYDeeee There are 2 more new review comments from codex, please take a look. |
Thanks! Both of them were minor but valid, addressed in latest commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7246c855d5
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d829719c0
ℹ️ 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".
| const parsed = await parseJsonPayload(request, headers); | ||
| if (parsed.ok) { | ||
| const publisherHandleRaw = |
There was a problem hiding this comment.
Return a 400 when the accept request body is invalid JSON instead of silently continuing. In this branch, parse failures are ignored (if (parsed.ok)), so a malformed payload intended to set publisherHandle still proceeds with acceptance and falls back to default ownership assignment, which can complete the transfer with the wrong destination publisher.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I would push this back. It is intentional behavior. The publisherHandle field is optional — an empty or missing body means "use the default publisher." Rejecting parse failures would break clients that send Content-Type: application/json with no body (the exact scenario flagged in the earlier review comment). The current pattern is standard for optional request bodies: parse if possible, default if not.
There was a problem hiding this comment.
The pushback is reasonable. Parse failures are not silently ignored — the if (parsed && !parsed.ok) return parsed.response guard already returns an error response when Content-Type: application/json is declared but the body is malformed JSON. The "silent continue" path only applies when no Content-Type: application/json header is present, in which case parsed is null and the body is intentionally skipped. That's the correct behavior for an optional field: don't attempt JSON parsing if the client didn't declare a JSON body.
There was a problem hiding this comment.
Thanks for the feedback, just to be sure is there anything else needed?
There was a problem hiding this comment.
Made a little adjustment on listIncomingInternal
Before: only queried transfers where toUserId === userId.
After: also queries transfers directed at orgs where the user is an owner or admin, then merges + deduplicates the two result sets. Also adds toPublisherId to the returned shape so the caller knows whether the transfer is org-targeted.
There was a problem hiding this comment.
Thanks for adding this, I like it. Good call 👍
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.
momothemage
force-pushed
the
feat/skill-transfer-org-support
branch
from
c112eb9 to
edb702b
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edb702bb57
ℹ️ 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 (!params.ownerPublisherId) { | ||
| // Personally owned — actor must be the owner | ||
| if (params.actorUserId !== params.ownerUserId) { | ||
| throw new Error("Forbidden"); | ||
| } |
There was a problem hiding this comment.
validateTransferOwnership only uses ownerUserId when ownerPublisherId is absent, but skills are commonly stored with a personal ownerPublisherId, so this branch treats personal ownership like org ownership. Because publishers.addMember allows adding admins/owners without a publisher-kind guard, a collaborator on a personal publisher can now request/cancel transfers for skills they do not own, which is a privilege escalation compared to the previous skill.ownerUserId === actorUserId check. The ownership check should key off publisher kind (or still require owner user for user-kind publishers), not just presence of ownerPublisherId.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid observation. We replaced the strict ownerUserId === actorUserId check with membership-based validation, which doesn't distinguish between personal publishers (kind: "user") and org publishers (kind: "org"). In theory, if someone was added as admin to a personal publisher via addMember (which has no kind guard), they could request transfers for skills they don't own. In practice, personal publishers are auto-created with only the owner as member and there's no UI flow to add members to them. Happy to add a publisher kind check here if you'd prefer, it's a small change. However, this could be addressed at the addMember level as a broader and cleaner fix separately.
There was a problem hiding this comment.
@TommYDeeee This comment sounds reasonable, lets see if there is maybe a privilege misescalation
There was a problem hiding this comment.
Like if I understand it correctly. The possible attack vector is:
- someone would need to signup and create a personal publisher
- the same person as an admin would need to add a user (via API as there is no UI for that) and give them admin privileges
- only after this the admin/owner person can do the transfer of a plugin.
But that would be self-sabotage from the very beginning, it can be easily addressed on a level of the API for transfers here or at the addMember level, but the use-case is very strange neverthless
There was a problem hiding this comment.
In this case the owner didn't intend to grant transfer authority. The addMember call is the entry point, but the impact lands in the transfer flow. I think it would be better to add a check in validateTransferOwnership to avoid this threat.
There was a problem hiding this comment.
Understood. Fixed in 9964342. validateTransferOwnership now checks publisher.kind — personal publishers (kind: "user") require direct ownerUserId match (same security level as before this PR), org publishers (kind: "org") use membership-based authorization.
Added also two tests covering the personal publisher path: owner can transfer their own skill, admin member on personal publisher is rejected.
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.
TommYDeeee
force-pushed
the
feat/skill-transfer-org-support
branch
from
da680f5 to
9964342
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 996434288d
ℹ️ 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".
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25a09399f5
ℹ️ 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".
requestTransferInternal now rejects toPublisherId with kind: "user", preventing creation of dead transfers that can never be accepted.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bff5a2595b
ℹ️ 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".
|
@momothemage Is this codex configured to always find something ahah :D like I can't beat it. I would say right now everything we have discussed or was pointed out is resolved. |
2 participants
Summary
Extends the existing skill transfer system to support org-targeted transfers (user→org, org→user, org→org), in addition to the existing user→user flow.
This is PR 1 of 2 — it lays the foundation with shared transfer helpers and org-aware skill transfers. #1604 (package ownership transfers) builds on top of this and is chained to this branch.
The split keeps each PR focused and independently reviewable: this PR touches only the skill transfer path and shared infrastructure, while PR 2 adds the new package transfer system as a clean incremental layer.
Resolves part of #1431
Motivation
Skill transfers were previously user-to-user only. Users who wanted to move a skill to an organization they admin had no way to do so after publishing. This PR enables publisher-based transfer flows so org admins can transfer skills to and from organizations.
What's new
Shared transfer helpers (
convex/lib/transfers.ts)validateTransferOwnership— supports both personal ownership (ownerUserIdmatch) and org admin/owner role checks viapublisherMembersvalidateTransferAcceptPermission— validates accept permission for personal and org targets, rejects transfers to inactive/deleted publishersOrg-targeted skill transfers
adminorownerrole required on source/target publisherfromPublisherIdfor org transfers (notfromUserId), correctly handling the case where the requesting admin differs fromownerUserIdtoUserIdorfromUserIdpublisherIdoverride on accept is validated against org membershipSchema changes
fromPublisherIdandtoPublisherIdfields toskillOwnershipTransferstoUserIdoptional to support org-targeted transfersAPI updates
POST /api/v1/skills/{slug}/transfer— optionaltoPublisherHandlein body for org targetingPOST /api/v1/skills/{slug}/transfer/accept— optionalpublisherHandlein body to assign to org on accepttype: "skill"discriminator field (preparation for unified listing in PR 2)toUserId/fromUserIdlookup misses, falls back to unfiltered pending transfer lookup so any org admin can actCLI updates
--type skill|packageflag — explicit type selection with auto-detection fallback (probes both APIs, errors on ambiguity)--publisher @org-handleflag — target an org on request, or assign to org on accepttransfer listoutput with TYPE column--typeflagBackwards compatibility
Files changed (13 files, +858 / -118)
convex/lib/transfers.tsconvex/lib/transfers.test.tsconvex/schema.tsskillOwnershipTransferswith publisher fieldsconvex/skillTransfers.tsconvex/skillTransfers.test.tsconvex/httpApiV1/skillsV1.tsconvex/httpApiV1/transfersV1.tstype: "skill"discriminator to listing resultspackages/clawhub/src/cli/commands/transfer.tspackages/clawhub/src/cli.tspackages/clawhub/src/schema/schemas.tsdocs/http-api.mddocs/cli.mddocs/orgs.mdTest plan