Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions internal/commands/cards.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"math"
"strconv"
"strings"
"time"

"github.com/spf13/cobra"

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
8 changes: 7 additions & 1 deletion internal/commands/comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion internal/commands/files.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
36 changes: 34 additions & 2 deletions internal/commands/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion internal/commands/messages.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion internal/commands/schedule.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
18 changes: 18 additions & 0 deletions internal/commands/stdin_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down