Extract shared media runtime service#532
Merged
justinmoon merged 4 commits intomasterfrom Mar 8, 2026
Merged
Conversation
📝 WalkthroughWalkthroughConsolidates ad-hoc media handling into a shared MediaRuntime: preparing/encrypting uploads, uploading blobs to Blossom, finishing uploads, parsing attachments from tags, downloading and decrypting media, and normalizing MIME/filename metadata across CLI, daemon, and app cores. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App/CLI/Daemon
participant MR as MediaRuntime
participant Signer as NostrSigner
participant BS as Blossom Server
App->>MR: prepare_upload(group_id, bytes, mime, filename)
activate MR
MR-->>App: PreparedMediaUpload{upload, encrypted_data}
deactivate MR
App->>Signer: sign/upload request details
Signer-->>App: signature
App->>MR: upload_encrypted_blob(signer, encrypted_data, mime, hash, servers)
activate MR
MR->>BS: POST encrypted blob (descriptor, body)
activate BS
BS-->>MR: 200 OK, uploaded_url
deactivate BS
MR-->>App: UploadedBlob{uploaded_url, descriptor_sha256_hex}
deactivate MR
App->>MR: finish_upload(upload, uploaded_url, descriptor_hash)
activate MR
MR-->>App: RuntimeMediaUploadResult{attachment, reference, imeta_tag}
deactivate MR
sequenceDiagram
participant App as App/CLI/Daemon
participant MR as MediaRuntime
participant BS as Blossom Server
App->>MR: parse_attachments_from_tags(tags)
activate MR
MR-->>App: Vec<ParsedMediaAttachment>
App->>MR: download_media(parsed_attachment)
activate MR
MR->>BS: GET encrypted blob (uploaded_url)
activate BS
BS-->>MR: encrypted data
deactivate BS
MR->>MR: decrypt_with_attachment
MR-->>App: RuntimeDownloadedMedia{attachment, decrypted_data}
deactivate MR
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/src/core/chat_media.rs (1)
1255-1276:⚠️ Potential issue | 🟡 MinorKeep the old preview path when the rehashed copy fails.
Line 1275 always re-resolves
att.local_pathfromfinal_local_path, even after the copy failed on Line 1264. In that casepp.local_pathis still valid, so the outbox thumbnail disappears unnecessarily until the message is refreshed.🩹 Proposed fix
if original_hash_hex != pp.pre_hash_hex { let final_local_path = media_file_path( &self.data_dir, &account_pubkey, &chat_id, @@ &original_hash_hex, &pp.local_filename, ); - if final_local_path != pp.local_path { - if let Err(e) = write_media_file(&final_local_path, &pp.media_data) { - tracing::warn!(%e, "failed to copy local preview to final hash path"); - } else { - let _ = std::fs::remove_file(&pp.local_path); - } - } + let effective_path = if final_local_path != pp.local_path { + if let Err(e) = write_media_file(&final_local_path, &pp.media_data) { + tracing::warn!(%e, "failed to copy local preview to final hash path"); + &pp.local_path + } else { + let _ = std::fs::remove_file(&pp.local_path); + &final_local_path + } + } else { + &final_local_path + }; // Update outbox attachment hash and local_path. if let Some(outbox) = self.local_outbox.get_mut(&chat_id) { if let Some(entry) = outbox.get_mut(&temp_rumor_id) { if let Some(att) = entry.media.get_mut(i) { att.original_hash_hex = original_hash_hex.clone(); - att.local_path = path_if_exists(&final_local_path); + att.local_path = path_if_exists(effective_path); } } } }Based on learnings: "when the MDK re-encodes media and the local preview copy to final_local_path fails, the code should fall back to pp.local_path instead of setting att.local_path to None."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/core/chat_media.rs` around lines 1255 - 1276, The outbox attachment path is being overwritten with path_if_exists(&final_local_path) even when the copy to final_local_path failed; update the block that modifies att.original_hash_hex and att.local_path (inside self.local_outbox.get_mut(&chat_id) -> entry.media.get_mut(i)) so that att.local_path is set to path_if_exists(&final_local_path) only if write_media_file succeeded, otherwise set att.local_path to path_if_exists(&pp.local_path) (or leave it unchanged) and ensure att.original_hash_hex is only updated when appropriate; use the existing symbols pp, final_local_path, write_media_file, pp.local_path, path_if_exists, local_outbox, temp_rumor_id, and i to locate and apply the change.cli/src/main.rs (1)
1646-1663:⚠️ Potential issue | 🔴 CriticalSanitize the sender-controlled filename before using it as the default output path.
On Line 1646,
downloaded.attachment.filenamecomes from message metadata. A malicious sender can set it to../../...or an absolute path, and Lines 1656-1663 will happily create parent directories and overwrite files outside the working directory when--outputis omitted.🛡️ Proposed fix
- let default_name = if downloaded.attachment.filename.is_empty() { - "download.bin" - } else { - &downloaded.attachment.filename - }; + let default_name = Path::new(downloaded.attachment.filename.trim()) + .file_name() + .and_then(|name| name.to_str()) + .filter(|name| !name.is_empty()) + .unwrap_or("download.bin"); let resolved_output = match output_path { Some(p) => p.to_path_buf(), None => PathBuf::from(default_name), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/main.rs` around lines 1646 - 1663, The code uses downloaded.attachment.filename directly as default_name and may create directories or write files outside the working dir; sanitize the sender-controlled filename before using it: reject absolute paths and strip any path components or parent traversals from downloaded.attachment.filename (e.g., take only the file_name component or run it through a filename sanitizer), fall back to "download.bin" if the sanitized name is empty or contains disallowed characters, and then use that sanitized value when building resolved_output; update the logic around default_name/resolved_output (references: downloaded.attachment.filename, default_name, resolved_output) so create_dir_all and write only operate on the sanitized, non-absolute, non-traversal path.
🧹 Nitpick comments (1)
crates/pika-marmot-runtime/src/media.rs (1)
176-179: Keepimetaparse failures visible.Line 178 converts every
parse_imeta_tagerror intoNone. Since this parser now feeds both the CLI and sidecar, malformed attachment tags will just disappear with no clue why. Please log the error at debug/trace level before dropping it.♻️ Suggested change
tags.into_iter() .filter(|tag| is_imeta_tag(tag)) - .filter_map(|tag| manager.parse_imeta_tag(tag).ok()) + .filter_map(|tag| match manager.parse_imeta_tag(tag) { + Ok(reference) => Some(reference), + Err(err) => { + tracing::debug!(error = ?err, "ignoring invalid imeta tag"); + None + } + }) .map(|reference| ParsedMediaAttachment { attachment: attachment_from_reference(&reference, None), reference, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pika-marmot-runtime/src/media.rs` around lines 176 - 179, The imeta parser errors are being swallowed by filter_map(|tag| manager.parse_imeta_tag(tag).ok()); update the pipeline around tags.into_iter().filter(|tag| is_imeta_tag(tag)).filter_map(|tag| manager.parse_imeta_tag(tag).ok()) so that parse failures are logged at debug/trace before being dropped: call manager.parse_imeta_tag(tag), match on Err(e) to emit a debug/trace log with the tag and error (using the project's logging/tracing facility), and only map Ok(reference) into ParsedMediaAttachment so malformed attachment tags remain visible in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pika-marmot-runtime/src/media.rs`:
- Around line 192-200: The download currently uses reqwest::Client::new() with
no timeout and calls response.bytes() which buffers the whole body; change to
build a Client with a sensible timeout (e.g., via
Client::builder().timeout(...).build()) and stream the response body instead of
using response.bytes(); perform an optional content-length pre-check against
MAX_CHAT_MEDIA_BYTES but unconditionally consume the body as a stream
(response.bytes_stream()) and accumulate into encrypted_data while enforcing a
hard byte cap (MAX_CHAT_MEDIA_BYTES) and returning an error if the cap is
exceeded or the read times out, keeping the existing error contexts for the URL
(reference.url) and read failures.
In `@crates/pikachat-sidecar/src/daemon.rs`:
- Around line 2996-3009: The daemon reply currently returns only uploaded_url
and original_hash_hex after runtime.finish_upload; include the selected blossom
server from the upload result (e.g., result.blossom_server or
uploaded.blossom_server depending on struct) in the JSON reply so callers know
which Blossom accepted the upload. Update the reply payload built in the match
arm after sign_and_publish (and the analogous reply blocks around the other
occurrences noted) to add a "blossom_server" field populated from the
finish_upload/upload response, ensuring you reference runtime.finish_upload,
result (or uploaded) and the reply_tx/out_ok call sites when making the change.
- Around line 542-550: The filename from resolve_upload_metadata is used
verbatim and can contain path separators, so sanitize
attachment.attachment.filename before building dest: normalize or strip any
directory components and path separators (e.g., remove '/' and '\' and any
leading dots), fallback to "download.bin" if result is empty, and if desired
restrict length/characters; then use the sanitized name when creating dest and
writing the file (referencing variables filename,
attachment.attachment.filename, dest, and media_dir).
---
Outside diff comments:
In `@cli/src/main.rs`:
- Around line 1646-1663: The code uses downloaded.attachment.filename directly
as default_name and may create directories or write files outside the working
dir; sanitize the sender-controlled filename before using it: reject absolute
paths and strip any path components or parent traversals from
downloaded.attachment.filename (e.g., take only the file_name component or run
it through a filename sanitizer), fall back to "download.bin" if the sanitized
name is empty or contains disallowed characters, and then use that sanitized
value when building resolved_output; update the logic around
default_name/resolved_output (references: downloaded.attachment.filename,
default_name, resolved_output) so create_dir_all and write only operate on the
sanitized, non-absolute, non-traversal path.
In `@rust/src/core/chat_media.rs`:
- Around line 1255-1276: The outbox attachment path is being overwritten with
path_if_exists(&final_local_path) even when the copy to final_local_path failed;
update the block that modifies att.original_hash_hex and att.local_path (inside
self.local_outbox.get_mut(&chat_id) -> entry.media.get_mut(i)) so that
att.local_path is set to path_if_exists(&final_local_path) only if
write_media_file succeeded, otherwise set att.local_path to
path_if_exists(&pp.local_path) (or leave it unchanged) and ensure
att.original_hash_hex is only updated when appropriate; use the existing symbols
pp, final_local_path, write_media_file, pp.local_path, path_if_exists,
local_outbox, temp_rumor_id, and i to locate and apply the change.
---
Nitpick comments:
In `@crates/pika-marmot-runtime/src/media.rs`:
- Around line 176-179: The imeta parser errors are being swallowed by
filter_map(|tag| manager.parse_imeta_tag(tag).ok()); update the pipeline around
tags.into_iter().filter(|tag| is_imeta_tag(tag)).filter_map(|tag|
manager.parse_imeta_tag(tag).ok()) so that parse failures are logged at
debug/trace before being dropped: call manager.parse_imeta_tag(tag), match on
Err(e) to emit a debug/trace log with the tag and error (using the project's
logging/tracing facility), and only map Ok(reference) into ParsedMediaAttachment
so malformed attachment tags remain visible in logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbd38594-c227-4ade-8690-07c125662f23
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
cli/src/main.rscrates/pika-marmot-runtime/Cargo.tomlcrates/pika-marmot-runtime/src/media.rscrates/pikachat-sidecar/src/daemon.rsrust/src/core/chat_media.rsrust/src/core/group_profile.rsrust/src/core/mod.rs
Comment on lines
+192
to
+200
| let response = reqwest::Client::new() | ||
| .get(reference.url.as_str()) | ||
| .send() | ||
| .await | ||
| .with_context(|| format!("download encrypted media from {}", reference.url))?; | ||
| if !response.status().is_success() { | ||
| anyhow::bail!("download failed: HTTP {}", response.status()); | ||
| } | ||
| let encrypted_data = response.bytes().await.context("read media response body")?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "media.rs" -path "*/pika-marmot-runtime/*" | head -20Repository: sledtools/pika
Length of output: 99
🏁 Script executed:
cat -n crates/pika-marmot-runtime/src/media.rs | sed -n '185,210p'Repository: sledtools/pika
Length of output: 1112
🏁 Script executed:
rg "MAX_CHAT_MEDIA_BYTES" -A 2 -B 2Repository: sledtools/pika
Length of output: 3205
🏁 Script executed:
rg "reqwest.*Client.*new" -A 10 | head -50Repository: sledtools/pika
Length of output: 3528
🏁 Script executed:
rg "ClientBuilder" -A 5 | head -60Repository: sledtools/pika
Length of output: 40
🏁 Script executed:
rg "timeout" -i crates/pika-marmot-runtime/src/media.rsRepository: sledtools/pika
Length of output: 40
🏁 Script executed:
cat -n crates/pika-marmot-runtime/src/media.rs | sed -n '209,250p'Repository: sledtools/pika
Length of output: 1918
🏁 Script executed:
rg "content.length|content_length" -i -A 3Repository: sledtools/pika
Length of output: 6706
🏁 Script executed:
cat -n crates/pika-marmot-runtime/src/media.rs | sed -n '1,50p'Repository: sledtools/pika
Length of output: 1718
🏁 Script executed:
rg "reqwest" -A 3 crates/pika-marmot-runtime/src/media.rs | head -40Repository: sledtools/pika
Length of output: 183
🏁 Script executed:
cat -n crates/pika-nse/src/lib.rs | sed -n '1,100p' | tail -50Repository: sledtools/pika
Length of output: 1764
🏁 Script executed:
cat -n crates/pika-marmot-runtime/src/media.rs | sed -n '186,246p'Repository: sledtools/pika
Length of output: 2786
🏁 Script executed:
rg "decrypt_from_download" -A 10 -B 2 | head -40Repository: sledtools/pika
Length of output: 2622
Add timeout and size limits to downloads.
Line 192 uses a default reqwest client with no timeout, and line 200 buffers the entire response with response.bytes(). In the shared runtime, a slow or oversized attachment can hang the async path or exhaust memory even though uploads are capped at MAX_CHAT_MEDIA_BYTES. Please stream the body with a hard size cap; a content-length pre-check alone is not enough.
🛡️ Suggested hardening
+use std::time::Duration;
...
- let response = reqwest::Client::new()
+ let mut response = reqwest::Client::new()
.get(reference.url.as_str())
+ .timeout(Duration::from_secs(30))
.send()
.await
.with_context(|| format!("download encrypted media from {}", reference.url))?;
if !response.status().is_success() {
anyhow::bail!("download failed: HTTP {}", response.status());
}
- let encrypted_data = response.bytes().await.context("read media response body")?;
+ if let Some(len) = response.content_length() {
+ if len > MAX_CHAT_MEDIA_BYTES as u64 {
+ anyhow::bail!("media too large (max 32 MB)");
+ }
+ }
+ let mut encrypted_data = Vec::new();
+ while let Some(chunk) = response.chunk().await.context("read media response chunk")? {
+ if encrypted_data.len() + chunk.len() > MAX_CHAT_MEDIA_BYTES {
+ anyhow::bail!("media too large (max 32 MB)");
+ }
+ encrypted_data.extend_from_slice(&chunk);
+ }
self.decrypt_downloaded_media(
mls_group_id,
reference,
&encrypted_data,
expected_encrypted_hash_hex,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pika-marmot-runtime/src/media.rs` around lines 192 - 200, The download
currently uses reqwest::Client::new() with no timeout and calls response.bytes()
which buffers the whole body; change to build a Client with a sensible timeout
(e.g., via Client::builder().timeout(...).build()) and stream the response body
instead of using response.bytes(); perform an optional content-length pre-check
against MAX_CHAT_MEDIA_BYTES but unconditionally consume the body as a stream
(response.bytes_stream()) and accumulate into encrypted_data while enforcing a
hard byte cap (MAX_CHAT_MEDIA_BYTES) and returning an error if the cap is
exceeded or the read times out, keeping the existing error contexts for the URL
(reference.url) and read failures.
Comment on lines
+542
to
550
| let filename = if attachment.attachment.filename.is_empty() { | ||
| "download.bin" | ||
| } else { | ||
| &reference.filename | ||
| &attachment.attachment.filename | ||
| }; | ||
| let dest = media_dir.join(format!( | ||
| "{}-{}", | ||
| hex::encode(&reference.original_hash[..8]), | ||
| &attachment.attachment.original_hash_hex[..16], | ||
| filename, |
There was a problem hiding this comment.
Sanitize the attachment filename before joining it into media_dir.
resolve_upload_metadata preserves a caller-supplied filename verbatim, so a name like foo/bar.png can reach this path unchanged. That turns dest into nested path components, and std::fs::write fails because only media_dir is created.
Possible fix
- let filename = if attachment.attachment.filename.is_empty() {
- "download.bin"
- } else {
- &attachment.attachment.filename
- };
+ let filename = Path::new(attachment.attachment.filename.trim())
+ .file_name()
+ .and_then(|name| name.to_str())
+ .filter(|name| !name.is_empty())
+ .unwrap_or("download.bin");
let dest = media_dir.join(format!(
"{}-{}",
&attachment.attachment.original_hash_hex[..16],
filename,
));📝 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
| let filename = if attachment.attachment.filename.is_empty() { | |
| "download.bin" | |
| } else { | |
| &reference.filename | |
| &attachment.attachment.filename | |
| }; | |
| let dest = media_dir.join(format!( | |
| "{}-{}", | |
| hex::encode(&reference.original_hash[..8]), | |
| &attachment.attachment.original_hash_hex[..16], | |
| filename, | |
| let filename = Path::new(attachment.attachment.filename.trim()) | |
| .file_name() | |
| .and_then(|name| name.to_str()) | |
| .filter(|name| !name.is_empty()) | |
| .unwrap_or("download.bin"); | |
| let dest = media_dir.join(format!( | |
| "{}-{}", | |
| &attachment.attachment.original_hash_hex[..16], | |
| filename, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pikachat-sidecar/src/daemon.rs` around lines 542 - 550, The filename
from resolve_upload_metadata is used verbatim and can contain path separators,
so sanitize attachment.attachment.filename before building dest: normalize or
strip any directory components and path separators (e.g., remove '/' and '\' and
any leading dots), fallback to "download.bin" if result is empty, and if desired
restrict length/characters; then use the sanitized name when creating dest and
writing the file (referencing variables filename,
attachment.attachment.filename, dest, and media_dir).
Comment on lines
+2996
to
3009
| let result = | ||
| runtime.finish_upload(&mls_group_id, &prepared.upload, uploaded.clone()); | ||
|
|
||
| // Build imeta tag and message | ||
| let imeta_tag = manager.create_imeta_tag(&upload, &uploaded_url); | ||
| let rumor = EventBuilder::new(Kind::ChatMessage, &caption) | ||
| .tag(imeta_tag) | ||
| .tag(result.imeta_tag.clone()) | ||
| .build(keys.public_key()); | ||
| match sign_and_publish(&client, &relay_urls, &mdk, &keys, &mls_group_id, rumor, "daemon_send_media").await { | ||
| Ok(ev) => { | ||
| let _ = reply_tx.send(out_ok(request_id, Some(json!({ | ||
| "event_id": ev.id.to_hex(), | ||
| "uploaded_url": uploaded_url, | ||
| "original_hash_hex": hex::encode(upload.original_hash), | ||
| "uploaded_url": uploaded.uploaded_url, | ||
| "original_hash_hex": result.attachment.original_hash_hex, | ||
| })))); |
There was a problem hiding this comment.
Return the actual Blossom server in the daemon reply.
upload_encrypted_blob/finish_upload preserve the selected blossom_server, but both response payloads only expose the blob URL(s). That drops the value this refactor just surfaced, so --remote callers still can't report which server actually accepted the upload.
Possible fix
let _ = reply_tx.send(out_ok(request_id, Some(json!({
"event_id": ev.id.to_hex(),
"uploaded_url": uploaded.uploaded_url,
+ "blossom_server": uploaded.blossom_server,
"original_hash_hex": result.attachment.original_hash_hex,
}))));- let batch_result: Result<(Vec<Tag>, Vec<String>, Vec<String>), ()> = async {
+ let batch_result: Result<(Vec<Tag>, Vec<String>, Vec<String>, Vec<String>), ()> = async {
let mut imeta_tags = Vec::new();
let mut original_hashes = Vec::new();
let mut uploaded_urls = Vec::new();
+ let mut blossom_servers = Vec::new();- original_hashes.push(result.attachment.original_hash_hex);
- uploaded_urls.push(result.uploaded_blob.uploaded_url);
+ original_hashes.push(result.attachment.original_hash_hex);
+ blossom_servers.push(result.uploaded_blob.blossom_server);
+ uploaded_urls.push(result.uploaded_blob.uploaded_url);
imeta_tags.push(result.imeta_tag);
}
- Ok((imeta_tags, original_hashes, uploaded_urls))
+ Ok((imeta_tags, original_hashes, uploaded_urls, blossom_servers))
}.await;
- let (imeta_tags, original_hashes, uploaded_urls) = match batch_result {
+ let (imeta_tags, original_hashes, uploaded_urls, blossom_servers) = match batch_result {
Ok(v) => v,
Err(()) => continue,
}; let _ = reply_tx.send(out_ok(request_id, Some(json!({
"event_id": ev.id.to_hex(),
"uploaded_urls": uploaded_urls,
+ "blossom_servers": blossom_servers,
"original_hashes": original_hashes,
}))));Also applies to: 3049-3053, 3119-3143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pikachat-sidecar/src/daemon.rs` around lines 2996 - 3009, The daemon
reply currently returns only uploaded_url and original_hash_hex after
runtime.finish_upload; include the selected blossom server from the upload
result (e.g., result.blossom_server or uploaded.blossom_server depending on
struct) in the JSON reply so callers know which Blossom accepted the upload.
Update the reply payload built in the match arm after sign_and_publish (and the
analogous reply blocks around the other occurrences noted) to add a
"blossom_server" field populated from the finish_upload/upload response,
ensuring you reference runtime.finish_upload, result (or uploaded) and the
reply_tx/out_ok call sites when making the change.
…for unknowns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+443
to
448
| fn mime_unknown_extension_defaults_to_octet_stream() { | ||
| assert_eq!( | ||
| mime_from_extension(Path::new("file.xyz")), | ||
| Some("application/octet-stream") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 Test asserts mime_from_extension returns Some for unknown extensions, but the function returns None
The test mime_unknown_extension_defaults_to_octet_stream at crates/pika-marmot-runtime/src/media.rs:443-448 asserts that mime_from_extension(Path::new("file.xyz")) returns Some("application/octet-stream"). However, mime_from_extension delegates to mime_from_extension_str (crates/pika-marmot-runtime/src/media.rs:374-401), which returns None for the _ (default) match arm. This test will always fail. The commit message ("Fix clippy useless_conversion and mime_from_extension returning Some for unknowns") suggests the intent was to fix this, but only the test expectation was changed without updating the function. The daemon's equivalent test at crates/pikachat-sidecar/src/daemon.rs:4937 correctly expects None.
Suggested change
| fn mime_unknown_extension_defaults_to_octet_stream() { | |
| assert_eq!( | |
| mime_from_extension(Path::new("file.xyz")), | |
| Some("application/octet-stream") | |
| ); | |
| } | |
| fn mime_unknown_extension_returns_none() { | |
| assert_eq!(mime_from_extension(Path::new("file.xyz")), None); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pika-marmot-runtime/src/media.rs`:
- Around line 443-448: The test mime_unknown_extension_defaults_to_octet_stream
is asserting the old behavior but the implementation (mime_from_extension ->
mime_from_extension_str) now returns None for unknown extensions; update the
assertion to expect None (e.g.,
assert_eq!(mime_from_extension(Path::new("file.xyz")), None)) and optionally
rename the test to reflect that unknown extensions return None instead of
defaulting to application/octet-stream so the test matches the current
mime_from_extension/mime_from_extension_str behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f87ee111-d3be-4030-bba1-bef1fb4af72f
📒 Files selected for processing (2)
cli/src/main.rscrates/pika-marmot-runtime/src/media.rs
Comment on lines
+443
to
448
| fn mime_unknown_extension_defaults_to_octet_stream() { | ||
| assert_eq!( | ||
| mime_from_extension(Path::new("file.xyz")), | ||
| Some("application/octet-stream") | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test assertion does not match implementation.
The test expects Some("application/octet-stream") for unknown extensions, but mime_from_extension_str returns None for unrecognized extensions (line 399). This test will fail.
Per the PR description, the behavior was intentionally changed so mime_from_extension does not return Some for unknown extensions. The test assertion needs to be updated to match.
🐛 Proposed fix
#[test]
-fn mime_unknown_extension_defaults_to_octet_stream() {
+fn mime_unknown_extension_returns_none() {
assert_eq!(
mime_from_extension(Path::new("file.xyz")),
- Some("application/octet-stream")
+ None
);
}📝 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
| fn mime_unknown_extension_defaults_to_octet_stream() { | |
| assert_eq!( | |
| mime_from_extension(Path::new("file.xyz")), | |
| Some("application/octet-stream") | |
| ); | |
| } | |
| fn mime_unknown_extension_returns_none() { | |
| assert_eq!( | |
| mime_from_extension(Path::new("file.xyz")), | |
| None | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/pika-marmot-runtime/src/media.rs` around lines 443 - 448, The test
mime_unknown_extension_defaults_to_octet_stream is asserting the old behavior
but the implementation (mime_from_extension -> mime_from_extension_str) now
returns None for unknown extensions; update the assertion to expect None (e.g.,
assert_eq!(mime_from_extension(Path::new("file.xyz")), None)) and optionally
rename the test to reflect that unknown extensions return None instead of
defaulting to application/octet-stream so the test matches the current
mime_from_extension/mime_from_extension_str behavior.
This was referenced Mar 9, 2026
justinmoon
added a commit
that referenced
this pull request
Mar 11, 2026
* Extract shared media runtime service * Report actual Blossom server in CLI media output * Simplify media hash mismatch test setup * Fix clippy useless_conversion and mime_from_extension returning Some for unknowns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 11, 2026
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
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Bug Fixes