diff --git a/internal/commands/cards.go b/internal/commands/cards.go index c15a58cf..4877b6e7 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,26 @@ 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 } + // 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 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 +1177,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..2854d102 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -443,6 +443,32 @@ 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)) +} + +// 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) { @@ -480,15 +506,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 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 } 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..3477cf88 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -387,6 +387,24 @@ 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"}, + {"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, + []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,