Skip to content

fix: allow package publishes when matching skill slug is owned#2391

Open
davidmosiah wants to merge 1 commit into
openclaw:mainfrom
davidmosiah:fix/package-skill-owner-collision
Open

fix: allow package publishes when matching skill slug is owned#2391
davidmosiah wants to merge 1 commit into
openclaw:mainfrom
davidmosiah:fix/package-skill-owner-collision

Conversation

@davidmosiah
Copy link
Copy Markdown

Summary

  • Allow package publishes to proceed when an existing skill with the same slug is owned by the same user/publisher.
  • Keep the existing collision block when the matching skill slug belongs to a different owner.
  • Add regression coverage for both same-owner and different-owner package publish paths.

Fixes #2390.

Tests

  • bunx vitest run convex/packages.public.test.ts --testNamePattern "allows package publishes when the matching skill slug belongs to the same owner" --reporter verbose (red before implementation, green after)
  • bunx vitest run convex/packages.public.test.ts --testNamePattern "matching skill slug" --reporter verbose
  • bunx vitest run convex/packages.public.test.ts --reporter dot
  • bunx oxfmt --check convex/packages.ts convex/packages.public.test.ts
  • git diff --check
  • bunx tsc --noEmit --pretty false
  • bunx oxlint --type-aware --tsconfig ./tsconfig.oxlint.json convex/packages.ts convex/packages.public.test.ts

Notes

Full repo bun run format:check currently reports an unrelated formatting issue in convex/lib/securityPrompt.test.ts, and full bun run lint reports unrelated no-unnecessary-type-conversion errors in convex/securityScan.test.ts. I left those files untouched.

@davidmosiah davidmosiah requested review from a team and Patrick-Erichsen as code owners May 24, 2026 13:54
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 24, 2026

@davidmosiah is attempting to deploy a commit to the Amantus Machina Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 24, 2026

Codex review: needs real behavior proof before merge.

Latest ClawSweeper review: 2026-05-24 14:03 UTC / May 24, 2026, 10:03 AM ET.

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 branch changes the package publish collision guard to permit same-owner skill-slug matches and adds unit tests for same-owner and different-owner collision paths.

Reproducibility: yes. Source inspection shows current main rejects any package publish whose normalized name resolves to a skill or alias before the existing package owner checks can run; I did not run a live publish because it depends on account/package state.

PR rating
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Summary: The PR has a plausible fix direction, but missing real behavior proof and an overbroad namespace bypass make it not merge-ready.

Rank-up moves:

  • Narrow the collision exception to existing owned package updates and add regression coverage for first-publish and org/personal publisher edge cases.
  • Add redacted live CLI or server publish output showing the same-owner existing package update succeeds after the fix.
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.

Real behavior proof
Needs real behavior proof before merge: The PR body lists unit, lint, and typecheck commands, but no redacted live CLI/server publish output or logs showing the fixed after-publish behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • The patch currently allows first publishes to bypass the skill/alias collision guard when the resolved skill shares ownerUserId, even though the reported failure is an existing package version publish.
  • The PR body lists tests and checks, but does not provide redacted live CLI/server output showing the fixed publish path in a real setup.

Maintainer options:

  1. Constrain The Bypass (recommended)
    Require an existing package and compare the requested package owner key to the resolved skill or alias owner before suppressing the collision error.
  2. Accept A Broader Namespace Policy
    If maintainers want same-owner first publishes over skills, document that policy and add focused tests for personal and org publisher cases before merging.

Next step before merge
Needs contributor proof plus a maintainer-reviewed revision to keep the package namespace guard narrow; this should not be sent to the repair lane while proof is missing.

Security
Needs attention: The diff weakens a package publish namespace guard beyond the reported same-owner existing-package update case.

Review findings

  • [P1] Keep first publishes under the collision guard — convex/packages.ts:4692-4695
Review details

Best possible solution:

Limit the collision exception to existing package version publishes where the requested package owner and resolved skill or alias owner match, keep first-publish and cross-owner collisions blocked, and add live proof for the package publish path.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main rejects any package publish whose normalized name resolves to a skill or alias before the existing package owner checks can run; I did not run a live publish because it depends on account/package state.

Is this the best way to solve the issue?

No. The direction is right, but the implementation should narrow the exception to existing owned package updates instead of allowing first publishes to bypass the collision guard.

Label changes:

  • add P2: This is a normal-priority package publish bug fix with limited blast radius, but it touches a security-sensitive namespace guard.
  • add merge-risk: 🚨 security-boundary: Merging the current diff would weaken the package publish skill/alias collision boundary beyond the linked existing-package update case.

Label justifications:

  • P2: This is a normal-priority package publish bug fix with limited blast radius, but it touches a security-sensitive namespace guard.
  • merge-risk: 🚨 security-boundary: Merging the current diff would weaken the package publish skill/alias collision boundary beyond the linked existing-package update case.
  • rating: 🧂 unranked krab: Current PR rating is 🧂 unranked krab because proof is 🧂 unranked krab, patch quality is 🧂 unranked krab, and The PR has a plausible fix direction, but missing real behavior proof and an overbroad namespace bypass make it not merge-ready.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists unit, lint, and typecheck commands, but no redacted live CLI/server publish output or logs showing the fixed after-publish behavior. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Full review comments:

  • [P1] Keep first publishes under the collision guard — convex/packages.ts:4692-4695
    The new check runs even when existingPackage is null, so a user can now create a first package publish over a skill or alias they own rather than only unblocking updates to an already-owned package. The linked bug is the existing package upgrade path; keep first publishes under the current collision block, then allow the bypass only after the package owner key is known to match the resolved skill owner.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.88

Security concerns:

  • [medium] Overbroad Collision Bypass — convex/packages.ts:4692
    Because the exception is not gated on an existing package update, the patch can allow new package creation over a resolved skill or alias that current main blocks, changing a security-sensitive namespace boundary.
    Confidence: 0.86

What I checked:

  • Current bug path exists on main: Current main resolves a skill or historical alias by the normalized package name and rejects any match before release insertion, matching the linked issue's failure mode. (convex/packages.ts:4677, 963b0a571943)
  • PR broadens the bypass: The PR diff suppresses the collision whenever the helper says the skill is owned by the requested user/publisher, without requiring an existing package update. (convex/packages.ts:4692, 5538311335f8)
  • Publisher ownership is the active package boundary: Package release insertion compares existing and requested package owner keys using ownerPublisherId when present, which is the narrower boundary the collision exception should preserve. (convex/packages.ts:5480, 963b0a571943)
  • Skill lookup includes aliases: getSkillBySlugInternal resolves both live skill slugs and aliases, so changing this guard affects historical alias collision protection too. (convex/skills.ts:2557, 963b0a571943)
  • Namespace intent: The plugin plan says package names are globally unique across families and no same-name package should exist once as a skill and once as a plugin, supporting a narrow existing-package exception rather than a broad first-publish bypass. (specs/plans/plugins.md:120, 963b0a571943)
  • Linked issue context: The linked issue asks to unblock publishing a new version for an already-present package with the same owned skill slug, not to allow new package creation over a skill slug.

Likely related people:

  • Patrick Erichsen: Current blame for the package publish flow and publisher target resolution points to the v0.17.0 release snapshot, and recent package/security-scan commits touched the package area. (role: recent area contributor; confidence: high; commits: b753b1f7ab0e, 636750fbf826, c538848a3d1e; files: convex/packages.ts, convex/publishers.ts, convex/packages.public.test.ts)
  • Peter Steinberger: Earlier commit metadata shows the native package registry, package auth boundary tightening, and the skill-slug collision guard were introduced in this area. (role: original package registry and authorization contributor; confidence: medium; commits: fc7d4dadca25, dff83a52bf7b, e7ef69101355; files: convex/packages.ts, convex/packages.public.test.ts, docs/http-api.md)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 963b0a571943.

@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. labels May 24, 2026
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 24, 2026

ClawSweeper 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 or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Package publish blocked when an owner also has a skill with the same slug

1 participant