feat: add skill ownership transfer #168
Conversation
This implements Phase 1 of the ownership architecture improvements:
Schema:
- Add skillOwnershipTransfers table with indexes
Backend (convex/skillTransfers.ts):
- requestTransfer: Owner can request transfer to another user
- acceptTransfer: Recipient accepts and becomes new owner
- rejectTransfer: Recipient declines
- cancelTransfer: Owner cancels pending request
- listIncoming/listOutgoing: Query pending transfers
- countIncoming: For notification badges
- Transfer expires after 7 days
API Routes:
- POST /api/v1/skills/{slug}/transfer - Request transfer
- POST /api/v1/skills/{slug}/transfer/accept - Accept
- POST /api/v1/skills/{slug}/transfer/reject - Reject
- POST /api/v1/skills/{slug}/transfer/cancel - Cancel
- GET /api/v1/transfers/incoming - List incoming requests
- GET /api/v1/transfers/outgoing - List outgoing requests
CLI:
- clawhub transfer request <slug> <handle> [--message]
- clawhub transfer list [--outgoing]
- clawhub transfer accept <slug>
- clawhub transfer reject <slug>
- clawhub transfer cancel <slug>
Audit logging included for all transfer actions.
Closes openclaw#167
|
@aronchick is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export const acceptTransfer = mutation({ | ||
| args: { | ||
| transferId: v.id('skillOwnershipTransfers'), |
There was a problem hiding this comment.
Ownership/state not revalidated
acceptTransfer updates skills.ownerUserId based on a previously-created transfer without re-checking the current skill state. If the skill became soft-deleted after the request, or if skill.ownerUserId no longer equals transfer.fromUserId (e.g. ownership changed via another action), accepting will still overwrite the owner.
Consider validating !skill.softDeletedAt and skill.ownerUserId === transfer.fromUserId immediately before patching, and failing otherwise to avoid transferring deleted skills or clobbering a newer owner.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skillTransfers.ts
Line: 82:84
Comment:
**Ownership/state not revalidated**
`acceptTransfer` updates `skills.ownerUserId` based on a previously-created transfer without re-checking the current skill state. If the skill became soft-deleted after the request, or if `skill.ownerUserId` no longer equals `transfer.fromUserId` (e.g. ownership changed via another action), accepting will still overwrite the owner.
Consider validating `!skill.softDeletedAt` and `skill.ownerUserId === transfer.fromUserId` immediately before patching, and failing otherwise to avoid transferring deleted skills or clobbering a newer owner.
How can I resolve this? If you propose a fix, please make it concise.| console.log(options.outgoing ? 'Outgoing transfers:' : 'Incoming transfers:') | ||
| for (const t of result.transfers) { | ||
| const other = options.outgoing ? t.toUser?.handle : t.fromUser?.handle | ||
| const expiresIn = Math.ceil((t.expiresAt - Date.now()) / (24 * 60 * 60 * 1000)) |
There was a problem hiding this comment.
May print @undefined
In transfer list, other is derived from t.toUser?.handle / t.fromUser?.handle, then interpolated as @${other}. If the API response ever omits those nested objects (your local type already allows it), the CLI will output @undefined.
Either make the API contract require fromUser for incoming and toUser for outgoing (and validate it), or handle the missing case here (e.g. print (unknown) and avoid always prefixing @).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/clawdhub/src/cli/commands/transfer.ts
Line: 98:101
Comment:
**May print `@undefined`**
In `transfer list`, `other` is derived from `t.toUser?.handle` / `t.fromUser?.handle`, then interpolated as `@${other}`. If the API response ever omits those nested objects (your local type already allows it), the CLI will output `@undefined`.
Either make the API contract require `fromUser` for incoming and `toUser` for outgoing (and validate it), or handle the missing case here (e.g. print `(unknown)` and avoid always prefixing `@`).
How can I resolve this? If you propose a fix, please make it concise.
|
This is a really good feature. Small question, the old namespace URL (clawhub.ai/old-owner/skill-slug) will not resolve after transfer right? It might be worth keeping the old URL and redirecting it (301) to the new owner URL to avoid breaking existing links or docs. Has it been considered here? |
|
Terrific idea, should I do that or do maintainers want to pick that up? |
2 participants
Summary
Implements Phase 1 of the ownership architecture improvements: skill ownership transfer.
What This Adds
Schema
skillOwnershipTransferstable tracking transfer requestsBackend (
convex/skillTransfers.ts)requestTransferacceptTransferrejectTransfercancelTransferlistIncominglistOutgoingcountIncomingAPI Routes
CLI
Audit Trail
All transfer actions are logged to
auditLogstable:skill.transfer.requestskill.transfer.acceptskill.transfer.rejectWhat's NOT Included (Future Phases)
Testing
Closes #167 (Phase 1)
Greptile Overview
Greptile Summary
This PR adds Phase 1 of skill ownership transfers.
skillOwnershipTransferstable (pending → accepted/rejected/cancelled/expired) with timestamps and expiry./skills/{slug}/transfer[/accept|reject|cancel]and adds a new GET router under/api/v1/transfers/{incoming|outgoing}.clawhub transfercommand group to request/list/accept/reject/cancel transfers via the new endpoints.Confidence Score: 3/5
acceptTransferdoes not revalidate the skill’s current owner or deletion state at acceptance time, which can lead to incorrect ownership changes if the skill changed after the request. Minor CLI output robustness issue also present.