From 879e752b314f7e7e75edf8a301a77327499e003e Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 21 Aug 2026 00:25:43 -0700 Subject: [PATCH 1/2] Close three remaining pre-read holes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- internal/commands/cards.go | 18 ++++++++++++---- internal/commands/comment.go | 8 ++++++- internal/commands/files.go | 2 +- internal/commands/helpers.go | 24 +++++++++++++++++++-- internal/commands/messages.go | 2 +- internal/commands/schedule.go | 2 +- internal/commands/stdin_integration_test.go | 12 +++++++++++ 7 files changed, 58 insertions(+), 10 deletions(-) diff --git a/internal/commands/cards.go b/internal/commands/cards.go index c15a58cf..3daa23af 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -7,6 +7,7 @@ import ( "math" "strconv" "strings" + "time" "github.com/spf13/cobra" @@ -1108,10 +1109,20 @@ You can pass either a card ID or a Basecamp URL: } // Attachment paths are readable or not regardless of the body, so - // check them before the pipe is drained. + // check them before the pipe is drained. The due date too: + // dateparse.Parse returns unrecognized input unchanged, so a bad + // value fails only at the server, after the producer is spent. + // todos update already rejects it locally; match that. if err := validateAttachPaths(attachFiles); err != nil { return err } + var parsedDue string + if strings.TrimSpace(due) != "" { + parsedDue = dateparse.Parse(due) + if _, err := time.Parse("2006-01-02", parsedDue); err != nil { + return output.ErrUsage(fmt.Sprintf("Invalid due date: %q", due)) + } + } // Syntactic checks first, then "-", then account and network: a // malformed ID is answered without waiting on the producer, and a @@ -1160,9 +1171,8 @@ You can pass either a card ID or a Basecamp URL: if html != "" { req.Content = &html } - if due != "" { - dueOn := dateparse.Parse(due) - req.DueOn = &dueOn + if parsedDue != "" { + req.DueOn = &parsedDue } if cmd.Flags().Changed("assignee") { assigneeID, err := resolveAssigneeID(cmd.Context(), app, assignee) diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 16348cb5..bbc51ac8 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -1167,10 +1167,16 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: } // Attachment paths are readable or not regardless of the body, so - // check them before the pipe is drained. + // check them before the pipe is drained. So is whether any target + // is even a number: the loop below tolerates individual bad IDs so + // a mixed batch still posts, but when none can parse the invocation + // creates nothing, and that is knowable from the argument alone. if err := validateAttachPaths(attachFiles); err != nil { return err } + if err := requireOneParseableTarget(recordingArg); err != nil { + return err + } var content string if len(args) > 1 { diff --git a/internal/commands/files.go b/internal/commands/files.go index b00a9f41..4b117bb4 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -1272,7 +1272,7 @@ Use - as the content argument to read the document body from stdin: // Resolve "-" before any account or network work, so a bad stdin // gets the stdin error rather than "--account is required". - if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { + if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe, subscribe); err != nil { return err } if err := requireNumericID(*vaultID, "folder ID"); err != nil { diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index d4164d4e..6dec01f3 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -443,6 +443,20 @@ func extractIDs(args []string) []string { return urlarg.ExtractIDs(args) } +// requireOneParseableTarget rejects a recording argument whose every +// comma-separated token fails to parse. Callers tolerate individual bad IDs so +// a mixed batch still posts what it can, but an all-invalid argument creates +// nothing — and extractIDs is pure, so that is decidable from the argument +// alone, before a "-" drains the producer. +func requireOneParseableTarget(arg string) error { + for _, id := range extractIDs([]string{arg}) { + if _, err := strconv.ParseInt(id, 10, 64); err == nil { + return nil + } + } + return output.ErrUsage(fmt.Sprintf("no valid recording ID in %q", arg)) +} + // resolvePersonIDs splits a comma-separated input string and resolves each // token (name, email, ID, or "me") to a person ID via the name resolver. func resolvePersonIDs(ctx context.Context, resolver *names.Resolver, input string) ([]int64, error) { @@ -480,15 +494,21 @@ func resolvePersonIDs(ctx context.Context, resolver *names.Resolver, input strin // settle it first: draining a pipe for an invocation this rejects makes the // caller wait on a producer whose output is discarded, and lets a blank pipe // answer "stdin is empty" instead of naming the conflict. -func rejectSubscribeConflict(subscribeChanged, noSubscribe bool) error { +func rejectSubscribeConflict(subscribeChanged, noSubscribe bool, subscribe string) error { if subscribeChanged && noSubscribe { return output.ErrUsage("--subscribe and --no-subscribe are mutually exclusive") } + // resolvePersonIDs skips blank tokens, so a changed-but-blank value can + // never resolve to anyone. Deciding that here rather than after the lookup + // keeps it ahead of any stdin read. + if subscribeChanged && strings.TrimSpace(subscribe) == "" { + return output.ErrUsage("--subscribe requires at least one person") + } return nil } func applySubscribeFlags(ctx context.Context, resolver *names.Resolver, subscribe string, subscribeChanged, noSubscribe bool) (*[]int64, error) { - if err := rejectSubscribeConflict(subscribeChanged, noSubscribe); err != nil { + if err := rejectSubscribeConflict(subscribeChanged, noSubscribe, subscribe); err != nil { return nil, err } if noSubscribe { diff --git a/internal/commands/messages.go b/internal/commands/messages.go index 3e6c1b94..f221d251 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -461,7 +461,7 @@ Use - as the body argument to read the body from stdin: if edit && body != "" { return output.ErrUsage("cannot combine --edit and body argument") } - if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { + if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe, subscribe); err != nil { return err } if err := requireNumericID(*messageBoard, "message board ID"); err != nil { diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index 31f7a5e9..b0787de8 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -441,7 +441,7 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { return err } - if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { + if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe, subscribe); err != nil { return err } if err := requireNumericID(*scheduleID, "schedule ID"); err != nil { diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index 2332e422..a063d5f5 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -387,6 +387,18 @@ func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { []string{"post", "-", "--content-type", "bogus"}, "unsupported --content-type"}, {"todos update bad due date", NewTodosCmd, []string{"update", "1", "--due", "not-a-date", "--description", "-"}, "Invalid due date"}, + {"chat update bad room", NewChatCmd, + []string{"update", "5", "-", "--room", "nope"}, "Invalid chat room ID"}, + {"cards create bad card-table id", NewCardsCmd, + []string{"create", "Title", "-", "--column", "Backlog", "--card-table", "nope"}, "Invalid card table ID"}, + {"cards update bad due date", NewCardsCmd, + []string{"update", "1", "--due", "not-a-date", "--body", "-"}, "Invalid due date"}, + {"comments create all-invalid targets", NewCommentsCmd, + []string{"create", "nope,alsonope", "-"}, "no valid recording ID"}, + {"docs create blank subscribe", NewDocsCmd, + []string{"documents", "create", "Title", "-", "--subscribe", ""}, "requires at least one person"}, + {"messages create blank subscribe", NewMessagesCmd, + []string{"create", "Title", "-", "--subscribe", ""}, "requires at least one person"}, {"chat update bad content-type", NewChatCmd, []string{"update", "1", "-", "--content-type", "bogus"}, "unsupported --content-type"}, {"boost bad id", NewBoostsCmd, From fbb3e73fea51740c0fe59b21614f12fe77663848 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 21 Aug 2026 10:12:14 -0700 Subject: [PATCH 2/2] Close two blank-value gaps in the new pre-read checks 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. --- internal/commands/cards.go | 8 +++++++- internal/commands/helpers.go | 20 ++++++++++++++++---- internal/commands/stdin_integration_test.go | 6 ++++++ 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 3daa23af..4877b6e7 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -1116,8 +1116,14 @@ You can pass either a card ID or a Basecamp URL: if err := validateAttachPaths(attachFiles); err != nil { return err } + // Every non-empty value is parsed, not just non-blank ones: the + // no-change guard above tests due == "", so a whitespace-only + // --due passes it, and dateparse.Parse trims that to an empty + // date. Parsing it here answers "Invalid due date" instead of + // sending an update with nothing in it. Surrounding whitespace on + // a real date is already handled by the parser. var parsedDue string - if strings.TrimSpace(due) != "" { + if due != "" { parsedDue = dateparse.Parse(due) if _, err := time.Parse("2006-01-02", parsedDue); err != nil { return output.ErrUsage(fmt.Sprintf("Invalid due date: %q", due)) diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 6dec01f3..2854d102 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -457,6 +457,18 @@ func requireOneParseableTarget(arg string) error { return output.ErrUsage(fmt.Sprintf("no valid recording ID in %q", arg)) } +// hasPersonToken reports whether input holds at least one token resolvePersonIDs +// would attempt to resolve. It splits the same way, so the pre-read guard and +// the resolver cannot disagree about what counts as empty. +func hasPersonToken(input string) bool { + for token := range strings.SplitSeq(input, ",") { + if strings.TrimSpace(token) != "" { + return true + } + } + return false +} + // resolvePersonIDs splits a comma-separated input string and resolves each // token (name, email, ID, or "me") to a person ID via the name resolver. func resolvePersonIDs(ctx context.Context, resolver *names.Resolver, input string) ([]int64, error) { @@ -498,10 +510,10 @@ func rejectSubscribeConflict(subscribeChanged, noSubscribe bool, subscribe strin if subscribeChanged && noSubscribe { return output.ErrUsage("--subscribe and --no-subscribe are mutually exclusive") } - // resolvePersonIDs skips blank tokens, so a changed-but-blank value can - // never resolve to anyone. Deciding that here rather than after the lookup - // keeps it ahead of any stdin read. - if subscribeChanged && strings.TrimSpace(subscribe) == "" { + // resolvePersonIDs skips blank tokens, so a value with no resolvable token + // can never name anyone — ",,," reaches the same error as "". Deciding it + // here rather than after the lookup keeps it ahead of any stdin read. + if subscribeChanged && !hasPersonToken(subscribe) { return output.ErrUsage("--subscribe requires at least one person") } return nil diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index a063d5f5..3477cf88 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -393,6 +393,12 @@ func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { []string{"create", "Title", "-", "--column", "Backlog", "--card-table", "nope"}, "Invalid card table ID"}, {"cards update bad due date", NewCardsCmd, []string{"update", "1", "--due", "not-a-date", "--body", "-"}, "Invalid due date"}, + {"cards update whitespace-only due date", NewCardsCmd, + []string{"update", "1", "--due", " ", "--body", "-"}, "Invalid due date"}, + {"docs create delimiter-only subscribe", NewDocsCmd, + []string{"documents", "create", "Title", "-", "--subscribe", ",,,"}, "requires at least one person"}, + {"schedule create delimiter-only subscribe", NewScheduleCmd, + []string{"create", "Title", "--starts-at", "2026-01-01T10:00:00Z", "--ends-at", "2026-01-01T11:00:00Z", "--subscribe", ", ,", "--description", "-"}, "requires at least one person"}, {"comments create all-invalid targets", NewCommentsCmd, []string{"create", "nope,alsonope", "-"}, "no valid recording ID"}, {"docs create blank subscribe", NewDocsCmd,