Skip to content

feat(skills): publish large bundles via staged upload - #3036

Open
autogame-17 wants to merge 2 commits into
openclaw:mainfrom
autogame-17:fix/skills-staged-upload-large-bundles
Open

feat(skills): publish large bundles via staged upload#3036
autogame-17 wants to merge 2 commits into
openclaw:mainfrom
autogame-17:fix/skills-staged-upload-large-bundles

Conversation

@autogame-17

Copy link
Copy Markdown
Contributor

What

Publish large skill bundles via a staged upload, fixing the edge 413 Request Entity Too Large that blocks any skill whose bundle exceeds the inline multipart body limit.

clawhub publish posts each skill file as an individual multipart files part to POST /api/v1/skills. That request goes through the Vercel edge (/api/**server/handlers/convexProxy.ts), whose serverless body limit (~4.5MB) rejects larger bundles with a raw 413 before the request ever reaches Convex — even though the app-level skill limits (MAX_PUBLISH_FILE_BYTES 10MB / MAX_PUBLISH_TOTAL_BYTES 50MB) are far higher. Packages already solve this with a staged upload-url + storage-ticket flow; skills had no equivalent.

This PR adds the same staged path for skills:

  1. POST /api/v1/skills/-/upload-url mints a one-shot storage upload URL + ticket bound to the publishing user.
  2. The CLI zips the bundle, PUTs it straight to Convex storage (bypassing the edge body limit), then publishes with a small multipart/form-data request carrying payload + bundleStorageId + bundleUploadTicket (no inline files).
  3. The server consumes the ticket, unzips the staged bundle, and runs every entry through the same per-file size gate and storage persistence as the inline multipart path, so validation is identical regardless of transport.

The CLI switches to the staged path automatically once a bundle exceeds ~4MB of content; smaller bundles keep the existing inline multipart path byte-for-byte unchanged.

Why this shape

I mirrored the existing package staged-upload flow rather than inventing a new one:

  • Ticket security model copied from packages (convex/uploads.ts consumePackagePublishUploadTicketInternal): the ticket is bound to the issuing user, has a 15-minute TTL, is single-use (usedAt), and the referenced storage blob must have been created after the ticket (anti-replay). Skills only authenticate via a user API token (requireApiTokenUserOrResponse), so the skill ticket is user-scoped only — there is no github-actions kind.
  • Zip, not tar: skills already round-trip zips on the download side (fflate zipSync/unzipSync), so the staged bundle reuses that format instead of introducing ClawPack tarballs.
  • Identical downstream validation: the staged parser produces the same files array shape as the inline path, so the 50MB total-bundle cap and all publish validation in skillPublish apply unchanged.

Changes

  • convex/schema.ts — new skillPublishUploadTickets table (+ retention policy in convex/lib/retentionPolicy.ts).
  • convex/uploads.tscreateSkillPublishUploadForUserInternal / consumeSkillPublishUploadTicketInternal.
  • convex/httpApiV1/skillsV1.ts — mint handler publishSkillUploadUrlV1Handler; publish handler routes multipart requests to inline vs staged based on the bundle* fields.
  • convex/httpApiV1/shared.ts — extracted buildPublishBody + parseMultipartPublishForm; added parseStagedBundlePublish and a pure, unit-testable extractSkillBundleEntries.
  • convex/http.ts, convex/httpApiV1.ts — register the new route/handler.
  • CLI (packages/clawhub/src) — skills.ts gains buildSkillBundleZip + shouldStageSkillBundle + threshold; cli/commands/publish.ts uses the staged path for large bundles.
  • Schema route skillsUploadUrl (source + built dist/routes.*), OpenAPI (public/api/v1/openapi.json), and docs/http-api.md.

Testing

  • Ran locally (green): tsc --noEmit on the app, schema, and CLI packages; oxfmt --check + oxlint on changed files; CLI package tests (bun run --cwd packages/clawhub test:src) including new skills.test.ts cases for the staging threshold and zip round-trip.
  • New server unit test convex/httpApiV1/skillBundle.test.ts covers extractSkillBundleEntries (round-trip, mac-junk skip, per-file size cap, invalid-zip rejection). I couldn't run the root convex/ vitest suite locally (the jsdom worker times out on my machine), so those run in CI.
  • Not run locally: the full e2e / handler integration suite (needs a live Convex backend + OAuth). The end-to-end staged publish is exercised by CI.

Notes

Per CONTRIBUTING, larger/architectural changes usually start with a Discord conversation — happy to move discussion there if preferred. I framed this as a bug fix (large valid skills currently can't publish at all) that deliberately reuses the established package staged-upload pattern rather than introducing a new mechanism. Feedback on the wire protocol (bundleStorageId + bundleUploadTicket multipart fields vs. a dedicated JSON shape) very welcome.

Fixes the edge 413 Request Entity Too Large that blocks publishing any skill
whose bundle exceeds the inline multipart body limit.

`clawhub publish` posts each file as an individual multipart `files` part to
POST /api/v1/skills. That request traverses the Vercel edge proxy
(/api/** -> server/handlers/convexProxy.ts), whose serverless body limit
(~4.5MB) rejects larger bundles with a raw 413 before the request reaches
Convex -- even though the app-level skill limits (10MB per file / 50MB total)
are far higher. Packages already bypass this with a staged upload-url +
storage-ticket flow; skills had no equivalent.

Add the same staged path for skills:
- POST /api/v1/skills/-/upload-url mints a one-shot storage upload URL + ticket
  bound to the publishing user (skillPublishUploadTickets, 15m TTL, single-use,
  storage blob must post-date the ticket -- mirrors the package ticket model).
- The CLI zips the bundle, PUTs it straight to storage (bypassing the edge
  body limit), then publishes with payload + bundleStorageId +
  bundleUploadTicket. Bundles under ~4MB keep the inline multipart path
  unchanged.
- The server consumes the ticket, unzips, and runs every entry through the
  same per-file size gate, path sanitization, and storage persistence as the
  inline path, so validation is identical regardless of transport.

Mirrors the existing package staged-upload flow rather than introducing a new
mechanism. Adds CLI unit tests (staging threshold + zip round-trip) and a
server unit test for the pure bundle-extraction logic; documents the new route
in the OpenAPI spec and docs/http-api.md.
@autogame-17
autogame-17 requested review from a team and Patrick-Erichsen as code owners July 9, 2026 17:48
@vercel

vercel Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@autogame-17 is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel.

A member of the Team first needs to authorize it.

knip flagged SkillBundleFile as an unused exported type; it is only used
internally by buildSkillBundleZip. Make it module-private.
@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. P2 Normal backlog priority with limited blast radius. labels Jul 9, 2026
@clawsweeper

clawsweeper Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 16, 2026, 10:32 AM ET / 14:32 UTC.

Summary
Adds an automatic staged ZIP upload path for large skill bundles across the CLI, HTTP API, Convex storage tickets and schema, public API documentation, and tests.

Reproducibility: yes. from source: publish an authenticated staged ZIP whose compressed bytes are small but whose entries expand far beyond the logical bundle limit; the synchronous unzip occurs before the per-file and total-size gates. The original edge 413 is also consistent with the documented 50 MB application limit versus the smaller proxy body limit.

Review metrics: 3 noteworthy metrics.

  • Patch surface: 19 files; 501 added, 48 removed. The change coordinates CLI, API, schema, generated routes, documentation, and tests, so partial rollout or validation can leave the publish protocol inconsistent.
  • Persistent model changes: 1 Convex table added. The new ticket table introduces deployment and retention behavior that must be validated with the endpoint and cleanup lifecycle.
  • New transport surface: 1 endpoint and 1 automatic CLI branch. Large publishes switch protocols automatically, making real fresh-install and upgraded-deployment proof important before release.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P1] Reject excessive archive expansion and entry counts before allocating uncompressed contents, with focused regression coverage.
  • Post redacted live output or logs showing an actual bundle above 4 MB publishes successfully through the staged path and that an expansion-limit case fails safely.
  • Remove the CHANGELOG.md edit while retaining release-note context in the PR body or commit message.

Proof guidance:

  • [P1] Needs real behavior proof before merge: No after-fix terminal output, live logs, recording, or linked artifact shows a real bundle above 4 MB completing the upload URL, storage PUT, ticket consumption, and publish sequence; add redacted proof, update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review if it does not rerun automatically.

Risk before merge

  • [P1] A small authenticated ZIP with a very large expansion ratio could exhaust Convex action memory before the 50 MB logical bundle limit is checked, degrading or terminating publish handling.
  • [P1] The new endpoint, storage PUT, ticket-consumption, archive extraction, and publish sequence has not been demonstrated against a real deployment, so operational failures or cleanup gaps remain unproven despite green CI.

Maintainer options:

  1. Harden and prove the staged flow (recommended)
    Bound archive expansion before allocation, remove the release-owned changelog edit, and add redacted live output showing a large publish succeeds and an expansion-limit case fails safely.
  2. Pause the staged endpoint
    Keep this PR open but do not merge the new upload route until the contributor can provide bounded extraction and real deployment evidence.

Next step before merge

  • [P1] Contributor follow-up is required to fix the archive-expansion blocker, remove the changelog edit, and provide real deployment proof; automation cannot supply proof from the contributor's publishing environment.

Security
Needs attention: The authenticated upload path introduces a concrete resource-exhaustion concern because archive expansion is not bounded before allocation.

Review findings

  • [P1] Bound staged archives before expanding them — convex/httpApiV1/shared.ts:434
  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:9
Review details

Best possible solution:

Keep the package-aligned staged transport, but reject excessive declared expansion, entry counts, and total uncompressed bytes before allocating archive contents, preserve the existing inline path, and demonstrate both a successful bundle above 4 MB and bounded rejection of an oversized expansion on a real deployment.

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

Yes, from source: publish an authenticated staged ZIP whose compressed bytes are small but whose entries expand far beyond the logical bundle limit; the synchronous unzip occurs before the per-file and total-size gates. The original edge 413 is also consistent with the documented 50 MB application limit versus the smaller proxy body limit.

Is this the best way to solve the issue?

No, not yet. Reusing the established ticketed storage-upload pattern is maintainable, but synchronous unbounded expansion and the absence of real deployment proof prevent this implementation from being the safe final solution.

Full review comments:

  • [P1] Bound staged archives before expanding them — convex/httpApiV1/shared.ts:434
    unzipSync materializes every entry before the per-file and downstream total-size checks run. An authenticated publisher can therefore upload a highly compressed archive that expands far beyond the 50 MB bundle limit and exhausts the action's memory before rejection; inspect bounded archive metadata or use bounded extraction so entry count and cumulative uncompressed bytes are rejected before allocation.
    Confidence: 0.98
  • [P3] Remove the release-owned changelog entry — CHANGELOG.md:9
    Repository policy reserves CHANGELOG.md for the release process, and the PR body already contains the needed release-note context. Remove this branch edit rather than creating a release-owned entry in a normal feature PR.
    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 high; reviewed against bf9c3be4a7b0.

Label changes

Label justifications:

  • P2: The PR addresses a real but size-dependent publishing failure with limited blast radius, while its current blocker is merge safety rather than an active core outage.
  • merge-risk: 🚨 availability: Eagerly expanding an attacker-controlled staged ZIP before enforcing total bounds can exhaust Convex action memory and disrupt publish availability.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab 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: No after-fix terminal output, live logs, recording, or linked artifact shows a real bundle above 4 MB completing the upload URL, storage PUT, ticket consumption, and publish sequence; add redacted proof, update the PR body to trigger review, or ask a maintainer to comment @clawsweeper re-review if it does not rerun automatically.
Evidence reviewed

Security concerns:

  • [high] ZIP expansion can exhaust action memory — convex/httpApiV1/shared.ts:434
    The server fully expands publisher-controlled ZIP data before enforcing logical file and bundle limits, so a high-ratio archive can consume excessive memory and impair publish availability.
    Confidence: 0.98

What I checked:

  • Documented behavior gap: Current documentation permits skill bundles up to 50 MB, while the existing multipart publishing path can be rejected by the edge around 4.5 MB, so the underlying problem is credible and remains worth fixing. (docs/skill-format.md, bf9c3be4a7b0)
  • Unbounded archive expansion: The proposed parser uses synchronous full ZIP expansion before its per-file and downstream total-size validation, allowing a small compressed upload to consume substantially more action memory than the documented 50 MB limit. (convex/httpApiV1/shared.ts:434, 7f85e1f6cc95)
  • Re-review continuity: The current head is identical to the previously reviewed SHA, so the prior P1 archive-expansion blocker and release-owned changelog finding remain unresolved rather than being newly discovered concerns. (convex/httpApiV1/shared.ts:434, 7f85e1f6cc95)
  • Proof remains absent: The PR reports unit, type, lint, and CI validation but explicitly says the live staged publish flow was not run locally; no redacted terminal transcript, logs, recording, or linked artifact shows a bundle above 4 MB completing upload URL creation, storage PUT, ticket consumption, and publication. (7f85e1f6cc95)
  • Release-owned changelog: The branch still edits CHANGELOG.md even though repository policy reserves that file for the release process and directs normal PRs to provide release-note context in the PR body or commit message instead. (CHANGELOG.md:9, 7f85e1f6cc95)
  • No canonical replacement: The supplied related items concern publisher ownership, web discovery, and prepublication state recovery rather than the edge body-size failure; none supersedes this staged-upload work.

Likely related people:

  • Peter Steinberger: Repository history records connect Peter to release preparation and adjacent upload, moderation, and skill-publishing infrastructure, making him a plausible routing candidate for the Convex availability boundary and release-owned changelog policy. (role: adjacent upload and release-history contributor; confidence: medium; commits: 8752e4bb7e92, 232e429dee6f; files: convex/uploads.ts, convex/httpApiV1/skillsV1.ts, CHANGELOG.md)
  • joshp123: Prior merged registry work credits joshp123 for publish and backup behavior, providing relevant historical context for changes to the skill publication pipeline. (role: historical registry publishing contributor; confidence: low; files: convex/httpApiV1/skillsV1.ts, packages/clawhub/src/cli/commands/publish.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.
Review history (3 earlier review cycles)
  • reviewed 2026-07-09T19:07:38.578Z sha 7f85e1f :: needs real behavior proof before merge. :: [P3] Remove the release-owned changelog entry
  • reviewed 2026-07-09T20:15:15.022Z sha 7f85e1f :: needs real behavior proof before merge. :: [P1] Bound staged bundles before unzipping them | [P3] Remove the release-owned changelog entry
  • reviewed 2026-07-16T12:59:05.130Z sha 7f85e1f :: needs real behavior proof before merge. :: [P1] Bound staged archives before expanding them | [P3] Remove the release-owned changelog entry

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. label Jul 9, 2026
@clawsweeper

clawsweeper Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: feat(skills): publish large bundles via staged upload This is item 1/1 in the current shard. Shard 12/24.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

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

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal backlog priority with limited blast radius. 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.

1 participant