fix(host-service): accept unknown mediaType on attachment upload#4439
Conversation
Browsers report empty File.type for unknown extensions (e.g. custom simulator extensions like .bmad), which was bouncing real uploads at the schema and at the mimeTypes.extension check. Fall back to application/octet-stream so the file lands; original filename is still preserved in metadata.
📝 WalkthroughWalkthroughThe attachment upload handler changes from rejecting unrecognized media types to accepting them and falling back to ChangesAttachment Media Type Fallback
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes the upload handler to gracefully accept files whose MIME type is empty or unrecognised by
Confidence Score: 4/5Safe to merge — the production change is a small, well-scoped fallback with no effect on existing known MIME types. The implementation change is minimal and correct: The empty-mediaType test in
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/attachments/attachments.ts | Relaxes mediaType schema from min(1) to allow empty string, and replaces the hard rejection of unknown MIME types with a fallback to application/octet-stream. Logic is correct and the fallback constant is clean. |
| packages/host-service/src/trpc/router/attachments/attachments.test.ts | Converts the old rejection test into two new fallback tests. The unrecognized media type test fully verifies disk writes; the empty media type test only asserts the returned mediaType without checking that the file was written or that originalFilename is persisted to metadata.json. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Client calls upload] --> B{Zod validation}
B -- fails --> ERR1[TRPC validation error]
B -- passes --> C{mimeTypes.extension check}
C -- extension found --> D[Use input.mediaType as-is]
C -- false or empty string --> E[Fallback to application/octet-stream]
D --> F{Size estimate}
E --> F
F -- oversized --> ERR2[PAYLOAD_TOO_LARGE]
F -- ok --> G[Buffer.from base64 decode]
G -- 0 bytes --> ERR3[BAD_REQUEST empty]
G -- bytes ok --> H[writeAttachment with resolved mediaType]
H --> I[Return attachmentId + mediaType + originalFilename + sizeBytes]
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/host-service/src/trpc/router/attachments/attachments.test.ts:103-111
**Empty-mediaType test doesn't verify disk write or metadata**
The unrecognized-mediaType test (line 87) fully verifies the upload by checking `existsSync(filePath)` and that the path ends in `.bin`. This test only checks `result.mediaType`, leaving it possible that a regression in `writeAttachment` (e.g. an exception thrown after the fallback is selected) would go undetected here. Adding an `existsSync` assertion and a check that `metadata.json` contains `originalFilename: "racetrack.bmad"` would give symmetrical coverage to the two new fallback paths.
Reviews (1): Last reviewed commit: "fix(host-service): accept unknown mediaT..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("falls back to application/octet-stream for empty media type", async () => { | ||
| const caller = createCaller(); | ||
| const result = await caller.upload({ | ||
| data: { kind: "base64", data: PNG_BASE64 }, | ||
| mediaType: "", | ||
| originalFilename: "racetrack.bmad", | ||
| }); | ||
| expect(result.mediaType).toBe("application/octet-stream"); | ||
| }); |
There was a problem hiding this comment.
The unrecognized-mediaType test (line 87) fully verifies the upload by checking existsSync(filePath) and that the path ends in .bin. This test only checks result.mediaType, leaving it possible that a regression in writeAttachment (e.g. an exception thrown after the fallback is selected) would go undetected here. Adding an existsSync assertion and a check that metadata.json contains originalFilename: "racetrack.bmad" would give symmetrical coverage to the two new fallback paths.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/attachments/attachments.test.ts
Line: 103-111
Comment:
**Empty-mediaType test doesn't verify disk write or metadata**
The unrecognized-mediaType test (line 87) fully verifies the upload by checking `existsSync(filePath)` and that the path ends in `.bin`. This test only checks `result.mediaType`, leaving it possible that a regression in `writeAttachment` (e.g. an exception thrown after the fallback is selected) would go undetected here. Adding an `existsSync` assertion and a check that `metadata.json` contains `originalFilename: "racetrack.bmad"` would give symmetrical coverage to the two new fallback paths.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/host-service/src/trpc/router/attachments/attachments.test.ts (1)
103-111: ⚡ Quick winStrengthen the empty-mediaType test with persistence assertions.
This test currently verifies only the returned
mediaType. Add.binpath andexistsSyncchecks (same as Line 95-100) to guard storage-path regressions for empty media types too.🤖 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 `@packages/host-service/src/trpc/router/attachments/attachments.test.ts` around lines 103 - 111, The test "falls back to application/octet-stream for empty media type" only asserts result.mediaType; extend it to also assert the persisted filename and file existence like the other test: after calling caller.upload (using PNG_BASE64), assert that result.path ends with ".bin" (or that the stored filename has a .bin extension) and use fs.existsSync(path.join(attachments storage dir, result.path)) to verify the file was actually written; reference the test's caller.upload call, result.path, PNG_BASE64, and use existsSync/path.join to perform the additional checks.
🤖 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 `@packages/host-service/src/trpc/router/attachments/attachments.test.ts`:
- Around line 103-111: The test "falls back to application/octet-stream for
empty media type" only asserts result.mediaType; extend it to also assert the
persisted filename and file existence like the other test: after calling
caller.upload (using PNG_BASE64), assert that result.path ends with ".bin" (or
that the stored filename has a .bin extension) and use
fs.existsSync(path.join(attachments storage dir, result.path)) to verify the
file was actually written; reference the test's caller.upload call, result.path,
PNG_BASE64, and use existsSync/path.join to perform the additional checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32bbf230-a66a-4a55-9b80-a303a308e52a
📒 Files selected for processing (2)
packages/host-service/src/trpc/router/attachments/attachments.test.tspackages/host-service/src/trpc/router/attachments/attachments.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Changes since v0.2.14: - workspaces: `superset workspaces list` now accepts `--project` and `--search` filters, matching the desktop list view. (#4455) - cli-framework: `--help` on a subcommand now shows the global options (e.g. `--json`, `--quiet`, `--api-key`) instead of hiding them. (#4424) - host-service: attachment upload no longer rejects unknown mediaType values returned by some hosts. (#4439) - host-service: PR fetch is now per-branch, avoiding 504s on repos with large numbers of open PRs. (#4268) Push cli-v0.2.15 after this lands to fire the release pipeline.
1 participant
Summary
.bmadFortran lattice file for accelerator simulations) hitToo small: expected string to have >=1 charactersonmediaTypebecause browsers report emptyFile.typefor unknown extensions.Unrecognized media typefor anythingmime-typesdidn't know.application/octet-streamwhen the mediaType is empty or unknown. Original filename is still preserved in metadata, and the agent receives the file via its on-disk path (with.binextension) plus the original filename.Test plan
application/x-totally-fake) is accepted and stored as.bin/tmp/sample.bmadinto the new-workspace modal and confirm it uploadsSummary by cubic
Fixes attachment uploads failing for unknown file types by accepting empty or unrecognized
mediaTypeso custom extensions (e.g. .bmad) upload successfully.attachments.uploadinput to allow emptymediaType.mime-typescan’t resolve the type, save asapplication/octet-streamwith a.binpath and keep the original filename in metadata; added tests for unknown and emptymediaType.Written for commit d9fcadb. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes