Skip to content

feat: scope skill slugs by owner#2299

Open
Patrick-Erichsen wants to merge 1 commit into
mainfrom
pe/owner-scoped-skill-slugs
Open

feat: scope skill slugs by owner#2299
Patrick-Erichsen wants to merge 1 commit into
mainfrom
pe/owner-scoped-skill-slugs

Conversation

@Patrick-Erichsen
Copy link
Copy Markdown
Contributor

@Patrick-Erichsen Patrick-Erichsen commented May 18, 2026

Summary

This PR makes skill slug resolution owner-aware across the skill publish/read/install surfaces that still assumed slug was globally unique. The platform invariant is owner/publisher + slug, not bare slug.

The immediate production crash from duplicate slugs in skills.checkSlugAvailability was 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

  • Added convex/lib/skills/slugResolution.ts as the shared skill-slug resolution layer.
    • Scoped lookups resolve by ownerPublisherId + slug.
    • Alias checks are scoped to the owner namespace.
    • Legacy slug-only reads use a controlled ambiguity path instead of global .unique() crashes.
  • Made skills.checkSlugAvailability require ownerHandle and answer the real question: “is this slug available for this publisher?”
  • Updated publish/versioning preflight and write paths to resolve the target owner before deciding whether a publish is new, an update, or an explicit migration.
  • Added sourceOwnerHandle + migrateOwner handling so moving an existing skill between publishers is opt-in and permission-checked instead of an accidental side effect of publish.
  • Added optional ownerHandle to existing v1 HTTP read/download/resolve/publish routes so new clients can be deterministic without breaking old clients.
  • Updated the ClawHub CLI to understand @owner/slug, pass owner context to detail/download/resolve/install/update flows, and store owner metadata in lock/origin files.
  • Updated CLI publish so --owner <handle> is explicit, while omitted --owner resolves /api/v1/whoami and sends the authenticated user handle for today’s ergonomics.
  • Updated publish/import/New Version UI flows to preserve and pass owner context when checking availability or publishing a new version.

Compatibility Decisions

  • Old CLI/API publish clients still work. If an older client omits ownerHandle, the HTTP publish endpoint preserves existing behavior by publishing into the authenticated user’s personal namespace.
  • Slug-only public v1 reads still exist. We did not make /api/v1/skills/<slug> or /api/v1/download?slug=<slug> hard-require owner context in this PR.
  • Legacy ambiguity is controlled. When a slug-only read has multiple possible matches, the helper either selects the legacy preferred openclaw publisher match or returns a structured ambiguity response instead of throwing from Convex .unique().
  • Owner migration is explicit. Republishing with a different owner does not silently transfer a skill; migration requires migrateOwner and validates source/destination permissions.

Validation Logic Covered

  • Duplicate slug under different publishers is allowed.
  • Duplicate slug under the same publisher is unavailable.
  • Availability requires an owner namespace.
  • Alias conflicts are checked in the same owner namespace.
  • Publish preflight uses the same owner-scoped resolution as the write path.
  • Old CLI publish requests that omit ownerHandle continue through the compatibility endpoint.
  • CLI sync preserves the installed origin owner instead of republishing an org-owned skill into the personal namespace.
  • Legacy slug-only reads fail gracefully when ambiguous instead of crashing.
  • Download/resolve/detail paths accept owner context when the caller has it.

Ruled Out / Not Doing Here

  • Not enforcing a v2-only API shape yet. The cleaner long-term shape is something like /api/v2/skills/@owner/slug, but this PR keeps v1 compatibility and adds optional owner context instead.
  • Not breaking old CLI users. Requiring ownerHandle at every HTTP boundary would be cleaner architecturally, but it would break older published CLIs.
  • Not treating slug as globally unique. The fix is not to reserve slugs globally; it is to consistently resolve within the publisher namespace.
  • Not allowing implicit owner transfers. Publishing a skill under another owner is distinct from migrating an existing skill’s owner.
  • Not claiming this is a small bugfix. This is an architectural hardening PR. fix: avoid slug availability crash for duplicate slugs #2179 fixed the immediate checkSlugAvailability crash; this PR makes the surrounding surfaces match the org-era invariant.

Verification

  • autoreview from main: clean, no actionable findings
  • VITE_CONVEX_URL=https://example.invalid bun run test
  • cd packages/clawhub && bun run test:src
  • bunx tsc -p convex/tsconfig.json --noEmit --pretty false
  • bunx tsc -p packages/clawhub/tsconfig.json --noEmit --pretty false
  • bunx tsc -p tsconfig.json --noEmit --pretty false

Reviewer 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.

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 18, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clawhub Ready Ready Preview, Comment May 18, 2026 9:54pm

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 18, 2026

Codex review: needs real behavior proof before merge.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The PR scopes skill slug resolution by owner across publish, update, availability, download, import, search, page, and CLI flows, with new slug-resolution helpers and regression coverage.

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
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Summary: Not quality-ready yet: the patch has useful tests and a coherent direction, but proof is missing and a high-impact owner-scoped management gap remains.

Rank-up moves:

  • Fix the V1 management route owner scoping gap and add duplicate-slug regression tests for rename/merge/delete/undelete or explicit ambiguous-request rejection.
  • Add redacted real behavior proof showing owner-scoped publish plus install/download and at least one management path against duplicate slugs.
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature, rarity, or ASCII portrait is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

Real behavior proof
Needs real behavior proof before merge: Missing: the PR body lists tests/typechecks/autoreview and has a Vercel preview, but no after-fix screenshot, terminal output, logs, recording, or linked artifact demonstrating owner-scoped behavior; please redact private endpoints, tokens, IPs, phone numbers, and other private details before posting proof, then update the PR body to trigger a fresh review or ask a maintainer for @clawsweeper re-review.

Risk before merge
Why this matters: - Merging as-is would allow duplicate owner-scoped slugs while V1 management actions such as rename, merge, transfer, delete, and undelete still resolve by unqualified slug, so users may be unable to target a non-legacy owner namespace or may mutate the wrong owned skill when they administer multiple matching publishers.

  • The upgrade path is broad: publish, import, HTTP API, search, page rendering, downloads, lockfiles, and CLI flows all change slug resolution semantics, but the PR currently provides tests only and no observed real behavior proof from a live setup.
  • The ownership-boundary surface is sensitive enough that green unit tests do not settle the merge risk; reviewer confidence needs a small real publish/install/manage transcript or recording with private data redacted.

Maintainer options:

  1. Fix Management Scoping First (recommended)
    Parse the request owner namespace for V1 skill management actions and pass it through rename, merge, transfer, delete, undelete, report, and appeal paths where owner-scoped slugs can collide.
  2. Accept Legacy-Only Management
    Maintainers could intentionally keep management endpoints slug-only, but that should be documented as unsupported for duplicate slugs before enabling owner-scoped publish behavior.
  3. Pause Until End-To-End Proof Exists
    Hold the PR until the branch includes redacted terminal logs, screenshots, or recordings showing owner-scoped publish, install/download, and management behavior in a real setup.

Next step before merge
Needs contributor proof and maintainer review of the owner-scoped management API gap before merge; automation should not take over because missing real behavior proof is a contributor-side gate.

Security
Needs attention: The diff touches publisher ownership boundaries, and the remaining slug-only management routes create ambiguous resource targeting for same-slug skills.

Review findings

  • [P1] Thread owner handles through management actions — convex/httpApiV1/skillsV1.ts:1266-1270
Review details

Best 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:

  • P2: This is a normal-priority feature with significant routing and compatibility surface, not an emergency runtime outage.
  • merge-risk: 🚨 compatibility: The PR changes slug identity from global to owner-scoped across APIs and CLI state, which can break callers that still use unqualified management paths.
  • merge-risk: 🚨 security-boundary: The diff touches publisher ownership boundaries, and unscoped management actions can target a different same-slug publisher than the caller intended.

Full review comments:

  • [P1] Thread owner handles through management actions — convex/httpApiV1/skillsV1.ts:1266-1270
    The PR makes skill slugs owner-scoped, but the V1 HTTP management handlers still invoke rename and merge without passing an owner namespace; delete/undelete have the same slug-only shape. Once two publishers can own the same slug, these routes cannot target the non-legacy owner and may resolve the wrong skill for users who administer multiple matching publishers. Parse ownerHandle/owner from the request and pass it through every management mutation, or reject ambiguous unscoped requests before merge.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.88

Security concerns:

  • [medium] Unscoped management routes cross owner namespaces — convex/httpApiV1/skillsV1.ts:1266
    Owner-scoped slugs make slug alone insufficient to identify a skill, but V1 management routes still call ownership mutations with only the slug; permission checks still run, but the selected resource can be from a different publisher than intended.
    Confidence: 0.82

What I checked:

  • Current main uses global skill slug lookup: Current main getBySlug accepts only { slug } and resolves through resolveSkillBySlugOrAlias, which performs a global by_slug unique lookup before aliases. (convex/skills.ts:1973, 2aa2a449e5fc)
  • Current main publish preflight is unscoped: Current main publish preflight calls internal.skills.getSkillBySlugInternal with only the normalized slug, so it cannot distinguish same slug under different owners. (convex/lib/skillPublish.ts:120, 2aa2a449e5fc)
  • Spec supports owner-scoped uniqueness: The orgs spec defines skill uniqueness as (ownerPublisherId, slug) and says old unscoped lookup must stop pretending there is one canonical answer once multiple scoped skills share the same local name. (specs/orgs.md:170, 2aa2a449e5fc)
  • PR adds owner-aware slug resolution: The PR branch adds resolvePublisherByOwnerHandle and publisher-scoped getSkillBySlugForPublisher helpers, and changes getBySlug to accept ownerHandle. (convex/lib/skills/slugResolution.ts:24, 5e46dd6a45ed)
  • PR leaves V1 management actions unscoped: On the PR branch, the V1 HTTP rename and merge handlers call internal mutations without ownerHandle, and delete/undelete still pass only slug to setSkillSoftDeletedInternal. (convex/httpApiV1/skillsV1.ts:1266, 5e46dd6a45ed)
  • Internal mutations already have owner parameters: On the PR branch, renameOwnedSkillInternal and mergeOwnedSkillIntoCanonicalInternal accept owner handle arguments, so the HTTP omission has a clear narrow repair path. (convex/skills.ts:7703, 5e46dd6a45ed)

Likely related people:

  • Peter Steinberger: Blame shows the current global slug lookup, publish preflight, and V1 skill management route code in the central files date to commit 1a00013c21b15c5b99fa2946eec065f4ec597cc8. (role: feature-history owner; confidence: high; commits: 1a00013c21b1; files: convex/skills.ts, convex/lib/skillPublish.ts, convex/httpApiV1/skillsV1.ts)
  • Patrick-Erichsen: Current main includes recent adjacent work in convex/skills.ts at commit 7cf16ebb28aa90434054b1eabd2bfe9dc5617710, and this PR continues in the same ownership and skill-routing area. (role: recent area contributor; confidence: medium; commits: 7cf16ebb28aa, 5e46dd6a45ed; files: convex/skills.ts, convex/httpApiV1/skillsV1.ts, packages/clawhub/src/cli/commands/skills.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 2aa2a449e5fc.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant