Reject three more doomed invocations before draining stdin - #645
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.
|
@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". |
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.