fix: delete unpublished skill blobs when publish fails

[SebTardif]

What Problem This Solves

Fixes an issue where authenticated POST /api/v1/skills with multipart/form-data left Convex storage blobs behind after a failed publish. The handler stored each uploaded file, then validated the payload. An invalid body, a later file over 10MB, or a 400 after a successful parse (MIT-0 license refusal, owner resolution failure) never deleted those blobs. They were not attached to a skill version, so they stayed billed and unreferenced.

Why This Change Was Made

Multipart publish now matches the existing skill-scan upload path. File sizes are checked before any storage.store. If store or payload parse fails, stored ids are deleted. If parse succeeds and the handler still returns 400, those ids are deleted too. A successful publish keeps the blobs.

User Impact

Failed CLI or API skill publishes no longer leave orphaned upload blobs in Convex storage. Successful publishes are unchanged.

Evidence

Live bun on Darwin arm64, worktree /tmp/oc-pr-clawhub-F004. The script calls production parseMultipartPublish with a mock ctx.storage and a multipart body that stores SKILL.md then fails schema validation (no slug). The old loop stores storage:1 and never deletes it. The patched function stores the same id and deletes it before rethrowing.

$ bun /tmp/clawhub-f004-proof.mjs
before_error=Publish payload: slug must be a string
before_stored=storage:1 before_deleted=-
after_error=Publish payload: slug: a string
after_stored=storage:1 after_deleted=storage:1

The unpatched path leaves storage:1 with before_deleted=-. The patched path prints after_deleted=storage:1.

Real behavior proof

• Behavior or issue addressed: Failed multipart skill publish now deletes Convex blobs that were stored before the request returned 400.
• Real environment tested: macOS Darwin 25.6.0 arm64, bun 1.3.14, worktree /tmp/oc-pr-clawhub-F004 on fix/f004-publish-blob-cleanup. Proof imported production parseMultipartPublish from convex/httpApiV1/shared.ts.
• Exact steps or command run after this patch: Ran bun /tmp/clawhub-f004-proof.mjs. The script posted a multipart payload without slug after attaching SKILL.md, first through the old store-then-parse loop, then through production parseMultipartPublish.
• Evidence after fix: terminal output copied above. After the patch, after_stored=storage:1 and after_deleted=storage:1. Before the patch, before_stored=storage:1 and before_deleted=-.
• Observed result after fix: An invalid multipart publish now removes the blob it stored. The old loop left that blob unreferenced.
• What was not tested: A live clawhub.ai POST with a real API token. Windows. Successful publish retention of stored blobs on the hosted backend (local handler path keeps them when publish returns 200).

Notes

Same cleanup contract as parseMultipartSkillScan in PR #2479 and admin publisher logo updates in handleAdminUpdatePublisherProfile. Multipart store-without-cleanup landed in PR #315 (2026-02-15, 195 days). License 400 after parse landed in PR #815.

Allow edits from maintainers is enabled.

Tracker

Ref #3670

That issue stays open if this PR is closed without landing on main.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[vercel]

@SebTardif is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel.

A member of the Team first needs to authorize it.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 11, 2026, 10:08 AM ET / 14:08 UTC (Revision 75).

ClawSweeper review

What this changes

The PR checks multipart skill upload sizes before storage and deletes newly stored blobs when parsing or publication fails.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

⛔ Blocked before merge - 8 items remain

The leak remains on current main, and this PR addresses distinct, useful work. The previously reported post-commit deletion defect is still present, and the supplied proof uses mocked storage.

Priority: P2
Reviewed head: d4bcf53fd9278badbe4010076452ffbc9ed01b37

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The focused cleanup is useful, but the unresolved artifact-deletion defect and mocked proof prevent merge readiness.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: Needs real behavior proof before merge: the captured Darwin/Bun run exercises parseMultipartPublish with mocked storage, and handler tests mock publication. It does not show actual Convex cleanup or retained files after success and post-commit failure. Provide redacted HTTP/storage logs or terminal output from a real setup; terminal screenshots or recordings also count. Redact credentials, private endpoints, and other private details. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review. 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.
Patch quality	🦪 silver shellfish (2/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: Needs real behavior proof before merge: the captured Darwin/Bun run exercises parseMultipartPublish with mocked storage, and handler tests mock publication. It does not show actual Convex cleanup or retained files after success and post-commit failure. Provide redacted HTTP/storage logs or terminal output from a real setup; terminal screenshots or recordings also count. Redact credentials, private endpoints, and other private details. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review. 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.
Evidence reviewed	10 items	Verified scope and policy: Read the full root AGENTS.md, ClawHub maintainer and Convex guidance, and generated Convex guidelines. No nested AGENTS.md applies to the changed paths, and no maintainer-notes directory exists. The introduced delta contains four files; the checkout matches the pinned PR head. Current main still leaks multipart uploads: The current-main parser stores files before parsing the publish body and has no cleanup boundary. The multipart handler likewise returns validation errors without deleting its newly stored files. Latest release comparison: The v0.23.3 parser also stores multipart files before payload validation without cleanup. Reading its blob through local Git failed; the GitHub contents endpoint successfully supplied the release source.
Findings	1 actionable finding	[P1] Preserve uploaded blobs once the version transaction commits
Security	None	None.

How this fits together

ClawHub’s skill publishing API receives authenticated uploads, stores their files in Convex, and attaches them to a skill version. Security scans and download endpoints subsequently consume those stored files.

flowchart TD
  A[Authenticated multipart upload] --> B[Validate sizes and store files]
  B --> C[Validate publish request]
  C --> D[Commit skill version]
  D --> E[Schedule security scans]
  C --> F[Clean up unpublished files]
  E --> G[Return publish result]
  D --> H[Stored files for downloads]

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: Needs real behavior proof before merge: the captured Darwin/Bun run exercises parseMultipartPublish with mocked storage, and handler tests mock publication. It does not show actual Convex cleanup or retained files after success and post-commit failure. Provide redacted HTTP/storage logs or terminal output from a real setup; terminal screenshots or recordings also count. Redact credentials, private endpoints, and other private details. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review. 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.
• Preserve uploaded blobs once the version transaction commits (P1) - This prior finding remains unresolved. With staged prepublication disabled, publishVersionForUserInternal commits insertVersion and then awaits scheduleSkillPublishFollowups (convex/lib/skillPublish.ts:580–585). If a scan scheduler call rejects, this flag remains false and the finally block deletes files already referenced by the committed version, breaking subsequent reads and scans. Transfer cleanup ownership at persistence and add regression coverage for a post-commit scheduler failure.
• Resolve merge risk (P1) - A scan-scheduling failure after publication commits can cause the new cleanup to delete every uploaded file referenced by that version.
• Resolve merge risk (P1) - Real Convex behavior for failed uploads, successful publication, and post-commit failures remains unproven.
• Complete next step (P2) - Repair the post-commit deletion boundary, document blob ownership in specs, and supply real Convex cleanup and retention evidence before merge.
• Improve patch quality - Protect persisted version files from cleanup after follow-up failures, with focused regression coverage.
• Improve patch quality - Provide real Convex evidence showing rejected-upload cleanup and committed-file retention.
• Improve patch quality - Record the multipart blob ownership and persistence invariant in specs.

Findings

• [P1] Preserve uploaded blobs once the version transaction commits — convex/httpApiV1/skillsV1.ts:2670-2675

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Diff size	+184/-41 across 4 files	The change is confined to multipart parsing, the skill handler, and their tests.
Production and test delta	Production +67/-40; tests +117/-1	Production growth supports the stated cleanup boundary, while added tests cover failures before publication commits.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #3670
Summary: This PR is the candidate fix for the multipart skill-publishing tracker; import and package cleanup require separate entrypoint changes.

Members:

• canonical: Multipart skill publish leaves Convex blobs after a failed publish #3670 - The tracker explicitly links this patch and includes the committed-file retention requirement.
• adjacent_distinct: fix(import): delete GitHub skill blobs when import fails #3612 - Covers GitHub import storage rather than the multipart skill API.
• adjacent_distinct: fix(packages): delete unpublished blobs when package publish fails #3613 - Covers package files and artifacts rather than skill versions.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

1. End cleanup ownership at persistence (recommended)
   Preserve blobs once a version owns them and verify that a subsequent scan-scheduling failure cannot delete them.

Technical review

Best possible solution:

Make blob ownership transfer explicit at version persistence, retaining committed files through later failures and documenting that invariant in specs.

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

Yes, source inspection establishes the leak: a multipart file is stored before missing-slug validation throws, with no cleanup on current main. No live Convex reproduction was executed.

Is this the best way to solve the issue?

No, the parser cleanup is appropriate, but publication cleanup must stop at persistence rather than helper completion. Relying on direct-upload ticket expiry would not cover this multipart endpoint.

Full review comments:

• [P1] Preserve uploaded blobs once the version transaction commits — convex/httpApiV1/skillsV1.ts:2670-2675
  This prior finding remains unresolved. With staged prepublication disabled, publishVersionForUserInternal commits insertVersion and then awaits scheduleSkillPublishFollowups (convex/lib/skillPublish.ts:580–585). If a scan scheduler call rejects, this flag remains false and the finally block deletes files already referenced by the committed version, breaking subsequent reads and scans. Transfer cleanup ownership at persistence and add regression coverage for a post-commit scheduler failure.
  Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against cbfee7343ddc.

Labels

Label justifications:

• P2: This fixes a bounded authenticated-publishing storage leak without evidence of an ongoing urgent production incident.
• merge-risk: 🚨 other: The patch can delete published artifact files after a later scheduling failure, a durable-storage risk outside the more specific PR labels.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Needs real behavior proof before merge: the captured Darwin/Bun run exercises parseMultipartPublish with mocked storage, and handler tests mock publication. It does not show actual Convex cleanup or retained files after success and post-commit failure. Provide redacted HTTP/storage logs or terminal output from a real setup; terminal screenshots or recordings also count. Redact credentials, private endpoints, and other private details. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review. 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.

Evidence

What I checked:

• Verified scope and policy: Read the full root AGENTS.md, ClawHub maintainer and Convex guidance, and generated Convex guidelines. No nested AGENTS.md applies to the changed paths, and no maintainer-notes directory exists. The introduced delta contains four files; the checkout matches the pinned PR head. (AGENTS.md:1, d4bcf53fd927)
• Current main still leaks multipart uploads: The current-main parser stores files before parsing the publish body and has no cleanup boundary. The multipart handler likewise returns validation errors without deleting its newly stored files. (convex/httpApiV1/shared.ts:389, cbfee7343ddc)
• Latest release comparison: The v0.23.3 parser also stores multipart files before payload validation without cleanup. Reading its blob through local Git failed; the GitHub contents endpoint successfully supplied the release source. (convex/httpApiV1/shared.ts, 87ca030c30f3)
• Cleanup extends beyond the commit boundary: The new keepStoredFiles flag becomes true only after the entire publishing helper resolves. However, the helper commits insertVersion and then awaits scan scheduling; those scheduler calls can throw before control returns to the handler. (convex/lib/skillPublish.ts:581, d4bcf53fd927)
• Committed files are durable version references: insertVersion writes args.files into skillVersions. A later action failure does not undo that completed mutation; deleting the files leaves the persisted version pointing at missing storage. (convex/skills.ts:12993, d4bcf53fd927)
• Existing alternatives have different ownership boundaries: The scan parser already cleans up interrupted uploads, as the contributor notes. Direct skill-upload tickets separately expire unconsumed files and are consumed atomically with the version; neither mechanism cleans this multipart request path. (specs/security-moderation.md:328, d4bcf53fd927)

Likely related people:

• unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

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

History

Review history (74 earlier review cycles; latest 8 shown)

• reviewed 2026-09-03T21:36:39.029Z sha b702bc1 :: needs real behavior proof before merge. :: [P1] Preserve blobs after the version commit | [P3] Record the multipart blob-ownership invariant
• reviewed 2026-09-03T22:40:56.933Z sha b702bc1 :: needs real behavior proof before merge. :: [P1] Preserve blobs once version insertion commits | [P3] Record the multipart blob-ownership invariant
• reviewed 2026-09-04T04:52:30.000Z sha b702bc1 :: needs real behavior proof before merge. :: [P1] Do not delete blobs after version insertion commits | [P3] Record the multipart blob-ownership invariant
• reviewed 2026-09-04T21:01:22.732Z sha b702bc1 :: needs real behavior proof before merge. :: [P1] Do not delete blobs after a version has committed | [P3] Document the multipart blob-lifetime invariant
• reviewed 2026-09-04T23:01:52.031Z sha b702bc1 :: needs real behavior proof before merge. :: [P1] Preserve blobs after the version transaction commits
• reviewed 2026-09-05T01:59:09.645Z sha b702bc1 :: needs real behavior proof before merge. :: [P1] Preserve uploaded blobs once the version transaction commits
• reviewed 2026-09-05T03:10:32.473Z sha b702bc1 :: needs real behavior proof before merge. :: [P1] Preserve uploaded blobs once the version transaction commits
• reviewed 2026-09-07T19:16:16.416Z sha d4bcf53 :: needs real behavior proof before merge. :: [P1] Preserve uploaded blobs once the version transaction commits

[SebTardif]

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Complete
• Detail: The durable review result and its route handoff completed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/34155038246
• Updated: 2026-09-07T19:18:06.486Z