feat: add --name flag for reading file bytes from stdin#20
Merged
Conversation
Pass `-` as the single file arg with --name BASENAME to read image
bytes from stdin, so tools like screencapture, magick, and pbpaste
can pipe directly into `gh attach` without a temp file:
screencapture -i -t png - | gh attach --name shot.png 123 -
magick input.png -resize 50% - | gh attach --name small.png --key docs/diagram -
pbpaste | gh attach --name clipboard.png 42 -
## How it works
The stdin reader is threaded through runDeps (same spot the delete
confirmation prompt reads from) and, when `-` is the sole file arg,
materialized into a fresh `os.MkdirTemp` directory under the
user-chosen basename. gh.PushAttachments then sees a normal
filesystem path and stays completely unchanged — the gh package
never learns about stdin. A deferred cleanup closure removes the
temp dir when runUpload returns.
## Validation
--name is required with `-`, rejected without, and validated as a
safe basename: non-empty, <=255 bytes, no `/` or `\`, not `.` or
`..`, no NUL bytes. `-` mixed with other file args is an error
(scanning the positional list gives a clear message instead of an
opaque "no files matched" from expandFiles).
Empty stdin is allowed (produces a 0-byte upload) so upstream tools
that pipe nothing fail loud at the subsequent embed instead of
silently no-op'ing here.
Works with --key, --comment, --json, and --repo exactly like a
disk-backed upload.
## Refactor along the way
Moved `stdin io.Reader` from a positional parameter on runWithDeps
and runDelete into runDeps itself, now that two subcommands
(delete confirmation + upload-from-stdin) need to read it. Keeps
the subcommand signatures short and matches how other injected
dependencies are passed. Test helpers (happyDeps, deleteDeps,
listDeps) default to `strings.NewReader("")` so tests that don't
care about stdin still have a valid reader.
## Test + coverage
New tests in internal/cli:
- TestValidateName (2 subtests, 15 cases)
- TestMaterializeStdin (4 subtests: happy path, empty input,
cleanup, io.Copy error via errReader)
- TestRunUpload_stdin_issue_mode / _key_mode / _with_comment /
_empty_allowed / _json / _via_runWithDeps
- TestRunUpload_stdin_arg_conflicts (6 table cases)
fakeGitClient gains an optional `savePushContent` flag that
snapshots file bytes at PushAttachments call time; stdin tests
use it because runUpload's deferred temp cleanup removes the file
before the test body runs assertions.
Total coverage: 96.2% (95.2% → 96.2% within this branch).
## Smoke test
End-to-end tested against #19 in both
issue and key modes, including byte-level verification of the
uploaded blob via the /contents API (base64 round-trip matched
the piped 1x1 PNG exactly). Test refs + issue were cleaned up
afterward.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to upload files from stdin by passing - as a file argument and providing a mandatory --name flag. The implementation includes new utility functions for filename validation and temporary file materialization, alongside updates to the dependency injection structure to support stdin across subcommands. Review feedback suggests enhancing the filename validation to include more reserved characters for better cross-platform support and refactoring the runUpload function to use an options struct to improve readability and maintainability.
The coverage bot flagged 5 new uncovered statements in files.go
(materializeStdin's MkdirTemp / Create / Close error branches, which
real OS fault injection can't reach cleanly) and 1 in run.go (the
`if stdinErr != nil { return stdinErr }` branch in runUpload).
## files.go refactor
Introduce a tiny injection seam: stdinFS { mkdirTemp, create } and
defaultStdinFS wiring those to os.MkdirTemp + a lambda around
os.Create (so the signature can return the narrower io.WriteCloser
instead of *os.File, which keeps fakes trivial). The public
materializeStdin wrapper is unchanged and just delegates to the new
materializeStdinWith(fs, stdin, name). Production call sites stay
exactly the same.
## New tests
TestMaterializeStdinWith (4 subtests):
- mkdirTemp error — returns immediately, no temp dir to clean
- create error — asserts the real temp dir is removed after failure
- close error — fakeWriteCloser with Write=nil, Close=err; verifies
temp dir cleanup
- write error — fakeWriteCloser with Write=err (symmetric to the
existing errReader test, confirms error wrapping is the same
regardless of which side of io.Copy fails)
TestRunUpload_stdin_read_error exercises the runUpload stdinErr
branch end-to-end by injecting errReader as deps.stdin and asserting
PushAttachments is never called.
## Coverage
- files.go: 89.13% → 100% (materializeStdin + materializeStdinWith
both at 100%)
- run.go: 97.08% → 97.81% (runUpload 97.6% → 98.8%; remaining 1.2%
is a pre-existing json.Encode error branch from PR #18, not mine)
- internal/cli total: 96.18% → 98.1%, above the pre-PR baseline
of 97.69%
… struct Two changes from gemini-code-assist review on #20: ## validateName: reject Windows-reserved characters Previous validation only blocked Unix path separators ('/', '\'), but the docstring promised "legal on every common filesystem". Windows also reserves '<', '>', ':', '"', '|', '?', '*' in file names — uploads containing those characters would round-trip fine on Linux/macOS but corrupt a Windows checkout. Block them here so the error surfaces at `gh attach --name` time with a clear message instead of at `git clone` time on a Windows box. Added 7 new subtest cases covering each rejected character. The set lives in a `nameReservedChars` const so the intent is explicit and the reference to the MSDN naming-conventions doc sits next to the constant that enforces it. ## runUpload: uploadOptions struct runUpload had grown to 11 positional parameters, which was hard to read at the call site and forced every test to pass `""`/`false` for unused fields. Introduce an `uploadOptions` struct bundling the parsed-flag state (number, filePaths, title, postComment, repoOverride, key, name, asJSON). The single production caller in runWithDeps now builds a labeled literal — no more guessing which positional is which. Function body reads `opts.X` directly, with the exception of `number` which is copied into a local because it's mutated by PR auto-detection. No semantic change — this is a pure readability refactor. All 23 test call sites updated; many become shorter because zero-valued fields can be omitted from the struct literal. Tests still green, coverage unchanged at 98.1%, golangci-lint 0 issues.
Member
Author
|
Addressed both review comments in d3ff5a8:
Tests + vet + lint all green, coverage unchanged at 98.1%. |
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
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
Adds a way to upload images from stdin instead of disk, so tools that emit bytes to a pipe — screen capture, image processing, clipboard readers — can compose directly with
gh attach:Pass
-as the single file argument and--name BASENAMEto supply the git tree entry + embed URL name. Works with--key,--comment,--json, and--repothe same way a disk-backed upload does.How it works
runDepsnow carriesstdin io.Reader(moved from a positional parameter onrunWithDeps/runDelete, since two subcommands — delete confirmation + stdin upload — need it).-is the sole file arg,materializeStdindrains the reader into a freshos.MkdirTempdirectory under--name, and defers cleanup.gh.PushAttachmentsstill receives a normal filesystem path — the gh package is untouched.Validation
--nameis required with-, rejected without.--namemust be a safe basename: non-empty, ≤255 bytes, no//\, not.or.., no NUL bytes.-mixed with any other file arg is an explicit error (rather than an opaque "no files matched" fromexpandFiles).Test plan
TestValidateName— 7 valid + 8 invalid basenamesTestMaterializeStdin— happy path, empty input, cleanup,io.Copyerror branch via anerrReaderTestRunUpload_stdin_issue_mode— end-to-end byte fidelity viafakeGitClient.savePushContentTestRunUpload_stdin_key_mode— refPath =uploads/misc/<key>, bytes matchTestRunUpload_stdin_with_comment—UpsertCommentfires,Commented:stderr line presentTestRunUpload_stdin_empty_allowed— 0-byte file passed through cleanlyTestRunUpload_stdin_json— JSON output suppresses stderr, parses back cleanlyTestRunUpload_stdin_via_runWithDeps— flag parsing + positional extraction end-to-endTestRunUpload_stdin_arg_conflicts— 6 table cases (dash without name, name without dash, dash mixed with files, path in name,.as name, etc.)nameparameter)go vetclean,golangci-lint0 issuesmaterializeStdin55.6% → 72.2% — remaining uncovered branches require OS-level fault injection)enthus-appdev/gh-attach#19in both issue and key modes, with byte-level verification of the uploaded blob via the/contentsAPI. Test refs + issue cleaned up.