feat(jtk): detect attachment MIME type and add --filename override for #465 - #466
Conversation
…#465 Uploads previously sent every file as application/octet-stream, so a screenshot attached to an issue wasn't recognized as an image by Jira (no thumbnail or inline preview). Derive the multipart part's Content-Type from the file extension, falling back to octet-stream for unknown types. Add a --filename flag to jtk attachments add so an attachment can be stored under a chosen name instead of the file's base name (valid with a single --file only). Also adds the previously-missing AddAttachment test coverage.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 029f0bae0488
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 2 |
| policies:conventions | 0 |
go:implementation-tests (2 findings)
Major - tools/jtk/api/attachments.go:23
quoteEscaperand its use inattachmentPartHeader(line 215) exist specifically so a user-supplied--filenameoverride containing a"or\cannot break out of theContent-Dispositionheader — the doc comment says so explicitly. Before this PR the stored name was alwaysfilepath.Base(filePath), a value the OS already constrains; now it can be arbitrary user input via--filename, making this escaping the load-bearing safety net for a new, untrusted input path. Neitherattachments_test.go(api) nor the cmd-level tests exercise an override containing a quote or backslash, so a regression in the escaping (e.g. reordering thestrings.NewReplacerpairs, or dropping the call) would ship undetected. Add a case toTestAddAttachmentwithoverride:before"exploit.png`" (or similar) and assert the multipart reader still parses a single part with the expected literal filename, proving the header isn't corrupted.
Minor - tools/jtk/internal/cmd/attachments/attachments_test.go:307
TestRunAdd_FilenameRequiresSingleFileonly proves the guard clause rejects--filenamewith multiple files; no test drivesrunAddend-to-end with a single file plus--filenameand asserts the server-observed filename matches the override.AddAttachment(ctx, issueKey, filePath, filename string)takes three same-typed string args, so a copy-paste/order regression inrunAdd's call (attachments.go:157) wouldn't be caught by the api-layer test (which callsclient.AddAttachmentdirectly) or by this guard-only test. Add a success-path test mirroringTestRunAdd_Successthat passes a non-emptyfilenameand asserts the httptest server received that name viapart.FileName().
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | tools/jtk/api/attachments.go, tools/jtk/api/attachments_test.go, tools/jtk/internal/cmd/attachments/attachments.go, tools/jtk/internal/cmd/attachments/attachments_test.go | unavailable | unavailable |
| policies:conventions | complete_broad | tools/jtk/README.md, tools/jtk/internal/cmd/attachments/attachments.go | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 16s | unavailable | claude-sonnet-5 | cr 0.10.264
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 16s wall · 4m 39s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 36s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 2m 14s |
| policies:conventions | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 34s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 14s |
Address review findings on #466: - TestAddAttachment now exercises override names containing a quote and a backslash, asserting the filename round-trips literally and that exactly one multipart part is produced — locking in quoteEscaper as the safety net for the new untrusted --filename input. - TestRunAdd_FilenameOverride drives runAdd end-to-end with a single file plus --filename and asserts the server observes the overridden name, guarding the cmd-layer call against arg-order regressions.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 4733699c161f
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | tools/jtk/api/attachments.go, tools/jtk/api/attachments_test.go, tools/jtk/internal/cmd/attachments/attachments.go, tools/jtk/internal/cmd/attachments/attachments_test.go | unavailable | unavailable |
| policies:conventions | complete_broad | tools/jtk/README.md, tools/jtk/internal/cmd/attachments/attachments.go | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 39s | unavailable | claude-sonnet-5 | cr 0.10.264
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 2m 39s wall · 3m 11s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 15s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 49s |
| policies:conventions | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 55s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 11s |
2 participants
Closes #465.
What the issue asked for vs. what already exists
The bulk of #465 — "attach a file to an issue" — already ships today as
jtk attachments add <issue-key> --file <path>(list/add/get/delete under theattachmentsgroup; API in place since #50, registered inmain.go, documented in the README). So this PR does not re-add attach support.The issue proposed the command as
jtk issues attach …, which is the likely reason it read as missing: attach lives in the top-levelattachmentsgroup, not underissues, sojtk issues --helpdoesn't surface it. That's a findability gap, not a docs-accuracy gap. I deliberately did not add a duplicatejtk issues attachalias — the repo'sGUARDRAILS.mddefinesattachments/addas the canonical resource/verb, and duplicating it underissueswould cut against that design language.What this PR does implement are the two concrete gaps the issue lists as nice-to-haves, which are the only parts of the existing feature that were genuinely absent.
Changes
Content-Typeis now derived from the file extension (mime.TypeByExtension), falling back toapplication/octet-stream.--filenameoverride onjtk attachments add— store the attachment under a chosen name instead of the file's base name. Valid with a single--fileonly (errors clearly otherwise).AddAttachment, which previously had none — asserts the wire filename, the detectedContent-Type, the octet-stream fallback, and the empty-arg guards.Why
The motivating use case in the issue is attaching screenshots. Before this change every upload was sent as
application/octet-stream, so Jira didn't recognize a.png/.pdfas an image/document — no thumbnail, no inline preview, it renders as a generic download. Detecting the MIME type from the extension fixes that.--filenamecovers the common case of uploading a temp file with an ugly generated name (/tmp/out-8f8a7b.png) but wanting it stored as something meaningful (before.png).Deliberately deferred
The issue's optional ask — referencing an uploaded attachment inline in a comment/description via Jira's
!filename!media syntax — is not included. jtk renders comments/descriptions as ADF (v3 API), and the sharedadfpackage has no media-node support; wiring inline embeds would need new shared node types plus a media-services UUID the v3 attachment API doesn't return, and it can't be verified without a live Jira. That's a larger, separately-scoped change and is left for a follow-up.Testing
go test -race ./tools/jtk/...— passgolangci-lint run(tools/jtk) — 0 issuesgo mod tidy— clean; cross-compile release guard (linux/windows) — pass--filenameappears inattachments add --help; the single---filevalidation errors with exit 1 before auth is attempted.