fix(import): delete GitHub skill blobs when import fails

[SebTardif]

What Problem This Solves

Fixes an issue where users importing a GitHub skill would leave Convex storage blobs behind when the import failed. The GitHub import action stored each selected file, then checked slug, owner, display name, and version. An invalid version, a later store failure, or a publish failure never deleted those blobs. They were not attached to a skill version, so they stayed billed and unreferenced.

Why This Change Was Made

GitHub import now checks slug, owner, display name, and version before any storage.store. Store and publish run inside one cleanup boundary. If store or publish fails, every stored id is deleted. A successful publish keeps the blobs.

User Impact

Failed GitHub skill imports no longer leave orphaned upload blobs in Convex storage. Successful imports are unchanged.

Evidence

Live bun on Windows 11, worktree C:/Users/sebta/.grok/tmp/pr-gate-batch/clawhub-f006. The script calls production importGitHubSkillForUser with in-memory storage and a local GitHub zip. Invalid version never stores. Publish failure stores storage:1 and storage:2, then deletes both. A later store failure stores storage:1 and deletes it.

$ bun C:/tmp/clawhub-f006-proof.mjs
invalid_version error=Version must be valid semver storeCalls=- deleteCalls=-
publish_fail error=Import failed during publish: slug exists. Check skill format, slug availability, and try again. storeCalls=storage:1,storage:2 deleteCalls=storage:1,storage:2
mid_store_fail error=Failed to store file "notes.md" (6 bytes). disk full storeCalls=storage:1 deleteCalls=storage:1

Invalid version prints storeCalls=-. Publish failure prints deleteCalls=storage:1,storage:2. Mid-store failure prints deleteCalls=storage:1.

Real behavior proof

• Behavior or issue addressed: Failed GitHub skill import now deletes Convex blobs that were stored before publish finished.
• Real environment tested: Windows 11 10.0.26200, bun 1.4.1, Node v24.19.0, worktree C:/Users/sebta/.grok/tmp/pr-gate-batch/clawhub-f006 on fix/github-import-blob-cleanup. Proof imported production importGitHubSkillForUser from convex/githubImport.ts.
• Exact steps or command run after this patch: Ran bun C:/tmp/clawhub-f006-proof.mjs. The script imported a two-file GitHub zip through production importGitHubSkillForUser with in-memory storage: invalid version, publish throw after store, and a later store throw after the first file.
• Evidence after fix: terminal output copied above. After the patch, invalid version prints storeCalls=- deleteCalls=-. Publish failure prints storeCalls=storage:1,storage:2 deleteCalls=storage:1,storage:2. Mid-store failure prints storeCalls=storage:1 deleteCalls=storage:1.
• Observed result after fix: Invalid metadata never writes a blob. A publish failure or a later store failure removes every id that was stored.
• What was not tested: A live clawhub.ai GitHub import with a signed-in GitHub account. Successful import retention of stored blobs on the hosted backend (the local helper path keeps them when publish returns).

Notes

Same cleanup contract as skill-scan upload in convex/httpApiV1/shared.ts and deleteStoredGitHubSkillScanFiles in convex/githubSkillSync.ts. Sibling multipart skill publish cleanup is PR #3549. This PR only changes GitHub import.

Allow edits from maintainers is enabled.

Tracker

Ref #3672

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 validates GitHub skill import metadata before storing files and deletes newly stored blobs when storage or publication fails, with three regression tests.

Merge readiness

⛔ Blocked before merge - 7 items remain

This remains useful: current main and v0.23.3 still lack failed-import cleanup. The previous blocking finding remains unresolved, and the supplied in-memory demonstration does not satisfy the real Convex proof requirement.

Priority: P2
Reviewed head: 094ebd4deb584f869c12f0e5a772f95a7014d0e0

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The focused leak repair has useful regression coverage, but retains a destructive post-commit cleanup defect and lacks real storage proof.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: The captured Windows/Bun transcript invokes the production import helper with in-memory storage and simulated publication failures. It proves callback behavior, but not real Convex deletion or preservation of committed files; the scoped repository guidance requires real-backend evidence. 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: The captured Windows/Bun transcript invokes the production import helper with in-memory storage and simulated publication failures. It proves callback behavior, but not real Convex deletion or preservation of committed files; the scoped repository guidance requires real-backend evidence. 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	Introduced cleanup boundary: The pinned merge-base-to-head patch adds unconditional deletion after any exception from storage or publishVersionForUser. Local diff totals match the supplied introduction evidence: two files, 247 additions and 73 deletions. Publication can throw after committing: The non-staged publication path awaits insertVersion before awaiting scheduleSkillPublishFollowups. Its security-scan scheduling calls can propagate exceptions. insertVersion persists args.files in skillVersions, while file preview later fails when its referenced blob is missing. Current main still needs the repair: Current main stores files before metadata validation and has no cleanup in this import failure path. Its publication helper also retains the commit-then-schedule sequence.
Findings	1 actionable finding	[P1] Preserve blobs once publication has committed
Security	None	None.

How this fits together

ClawHub imports selected files from a publisher’s public GitHub repository into Convex storage, then publishes a skill version referencing those files. Publication also schedules security scans and other follow-up work.

flowchart TD
  A[GitHub repository and selected files] --> B[Validate identity and metadata]
  B --> C[Store file blobs]
  C --> D[Commit skill version]
  D --> E[Schedule follow-up work]
  C -->|Failure| F[Import cleanup]
  E -->|Failure propagates| F
  F --> G[Delete recorded blobs]

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The captured Windows/Bun transcript invokes the production import helper with in-memory storage and simulated publication failures. It proves callback behavior, but not real Convex deletion or preservation of committed files; the scoped repository guidance requires real-backend evidence. 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 blobs once publication has committed (P1) - When staged publication is disabled, publishVersionForUser commits skills.insertVersion before awaiting security-scan scheduling (convex/lib/skillPublish.ts:580–585,884–905). If scheduling rejects, this catch deletes all imported blobs even though the committed version still references them, causing subsequent file reads to fail. This is the still-unfixed finding from the previous review. Track durable ownership at the commit boundary and test a post-commit scheduling failure; a success flag set only after the helper returns is insufficient.
• Resolve merge risk (P1) - Merging the unconditional cleanup can leave a committed skill version referencing deleted files when follow-up scheduling fails.
• Resolve merge risk (P1) - Actual Convex blob cleanup and committed-file retention remain unproven by the submitted in-memory run.
• Complete next step (P2) - Fix the post-commit deletion finding and add real Convex cleanup/retention evidence. Terminal output or logs are suitable; 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.
• Improve patch quality - Repair the commit boundary and add regression coverage that keeps committed version files readable after follow-up scheduling fails.
• Improve patch quality - Provide redacted real Convex evidence of failed-import cleanup and successful or committed publication retention.

Findings

• [P1] Preserve blobs once publication has committed — convex/githubImport.ts:404-406

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch size	+247/-73 across 2 files	The patch is confined to the GitHub import implementation and its tests.
Production versus tests	Production net +6; tests +168	Production growth supports the stated cleanup boundary, while the added tests cover three simulated failure cases.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #3672
Summary: This PR is the candidate implementation for the explicit GitHub import blob-leak tracker.

Members:

• canonical: GitHub skill import leaves Convex blobs after a failed import #3672 - The tracker names this implementation and includes the missing committed-file retention requirement.
• adjacent_distinct: fix: delete unpublished skill blobs when publish fails #3549 - Multipart publishing has a separate upload entrypoint and does not cover GitHub import.
• adjacent_distinct: fix: delete unpublished package zip on publish failure #3622 - Package ZIP publication uses a different storage and persistence path.
• adjacent_distinct: GitHub import unzips the archive before size limits #3678 - Archive inflation limits concern an earlier import stage, independently of blob cleanup.

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

Merge-risk options

Maintainer options:

1. Respect the publication commit boundary (recommended)
   Repair cleanup ownership and demonstrate that a scheduling failure after publication leaves every referenced file readable.

Technical review

Best possible solution:

Make cleanup follow durable file ownership: delete only unclaimed import blobs and preserve files once a pending or published version owns them.

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

Yes, source establishes both the current-main leak and the introduced failure path: commit a non-staged version, then let follow-up scheduling reject. No runtime reproduction was executed during this read-only review.

Is this the best way to solve the issue?

No. Earlier validation is appropriate, but catching the entire publishing helper confuses failure with lack of commitment; cleanup must distinguish unclaimed files from files already persisted on a version.

Full review comments:

• [P1] Preserve blobs once publication has committed — convex/githubImport.ts:404-406
  When staged publication is disabled, publishVersionForUser commits skills.insertVersion before awaiting security-scan scheduling (convex/lib/skillPublish.ts:580–585,884–905). If scheduling rejects, this catch deletes all imported blobs even though the committed version still references them, causing subsequent file reads to fail. This is the still-unfixed finding from the previous review. Track durable ownership at the commit boundary and test a post-commit scheduling failure; a success flag set only after the helper returns is insufficient.
  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 addresses a bounded authenticated-import storage leak rather than an established live service emergency.
• merge-risk: 🚨 compatibility: The new catch can delete files belonging to an already committed version and break subsequent file access.
• 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: The captured Windows/Bun transcript invokes the production import helper with in-memory storage and simulated publication failures. It proves callback behavior, but not real Convex deletion or preservation of committed files; the scoped repository guidance requires real-backend evidence. 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:

• Introduced cleanup boundary: The pinned merge-base-to-head patch adds unconditional deletion after any exception from storage or publishVersionForUser. Local diff totals match the supplied introduction evidence: two files, 247 additions and 73 deletions. (convex/githubImport.ts:405, 094ebd4deb58)
• Publication can throw after committing: The non-staged publication path awaits insertVersion before awaiting scheduleSkillPublishFollowups. Its security-scan scheduling calls can propagate exceptions. insertVersion persists args.files in skillVersions, while file preview later fails when its referenced blob is missing. (convex/lib/skillPublish.ts:581, 094ebd4deb58)
• Current main still needs the repair: Current main stores files before metadata validation and has no cleanup in this import failure path. Its publication helper also retains the commit-then-schedule sequence. (convex/githubImport.ts:308, cbfee7343ddc)
• Latest release comparison: The supplied latest release, v0.23.3, also contains the store-before-validation implementation without failed-import deletion; the central repair is not already present there. (convex/githubImport.ts:308, 87ca030c30f3)
• Review continuity: The previous completed review raised “Preserve blobs once publication has committed” against the same head. The file comparison is empty, and the defect remains present. REST verification confirms this PR is open and unmerged at that head. (convex/githubImport.ts:405, 094ebd4deb58)
• Proof scope and repository requirement: The complete supplied PR body, captured under sourceRevision 4bb020c50ba198d0e5692f79fd69f5d5e774d206bec2f1502fd4aef61ee663e7, describes Windows/Bun execution with in-memory storage and a local ZIP. It shows callback deletion on simulated errors but explicitly excludes hosted storage retention. The ClawHub Convex skill requires real Convex validation for storage and action-to-function behavior. (.agents/skills/clawhub-convex/SKILL.md, 094ebd4deb58)

Likely related people:

• Patrick-Erichsen: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• vyctorbrzezowski: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; 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 (1 earlier review cycle)

• reviewed 2026-09-05T12:20:46.539Z sha 094ebd4 :: needs real behavior proof before merge. :: [P1] Preserve blobs once publication has committed