Skip to content

[codex] Fix TUS upload HEAD resume probes#2071

Merged
riderx merged 1 commit into
mainfrom
codex/fix-tus-upload-head-resume
May 7, 2026
Merged

[codex] Fix TUS upload HEAD resume probes#2071
riderx merged 1 commit into
mainfrom
codex/fix-tus-upload-head-resume

Conversation

@riderx
Copy link
Copy Markdown
Member

@riderx riderx commented May 7, 2026

Summary (AI generated)

  • Route real build-upload HEAD requests through the TUS proxy even when Tus-Resumable is absent.
  • Keep plain GET upload URLs hidden unless they are TUS-shaped HEAD fallbacks.
  • Add regression tests for route-level HEAD handling and PATCH -> HEAD -> PATCH resume behavior.

Motivation (AI generated)

The build TUS proxy could return route-level 404 for active upload URLs on HEAD, while PATCH continued to work. Standard TUS clients rely on HEAD to discover the current upload offset before resuming.

Business Impact (AI generated)

This restores resumable native build uploads, reducing failed uploads, wasted bandwidth, repeated build attempts, and support risk for customers using standard TUS clients.

Test Plan (AI generated)

  • bunx vitest run tests/build-upload-head-routing.test.ts tests/build-upload-security.test.ts
  • bun lint:backend
  • bun run supabase:db:reset
  • bunx supabase test db supabase/tests/49_test_apikey_oracle_rpc_permissions.sql --workdir .context/supabase-worktrees/7d95798b
  • pre-commit hook: bun run cli:build && vue-tsc --noEmit
  • git diff --check

Generated with AI

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Enhanced HEAD request recognition in upload routing to properly handle both standard HTTP and TUS protocol requests.
    • Improved upload state continuity tracking across multiple requests.
  • Tests

    • Added tests for upload HEAD request routing behavior.
    • Added tests for upload resume state preservation across request sequences.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9b7819f3-fb5e-4e18-a8aa-89529ff84d05

📥 Commits

Reviewing files that changed from the base of the PR and between 0f5b8d3 and 4765422.

📒 Files selected for processing (3)
  • supabase/functions/_backend/public/build/index.ts
  • tests/build-upload-head-routing.test.ts
  • tests/build-upload-security.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • supabase/functions/_backend/public/build/index.ts

📝 Walkthrough

Walkthrough

The PR enhances TUS upload HEAD request handling by modifying probe detection to recognize both actual HTTP HEAD requests and TUS-Resumable header probes, then forwards them upstream without a request body. Tests verify HEAD routing through auth middleware and proper Upload-Offset header preservation across PATCH-HEAD-PATCH request sequences.

Changes

Build Upload HEAD Handling

Layer / File(s) Summary
TUS Probe Detection Logic
supabase/functions/_backend/public/build/index.ts
isTusHeadProbe updated to return true for either actual HTTP HEAD requests or requests containing the Tus-Resumable header. Comments adjusted to reflect acceptance of both real HEAD and TUS-shaped probes forwarded upstream.
HEAD Request Routing Tests
tests/build-upload-head-routing.test.ts
New test case verifies HEAD /build/upload/:jobId/* without Tus-Resumable header is routed through auth middleware and returns status in [400, 401] instead of 404.
HEAD Forwarding Behavior Tests
tests/build-upload-security.test.ts
New test mocks sequential PATCH → HEAD → PATCH builder interactions, asserts proxy forwards methods in exact order, validates Upload-Offset headers are preserved across responses, and confirms forwarded HEAD request includes no body or duplex fields. Existing HEAD forwarding test remains to document baseline behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Cap-go/capgo#2055: Both PRs make matching changes to TUS HEAD probe handling—accepting real HEAD requests and forwarding them (bodyless) to the builder and adding tests for that behavior.
  • Cap-go/capgo#1959: Both PRs modify the same TUS HEAD probe handling and mounted /build/upload routing—updating isTusHeadProbe/proxyTusHead behavior and related tests to forward real HEAD or TUS-shaped probes upstream.

Poem

🐰 A rabbit's ode to HEAD requests.
When builders need to probe and peek,
Our TUS-aware detection's sleek—
Both real HEAD and Resumable shine,
No body clogs the proxy line,
Upload-Offset dances through!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: fixing TUS upload HEAD resume probes in the build system.
Description check ✅ Passed The description provides a summary, motivation, business impact, and test plan that align with the template requirements, though the template sections are presented in an AI-generated format rather than exact template matching.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-tus-upload-head-resume

Comment @coderabbitai help to get the list of available commands and usage tips.

@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq Bot commented May 7, 2026

Merging this PR will not alter performance

✅ 28 untouched benchmarks


Comparing codex/fix-tus-upload-head-resume (4765422) with main (4cff0be)

Open in CodSpeed

@riderx riderx force-pushed the codex/fix-tus-upload-head-resume branch from 9ffb3b3 to a25fb72 Compare May 7, 2026 14:20
@riderx riderx marked this pull request as ready for review May 7, 2026 17:16
Copy link
Copy Markdown
Member Author

riderx commented May 7, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

1 similar comment
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
supabase/migrations/20260507142606_restore_apikey_apps_rpc.sql (1)

63-65: 💤 Low value

Minor: Comment mentions "legacy fallback" but function has no visible fallback logic.

The phrase "with legacy fallback" in the comment may be misleading since this function itself doesn't implement any fallback behavior. If the fallback exists within rbac_check_permission_direct, consider clarifying to avoid confusion, e.g., "using RBAC-aware permission checks."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@supabase/migrations/20260507142606_restore_apikey_apps_rpc.sql` around lines
63 - 65, The COMMENT for function get_accessible_apps_for_apikey_v2 is
misleading by saying "with legacy fallback" even though the fallback logic isn't
in this function; update the comment to remove or clarify that phrase—either
delete "with legacy fallback" so it reads "using RBAC-aware permission checks"
or explicitly state that any legacy fallback is implemented inside
rbac_check_permission_direct so callers know where the behavior lives.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@supabase/migrations/20260507142606_restore_apikey_apps_rpc.sql`:
- Around line 63-65: The COMMENT for function get_accessible_apps_for_apikey_v2
is misleading by saying "with legacy fallback" even though the fallback logic
isn't in this function; update the comment to remove or clarify that
phrase—either delete "with legacy fallback" so it reads "using RBAC-aware
permission checks" or explicitly state that any legacy fallback is implemented
inside rbac_check_permission_direct so callers know where the behavior lives.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ad53f2f5-ec8f-4c4d-a815-65faeabd5387

📥 Commits

Reviewing files that changed from the base of the PR and between f14f684 and 0f5b8d3.

📒 Files selected for processing (4)
  • supabase/functions/_backend/public/build/index.ts
  • supabase/migrations/20260507142606_restore_apikey_apps_rpc.sql
  • tests/build-upload-head-routing.test.ts
  • tests/build-upload-security.test.ts

@riderx riderx force-pushed the codex/fix-tus-upload-head-resume branch from 0f5b8d3 to 4765422 Compare May 7, 2026 22:13
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 7, 2026

@riderx riderx merged commit 1300a33 into main May 7, 2026
40 checks passed
@riderx riderx deleted the codex/fix-tus-upload-head-resume branch May 7, 2026 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant