Skip to content

fixup: give required presigned headers directly to the client#334

Open
cole-h wants to merge 1 commit into
mainfrom
server-side-headers
Open

fixup: give required presigned headers directly to the client#334
cole-h wants to merge 1 commit into
mainfrom
server-side-headers

Conversation

@cole-h

@cole-h cole-h commented Jul 10, 2026

Copy link
Copy Markdown
Member

We still override ones with what we calculated (so if something went wrong, the upload will fail because of that), but there could be some additional headers that are unknowable to the client that we just have to send along verbatim.

Summary by CodeRabbit

  • New Features
    • Release uploads now support additional staging-provided S3 headers.
    • Uploads automatically include/override required S3 metadata (content type, content length, and checksum) alongside any provided headers.
  • Bug Fixes
    • Improved header merging and case-insensitive handling to ensure consistent request behavior.
    • Enhanced validation and error messages for missing/invalid header names or values during upload.

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

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: 796c6c62-dd5a-4d1f-bbec-83e3ea134841

📥 Commits

Reviewing files that changed from the base of the PR and between bcff340 and 0e0f0c1.

📒 Files selected for processing (3)
  • src/flakehub_client.rs
  • src/main.rs
  • src/s3.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/flakehub_client.rs
  • src/s3.rs
  • src/main.rs

📝 Walkthrough

Walkthrough

S3 upload headers are deserialized from stage metadata, passed through the release flow, merged with required tarball headers, validated, and used in the upload request.

Changes

S3 header upload flow

Layer / File(s) Summary
Stage metadata propagation
src/flakehub_client.rs, src/main.rs
StageResult deserializes S3 upload headers, and the release flow forwards them to upload_release_to_s3 while adding contextual response-decoding errors.
Validated S3 request headers
src/s3.rs
The uploader merges caller-provided headers with content length, gzip content type, and SHA-256 checksum overrides using case-insensitive matching and fallible parsing.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ReleaseFlow
  participant FlakeHub
  participant upload_release_to_s3
  participant S3
  ReleaseFlow->>FlakeHub: Request release-stage metadata
  FlakeHub-->>ReleaseFlow: Return upload URL and headers
  ReleaseFlow->>upload_release_to_s3: Pass URL, headers, and tarball
  upload_release_to_s3->>S3: PUT tarball with validated headers
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: passing required presigned headers to the client for S3 upload handling.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ 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 server-side-headers

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

@cole-h
cole-h requested a review from colemickens July 10, 2026 21:39
colemickens
colemickens previously approved these changes Jul 10, 2026
@cole-h
cole-h marked this pull request as ready for review July 13, 2026 16:51

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@src/flakehub_client.rs`:
- Line 25: Add the serde default attribute to the StageResult::s3_upload_headers
field so deserialization supplies an empty HashMap when the server omits it,
preserving compatibility with older responses.

In `@src/s3.rs`:
- Around line 45-66: Update build_headers so it inserts every caller-provided
header and then unconditionally inserts all entries from overrides, ensuring
required values such as Content-Length, Content-Type, and x-amz-checksum-sha256
are present even when absent from the caller map. Preserve case-insensitive
matching and allow override entries to replace caller values without duplicating
headers.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8c466f06-a10d-4437-a872-6667748eccda

📥 Commits

Reviewing files that changed from the base of the PR and between 0e801cf and bcff340.

📒 Files selected for processing (3)
  • src/flakehub_client.rs
  • src/main.rs
  • src/s3.rs

Comment thread src/flakehub_client.rs
#[derive(serde::Deserialize)]
pub(crate) struct StageResult {
pub(crate) s3_upload_url: String,
pub(crate) s3_upload_headers: HashMap<String, String>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the target file and nearby definitions.
wc -l src/flakehub_client.rs
cat -n src/flakehub_client.rs | sed -n '1,220p'

# Find all serde-related usage for the type and response path.
rg -n "s3_upload_headers|Decoding release metadata POST response|serde(default)|Deserialize|response\.json|flakehub_client" src

Repository: DeterminateSystems/flakehub-push

Length of output: 6044


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the response handling and the release stage flow.
cat -n src/main.rs | sed -n '130,220p'

# Check whether StageResult is used anywhere else or transformed before deserialization.
rg -n "StageResult|release_stage\(|release_publish\(|Decoding release metadata POST response|s3_upload_headers" src

Repository: DeterminateSystems/flakehub-push

Length of output: 4656


Add a default for StageResult::s3_upload_headers src/flakehub_client.rs/src/main.rs

If a server response ever omits s3_upload_headers, response.json() will fail with a missing-field error and abort the release. Add #[serde(default)] so older responses can still decode cleanly.

🤖 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 `@src/flakehub_client.rs` at line 25, Add the serde default attribute to the
StageResult::s3_upload_headers field so deserialization supplies an empty
HashMap when the server omits it, preserving compatibility with older responses.

Comment thread src/s3.rs
We still override ones with what we calculated (so if something went
wrong, the upload will fail because of that), but there could be some
additional headers that are unknowable to the client that we just have
to send along verbatim.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants