Reject three more doomed invocations before draining stdin - #645
Merged
Conversation
comments create tolerates individual bad IDs so a mixed batch still posts what it can, but an all-invalid argument creates nothing — and extractIDs is pure, so requireOneParseableTarget settles it before the read without disturbing the partial-success behavior. An explicitly blank --subscribe can never resolve to anyone, because resolvePersonIDs skips blank tokens. rejectSubscribeConflict now decides that too, ahead of the read at all three creates; applySubscribeFlags still calls it, so the message stays in one place. cards update accepted an unparseable --due: dateparse.Parse returns unrecognized input unchanged, so it failed only at the server, after the producer was spent. It now rejects locally, as todos update already did, and the parsed value is carried forward rather than re-derived. Six more tracking-reader cases, including two orderings that were already correct but unpinned (chat update --room, cards create --card-table).
github-actions
Bot
added
commands
There was a problem hiding this comment.
Pull request overview
Rejects deterministic argument errors before stdin is consumed.
Changes:
- Validates comment targets and card due dates before reading stdin.
- Rejects blank subscription values earlier.
- Adds integration coverage for failure ordering.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/commands/cards.go |
Validates and reuses parsed due dates. |
internal/commands/comment.go |
Validates comment targets before reading content. |
internal/commands/files.go |
Moves blank subscription rejection before document input. |
internal/commands/helpers.go |
Adds shared target and subscription validation. |
internal/commands/messages.go |
Rejects invalid subscriptions before message input. |
internal/commands/schedule.go |
Rejects invalid subscriptions before description input. |
internal/commands/stdin_integration_test.go |
Tests deterministic pre-stdin failures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 879e752b31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A whitespace-only --due skipped validation on cards update: the check tested the trimmed value while the no-change guard above tests due == "", so " " passed both and dateparse.Parse turned it into an empty date, sending an update with nothing in it. Every non-empty raw value is parsed now; the parser already trims a real date. A delimiter-only --subscribe passed the pre-read guard because trimming ",,," leaves commas. hasPersonToken splits the way resolvePersonIDs does, so the guard and the resolver cannot disagree about what counts as empty, and the same error arrives before the read rather than after it. Three more tracking-reader cases.
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
jeremy
added a commit
that referenced
this pull request
Aug 22, 2026
…or-roundtrip * origin/main: (96 commits) ci: bump the github-actions group with 6 updates (#639) Reject three more doomed invocations before draining stdin (#645) Stdin `-` support everywhere sensible; usage error for stray `-` elsewhere (#641) Add hey-cli Windows signing secrets to the release env manifest (#642) deps: bump the go-dependencies group with 5 updates (#638) Update nix flake and plugin version for v0.9.1 ci: bump the github-actions group with 4 updates (#633) Add basecamp files replace: publish a new version of an uploaded file (#634) Add basecamp files versions — HELD, blocked on the SDK (#622) Update nix flake and plugin version for v0.9.0 Make the Codex probe's timeout actually bound doctor (#629) Make the lockstep check catch stale agreement and .yaml workflows (#628) Keep refreshing opencode's other spelling (#627) Lint the release the same way we lint everything else (#625) Install the skill where opencode actually looks (#624) Take the communiques out of the source tree (#623) Correct the API coverage claim: 183/184, not 100% (#621) Stop echoing back step fields the caller never changed (#620) Drive the circuit breaker's clock from tests, not sleep() (#619) Tell agents the truth about card column moves (#618) ...
This was referenced Aug 22, 2026
Merged
jeremy
added a commit
that referenced
this pull request
Aug 22, 2026
readStdinContent was a bare io.ReadAll with no cap, and all 36 "-"-accepting call sites funnel through it. `yes | basecamp api post /valid --data -` is a perfectly valid invocation that read until the process died — which is why the recent pre-read ordering work (#641, #645) did not touch it: nothing here is a doomed invocation to reject early, the read itself was unbounded. Refuse rather than truncate. Silently posting the first megabyte would write partial content to Basecamp and report success, and a note or message is unrecoverable once saved. The cap counts bytes read, so it lands before the trailing-newline trim: an overflow that is only a trailing "\n" is still a refusal, because telling it apart from any other overflow would mean reading past the cap. `notes set --file <path>` was the file twin of the same read — a bare os.ReadFile — so it gets the same bound and the same error shape. Which side of the "-" the bytes arrive on no longer changes whether they are accepted. Worth naming: `boost create -` now stops at 1 MiB before its 16-rune check, instead of reading an unbounded stream and copying it to a string in order to reject 16 characters. maxStdinContent is declared alongside the stdin machinery rather than reusing maxAgentHookInput. Same value today, different purpose — one bounds a JSON envelope an agent harness writes, the other bounds prose a person pipes — and they should stay free to move apart. The reads left alone are internal/editor/editor.go and the TUI composer: both read back what the user's own $EDITOR just wrote, which is not a streaming source.
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
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.
Follow-up to #641, which was squash-merged with eleven review threads still open. Eight of those described code the later commits on that branch had already changed; three were still live and are fixed here.
What was still live
comments createwith an all-invalid target. The create loop deliberately tolerates individual bad IDs so a mixed batch still posts what it can, but when every comma-separated token is malformed the invocation creates nothing — andextractIDsis pure, so that is knowable from the argument alone.requireOneParseableTargetsettles it before the read; the partial-success behavior for mixed batches is untouched.An explicitly blank
--subscribe.resolvePersonIDsskips blank tokens, so--subscribe ""can never resolve to anyone.rejectSubscribeConflictnow decides that too, ahead of the read atdocs documents create,messages createandschedule create.applySubscribeFlagsstill calls it, so the message stays in one place.cards update --due.dateparse.Parsereturns unrecognized input unchanged, so an unparseable due date failed only at the server — after the producer had been spent. It now rejects locally, astodos updatealready did, and the parsed value is carried forward rather than re-derived.Why these matter
Same contract as #641: an invocation that is already doomed by its arguments must be rejected before stdin is drained. Otherwise the caller waits on a producer whose output is discarded, and a blank pipe answers "stdin is empty" instead of naming the real problem.
Tests
Six more cases in the tracking-reader table (42 total), each asserting the deterministic error, that stdin was never read, and that no request was issued. Two of the six pin orderings that were already correct but unpinned —
chat update --roomandcards create --card-table— so they cannot regress silently.bin/cigreen on top of merged main.Summary by cubic
Rejects three more doomed invocations before reading stdin and closes two blank-value gaps, so callers aren’t blocked on discarded producer output. Previously these cases read stdin and failed late; now they fail fast with clear usage errors.
Written for commit fbb3e73. Summary will update on new commits.