feat: scope skill slugs by owner#2299
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Codex review: needs real behavior proof before merge. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. for the review finding from source inspection: the PR branch adds owner-aware internal mutations but the V1 HTTP management handlers still call them without owner handles. I did not execute tests because this review must keep the checkout read-only. PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. PR egg Where did the egg go?
Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review findings
Review detailsBest possible solution: Land owner-scoped skill slugs only after every slug-taking read/write/management path either accepts an owner namespace or intentionally rejects ambiguous unscoped requests, with regression tests and real proof for fresh and upgrade-style flows. Do we have a high-confidence way to reproduce the issue? Yes for the review finding from source inspection: the PR branch adds owner-aware internal mutations but the V1 HTTP management handlers still call them without owner handles. I did not execute tests because this review must keep the checkout read-only. Is this the best way to solve the issue? No as proposed: the owner-scoped direction matches the existing orgs spec, but the implementation is incomplete until management APIs are scoped or explicitly reject ambiguous unscoped requests. The safer path is to thread ownerHandle through those endpoints and add focused duplicate-slug regression coverage plus real proof. Label justifications:
Full review comments:
Overall correctness: patch is incorrect Security concerns:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 2aa2a449e5fc. |
Summary
This PR makes skill slug resolution owner-aware across the skill publish/read/install surfaces that still assumed
slugwas globally unique. The platform invariant isowner/publisher + slug, not bareslug.The immediate production crash from duplicate slugs in
skills.checkSlugAvailabilitywas already fixed by #2179. This PR is the broader follow-up: make the surrounding publish/update/API/CLI/UI paths preserve the owner namespace so future org publishing and duplicate slugs do not regress into ambiguous or wrong-owner behavior.Paired PR
What Changed
convex/lib/skills/slugResolution.tsas the shared skill-slug resolution layer.ownerPublisherId + slug..unique()crashes.skills.checkSlugAvailabilityrequireownerHandleand answer the real question: “is this slug available for this publisher?”sourceOwnerHandle+migrateOwnerhandling so moving an existing skill between publishers is opt-in and permission-checked instead of an accidental side effect of publish.ownerHandleto existing v1 HTTP read/download/resolve/publish routes so new clients can be deterministic without breaking old clients.@owner/slug, pass owner context to detail/download/resolve/install/update flows, and store owner metadata in lock/origin files.--owner <handle>is explicit, while omitted--ownerresolves/api/v1/whoamiand sends the authenticated user handle for today’s ergonomics.Compatibility Decisions
ownerHandle, the HTTP publish endpoint preserves existing behavior by publishing into the authenticated user’s personal namespace./api/v1/skills/<slug>or/api/v1/download?slug=<slug>hard-require owner context in this PR.openclawpublisher match or returns a structured ambiguity response instead of throwing from Convex.unique().migrateOwnerand validates source/destination permissions.Validation Logic Covered
ownerHandlecontinue through the compatibility endpoint.Ruled Out / Not Doing Here
/api/v2/skills/@owner/slug, but this PR keeps v1 compatibility and adds optional owner context instead.ownerHandleat every HTTP boundary would be cleaner architecturally, but it would break older published CLIs.checkSlugAvailabilitycrash; this PR makes the surrounding surfaces match the org-era invariant.Verification
autoreviewfrommain: clean, no actionable findingsVITE_CONVEX_URL=https://example.invalid bun run testcd packages/clawhub && bun run test:srcbunx tsc -p convex/tsconfig.json --noEmit --pretty falsebunx tsc -p packages/clawhub/tsconfig.json --noEmit --pretty falsebunx tsc -p tsconfig.json --noEmit --pretty falseReviewer Notes
This branch is intentionally broader than #2179 because once one surface starts carrying owner context, downstream surfaces need to preserve it or they can silently switch namespaces. The highest-risk areas to review are publish/versioning preflight, owner migration permissions, legacy slug-only fallback behavior, and CLI sync owner preservation.