fix(api): route direct downloads through Nitro - #3506
Conversation
Punchcard-Session: calm-lantern-orchard-96
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment has been minimized.
This comment has been minimized.
|
Codex review: needs changes before merge. Reviewed August 22, 2026, 11:34 AM ET / 15:34 UTC. ClawSweeper reviewWhat this changesThe PR redirects direct hosted Convex archive-download requests to ClawHub’s Nitro API while retaining local and internal manifest paths. Merge readinessKeep 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 Review scores
Verification
Live VerificationCommand: Result: PASS (completed) Assertions:
How this fits togetherClawHub’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]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest 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:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fb0ef1d21eab. LabelsLabel justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (16 earlier review cycles; latest 8 shown)
|
Punchcard-Session: calm-lantern-orchard-96
Summary
Evidence
HttpResponseTooLargefailures for directGET /api/v1/downloadin2026-08-20T15:19:31Z..2026-08-21T17:54:10Z, through2026-08-21T17:09:27Z, at Convex's 20 MiB response ceiling.mainat845c6d3bdb1a36573d8d28be2a8fb85a3c476720still returned the direct ZIP body fromconvex/downloads.tswhen the internal manifest header was absent.307and received404after the request entered Convex backend work.https://clawhub.ai/api/v1/downloadwith its query intact and performs no rate-limit mutation or database query.410and received503because 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 passedbun run ci:static- passedbun run ci:unit- 6,216 passed, 3 skipped; coverage gate passedbun run ci:types-build- passedbun 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 unrelatedskillVersions.aigAnalysisfield missing from currentmain. No cloud deployment, production mutation, monitor change, merge, or release was performed.Exact head:
f02c0bfa604b366a8cc0ee23e6ced7a1c337ac17Punchcard-Session: calm-lantern-orchard-96