Skip to content

fix(api): route direct downloads through Nitro - #3506

Open
Patrick-Erichsen wants to merge 2 commits into
mainfrom
fix/stream-direct-download
Open

fix(api): route direct downloads through Nitro#3506
Patrick-Erichsen wants to merge 2 commits into
mainfrom
fix/stream-direct-download

Conversation

@Patrick-Erichsen

@Patrick-Erichsen Patrick-Erichsen commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • redirect direct production Convex-site download requests to the canonical ClawHub API origin before rate limiting, database reads, or storage access
  • keep authenticated Nitro manifest requests and explicit loopback downloads on their existing paths while covering hosted Convex custom domains
  • document that all production archive bytes must cross the existing signed Convex-to-Nitro manifest boundary

Evidence

  • Axiom recorded 2,029 HttpResponseTooLarge failures for direct GET /api/v1/download in 2026-08-20T15:19:31Z..2026-08-21T17:54:10Z, through 2026-08-21T17:09:27Z, at Convex's 20 MiB response ceiling.
  • #3451 and #3498 are merged; there is no open duplicate download-streaming PR.
  • Current main at 845c6d3bdb1a36573d8d28be2a8fb85a3c476720 still returned the direct ZIP body from convex/downloads.ts when the internal manifest header was absent.
  • Red: the public-handler regression expected 307 and received 404 after the request entered Convex backend work.
  • Green: the same request redirects to https://clawhub.ai/api/v1/download with its query intact and performs no rate-limit mutation or database query.
  • First-head hosted red: local-auth version deletion expected direct local Convex status 410 and received 503 because the redirect also caught loopback. The final patch exempts only explicit loopback hosts; a custom-domain regression proves non-local hosted origins still redirect.

Validation

  • bunx vitest run convex/downloads.test.ts server/convexProxy.test.ts - 56 passed
  • three initially starved full-suite timeout cases rerun individually - 3 passed
  • bun run ci:static - passed
  • bun run ci:unit - 6,216 passed, 3 skipped; coverage gate passed
  • bun run ci:types-build - passed
  • bun run ci:e2e-http - passed
  • .agents/skills/autoreview/scripts/autoreview --mode local --stream-engine-output - clean after one accepted custom-domain finding (0.96 confidence)

Local Convex function preparation was attempted against 127.0.0.1; the copied fixture snapshot failed schema validation because it contains the unrelated skillVersions.aigAnalysis field missing from current main. No cloud deployment, production mutation, monitor change, merge, or release was performed.

Exact head: f02c0bfa604b366a8cc0ee23e6ced7a1c337ac17

Punchcard-Session: calm-lantern-orchard-96

Punchcard-Session: calm-lantern-orchard-96
@clawsweeper

clawsweeper Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

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

@vercel

vercel Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clawhub Ready Ready Preview Aug 21, 2026 7:02pm

Request Review

@blacksmith-sh

This comment has been minimized.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 21, 2026
@clawsweeper

clawsweeper Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed August 22, 2026, 11:34 AM ET / 15:34 UTC.

ClawSweeper review

What this changes

The PR redirects direct hosted Convex archive-download requests to ClawHub’s Nitro API while retaining local and internal manifest paths.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep open: the PR addresses a real hosted-download availability problem, but its new redirect runs after the globally registered route limiter and therefore cannot redirect callers already over quota.

Priority: P1
Reviewed head: f02c0bfa604b366a8cc0ee23e6ced7a1c337ac17

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The patch has a focused, evidence-backed goal but a source-proven route-ordering blocker remains.
Proof confidence 🌊 off-meta tidepool Not applicable: This collaborator-authored backend PR is exempt from the external-contributor proof gate; its CI and handler tests remain supplemental to the required route-level regression.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This collaborator-authored backend PR is exempt from the external-contributor proof gate; its CI and handler tests remain supplemental to the required route-level regression.
Evidence reviewed 6 items Current main still needs this work: Current main proceeds from manifest detection into download processing without the hosted-origin redirect, so the central behavior is not already implemented.
Redirect is too late in the registered path: The added redirect is inside the handler, but the global wrapper applies its limiter before invoking that handler; an exhausted hosted caller receives 429 rather than 307.
Route registration proves the wrapper applies: The download route is registered after installRateLimitedRoutes wraps every route, and the default policy classifies it as the download limiter.
Findings 1 actionable finding [P2] Bypass the route limiter before hosted redirects
Security None None.

Live Verification

Command: bunx vitest run convex/downloads.test.ts convex/lib/httpRouteRateLimit.test.ts

Result: PASS (completed)

bunx vitest run convex/downloads.test.ts convex/lib/httpRouteRateLimit.test.ts
runner@runnervm76f27:/tmp/clawsweeper-live-proof-3506-2RL9uZ/target$ bunx vitest run convex/downloads.test.ts convex/lib/httpRouteRateLimit.test.ts

 RUN  v4.1.10 /tmp/clawsweeper-live-proof-3506-2RL9uZ/target

bunx vitest run convex/downloads.test.ts convex/lib/httpRouteRateLimit.test.ts
stdout | convex/lib/httpRouteRateLimit.test.ts › rateLimitedHttpAction › blocks before the wrapped HTTP handler runs
rate_limit_denied {
  kind: 'read',
  auth: false,
  userAllowed: null,
  ipAllowed: false,
  ipSource: 'none',
  hasClientIp: false
}

stdout | convex/lib/httpRouteRateLimit.test.ts › installRateLimitedRoutes › wraps ordinary route registrations with the default method limit
rate_limit_denied {
  kind: 'read',
  auth: false,
  userAllowed: null,
  ipAllowed: false,
  ipSource: 'none',
  hasClientIp: false
}

 ✓ convex/lib/httpRouteRateLimit.test.ts (22 tests) 29ms
 ✓ convex/downloads.test.ts (31 tests) 352ms

 Test Files  2 passed (2)
      Tests  53 passed (53)
   Start at  15:34:26
   Duration  2.55s (transform 765ms, setup 62ms, import 2.55s, tests 381ms, environment 791ms)

runner@runnervm76f27:/tmp/clawsweeper-live-proof-3506-2RL9uZ/target$ bunx vitest run convex/downloads.test.ts convex/lib/httpRouteRateLimit.test.ts
















Assertions:

  • PASS expect_output: Test Files

How this fits together

ClawHub’s download endpoint resolves and authorizes a skill archive in Convex, then Nitro streams the ZIP so archive bytes do not exceed Convex response limits. Requests enter a global HTTP route policy before reaching the download handler.

flowchart LR
  A[Hosted download request] --> B[Global HTTP route policy]
  B --> C[Download handler]
  C --> D{Manifest or local host?}
  D -->|Hosted direct request| E[Nitro download API]
  E --> F[Signed archive manifest]
  F --> G[Streaming ZIP response]
Loading

Before merge

  • Bypass the route limiter before hosted redirects (P2) - installRateLimitedRoutes applies the download limiter before downloadZipHandler runs. A hosted caller already over quota therefore gets 429 instead of this 307, contradicting the intended before-rate-limit redirect; classify this narrow request shape in route policy and test the registered route.
  • Resolve merge risk (P1) - A hosted Convex caller already over the download quota receives 429 and consumes limiter state before it can reach the intended 307 redirect.
  • Complete next step (P2) - The remaining blocker is a narrow mechanical route-policy repair with a clear regression test boundary.

Findings

  • [P2] Bypass the route limiter before hosted redirects — convex/downloads.ts:88-98
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch size production +16/-1, tests +54, spec +3 The focused scope is appropriate, but it currently omits the central route-policy test surface.
Reported production failures 2,029 HttpResponseTooLarge failures The PR body reports repeated direct-download failures at Convex’s response ceiling, supporting urgent review once the limiter ordering is repaired.

Merge-risk options

Maintainer options:

  1. Move hosted redirects ahead of rate limiting (recommended)
    Add a narrow route-policy exemption for the hosted direct-download redirect and prove an exhausted caller still receives 307 without a limiter mutation.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Update the registered route policy so only hosted, non-manifest direct downloads bypass the limiter before the 307; preserve local and manifest rate limiting, and add an exhausted-limit regression through the registered route.

Technical review

Best possible solution:

Classify only hosted, non-manifest direct downloads as redirect-only before the global limiter, while preserving rate limits for local and signed-manifest requests.

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

Yes—source inspection shows the registered route limiter returns before the handler’s redirect branch for an over-quota hosted request; the current handler-only tests do not exercise that path.

Is this the best way to solve the issue?

No—the redirect must be decided in the pre-handler route policy, then covered by a registered-route regression that verifies a 307 and no limiter mutation.

Full review comments:

  • [P2] Bypass the route limiter before hosted redirects — convex/downloads.ts:88-98
    installRateLimitedRoutes applies the download limiter before downloadZipHandler runs. A hosted caller already over quota therefore gets 429 instead of this 307, contradicting the intended before-rate-limit redirect; classify this narrow request shape in route policy and test the registered route.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against fb0ef1d21eab.

Labels

Label justifications:

  • P1: The PR targets recurring production download failures that block archive delivery.
  • merge-risk: 🚨 compatibility: Hosted direct-download callers change from a direct response to a redirect and need the intended route behavior preserved.
  • merge-risk: 🚨 availability: The current ordering leaves over-quota hosted callers unable to reach Nitro despite the redirect fix.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This collaborator-authored backend PR is exempt from the external-contributor proof gate; its CI and handler tests remain supplemental to the required route-level regression.

Evidence

Acceptance criteria:

  • [P1] bunx vitest run convex/downloads.test.ts convex/lib/httpRouteRateLimit.test.ts.
  • [P1] bun run ci:unit.
  • [P1] bun run ci:types-build.

What I checked:

  • Current main still needs this work: Current main proceeds from manifest detection into download processing without the hosted-origin redirect, so the central behavior is not already implemented. (convex/downloads.ts:85, fb0ef1d21eab)
  • Redirect is too late in the registered path: The added redirect is inside the handler, but the global wrapper applies its limiter before invoking that handler; an exhausted hosted caller receives 429 rather than 307. (convex/downloads.ts:88, f02c0bfa604b)
  • Route registration proves the wrapper applies: The download route is registered after installRateLimitedRoutes wraps every route, and the default policy classifies it as the download limiter. (convex/http.ts:97, fb0ef1d21eab)
  • Existing tests bypass the affected layer: The added tests invoke downloadZipHandler directly, so their no-mutation assertions cannot establish the behavior of the registered, rate-limited route. (convex/downloads.test.ts:115, f02c0bfa604b)
  • Route-policy provenance: Blame attributes the registration wrapper and download-rate classification to the route-policy introduction commit, identifying the adjacent ownership surface for the repair. (convex/lib/httpRouteRateLimit.ts:34, 307470174063)
  • Autoreview helper result: The requested helper was invoked on the committed PR head but stopped before engine review because TruffleHog is unavailable in this review environment; the routing finding is independently source-proven. (f02c0bfa604b)

Likely related people:

  • Patrick Erichsen: Introduced the merged Nitro streaming boundary and has the most sustained recent history in the download handler. (role: streaming download feature owner; confidence: high; commits: 60b02c09f936, f02c0bfa604b; files: convex/downloads.ts, specs/spec.md)
  • Vyctor H. Brzezowski: Git blame ties the route-registration wrapper and download limiter classification to this area of HTTP policy. (role: central route-policy introducer; confidence: high; commits: 307470174063; files: convex/lib/httpRouteRateLimit.ts)

Rank-up moves

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

  • Move hosted direct-download classification into the global route policy and add a registered-route over-quota regression.
  • Re-run the focused route and download tests after the repair.

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 (16 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-22T02:39:12.689Z sha f02c0bf :: needs changes before merge. :: [P2] Exempt hosted redirects before the route limiter
  • reviewed 2026-08-22T04:37:48.136Z sha f02c0bf :: needs changes before merge. :: [P2] Exempt hosted redirects before the route limiter
  • reviewed 2026-08-22T06:10:52.692Z sha f02c0bf :: needs changes before merge. :: [P2] Bypass rate limiting before hosted redirects
  • reviewed 2026-08-22T07:37:10.554Z sha f02c0bf :: needs changes before merge. :: [P2] Bypass rate limiting before hosted redirects
  • reviewed 2026-08-22T09:36:11.065Z sha f02c0bf :: needs changes before merge. :: [P2] Bypass rate limiting before hosted redirects
  • reviewed 2026-08-22T10:57:04.579Z sha f02c0bf :: needs changes before merge. :: [P2] Bypass rate limiting before hosted redirects
  • reviewed 2026-08-22T12:37:57.445Z sha f02c0bf :: needs changes before merge. :: [P2] Bypass rate limiting before hosted redirects
  • reviewed 2026-08-22T14:08:46.853Z sha f02c0bf :: needs changes before merge. :: [P2] Bypass the route limiter before hosted redirects

Punchcard-Session: calm-lantern-orchard-96
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 21, 2026
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. 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. 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