Skip to content

fix: block direct skill transfers under moderation#2282

Open
vyctorbrzezowski wants to merge 4 commits into
openclaw:mainfrom
vyctorbrzezowski:contrib/direct-transfer-state-guards
Open

fix: block direct skill transfers under moderation#2282
vyctorbrzezowski wants to merge 4 commits into
openclaw:mainfrom
vyctorbrzezowski:contrib/direct-transfer-state-guards

Conversation

@vyctorbrzezowski
Copy link
Copy Markdown
Contributor

@vyctorbrzezowski vyctorbrzezowski commented May 16, 2026

Summary

Prevents moderated skills from changing owners. Direct transfers, accepted transfer requests, admin owner changes, and publish-time owner migration now all wait until moderation is cleared.

What changed

  • Direct owner moves are rejected when a skill is hidden, removed, suspicious, or malicious.
  • Accepted pending-transfer flows run the same moderation guard before writing the new owner.
  • Permission checks still happen before moderation-state checks so unauthorized users do not learn hidden/moderation state.

Public behavior

Direct transfers and accepted pending transfers fail explicitly when the skill is under moderation. They do not silently move ownership while the skill is hidden, removed, suspicious, or malicious.

Unauthorized actors are still rejected before moderation state is revealed.

Behavior proof

Live Convex runtime proof from both ownership-transfer paths:

$ bunx convex run --push --typecheck=disable --codegen disable proof2282:run
- Preparing Convex functions...

✔ Convex functions ready!
{
  "directTransfer": {
    "error": "Skill is not eligible for ownership transfer while under moderation",
    "ok": false
  },
  "acceptedTransfer": {
    "ok": true,
    "value": {
      "error": "Skill is under moderation",
      "ok": false
    }
  },
  "transferAfterAccept": {
    "respondedAt": "<redacted>",
    "status": "cancelled"
  }
}

This proof creates a moderated skill, attempts a direct owner transfer, then attempts to accept a pending transfer. Both ownership writes are rejected; the accepted-transfer path also cancels the pending transfer instead of moving the owner.

Focused regression suite:

$ bun run test convex/skillTransfers.test.ts convex/skills.ownership.test.ts
Test Files  2 passed (2)
Tests       16 passed (16)

Validation

$ bun run ci:unit
Test Files  204 passed (204)
Tests       1988 passed (1988)
Statements  85.98%
Branches    75.17%
Functions   86.37%
Lines        89.48%
$ bun run ci:types-build
tsc, package typechecks, clawhub-mod typecheck, Vite build, and Nitro build passed.

Current GitHub CI for this head also has packages, types-build, e2e-http, playwright-smoke, and playwright-local-auth passing. The static job currently stops at bun audit on the existing transitive ws advisory GHSA-58qx-3vcg-4xpx.

@vyctorbrzezowski vyctorbrzezowski requested review from a team and Patrick-Erichsen as code owners May 16, 2026 04:00
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 16, 2026

@vyctorbrzezowski 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 16, 2026

Codex review: needs changes before merge. Reviewed May 24, 2026, 4:44 PM ET / 20:44 UTC.

Summary
The branch adds a shared moderation-transfer guard for direct, accepted, admin, reclaim, and publish-time skill ownership moves, with regression tests, a spec note, and a changelog entry.

Reproducibility: yes. Current-main source inspection shows the broader transfer paths lack the new moderation guard, and the PR-head unit job gives a high-confidence reproduction of the current blocker with unexpected skillSlugAliases index by_skill.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Fix the ownership-heal fixture for the alias-sync query path.
  • Rerun the focused ownership-transfer/rate-limit tests and bun run ci:unit.
  • Get maintainer acceptance or narrowing for the fail-closed moderated-transfer policy.

Risk before merge

  • The PR cannot merge while ci:unit fails in the ownership-heal fixture for the new skillSlugAliases.by_skill query path.
  • The branch intentionally makes owner changes, direct transfers, accepted transfers, reclaim moves, and publish-time owner migration fail closed for moderated skills; existing admin or ownership-recovery workflows may stop until moderation clears.

Maintainer options:

  1. Fix the red ownership-heal fixture (recommended)
    Update the rate-limit ownership-heal fixture so it supports the alias-sync query path introduced by transferSkillOwnershipAndEmbeddings, then rerun focused tests and ci:unit.
  2. Accept the fail-closed policy
    Maintainers can intentionally keep the policy if moderated skills must be cleared before any owner migration, admin transfer, direct transfer, accepted transfer, or reclaim write changes ownership.
  3. Pause for a narrower exception
    If admin recovery or same-identity ownership healing must work while a skill is moderated, pause this PR and request a narrower policy before merge.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Update convex/skills.rateLimit.test.ts so the ownership-heal fixture supports the skillSlugAliases by_skill/collect() query introduced by transferSkillOwnershipAndEmbeddings, keep the fail-closed moderation guard intact, and run the focused ownership-transfer/rate-limit tests plus ci:unit.

Next step before merge
A focused automated repair can update the failing rate-limit fixture; maintainers still need to own the fail-closed policy decision after the fixture is fixed.

Security
Cleared: The patch narrows a moderation and ownership bypass and does not add dependencies, workflows, secrets handling, generated artifacts, or new supply-chain execution paths.

Review findings

  • [P2] Update the ownership-heal fixture for alias syncing — convex/skills.ts:9601
Review details

Best possible solution:

Keep the shared moderation-transfer guard, repair the red rate-limit fixture, and merge only after maintainers explicitly accept the fail-closed ownership-transfer policy.

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

Yes. Current-main source inspection shows the broader transfer paths lack the new moderation guard, and the PR-head unit job gives a high-confidence reproduction of the current blocker with unexpected skillSlugAliases index by_skill.

Is this the best way to solve the issue?

No, not as-is. The shared guard is a maintainable direction, but the branch still needs the failing fixture repaired and maintainer acceptance of the compatibility-sensitive fail-closed policy.

Full review comments:

  • [P2] Update the ownership-heal fixture for alias syncing — convex/skills.ts:9601
    ci:unit is red because this new call now reaches transferSkillOwnershipAndEmbeddings, which queries skillSlugAliases with by_skill, but the existing convex/skills.rateLimit.test.ts ownership-heal fixture only accepts by_slug. Please update the fixture to cover the alias-sync query before merge.
    Confidence: 0.94

Overall correctness: patch is incorrect
Overall confidence: 0.9

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

Label changes

Label justifications:

  • P1: The PR addresses a moderation/ownership bypass that can move hidden, suspicious, or malicious skills across owners, but unit CI is currently red.
  • merge-risk: 🚨 compatibility: The diff intentionally changes existing owner-transfer and publish-migration workflows to fail closed for moderated skills until moderation clears.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body includes live Convex runtime output for direct and accepted transfer rejection, plus focused test and CI summaries, which is sufficient backend behavior proof.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes live Convex runtime output for direct and accepted transfer rejection, plus focused test and CI summaries, which is sufficient backend behavior proof.
Evidence reviewed

Acceptance criteria:

  • bun run test convex/skills.rateLimit.test.ts convex/skillTransfers.test.ts convex/skills.ownership.test.ts convex/skills.ownerMigration.test.ts convex/skills.reclaim.test.ts
  • bun run ci:unit

What I checked:

  • live PR state: Live PR metadata shows head 4787eb6, contributor live Convex proof in the body, and the latest CI unit job failing while static, packages, types-build, e2e-http, and Playwright checks pass. (4787eb6f4ddd)
  • current main accepted-transfer gap: Current main only cancels accepted transfers for malicious, hidden, or removed skills, leaving suspicious flags/reasons and reason-code-derived verdicts uncovered in that path. (convex/skillTransfers.ts:193, 963b0a571943)
  • current main direct-transfer gap: Current main routes direct owner transfers through transferSkillOwnershipAndEmbeddings after permission checks, but the shared helper does not currently check moderation state before patching skill ownership and aliases. (convex/skills.ts:8830, 963b0a571943)
  • PR implementation evidence: The PR head adds isSkillTransferBlockedByModeration, deriving verdicts from reason codes when needed and blocking hidden, removed, suspicious, malicious, suspicious-flagged, and malware-flagged states. (convex/lib/skillSafety.ts:20, 4787eb6f4ddd)
  • CI failure evidence: The unit job fails because the ownership-heal fixture only accepts skillSlugAliases.by_slug while the new shared helper now queries skillSlugAliases.by_skill and collect(). (convex/skills.rateLimit.test.ts:981, 4787eb6f4ddd)
  • related merged transfer hardening: Related merged work hardened accepted transfer cancellation, but it does not cover this PR's broader direct/admin/reclaim/publish ownership-transfer surfaces. (convex/skillTransfers.ts:171, 8bbc66868d63)

Likely related people:

  • steipete: Git history ties the original skill transfer API and CLI to Peter Steinberger's commit 460ad3c. (role: introduced behavior; confidence: high; commits: 460ad3c13dd9; files: convex/skillTransfers.ts)
  • vyctorbrzezowski: They authored the merged transfer hardening in 8bbc668 before this PR, so they are connected to current-main transfer behavior beyond being this PR author. (role: recent area contributor; confidence: high; commits: 8bbc66868d63, 07bf41e1091e; files: convex/skillTransfers.ts, convex/skillTransfers.test.ts, convex/httpApiV1/skillsV1.ts)
  • Patrick-Erichsen: Current-main blame and recent file history show recent work around central skill ownership and API code, and the PR has a review request for this person. (role: recent area contributor; confidence: medium; commits: b753b1f7ab0e, 0907fae0d991; files: convex/skills.ts, convex/skillTransfers.ts)
  • Momo: Commit 5b63d5d hardened skill owner migration, one of the paths affected by this PR's shared ownership helper. (role: adjacent owner; confidence: medium; commits: 5b63d5df6071; files: convex/skills.ts)
  • ImLukeF: Related merged pull request fix: align transferred skill publisher ownership #1344 changed transferred skill publisher ownership semantics in the same transfer module. (role: adjacent owner; confidence: medium; commits: d8e1f0daa1a4; files: convex/skillTransfers.ts, convex/skillTransfers.test.ts)
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.

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.

@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/direct-transfer-state-guards branch from c117cdb to 1065c0b Compare May 16, 2026 22:44
Comment thread convex/skillTransfers.test.ts
Comment thread convex/skillTransfers.ts Outdated
@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/direct-transfer-state-guards branch from 1065c0b to e299a5d Compare May 16, 2026 22:50
@clawsweeper clawsweeper Bot added P1 High-priority user-facing bug, regression, or broken workflow. impact:security Security boundary, credential, authz, sandbox, or sensitive-data risk. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels May 16, 2026
@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/direct-transfer-state-guards branch from e299a5d to c7bd672 Compare May 18, 2026 15:01
@vyctorbrzezowski
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added the status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. label May 18, 2026
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 18, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot removed the impact:security Security boundary, credential, authz, sandbox, or sensitive-data risk. label May 18, 2026
@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/direct-transfer-state-guards branch from c7bd672 to ef514a4 Compare May 18, 2026 20:13
@vyctorbrzezowski
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed 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 18, 2026
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 18, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 20, 2026

ClawSweeper PR egg

🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress.

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.
What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@vyctorbrzezowski vyctorbrzezowski force-pushed the contrib/direct-transfer-state-guards branch from 98b5244 to 4eb0f2b Compare May 21, 2026 15:19
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels May 22, 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. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant