Open
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds configurable OCI download retries with Range-resume, centralizes download error mapping and retry decisions, performs initial blob pre-read for compression detection, abstracts decompressor selection/spawning, narrows stream receiver API to byte-bounded channels, and adds tests plus a dev-dependency. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Registry as RegistryClient
participant Blob as BlobStream
participant Retry as RetryHandler
participant Decomp as Decompressor
Client->>Registry: fetch_blob_with_detection(digest, options)
activate Registry
Registry->>Blob: get_blob_stream_range(digest, None)
activate Blob
Blob-->>Registry: response stream + initial bytes
deactivate Blob
Registry-->>Client: (stream, initial_chunks, compression)
deactivate Registry
Client->>Client: start retry_download_loop()
loop until success or max_retries
Client->>Blob: read bytes from stream
alt connection/truncation detected
Client->>Retry: handle_download_retry(error, &mut retry_count, max_retries, delay)
Retry-->>Client: Some(delay) or None
alt Some(delay)
Client->>Client: wait(delay)
Client->>Registry: get_blob_stream_range(digest, bytes_received)
Registry->>Blob: resume with Range header
Blob-->>Client: 206 Partial Content + bytes
end
end
end
Client->>Decomp: start_decompressor_for_compression(compression)
Client->>Decomp: pipe downloaded bytes
Decomp-->>Client: decompressed output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/fls/options.rs (1)
65-66: Consider adding aDefaultimpl forOciOptions.
BlockFlashOptionshas aDefaultimplementation that uses the centralized constants, butOciOptionsdoes not. For API consistency and to reduce boilerplate at call sites, consider implementingDefaultforOciOptionsas well.♻️ Suggested Default implementation
impl Default for OciOptions { fn default() -> Self { Self { common: FlashOptions::default(), username: None, password: None, file_pattern: None, max_retries: DEFAULT_MAX_RETRIES, retry_delay_secs: DEFAULT_RETRY_DELAY_SECS, } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/options.rs` around lines 65 - 66, Add a Default implementation for OciOptions to match BlockFlashOptions and centralize defaults: implement impl Default for OciOptions { fn default() -> Self { ... } } returning OciOptions with common: FlashOptions::default(), username: None, password: None, file_pattern: None, max_retries: DEFAULT_MAX_RETRIES, retry_delay_secs: DEFAULT_RETRY_DELAY_SECS; this provides API consistency and reduces boilerplate when constructing OciOptions.tests/oci_extract.rs (1)
502-540: Make the truncation fixture requireRangeon retries.After the first truncated response, this helper still serves a full
200 OKbody for later non-Rangerequests. That means the new test proves “some retry happened”, but not that resume support stayed intact. Reject the retry unless it includes a validRangeheader, or record/assert the header value explicitly.🧪 Tighten the fixture
- } else { - Response::builder() - .status(StatusCode::OK) - .header("Content-Length", blob_body.len().to_string()) - .body(full(Bytes::copy_from_slice(&blob_body))) - .unwrap() - } + } else { + Response::builder() + .status(StatusCode::RANGE_NOT_SATISFIABLE) + .body(full(Bytes::new())) + .unwrap() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/oci_extract.rs` around lines 502 - 540, The retry fixture must reject non-resumed retries: when req_num != 0, check the incoming request's "Range" header (e.g., via req.headers().get("range")) and only return the full-body 200 if a valid Range requesting the remainder (starting at split_point, e.g., "bytes={split_point}-") is present; otherwise return an error response (or 416/400) so the test ensures resume used Range. Update the branch using Response::builder()/body(full(...)) to inspect the Range header and conditionally respond based on its value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 24: The http-body crate is only used in tests, so remove the http-body =
"1.0.1" entry from the normal dependencies section and add the same entry under
[dev-dependencies] in Cargo.toml; verify tests that reference http_body::Frame
(e.g., tests/oci_extract.rs) still compile and run, and ensure there are no
other production uses of the http_body symbol before committing the change.
In `@src/fls/oci/from_oci.rs`:
- Around line 133-144: The current match wraps every Err(e) from
client.get_blob_stream into DownloadError::Other which loses HTTP status info
and prevents handle_download_retry from applying correct policies; instead,
inspect the error returned by get_blob_stream (preserving any HTTP status/code
or mapping it to a DownloadError variant that includes status) before calling
handle_download_retry so 401/404 are treated non-retryable and 429/5xx get the
proper backoff; update the error conversion in the Err branch (around
client.get_blob_stream handling) to extract status (or convert to a
DownloadError::Status/code variant) and pass that to handle_download_retry while
only returning the original error e when handle_download_retry indicates no
retry.
- Around line 335-343: The closure passed into retry_download_loop must not
treat writer_handle.is_finished() or decompressor_writer_handle.is_finished() as
a sign of pipeline failure because stream_blob_to_tar_files intentionally lets
the writer exit successfully; remove those is_finished checks from the liveness
predicate and instead let retry_download_loop run based on its existing
download/stream conditions, then after retry_download_loop returns await/join
writer_handle and decompressor_writer_handle to inspect their actual Result/Err
values and handle/log real errors; update references in from_oci.rs to stop
using writer_handle.is_finished()/decompressor_writer_handle.is_finished() in
the closure and perform proper join/error handling of those handles once
download completes.
- Around line 275-301: When client.get_blob_stream_range(digest,
Some(bytes_received)).await returns Err(e), the code currently continues leaving
`stream` pointing at the stale/broken body which can later be treated as EOF;
change this to fail closed: on Err from get_blob_stream_range (in the match arm
handling the Err) return an Err (or propagate the received error) immediately
instead of `continue`, so that the function does not reuse the stale `stream`
and does not return a false-success bytes_received; update any error message to
include context (digest/bytes_received) as needed.
---
Nitpick comments:
In `@src/fls/options.rs`:
- Around line 65-66: Add a Default implementation for OciOptions to match
BlockFlashOptions and centralize defaults: implement impl Default for OciOptions
{ fn default() -> Self { ... } } returning OciOptions with common:
FlashOptions::default(), username: None, password: None, file_pattern: None,
max_retries: DEFAULT_MAX_RETRIES, retry_delay_secs: DEFAULT_RETRY_DELAY_SECS;
this provides API consistency and reduces boilerplate when constructing
OciOptions.
In `@tests/oci_extract.rs`:
- Around line 502-540: The retry fixture must reject non-resumed retries: when
req_num != 0, check the incoming request's "Range" header (e.g., via
req.headers().get("range")) and only return the full-body 200 if a valid Range
requesting the remainder (starting at split_point, e.g., "bytes={split_point}-")
is present; otherwise return an error response (or 416/400) so the test ensures
resume used Range. Update the branch using Response::builder()/body(full(...))
to inspect the Range header and conditionally respond based on its value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66d1ccc8-cc7f-4acb-b0a5-ed088343583e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlsrc/fls/decompress.rssrc/fls/download_error.rssrc/fls/fastboot.rssrc/fls/from_url.rssrc/fls/mod.rssrc/fls/oci/from_oci.rssrc/fls/oci/registry.rssrc/fls/options.rssrc/lib.rssrc/main.rstests/oci_extract.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/fls/oci/from_oci.rs (2)
303-306:⚠️ Potential issue | 🔴 CriticalResume failure allows stale stream to be reused, risking false success.
When
get_blob_stream_range()fails, the codecontinues to the outer loop, butstreamstill references the old broken response. On the next iteration,stream.next()may returnOk(None)(EOF), causing the function to returnOk(bytes_received)with truncated data.This matches the concern from the previous review. The resume failure should fail closed:
🔧 Proposed fix
Err(e) => { - eprintln!("Retry connection failed: {}", e); - continue; + return Err(format!( + "Failed to resume blob download from byte {}: {}", + bytes_received, e + ) + .into()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/from_oci.rs` around lines 303 - 306, When get_blob_stream_range() returns Err(e) while attempting to resume, do not continue and reuse the old broken stream; instead fail closed by returning an Err so the stale stream cannot be used to produce a false success. Update the Err(e) branch in the retry/resume loop (the block that currently eprintln! and continue) to return an appropriate Err value from the current function (propagate or map the error) so that the function does not proceed to call stream.next() and cannot return Ok(bytes_received) with truncated data; reference get_blob_stream_range, stream, stream.next(), and bytes_received to locate and change the handling.
133-150:⚠️ Potential issue | 🟠 MajorHTTP status information lost during error classification.
When
get_blob_stream()fails, all errors are wrapped asDownloadError::Other(e.to_string()), which marks them as retryable. This means permanent failures like HTTP 401/403/404 (which are caught inregistry.rsbut returned as boxed errors) will be retried unnecessarily.Consider extracting the reqwest error from the boxed error to use
DownloadError::from_reqwest()for proper classification:💡 Suggested approach
Err(e) => { - // Classify connection-level errors for retry - let dl_err = DownloadError::Other(e.to_string()); + // Try to extract reqwest error for proper classification + let dl_err = e + .downcast_ref::<reqwest::Error>() + .map(|re| DownloadError::from_reqwest(re.clone())) + .unwrap_or_else(|| DownloadError::Other(e.to_string()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/from_oci.rs` around lines 133 - 150, When client.get_blob_stream(digest) fails in the get_blob_stream match arm, don't always wrap the boxed error into DownloadError::Other; instead try to extract the underlying reqwest error from the boxed error and convert it via DownloadError::from_reqwest(...) so HTTP status-based cases (401/403/404) are classified correctly before calling handle_download_retry(&dl_err, &mut retry_count, max_retries, retry_delay_secs); if extraction fails, fall back to DownloadError::Other(e.to_string()), keep using retry_count/max_retries/retry_delay_secs and tokio::time::sleep(delay).await on Some(delay), and on None return the original boxed error as before.
🧹 Nitpick comments (1)
src/fls/decompress.rs (1)
62-62: Consider usingeprintln!or a proper logging framework.Diagnostic output using
println!can interfere with stdout-based pipelines. Since this is informational/diagnostic,eprintln!(consistent with line 62 in from_oci.rs:eprintln!("Downloaded blob to {}")) or a structured logging crate liketracingwould be more appropriate.This also applies to line 39.
♻️ Suggested fix
- println!("Using decompressor: {}", cmd); + eprintln!("Using decompressor: {}", cmd);And similarly for line 39.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/decompress.rs` at line 62, Replace the diagnostic println! calls in this module with stderr or a logging facility: change the println!("Using decompressor: {}", cmd) (and the other println! at the earlier occurrence around line 39) to eprintln! or wire them into the project's logging/tracing crate so informational output doesn't pollute stdout-based pipelines; ensure the same formatting and message text is preserved when switching to eprintln! or the chosen logger.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fls/oci/from_oci.rs`:
- Around line 341-349: The pipeline liveness closure passed into
retry_download_loop incorrectly treats an early writer exit as failure; update
the closure in the call inside stream_blob_to_tar_files so it only checks
decompressor_writer_handle.is_finished() (i.e., use &||
!decompressor_writer_handle.is_finished()) rather than the current AND of both
writer_handle and decompressor_writer_handle, leaving send/error checks as-is;
this ensures retry_download_loop treats normal early writer termination (when
requested files are found) as successful while still detecting decompressor
failures.
---
Duplicate comments:
In `@src/fls/oci/from_oci.rs`:
- Around line 303-306: When get_blob_stream_range() returns Err(e) while
attempting to resume, do not continue and reuse the old broken stream; instead
fail closed by returning an Err so the stale stream cannot be used to produce a
false success. Update the Err(e) branch in the retry/resume loop (the block that
currently eprintln! and continue) to return an appropriate Err value from the
current function (propagate or map the error) so that the function does not
proceed to call stream.next() and cannot return Ok(bytes_received) with
truncated data; reference get_blob_stream_range, stream, stream.next(), and
bytes_received to locate and change the handling.
- Around line 133-150: When client.get_blob_stream(digest) fails in the
get_blob_stream match arm, don't always wrap the boxed error into
DownloadError::Other; instead try to extract the underlying reqwest error from
the boxed error and convert it via DownloadError::from_reqwest(...) so HTTP
status-based cases (401/403/404) are classified correctly before calling
handle_download_retry(&dl_err, &mut retry_count, max_retries, retry_delay_secs);
if extraction fails, fall back to DownloadError::Other(e.to_string()), keep
using retry_count/max_retries/retry_delay_secs and
tokio::time::sleep(delay).await on Some(delay), and on None return the original
boxed error as before.
---
Nitpick comments:
In `@src/fls/decompress.rs`:
- Line 62: Replace the diagnostic println! calls in this module with stderr or a
logging facility: change the println!("Using decompressor: {}", cmd) (and the
other println! at the earlier occurrence around line 39) to eprintln! or wire
them into the project's logging/tracing crate so informational output doesn't
pollute stdout-based pipelines; ensure the same formatting and message text is
preserved when switching to eprintln! or the chosen logger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 031b2370-9a74-4bba-a16f-06a8390b42fc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsrc/fls/decompress.rssrc/fls/download_error.rssrc/fls/fastboot.rssrc/fls/from_url.rssrc/fls/mod.rssrc/fls/oci/from_oci.rssrc/fls/oci/registry.rssrc/fls/options.rssrc/fls/stream_utils.rssrc/lib.rssrc/main.rstests/oci_extract.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/fls/options.rs
- src/fls/from_url.rs
- src/main.rs
- src/fls/mod.rs
- Cargo.toml
- src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/fls/oci/registry.rs (1)
245-248:⚠️ Potential issue | 🟠 MajorPreserve structured HTTP failure details instead of stringifying them.
At Line 245–Line 248, converting blob fetch failures to
Stringloses status semantics (e.g., 401/404 vs 429/5xx), so upstream retry logic can misclassify permanent failures as retryable.Suggested fix
+use crate::fls::download_error::DownloadError; ... - if !response.status().is_success() && response.status() != StatusCode::PARTIAL_CONTENT { - let status = response.status(); - let body = response.text().await.unwrap_or_default(); - return Err(format!("Failed to fetch blob: {} - {}", status, body).into()); - } + if !response.status().is_success() && response.status() != StatusCode::PARTIAL_CONTENT { + return Err(DownloadError::from_http_response(&response).into()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/registry.rs` around lines 245 - 248, The code currently turns non-success HTTP responses into a String error (using response.status() and response.text()) which loses structured status semantics; instead, preserve the status and body by returning a typed error that includes the StatusCode and optional body (e.g., a new RegistryError::Http { status: StatusCode, body: String } or similar) rather than stringifying them—replace the Err(format!(...)).into() return with constructing and returning that structured error so upstream retry/handling can inspect response.status() (use the existing response, status and body variables when building the error).
🧹 Nitpick comments (1)
src/fls/stream_utils.rs (1)
9-16: Optional: Consider removing theReceiverVariantwrapper.Since there's now only a single variant,
ReceiverVariantis just a passthrough newtype. You could simplify by usingByteBoundedReceiver<Bytes>directly as therxfield inChannelReader, eliminating this indirection.That said, keeping the wrapper is fine if you anticipate adding other receiver types in the future or prefer the explicit abstraction boundary.
♻️ Proposed simplification (optional)
-/// Wrapper over a byte-bounded receiver for use in synchronous readers -struct ReceiverVariant(ByteBoundedReceiver<Bytes>); - -impl ReceiverVariant { - fn blocking_recv(&mut self) -> Option<Bytes> { - self.0.blocking_recv() - } -} - // ... pub struct ChannelReader { - rx: ReceiverVariant, + rx: ByteBoundedReceiver<Bytes>, current: Option<Bytes>, offset: usize, } impl ChannelReader { pub fn new_byte_bounded(rx: ByteBoundedReceiver<Bytes>) -> Self { Self { - rx: ReceiverVariant(rx), + rx, current: None, offset: 0, } } } // In Read::read, change self.rx.blocking_recv() to self.rx.blocking_recv() // (no change needed since method name is the same)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/stream_utils.rs` around lines 9 - 16, The ReceiverVariant newtype is unnecessary since it only forwards blocking_recv to the inner ByteBoundedReceiver<Bytes>; simplify by replacing ReceiverVariant with ByteBoundedReceiver<Bytes> directly (use it as the rx field type in ChannelReader) and call its blocking_recv method where ReceiverVariant::blocking_recv was used; update any constructor/field accesses for ChannelReader and remove the ReceiverVariant struct and impl to eliminate the passthrough indirection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fls/oci/from_oci.rs`:
- Around line 283-299: When handling a 206 Partial Content response in the
resume logic, validate the response's "Content-Range" header actually starts at
bytes_received to avoid corruption: read the "Content-Range" header from
response, parse the leading start value (e.g., "bytes {start}-{end}/..."),
ensure start == bytes_received, and return an error if the header is missing or
the start mismatches; update the branch that checks response.status() ==
StatusCode::PARTIAL_CONTENT (and before assigning stream =
Box::pin(response.bytes_stream())) to perform this validation and only proceed
to set stream when the range start matches.
- Around line 341-349: The download_result is propagated early with
download_result? which can return before the decompressor child
(decompressor_writer_handle) is awaited and leave it unreaped; change the
control flow so you always await the spawned handles (writer_handle and
decompressor_writer_handle via tokio::join! or awaiting each) before returning
the download_result, e.g., run let (wr, dw) = tokio::join!(writer_handle,
decompressor_writer_handle) and then handle their results, and only after both
handles have completed propagate retry_download_loop's result (download_result)
so the decompressor child is always reaped even on errors.
---
Duplicate comments:
In `@src/fls/oci/registry.rs`:
- Around line 245-248: The code currently turns non-success HTTP responses into
a String error (using response.status() and response.text()) which loses
structured status semantics; instead, preserve the status and body by returning
a typed error that includes the StatusCode and optional body (e.g., a new
RegistryError::Http { status: StatusCode, body: String } or similar) rather than
stringifying them—replace the Err(format!(...)).into() return with constructing
and returning that structured error so upstream retry/handling can inspect
response.status() (use the existing response, status and body variables when
building the error).
---
Nitpick comments:
In `@src/fls/stream_utils.rs`:
- Around line 9-16: The ReceiverVariant newtype is unnecessary since it only
forwards blocking_recv to the inner ByteBoundedReceiver<Bytes>; simplify by
replacing ReceiverVariant with ByteBoundedReceiver<Bytes> directly (use it as
the rx field type in ChannelReader) and call its blocking_recv method where
ReceiverVariant::blocking_recv was used; update any constructor/field accesses
for ChannelReader and remove the ReceiverVariant struct and impl to eliminate
the passthrough indirection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 768c5280-d09c-4542-998b-1aacef811816
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsrc/fls/decompress.rssrc/fls/download_error.rssrc/fls/fastboot.rssrc/fls/from_url.rssrc/fls/mod.rssrc/fls/oci/from_oci.rssrc/fls/oci/registry.rssrc/fls/options.rssrc/fls/stream_utils.rssrc/lib.rssrc/main.rstests/oci_extract.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/fls/fastboot.rs
- src/fls/mod.rs
- src/lib.rs
- Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/fls/oci/from_oci.rs (1)
292-311:⚠️ Potential issue | 🟠 MajorFail closed on missing/invalid
Content-Rangein 206 responses.The current check only validates when parsing succeeds; missing or malformed headers still pass. Resume should reject 206 responses unless
Content-Rangeis present and starts exactly atbytes_received.Suggested fix
- if let Some(cr) = response - .headers() - .get("content-range") - .and_then(|v| v.to_str().ok()) - { - // Expected: "bytes <start>-<end>/<total>" - if let Some(start_str) = - cr.strip_prefix("bytes ").and_then(|s| s.split('-').next()) - { - if let Ok(start) = start_str.parse::<u64>() { - if start != bytes_received { - return Err(format!( - "Content-Range mismatch: expected start={}, got start={}", - bytes_received, start - ) - .into()); - } - } - } - } + let cr = response + .headers() + .get(reqwest::header::CONTENT_RANGE) + .and_then(|v| v.to_str().ok()) + .ok_or("Missing Content-Range on 206 response")?; + + let start = cr + .strip_prefix("bytes ") + .and_then(|s| s.split('-').next()) + .and_then(|s| s.parse::<u64>().ok()) + .ok_or_else(|| format!("Invalid Content-Range: {}", cr))?; + + if start != bytes_received { + return Err(format!( + "Content-Range mismatch: expected start={}, got start={} ({})", + bytes_received, start, cr + ) + .into()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/from_oci.rs` around lines 292 - 311, The resume logic currently only errors if Content-Range parses and start mismatches; instead, when handling a 206 response you must require a valid Content-Range header that parses and whose start equals bytes_received. In the block referring to response and bytes_received, change the check so that if headers().get("content-range") is missing or to_str()/parse::<u64>() fails, you return an Err indicating missing/invalid Content-Range, and if parsed start != bytes_received you return the existing mismatch Err; ensure this applies for 206 responses (response.status() == 206) so malformed or absent headers fail closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/oci_extract.rs`:
- Around line 482-484: The test reads start_str.parse::<usize>() into start and
then uses blob_body[start..], which panics if start > blob_body.len(); update
the logic around start_str/start parse to validate the parsed start against
blob_body.len() and handle out-of-range values by returning/setting an
appropriate HTTP error or using a safe slice (e.g., skip the range handling)
instead of slicing past the end; specifically guard the code that computes
remaining = &blob_body[start..] and the subsequent content_range formatting so
that if start > blob_body.len() you produce a controlled HTTP 416/400 response
or clamp start to blob_body.len() to avoid panics.
---
Duplicate comments:
In `@src/fls/oci/from_oci.rs`:
- Around line 292-311: The resume logic currently only errors if Content-Range
parses and start mismatches; instead, when handling a 206 response you must
require a valid Content-Range header that parses and whose start equals
bytes_received. In the block referring to response and bytes_received, change
the check so that if headers().get("content-range") is missing or
to_str()/parse::<u64>() fails, you return an Err indicating missing/invalid
Content-Range, and if parsed start != bytes_received you return the existing
mismatch Err; ensure this applies for 206 responses (response.status() == 206)
so malformed or absent headers fail closed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80ccbadd-d300-4701-bcc0-c2aa24fcca08
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsrc/fls/decompress.rssrc/fls/download_error.rssrc/fls/fastboot.rssrc/fls/from_url.rssrc/fls/mod.rssrc/fls/oci/from_oci.rssrc/fls/oci/registry.rssrc/fls/options.rssrc/fls/stream_utils.rssrc/lib.rssrc/main.rstests/oci_extract.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/fls/from_url.rs
- Cargo.toml
- src/main.rs
- src/fls/decompress.rs
- src/fls/fastboot.rs
- src/fls/oci/registry.rs
Comment on lines
+482
to
+484
| if let Ok(start) = start_str.parse::<usize>() { | ||
| let remaining = &blob_body[start..]; | ||
| let content_range = format!( |
There was a problem hiding this comment.
Guard invalid Range offsets to avoid test-server panics.
blob_body[start..] will panic if start > blob_body.len(), which can turn protocol bugs into opaque task aborts instead of clear HTTP behavior.
Suggested fix
- if let Ok(start) = start_str.parse::<usize>() {
- let remaining = &blob_body[start..];
+ if let Ok(start) = start_str.parse::<usize>() {
+ if start > blob_body.len() {
+ let resp: Response<BoxBody> = Response::builder()
+ .status(StatusCode::RANGE_NOT_SATISFIABLE)
+ .body(full(Bytes::from("Invalid range start")))
+ .unwrap();
+ return Ok(resp);
+ }
+ let remaining = &blob_body[start..];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Ok(start) = start_str.parse::<usize>() { | |
| let remaining = &blob_body[start..]; | |
| let content_range = format!( | |
| if let Ok(start) = start_str.parse::<usize>() { | |
| if start > blob_body.len() { | |
| let resp: Response<BoxBody> = Response::builder() | |
| .status(StatusCode::RANGE_NOT_SATISFIABLE) | |
| .body(full(Bytes::from("Invalid range start"))) | |
| .unwrap(); | |
| return Ok(resp); | |
| } | |
| let remaining = &blob_body[start..]; | |
| let content_range = format!( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/oci_extract.rs` around lines 482 - 484, The test reads
start_str.parse::<usize>() into start and then uses blob_body[start..], which
panics if start > blob_body.len(); update the logic around start_str/start parse
to validate the parsed start against blob_body.len() and handle out-of-range
values by returning/setting an appropriate HTTP error or using a safe slice
(e.g., skip the range handling) instead of slicing past the end; specifically
guard the code that computes remaining = &blob_body[start..] and the subsequent
content_range formatting so that if start > blob_body.len() you produce a
controlled HTTP 416/400 response or clamp start to blob_body.len() to avoid
panics.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/oci_extract.rs (1)
482-484:⚠️ Potential issue | 🟡 MinorGuard invalid Range offsets to avoid test-server panics.
blob_body[start..]will panic ifstart > blob_body.len(). While this is test code, it can turn protocol bugs into opaque task aborts instead of clear HTTP 416 responses.Suggested fix
if let Ok(start) = start_str.parse::<usize>() { + if start > blob_body.len() { + let resp: Response<BoxBody> = Response::builder() + .status(StatusCode::RANGE_NOT_SATISFIABLE) + .body(full(Bytes::from("Invalid range start"))) + .unwrap(); + return Ok(resp); + } let remaining = &blob_body[start..];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/oci_extract.rs` around lines 482 - 484, The code uses start_str.parse::<usize>() and then unconditionally slices blob_body[start..], which panics when start > blob_body.len(); update the logic around the parsing branch (the start variable and the creation of remaining/content_range) to first check that start <= blob_body.len() and handle the out-of-range case by returning an appropriate HTTP 416-style error (or test assertion) instead of slicing; adjust the branch that sets remaining and content_range to only run when the guard passes so tests produce a clear 416-like failure rather than a panic.src/fls/oci/from_oci.rs (1)
288-304:⚠️ Potential issue | 🟠 MajorMissing
Content-Rangevalidation when resuming downloads.When the registry returns 206 Partial Content, the code proceeds without verifying that the returned range actually starts at
bytes_received. A misbehaving registry or proxy could return a mismatched range, causing data corruption in the reconstructed blob stream.Suggested fix
Ok(response) => { if response.status() == StatusCode::PARTIAL_CONTENT { + // Validate Content-Range header matches requested offset + let content_range = response + .headers() + .get(reqwest::header::CONTENT_RANGE) + .and_then(|v| v.to_str().ok()); + + if let Some(range_str) = content_range { + // Parse "bytes START-END/TOTAL" format + if let Some(start) = range_str + .strip_prefix("bytes ") + .and_then(|s| s.split('-').next()) + .and_then(|s| s.parse::<u64>().ok()) + { + if start != bytes_received { + return Err(format!( + "Resume offset mismatch: requested {}, got {} ({})", + bytes_received, start, range_str + ) + .into()); + } + } + } + if debug { eprintln!( "[DEBUG] Registry supports Range, resuming from byte {}", bytes_received ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/from_oci.rs` around lines 288 - 304, When handling a 206 Partial Content response in the blob download logic, validate the response's Content-Range header to ensure the returned range actually starts at bytes_received before assigning stream; specifically, read response.headers().get("content-range"), parse the range start (e.g., the "bytes X-Y/..." format) and verify X == bytes_received, returning an Err if the header is missing or the start does not match; add a debug eprintln showing the Content-Range when debug is true and fail early instead of using response.bytes_stream() if the range is inconsistent to avoid corrupting the reconstructed blob.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/fls/oci/from_oci.rs`:
- Around line 288-304: When handling a 206 Partial Content response in the blob
download logic, validate the response's Content-Range header to ensure the
returned range actually starts at bytes_received before assigning stream;
specifically, read response.headers().get("content-range"), parse the range
start (e.g., the "bytes X-Y/..." format) and verify X == bytes_received,
returning an Err if the header is missing or the start does not match; add a
debug eprintln showing the Content-Range when debug is true and fail early
instead of using response.bytes_stream() if the range is inconsistent to avoid
corrupting the reconstructed blob.
In `@tests/oci_extract.rs`:
- Around line 482-484: The code uses start_str.parse::<usize>() and then
unconditionally slices blob_body[start..], which panics when start >
blob_body.len(); update the logic around the parsing branch (the start variable
and the creation of remaining/content_range) to first check that start <=
blob_body.len() and handle the out-of-range case by returning an appropriate
HTTP 416-style error (or test assertion) instead of slicing; adjust the branch
that sets remaining and content_range to only run when the guard passes so tests
produce a clear 416-like failure rather than a panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3d23570-3bed-4b94-babd-84fe8b487375
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsrc/fls/decompress.rssrc/fls/download_error.rssrc/fls/fastboot.rssrc/fls/from_url.rssrc/fls/mod.rssrc/fls/oci/from_oci.rssrc/fls/oci/registry.rssrc/fls/options.rssrc/fls/stream_utils.rssrc/lib.rssrc/main.rstests/oci_extract.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- Cargo.toml
- src/fls/mod.rs
- src/main.rs
- src/fls/options.rs
- src/fls/stream_utils.rs
- src/fls/from_url.rs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/oci_extract.rs (1)
482-498:⚠️ Potential issue | 🟡 MinorGuard invalid Range offsets to avoid test-server panics.
blob_body[start..]at line 483 will panic ifstart > blob_body.len(). While this is test code, a protocol bug could cause opaque task aborts instead of clear HTTP 416 responses.Suggested fix
if let Ok(start) = start_str.parse::<usize>() { + if start > blob_body.len() { + let resp: Response<BoxBody> = Response::builder() + .status(StatusCode::RANGE_NOT_SATISFIABLE) + .body(full(Bytes::from("Invalid range start"))) + .unwrap(); + return Ok(resp); + } let remaining = &blob_body[start..];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/oci_extract.rs` around lines 482 - 498, The code currently parses start_str into start and then slices blob_body[start..], which will panic if start > blob_body.len(); update the Range-handling in the block that parses start_str (the start variable and subsequent slicing of blob_body) to first check if start <= blob_body.len() and only proceed to build the PARTIAL_CONTENT Response when valid, otherwise return a 416 Range Not Satisfiable Response (use StatusCode::RANGE_NOT_SATISFIABLE) with an appropriate Content-Range header like "bytes */{blob_body.len()}" and no body; ensure you adjust the Response::builder() branch that currently sets Content-Length/Content-Range/Accept-Ranges so the invalid-case returns the 416 response instead of panicking.src/fls/oci/from_oci.rs (1)
288-304:⚠️ Potential issue | 🟠 MajorValidate
Content-Rangestart when resuming to prevent data corruption.When receiving a 206 response, the code assumes the registry honored the requested offset, but a misbehaving registry or proxy could return a different range. Without validation, the reconstructed blob would be corrupted silently.
Suggested fix
Ok(response) => { if response.status() == StatusCode::PARTIAL_CONTENT { + // Validate Content-Range header matches our requested offset + if let Some(content_range) = response + .headers() + .get(reqwest::header::CONTENT_RANGE) + .and_then(|v| v.to_str().ok()) + { + // Parse "bytes START-END/TOTAL" + if let Some(start) = content_range + .strip_prefix("bytes ") + .and_then(|s| s.split('-').next()) + .and_then(|s| s.parse::<u64>().ok()) + { + if start != bytes_received { + return Err(format!( + "Resume offset mismatch: requested {}, got {} ({})", + bytes_received, start, content_range + ) + .into()); + } + } + } if debug {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/from_oci.rs` around lines 288 - 304, When handling a 206 response for resuming downloads (the branch checking response.status() == StatusCode::PARTIAL_CONTENT), validate the Content-Range header before accepting the stream: read response.headers().get("content-range"), parse the range start (from the "bytes start-end/total" format), ensure that the parsed start equals bytes_received, and return an error if the header is missing, malformed, or the start does not match bytes_received; only then set stream = Box::pin(response.bytes_stream()). This prevents silent corruption from misbehaving registries or proxies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/fls/oci/from_oci.rs`:
- Around line 288-304: When handling a 206 response for resuming downloads (the
branch checking response.status() == StatusCode::PARTIAL_CONTENT), validate the
Content-Range header before accepting the stream: read
response.headers().get("content-range"), parse the range start (from the "bytes
start-end/total" format), ensure that the parsed start equals bytes_received,
and return an error if the header is missing, malformed, or the start does not
match bytes_received; only then set stream = Box::pin(response.bytes_stream()).
This prevents silent corruption from misbehaving registries or proxies.
In `@tests/oci_extract.rs`:
- Around line 482-498: The code currently parses start_str into start and then
slices blob_body[start..], which will panic if start > blob_body.len(); update
the Range-handling in the block that parses start_str (the start variable and
subsequent slicing of blob_body) to first check if start <= blob_body.len() and
only proceed to build the PARTIAL_CONTENT Response when valid, otherwise return
a 416 Range Not Satisfiable Response (use StatusCode::RANGE_NOT_SATISFIABLE)
with an appropriate Content-Range header like "bytes */{blob_body.len()}" and no
body; ensure you adjust the Response::builder() branch that currently sets
Content-Length/Content-Range/Accept-Ranges so the invalid-case returns the 416
response instead of panicking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 167aaf70-b02f-4501-ba22-807eba603756
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsrc/fls/decompress.rssrc/fls/download_error.rssrc/fls/fastboot.rssrc/fls/from_url.rssrc/fls/mod.rssrc/fls/oci/from_oci.rssrc/fls/oci/registry.rssrc/fls/options.rssrc/fls/stream_utils.rssrc/lib.rssrc/main.rstests/oci_extract.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/fls/mod.rs
- src/main.rs
- src/fls/from_url.rs
- src/fls/fastboot.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/fls/oci/from_oci.rs (1)
288-304:⚠️ Potential issue | 🟠 MajorMissing
Content-Rangevalidation when resuming downloads.When the server responds with 206 Partial Content, the code doesn't verify that the returned range actually starts at
bytes_received. A misbehaving registry or proxy could return a mismatched range, leading to data corruption in the reconstructed blob.🛡️ Suggested fix to validate Content-Range
Ok(response) => { if response.status() == StatusCode::PARTIAL_CONTENT { + // Validate Content-Range header to prevent data corruption + let content_range = response + .headers() + .get(reqwest::header::CONTENT_RANGE) + .and_then(|v| v.to_str().ok()); + + if let Some(range_str) = content_range { + // Parse "bytes START-END/TOTAL" format + if let Some(start) = range_str + .strip_prefix("bytes ") + .and_then(|s| s.split('-').next()) + .and_then(|s| s.parse::<u64>().ok()) + { + if start != bytes_received { + return Err(format!( + "Resume offset mismatch: requested {}, got {} (Content-Range: {})", + bytes_received, start, range_str + ).into()); + } + } + } + if debug { eprintln!( "[DEBUG] Registry supports Range, resuming from byte {}", bytes_received ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/from_oci.rs` around lines 288 - 304, The code handling a 206 Partial Content response (the branch checking response.status() == StatusCode::PARTIAL_CONTENT) does not validate the Content-Range header against bytes_received, so a mismatched range could corrupt the blob; update the handler to read response.headers().get("Content-Range"), parse the returned start byte and ensure it equals bytes_received (or return an Err if missing/mismatched), log a debug message when it matches, and only then set stream = Box::pin(response.bytes_stream()); reference the symbols response, StatusCode::PARTIAL_CONTENT, bytes_received, Content-Range header, and stream to find where to insert the validation and error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fls/oci/from_oci.rs`:
- Around line 453-456: Wrap the Unix-specific conversion of decompressor_stdout
into a std::fs::File with conditional compilation: enclose the existing call
using decompressor_stdout.into_owned_fd().map_err(...).into() inside a
#[cfg(unix)] block (referencing decompressor_stdout and into_owned_fd and the
std_stdout binding), and add a #[cfg(not(unix))] branch that provides a
platform-appropriate fallback (for example return a descriptive Err or an
alternative conversion) so the code compiles on non-Unix targets; ensure both
branches assign/return the same std_stdout symbol or equivalent result type.
---
Duplicate comments:
In `@src/fls/oci/from_oci.rs`:
- Around line 288-304: The code handling a 206 Partial Content response (the
branch checking response.status() == StatusCode::PARTIAL_CONTENT) does not
validate the Content-Range header against bytes_received, so a mismatched range
could corrupt the blob; update the handler to read
response.headers().get("Content-Range"), parse the returned start byte and
ensure it equals bytes_received (or return an Err if missing/mismatched), log a
debug message when it matches, and only then set stream =
Box::pin(response.bytes_stream()); reference the symbols response,
StatusCode::PARTIAL_CONTENT, bytes_received, Content-Range header, and stream to
find where to insert the validation and error path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faadd08c-4e24-4db7-b277-08dc77d88c1a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsrc/fls/decompress.rssrc/fls/download_error.rssrc/fls/fastboot.rssrc/fls/from_url.rssrc/fls/mod.rssrc/fls/oci/from_oci.rssrc/fls/oci/registry.rssrc/fls/options.rssrc/fls/stream_utils.rssrc/lib.rssrc/main.rstests/oci_extract.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main.rs
- src/fls/from_url.rs
- Cargo.toml
- src/fls/stream_utils.rs
- src/fls/fastboot.rs
Comment on lines
+453
to
+456
| let std_stdout: std::fs::File = decompressor_stdout | ||
| .into_owned_fd() | ||
| .map_err(|e| format!("Failed to convert decompressor stdout: {}", e))? | ||
| .into(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any platform guards or documentation about Unix-only support
rg -n "cfg.*unix|unix.*only|linux.*only" src/ --type rust -B 1 -A 1
# Check if there are other uses of Unix-specific APIs
rg -n "into_owned_fd|from_raw_fd|as_raw_fd" src/ --type rust -B 1 -A 1Repository: jumpstarter-dev/fls
Length of output: 3568
Add platform guards around Unix-specific API usage at lines 453-456.
The into_owned_fd() call is Unix-specific and will fail to compile on non-Unix platforms (e.g., Windows). The same file already demonstrates the correct pattern with platform guards at lines 1447–1459. Wrap the conversion in a #[cfg(unix)] block, and provide a #[cfg(not(unix))] fallback implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fls/oci/from_oci.rs` around lines 453 - 456, Wrap the Unix-specific
conversion of decompressor_stdout into a std::fs::File with conditional
compilation: enclose the existing call using
decompressor_stdout.into_owned_fd().map_err(...).into() inside a #[cfg(unix)]
block (referencing decompressor_stdout and into_owned_fd and the std_stdout
binding), and add a #[cfg(not(unix))] branch that provides a
platform-appropriate fallback (for example return a descriptive Err or an
alternative conversion) so the code compiles on non-Unix targets; ensure both
branches assign/return the same std_stdout symbol or equivalent result type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores