fix(storage): set content type on CLI uploads instead of octet-stream#168
Conversation
`storage upload` built the multipart blob with `new Blob([fileContent])` and never set a type, so every uploaded object was stored as application/octet-stream regardless of the actual file type. The backend trusts the client's content type and does not infer one from the extension, so the wrong type stuck. Infer the MIME type from the file's extension (the approach `aws s3 cp` uses) and set it on the blob so it reaches the server via the multipart part. Add a `--content-type` flag to override the inferred value. Unknown extensions still fall back to application/octet-stream, so there is no regression. Ref: INS-358 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughAdds ChangesMIME Inference and Upload Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Greptile SummaryFixes a longstanding bug where CLI storage uploads always sent
Confidence Score: 5/5Safe to merge — the change is additive and the unknown-extension fallback to The fix is narrowly scoped: one Blob constructor call gains a No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant CLI as upload.ts
participant Mime as mime.ts
participant Server as Backend API
User->>CLI: storage upload ./photo.png --bucket images
CLI->>Mime: mimeTypeFromName("./photo.png")
Mime-->>CLI: "image/png"
CLI->>CLI: resolveUploadContentType(file, opts.contentType) → "image/png"
CLI->>CLI: "new Blob([fileContent], { type: "image/png" })"
CLI->>CLI: formData.append("file", blob, objectKey)
CLI->>Server: PUT /api/storage/buckets/images/objects/photo.png (multipart: Content-Type: image/png)
Server-->>CLI: 200 OK
CLI-->>User: Uploaded "photo.png" to bucket "images".
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant CLI as upload.ts
participant Mime as mime.ts
participant Server as Backend API
User->>CLI: storage upload ./photo.png --bucket images
CLI->>Mime: mimeTypeFromName("./photo.png")
Mime-->>CLI: "image/png"
CLI->>CLI: resolveUploadContentType(file, opts.contentType) → "image/png"
CLI->>CLI: "new Blob([fileContent], { type: "image/png" })"
CLI->>CLI: formData.append("file", blob, objectKey)
CLI->>Server: PUT /api/storage/buckets/images/objects/photo.png (multipart: Content-Type: image/png)
Server-->>CLI: 200 OK
CLI-->>User: Uploaded "photo.png" to bucket "images".
Reviews (3): Last reviewed commit: "chore: bump version to 0.1.91" | Re-trigger Greptile |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Review: fix(storage): set content type on CLI uploads
Summary: A clean, well-scoped bug fix that infers the MIME type from the file extension (with a --content-type override) so CLI uploads stop defaulting to application/octet-stream — the root cause and fix are correctly identified.
Requirements context: No matching spec/plan found under docs/specs/ (the only specs there cover the diagnose and db-migrations commands). Assessed against the PR description and Linear INS-358. The diff matches the stated intent precisely — three files, no scope creep.
Critical
(none)
Suggestion
Software engineering — no test for the command-level resolution logic (src/commands/storage/upload.ts:36-43)
The pure helper mimeTypeFromName is nicely covered, but the precedence chain that actually wires it up — explicit --content-type flag wins → inferred → application/octet-stream fallback, and that the resolved value lands on the Blob type — has no test. This repo does have command-level tests (e.g. src/commands/projects/link.test.ts, src/commands/payments/transactions.test.ts), so a small test asserting that opts.contentType overrides inference and that an unknown extension falls back would fit conventions and lock in the behavior. Non-blocking since the core logic is the well-tested pure helper.
Functionality — inference runs on the full path, not the basename (src/lib/mime.ts:77-81)
mimeTypeFromName calls name.lastIndexOf('.') on the whole string. For a path where a parent directory contains a dot and the file has no extension (e.g. /home/user.name/datafile), ext becomes name/datafile, which isn't a map key, so it returns undefined. This is safe — it can only ever degrade to the octet-stream fallback, never produce a wrong type, because no map key contains / — but it silently skips inference for those paths. Consider extracting the basename first (basename is already imported in upload.ts) so directory dots can't shadow a real extension.
Information
--content-type ""(empty string) (src/commands/storage/upload.ts:36-37):??only short-circuits onnull/undefined, so an explicit empty-string flag value would pass through as''. ABlobwith an empty type effectively serializes as octet-stream, so the outcome is benign, butopts.contentType || mimeTypeFromName(file) || '...'(or trimming) would treat empty as "not provided" if that's the desired semantics. Minor edge case.- Naming/imports are consistent with the codebase (
.jsextension on relative imports,src/lib/*.test.tscolocated tests). The lookup table is a reasonable curated set; unknown extensions correctly fall back, so there's genuinely no regression as claimed.
Dimension coverage
- Software engineering: Good — pure helper has thorough unit tests (case-insensitivity, multi-dot, missing/trailing-dot, dotfiles). Only gap is the untested command wiring (Suggestion above).
- Functionality: Solves INS-358 as described. Fallback chain is correct; the path-dot case (Suggestion) only ever degrades safely.
- Security: No security-relevant regressions. The
--content-typeuser value flows into theBlobtype → multipartContent-Type, but theBlobconstructor normalizes the type per the WHATWG spec (characters outside U+0020–U+007E force the type to""), so CRLF/header injection is not possible. No secrets or auth handling touched; theAuthorizationheader is unchanged. - Performance: No concerns — a static O(1) lookup table;
readFileSyncis pre-existing and unchanged.
Verdict: approved (informational)
No blocking issues. The two Suggestions (a command-level test and basename extraction) and the Information notes are worth addressing but do not gate merge. Final GitHub approval remains a human action.
- mimeTypeFromName: strip the directory portion before parsing the extension, so a dot in a parent directory (e.g. /home/user.name/datafile) can no longer shadow a real extension. - Extract resolveUploadContentType() and use `||` so an explicit empty/whitespace --content-type falls through to inference instead of forcing an empty type. - Add command-level tests for the precedence chain (override > inferred > fallback) and basename-only inference cases. Review feedback identified by jwfing on PR #168. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Why
Linear INS-358: storage uploads default to
application/octet-streaminstead of the actual file type.Verified against source that the CLI is the affected path, on every upload.
storage uploadbuilt the body withnew Blob([fileContent])and never set a type (src/commands/storage/upload.ts:33). A typelessBlobserializes its multipart part asapplication/octet-stream, and the backend trusts the client's value — it storesfile.mimetype || nulland the S3 provider falls back toapplication/octet-stream; it never infers from the extension. So every CLI-uploaded object got the wrong MIME type.What
aws s3 cpuses).--content-type <type>flag to override the inferred value.src/lib/mime.tshelper (small self-contained lookup, no new runtime dependency) + unit tests.application/octet-stream, so there is no regression.Test
vitest run src/lib/mime.test.ts✅ (case-insensitivity, paths, unknown/missing extensions)npm run lint✅ (0 errors) ·npm run build✅🤖 Generated with Claude Code
Summary by cubic
CLI storage uploads now send the correct MIME type instead of always
application/octet-stream, fixing INS-358. Also bumps@insforge/clito 0.1.91.Bug Fixes
application/octet-streamfor unknown/missing extensions.--content-type.New Features
--content-type <type>to override the inferred type.src/lib/mime.tsandresolveUploadContentTypewith unit tests; no new runtime dependencies.Written for commit 5ca75b3. Summary will update on new commits.
Note
Set MIME content type on
storage uploadCLI command based on file extension--content-typeflag to thestorage uploadcommand; if omitted, the type is inferred from the file extension via a newmimeTypeFromNameutility in mime.ts, falling back toapplication/octet-stream.mimeTypeFromNamecovers common extensions across images, documents, text/data, archives, audio, video, and fonts.Blobconstructor so the multipartfilepart carries the correctContent-Typeinstead of always sendingapplication/octet-stream.Changes since #168 opened
mimeTypeFromNameutility to extract file extensions from the basename only [77bb5bc]resolveUploadContentTypeutility and integrated it into storage upload command [77bb5bc]Macroscope summarized 0742071.
Summary by CodeRabbit
New Features
--content-typeflag for storage uploads, allowing explicit MIME type specification or automatic detection from file extension, withapplication/octet-streamas fallback.Chores