fix: delete skill-card blob when attach fails

[SebTardif]

What Problem This Solves

The skill-card worker stores generated skill-card.md in Convex storage, then attaches it to the skill version. If attach fails (stale lease or a deleted version), that blob is never deleted. Retries store another orphan. Storage grows on failed worker completions.

Evidence

Live bun run of completeSkillCardJob against this branch. storage.store returned _storage:proof-card. Attach threw Lease mismatch. The handler then called storage.delete on that id.

$ bun proof-f008.mts
{
  "events": [
    "store",
    "attach",
    "delete:_storage:proof-card"
  ],
  "threw": "Lease mismatch",
  "elapsedMs": 3
}

On unfixed main, the same attach failure stops after store and attach. There is no storage.delete in convex/skillCards.ts.

Real behavior proof

• Behavior or issue addressed: When skill-card attach fails after store, the new Convex blob is deleted instead of left unreferenced.
• Real environment tested: Windows 11, Bun 1.4.1, worktree fix/skill-card-store-cleanup on origin/main 0b34234f, live bun calling completeSkillCardJob with a throwing attach mutation.
• Exact steps or command run after this patch: bun proof-f008.mts from the patched worktree (proof helper not committed).
• Evidence after fix: terminal output copied below.

$ bun proof-f008.mts
{
  "events": [
    "store",
    "attach",
    "delete:_storage:proof-card"
  ],
  "threw": "Lease mismatch",
  "elapsedMs": 3
}

• Observed result after fix: Store still happens first. Attach still throws. Delete runs on _storage:proof-card. The lease error is rethrown.
• What was not tested: A live skill-card worker against production Convex with a real expired lease.

Summary

Same cleanup class as unpublished skill/package blob deletes. Successful attach still keeps the new blob. Old generated cards stay on success so historical fingerprints remain resolvable.

Tracker

Ref #3674

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 2).

ClawSweeper review

What this changes

The PR deletes newly stored skill-card Markdown when attachment fails and adds tests for failure cleanup and successful retention.

Merge readiness

⛔ Blocked before merge - 3 items remain

The fix remains necessary on current main and v0.23.3. No blocking code findings were identified, but the supplied trace does not satisfy the outstanding real Convex proof requirement.

Priority: P2
Reviewed head: 2d28ce6bc8450599d98721457e9feb7ead317156

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The focused patch has no blocking code findings, but real storage behavior remains unproven.
Proof confidence	🦪 silver shellfish (2/6)	Needs stronger real behavior proof before merge: The supplied Bun trace exercises the completion handler’s cleanup ordering, but does not show the production Convex action deleting a real stored blob after rejection or retaining it after success; the prior review’s scoped runtime request remains unresolved. 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	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The supplied Bun trace exercises the completion handler’s cleanup ordering, but does not show the production Convex action deleting a real stored blob after rejection or retaining it after success; the prior review’s scoped runtime request remains unresolved. 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	9 items	Current main still lacks cleanup: The completion action stores the blob before calling the attachment mutation without a cleanup catch. The mutation rejects mismatched leases and missing or soft-deleted versions. Latest release comparison: The supplied latest release, v0.23.3, contains the same store-then-attach path without failure cleanup. Introduced patch and regression coverage: The verified merge-base-to-head delta changes only the completion action and its tests: cleanup targets the newly allocated storage ID, preserves the original error, and leaves successful attachment unchanged. Two added tests exercise cleanup and retention with mocked context methods.
Findings	None	None.
Security	None	None.

How this fits together

ClawHub’s skill-card worker generates Markdown and submits it to a Convex action, which stores the file and attaches it to a skill version. The attachment mutation checks the job lease and preserves historical bundle fingerprints.

flowchart TD
  A[Skill-card worker] --> B[Validate worker token and Markdown]
  B --> C[Store new card blob]
  C --> D[Attach to skill version]
  D -->|Success| E[Retain card and fingerprints]
  D -->|Failure| F[Delete new blob]
  F --> G[Rethrow attachment error]

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The supplied Bun trace exercises the completion handler’s cleanup ordering, but does not show the production Convex action deleting a real stored blob after rejection or retaining it after success; the prior review’s scoped runtime request remains unresolved. 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.
• Resolve merge risk (P1) - Actual Convex blob removal after rejected attachment and retention after successful attachment remain unverified by the supplied evidence.
• Complete next step (P2) - Add real local/dev Convex evidence showing blob removal after rejected attachment and retention after success. Terminal output or redacted logs count; redact credentials, private endpoints, and other private details. Update the PR body to trigger re-review; if it does not run, ask a maintainer to comment @clawsweeper re-review.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch size	+72/-12 across 2 files; production net +5, tests +55	The small production increase implements bounded cleanup, with two focused regression tests.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #3674
Summary: This PR is the proposed fix for the explicit skill-card resource-leak tracker.

Members:

• canonical: Skill-card worker leaves a Convex blob when attach fails #3674 - The tracker names this patch and defines matching cleanup and retention acceptance criteria.
• adjacent_distinct: fix: delete unpublished package zip on publish failure #3622 - It addresses orphan package ZIPs in a different publication path.

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

Merge-risk options

Maintainer options:

1. Decide the mitigation before merge
   Keep cleanup local to the completion action, deleting only its unattached blob while preserving successful cards and historical fingerprints.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Keep cleanup local to the completion action, deleting only its unattached blob while preserving successful cards and historical fingerprints.

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

Yes, source establishes the failure path: store a card, then reject attachment for a stale lease or missing version without deleting the stored ID on current main. No runtime reproduction was executed during this read-only review.

Is this the best way to solve the issue?

Yes, this is the narrowest repair for the reported exception path and matches existing upload cleanup. A preflight lease check alone would still race with attachment; a broader storage sweeper is unnecessary for this focused fix.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

• P2: Failed worker completions can accumulate unreferenced blobs, a bounded backend resource leak.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The supplied Bun trace exercises the completion handler’s cleanup ordering, but does not show the production Convex action deleting a real stored blob after rejection or retaining it after success; the prior review’s scoped runtime request remains unresolved. 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:

• Current main still lacks cleanup: The completion action stores the blob before calling the attachment mutation without a cleanup catch. The mutation rejects mismatched leases and missing or soft-deleted versions. (convex/skillCards.ts:529, cbfee7343ddc)
• Latest release comparison: The supplied latest release, v0.23.3, contains the same store-then-attach path without failure cleanup. (convex/skillCards.ts:529, 87ca030c30f3)
• Introduced patch and regression coverage: The verified merge-base-to-head delta changes only the completion action and its tests: cleanup targets the newly allocated storage ID, preserves the original error, and leaves successful attachment unchanged. Two added tests exercise cleanup and retention with mocked context methods. (convex/skillCards.ts:532, 2d28ce6bc845)
• Runtime boundary and existing cleanup pattern: The actual worker calls completeSkillCardJob through client.action. The skill-upload HTTP path already uses best-effort deletion after failed attachment, supporting cleanup at the action that owns the newly stored blob. (scripts/skill-cards/run-skill-card-worker.ts:441, 2d28ce6bc845)
• Applicable Convex proof policy: Root AGENTS.md was read fully; no nested policy applies to the changed files. Its Convex companion guidance requires a real Convex path for storage and action-to-mutation behavior; mocked context tests cover business logic only. (.agents/skills/clawhub-convex/SKILL.md:49, 2d28ce6bc845)
• Proof and review continuity: The complete supplied body records a Windows/Bun handler invocation with an injected attachment exception and a synthetic storage ID. It shows store/attach/delete call order, but no real Convex storage observation or successful retention run. The prior completed review at this same head requested that evidence and reported no findings. A REST read confirmed the PR remains open at the pinned head and its body matches the supplied text. (2d28ce6bc845)

Likely related people:

• Patrick-Erichsen: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• jesse-merhi: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Provide a redacted local/dev Convex transcript showing failed-attachment blob removal and successful-attachment retention.

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 (1 earlier review cycle)

• reviewed 2026-09-05T16:41:04.470Z sha 2d28ce6 :: needs real behavior proof before merge. :: none