From b81a070f076357e035e185b17b17818efe0a7677 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 18 Aug 2026 20:45:36 -0700 Subject: [PATCH 01/19] Honor - (stdin) on every content input; reject stray - when piped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agents instinctively pass - to mean "read content from stdin", but only comments create/update honored it — everywhere else the hyphen landed as literal content (a todo titled "-", a message body of "-"). Tier 1: - now reads stdin on every content-kind positional (comments create/update, checkins answer create/update, todos create, messages create [body], cards create [body], docs create [content], chat post/update, boost create, notes set) and content flag (--data on api post/put, --body, --content, --description, --comment on todos sweep, --file on notes set). Resolution lives in internal/commands/stdin.go; the shared vocabulary (allow_dash annotation, pipe detection) in the new internal/stdinarg leaf package, since internal/cli needs it too for agent help. Tier 2: everywhere else, a literal - combined with piped stdin is ambiguous — the caller almost certainly meant the pipe — so a central guard wrapped around every RunE in the tree rejects it with a usage error naming the offender, pointing at where the command does accept stdin, and teaching the -- escape for a literal hyphen. On a TTY, literal - stays legal everywhere. --out - (attachments/files download) is exempted as the stdout idiom. Behavior changes: - "comments create 123 - extra" — was a silent literal "- extra" comment, now a usage error. - "-" with TTY stdin — was hang-until-Ctrl-D, now an immediate usage error teaching the escapes (pipe, heredoc, cat |, --edit where it exists). No new --stdin flag: - is the universal idiom, and --stdin in the wild means other things (git plumbing, kubectl). - Bare-pipe auto-read removed from comments create and notes set: a pipe without - errors with a hint instead of being silently consumed. Pipes are only ever a source through an explicit -; an unclaimed pipe alongside a named source is ignored, the CLI-wide rule. - "notes set -" (piped) — was a bogus two-source error, now works; "notes set --file -" — was ENOENT on a file named -, now stdin. - Piped scripts passing literal - as a title/name/path now error; -- is the documented escape. - Stdin content gets trailing newlines trimmed (Markdown doesn't care; titles and boost's 16-rune limit do). Agent help auto-documents each command's stdin inputs from the allow_dash annotation; SKILL.md generalizes the - idiom it previously over-promised. --- e2e/stdin_dash.bats | 69 ++++++ internal/cli/root.go | 42 ++++ internal/commands/api.go | 32 ++- internal/commands/attachments.go | 3 + internal/commands/boost.go | 8 +- internal/commands/cards.go | 40 ++- internal/commands/chat.go | 35 ++- internal/commands/checkins.go | 14 +- internal/commands/commands_test.go | 1 + internal/commands/comment.go | 45 ++-- internal/commands/comment_test.go | 29 ++- internal/commands/dash_guard_test.go | 151 ++++++++++++ internal/commands/files.go | 53 +++- internal/commands/gauges.go | 17 +- internal/commands/helpers.go | 20 -- internal/commands/messages.go | 26 +- internal/commands/notes.go | 64 +++-- internal/commands/notes_test.go | 74 +++++- internal/commands/projects.go | 18 +- internal/commands/schedule.go | 19 +- internal/commands/stdin.go | 255 ++++++++++++++++++++ internal/commands/stdin_integration_test.go | 151 ++++++++++++ internal/commands/stdin_test.go | 160 ++++++++++++ internal/commands/templates.go | 27 ++- internal/commands/todolists.go | 18 +- internal/commands/todos.go | 39 ++- internal/stdinarg/stdinarg.go | 93 +++++++ internal/stdinarg/stdinarg_test.go | 67 +++++ skills/basecamp/SKILL.md | 22 +- 29 files changed, 1444 insertions(+), 148 deletions(-) create mode 100644 e2e/stdin_dash.bats create mode 100644 internal/commands/dash_guard_test.go create mode 100644 internal/commands/stdin.go create mode 100644 internal/commands/stdin_integration_test.go create mode 100644 internal/commands/stdin_test.go create mode 100644 internal/stdinarg/stdinarg.go create mode 100644 internal/stdinarg/stdinarg_test.go diff --git a/e2e/stdin_dash.bats b/e2e/stdin_dash.bats new file mode 100644 index 000000000..b56f4c7d3 --- /dev/null +++ b/e2e/stdin_dash.bats @@ -0,0 +1,69 @@ +#!/usr/bin/env bats +# stdin_dash.bats - "-" (read from stdin) support and the tier-2 dash guard. +# +# Tier 1: content inputs accept "-" to read piped stdin. Tier 2: everywhere +# else, a literal "-" combined with piped stdin is a usage error instead of +# silently becoming literal content. All cases here fail before any HTTP, so +# no cassette or server is needed. + +load test_helper + + +# Tier 1 — "-" resolves against stdin + +@test "todos create - with empty pipe is a usage error, not an empty todo" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run bash -c "printf '' | basecamp todos create - --json" + assert_failure + assert_json_value '.code' 'usage' + assert_output_contains "empty" +} + +@test "comments create - on a TTY-like stdin errors immediately instead of hanging" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + # /dev/null is a character device — the TTY stand-in. Must not block. + run bash -c "basecamp comments create 123 - --json < /dev/null" + assert_failure + assert_json_value '.code' 'usage' + assert_output_contains "nothing is piped" +} + +@test "comments create with a bare pipe and no dash teaches the dash" { + create_credentials + create_global_config '{"account_id": 99999, "project_id": 123}' + + run bash -c "printf 'hello' | basecamp comments create 123 --json" + assert_failure + assert_json_value '.code' 'usage' + assert_json_value '.hint | contains("pass \"-\"")' 'true' +} + + +# Tier 2 — stray literal "-" with a pipe is rejected + +@test "projects create - with piped stdin is rejected with the -- escape" { + create_credentials + create_global_config '{"account_id": 99999}' + + run bash -c "printf 'x' | basecamp projects create - --json" + assert_failure + assert_json_value '.code' 'usage' + assert_json_value '.error | contains("")' 'true' + assert_json_value '.hint | contains("after the -- separator")' 'true' +} + +@test "projects create -- - with piped stdin passes the guard" { + # The -- separator makes the "-" literal; the command proceeds past the + # guard and fails on the (unreachable) API instead of on usage. + export BASECAMP_BASE_URL="http://127.0.0.1:1" + create_credentials + create_global_config '{"account_id": 99999}' + + run bash -c "printf 'x' | basecamp projects create --json -- -" + assert_failure + [[ "$output" != *'does not read stdin'* ]] +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 49a6c7846..ccc028354 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -20,6 +20,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/harness" "github.com/basecamp/basecamp-cli/internal/hostutil" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -352,6 +353,10 @@ func Execute() { cmd.AddCommand(commands.NewBonfireCmd()) cmd.AddCommand(commands.NewAgentHookCmd()) + // Tier-2 stdin guard: reject a stray literal "-" when stdin is piped, + // everywhere a command doesn't explicitly accept it. + commands.InstallDashGuard(cmd) + // Use ExecuteC to get the executed command (for correct context access) executedCmd, err := cmd.ExecuteC() @@ -762,6 +767,12 @@ func emitAgentHelp(cmd *cobra.Command) { } } + // Synthesize the stdin note from the allow_dash annotation, so every + // command that accepts "-" auto-documents it. + if note := stdinDashNote(cmd, info.Args); note != "" { + info.Notes = append(info.Notes, note) + } + // Subcommands (include aliases so the CLI surface snapshot tracks them) for _, sub := range cmd.Commands() { if sub.IsAvailableCommand() || sub.Name() == "help" { @@ -826,3 +837,34 @@ func emitAgentHelp(cmd *cobra.Command) { _ = json.NewEncoder(cmd.OutOrStdout()).Encode(info) } + +// stdinDashNote renders the "-" (stdin) inputs a command accepts, from its +// allow_dash annotation: positionals by their Use-string names, flags by +// --name. Returns "" when the command reads no stdin input — --out's "-" +// means stdout, so it never appears here. +func stdinDashNote(cmd *cobra.Command, args []ArgInfo) string { + allow := stdinarg.ParseAllow(cmd.Annotations[stdinarg.AnnotationAllowDash]) + if allow.Empty() { + return "" + } + + var parts []string + for i, a := range args { + if allow.Arg(i) { + if a.Required { + parts = append(parts, "<"+a.Name+">") + } else { + parts = append(parts, "["+a.Name+"]") + } + } + } + for _, token := range strings.Fields(cmd.Annotations[stdinarg.AnnotationAllowDash]) { + if name, ok := strings.CutPrefix(token, "flag:"); ok && name != "out" { + parts = append(parts, "--"+name) + } + } + if len(parts) == 0 { + return "" + } + return "Pass - to read from stdin: " + strings.Join(parts, ", ") +} diff --git a/internal/commands/api.go b/internal/commands/api.go index 959df7628..fa70ee683 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -74,7 +74,10 @@ func newAPIPostCmd() *cobra.Command { cmd := &cobra.Command{ Use: "post ", Short: "POST request to API", - Long: "Make a raw POST request to any Basecamp API endpoint.", + Long: `Make a raw POST request to any Basecamp API endpoint. + +Use --data - to read the JSON body from stdin: + printf '{"content":"Buy milk"}' | basecamp api post buckets/1/todolists/2/todos.json --data -`, Example: ` basecamp api post buckets/123/todolists/456/todos.json -d '{"content":"Buy milk"}' basecamp api post buckets/123/message_boards/789/messages.json -d '{"subject":"Hello","content":"

World

"}'`, Args: apiPathArgs, @@ -84,6 +87,11 @@ func newAPIPostCmd() *cobra.Command { return missingArg(cmd, "--data") } + data, err := resolveContentValue(cmd, data, -1, "--data") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { return err @@ -116,7 +124,9 @@ func newAPIPostCmd() *cobra.Command { }, } - cmd.Flags().StringVarP(&data, "data", "d", "", "JSON request body (required)") + cmd.Flags().StringVarP(&data, "data", "d", "", "JSON request body (required); use - to read from stdin") + + allowDash(cmd, "flag:data") return cmd } @@ -125,9 +135,12 @@ func newAPIPutCmd() *cobra.Command { var data string cmd := &cobra.Command{ - Use: "put ", - Short: "PUT request to API", - Long: "Make a raw PUT request to any Basecamp API endpoint.", + Use: "put ", + Short: "PUT request to API", + Long: `Make a raw PUT request to any Basecamp API endpoint. + +Use --data - to read the JSON body from stdin: + printf '{"content":"Updated"}' | basecamp api put buckets/1/todos/2.json --data -`, Example: ` basecamp api put buckets/123/todos/456.json -d '{"content":"Updated todo"}'`, Args: apiPathArgs, RunE: func(cmd *cobra.Command, args []string) error { @@ -136,6 +149,11 @@ func newAPIPutCmd() *cobra.Command { return missingArg(cmd, "--data") } + data, err := resolveContentValue(cmd, data, -1, "--data") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { return err @@ -168,7 +186,9 @@ func newAPIPutCmd() *cobra.Command { }, } - cmd.Flags().StringVarP(&data, "data", "d", "", "JSON request body (required)") + cmd.Flags().StringVarP(&data, "data", "d", "", "JSON request body (required); use - to read from stdin") + + allowDash(cmd, "flag:data") return cmd } diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index e1b383294..44bf3baa3 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -395,6 +395,9 @@ Options: cmd.Flags().IntVar(&index, "index", 0, "Select attachment by 1-based index") cmd.Flags().StringVarP(&recordType, "type", "t", "", "Recording type hint (todo, todolist, message, comment, card, card-table, document, schedule-entry, checkin, answer, forward, upload)") + // --out - means stream to stdout — exempt from the stdin dash guard. + allowDash(cmd, "flag:out") + return cmd } diff --git a/internal/commands/boost.go b/internal/commands/boost.go index 66d05e879..3e64c845e 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -271,15 +271,21 @@ Use --event to boost a specific event within the item.`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + content, err := resolveContentValue(cmd, args[1], 1, "") + if err != nil { + return err + } if err := ensureAccount(cmd, app); err != nil { return err } - return runBoostCreate(cmd, app, args[0], *project, args[1], eventID) + return runBoostCreate(cmd, app, args[0], *project, content, eventID) }, } cmd.Flags().StringVar(&eventID, "event", "", "Event ID (for event-specific boosts)") + allowDash(cmd, "arg:1") + return cmd } diff --git a/internal/commands/cards.go b/internal/commands/cards.go index bfb38631e..0c8469d8b 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -851,7 +851,10 @@ func newCardsCreateCmd(project, cardTable *string) *cobra.Command { cmd := &cobra.Command{ Use: "create [body]", Short: "Create a new card", - Long: "Create a new card in a project's card table.", + Long: `Create a new card in a project's card table. + +Use - as the body argument to read the body from stdin: + printf 'Card body' | basecamp cards create "My card" - --in myproject`, Example: ` basecamp cards create "My card" --in myproject basecamp cards create --in myproject -- "--title with dashes"`, RunE: func(cmd *cobra.Command, args []string) error { @@ -866,7 +869,11 @@ func newCardsCreateCmd(project, cardTable *string) *cobra.Command { } var content string if len(args) > 1 { - content = args[1] + var err error + content, err = resolveContentValue(cmd, args[1], 1, "[body]") + if err != nil { + return err + } } app := appctx.FromContext(cmd.Context()) @@ -1051,6 +1058,8 @@ func newCardsCreateCmd(project, cardTable *string) *cobra.Command { cmd.Flags().StringVar(&assignee, "to", "", "Assignee (alias for --assignee)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") + allowDash(cmd, "arg:1") + completer := completion.NewCompleter(nil) _ = cmd.RegisterFlagCompletionFunc("assignee", completer.PeopleNameCompletion()) _ = cmd.RegisterFlagCompletionFunc("to", completer.PeopleNameCompletion()) @@ -1097,6 +1106,11 @@ You can pass either a card ID or a Basecamp URL: if title != "" { req.Title = &title } + content, err = resolveContentValue(cmd, content, -1, "--body") + if err != nil { + return err + } + var mentionNotice string var html string if content != "" { @@ -1160,7 +1174,7 @@ You can pass either a card ID or a Basecamp URL: } cmd.Flags().StringVarP(&title, "title", "t", "", "New title") - cmd.Flags().StringVarP(&content, "body", "b", "", "New body content") + cmd.Flags().StringVarP(&content, "body", "b", "", "New body content; use - to read from stdin") cmd.Flags().StringVarP(&due, "due", "d", "", "Due date (natural language or YYYY-MM-DD)") cmd.Flags().StringVar(&assignee, "assignee", "", "Assignee ID or name") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") @@ -1169,6 +1183,8 @@ You can pass either a card ID or a Basecamp URL: completer := completion.NewCompleter(nil) _ = cmd.RegisterFlagCompletionFunc("assignee", completer.PeopleNameCompletion()) + allowDash(cmd, "flag:body") + return cmd } @@ -2022,6 +2038,11 @@ func newCardsColumnCreateCmd(project, cardTable *string) *cobra.Command { app := appctx.FromContext(cmd.Context()) + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -2085,7 +2106,9 @@ func newCardsColumnCreateCmd(project, cardTable *string) *cobra.Command { }, } - cmd.Flags().StringVarP(&description, "description", "d", "", "Column description") + cmd.Flags().StringVarP(&description, "description", "d", "", "Column description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } @@ -2108,6 +2131,11 @@ You can pass either a column ID or a Basecamp URL: return noChanges(cmd) } + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { @@ -2138,7 +2166,9 @@ You can pass either a column ID or a Basecamp URL: } cmd.Flags().StringVarP(&title, "title", "t", "", "New title") - cmd.Flags().StringVarP(&description, "description", "d", "", "New description") + cmd.Flags().StringVarP(&description, "description", "d", "", "New description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 47f765353..66d89ea97 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -308,15 +308,28 @@ By default, messages are sent as plain text. Use --content-type text/html for rich text (HTML) messages. @mentions (@Name or @First.Last) are resolved automatically and the -content type is promoted to text/html when mentions are present.`, +content type is promoted to text/html when mentions are present. + +Use - as the message argument to read the message from stdin: + printf 'Build is green' | basecamp chat post - --in my-project`, Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - // Validate user input first, before checking account + // Validate user input first, before checking account. The + // positional wins over --content before "-" resolution, so only + // the winning source can consume stdin. messageContent := content + argIndex, what := -1, "--content" if len(args) > 0 { messageContent = args[0] + argIndex, what = 0, "<message>" + } + + var err error + messageContent, err = resolveContentValue(cmd, messageContent, argIndex, what) + if err != nil { + return err } // Show help when invoked with no message content @@ -332,10 +345,12 @@ content type is promoted to text/html when mentions are present.`, }, } - cmd.Flags().StringVar(&content, "content", "", "Message content") + cmd.Flags().StringVar(&content, "content", "", "Message content; use - to read from stdin") cmd.Flags().StringVar(contentType, "content-type", "", "Content type (text/html for rich text)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") + allowDash(cmd, "arg:0", "flag:content") + return cmd } @@ -766,9 +781,19 @@ edit to rich text.`, return missingArg(cmd, "<id|url>") } + // The positional wins over --content before "-" resolution, so + // only the winning source can consume stdin. messageContent := content + argIndex, what := -1, "--content" if len(args) > 1 { messageContent = args[1] + argIndex, what = 1, "[content]" + } + + var contentErr error + messageContent, contentErr = resolveContentValue(cmd, messageContent, argIndex, what) + if contentErr != nil { + return contentErr } if strings.TrimSpace(messageContent) == "" { @@ -958,9 +983,11 @@ edit to rich text.`, }, } - cmd.Flags().StringVar(&content, "content", "", "New message content") + cmd.Flags().StringVar(&content, "content", "", "New message content; use - to read from stdin") cmd.Flags().StringVar(contentType, "content-type", "", "Input handling: text/html (supply HTML) or text/plain (verbatim); applied locally, edits always render as rich text") + allowDash(cmd, "arg:1", "flag:content") + return cmd } diff --git a/internal/commands/checkins.go b/internal/commands/checkins.go index ee4a327fa..90d614b12 100644 --- a/internal/commands/checkins.go +++ b/internal/commands/checkins.go @@ -1269,7 +1269,10 @@ func newCheckinsAnswerCreateCmd(project *string) *cobra.Command { } questionID := args[0] - content := strings.Join(args[1:], " ") + content, err := resolveContentArg(cmd, args[1:], 1) + if err != nil { + return err + } app := appctx.FromContext(cmd.Context()) @@ -1359,6 +1362,8 @@ func newCheckinsAnswerCreateCmd(project *string) *cobra.Command { cmd.Flags().StringVar(&groupOn, "date", "", "Date to group answer (ISO 8601, e.g., 2024-01-22; defaults to today)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") + allowDash(cmd, "arg:1+") + return cmd } @@ -1381,7 +1386,10 @@ You can pass either an answer ID or a Basecamp URL: // Extract ID and project from URL if provided answerIDStr, urlProjectID := extractWithProject(args[0]) - content := strings.Join(args[1:], " ") + content, err := resolveContentArg(cmd, args[1:], 1) + if err != nil { + return err + } app := appctx.FromContext(cmd.Context()) @@ -1461,6 +1469,8 @@ You can pass either an answer ID or a Basecamp URL: }, } + allowDash(cmd, "arg:1+") + return cmd } diff --git a/internal/commands/commands_test.go b/internal/commands/commands_test.go index f9a8dc975..85336c486 100644 --- a/internal/commands/commands_test.go +++ b/internal/commands/commands_test.go @@ -119,6 +119,7 @@ func buildRootWithAllCommands() *cobra.Command { root.AddCommand(commands.NewTUICmd()) root.AddCommand(commands.NewProfileCmd()) root.AddCommand(commands.NewBonfireCmd()) + commands.InstallDashGuard(root) root.InitDefaultHelpCmd() return root } diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 05ec36564..b67d61177 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "net/http" "os" "sort" @@ -23,6 +22,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/hostutil" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/richtext" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/urlarg" ) @@ -1049,7 +1049,7 @@ as backslash-n.`, return missingArg(cmd, "<content>") } - content, err := contentArgOrStdin(cmd, args[1:]) + content, err := resolveContentArg(cmd, args[1:], 1) if err != nil { return err } @@ -1114,6 +1114,8 @@ as backslash-n.`, }, } + allowDash(cmd, "arg:1+") + return cmd } @@ -1132,8 +1134,8 @@ Comma-separated IDs add the same comment to multiple items: basecamp comments create 789,012,345 "Looks good!" basecamp comments create https://3.basecamp.com/123/buckets/456/todos/789 "Looks good!" -Content can also be piped from stdin: - printf 'Looks good!' | basecamp comments create 789 +Content can be piped from stdin by passing - as the content argument: + printf 'Looks good!' | basecamp comments create 789 - Content supports Markdown and @mentions (@Name or @First.Last): basecamp comments create 789 "Hey @Jane.Smith, **please review**" @@ -1164,7 +1166,7 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: var content string if len(args) > 1 { var err error - content, err = contentArgOrStdin(cmd, args[1:]) + content, err = resolveContentArg(cmd, args[1:], 1) if err != nil { return err } @@ -1180,21 +1182,19 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: } } - if !edit && strings.TrimSpace(content) == "" { - stdinContent, hasPipedStdin, err := readPipedStdin(cmd) - if err != nil { - return err - } - if hasPipedStdin { - content = stdinContent - } - } - - // Show help when invoked with no content; keep error if editor was opened + // Show help when invoked with no content; keep error if editor was opened. + // A pipe without "-" is deliberately not consumed: teach the explicit + // placeholder instead of silently reading stdin. if strings.TrimSpace(content) == "" { if edit { return output.ErrUsage("Comment content required") } + if stdinarg.IsPiped(cmd.InOrStdin()) { + return output.ErrUsageHint( + "<content> required", + fmt.Sprintf(`To read the piped stdin, pass "-" as the content: %s %s -`, cmd.CommandPath(), recordingArg), + ) + } return missingArg(cmd, "<content>") } @@ -1336,16 +1336,7 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: cmd.Flags().BoolVar(&edit, "edit", false, "Open $EDITOR to compose content") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") - return cmd -} + allowDash(cmd, "arg:1+") -func contentArgOrStdin(cmd *cobra.Command, args []string) (string, error) { - if len(args) == 1 && args[0] == "-" { - b, err := io.ReadAll(cmd.InOrStdin()) - if err != nil { - return "", output.ErrUsage(fmt.Sprintf("failed to read content from stdin: %v", err)) - } - return string(b), nil - } - return strings.Join(args, " "), nil + return cmd } diff --git a/internal/commands/comment_test.go b/internal/commands/comment_test.go index a44f8c4f5..9c40d2203 100644 --- a/internal/commands/comment_test.go +++ b/internal/commands/comment_test.go @@ -84,23 +84,26 @@ func TestCommentsUpdateRejectsEmptyDashContent(t *testing.T) { var outErr *output.Error require.True(t, errors.As(err, &outErr), "expected *output.Error, got %T: %v", err, err) assert.Equal(t, output.CodeUsage, outErr.Code) - assert.Equal(t, "<content> required", outErr.Message) + assert.Equal(t, "stdin for <content> is empty", outErr.Message) assert.Empty(t, transport.capturedBodies) } -func TestCommentsCreateReadsContentFromStdin(t *testing.T) { +// A pipe without "-" is no longer consumed implicitly: the error teaches the +// explicit placeholder instead, and nothing reaches the server. +func TestCommentsCreateBarePipeErrorsWithDashHint(t *testing.T) { transport := &mockCommentWriteTransport{} app, _ := setupCommentsWriteTestApp(t, transport) + app.Flags.JSON = true cmd := NewCommentsCmd() cmd.SetIn(strings.NewReader("hello from stdin")) err := executeCommand(cmd, app, "create", "123") - require.NoError(t, err) - require.Len(t, transport.capturedBodies, 1) - - var body map[string]any - require.NoError(t, json.Unmarshal(transport.capturedBodies[0], &body)) - assert.Equal(t, "<p>hello from stdin</p>", body["content"]) + require.Error(t, err) + var outErr *output.Error + require.True(t, errors.As(err, &outErr)) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Hint, `"-"`) + assert.Empty(t, transport.capturedBodies) } func TestCommentsCreatePrefersPositionalContentOverStdin(t *testing.T) { @@ -140,7 +143,7 @@ func TestCommentsCreateMissingContentReturnsUsageBeforeAccountResolution(t *test assert.NotContains(t, err.Error(), "account") } -func TestReadPipedStdinIgnoresUnreadableStdin(t *testing.T) { +func TestReadStdinContentUnreadableStdinIsUsageError(t *testing.T) { r, w, err := os.Pipe() require.NoError(t, err) require.NoError(t, r.Close()) @@ -148,10 +151,12 @@ func TestReadPipedStdinIgnoresUnreadableStdin(t *testing.T) { cmd := newCommentsCreateCmd() cmd.SetIn(r) - content, hasPipedStdin, err := readPipedStdin(cmd) - require.NoError(t, err) + content, err := readStdinContent(cmd, "<content>") + require.Error(t, err) + var outErr *output.Error + require.True(t, errors.As(err, &outErr)) + assert.Equal(t, output.CodeUsage, outErr.Code) assert.Empty(t, content) - assert.False(t, hasPipedStdin) } func setupCommentsWriteTestApp(t *testing.T, transport http.RoundTripper) (*appctx.App, *bytes.Buffer) { diff --git a/internal/commands/dash_guard_test.go b/internal/commands/dash_guard_test.go new file mode 100644 index 000000000..296d55575 --- /dev/null +++ b/internal/commands/dash_guard_test.go @@ -0,0 +1,151 @@ +package commands + +import ( + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/stdinarg" +) + +// dashProbe records whether a guarded command's original RunE ran, and with +// which args — the passthrough half of the guard's contract. +type dashProbe struct { + ran bool + args []string +} + +func newDashProbeCmd(probe *dashProbe, tokens ...string) *cobra.Command { + cmd := &cobra.Command{ + Use: "probe <name>", + RunE: func(cmd *cobra.Command, args []string) error { + probe.ran = true + probe.args = args + return nil + }, + } + cmd.Flags().String("title", "", "") + cmd.Flags().String("out", "", "") + cmd.Flags().StringArray("attach", nil, "") + if len(tokens) > 0 { + allowDash(cmd, tokens...) + } + InstallDashGuard(cmd) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + return cmd +} + +func TestDashGuardRejectsUnlistedPositionalWhenPiped(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewProjectsCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "create", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "<name>") + assert.Contains(t, outErr.Hint, "--") + assert.Contains(t, outErr.Hint, "--description", "hint should point at where stdin is accepted") +} + +func TestDashGuardRejectsUnlistedFlagWhenPiped(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewTodosCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "update", "1", "--title", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "--title") +} + +func TestDashGuardPassesLiteralDashOnTTY(t *testing.T) { + probe := &dashProbe{} + cmd := newDashProbeCmd(probe) + devNullStdin(t, cmd) + cmd.SetArgs([]string{"-", "--title", "-"}) + + require.NoError(t, cmd.Execute()) + assert.True(t, probe.ran) + assert.Equal(t, []string{"-"}, probe.args) +} + +func TestDashGuardExemptsOutFlag(t *testing.T) { + probe := &dashProbe{} + cmd := newDashProbeCmd(probe, "flag:out") + cmd.SetIn(strings.NewReader("piped")) + cmd.SetArgs([]string{"name", "--out", "-"}) + + require.NoError(t, cmd.Execute()) + assert.True(t, probe.ran) +} + +func TestDashGuardRejectsTwoAllowedDashes(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewChatCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "post", "-", "--content", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "only one input") +} + +// Two allowed dashes can never both be satisfied, even on a TTY. +func TestDashGuardRejectsTwoAllowedDashesOnTTY(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewChatCmd() + InstallDashGuard(cmd) + devNullStdin(t, cmd) + + err := executeCommand(cmd, app, "post", "-", "--content", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "only one input") +} + +func TestDashGuardRejectsUnlistedAttachAlongsideAllowedBody(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewMessagesCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "create", "title", "-", "--attach", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "--attach") +} + +func TestDashGuardSeparatorEscapesLiteralDash(t *testing.T) { + probe := &dashProbe{} + cmd := newDashProbeCmd(probe) + cmd.SetIn(strings.NewReader("piped")) + cmd.SetArgs([]string{"--", "-"}) + + require.NoError(t, cmd.Execute()) + assert.True(t, probe.ran) + assert.Equal(t, []string{"-"}, probe.args) +} + +// The download commands carry the --out exemption so "-" (stdout) never trips +// the stdin guard. +func TestDownloadCommandsExemptOutFlag(t *testing.T) { + for _, cmd := range []*cobra.Command{NewAttachmentsCmd(), NewFilesCmd()} { + download := findSubcommand(cmd, "download") + require.NotNil(t, download, "%s download not found", cmd.Name()) + allow := stdinarg.ParseAllow(download.Annotations[stdinarg.AnnotationAllowDash]) + assert.True(t, allow.Flag("out"), "%s download should exempt --out", cmd.Name()) + } +} diff --git a/internal/commands/files.go b/internal/commands/files.go index 3f543348b..2a16f531c 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -887,13 +887,19 @@ as an upload in the target folder (vault).`, basecamp uploads create ./photo.png --folder 123 --description "Site photo"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } return runUploadFile(cmd, *project, *vaultID, args[0], description, visibleToClients) }, } - cmd.Flags().StringVar(&description, "description", "", "Upload description (Markdown)") + cmd.Flags().StringVar(&description, "description", "", "Upload description (Markdown); use - to read from stdin") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the upload visible to clients (root Docs & Files folder only; a nested folder inherits its folder's visibility). Omit for the server default.") + allowDash(cmd, "flag:description") + return cmd } @@ -915,6 +921,10 @@ attachment and then created as an upload in the target folder.`, basecamp upload ./photo.png --folder 123 --description "Site photo"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } return runUploadFile(cmd, project, vaultID, args[0], description, visibleToClients) }, } @@ -923,9 +933,11 @@ attachment and then created as an upload in the target folder.`, cmd.Flags().StringVar(&project, "in", "", "Project ID (alias for --project)") cmd.Flags().StringVar(&vaultID, "vault", "", "Folder ID (default: root)") cmd.Flags().StringVar(&vaultID, "folder", "", "Folder ID (alias for --vault)") - cmd.Flags().StringVar(&description, "description", "", "Upload description (Markdown)") + cmd.Flags().StringVar(&description, "description", "", "Upload description (Markdown); use - to read from stdin") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the upload visible to clients (root Docs & Files folder only; a nested folder inherits its folder's visibility). Omit for the server default.") + allowDash(cmd, "flag:description") + return cmd } @@ -1214,6 +1226,10 @@ func newDocsCreateCmd(project, vaultID *string) *cobra.Command { cmd := &cobra.Command{ Use: "create <title> [content]", Short: "Create a new document", + Long: `Create a new document in a project's Docs & Files area. + +Use - as the content argument to read the document body from stdin: + basecamp docs create "Title" - --in my-project < body.md`, RunE: func(cmd *cobra.Command, args []string) error { // Show help when invoked with no arguments if len(args) == 0 { @@ -1229,7 +1245,11 @@ func newDocsCreateCmd(project, vaultID *string) *cobra.Command { } content := "" if len(args) > 1 { - content = args[1] + var contentErr error + content, contentErr = resolveContentValue(cmd, args[1], 1, "[content]") + if contentErr != nil { + return contentErr + } } // Resolve subscription flags before project (fail fast on bad input) @@ -1339,6 +1359,8 @@ func newDocsCreateCmd(project, vaultID *string) *cobra.Command { cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the document visible to clients (root Docs & Files folder only; a nested folder inherits its folder's visibility). Omit for the server default.") + allowDash(cmd, "arg:1") + return cmd } @@ -1693,6 +1715,13 @@ You can pass either an upload ID or a Basecamp URL: Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + + // Only an exact "-" reads stdin; --description "" stays the clear idiom. + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -1805,9 +1834,11 @@ You can pass either an upload ID or a Basecamp URL: }, } - cmd.Flags().StringVar(&description, "description", "", "New description (Markdown); omit to carry the current one forward") + cmd.Flags().StringVar(&description, "description", "", "New description (Markdown); omit to carry the current one forward; use - to read from stdin") cmd.Flags().StringVar(&baseName, "base-name", "", "Rename the file (without extension); omit to keep the uploaded file's name") + allowDash(cmd, "flag:description") + return cmd } @@ -1866,6 +1897,13 @@ You can pass either an item ID or a Basecamp URL: Annotations: map[string]string{"agent_notes": "Document updates preserve untouched title/content by fetching current state first because BC3 rebuilds documents from permitted params on PUT; explicit clears via --title \"\"/--content \"\" work because the SDK strips empty strings to absent fields, which the controller then nulls. Upload/vault updates do not clear by omission, so empty-valued flags are rejected CLI-side."}, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // Only an exact "-" reads stdin; --content "" stays the clear idiom. + var contentErr error + content, contentErr = resolveContentValue(cmd, content, -1, "--content") + if contentErr != nil { + return contentErr + } + titleChanged := cmd.Flags().Changed("title") contentChanged := cmd.Flags().Changed("content") titleTrimmed := strings.TrimSpace(title) @@ -2061,9 +2099,11 @@ You can pass either an item ID or a Basecamp URL: } cmd.Flags().StringVarP(&title, "title", "t", "", "New title") - cmd.Flags().StringVarP(&content, "content", "c", "", "New content") + cmd.Flags().StringVarP(&content, "content", "c", "", "New content; use - to read from stdin") cmd.Flags().StringVar(&itemType, "type", "", "Item type (vault, document, upload)") + allowDash(cmd, "flag:content") + return cmd } @@ -2280,6 +2320,9 @@ Use --out - to stream the file to stdout (for piping to other commands).`, cmd.Flags().StringVarP(&outDir, "out", "o", "", "Output directory (default: current directory)") + // --out - means stream to stdout — exempt from the stdin dash guard. + allowDash(cmd, "flag:out") + return cmd } diff --git a/internal/commands/gauges.go b/internal/commands/gauges.go index f90e0aae5..d90fcdfe3 100644 --- a/internal/commands/gauges.go +++ b/internal/commands/gauges.go @@ -208,6 +208,10 @@ func newGaugesCreateCmd(project *string) *cobra.Command { req.Color = color } if description != "" { + description, err = resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } req.Description = description } if notify != "" { @@ -240,10 +244,12 @@ func newGaugesCreateCmd(project *string) *cobra.Command { cmd.Flags().Int32Var(&position, "position", 0, "Position on gauge (0-100, required)") cmd.Flags().StringVar(&color, "color", "", "Needle color: green, yellow, or red") - cmd.Flags().StringVar(&description, "description", "", "Description (rich text HTML)") + cmd.Flags().StringVar(&description, "description", "", "Description (rich text HTML); use - to read from stdin") cmd.Flags().StringVar(¬ify, "notify", "", "Notification mode: everyone, working_on, or custom") cmd.Flags().Int64SliceVar(&subscriptions, "subscriptions", nil, "Person IDs to notify (used with --notify custom)") + allowDash(cmd, "flag:description") + return cmd } @@ -273,6 +279,11 @@ func newGaugesUpdateCmd() *cobra.Command { return output.ErrUsage("No changes specified (use --description)") } + description, err = resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + req := &basecamp.UpdateGaugeNeedleRequest{ Description: basecamp.Ptr(description), } @@ -295,7 +306,9 @@ func newGaugesUpdateCmd() *cobra.Command { }, } - cmd.Flags().StringVar(&description, "description", "", "New description (rich text HTML)") + cmd.Flags().StringVar(&description, "description", "", "New description (rich text HTML); use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 0e81dd215..54f2d1e02 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "os" "strconv" "strings" @@ -87,25 +86,6 @@ func isMachineOutput(cmd *cobra.Command) bool { return false } -func readPipedStdin(cmd *cobra.Command) (string, bool, error) { - stdin := cmd.InOrStdin() - if f, ok := stdin.(*os.File); ok { - fi, _ := f.Stat() - if fi == nil { - return "", false, nil - } - if (fi.Mode() & os.ModeCharDevice) != 0 { - return "", false, nil - } - } - - data, err := io.ReadAll(stdin) - if err != nil { - return "", false, fmt.Errorf("failed to read stdin: %w", err) - } - return string(data), true, nil -} - // DockTool represents a tool in a project's dock. type DockTool struct { Name string `json:"name"` diff --git a/internal/commands/messages.go b/internal/commands/messages.go index 82014f678..a9355fa39 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -431,7 +431,10 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command cmd := &cobra.Command{ Use: "create <title> [body]", Short: "Create a new message", - Long: "Post a new message to a project's message board.", + Long: `Post a new message to a project's message board. + +Use - as the body argument to read the body from stdin: + printf 'Long **Markdown** body' | basecamp messages create "Title" -`, RunE: func(cmd *cobra.Command, args []string) error { // Show help when invoked with no title if len(args) == 0 { @@ -449,10 +452,17 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command return cmd.Help() } - // Validate user input first, before checking account + // Validate user input first, before checking account. The --edit + // exclusion runs before "-" resolution so --edit … - errors + // without consuming stdin. if edit && body != "" { return output.ErrUsage("cannot combine --edit and body argument") } + var err error + body, err = resolveContentValue(cmd, body, 1, "[body]") + if err != nil { + return err + } if edit { fi, err := os.Stdin.Stat() if err != nil || (fi.Mode()&os.ModeCharDevice) == 0 { @@ -591,6 +601,8 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the message visible to clients on the project (omit for the server default; client-authenticated callers always post client-visible)") + allowDash(cmd, "arg:1") + return cmd } @@ -612,6 +624,12 @@ You can pass either a message ID or a Basecamp URL: return noChanges(cmd) } + var err error + body, err = resolveContentValue(cmd, body, -1, "--body") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { @@ -672,7 +690,9 @@ You can pass either a message ID or a Basecamp URL: } cmd.Flags().StringVarP(&title, "title", "t", "", "New title") - cmd.Flags().StringVarP(&body, "body", "b", "", "New body content") + cmd.Flags().StringVarP(&body, "body", "b", "", "New body content; use - to read from stdin") + + allowDash(cmd, "flag:body") return cmd } diff --git a/internal/commands/notes.go b/internal/commands/notes.go index f43d41f35..0405e2769 100644 --- a/internal/commands/notes.go +++ b/internal/commands/notes.go @@ -31,7 +31,7 @@ person, so there is nothing to list and no id to pass. basecamp notes show basecamp notes set "Remember to follow up on the Q3 rollout" basecamp notes set --file notes.md - cat notes.md | basecamp notes set`, + cat notes.md | basecamp notes set -`, Annotations: map[string]string{ "agent_notes": "Account-wide and personal — no --in <project> needed.\n" + "Singleton: no id. 'set' replaces the whole note; it does not append.", @@ -108,16 +108,17 @@ func newNotesSetCmd() *cobra.Command { Short: "Replace your personal note", Long: `Replace your personal note with new content. -Content comes from a positional argument, --file, or piped stdin. Markdown is -converted to HTML, since the note is a rich text field — passing raw text -through would store escaped markup rather than formatting. +Content comes from a positional argument or --file; either accepts - to read +from stdin. Markdown is converted to HTML, since the note is a rich text +field — passing raw text through would store escaped markup rather than +formatting. This replaces the whole note; it does not append. The first write creates the note, so there is no separate "create" step. basecamp notes set "Follow up with Ann on the rollout" basecamp notes set --file notes.md - cat notes.md | basecamp notes set + cat notes.md | basecamp notes set - Attachments are out of scope: this writes the note body only. @@ -163,44 +164,37 @@ a destructive verb deserves its own review, not a rider on a bump.`, }, } - cmd.Flags().StringVarP(&file, "file", "f", "", "Read note content from a file") + cmd.Flags().StringVarP(&file, "file", "f", "", "Read note content from a file; use - to read from stdin") + + allowDash(cmd, "arg:0", "flag:file") return cmd } -// notesContent resolves the note body from exactly one of the three inputs. -// -// Naming two sources is a usage error rather than a silent precedence rule: a +// notesContent resolves the note body from exactly one of two inputs: the +// positional argument (where "-" reads stdin) or --file (where "-" also reads +// stdin). Naming both is a usage error rather than a silent precedence rule: a // caller who passes both an argument and --file has a wrong expectation about // which one wins, and this command overwrites the whole note. // -// All three sources are detected before any of them is chosen. Checking stdin -// only after an argument and --file had been ruled out made -// `generate | basecamp notes set --file fallback.md` overwrite the note from -// the file and discard the generated body without a word — the precise failure -// this function exists to prevent, in the one command that replaces everything. +// A pipe without "-" is deliberately not consumed as an implicit third source. +// Reading it silently made `generate | basecamp notes set --file fallback.md` +// a coin-flip over which body survives; requiring the explicit "-" makes the +// caller name the source in the one command that replaces everything. func notesContent(cmd *cobra.Command, args []string, file string) (string, error) { positional := strings.Join(args, " ") - piped, ok, err := readPipedStdin(cmd) - if err != nil { - return "", err - } - // An empty pipe is not a source. A redirected-but-empty stdin carries no - // body to lose, so it must not turn a valid `--file` call into an error. - hasPipe := ok && strings.TrimSpace(piped) != "" - - named := 0 - for _, present := range []bool{file != "", positional != "", hasPipe} { - if present { - named++ - } - } - if named > 1 { - return "", output.ErrUsage("pass note content as an argument, with --file, or on stdin — not more than one") + if file != "" && positional != "" { + return "", output.ErrUsage("pass note content as an argument or with --file — not both") } switch { + case file == "-": + content, err := readStdinContent(cmd, "--file") + if err != nil { + return "", err + } + return notesRequireContent(content) case file != "": data, err := os.ReadFile(file) if err != nil { @@ -208,14 +202,16 @@ func notesContent(cmd *cobra.Command, args []string, file string) (string, error } return notesRequireContent(string(data)) case positional != "": - return notesRequireContent(positional) - case hasPipe: - return notesRequireContent(piped) + content, err := resolveContentValue(cmd, positional, 0, "[content]") + if err != nil { + return "", err + } + return notesRequireContent(content) } return "", output.ErrUsageHint( "note content is required", - `Pass it as an argument, with --file, or on stdin: basecamp notes set "..."`, + `Pass it as an argument, with --file, or pipe it and pass "-": basecamp notes set -`, ) } diff --git a/internal/commands/notes_test.go b/internal/commands/notes_test.go index e89b12932..4b960158d 100644 --- a/internal/commands/notes_test.go +++ b/internal/commands/notes_test.go @@ -124,13 +124,13 @@ func TestNotesSetReadsFromAFile(t *testing.T) { assert.NotContains(t, body.Note.Content, "# Heading", "raw Markdown must not reach the wire") } -func TestNotesSetReadsPipedStdin(t *testing.T) { +func TestNotesSetReadsDashFromStdin(t *testing.T) { app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) cmd := NewNotesCmd() cmd.SetIn(strings.NewReader("piped note body")) - require.NoError(t, executeRecordingCommand(cmd, app, "set")) + require.NoError(t, executeRecordingCommand(cmd, app, "set", "-")) var body struct { Note struct { @@ -141,6 +141,38 @@ func TestNotesSetReadsPipedStdin(t *testing.T) { assert.Contains(t, body.Note.Content, "piped note body") } +func TestNotesSetReadsDashFileFromStdin(t *testing.T) { + app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) + + cmd := NewNotesCmd() + cmd.SetIn(strings.NewReader("piped note body")) + + require.NoError(t, executeRecordingCommand(cmd, app, "set", "--file", "-")) + + var body struct { + Note struct { + Content string `json:"content"` + } `json:"note"` + } + require.NoError(t, json.Unmarshal([]byte(transport.last(t).Body), &body)) + assert.Contains(t, body.Note.Content, "piped note body") +} + +// A pipe without "-" is not consumed. With no other source named, the error +// teaches the explicit placeholder instead of silently reading the pipe. +func TestNotesSetBarePipeErrorsWithDashHint(t *testing.T) { + app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) + + cmd := NewNotesCmd() + cmd.SetIn(strings.NewReader("piped note body")) + + err := executeRecordingCommand(cmd, app, "set") + + outErr := requireBookmarksUsageError(t, err) + assert.Contains(t, outErr.Hint, "notes set -") + assert.Empty(t, transport.recorded(), "a rejected write must not reach the server") +} + // set replaces the whole note, so the failure modes that would silently erase // it are rejected before the request rather than written through. func TestNotesSetRejectsAmbiguousOrEmptyInput(t *testing.T) { @@ -169,19 +201,20 @@ func TestNotesSetRejectsAmbiguousOrEmptyInput(t *testing.T) { } } -// A piped body must never lose to a flag. `generate | basecamp notes set --file -// fallback.md` used to overwrite the note from the file and throw the generated -// body away, because stdin was only consulted after --file had been ruled out. -func TestNotesSetRejectsPipedContentAlongsideAnotherSource(t *testing.T) { +// A pipe is only ever a source through an explicit "-". When another source is +// named, the unclaimed pipe is ignored — the CLI-wide rule since bare-pipe +// reads were removed — rather than triggering the old ambiguity error. +func TestNotesSetIgnoresUnclaimedPipeWhenSourceIsNamed(t *testing.T) { populated := filepath.Join(t.TempDir(), "note.md") require.NoError(t, os.WriteFile(populated, []byte("from the file"), 0o600)) for _, tc := range []struct { name string args []string + want string }{ - {"pipe and --file together", []string{"set", "--file", populated}}, - {"pipe and an argument together", []string{"set", "inline"}}, + {"pipe and --file together", []string{"set", "--file", populated}, "from the file"}, + {"pipe and an argument together", []string{"set", "inline"}, "inline"}, } { t.Run(tc.name, func(t *testing.T) { app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) @@ -189,14 +222,33 @@ func TestNotesSetRejectsPipedContentAlongsideAnotherSource(t *testing.T) { cmd := NewNotesCmd() cmd.SetIn(strings.NewReader("piped note body")) - err := executeRecordingCommand(cmd, app, tc.args...) + require.NoError(t, executeRecordingCommand(cmd, app, tc.args...)) - requireBookmarksUsageError(t, err) - assert.Empty(t, transport.recorded(), "a rejected write must not reach the server") + var body struct { + Note struct { + Content string `json:"content"` + } `json:"note"` + } + require.NoError(t, json.Unmarshal([]byte(transport.last(t).Body), &body)) + assert.Contains(t, body.Note.Content, tc.want) + assert.NotContains(t, body.Note.Content, "piped note body") }) } } +// Naming both explicit sources is still an ambiguity error. +func TestNotesSetRejectsArgumentAndFileTogether(t *testing.T) { + populated := filepath.Join(t.TempDir(), "note.md") + require.NoError(t, os.WriteFile(populated, []byte("from the file"), 0o600)) + + app, transport, _ := setupPersonalFeedApp(t, notesUpdateRoute()) + + err := executeRecordingCommand(NewNotesCmd(), app, "set", "inline", "--file", populated) + + requireBookmarksUsageError(t, err) + assert.Empty(t, transport.recorded(), "a rejected write must not reach the server") +} + // An empty pipe carries no body to lose, so it must not turn an otherwise valid // --file call into an ambiguity error. func TestNotesSetIgnoresAnEmptyPipeAlongsideAFile(t *testing.T) { diff --git a/internal/commands/projects.go b/internal/commands/projects.go index 0ec4164e6..27e3383cc 100644 --- a/internal/commands/projects.go +++ b/internal/commands/projects.go @@ -274,6 +274,11 @@ func newProjectsCreateCmd() *cobra.Command { return fmt.Errorf("app not initialized") } + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + // Resolve account if not configured (enables interactive prompt) if err := ensureAccount(cmd, app); err != nil { return err @@ -296,7 +301,9 @@ func newProjectsCreateCmd() *cobra.Command { }, } - cmd.Flags().StringVarP(&description, "description", "d", "", "Project description") + cmd.Flags().StringVarP(&description, "description", "d", "", "Project description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } @@ -324,6 +331,11 @@ Examples: return fmt.Errorf("app not initialized") } + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + // Resolve account if not configured (enables interactive prompt) if err := ensureAccount(cmd, app); err != nil { return err @@ -372,7 +384,9 @@ Examples: } cmd.Flags().StringVarP(&name, "name", "n", "", "New name") - cmd.Flags().StringVarP(&description, "description", "d", "", "New description") + cmd.Flags().StringVarP(&description, "description", "d", "", "New description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index 743b495ef..c1c7e667b 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -446,6 +446,11 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { return err } + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + return runScheduleCreate(cmd, app, *project, *scheduleID, entrySummary, startsAt, endsAt, description, allDay, notify, visibleToClients, participants, subscribe, noSubscribe, attachFiles) }, } @@ -456,13 +461,15 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { cmd.Flags().StringVar(&startsAt, "start", "", "Start time (alias)") cmd.Flags().StringVar(&endsAt, "ends-at", "", "End time (ISO 8601)") cmd.Flags().StringVar(&endsAt, "end", "", "End time (alias)") - cmd.Flags().StringVar(&description, "description", "", "Detailed description") + cmd.Flags().StringVar(&description, "description", "", "Detailed description; use - to read from stdin") cmd.Flags().StringVar(&description, "desc", "", "Description (alias)") cmd.Flags().BoolVar(&allDay, "all-day", false, "Mark as all-day event") cmd.Flags().BoolVar(¬ify, "notify", false, "Notify participants") cmd.Flags().StringVar(&participants, "participants", "", "Comma-separated person IDs") cmd.Flags().StringVar(&participants, "people", "", "Person IDs (alias)") cmd.Flags().StringVar(&subscribe, "subscribe", "", "Subscribe specific people (comma-separated names, emails, IDs, or \"me\")") + + allowDash(cmd, "flag:description", "flag:desc") cmd.Flags().BoolVar(&noSubscribe, "no-subscribe", false, "Don't subscribe anyone else (silent, no notifications)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the schedule entry visible to clients on the project (omit for the server default; client-authenticated callers always post client-visible)") @@ -613,6 +620,12 @@ You can pass either an entry ID or a Basecamp URL: Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -764,7 +777,7 @@ You can pass either an entry ID or a Basecamp URL: cmd.Flags().StringVar(&startsAt, "start", "", "Start time (alias)") cmd.Flags().StringVar(&endsAt, "ends-at", "", "End time (ISO 8601)") cmd.Flags().StringVar(&endsAt, "end", "", "End time (alias)") - cmd.Flags().StringVar(&description, "description", "", "Detailed description") + cmd.Flags().StringVar(&description, "description", "", "Detailed description; use - to read from stdin") cmd.Flags().StringVar(&description, "desc", "", "Description (alias)") cmd.Flags().BoolVar(&allDay, "all-day", false, "Mark as all-day event") cmd.Flags().BoolVar(¬ify, "notify", false, "Notify participants") @@ -772,6 +785,8 @@ You can pass either an entry ID or a Basecamp URL: cmd.Flags().StringVar(&participants, "people", "", "Person IDs (alias)") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") + allowDash(cmd, "flag:description", "flag:desc") + return cmd } diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go new file mode 100644 index 000000000..43e499df3 --- /dev/null +++ b/internal/commands/stdin.go @@ -0,0 +1,255 @@ +package commands + +import ( + "fmt" + "io" + "strings" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/stdinarg" +) + +// This file is the single home for "-" (read from stdin) handling. +// +// Tier 1: commands that accept content register where "-" is honored via +// allowDash and resolve it with resolveContentArg / resolveContentValue. +// Tier 2: every other exact "-" — positional or flag value — is caught by the +// dash guard installed over the whole command tree: when stdin is piped, a +// stray "-" is ambiguous (the caller almost certainly meant "read the pipe"), +// so it fails as a usage error instead of landing as literal content. On a +// TTY, a literal "-" stays legal everywhere. + +// allowDash marks where cmd accepts "-" as "read from stdin", merging with any +// tokens already registered. Tokens: "arg:0" (exact positional index), +// "arg:1+" (that index and beyond), "flag:data" (a flag value). +func allowDash(cmd *cobra.Command, tokens ...string) { + if cmd.Annotations == nil { + cmd.Annotations = map[string]string{} + } + merged := strings.Join(tokens, " ") + if existing := cmd.Annotations[stdinarg.AnnotationAllowDash]; existing != "" { + merged = existing + " " + merged + } + cmd.Annotations[stdinarg.AnnotationAllowDash] = merged +} + +// readStdinContent reads content for a "-" placeholder from piped stdin. +// +// Nothing piped (a TTY) is a usage error rather than a silent read: waiting on +// an interactive terminal looks like a hang, so the error teaches the escape +// hatches instead. A piped-but-blank stdin is also refused — blank content is +// never an intentional write, and for update-style commands it would be an +// implicit clear. +// +// Trailing newlines are trimmed: Markdown bodies don't care, but titles and +// boosts (16-rune limit) do, and virtually every pipe ends with one. +func readStdinContent(cmd *cobra.Command, what string) (string, error) { + if !stdinarg.IsPiped(cmd.InOrStdin()) { + return "", output.ErrUsageHint( + fmt.Sprintf(`%s is "-" (read from stdin) but nothing is piped`, what), + stdinEscapeHint(cmd), + ) + } + data, err := io.ReadAll(cmd.InOrStdin()) + if err != nil { + return "", output.ErrUsage(fmt.Sprintf("failed to read %s from stdin: %v", what, err)) + } + content := strings.TrimRight(string(data), "\n") + if strings.TrimSpace(content) == "" { + return "", output.ErrUsage(fmt.Sprintf("stdin for %s is empty", what)) + } + return content, nil +} + +// stdinEscapeHint lists the ways to satisfy a "-" from an interactive +// terminal, mentioning --edit only where the command has it. +func stdinEscapeHint(cmd *cobra.Command) string { + path := cmd.CommandPath() + hint := fmt.Sprintf( + "Pipe the content (printf '...' | %[1]s ... -), use a heredoc (%[1]s ... - <<'EOF'), or run cat | %[1]s ... - and type the content, ending with Ctrl-D", + path) + if cmd.Flags().Lookup("edit") != nil { + hint += "; or compose with --edit" + } + return hint +} + +// resolveContentArg resolves the join-all positional content pattern: exactly +// ["-"] reads stdin, any other args join with spaces. A "-" mixed in with +// other tokens is a usage error — it can't be both stdin and part of the +// joined text, and pre-guard versions silently posted the literal join. +// Every join-all site names its positional <content>, so errors do too. +// +// argsOffset is the index of args[0] in the command's full positional list, +// so a "-" placed at or after the "--" separator stays literal. +func resolveContentArg(cmd *cobra.Command, args []string, argsOffset int) (string, error) { + dashes := 0 + for i, a := range args { + if a == "-" && !afterDashSeparator(cmd, argsOffset+i) { + dashes++ + } + } + switch { + case dashes == 0: + return strings.Join(args, " "), nil + case len(args) == 1: + return readStdinContent(cmd, "<content>") + default: + return "", output.ErrUsage(`"-" (stdin) must be the only <content> argument`) + } +} + +// resolveContentValue resolves a single content value — an exact positional +// (pass its index) or a flag value (pass argIndex -1). Exactly "-" reads +// stdin; a positional "-" at or after the "--" separator stays literal. +func resolveContentValue(cmd *cobra.Command, value string, argIndex int, what string) (string, error) { + if value != "-" || (argIndex >= 0 && afterDashSeparator(cmd, argIndex)) { + return value, nil + } + return readStdinContent(cmd, what) +} + +// afterDashSeparator reports whether the positional at index came after the +// "--" separator, making it literal by definition. +func afterDashSeparator(cmd *cobra.Command, index int) bool { + lenAtDash := cmd.ArgsLenAtDash() + return lenAtDash >= 0 && index >= lenAtDash +} + +// InstallDashGuard wraps every runnable command in the tree with the tier-2 +// dash guard. It wraps RunE rather than hooking PersistentPreRunE because +// cobra runs only the innermost PersistentPreRunE — the agent hook already +// shadows the root's, and any future subtree would silently lose the guard. +// Wrapping RunE also runs after flag parsing and Args validation, with +// ArgsLenAtDash available. +func InstallDashGuard(root *cobra.Command) { + if run := root.RunE; run != nil { + root.RunE = func(cmd *cobra.Command, args []string) error { + if err := guardDashArgs(cmd, args); err != nil { + return err + } + return run(cmd, args) + } + } + for _, sub := range root.Commands() { + InstallDashGuard(sub) + } +} + +// guardDashArgs enforces the tier-2 policy for one invocation: +// +// 1. Collect every exact "-" — positionals before the "--" separator, plus +// changed string-ish flags whose value (or element) is exactly "-". +// 2. More than one allowed "-" can never be satisfied by one stdin, so that +// fails regardless of pipe state. +// 3. A disallowed "-" combined with piped stdin is ambiguous — the caller +// meant the pipe — so it fails with a hint naming the offender. +// 4. Otherwise pass through: a TTY literal "-" stays legal everywhere. +func guardDashArgs(cmd *cobra.Command, args []string) error { + allow := stdinarg.ParseAllow(cmd.Annotations[stdinarg.AnnotationAllowDash]) + + allowed := 0 + var disallowed []string + + for i, a := range args { + if a != "-" || afterDashSeparator(cmd, i) { + continue + } + if allow.Arg(i) { + allowed++ + } else { + disallowed = append(disallowed, positionalName(cmd, i)) + } + } + + cmd.Flags().VisitAll(func(f *pflag.Flag) { + if !f.Changed { + return + } + dashes := 0 + switch f.Value.Type() { + case "string": + if f.Value.String() == "-" { + dashes = 1 + } + case "stringArray", "stringSlice": + if sv, ok := f.Value.(pflag.SliceValue); ok { + for _, v := range sv.GetSlice() { + if v == "-" { + dashes++ + } + } + } + } + if dashes == 0 { + return + } + if allow.Flag(f.Name) { + allowed += dashes + } else { + disallowed = append(disallowed, "--"+f.Name) + } + }) + + if allowed > 1 { + return output.ErrUsage(`only one input can read from stdin ("-") at a time`) + } + if len(disallowed) > 0 && stdinarg.IsPiped(cmd.InOrStdin()) { + msg := fmt.Sprintf(`%s does not read stdin via "-" for %s`, + cmd.CommandPath(), strings.Join(disallowed, ", ")) + hint := `For a literal "-", pass it after the -- separator` + if accepts := describeAllowed(cmd, allow); accepts != "" { + hint += "; this command reads stdin when \"-\" is given as " + accepts + } + return output.ErrUsageHint(msg, hint) + } + return nil +} + +// positionalName names a positional for guard errors, preferring the +// placeholder from the Use string ("<name>") over a bare ordinal. +func positionalName(cmd *cobra.Command, index int) string { + if placeholders := usePlaceholders(cmd); index < len(placeholders) { + return placeholders[index] + } + return fmt.Sprintf("argument %d", index+1) +} + +// describeAllowed renders the allow set for hints: placeholder names for +// positionals, --name for flags. +func describeAllowed(cmd *cobra.Command, allow stdinarg.Allow) string { + var parts []string + placeholders := usePlaceholders(cmd) + for i, p := range placeholders { + if allow.Arg(i) { + parts = append(parts, p) + } + } + for _, token := range strings.Fields(cmd.Annotations[stdinarg.AnnotationAllowDash]) { + // --out is exempted for "-" meaning stdout, not stdin — listing it + // under "reads stdin" would teach the wrong thing. + if name, ok := strings.CutPrefix(token, "flag:"); ok && name != "out" { + parts = append(parts, "--"+name) + } + } + return strings.Join(parts, ", ") +} + +// usePlaceholders extracts the positional placeholders ("<id|url>", +// "[content]") from the command's Use string, in order. +func usePlaceholders(cmd *cobra.Command) []string { + fields := strings.Fields(cmd.Use) + if len(fields) == 0 { + return nil + } + var placeholders []string + for _, f := range fields[1:] { + if strings.HasPrefix(f, "<") || strings.HasPrefix(f, "[") { + placeholders = append(placeholders, f) + } + } + return placeholders +} diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go new file mode 100644 index 000000000..df20696f5 --- /dev/null +++ b/internal/commands/stdin_integration_test.go @@ -0,0 +1,151 @@ +package commands + +// Integration coverage for the "-" (stdin) tier-1 patterns: one command per +// resolver shape, driven through a real Execute with a mock transport. + +import ( + "bytes" + "encoding/json" + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/auth" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/names" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// Pattern 2: exact positional — messages create <title> [body]. +func TestMessagesCreateBodyDashReadsStdin(t *testing.T) { + transport := &mockMessageCreateTransport{} + app, _ := setupMessagesMockApp(t, transport) + + cmd := NewMessagesCmd() + cmd.SetIn(strings.NewReader("Body **from stdin**\n")) + + err := executeMessagesCommand(cmd, app, "create", "Title", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBody, &body)) + assert.Equal(t, "Title", body["subject"]) + content, _ := body["content"].(string) + assert.Contains(t, content, "<strong>from stdin</strong>") +} + +// Pattern 3: content flag — api post --data -. +func TestAPIPostDataDashReadsStdin(t *testing.T) { + transport := &mockCommentWriteTransport{} + app, _ := setupCommentsWriteTestApp(t, transport) + + cmd := NewAPICmd() + cmd.SetIn(strings.NewReader(`{"content":"from stdin"}` + "\n")) + + err := executeCommand(cmd, app, "post", "/buckets/1/todos.json", "--data", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBodies) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBodies[0], &body)) + assert.Equal(t, "from stdin", body["content"]) +} + +// Pattern 1: join-all positionals — todos create <content>. +func TestTodosCreateDashReadsStdin(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + transport := &mockTodoCreateTransport{} + cfg := &config.Config{AccountID: "99999", ProjectID: "123", TodolistID: "456"} + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &todosTestTokenProvider{}, + basecamp.WithTransport(transport), + basecamp.WithMaxRetries(1), + ) + authMgr := auth.NewManager(cfg, nil) + app := &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + Names: names.NewResolver(sdkClient, authMgr, cfg.AccountID), + Output: output.New(output.Options{Format: output.FormatJSON, Writer: &bytes.Buffer{}}), + } + + cmd := NewTodosCmd() + cmd.SetIn(strings.NewReader("Call the vendor back\n")) + + err := executeTodosCommand(cmd, app, "create", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBody, &body)) + assert.Equal(t, "Call the vendor back", body["content"], + "the trailing newline must be trimmed from a piped title") +} + +// Boost content from stdin: the trailing newline is trimmed before the 16-rune +// limit is applied, so a printf'd emoji doesn't burn a rune. +func TestBoostCreateDashReadsStdin(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + transport := &mockBoostTransport{} + app, _ := newBoostTestApp(transport) + + cmd := NewBoostsCmd() + cmd.SetIn(strings.NewReader("🎉\n")) + + err := executeBoostCommand(cmd, app, "create", "456", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBody) + + var body map[string]any + require.NoError(t, json.Unmarshal(transport.capturedBody, &body)) + assert.Equal(t, "🎉", body["content"]) +} + +func TestBoostCreateDashOverLimitStillRejected(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + transport := &mockBoostTransport{} + app, _ := newBoostTestApp(transport) + + cmd := NewBoostsCmd() + cmd.SetIn(strings.NewReader("seventeen chars!!\n")) + + err := executeBoostCommand(cmd, app, "create", "456", "-") + require.Error(t, err) + var e *output.Error + require.True(t, errors.As(err, &e)) + assert.Contains(t, e.Message, "Boost content too long") +} + +// Pattern 3 on an update: todos update --description -. +func TestTodosUpdateDescriptionDashReadsStdin(t *testing.T) { + transport := &mockCommentWriteTransport{} + app, _ := setupCommentsWriteTestApp(t, transport) + + cmd := NewTodosCmd() + cmd.SetIn(strings.NewReader("New **details**\n")) + + err := executeCommand(cmd, app, "update", "789", "--description", "-") + require.NoError(t, err) + require.NotEmpty(t, transport.capturedBodies) + + found := false + for _, captured := range transport.capturedBodies { + var body map[string]any + if json.Unmarshal(captured, &body) == nil { + if desc, _ := body["description"].(string); strings.Contains(desc, "<strong>details</strong>") { + found = true + } + } + } + assert.True(t, found, "the piped description should reach the wire as HTML") +} diff --git a/internal/commands/stdin_test.go b/internal/commands/stdin_test.go new file mode 100644 index 000000000..891adedd7 --- /dev/null +++ b/internal/commands/stdin_test.go @@ -0,0 +1,160 @@ +package commands + +import ( + "errors" + "os" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// devNullStdin wires a command's stdin to /dev/null, a character device — the +// established TTY stand-in (see edit_test.go). +func devNullStdin(t *testing.T, cmd *cobra.Command) { + t.Helper() + devNull, err := os.Open(os.DevNull) + require.NoError(t, err) + t.Cleanup(func() { devNull.Close() }) + cmd.SetIn(devNull) +} + +func requireUsageErr(t *testing.T, err error) *output.Error { + t.Helper() + require.Error(t, err) + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "expected *output.Error, got %T: %v", err, err) + assert.Equal(t, output.CodeUsage, outErr.Code) + return outErr +} + +func TestReadStdinContentTrimsTrailingNewlines(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("🎉\n")) + + content, err := readStdinContent(cmd, "<content>") + require.NoError(t, err) + assert.Equal(t, "🎉", content) +} + +func TestReadStdinContentTTYIsUsageErrorWithEscapeHints(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + devNullStdin(t, cmd) + + _, err := readStdinContent(cmd, "<content>") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "nothing is piped") + assert.Contains(t, outErr.Hint, "heredoc") + assert.NotContains(t, outErr.Hint, "--edit", "no --edit flag on this command") +} + +func TestReadStdinContentTTYHintMentionsEditWhenAvailable(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.Flags().Bool("edit", false, "") + devNullStdin(t, cmd) + + _, err := readStdinContent(cmd, "<content>") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Hint, "--edit") +} + +func TestReadStdinContentBlankPipeIsUsageError(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader(" \n\n")) + + _, err := readStdinContent(cmd, "<content>") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "empty") +} + +func TestResolveContentArgJoinsLiteralArgs(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + content, err := resolveContentArg(cmd, []string{"hello", "world"}, 1) + require.NoError(t, err) + assert.Equal(t, "hello world", content) +} + +func TestResolveContentArgLoneDashReadsStdin(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("from stdin\n")) + + content, err := resolveContentArg(cmd, []string{"-"}, 1) + require.NoError(t, err) + assert.Equal(t, "from stdin", content) +} + +func TestResolveContentArgDashAmongOthersIsUsageError(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("from stdin")) + + _, err := resolveContentArg(cmd, []string{"-", "extra"}, 1) + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "only") +} + +// A "-" placed after the -- separator is literal: parsed through a real +// Execute so ArgsLenAtDash is set. +func TestResolveContentArgDashAfterSeparatorIsLiteral(t *testing.T) { + var content string + cmd := &cobra.Command{ + Use: "x <id> <content>", + RunE: func(cmd *cobra.Command, args []string) error { + var err error + content, err = resolveContentArg(cmd, args[1:], 1) + return err + }, + } + cmd.SetIn(strings.NewReader("must not be read")) + cmd.SetArgs([]string{"123", "--", "-"}) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + + require.NoError(t, cmd.Execute()) + assert.Equal(t, "-", content) +} + +func TestResolveContentValueFlagDashReadsStdin(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("flag body\n")) + + content, err := resolveContentValue(cmd, "-", -1, "--data") + require.NoError(t, err) + assert.Equal(t, "flag body", content) +} + +func TestResolveContentValueLiteralPassesThrough(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + content, err := resolveContentValue(cmd, "plain", -1, "--data") + require.NoError(t, err) + assert.Equal(t, "plain", content) +} + +func TestResolveContentValuePositionalDashAfterSeparatorIsLiteral(t *testing.T) { + var body string + cmd := &cobra.Command{ + Use: "x <title> [body]", + RunE: func(cmd *cobra.Command, args []string) error { + var err error + body, err = resolveContentValue(cmd, args[1], 1, "[body]") + return err + }, + } + cmd.SetIn(strings.NewReader("must not be read")) + cmd.SetArgs([]string{"--", "title", "-"}) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + + require.NoError(t, cmd.Execute()) + assert.Equal(t, "-", body) +} + +func TestAllowDashMergesTokens(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + allowDash(cmd, "arg:0") + allowDash(cmd, "flag:description") + assert.Equal(t, "arg:0 flag:description", cmd.Annotations["allow_dash"]) +} diff --git a/internal/commands/templates.go b/internal/commands/templates.go index 654ba376b..3717009bd 100644 --- a/internal/commands/templates.go +++ b/internal/commands/templates.go @@ -201,6 +201,11 @@ func newTemplatesCreateCmd() *cobra.Command { app := appctx.FromContext(cmd.Context()) + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -234,9 +239,11 @@ func newTemplatesCreateCmd() *cobra.Command { } cmd.Flags().StringVar(&name, "name", "", "Template name") - cmd.Flags().StringVar(&description, "description", "", "Template description") + cmd.Flags().StringVar(&description, "description", "", "Template description; use - to read from stdin") cmd.Flags().StringVar(&description, "desc", "", "Template description (alias)") + allowDash(cmd, "flag:description", "flag:desc") + return cmd } @@ -265,6 +272,11 @@ func newTemplatesUpdateCmd() *cobra.Command { return noChanges(cmd) } + description, err = resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + // SDK requires name for update, fetch current if not provided updateName := name if updateName == "" { @@ -299,9 +311,11 @@ func newTemplatesUpdateCmd() *cobra.Command { } cmd.Flags().StringVar(&name, "name", "", "New name") - cmd.Flags().StringVar(&description, "description", "", "New description") + cmd.Flags().StringVar(&description, "description", "", "New description; use - to read from stdin") cmd.Flags().StringVar(&description, "desc", "", "New description (alias)") + allowDash(cmd, "flag:description", "flag:desc") + return cmd } @@ -375,6 +389,11 @@ which can be polled via 'templates construction' until the status is "completed" return output.ErrUsage("--name is required (project name)") } + projectDesc, err := resolveContentValue(cmd, projectDesc, -1, "--description") + if err != nil { + return err + } + construction, err := app.Account().Templates().CreateProject(cmd.Context(), templateID, projectName, projectDesc) if err != nil { return convertSDKError(err) @@ -394,10 +413,12 @@ which can be polled via 'templates construction' until the status is "completed" } cmd.Flags().StringVar(&projectName, "name", "", "Project name (required)") - cmd.Flags().StringVar(&projectDesc, "description", "", "Project description") + cmd.Flags().StringVar(&projectDesc, "description", "", "Project description; use - to read from stdin") cmd.Flags().StringVar(&projectDesc, "desc", "", "Project description (alias)") _ = cmd.MarkFlagRequired("name") + allowDash(cmd, "flag:description", "flag:desc") + return cmd } diff --git a/internal/commands/todolists.go b/internal/commands/todolists.go index 9405e69a7..476cd4283 100644 --- a/internal/commands/todolists.go +++ b/internal/commands/todolists.go @@ -288,6 +288,11 @@ func newTodolistsCreateCmd(project, todosetID *string) *cobra.Command { return fmt.Errorf("app not initialized") } + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -366,9 +371,11 @@ func newTodolistsCreateCmd(project, todosetID *string) *cobra.Command { } cmd.Flags().StringVarP(todosetID, "todoset", "t", "", "Todoset ID (for projects with multiple todosets)") - cmd.Flags().StringVarP(&description, "description", "d", "", "Todolist description") + cmd.Flags().StringVarP(&description, "description", "d", "", "Todolist description; use - to read from stdin") cmd.Flags().BoolVar(&visibleToClients, "visible-to-clients", false, "Make the todolist visible to clients on the project (omit for the server default; client-authenticated callers always post client-visible)") + allowDash(cmd, "flag:description") + return cmd } @@ -395,6 +402,11 @@ You can pass either a todolist ID or a Basecamp URL: return fmt.Errorf("app not initialized") } + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -457,7 +469,9 @@ You can pass either a todolist ID or a Basecamp URL: } cmd.Flags().StringVarP(&name, "name", "n", "", "New name") - cmd.Flags().StringVarP(&description, "description", "d", "", "New description") + cmd.Flags().StringVarP(&description, "description", "d", "", "New description; use - to read from stdin") + + allowDash(cmd, "flag:description") return cmd } diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 6850ed403..cd8db6bef 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1243,7 +1243,10 @@ project's to-do set instead, outside any list: basecamp todos create "Call the vendor back" --loose --in <project> ---loose needs no list, so it neither prompts for one nor accepts --list.`, +--loose needs no list, so it neither prompts for one nor accepts --list. + +Use - as the content argument to read the todo title from stdin: + printf 'Call the vendor back' | basecamp todos create - --in <project>`, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) if app == nil { @@ -1254,11 +1257,19 @@ project's to-do set instead, outside any list: if len(args) == 0 { return missingArg(cmd, "<content>") } - content := strings.Join(args, " ") + content, err := resolveContentArg(cmd, args, 0) + if err != nil { + return err + } if strings.TrimSpace(content) == "" { return cmd.Help() } + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -1437,13 +1448,15 @@ project's to-do set instead, outside any list: cmd.Flags().StringVar(&assignee, "assignee", "", "Assignee ID") cmd.Flags().StringVar(&assignee, "to", "", "Assignee ID (alias for --assignee)") cmd.Flags().StringVarP(&due, "due", "d", "", "Due date (YYYY-MM-DD)") - cmd.Flags().StringVar(&description, "description", "", "Extended description (Markdown)") + cmd.Flags().StringVar(&description, "description", "", "Extended description (Markdown); use - to read from stdin") cmd.Flags().StringArrayVar(&attachFiles, "attach", nil, "Attach file (repeatable)") cmd.Flags().StringVar(¬ifyOnCompletion, "notify-on-completion", "", "People to notify when done (names or IDs, comma-separated)") // Not --todoset: that flag already means "which to-do set", and this one // means "no list at all". cmd.Flags().BoolVar(&loose, "loose", false, "Create on the to-do set, outside any list") + allowDash(cmd, "arg:0+", "flag:description") + // Register tab completion for flags completer := completion.NewCompleter(nil) _ = cmd.RegisterFlagCompletionFunc("project", completer.ProjectNameCompletion()) @@ -1537,6 +1550,12 @@ Set or clear the people notified when the todo is completed: return noChanges(cmd) } + // Only an exact "-" reads stdin; --description "" stays the clear idiom. + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if app == nil { return fmt.Errorf("app not initialized") @@ -1664,7 +1683,7 @@ Set or clear the people notified when the todo is completed: } cmd.Flags().StringVarP(&title, "title", "t", "", "Todo title (plain text)") - cmd.Flags().StringVar(&description, "description", "", "Extended description (Markdown)") + cmd.Flags().StringVar(&description, "description", "", "Extended description (Markdown); use - to read from stdin") cmd.Flags().StringVar(&assignee, "assignee", "", "Assignees (names or IDs, comma-separated)") cmd.Flags().StringVar(&assignee, "to", "", "Assignees (alias for --assignee)") cmd.Flags().StringVarP(&due, "due", "d", "", "Due date (natural language or YYYY-MM-DD)") @@ -1682,6 +1701,8 @@ Set or clear the people notified when the todo is completed: _ = cmd.RegisterFlagCompletionFunc("to", completer.PeopleNameCompletion()) _ = cmd.RegisterFlagCompletionFunc("notify-on-completion", completer.PeopleNameCompletion()) + allowDash(cmd, "flag:description") + return cmd } @@ -1873,6 +1894,12 @@ Examples: basecamp todos sweep --in <project> --assignee me --comment "Following up"`, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + + comment, err := resolveContentValue(cmd, comment, -1, "--comment") + if err != nil { + return err + } + if err := ensureAccount(cmd, app); err != nil { return err } @@ -2020,11 +2047,13 @@ Examples: cmd.Flags().StringVarP(&todoset, "todoset", "t", "", "Todoset ID (for projects with multiple todosets)") cmd.Flags().StringVar(&assignee, "assignee", "", "Filter by assignee") cmd.Flags().BoolVar(&overdueOnly, "overdue", false, "Filter overdue todos") - cmd.Flags().StringVarP(&comment, "comment", "c", "", "Comment to add to matching todos") + cmd.Flags().StringVarP(&comment, "comment", "c", "", "Comment to add to matching todos; use - to read from stdin") cmd.Flags().BoolVar(&complete, "complete", false, "Mark matching todos as complete") cmd.Flags().BoolVar(&complete, "done", false, "Mark matching todos as complete (alias)") cmd.Flags().BoolVarP(&dryRun, "dry-run", "n", false, "Preview without making changes") + allowDash(cmd, "flag:comment") + // Register tab completion for flags completer := completion.NewCompleter(nil) _ = cmd.RegisterFlagCompletionFunc("project", completer.ProjectNameCompletion()) diff --git a/internal/stdinarg/stdinarg.go b/internal/stdinarg/stdinarg.go new file mode 100644 index 000000000..d22d5e4ed --- /dev/null +++ b/internal/stdinarg/stdinarg.go @@ -0,0 +1,93 @@ +// Package stdinarg carries the shared vocabulary for "-" (read from stdin) +// argument handling: the cobra annotation that marks where a command accepts +// "-", and pipe detection for deciding whether a stray "-" is ambiguous. +// +// It is a leaf package because both internal/commands (which resolves "-" and +// installs the guard) and internal/cli (which surfaces the annotation in agent +// help) need the same annotation key, and cli already depends on commands. +package stdinarg + +import ( + "io" + "os" + "strconv" + "strings" +) + +// AnnotationAllowDash is the cmd.Annotations key marking where a command +// accepts "-" as "read from stdin". The value is a space-separated list of +// tokens: "arg:0" (exact positional index), "arg:1+" (that index and beyond), +// "flag:data" (the --data flag). Everything not listed is guarded: a literal +// "-" there combined with piped stdin is rejected as ambiguous. +const AnnotationAllowDash = "allow_dash" + +// Allow is the parsed form of an AnnotationAllowDash value. +type Allow struct { + args map[int]bool + argsFrom int // "arg:N+" allows every index >= argsFrom; -1 when absent + flags map[string]bool +} + +// ParseAllow parses a space-separated token list ("arg:0 arg:1+ flag:data") +// into an Allow. Unrecognized tokens are ignored rather than failing: the +// annotation is authored in-repo and covered by tests, so a typo shows up as +// a guarded (rejected) input, not a silent bypass. +func ParseAllow(s string) Allow { + allow := Allow{argsFrom: -1} + for _, token := range strings.Fields(s) { + switch { + case strings.HasPrefix(token, "arg:"): + spec := strings.TrimPrefix(token, "arg:") + open := strings.HasSuffix(spec, "+") + if n, err := strconv.Atoi(strings.TrimSuffix(spec, "+")); err == nil { + if open { + if allow.argsFrom == -1 || n < allow.argsFrom { + allow.argsFrom = n + } + } else { + if allow.args == nil { + allow.args = map[int]bool{} + } + allow.args[n] = true + } + } + case strings.HasPrefix(token, "flag:"): + if allow.flags == nil { + allow.flags = map[string]bool{} + } + allow.flags[strings.TrimPrefix(token, "flag:")] = true + } + } + return allow +} + +// Arg reports whether "-" is allowed at positional index i. +func (a Allow) Arg(i int) bool { + return a.args[i] || (a.argsFrom != -1 && i >= a.argsFrom) +} + +// Flag reports whether "-" is allowed as the named flag's value. +func (a Allow) Flag(name string) bool { + return a.flags[name] +} + +// Empty reports whether the Allow permits "-" nowhere. +func (a Allow) Empty() bool { + return len(a.args) == 0 && a.argsFrom == -1 && len(a.flags) == 0 +} + +// IsPiped reports whether the reader carries piped (redirected) input rather +// than an interactive terminal. A non-*os.File reader — the cmd.SetIn test +// seam — always counts as piped. For a real file, a character device means a +// terminal; anything else (pipe, regular file redirect) is piped input. +func IsPiped(r io.Reader) bool { + f, ok := r.(*os.File) + if !ok { + return true + } + fi, err := f.Stat() + if err != nil { + return false + } + return fi.Mode()&os.ModeCharDevice == 0 +} diff --git a/internal/stdinarg/stdinarg_test.go b/internal/stdinarg/stdinarg_test.go new file mode 100644 index 000000000..34ccbb135 --- /dev/null +++ b/internal/stdinarg/stdinarg_test.go @@ -0,0 +1,67 @@ +package stdinarg + +import ( + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseAllowExactArg(t *testing.T) { + allow := ParseAllow("arg:0") + assert.True(t, allow.Arg(0)) + assert.False(t, allow.Arg(1)) + assert.False(t, allow.Flag("data")) + assert.False(t, allow.Empty()) +} + +func TestParseAllowOpenEndedArg(t *testing.T) { + allow := ParseAllow("arg:1+") + assert.False(t, allow.Arg(0)) + assert.True(t, allow.Arg(1)) + assert.True(t, allow.Arg(5)) +} + +func TestParseAllowFlags(t *testing.T) { + allow := ParseAllow("flag:data flag:out") + assert.True(t, allow.Flag("data")) + assert.True(t, allow.Flag("out")) + assert.False(t, allow.Flag("body")) + assert.False(t, allow.Arg(0)) +} + +func TestParseAllowMixed(t *testing.T) { + allow := ParseAllow("arg:0 arg:2+ flag:description") + assert.True(t, allow.Arg(0)) + assert.False(t, allow.Arg(1)) + assert.True(t, allow.Arg(2)) + assert.True(t, allow.Arg(3)) + assert.True(t, allow.Flag("description")) +} + +func TestParseAllowEmptyAndGarbage(t *testing.T) { + assert.True(t, ParseAllow("").Empty()) + assert.True(t, ParseAllow("arg:x bogus flag").Empty()) +} + +func TestIsPipedNonFileReader(t *testing.T) { + assert.True(t, IsPiped(strings.NewReader("piped"))) +} + +func TestIsPipedCharDevice(t *testing.T) { + devNull, err := os.Open(os.DevNull) + require.NoError(t, err) + defer devNull.Close() + + assert.False(t, IsPiped(devNull)) +} + +func TestIsPipedRegularFile(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "stdin") + require.NoError(t, err) + defer f.Close() + + assert.True(t, IsPiped(f)) +} diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 35138fed9..a7a7f4c00 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -108,6 +108,23 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, ```bash printf '%s\n' '海报 mockup 方向稿：' '' '<bc-attachment ...>' | basecamp comments create <recording_id> - --in <project> --json ``` + `-` means "read from stdin" on every content input: content-kind positionals + (`comments create/update`, `messages create [body]`, `cards create [body]`, + `todos create`, `docs create [content]`, `chat post/update`, `boost create`, + `checkins answer create/update`, `notes set`) and content flags (`--data` on + `api post/put`, `--body`, `--content`, `--description`, `--comment` on + `todos sweep`, `--file` on `notes set`). Each command's `--agent` help lists + its stdin inputs. Rules: + - A pipe is **never consumed implicitly** — without `-` it is ignored (or, where + content is required and missing, the error teaches `-`). + - Only one input can read stdin per invocation. + - A literal `-` anywhere else (a title, a name, a path) **errors when stdin is + piped**; escape it after the `--` separator: `basecamp projects create -- -`. + - `-` with nothing piped (interactive TTY) errors immediately instead of + hanging; use a pipe, a heredoc (`basecamp comments create <id> - <<'EOF'`), + or `--edit` where offered. + - Trailing newlines are trimmed from stdin content, so `printf 'x\n' | ... -` + posts `x` (this keeps `boost create -` inside its 16-rune limit). 6. **Project scope is mandatory for most commands** — via `--in <project>` or `.basecamp/config.json`. Cross-project exceptions: `basecamp reports assigned` for assigned work, `basecamp assignments` for structured assignment views, `basecamp reports overdue` for overdue todos, `basecamp reports schedule` for upcoming schedule across all projects, `basecamp recordings <type>` for browsing by type, `basecamp notifications` for notifications, `basecamp gauges list` for account-wide gauges, and the seven list commands covered in item 7. 7. **Account-wide listing.** `basecamp todos list --all-projects --json` lists across every project; the same flag does the same on `cards list`, `messages list`, `comments list`, `files list`, `forwards list`, and `checkins answers`. It overrides a configured project, and with no project in scope those commands already list account-wide rather than prompting. Flags that name something inside a single project are rejected there rather than silently ignored. Account-wide listings return **the first 100 items by default** — account-wide "all" is the whole account, not one project's worth. Use `--limit N` to raise the cap (it walks pages until N are collected) or `--all` for everything. `--page N` fetches exactly one page, but only on the paginated listings. @@ -1086,8 +1103,9 @@ at 250 server-side. `notes` is a single private scratchpad — one per person, no id, nothing to list. Before your first write it renders empty rather than 404ing. `set` **replaces** -the whole note (it does not append) and takes content from an argument, -`--file`, or piped stdin; Markdown is converted to HTML. +the whole note (it does not append) and takes content from an argument or +`--file` — either accepts `-` to read stdin (`cat notes.md | basecamp notes set -`); +a pipe without `-` is not consumed. Markdown is converted to HTML. ### Calendars From cfb99594f7218e2083d069a58e466768cb1bac8e Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Tue, 18 Aug 2026 22:17:22 -0700 Subject: [PATCH 02/19] Address review: guard at Args time, alias dedupe, honest hints, CRLF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes to the dash guard's contract from review: - Run the guard at Args-validation time instead of wrapping RunE. Cobra runs ValidateArgs after flag parsing (Changed and ArgsLenAtDash are available) but before the persistent pre-run chain, PreRunE, and required-flag validation — so the stray-dash error fires before any lifecycle side effect (config hardening, the update check) and before a competing usage error can shadow it. The root command stays unwrapped: its nil Args is load-bearing — cobra's Find() rejects unknown subcommands (legacyArgs) only while Args == nil, and wrapping it turned "basecamp unknowncmd" into a quickstart run (caught by core.bats). Nothing is lost: root positionals are subcommand names, and a bare "basecamp -" runs quickstart, which posts no content. - Dedupe alias flags by their shared pflag.Value. --description and --desc wrap one backing variable, and pflag hands both the same Value instance; counting each spelling separately made "--description old --desc -" a false "two stdin inputs" error. One logical value now counts once, in both flag orders. - Stop advertising -- as the escape for flag values — it only escapes positionals. Positional offenders keep the -- hint; flag offenders get the honest remedy (run without piped stdin, append </dev/tty). SKILL.md updated to match. - Trim trailing CRLF, not just LF, from stdin content: a Windows-style pipe left \r behind, counting a phantom rune against boost's 16-rune limit. Also replace the weak final e2e case (which contradicted the file's no-network header by dialing localhost) with a deterministic local success: config set ... -- - stores a literal "-", read back via config show. --- e2e/stdin_dash.bats | 22 +++++---- internal/commands/dash_guard_test.go | 71 ++++++++++++++++++++++++++++ internal/commands/stdin.go | 68 +++++++++++++++++++------- internal/commands/stdin_test.go | 11 +++++ skills/basecamp/SKILL.md | 4 +- 5 files changed, 148 insertions(+), 28 deletions(-) diff --git a/e2e/stdin_dash.bats b/e2e/stdin_dash.bats index b56f4c7d3..ac8cc4d50 100644 --- a/e2e/stdin_dash.bats +++ b/e2e/stdin_dash.bats @@ -3,8 +3,9 @@ # # Tier 1: content inputs accept "-" to read piped stdin. Tier 2: everywhere # else, a literal "-" combined with piped stdin is a usage error instead of -# silently becoming literal content. All cases here fail before any HTTP, so -# no cassette or server is needed. +# silently becoming literal content. Every case resolves locally — usage +# errors before any request, or a config write — so no cassette or server +# is needed. load test_helper @@ -56,14 +57,17 @@ load test_helper assert_json_value '.hint | contains("after the -- separator")' 'true' } -@test "projects create -- - with piped stdin passes the guard" { - # The -- separator makes the "-" literal; the command proceeds past the - # guard and fails on the (unreachable) API instead of on usage. - export BASECAMP_BASE_URL="http://127.0.0.1:1" +@test "a -- - after the separator passes the guard and lands literally" { create_credentials create_global_config '{"account_id": 99999}' - run bash -c "printf 'x' | basecamp projects create --json -- -" - assert_failure - [[ "$output" != *'does not read stdin'* ]] + # config set writes locally — a deterministic success proving the escaped + # "-" passed the guard and was stored as a literal value. + run bash -c "cd '$TEST_PROJECT' && printf 'x' | basecamp config set project_id --json -- -" + assert_success + assert_json_value '.data.value' '-' + + run bash -c "cd '$TEST_PROJECT' && basecamp config show --json < /dev/null" + assert_success + assert_json_value '.data.project_id.value' '-' } diff --git a/internal/commands/dash_guard_test.go b/internal/commands/dash_guard_test.go index 296d55575..9634afed9 100644 --- a/internal/commands/dash_guard_test.go +++ b/internal/commands/dash_guard_test.go @@ -65,6 +65,77 @@ func TestDashGuardRejectsUnlistedFlagWhenPiped(t *testing.T) { err := executeCommand(cmd, app, "update", "1", "--title", "-") outErr := requireUsageErr(t, err) assert.Contains(t, outErr.Message, "--title") + // -- doesn't escape flag values, so the hint must not claim it does. + assert.NotContains(t, outErr.Hint, "-- separator") + assert.Contains(t, outErr.Hint, "/dev/tty") +} + +// The guard runs at Args-validation time: before the command's own Args +// check, PreRunE, and required-flag validation, so the stray-dash error is +// what the caller sees instead of a competing usage error — and no pre-run +// side effect happens first. +func TestDashGuardFiresBeforeArgsPreRunAndRequiredFlags(t *testing.T) { + preRunRan := false + cmd := &cobra.Command{ + Use: "probe <name> <other>", + Args: cobra.ExactArgs(2), + PreRunE: func(cmd *cobra.Command, args []string) error { + preRunRan = true + return nil + }, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + cmd.Flags().String("title", "", "") + require.NoError(t, cmd.MarkFlagRequired("title")) + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + cmd.SetArgs([]string{"-"}) // one arg: ExactArgs(2) and the missing --title would both error later + + err := cmd.Execute() + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "<name>") + assert.False(t, preRunRan, "guard must fire before PreRunE") +} + +// Alias flags share one backing value; a value set through both spellings is +// one logical input, not two. +func TestDashGuardAliasFlagsCountOnce(t *testing.T) { + newAliasCmd := func(probe *dashProbe) *cobra.Command { + var description string + cmd := &cobra.Command{ + Use: "probe", + RunE: func(cmd *cobra.Command, args []string) error { + probe.ran = true + probe.args = []string{description} + return nil + }, + } + cmd.Flags().StringVar(&description, "description", "", "") + cmd.Flags().StringVar(&description, "desc", "", "") + allowDash(cmd, "flag:description", "flag:desc") + InstallDashGuard(cmd) + cmd.SetOut(&strings.Builder{}) + cmd.SetErr(&strings.Builder{}) + return cmd + } + + // Dash last: the merged value is "-", one allowed stdin input. + probe := &dashProbe{} + cmd := newAliasCmd(probe) + cmd.SetIn(strings.NewReader("piped")) + cmd.SetArgs([]string{"--description", "old", "--desc", "-"}) + require.NoError(t, cmd.Execute()) + assert.Equal(t, []string{"-"}, probe.args) + + // Dash first: the literal value wins, no dash in play at all. + probe = &dashProbe{} + cmd = newAliasCmd(probe) + cmd.SetIn(strings.NewReader("piped")) + cmd.SetArgs([]string{"--desc", "-", "--description", "old"}) + require.NoError(t, cmd.Execute()) + assert.Equal(t, []string{"old"}, probe.args) } func TestDashGuardPassesLiteralDashOnTTY(t *testing.T) { diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index 43e499df3..df87d8353 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -44,8 +44,9 @@ func allowDash(cmd *cobra.Command, tokens ...string) { // never an intentional write, and for update-style commands it would be an // implicit clear. // -// Trailing newlines are trimmed: Markdown bodies don't care, but titles and -// boosts (16-rune limit) do, and virtually every pipe ends with one. +// Trailing newlines — LF and CRLF alike — are trimmed: Markdown bodies don't +// care, but titles and boosts (16-rune limit) do, and virtually every pipe +// ends with one. Interior line breaks are untouched. func readStdinContent(cmd *cobra.Command, what string) (string, error) { if !stdinarg.IsPiped(cmd.InOrStdin()) { return "", output.ErrUsageHint( @@ -57,7 +58,7 @@ func readStdinContent(cmd *cobra.Command, what string) (string, error) { if err != nil { return "", output.ErrUsage(fmt.Sprintf("failed to read %s from stdin: %v", what, err)) } - content := strings.TrimRight(string(data), "\n") + content := strings.TrimRight(string(data), "\r\n") if strings.TrimSpace(content) == "" { return "", output.ErrUsage(fmt.Sprintf("stdin for %s is empty", what)) } @@ -120,18 +121,33 @@ func afterDashSeparator(cmd *cobra.Command, index int) bool { } // InstallDashGuard wraps every runnable command in the tree with the tier-2 -// dash guard. It wraps RunE rather than hooking PersistentPreRunE because -// cobra runs only the innermost PersistentPreRunE — the agent hook already -// shadows the root's, and any future subtree would silently lose the guard. -// Wrapping RunE also runs after flag parsing and Args validation, with -// ArgsLenAtDash available. +// dash guard. It wraps the Args validator: cobra runs ValidateArgs after flag +// parsing (so Changed and ArgsLenAtDash are available) but before the +// persistent pre-run chain, PreRunE, and required-flag validation — so a +// stray "-" is rejected before any lifecycle side effect (config hardening, +// the update check) and before a competing usage error can shadow it. It is +// not a PersistentPreRunE hook because cobra runs only the innermost one — +// the agent hook already shadows the root's, and any future subtree would +// silently lose the guard. A nil Args means ArbitraryArgs (always nil), so +// wrapping it is behavior-preserving. func InstallDashGuard(root *cobra.Command) { - if run := root.RunE; run != nil { - root.RunE = func(cmd *cobra.Command, args []string) error { + // The root's nil Args is load-bearing: cobra's Find() rejects unknown + // subcommands (legacyArgs) only while Args == nil, so wrapping it would + // turn "basecamp unknowncmd" into a quickstart run. Leaving the root + // unguarded loses nothing: its positionals are subcommand names, and a + // bare "basecamp -" just runs quickstart, which posts no content for a + // literal "-" to corrupt. + skipRoot := root.Args == nil && !root.HasParent() && root.HasSubCommands() + if root.Runnable() && !skipRoot { + existing := root.Args + root.Args = func(cmd *cobra.Command, args []string) error { if err := guardDashArgs(cmd, args); err != nil { return err } - return run(cmd, args) + if existing != nil { + return existing(cmd, args) + } + return nil } } for _, sub := range root.Commands() { @@ -152,7 +168,7 @@ func guardDashArgs(cmd *cobra.Command, args []string) error { allow := stdinarg.ParseAllow(cmd.Annotations[stdinarg.AnnotationAllowDash]) allowed := 0 - var disallowed []string + var disallowedArgs, disallowedFlags []string for i, a := range args { if a != "-" || afterDashSeparator(cmd, i) { @@ -161,12 +177,16 @@ func guardDashArgs(cmd *cobra.Command, args []string) error { if allow.Arg(i) { allowed++ } else { - disallowed = append(disallowed, positionalName(cmd, i)) + disallowedArgs = append(disallowedArgs, positionalName(cmd, i)) } } + // Alias flags (--description/--desc) share one backing value, and pflag + // hands each alias the same Value instance — dedupe on it, or a value set + // through both spellings would count as two stdin inputs. + seen := map[pflag.Value]bool{} cmd.Flags().VisitAll(func(f *pflag.Flag) { - if !f.Changed { + if !f.Changed || seen[f.Value] { return } dashes := 0 @@ -183,28 +203,40 @@ func guardDashArgs(cmd *cobra.Command, args []string) error { } } } + default: + return } + seen[f.Value] = true if dashes == 0 { return } if allow.Flag(f.Name) { allowed += dashes } else { - disallowed = append(disallowed, "--"+f.Name) + disallowedFlags = append(disallowedFlags, "--"+f.Name) } }) if allowed > 1 { return output.ErrUsage(`only one input can read from stdin ("-") at a time`) } + disallowed := append(append([]string{}, disallowedArgs...), disallowedFlags...) if len(disallowed) > 0 && stdinarg.IsPiped(cmd.InOrStdin()) { msg := fmt.Sprintf(`%s does not read stdin via "-" for %s`, cmd.CommandPath(), strings.Join(disallowed, ", ")) - hint := `For a literal "-", pass it after the -- separator` + // -- only escapes positionals; a flag value has no in-line escape, so + // the honest remedy there is an unpiped stdin. + var hints []string + if len(disallowedArgs) > 0 { + hints = append(hints, `For a literal "-" argument, pass it after the -- separator`) + } + if len(disallowedFlags) > 0 { + hints = append(hints, `For a literal "-" flag value, run without piped stdin (append </dev/tty)`) + } if accepts := describeAllowed(cmd, allow); accepts != "" { - hint += "; this command reads stdin when \"-\" is given as " + accepts + hints = append(hints, "this command reads stdin when \"-\" is given as "+accepts) } - return output.ErrUsageHint(msg, hint) + return output.ErrUsageHint(msg, strings.Join(hints, "; ")) } return nil } diff --git a/internal/commands/stdin_test.go b/internal/commands/stdin_test.go index 891adedd7..28e5bf444 100644 --- a/internal/commands/stdin_test.go +++ b/internal/commands/stdin_test.go @@ -41,6 +41,17 @@ func TestReadStdinContentTrimsTrailingNewlines(t *testing.T) { assert.Equal(t, "🎉", content) } +// CRLF pipes (Windows tools, curl -w) must not leave a stray \r behind — it +// would count against boost's 16-rune limit and corrupt titles. +func TestReadStdinContentTrimsTrailingCRLF(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.SetIn(strings.NewReader("exactly16chars!!\r\n")) + + content, err := readStdinContent(cmd, "<content>") + require.NoError(t, err) + assert.Equal(t, "exactly16chars!!", content) +} + func TestReadStdinContentTTYIsUsageErrorWithEscapeHints(t *testing.T) { cmd := &cobra.Command{Use: "x"} devNullStdin(t, cmd) diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index a7a7f4c00..017141a06 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -119,7 +119,9 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, content is required and missing, the error teaches `-`). - Only one input can read stdin per invocation. - A literal `-` anywhere else (a title, a name, a path) **errors when stdin is - piped**; escape it after the `--` separator: `basecamp projects create -- -`. + piped**. Escape a positional after the `--` separator + (`basecamp projects create -- -`); a flag value has no in-line escape — run + it without the pipe (append `</dev/tty`). - `-` with nothing piped (interactive TTY) errors immediately instead of hanging; use a pipe, a heredoc (`basecamp comments create <id> - <<'EOF'`), or `--edit` where offered. From b8f09d7a7dbc482c7d2c65e74513faf34499ee19 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Wed, 19 Aug 2026 13:24:38 -0700 Subject: [PATCH 03/19] Address review: source-preserving stdin hints, bounded create arity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TTY hint for a flag-borne "-" suggested a bare trailing "-", which would exceed the command's positional arity — it now repeats the flag ("api post ... --data -"). messages/cards/docs create took unbounded positionals, so a stray third token was silently dropped after "-" had already drained stdin. All three now bound at MaximumNArgs(2), which runs before the read; the other exact-positional consumers were already bounded. Cobra's arity errors classified as api_error, telling agents to retry a call that can never succeed. They are usage errors by construction. Drop the concrete </dev/tty redirect from the literal-dash hint: it is unusable on Windows and on headless runners with no controlling terminal. The remedy stays, minus the platform-specific spelling. --- internal/cli/cobra_error_test.go | 41 ++++++++++ internal/cli/root.go | 8 ++ internal/commands/cards.go | 3 + internal/commands/dash_guard_test.go | 2 +- internal/commands/files.go | 3 + internal/commands/messages.go | 3 + internal/commands/stdin.go | 21 +++-- internal/commands/stdin_integration_test.go | 89 +++++++++++++++++++++ internal/commands/stdin_test.go | 13 +++ skills/basecamp/SKILL.md | 2 +- 10 files changed, 177 insertions(+), 8 deletions(-) create mode 100644 internal/cli/cobra_error_test.go diff --git a/internal/cli/cobra_error_test.go b/internal/cli/cobra_error_test.go new file mode 100644 index 000000000..0132ed8c4 --- /dev/null +++ b/internal/cli/cobra_error_test.go @@ -0,0 +1,41 @@ +package cli + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// Cobra's arity messages are usage failures by construction. They used to fall +// through to the default classification (api_error, exit 4), which tells an +// agent to retry a call that can never succeed. +func TestTransformCobraErrorClassifiesArityAsUsage(t *testing.T) { + for _, msg := range []string{ + "accepts at most 2 arg(s), received 3", + "accepts 1 arg(s), received 2", + "accepts between 1 and 2 arg(s), received 4", + } { + t.Run(msg, func(t *testing.T) { + err := transformCobraError(errors.New(msg)) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "expected *output.Error, got %T", err) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Equal(t, msg, outErr.Message, "the wording is already clear; only the code was wrong") + }) + } +} + +// The zero-arg case keeps its friendlier rewrite. +func TestTransformCobraErrorKeepsZeroArgRewrite(t *testing.T) { + err := transformCobraError(errors.New("accepts 1 arg(s), received 0")) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr)) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Equal(t, "ID required", outErr.Message) +} diff --git a/internal/cli/root.go b/internal/cli/root.go index ccc028354..c86e3a52a 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -664,6 +664,14 @@ func transformCobraError(err error) error { return output.ErrUsage("ID required") } + // Every other cobra arity message ("accepts at most 2 arg(s), received 3") + // is a usage error by construction — only the code was wrong, so agents + // branching on it saw api_error and could retry a call that will never + // succeed. The wording is already clear; keep it and fix the code. + if strings.Contains(msg, "arg(s), received ") { + return output.ErrUsage(msg) + } + // Transform "required flag(s) X not set" → more specific message if strings.HasPrefix(msg, "required flag(s) ") { re := regexp.MustCompile(`required flag\(s\) "(\w+)" not set`) diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 0c8469d8b..0022abad9 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -857,6 +857,9 @@ Use - as the body argument to read the body from stdin: printf 'Card body' | basecamp cards create "My card" - --in myproject`, Example: ` basecamp cards create "My card" --in myproject basecamp cards create --in myproject -- "--title with dashes"`, + // Bounded so a stray third token is a usage error rather than being + // silently dropped after "-" has already drained stdin. + Args: cobra.MaximumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { // Show help when invoked with no title if len(args) == 0 { diff --git a/internal/commands/dash_guard_test.go b/internal/commands/dash_guard_test.go index 9634afed9..3ac0c7cbf 100644 --- a/internal/commands/dash_guard_test.go +++ b/internal/commands/dash_guard_test.go @@ -67,7 +67,7 @@ func TestDashGuardRejectsUnlistedFlagWhenPiped(t *testing.T) { assert.Contains(t, outErr.Message, "--title") // -- doesn't escape flag values, so the hint must not claim it does. assert.NotContains(t, outErr.Hint, "-- separator") - assert.Contains(t, outErr.Hint, "/dev/tty") + assert.Contains(t, outErr.Hint, "without piped stdin") } // The guard runs at Args-validation time: before the command's own Args diff --git a/internal/commands/files.go b/internal/commands/files.go index 2a16f531c..2724226d9 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -1230,6 +1230,9 @@ func newDocsCreateCmd(project, vaultID *string) *cobra.Command { Use - as the content argument to read the document body from stdin: basecamp docs create "Title" - --in my-project < body.md`, + // Bounded so a stray third token is a usage error rather than being + // silently dropped after "-" has already drained stdin. + Args: cobra.MaximumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { // Show help when invoked with no arguments if len(args) == 0 { diff --git a/internal/commands/messages.go b/internal/commands/messages.go index a9355fa39..6e5460996 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -435,6 +435,9 @@ func newMessagesCreateCmd(project *string, messageBoard *string) *cobra.Command Use - as the body argument to read the body from stdin: printf 'Long **Markdown** body' | basecamp messages create "Title" -`, + // Bounded so a stray third token is a usage error rather than being + // silently dropped after "-" has already drained stdin. + Args: cobra.MaximumNArgs(2), RunE: func(cmd *cobra.Command, args []string) error { // Show help when invoked with no title if len(args) == 0 { diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index df87d8353..3eb2db466 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -51,7 +51,7 @@ func readStdinContent(cmd *cobra.Command, what string) (string, error) { if !stdinarg.IsPiped(cmd.InOrStdin()) { return "", output.ErrUsageHint( fmt.Sprintf(`%s is "-" (read from stdin) but nothing is piped`, what), - stdinEscapeHint(cmd), + stdinEscapeHint(cmd, what), ) } data, err := io.ReadAll(cmd.InOrStdin()) @@ -67,11 +67,18 @@ func readStdinContent(cmd *cobra.Command, what string) (string, error) { // stdinEscapeHint lists the ways to satisfy a "-" from an interactive // terminal, mentioning --edit only where the command has it. -func stdinEscapeHint(cmd *cobra.Command) string { +// +// The examples repeat the input that actually carried the "-": suggesting a +// bare trailing "-" for a flag would exceed the command's positional arity. +func stdinEscapeHint(cmd *cobra.Command, what string) string { path := cmd.CommandPath() + source := "-" + if strings.HasPrefix(what, "--") { + source = what + " -" + } hint := fmt.Sprintf( - "Pipe the content (printf '...' | %[1]s ... -), use a heredoc (%[1]s ... - <<'EOF'), or run cat | %[1]s ... - and type the content, ending with Ctrl-D", - path) + "Pipe the content (printf '...' | %[1]s ... %[2]s), use a heredoc (%[1]s ... %[2]s <<'EOF'), or run cat | %[1]s ... %[2]s and type the content, ending with Ctrl-D", + path, source) if cmd.Flags().Lookup("edit") != nil { hint += "; or compose with --edit" } @@ -225,13 +232,15 @@ func guardDashArgs(cmd *cobra.Command, args []string) error { msg := fmt.Sprintf(`%s does not read stdin via "-" for %s`, cmd.CommandPath(), strings.Join(disallowed, ", ")) // -- only escapes positionals; a flag value has no in-line escape, so - // the honest remedy there is an unpiped stdin. + // the honest remedy there is an unpiped stdin. Naming a concrete + // redirect would be wrong on Windows and on headless runners with no + // controlling terminal, so the hint stays at the shape of the fix. var hints []string if len(disallowedArgs) > 0 { hints = append(hints, `For a literal "-" argument, pass it after the -- separator`) } if len(disallowedFlags) > 0 { - hints = append(hints, `For a literal "-" flag value, run without piped stdin (append </dev/tty)`) + hints = append(hints, `For a literal "-" flag value, run the command without piped stdin`) } if accepts := describeAllowed(cmd, allow); accepts != "" { hints = append(hints, "this command reads stdin when \"-\" is given as "+accepts) diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index df20696f5..5dec0a764 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -7,9 +7,11 @@ import ( "bytes" "encoding/json" "errors" + "net/http" "strings" "testing" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -149,3 +151,90 @@ func TestTodosUpdateDescriptionDashReadsStdin(t *testing.T) { } assert.True(t, found, "the piped description should reach the wire as HTML") } + +// countingTransport records whether any request escaped the command. +type countingTransport struct{ calls int } + +func (t *countingTransport) RoundTrip(*http.Request) (*http.Response, error) { + t.calls++ + return nil, errors.New("network disabled in tests") +} + +// trackingReader records whether stdin was ever read. +type trackingReader struct { + r *strings.Reader + read bool +} + +func (t *trackingReader) Read(p []byte) (int, error) { + t.read = true + return t.r.Read(p) +} + +func setupTransportTestApp(t *testing.T, transport http.RoundTripper) *appctx.App { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") + + cfg := &config.Config{AccountID: "99999", ProjectID: "123"} + authMgr := auth.NewManager(cfg, nil) + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &testTokenProvider{}, + basecamp.WithTransport(transport), + basecamp.WithMaxRetries(1), + ) + return &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + Names: names.NewResolver(sdkClient, authMgr, cfg.AccountID), + Output: output.New(output.Options{Format: output.FormatJSON, Writer: &bytes.Buffer{}}), + } +} + +// The <title> [body] creates bound their arity, so a stray trailing token is a +// usage error at Args-validation time — before "-" drains stdin and before any +// request is built. Without the bound the extra token was silently dropped +// *after* stdin had already been consumed. +func TestExactPositionalCreatesRejectExtraArgsBeforeConsumingStdin(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + path []string + }{ + {"messages", NewMessagesCmd, []string{"create"}}, + {"cards", NewCardsCmd, []string{"create"}}, + {"docs", NewDocsCmd, []string{"documents", "create"}}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + stdin := &trackingReader{r: strings.NewReader("body from stdin")} + cmd := tc.cmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + args := append(append([]string{}, tc.path...), "Title", "-", "unexpected") + err := executeCommand(cmd, app, args...) + require.Error(t, err) + assert.Contains(t, err.Error(), "accepts at most 2 arg") + assert.False(t, stdin.read, "stdin must not be consumed before arity validation") + assert.Zero(t, transport.calls, "no request may be issued") + }) + } +} + +// A flag-borne "-" with nothing piped must suggest an escape that parses: +// "api post ... --data -", never a bare positional "-" (api post takes one). +func TestAPIPostDataDashOnTTYHintPreservesTheFlag(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + cmd := NewAPICmd() + InstallDashGuard(cmd) + devNullStdin(t, cmd) + + err := executeCommand(cmd, app, "post", "/buckets/1/todos.json", "--data", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Hint, "api post ... --data -") + assert.Zero(t, transport.calls) +} diff --git a/internal/commands/stdin_test.go b/internal/commands/stdin_test.go index 28e5bf444..229d81e54 100644 --- a/internal/commands/stdin_test.go +++ b/internal/commands/stdin_test.go @@ -63,6 +63,19 @@ func TestReadStdinContentTTYIsUsageErrorWithEscapeHints(t *testing.T) { assert.NotContains(t, outErr.Hint, "--edit", "no --edit flag on this command") } +// The hint must name the input that actually carried the "-". Suggesting a +// bare trailing "-" for a flag-borne dash would exceed the command's +// positional arity — an escape the caller cannot use. +func TestReadStdinContentTTYHintNamesTheFlag(t *testing.T) { + cmd := &cobra.Command{Use: "post <path>"} + devNullStdin(t, cmd) + + _, err := readStdinContent(cmd, "--data") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Hint, "--data -") + assert.NotContains(t, outErr.Hint, "... -)", "a bare positional dash would break arity") +} + func TestReadStdinContentTTYHintMentionsEditWhenAvailable(t *testing.T) { cmd := &cobra.Command{Use: "x"} cmd.Flags().Bool("edit", false, "") diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 017141a06..2ec8e0957 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -121,7 +121,7 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, - A literal `-` anywhere else (a title, a name, a path) **errors when stdin is piped**. Escape a positional after the `--` separator (`basecamp projects create -- -`); a flag value has no in-line escape — run - it without the pipe (append `</dev/tty`). + the command without piped stdin. - `-` with nothing piped (interactive TTY) errors immediately instead of hanging; use a pipe, a heredoc (`basecamp comments create <id> - <<'EOF'`), or `--edit` where offered. From 9310135a9a68abc098c7eb5bdfefe2316e1d54ad Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Wed, 19 Aug 2026 14:10:32 -0700 Subject: [PATCH 04/19] Address review: gate pickers on stdin, reject dual chat sources, fix docs path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Resolver.IsInteractive now requires stdin to be a character device too: a Bubble Tea picker reads keystrokes from stdin, so piped stdin can never drive one — and when a command is consuming piped content (a "-" stdin input), a picker would eat that content as key events. This closes the ordering hazard for every picker at the mechanism rather than reordering each stdin-enabled RunE (gauges create, docs, cards, schedule, templates). - chat post/update reject a positional message combined with --content instead of the positional silently winning; with "-" in play the losing source would discard piped content unread. - SKILL.md and the docs-create example referenced 'docs create', which does not exist; the registered path is 'docs documents create'. --- internal/commands/chat.go | 18 ++++++++--- internal/commands/chat_test.go | 16 ++++++++++ internal/commands/files.go | 2 +- internal/tui/resolve/resolve.go | 20 +++++++++--- internal/tui/resolve/resolve_test.go | 46 ++++++++++++++++++++++++++++ skills/basecamp/SKILL.md | 2 +- 6 files changed, 93 insertions(+), 11 deletions(-) create mode 100644 internal/tui/resolve/resolve_test.go diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 66d89ea97..26e931a00 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -316,12 +316,16 @@ Use - as the message argument to read the message from stdin: RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - // Validate user input first, before checking account. The - // positional wins over --content before "-" resolution, so only - // the winning source can consume stdin. + // Validate user input first, before checking account. A + // positional and --content are the same input, so supplying both + // is rejected rather than one silently winning — with "-" in + // play, the losing source would discard piped content unread. messageContent := content argIndex, what := -1, "--content" if len(args) > 0 { + if cmd.Flags().Changed("content") { + return output.ErrUsage("cannot combine a <message> argument with --content") + } messageContent = args[0] argIndex, what = 0, "<message>" } @@ -781,11 +785,15 @@ edit to rich text.`, return missingArg(cmd, "<id|url>") } - // The positional wins over --content before "-" resolution, so - // only the winning source can consume stdin. + // A positional and --content are the same input, so supplying + // both is rejected rather than one silently winning — with "-" + // in play, the losing source would discard piped content unread. messageContent := content argIndex, what := -1, "--content" if len(args) > 1 { + if cmd.Flags().Changed("content") { + return output.ErrUsage("cannot combine a [content] argument with --content") + } messageContent = args[1] argIndex, what = 1, "[content]" } diff --git a/internal/commands/chat_test.go b/internal/commands/chat_test.go index f58cafd4f..8d126f7d5 100644 --- a/internal/commands/chat_test.go +++ b/internal/commands/chat_test.go @@ -1796,3 +1796,19 @@ func TestChatRoomShorthandFlag(t *testing.T) { require.Len(t, envelope.Data, 1) assert.Equal(t, "Engineering", envelope.Data[0]["title"]) } + +// TestChatPostRejectsPositionalWithContentFlag verifies that a positional +// message and --content are rejected together instead of one silently +// winning — with "-" in play, the losing source would discard piped +// content unread. +func TestChatPostRejectsPositionalWithContentFlag(t *testing.T) { + app := &appctx.App{Config: &config.Config{}} + + err := executeChatCommand(NewChatCmd(), app, "post", "literal", "--content", "other") + require.Error(t, err) + require.Contains(t, err.Error(), "cannot combine") + + err = executeChatCommand(NewChatCmd(), app, "update", "123", "literal", "--content", "other") + require.Error(t, err) + require.Contains(t, err.Error(), "cannot combine") +} diff --git a/internal/commands/files.go b/internal/commands/files.go index 2724226d9..1aae47149 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -1229,7 +1229,7 @@ func newDocsCreateCmd(project, vaultID *string) *cobra.Command { Long: `Create a new document in a project's Docs & Files area. Use - as the content argument to read the document body from stdin: - basecamp docs create "Title" - --in my-project < body.md`, + basecamp docs documents create "Title" - --in my-project < body.md`, // Bounded so a stray third token is a usage error rather than being // silently dropped after "-" has already drained stdin. Args: cobra.MaximumNArgs(2), diff --git a/internal/tui/resolve/resolve.go b/internal/tui/resolve/resolve.go index 3018737aa..f66249791 100644 --- a/internal/tui/resolve/resolve.go +++ b/internal/tui/resolve/resolve.go @@ -101,11 +101,11 @@ func (r *Resolver) Flags() *Flags { } // IsInteractive returns true if interactive prompts can be shown. -// This checks both stdout and machine-output flags. +// This checks stdout, stdin, and machine-output flags. // Returns false if BASECAMP_NONINTERACTIVE is set, if any machine-output flag is -// set (--agent, --json, --quiet, --ids-only, --count), or if stdout is not a -// character device (the guard treats any char device — a terminal, /dev/null, -// etc. — as interactive-capable). +// set (--agent, --json, --quiet, --ids-only, --count), or if stdout or stdin is +// not a character device (the guard treats any char device — a terminal, +// /dev/null, etc. — as interactive-capable). func (r *Resolver) IsInteractive() bool { // Explicit escape hatch: BASECAMP_NONINTERACTIVE forces non-interactive mode // even under a PTY, without changing the output format. @@ -125,6 +125,18 @@ func (r *Resolver) IsInteractive() bool { if err != nil { return false } + if fi.Mode()&os.ModeCharDevice == 0 { + return false + } + + // Stdin must be a character device too: pickers read keystrokes from + // stdin, so a pipe or redirected file can never drive one — and when the + // command is consuming piped content (a "-" stdin input), a picker would + // eat that content as key events. + fi, err = os.Stdin.Stat() + if err != nil { + return false + } return (fi.Mode() & os.ModeCharDevice) != 0 } diff --git a/internal/tui/resolve/resolve_test.go b/internal/tui/resolve/resolve_test.go new file mode 100644 index 000000000..bdd1a0235 --- /dev/null +++ b/internal/tui/resolve/resolve_test.go @@ -0,0 +1,46 @@ +package resolve + +import ( + "os" + "testing" +) + +// TestIsInteractiveRequiresStdinCharDevice proves pickers are gated off when +// stdin is piped: a Bubble Tea picker reads keystrokes from stdin, so piped +// stdin can never drive one — and when a command is consuming piped content +// (a "-" stdin input), a picker would eat that content as key events. +func TestIsInteractiveRequiresStdinCharDevice(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "") + + devnull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) + if err != nil { + t.Fatalf("open %s: %v", os.DevNull, err) + } + defer devnull.Close() + + pipeR, pipeW, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer pipeR.Close() + defer pipeW.Close() + + origOut, origIn := os.Stdout, os.Stdin + t.Cleanup(func() { os.Stdout, os.Stdin = origOut, origIn }) + + // /dev/null is a character device, so it stands in for a terminal on + // both ends without needing a PTY. + os.Stdout = devnull + + r := New(nil, nil, nil) + + os.Stdin = devnull + if !r.IsInteractive() { + t.Fatal("expected interactive with char-device stdout and stdin") + } + + os.Stdin = pipeR + if r.IsInteractive() { + t.Fatal("expected non-interactive with piped stdin: a picker would consume the pipe as key events") + } +} diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 2ec8e0957..cf89c67b9 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -110,7 +110,7 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, ``` `-` means "read from stdin" on every content input: content-kind positionals (`comments create/update`, `messages create [body]`, `cards create [body]`, - `todos create`, `docs create [content]`, `chat post/update`, `boost create`, + `todos create`, `docs documents create [content]`, `chat post/update`, `boost create`, `checkins answer create/update`, `notes set`) and content flags (`--data` on `api post/put`, `--body`, `--content`, `--description`, `--comment` on `todos sweep`, `--file` on `notes set`). Each command's `--agent` help lists From 1322a368b9b8638249569bb10283fd79d181613c Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Wed, 19 Aug 2026 15:29:01 -0700 Subject: [PATCH 05/19] Address review: gate first-run wizard (all TUIs) on stdin too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The root skip's justification held for quickstart's summary but missed the first-run wizard: isFirstRun gates on App.IsInteractive, which only looked at stdout, so piped stdin plus a terminal stdout on a first run launched the wizard reading the pipe as keystrokes. Extract the stdout+stdin character-device check into stdinarg.InteractiveStdio — the second copy of this logic — and use it from both App.IsInteractive and resolve.Resolver.IsInteractive. Every TUI gate (wizard, pickers, animations, update notice) now takes its non-interactive path when either end of stdio is piped. --- internal/appctx/context.go | 13 +++++----- internal/commands/stdin.go | 4 +++- internal/stdinarg/stdinarg.go | 16 +++++++++++++ internal/stdinarg/stdinarg_test.go | 38 ++++++++++++++++++++++++++++++ internal/tui/resolve/resolve.go | 25 +++++--------------- 5 files changed, 69 insertions(+), 27 deletions(-) diff --git a/internal/appctx/context.go b/internal/appctx/context.go index 53bacef99..54906af87 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -19,6 +19,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/observability" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/resilience" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui/resolve" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -374,13 +375,11 @@ func (a *App) IsInteractive() bool { return false } - // Check if stdout is a terminal - fi, err := os.Stdout.Stat() - if err != nil { - return false - } - - return (fi.Mode() & os.ModeCharDevice) != 0 + // Both stdout and stdin must be character devices: a TUI draws to stdout + // and reads keystrokes from stdin, so a pipe on either end can never + // drive one — and when the command is consuming piped content (a "-" + // stdin input), a TUI would eat that content as key events. + return stdinarg.InteractiveStdio() } // WithApp stores the app in the context. diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index 3eb2db466..1cd458517 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -143,7 +143,9 @@ func InstallDashGuard(root *cobra.Command) { // turn "basecamp unknowncmd" into a quickstart run. Leaving the root // unguarded loses nothing: its positionals are subcommand names, and a // bare "basecamp -" just runs quickstart, which posts no content for a - // literal "-" to corrupt. + // literal "-" to corrupt and whose TUI paths (the first-run wizard) are + // stdin-gated by stdinarg.InteractiveStdio — piped stdin routes to the + // non-interactive summary instead of a wizard that would eat the pipe. skipRoot := root.Args == nil && !root.HasParent() && root.HasSubCommands() if root.Runnable() && !skipRoot { existing := root.Args diff --git a/internal/stdinarg/stdinarg.go b/internal/stdinarg/stdinarg.go index d22d5e4ed..193643bc8 100644 --- a/internal/stdinarg/stdinarg.go +++ b/internal/stdinarg/stdinarg.go @@ -91,3 +91,19 @@ func IsPiped(r io.Reader) bool { } return fi.Mode()&os.ModeCharDevice == 0 } + +// InteractiveStdio reports whether both stdout and stdin are character +// devices — the floor for launching anything that draws to the terminal and +// reads keystrokes. A TUI (picker, wizard) reads key events from stdin, so a +// pipe or redirected file can never drive one — and when the command is +// consuming piped content (a "-" stdin input), a TUI would eat that content +// as key events. +func InteractiveStdio() bool { + for _, f := range []*os.File{os.Stdout, os.Stdin} { + fi, err := f.Stat() + if err != nil || fi.Mode()&os.ModeCharDevice == 0 { + return false + } + } + return true +} diff --git a/internal/stdinarg/stdinarg_test.go b/internal/stdinarg/stdinarg_test.go index 34ccbb135..d80c06579 100644 --- a/internal/stdinarg/stdinarg_test.go +++ b/internal/stdinarg/stdinarg_test.go @@ -65,3 +65,41 @@ func TestIsPipedRegularFile(t *testing.T) { assert.True(t, IsPiped(f)) } + +// TestInteractiveStdio proves TUIs are gated off when stdin is piped: a +// wizard or picker reads keystrokes from stdin, so piped stdin would be +// consumed as key events — including piped content meant for a "-" input. +func TestInteractiveStdio(t *testing.T) { + devnull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0) + if err != nil { + t.Fatalf("open %s: %v", os.DevNull, err) + } + defer devnull.Close() + + pipeR, pipeW, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer pipeR.Close() + defer pipeW.Close() + + origOut, origIn := os.Stdout, os.Stdin + t.Cleanup(func() { os.Stdout, os.Stdin = origOut, origIn }) + + // /dev/null is a character device, standing in for a terminal on both + // ends without needing a PTY. + os.Stdout, os.Stdin = devnull, devnull + if !InteractiveStdio() { + t.Fatal("expected interactive with char-device stdout and stdin") + } + + os.Stdin = pipeR + if InteractiveStdio() { + t.Fatal("expected non-interactive with piped stdin") + } + + os.Stdout, os.Stdin = pipeW, devnull + if InteractiveStdio() { + t.Fatal("expected non-interactive with piped stdout") + } +} diff --git a/internal/tui/resolve/resolve.go b/internal/tui/resolve/resolve.go index f66249791..65a9496dd 100644 --- a/internal/tui/resolve/resolve.go +++ b/internal/tui/resolve/resolve.go @@ -5,12 +5,12 @@ package resolve import ( "context" - "os" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/basecamp/basecamp-cli/internal/auth" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui" ) @@ -120,24 +120,11 @@ func (r *Resolver) IsInteractive() bool { } } - // Check if stdout is a character device (e.g. a terminal) - fi, err := os.Stdout.Stat() - if err != nil { - return false - } - if fi.Mode()&os.ModeCharDevice == 0 { - return false - } - - // Stdin must be a character device too: pickers read keystrokes from - // stdin, so a pipe or redirected file can never drive one — and when the - // command is consuming piped content (a "-" stdin input), a picker would - // eat that content as key events. - fi, err = os.Stdin.Stat() - if err != nil { - return false - } - return (fi.Mode() & os.ModeCharDevice) != 0 + // Both stdout and stdin must be character devices: pickers draw to + // stdout and read keystrokes from stdin, so a pipe on either end can + // never drive one — and when the command is consuming piped content + // (a "-" stdin input), a picker would eat that content as key events. + return stdinarg.InteractiveStdio() } // ResolvedValue represents a value that was resolved, along with metadata From 6a98dd7ef587c171a3e110442f1c1097a3a3646b Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Wed, 19 Aug 2026 15:28:19 -0700 Subject: [PATCH 06/19] Address review: source ordering, chat precedence, alias groups, anchored arity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit chat post/update let a positional and an explicit --content coexist, with the positional silently winning — so --content - dropped the flag and left the pipe unread. Both now reject two explicit sources. Seven commands resolved "-" after account or project work, so a stdin mistake surfaced as "--account is required" instead of the stdin hint. The order is now local validation, then stdin, then account/network at every resolveContentValue site; an audit script confirms none remain. Long help and SKILL.md taught "basecamp docs create", which resolves to the docs group and exits 0 showing help. The real path is "docs documents create". A new test resolves every help example through Find and fails when a group swallows a leftover subcommand name. transformCobraError matched arity text anywhere in any error, flattening typed errors that merely quoted the phrase. It now returns typed errors untouched and anchors on cobra's exact arity formats. The guard named one alias of a shared value, reporting --in for a caller who wrote --project. Parsed state cannot say which spelling was typed, so the error names the group (--in/--project). --- internal/cli/cobra_error_test.go | 48 +++++++++- internal/cli/root.go | 22 ++++- internal/commands/cards.go | 11 ++- internal/commands/dash_guard_test.go | 33 +++++++ internal/commands/files.go | 13 ++- internal/commands/gauges.go | 45 +++++---- internal/commands/help_paths_test.go | 101 ++++++++++++++++++++ internal/commands/schedule.go | 12 ++- internal/commands/stdin.go | 78 +++++++++++---- internal/commands/stdin_integration_test.go | 89 +++++++++++++++++ internal/commands/templates.go | 40 ++++---- 11 files changed, 421 insertions(+), 71 deletions(-) create mode 100644 internal/commands/help_paths_test.go diff --git a/internal/cli/cobra_error_test.go b/internal/cli/cobra_error_test.go index 0132ed8c4..f44ae1b5c 100644 --- a/internal/cli/cobra_error_test.go +++ b/internal/cli/cobra_error_test.go @@ -7,11 +7,13 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-cli/internal/output" ) // Cobra's arity messages are usage failures by construction. They used to fall -// through to the default classification (api_error, exit 4), which tells an +// through to the default classification (api_error, exit 7), which tells an // agent to retry a call that can never succeed. func TestTransformCobraErrorClassifiesArityAsUsage(t *testing.T) { for _, msg := range []string{ @@ -39,3 +41,47 @@ func TestTransformCobraErrorKeepsZeroArgRewrite(t *testing.T) { assert.Equal(t, output.CodeUsage, outErr.Code) assert.Equal(t, "ID required", outErr.Message) } + +// A typed error already carries a code, an HTTP status and a retryable flag. +// Matching on its rendered text would flatten all of that into a bare usage +// string — so an API error that merely quotes an arity phrase is left alone. +func TestTransformCobraErrorPreservesTypedErrors(t *testing.T) { + t.Run("SDK error", func(t *testing.T) { + original := &basecamp.Error{ + Code: basecamp.CodeAPI, + Message: "server rejected the payload: accepts 1 arg(s), received 2", + HTTPStatus: 422, + Retryable: true, + } + + err := transformCobraError(original) + + var sdkErr *basecamp.Error + require.True(t, errors.As(err, &sdkErr), "expected the SDK error to survive, got %T", err) + assert.Equal(t, basecamp.CodeAPI, sdkErr.Code) + assert.Equal(t, 422, sdkErr.HTTPStatus) + assert.True(t, sdkErr.Retryable) + }) + + t.Run("output error", func(t *testing.T) { + original := output.ErrNotFound("todo", "123") + + err := transformCobraError(original) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr)) + assert.Equal(t, output.CodeNotFound, outErr.Code, "must not be reclassified as usage") + }) +} + +// Anchored: a command's own error that merely contains the phrase is not an +// arity failure and must keep its own classification path. +func TestTransformCobraErrorIgnoresUnanchoredArityText(t *testing.T) { + msg := "the API said: accepts 1 arg(s), received 2 (and then some)" + + err := transformCobraError(errors.New(msg)) + + var outErr *output.Error + assert.False(t, errors.As(err, &outErr), "should be left untouched, got %T", err) + assert.Equal(t, msg, err.Error()) +} diff --git a/internal/cli/root.go b/internal/cli/root.go index c86e3a52a..7431a4102 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -2,6 +2,7 @@ package cli import ( "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -9,6 +10,7 @@ import ( "sort" "strings" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/itchyny/gojq" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -619,9 +621,27 @@ func isMachineConsumer(root *cobra.Command) bool { return false } +// cobraArityError matches cobra's four arity messages exactly (ExactArgs, +// MaximumNArgs, RangeArgs; MinimumNArgs is handled by an earlier rule). Anchored +// so a command's own error that merely quotes the phrase is left alone. +var cobraArityError = regexp.MustCompile(`^accepts (\d+|at most \d+|between \d+ and \d+) arg\(s\), received \d+$`) + // transformCobraError transforms Cobra's default error messages to match the // Bash CLI format for consistency with existing tests and user expectations. +// +// Only untyped errors are rewritten. An error that already carries a code, an +// HTTP status, a hint or a retryable flag is ours or the SDK's, and matching on +// its rendered text would flatten that metadata into a bare usage string. func transformCobraError(err error) error { + var outErr *output.Error + if errors.As(err, &outErr) { + return err + } + var sdkErr *basecamp.Error + if errors.As(err, &sdkErr) { + return err + } + msg := err.Error() // Transform "flag needs an argument: --FLAG" → "--FLAG requires a value" @@ -668,7 +688,7 @@ func transformCobraError(err error) error { // is a usage error by construction — only the code was wrong, so agents // branching on it saw api_error and could retry a call that will never // succeed. The wording is already clear; keep it and fix the code. - if strings.Contains(msg, "arg(s), received ") { + if cobraArityError.MatchString(msg) { return output.ErrUsage(msg) } diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 0022abad9..764d363ac 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -1091,6 +1091,13 @@ You can pass either a card ID or a Basecamp URL: return noChanges(cmd) } + // Resolve "-" before any account or network work, so a bad stdin + // gets the stdin error rather than "--account is required". + content, err := resolveContentValue(cmd, content, -1, "--body") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { @@ -1109,10 +1116,6 @@ You can pass either a card ID or a Basecamp URL: if title != "" { req.Title = &title } - content, err = resolveContentValue(cmd, content, -1, "--body") - if err != nil { - return err - } var mentionNotice string var html string diff --git a/internal/commands/dash_guard_test.go b/internal/commands/dash_guard_test.go index 3ac0c7cbf..75b0eb702 100644 --- a/internal/commands/dash_guard_test.go +++ b/internal/commands/dash_guard_test.go @@ -220,3 +220,36 @@ func TestDownloadCommandsExemptOutFlag(t *testing.T) { assert.True(t, allow.Flag("out"), "%s download should exempt --out", cmd.Name()) } } + +// Parsed state keeps only the merged value, never which spelling the caller +// typed — so naming one alias is a coin flip that reports "--in" for a caller +// who wrote "--project -". Name the whole group instead. +func TestDashGuardNamesTheWholeAliasGroup(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + // --project/--in are two spellings of one persistent value on the group. + cmd := NewCardsCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "create", "Title", "--in", "old", "--project", "-") + outErr := requireUsageErr(t, err) + // pflag visits flags in sorted order, so the group label is stable. + assert.Contains(t, outErr.Message, "--in/--project") +} + +// A flag with no alias still reads as a single spelling. +func TestDashGuardNamesASoloFlagPlainly(t *testing.T) { + app, _ := setupTestApp(t) + app.Flags.JSON = true + + cmd := NewTodosCmd() + InstallDashGuard(cmd) + cmd.SetIn(strings.NewReader("piped")) + + err := executeCommand(cmd, app, "update", "1", "--title", "-") + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "--title") + assert.NotContains(t, outErr.Message, "/--") +} diff --git a/internal/commands/files.go b/internal/commands/files.go index 1aae47149..ff0367916 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -1241,11 +1241,8 @@ Use - as the content argument to read the document body from stdin: title := args[0] - app := appctx.FromContext(cmd.Context()) - - if err := ensureAccount(cmd, app); err != nil { - return err - } + // Resolve "-" before any account or network work, so a bad stdin + // gets the stdin error rather than "--account is required". content := "" if len(args) > 1 { var contentErr error @@ -1255,6 +1252,12 @@ Use - as the content argument to read the document body from stdin: } } + app := appctx.FromContext(cmd.Context()) + + if err := ensureAccount(cmd, app); err != nil { + return err + } + // Resolve subscription flags before project (fail fast on bad input) subs, err := applySubscribeFlags(cmd.Context(), app.Names, subscribe, cmd.Flags().Changed("subscribe"), noSubscribe) if err != nil { diff --git a/internal/commands/gauges.go b/internal/commands/gauges.go index d90fcdfe3..33f55facd 100644 --- a/internal/commands/gauges.go +++ b/internal/commands/gauges.go @@ -178,6 +178,20 @@ func newGaugesCreateCmd(project *string) *cobra.Command { basecamp gauges create --position 75 --color green --in MyProject basecamp gauges create --position 50 --color yellow --description "Halfway there" --in MyProject`, RunE: func(cmd *cobra.Command, args []string) error { + if !cmd.Flags().Changed("position") { + return output.ErrUsage("--position is required") + } + if position < 0 || position > 100 { + return output.ErrUsage("--position must be between 0 and 100") + } + + // Local validation, then "-", then account: a bad stdin gets the + // stdin error rather than "--account is required". + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { @@ -194,13 +208,6 @@ func newGaugesCreateCmd(project *string) *cobra.Command { return output.ErrUsage("Invalid project ID") } - if !cmd.Flags().Changed("position") { - return output.ErrUsage("--position is required") - } - if position < 0 || position > 100 { - return output.ErrUsage("--position must be between 0 and 100") - } - req := &basecamp.CreateGaugeNeedleRequest{ Position: position, } @@ -208,10 +215,6 @@ func newGaugesCreateCmd(project *string) *cobra.Command { req.Color = color } if description != "" { - description, err = resolveContentValue(cmd, description, -1, "--description") - if err != nil { - return err - } req.Description = description } if notify != "" { @@ -264,6 +267,17 @@ func newGaugesUpdateCmd() *cobra.Command { basecamp gauges update 12345 --description "Updated status"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if !cmd.Flags().Changed("description") { + return output.ErrUsage("No changes specified (use --description)") + } + + // Resolve "-" before any account or network work, so a bad stdin + // gets the stdin error rather than "--account is required". + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { @@ -275,15 +289,6 @@ func newGaugesUpdateCmd() *cobra.Command { return output.ErrUsage("Invalid needle ID") } - if !cmd.Flags().Changed("description") { - return output.ErrUsage("No changes specified (use --description)") - } - - description, err = resolveContentValue(cmd, description, -1, "--description") - if err != nil { - return err - } - req := &basecamp.UpdateGaugeNeedleRequest{ Description: basecamp.Ptr(description), } diff --git a/internal/commands/help_paths_test.go b/internal/commands/help_paths_test.go new file mode 100644 index 000000000..194ef9e83 --- /dev/null +++ b/internal/commands/help_paths_test.go @@ -0,0 +1,101 @@ +package commands_test + +import ( + "regexp" + "strings" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A help example that names a path cobra cannot resolve exits 0 showing group +// help, so nothing fails and the wrong path keeps getting taught. That is how +// "basecamp docs create" survived: Find() landed on the `docs` group with +// "create" left over. +// +// Coverage is bounded and deliberately so: only the leading run of bare +// lowercase words after "basecamp" is resolved (stopping at the first flag, +// placeholder, quote, or shell metacharacter), and a leftover token is only +// reported when it is the name or alias of some command in the tree — which is +// what distinguishes a mistyped subcommand from a literal argument like a +// project name. +func TestHelpExampleCommandPathsResolveExactly(t *testing.T) { + root := buildRootWithAllCommands() + + commandWords := map[string]bool{} + var collect func(*cobra.Command) + collect = func(cmd *cobra.Command) { + for _, sub := range cmd.Commands() { + commandWords[sub.Name()] = true + for _, alias := range sub.Aliases { + commandWords[alias] = true + } + collect(sub) + } + } + collect(root) + + word := regexp.MustCompile(`^[a-z][a-z0-9-]*$`) + + var checked int + var walk func(*cobra.Command) + walk = func(cmd *cobra.Command) { + for _, text := range []string{cmd.Long, cmd.Example} { + for _, line := range strings.Split(text, "\n") { + _, after, found := strings.Cut(strings.TrimSpace(line), "basecamp ") + if !found { + continue + } + var path []string + for _, token := range strings.Fields(after) { + if !word.MatchString(token) { + break + } + path = append(path, token) + } + // Prose ("basecamp is a CLI tool ...") is not an invocation: + // require the first word to be a real top-level command. + if len(path) == 0 || root.Commands() == nil || !isTopLevel(root, path[0]) { + continue + } + + target, remaining, err := root.Find(path) + require.NoError(t, err, "line %q", line) + checked++ + + // Only a *group* can swallow a mistyped subcommand: it shows + // its help and exits 0. A leaf's leftovers are its arguments + // ("assignments due overdue"), even when the word happens to + // name a command elsewhere in the tree. + if len(remaining) > 0 && target.HasSubCommands() && commandWords[remaining[0]] { + assert.Fail(t, + "example names a path that does not resolve", + "%s: %q resolves to the %q group with %q left over — it exits 0 showing group help", + cmd.CommandPath(), strings.TrimSpace(line), target.CommandPath(), remaining[0]) + } + } + } + for _, sub := range cmd.Commands() { + walk(sub) + } + } + walk(root) + + require.Greater(t, checked, 100, "expected the help corpus to yield many command paths") +} + +func isTopLevel(root *cobra.Command, name string) bool { + for _, sub := range root.Commands() { + if sub.Name() == name { + return true + } + for _, alias := range sub.Aliases { + if alias == name { + return true + } + } + } + return false +} diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index c1c7e667b..e4fd808c9 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -428,11 +428,6 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { return missingArg(cmd, "<summary>") } - app := appctx.FromContext(cmd.Context()) - if err := ensureAccount(cmd, app); err != nil { - return err - } - if startsAt == "" { return output.ErrUsage("--starts-at required (ISO 8601 datetime)") } @@ -446,11 +441,18 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { return err } + // Local validation, then "-", then account: a bad stdin gets the + // stdin error rather than "--account is required". description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err } + app := appctx.FromContext(cmd.Context()) + if err := ensureAccount(cmd, app); err != nil { + return err + } + return runScheduleCreate(cmd, app, *project, *scheduleID, entrySummary, startsAt, endsAt, description, allDay, notify, visibleToClients, participants, subscribe, noSubscribe, attachFiles) }, } diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index 1cd458517..1e0efe076 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -190,22 +190,15 @@ func guardDashArgs(cmd *cobra.Command, args []string) error { } } - // Alias flags (--description/--desc) share one backing value, and pflag - // hands each alias the same Value instance — dedupe on it, or a value set - // through both spellings would count as two stdin inputs. - seen := map[pflag.Value]bool{} - cmd.Flags().VisitAll(func(f *pflag.Flag) { - if !f.Changed || seen[f.Value] { - return - } + for _, group := range changedFlagGroups(cmd) { dashes := 0 - switch f.Value.Type() { + switch group.value.Type() { case "string": - if f.Value.String() == "-" { + if group.value.String() == "-" { dashes = 1 } case "stringArray", "stringSlice": - if sv, ok := f.Value.(pflag.SliceValue); ok { + if sv, ok := group.value.(pflag.SliceValue); ok { for _, v := range sv.GetSlice() { if v == "-" { dashes++ @@ -213,18 +206,17 @@ func guardDashArgs(cmd *cobra.Command, args []string) error { } } default: - return + continue } - seen[f.Value] = true if dashes == 0 { - return + continue } - if allow.Flag(f.Name) { + if group.allowed(allow) { allowed += dashes } else { - disallowedFlags = append(disallowedFlags, "--"+f.Name) + disallowedFlags = append(disallowedFlags, group.label()) } - }) + } if allowed > 1 { return output.ErrUsage(`only one input can read from stdin ("-") at a time`) @@ -252,6 +244,58 @@ func guardDashArgs(cmd *cobra.Command, args []string) error { return nil } +// flagGroup is one logical input: every spelling pflag has bound to the same +// backing value. Aliases (--description/--desc, --in/--project) share a single +// pflag.Value instance, so grouping on it keeps a value set through two +// spellings from counting as two stdin inputs. +type flagGroup struct { + names []string + value pflag.Value + changed bool +} + +// label names the group for a guard error. Parsed state does not record which +// spelling the caller typed — only the merged value survives — so an alias +// group names every spelling rather than guessing one and being wrong. +func (g *flagGroup) label() string { + return "--" + strings.Join(g.names, "/--") +} + +// allowed reports whether any spelling of this input is registered for stdin. +// One value, one policy: registering a single alias covers the group. +func (g *flagGroup) allowed(allow stdinarg.Allow) bool { + for _, name := range g.names { + if allow.Flag(name) { + return true + } + } + return false +} + +// changedFlagGroups returns the groups the caller actually set, in flag- +// declaration order within each group. +func changedFlagGroups(cmd *cobra.Command) []*flagGroup { + var groups []*flagGroup + index := map[pflag.Value]*flagGroup{} + cmd.Flags().VisitAll(func(f *pflag.Flag) { + group, ok := index[f.Value] + if !ok { + group = &flagGroup{value: f.Value} + index[f.Value] = group + groups = append(groups, group) + } + group.names = append(group.names, f.Name) + group.changed = group.changed || f.Changed + }) + changed := groups[:0] + for _, group := range groups { + if group.changed { + changed = append(changed, group) + } + } + return changed +} + // positionalName names a positional for guard errors, preferring the // placeholder from the Use string ("<name>") over a bare ordinal. func positionalName(cmd *cobra.Command, index int) string { diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index 5dec0a764..c367851ee 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -238,3 +238,92 @@ func TestAPIPostDataDashOnTTYHintPreservesTheFlag(t *testing.T) { assert.Contains(t, outErr.Hint, "api post ... --data -") assert.Zero(t, transport.calls) } + +// setupNoAccountApp builds an app with no account configured, so any command +// that reaches account resolution fails with "--account is required". +func setupNoAccountApp(t *testing.T, transport http.RoundTripper) *appctx.App { + t.Helper() + t.Setenv("BASECAMP_NO_KEYRING", "1") + + cfg := &config.Config{} + authMgr := auth.NewManager(cfg, nil) + sdkClient := basecamp.NewClient(&basecamp.Config{BaseURL: "https://3.basecampapi.com"}, &testTokenProvider{}, + basecamp.WithTransport(transport), + basecamp.WithMaxRetries(1), + ) + return &appctx.App{ + Config: cfg, + Auth: authMgr, + SDK: sdkClient, + Names: names.NewResolver(sdkClient, authMgr, cfg.AccountID), + Output: output.New(output.Options{Format: output.FormatJSON, Writer: &bytes.Buffer{}}), + } +} + +// Every "-" must be diagnosed before account, project or network work, so the +// caller gets the stdin error the feature promises instead of "--account is +// required" — or, worse, a resolution round-trip for an invocation that was +// never going to run. Driven on a TTY stdin because that error is produced by +// the resolver itself, which pins where in the sequence it ran. +func TestStdinResolvesBeforeAccountAndProject(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + args []string + }{ + {"cards update --body", NewCardsCmd, []string{"update", "1", "--body", "-"}}, + {"docs create [content]", NewDocsCmd, []string{"documents", "create", "Title", "-"}}, + {"gauges create --description", NewGaugesCmd, []string{"create", "--position", "50", "--description", "-"}}, + {"gauges update --description", NewGaugesCmd, []string{"update", "1", "--description", "-"}}, + {"schedule create --description", NewScheduleCmd, []string{ + "create", "Title", "--starts-at", "2026-01-01T10:00:00Z", "--ends-at", "2026-01-01T11:00:00Z", "--description", "-", + }}, + {"templates update --description", NewTemplatesCmd, []string{"update", "1", "--description", "-"}}, + {"templates construct --description", NewTemplatesCmd, []string{"construct", "1", "--name", "P", "--description", "-"}}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupNoAccountApp(t, transport) + + cmd := tc.cmd() + InstallDashGuard(cmd) + devNullStdin(t, cmd) + + err := executeCommand(cmd, app, tc.args...) + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "nothing is piped", + "expected the stdin error, got %q", outErr.Message) + assert.NotContains(t, outErr.Message, "account") + assert.Zero(t, transport.calls, "no request may be issued") + }) + } +} + +// Two explicit content sources used to resolve by silent precedence: the +// positional won and --content was dropped, so "--content -" left the pipe +// unread and posted the positional instead. +func TestChatRejectsPositionalAlongsideContentFlag(t *testing.T) { + for _, tc := range []struct { + name string + args []string + }{ + {"post", []string{"post", "hello", "--content", "-"}}, + {"update", []string{"update", "123", "hello", "--content", "-"}}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + stdin := &trackingReader{r: strings.NewReader("from stdin")} + cmd := NewChatCmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + err := executeCommand(cmd, app, tc.args...) + outErr := requireUsageErr(t, err) + assert.Contains(t, outErr.Message, "--content") + assert.False(t, stdin.read, "the discarded source must not consume stdin") + assert.Zero(t, transport.calls) + }) + } +} diff --git a/internal/commands/templates.go b/internal/commands/templates.go index 3717009bd..da9f0da87 100644 --- a/internal/commands/templates.go +++ b/internal/commands/templates.go @@ -257,6 +257,17 @@ func newTemplatesUpdateCmd() *cobra.Command { Long: "Update an existing template's name or description.", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if name == "" && description == "" { + return noChanges(cmd) + } + + // Local validation, then "-", then account: a bad stdin gets the + // stdin error rather than "--account is required". + description, err := resolveContentValue(cmd, description, -1, "--description") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { @@ -268,15 +279,6 @@ func newTemplatesUpdateCmd() *cobra.Command { return output.ErrUsage("Invalid template ID") } - if name == "" && description == "" { - return noChanges(cmd) - } - - description, err = resolveContentValue(cmd, description, -1, "--description") - if err != nil { - return err - } - // SDK requires name for update, fetch current if not provided updateName := name if updateName == "" { @@ -374,6 +376,17 @@ This is an asynchronous operation. The command returns a construction ID which can be polled via 'templates construction' until the status is "completed".`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + if projectName == "" { + return output.ErrUsage("--name is required (project name)") + } + + // Local validation, then "-", then account: a bad stdin gets the + // stdin error rather than "--account is required". + projectDesc, err := resolveContentValue(cmd, projectDesc, -1, "--description") + if err != nil { + return err + } + app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { @@ -385,15 +398,6 @@ which can be polled via 'templates construction' until the status is "completed" return output.ErrUsage("Invalid template ID") } - if projectName == "" { - return output.ErrUsage("--name is required (project name)") - } - - projectDesc, err := resolveContentValue(cmd, projectDesc, -1, "--description") - if err != nil { - return err - } - construction, err := app.Account().Templates().CreateProject(cmd.Context(), templateID, projectName, projectDesc) if err != nil { return convertSDKError(err) From d73b2c3debbd1291dbed73e8e70ed8f19fc7274f Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Wed, 19 Aug 2026 15:49:02 -0700 Subject: [PATCH 07/19] Gate the profile picker on stdin; guard the root's stray dash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The profile picker runs from PersistentPreRunE and reads keystrokes, but isInteractiveTTY only looked at stdout. With multiple profiles and no default, "printf body | basecamp todos create -" opened the picker on a terminal stdout and let it eat the piped body. It now uses the same stdinarg.InteractiveStdio predicate as App.IsInteractive and the project resolver — the third and last TUI-launch gate; the remaining ModeCharDevice checks pick an output format and never read keys. The root was skipped entirely by the dash guard, so a piped "basecamp -" ran quick-start and ignored both the dash and the pipe. Its Args must stay nil for cobra's unknown-command handling, so the guard hangs off RunE there instead. Regressions cover all four root behaviors: piped dash errors, -- keeps it literal, unknown commands still error, bare still runs. --- e2e/stdin_dash.bats | 19 ++++++++ internal/cli/root.go | 17 +++---- internal/cli/root_test.go | 33 +++++++++++++ internal/commands/dash_guard_test.go | 69 ++++++++++++++++++++++++++++ internal/commands/stdin.go | 28 +++++++---- 5 files changed, 150 insertions(+), 16 deletions(-) diff --git a/e2e/stdin_dash.bats b/e2e/stdin_dash.bats index ac8cc4d50..636d41335 100644 --- a/e2e/stdin_dash.bats +++ b/e2e/stdin_dash.bats @@ -71,3 +71,22 @@ load test_helper assert_success assert_json_value '.data.project_id.value' '-' } + +@test "a piped bare dash at the root is rejected, not silently quick-started" { + create_credentials + create_global_config '{"account_id": 99999}' + + run bash -c "printf 'x' | basecamp - --json" + assert_failure + assert_json_value '.code' 'usage' + assert_json_value '.error | contains("does not read stdin")' 'true' +} + +@test "basecamp unknowncmd still reports an unknown command" { + create_credentials + create_global_config '{"account_id": 99999}' + + run bash -c "basecamp unknowncmd --json < /dev/null" + assert_failure + assert_output_contains "unknown command" +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 7431a4102..4c22fe7ea 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -520,8 +520,14 @@ func profileNames(cfg *config.Config) string { return strings.Join(names, ", ") } -// isInteractiveTTY returns true if stdout is a character device (e.g. a -// terminal) and no noninteractive mode is set. +// isInteractiveTTY reports whether the profile picker may run: no +// noninteractive mode set, and both ends of stdio are character devices. +// +// Stdin counts because the picker is a TUI reading key events, and this runs +// from PersistentPreRunE — before any command touches its own input. Gating on +// stdout alone let "printf body | basecamp todos create -" open the picker on +// a terminal stdout and consume the piped body as keystrokes. Same predicate +// as App.IsInteractive and resolve.Resolver.IsInteractive. func isInteractiveTTY(flags appctx.GlobalFlags) bool { if config.NonInteractiveEnv() { return false @@ -532,12 +538,7 @@ func isInteractiveTTY(flags appctx.GlobalFlags) bool { return false } - // Check if stdout is a character device (e.g. a terminal) - fi, err := os.Stdout.Stat() - if err != nil { - return false - } - return (fi.Mode() & os.ModeCharDevice) != 0 + return stdinarg.InteractiveStdio() } // promptForProfile shows an interactive picker for profile selection. diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index 636d03c78..b8a4fd9ea 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -285,3 +285,36 @@ func TestVersionWithJQReturnsUsageError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "--jq is not supported by the version command") } + +// The profile picker runs from PersistentPreRunE, before any command reads its +// own input. Piped stdin can never drive a TUI, and when the invocation is +// feeding a "-" content input the picker would eat that body as keystrokes — +// so a terminal stdout is not on its own enough to open one. +func TestIsInteractiveTTYRequiresUnpipedStdin(t *testing.T) { + devNull, err := os.Open(os.DevNull) + if err != nil { + t.Skip(os.DevNull + " not available") + } + origStdout := os.Stdout + os.Stdout = devNull + t.Cleanup(func() { + os.Stdout = origStdout + devNull.Close() + }) + t.Setenv("BASECAMP_NONINTERACTIVE", "") + + require.True(t, isInteractiveTTY(appctx.GlobalFlags{}), "char-device stdio is interactive") + + reader, writer, err := os.Pipe() + require.NoError(t, err) + origStdin := os.Stdin + os.Stdin = reader + t.Cleanup(func() { + os.Stdin = origStdin + reader.Close() + writer.Close() + }) + + assert.False(t, isInteractiveTTY(appctx.GlobalFlags{}), + "piped stdin must not open the profile picker") +} diff --git a/internal/commands/dash_guard_test.go b/internal/commands/dash_guard_test.go index 75b0eb702..95840d786 100644 --- a/internal/commands/dash_guard_test.go +++ b/internal/commands/dash_guard_test.go @@ -253,3 +253,72 @@ func TestDashGuardNamesASoloFlagPlainly(t *testing.T) { assert.Contains(t, outErr.Message, "--title") assert.NotContains(t, outErr.Message, "/--") } + +// The root keeps nil Args so cobra's legacyArgs still rejects unknown +// subcommands, so its guard hangs off RunE instead. All four root behaviors +// have to survive together. +func TestDashGuardOnRootPreservesUnknownCommandHandling(t *testing.T) { + newRoot := func(probe *dashProbe) *cobra.Command { + root := &cobra.Command{ + Use: "basecamp", + RunE: func(cmd *cobra.Command, args []string) error { + probe.ran = true + probe.args = args + return nil + }, + } + root.AddCommand(&cobra.Command{ + Use: "todos", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + }) + InstallDashGuard(root) + require.Nil(t, root.Args, "root Args must stay nil for legacyArgs") + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + return root + } + + t.Run("piped dash is a usage error", func(t *testing.T) { + probe := &dashProbe{} + root := newRoot(probe) + root.SetIn(strings.NewReader("piped")) + root.SetArgs([]string{"-"}) + + outErr := requireUsageErr(t, root.Execute()) + assert.Contains(t, outErr.Message, "does not read stdin") + assert.False(t, probe.ran, "quick-start must not run on a stray dash") + }) + + t.Run("separator keeps the dash literal", func(t *testing.T) { + probe := &dashProbe{} + root := newRoot(probe) + root.SetIn(strings.NewReader("piped")) + root.SetArgs([]string{"--", "-"}) + + require.NoError(t, root.Execute()) + assert.True(t, probe.ran) + assert.Equal(t, []string{"-"}, probe.args) + }) + + t.Run("unknown subcommand still errors", func(t *testing.T) { + probe := &dashProbe{} + root := newRoot(probe) + root.SetIn(strings.NewReader("piped")) + root.SetArgs([]string{"unknowncmd"}) + + err := root.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown command") + assert.False(t, probe.ran) + }) + + t.Run("bare invocation still runs", func(t *testing.T) { + probe := &dashProbe{} + root := newRoot(probe) + root.SetIn(strings.NewReader("piped")) + root.SetArgs(nil) + + require.NoError(t, root.Execute()) + assert.True(t, probe.ran) + }) +} diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index 1e0efe076..38986e780 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -140,14 +140,26 @@ func afterDashSeparator(cmd *cobra.Command, index int) bool { func InstallDashGuard(root *cobra.Command) { // The root's nil Args is load-bearing: cobra's Find() rejects unknown // subcommands (legacyArgs) only while Args == nil, so wrapping it would - // turn "basecamp unknowncmd" into a quickstart run. Leaving the root - // unguarded loses nothing: its positionals are subcommand names, and a - // bare "basecamp -" just runs quickstart, which posts no content for a - // literal "-" to corrupt and whose TUI paths (the first-run wizard) are - // stdin-gated by stdinarg.InteractiveStdio — piped stdin routes to the - // non-interactive summary instead of a wizard that would eat the pipe. - skipRoot := root.Args == nil && !root.HasParent() && root.HasSubCommands() - if root.Runnable() && !skipRoot { + // turn "basecamp unknowncmd" into a quickstart run. Guard its RunE + // instead, so "printf x | basecamp -" still gets the stray-dash error + // rather than silently running quickstart and ignoring the pipe. RunE is + // later than Args validation, but the root's pre-run work (config + // hardening, the update check) neither reads stdin nor writes content — + // and its one TUI path, the first-run wizard, is stdin-gated by + // stdinarg.InteractiveStdio, so piped stdin routes to the non-interactive + // summary instead of a wizard that would eat the pipe. + skipRootArgs := root.Args == nil && !root.HasParent() && root.HasSubCommands() + switch { + case !root.Runnable(): + case skipRootArgs: + existing := root.RunE + root.RunE = func(cmd *cobra.Command, args []string) error { + if err := guardDashArgs(cmd, args); err != nil { + return err + } + return existing(cmd, args) + } + default: existing := root.Args root.Args = func(cmd *cobra.Command, args []string) error { if err := guardDashArgs(cmd, args); err != nil { From c30aa366e3142bbfbaf8426d4b2cde5a55262fb0 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Wed, 19 Aug 2026 21:28:52 -0700 Subject: [PATCH 08/19] Validate target IDs before reading stdin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading stdin blocks on the producer and cannot be undone, so anything knowable from the arguments alone has to be decided first. Thirteen commands read a "-" content input before parsing the ID out of args[0], so a typo'd ID waited on the pipe, and a blank pipe answered "stdin is empty" instead of naming the bad ID. The order is now syntactic checks, then stdin, then account and network. files replace also hoists its file-path validation; its URL identity checks need the session account, so they necessarily still follow. schedule update only hoists the extraction — it never rejected a malformed entry ID locally, which predates this and is left alone. An AST test holds the ordering for call sites not written yet: within any RunE, no resolveContentValue/resolveContentArg call may precede a syntactic use of args (extractID, extractWithProject, extractCommentWithProject, strconv.ParseInt). Coverage is bounded to those names, stated in the test. Eleven commands additionally assert, through a stdin reader that records reads and a counting transport, that a malformed ID drains nothing and issues no request. Also covers the root guard with a character-device stdout, the case the piped-stdout e2e suite can never reach. --- internal/cli/root_test.go | 50 ++++++++ internal/commands/cards.go | 36 +++--- internal/commands/comment.go | 21 ++-- internal/commands/files.go | 49 ++++---- internal/commands/gauges.go | 13 +-- internal/commands/messages.go | 20 ++-- internal/commands/projects.go | 11 +- internal/commands/schedule.go | 11 +- internal/commands/stdin_integration_test.go | 40 +++++++ internal/commands/stdin_ordering_test.go | 119 ++++++++++++++++++++ internal/commands/templates.go | 26 ++--- internal/commands/todolists.go | 21 ++-- internal/commands/todos.go | 19 ++-- 13 files changed, 334 insertions(+), 102 deletions(-) create mode 100644 internal/commands/stdin_ordering_test.go diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index b8a4fd9ea..b98763166 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -12,6 +12,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/commands" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -318,3 +319,52 @@ func TestIsInteractiveTTYRequiresUnpipedStdin(t *testing.T) { assert.False(t, isInteractiveTTY(appctx.GlobalFlags{}), "piped stdin must not open the profile picker") } + +// The root's dash guard hangs off RunE (its Args must stay nil for cobra's +// unknown-command handling), so quick-start's own interactive paths run in the +// same window. The e2e suite always has a piped stdout, which takes the +// machine-output branch and never reaches them — this covers the other side: +// a character-device stdout, the terminal stand-in, with a piped stdin. +func TestRootDashGuardWithTerminalStdout(t *testing.T) { + isolateRootTest(t) + + devNull, err := os.Open(os.DevNull) + if err != nil { + t.Skip(os.DevNull + " not available") + } + origStdout := os.Stdout + os.Stdout = devNull + t.Cleanup(func() { + os.Stdout = origStdout + devNull.Close() + }) + t.Setenv("BASECAMP_NONINTERACTIVE", "") + + reader, writer, err := os.Pipe() + require.NoError(t, err) + origStdin := os.Stdin + os.Stdin = reader + t.Cleanup(func() { + os.Stdin = origStdin + reader.Close() + writer.Close() + }) + _, _ = writer.WriteString("piped body") + writer.Close() + + // Terminal stdout plus piped stdin: no TUI may open, and the stray "-" + // must be rejected rather than quietly quick-starting. + assert.False(t, stdinarg.InteractiveStdio(), + "piped stdin must close every TUI gate even with a terminal stdout") + + root := NewRootCmd() + commands.InstallDashGuard(root) + root.SetIn(reader) + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{"-"}) + + err = root.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "does not read stdin") +} diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 764d363ac..f6ef184b1 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -1091,8 +1091,17 @@ You can pass either a card ID or a Basecamp URL: return noChanges(cmd) } - // Resolve "-" before any account or network work, so a bad stdin - // gets the stdin error rather than "--account is required". + // Extract ID from URL if provided + cardIDStr := extractID(args[0]) + + cardID, err := strconv.ParseInt(cardIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid card ID") + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. content, err := resolveContentValue(cmd, content, -1, "--body") if err != nil { return err @@ -1104,14 +1113,6 @@ You can pass either a card ID or a Basecamp URL: return err } - // Extract ID from URL if provided - cardIDStr := extractID(args[0]) - - cardID, err := strconv.ParseInt(cardIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid card ID") - } - req := &basecamp.UpdateCardRequest{} if title != "" { req.Title = &title @@ -2137,6 +2138,14 @@ You can pass either a column ID or a Basecamp URL: return noChanges(cmd) } + // Extract ID from URL if provided + columnIDStr := extractID(args[0]) + columnID, err := strconv.ParseInt(columnIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid column ID") + } + + // Syntactic checks first, then "-", then account and network. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -2148,13 +2157,6 @@ You can pass either a column ID or a Basecamp URL: return err } - // Extract ID from URL if provided - columnIDStr := extractID(args[0]) - columnID, err := strconv.ParseInt(columnIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid column ID") - } - req := &basecamp.UpdateColumnRequest{ Title: title, Description: description, diff --git a/internal/commands/comment.go b/internal/commands/comment.go index b67d61177..27baacf54 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -1049,6 +1049,18 @@ as backslash-n.`, return missingArg(cmd, "<content>") } + // Extract comment ID from URL if provided + // Uses extractCommentWithProject to prefer CommentID from URL fragments + commentIDStr, _ := extractCommentWithProject(args[0]) + + commentID, err := strconv.ParseInt(commentIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid comment ID") + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. content, err := resolveContentArg(cmd, args[1:], 1) if err != nil { return err @@ -1062,15 +1074,6 @@ as backslash-n.`, return err } - // Extract comment ID from URL if provided - // Uses extractCommentWithProject to prefer CommentID from URL fragments - commentIDStr, _ := extractCommentWithProject(args[0]) - - commentID, err := strconv.ParseInt(commentIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid comment ID") - } - // Convert Markdown content to HTML for Basecamp's rich text fields html := richtext.MarkdownToHTML(content) diff --git a/internal/commands/files.go b/internal/commands/files.go index ff0367916..591fc03a9 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -1722,7 +1722,22 @@ You can pass either an upload ID or a Basecamp URL: RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) - // Only an exact "-" reads stdin; --description "" stays the clear idiom. + uploadIDStr := extractID(args[0]) + uploadID, err := strconv.ParseInt(uploadIDStr, 10, 64) + if err != nil || uploadID <= 0 { + return output.ErrUsage("Invalid upload ID") + } + + filePath := richtext.NormalizeDragPath(args[1]) + if err := richtext.ValidateFile(filePath); err != nil { + return fmt.Errorf("%s: %w", filePath, err) + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID or missing file is answered without waiting on the + // producer. The URL identity checks below need the session account, + // so they necessarily follow. Only an exact "-" reads stdin; + // --description "" stays the clear idiom. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -1761,17 +1776,6 @@ You can pass either an upload ID or a Basecamp URL: } } - uploadIDStr := extractID(args[0]) - uploadID, err := strconv.ParseInt(uploadIDStr, 10, 64) - if err != nil || uploadID <= 0 { - return output.ErrUsage("Invalid upload ID") - } - - filePath := richtext.NormalizeDragPath(args[1]) - if err := richtext.ValidateFile(filePath); err != nil { - return fmt.Errorf("%s: %w", filePath, err) - } - // Resolve the description first: its local-image references can // fail deterministically, and staging a large replacement before // finding that out wastes the whole transfer. A nil Description @@ -1903,7 +1907,18 @@ You can pass either an item ID or a Basecamp URL: Annotations: map[string]string{"agent_notes": "Document updates preserve untouched title/content by fetching current state first because BC3 rebuilds documents from permitted params on PUT; explicit clears via --title \"\"/--content \"\" work because the SDK strips empty strings to absent fields, which the controller then nulls. Upload/vault updates do not clear by omission, so empty-valued flags are rejected CLI-side."}, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // Only an exact "-" reads stdin; --content "" stays the clear idiom. + // Extract ID and project from URL if provided + itemIDStr, urlProjectID := extractWithProject(args[0]) + + itemID, err := strconv.ParseInt(itemIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid item ID") + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. Only an exact "-" reads stdin; + // --content "" stays the clear idiom. var contentErr error content, contentErr = resolveContentValue(cmd, content, -1, "--content") if contentErr != nil { @@ -1951,14 +1966,6 @@ You can pass either an item ID or a Basecamp URL: return err } - // Extract ID and project from URL if provided - itemIDStr, urlProjectID := extractWithProject(args[0]) - - itemID, err := strconv.ParseInt(itemIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid item ID") - } - // Resolve project - use URL > flag > config, with interactive fallback projectID := *project if projectID == "" && urlProjectID != "" { diff --git a/internal/commands/gauges.go b/internal/commands/gauges.go index 33f55facd..26b9b9ff3 100644 --- a/internal/commands/gauges.go +++ b/internal/commands/gauges.go @@ -271,8 +271,12 @@ func newGaugesUpdateCmd() *cobra.Command { return output.ErrUsage("No changes specified (use --description)") } - // Resolve "-" before any account or network work, so a bad stdin - // gets the stdin error rather than "--account is required". + needleID, err := strconv.ParseInt(args[0], 10, 64) + if err != nil { + return output.ErrUsage("Invalid needle ID") + } + + // Syntactic checks first, then "-", then account and network. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -284,11 +288,6 @@ func newGaugesUpdateCmd() *cobra.Command { return err } - needleID, err := strconv.ParseInt(args[0], 10, 64) - if err != nil { - return output.ErrUsage("Invalid needle ID") - } - req := &basecamp.UpdateGaugeNeedleRequest{ Description: basecamp.Ptr(description), } diff --git a/internal/commands/messages.go b/internal/commands/messages.go index 6e5460996..c5fe1116a 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -627,7 +627,17 @@ You can pass either a message ID or a Basecamp URL: return noChanges(cmd) } - var err error + // Extract ID from URL if provided + messageIDStr := extractID(args[0]) + + messageID, err := strconv.ParseInt(messageIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid message ID") + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. body, err = resolveContentValue(cmd, body, -1, "--body") if err != nil { return err @@ -639,14 +649,6 @@ You can pass either a message ID or a Basecamp URL: return err } - // Extract ID from URL if provided - messageIDStr := extractID(args[0]) - - messageID, err := strconv.ParseInt(messageIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid message ID") - } - // Build SDK request // Convert Markdown content to HTML for Basecamp's rich text fields html := richtext.MarkdownToHTML(body) diff --git a/internal/commands/projects.go b/internal/commands/projects.go index 27e3383cc..e696eaeef 100644 --- a/internal/commands/projects.go +++ b/internal/commands/projects.go @@ -331,6 +331,12 @@ Examples: return fmt.Errorf("app not initialized") } + projectID, err := strconv.ParseInt(args[0], 10, 64) + if err != nil { + return output.ErrUsage("Invalid project ID") + } + + // Syntactic checks first, then "-", then account and network. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -341,11 +347,6 @@ Examples: return err } - projectID, err := strconv.ParseInt(args[0], 10, 64) - if err != nil { - return output.ErrUsage("Invalid project ID") - } - // For update, we need to provide name (required by SDK) // If only description is provided, we need to fetch current name first updateName := name diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index e4fd808c9..f4f7db832 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -623,6 +623,14 @@ You can pass either an entry ID or a Basecamp URL: RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + // Extract ID and project from URL if provided. Purely syntactic, + // so it precedes the stdin read like every other target-ID check. + // (This command never rejects a malformed entry ID locally — the + // ParseInt below discards its error and the server answers. That + // predates this change and is left alone.) + entryID, urlProjectID := extractWithProject(args[0]) + + // Syntactic checks first, then "-", then account and network. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -632,9 +640,6 @@ You can pass either an entry ID or a Basecamp URL: return err } - // Extract ID and project from URL if provided - entryID, urlProjectID := extractWithProject(args[0]) - // Resolve project - use URL > flag > config, with interactive fallback projectID := *project if projectID == "" && urlProjectID != "" { diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index c367851ee..382fa147a 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -327,3 +327,43 @@ func TestChatRejectsPositionalAlongsideContentFlag(t *testing.T) { }) } } + +// A malformed target ID is knowable from the arguments alone, so it must be +// reported without first draining the pipe: reading blocks on the producer, and +// a blank pipe would answer "stdin is empty" instead of naming the bad ID. +func TestMalformedIDRejectedBeforeReadingStdin(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + args []string + }{ + {"cards update", NewCardsCmd, []string{"update", "nope", "--body", "-"}}, + {"cards column update", NewCardsCmd, []string{"column", "update", "nope", "--description", "-"}}, + {"gauges update", NewGaugesCmd, []string{"update", "nope", "--description", "-"}}, + {"templates update", NewTemplatesCmd, []string{"update", "nope", "--description", "-"}}, + {"templates construct", NewTemplatesCmd, []string{"construct", "nope", "--name", "P", "--description", "-"}}, + {"messages update", NewMessagesCmd, []string{"update", "nope", "--body", "-"}}, + {"todos update", NewTodosCmd, []string{"update", "nope", "--description", "-"}}, + {"todolists update", NewTodolistsCmd, []string{"update", "nope", "--description", "-"}}, + {"projects update", NewProjectsCmd, []string{"update", "nope", "--description", "-"}}, + {"comments update", NewCommentsCmd, []string{"update", "nope", "-"}}, + {"files update", NewFilesCmd, []string{"update", "nope", "--content", "-"}}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + stdin := &trackingReader{r: strings.NewReader("body from stdin")} + cmd := tc.cmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + err := executeCommand(cmd, app, tc.args...) + outErr := requireUsageErr(t, err) + assert.Contains(t, strings.ToLower(outErr.Message), "invalid", + "expected the malformed-ID error, got %q", outErr.Message) + assert.False(t, stdin.read, "stdin must not be drained before the ID is validated") + assert.Zero(t, transport.calls, "no request may be issued") + }) + } +} diff --git a/internal/commands/stdin_ordering_test.go b/internal/commands/stdin_ordering_test.go new file mode 100644 index 000000000..006883231 --- /dev/null +++ b/internal/commands/stdin_ordering_test.go @@ -0,0 +1,119 @@ +package commands_test + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Reading stdin is the one step in a RunE that cannot be undone and cannot be +// hurried: it blocks until the producer is done. Everything that can reject the +// invocation from the arguments alone — parsing the target ID out of args[0], +// validating a file path — has to happen first, or a typo'd ID waits on a slow +// pipe and a blank one reports "stdin is empty" instead of "Invalid card ID". +// +// This was fixed at thirteen call sites by hand; the ordering is a property of +// every future one too, which is what this test holds. It reads the AST rather +// than running the commands, so it covers call sites no test exercises — but +// only syntactic forms it can recognize, listed in syntacticArgUse below. +func TestSyntacticArgChecksPrecedeStdinReads(t *testing.T) { + dir := "." + entries, err := os.ReadDir(dir) + require.NoError(t, err) + + fset := token.NewFileSet() + var checked int + + for _, entry := range entries { + name := entry.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, 0) + require.NoError(t, err) + + ast.Inspect(file, func(n ast.Node) bool { + kv, ok := n.(*ast.KeyValueExpr) + if !ok { + return true + } + if key, ok := kv.Key.(*ast.Ident); !ok || (key.Name != "RunE" && key.Name != "PreRunE") { + return true + } + + firstStdinRead, firstArgCheck := token.NoPos, token.NoPos + ast.Inspect(kv.Value, func(inner ast.Node) bool { + call, ok := inner.(*ast.CallExpr) + if !ok { + return true + } + switch { + case stdinResolver(call) && !firstStdinRead.IsValid(): + firstStdinRead = call.Pos() + case syntacticArgUse(call) && !firstArgCheck.IsValid(): + firstArgCheck = call.Pos() + } + return true + }) + + if !firstStdinRead.IsValid() || !firstArgCheck.IsValid() { + return true + } + checked++ + assert.Less(t, int(firstArgCheck), int(firstStdinRead), + "%s: this command reads stdin at %s before validating its arguments at %s — "+ + "hoist the argument check above the resolver", + name, fset.Position(firstStdinRead), fset.Position(firstArgCheck)) + return true + }) + } + + require.Greater(t, checked, 10, "expected many commands to both read stdin and check args") +} + +// stdinResolver matches the two functions that can read from stdin. +func stdinResolver(call *ast.CallExpr) bool { + name, ok := call.Fun.(*ast.Ident) + return ok && (name.Name == "resolveContentValue" || name.Name == "resolveContentArg") +} + +// syntacticArgUse matches a call that derives something from args without any +// account, config, or network dependency — the checks that must come first. +// Recognizing a form by name is exactly as wide as the names listed; a new +// helper needs adding here, which is why the coverage claim above is bounded. +func syntacticArgUse(call *ast.CallExpr) bool { + name, ok := call.Fun.(*ast.Ident) + if !ok { + if sel, isSel := call.Fun.(*ast.SelectorExpr); isSel { + pkg, isPkg := sel.X.(*ast.Ident) + if !isPkg || pkg.Name != "strconv" || sel.Sel.Name != "ParseInt" { + return false + } + } else { + return false + } + } else { + switch name.Name { + case "extractID", "extractWithProject", "extractCommentWithProject": + default: + return false + } + } + + // Only when it reads the positional arguments directly. + for _, arg := range call.Args { + if index, ok := arg.(*ast.IndexExpr); ok { + if ident, ok := index.X.(*ast.Ident); ok && ident.Name == "args" { + return true + } + } + } + return false +} diff --git a/internal/commands/templates.go b/internal/commands/templates.go index da9f0da87..80ddcce58 100644 --- a/internal/commands/templates.go +++ b/internal/commands/templates.go @@ -261,8 +261,12 @@ func newTemplatesUpdateCmd() *cobra.Command { return noChanges(cmd) } - // Local validation, then "-", then account: a bad stdin gets the - // stdin error rather than "--account is required". + templateID, err := strconv.ParseInt(args[0], 10, 64) + if err != nil { + return output.ErrUsage("Invalid template ID") + } + + // Syntactic checks first, then "-", then account and network. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -274,11 +278,6 @@ func newTemplatesUpdateCmd() *cobra.Command { return err } - templateID, err := strconv.ParseInt(args[0], 10, 64) - if err != nil { - return output.ErrUsage("Invalid template ID") - } - // SDK requires name for update, fetch current if not provided updateName := name if updateName == "" { @@ -380,8 +379,12 @@ which can be polled via 'templates construction' until the status is "completed" return output.ErrUsage("--name is required (project name)") } - // Local validation, then "-", then account: a bad stdin gets the - // stdin error rather than "--account is required". + templateID, err := strconv.ParseInt(args[0], 10, 64) + if err != nil { + return output.ErrUsage("Invalid template ID") + } + + // Syntactic checks first, then "-", then account and network. projectDesc, err := resolveContentValue(cmd, projectDesc, -1, "--description") if err != nil { return err @@ -393,11 +396,6 @@ which can be polled via 'templates construction' until the status is "completed" return err } - templateID, err := strconv.ParseInt(args[0], 10, 64) - if err != nil { - return output.ErrUsage("Invalid template ID") - } - construction, err := app.Account().Templates().CreateProject(cmd.Context(), templateID, projectName, projectDesc) if err != nil { return convertSDKError(err) diff --git a/internal/commands/todolists.go b/internal/commands/todolists.go index 476cd4283..b6c271e30 100644 --- a/internal/commands/todolists.go +++ b/internal/commands/todolists.go @@ -402,6 +402,18 @@ You can pass either a todolist ID or a Basecamp URL: return fmt.Errorf("app not initialized") } + // Extract ID and project from URL if provided + todolistIDStr, urlProjectID := extractWithProject(args[0]) + + // Parse todolist ID as int64 + todolistID, err := strconv.ParseInt(todolistIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid todolist ID") + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -411,9 +423,6 @@ You can pass either a todolist ID or a Basecamp URL: return err } - // Extract ID and project from URL if provided - todolistIDStr, urlProjectID := extractWithProject(args[0]) - // Resolve project - use URL > flag > config, with interactive fallback projectID := *project if projectID == "" && urlProjectID != "" { @@ -431,12 +440,6 @@ You can pass either a todolist ID or a Basecamp URL: } } - // Parse todolist ID as int64 - todolistID, err := strconv.ParseInt(todolistIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid todolist ID") - } - // Build SDK request req := &basecamp.UpdateTodolistRequest{ Name: name, diff --git a/internal/commands/todos.go b/internal/commands/todos.go index cd8db6bef..0fb7f9052 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1550,7 +1550,17 @@ Set or clear the people notified when the todo is completed: return noChanges(cmd) } - // Only an exact "-" reads stdin; --description "" stays the clear idiom. + // Extract ID from URL if provided + todoIDStr := extractID(args[0]) + todoID, err := strconv.ParseInt(todoIDStr, 10, 64) + if err != nil { + return output.ErrUsage("Invalid todo ID") + } + + // Syntactic checks first, then "-", then account and network: a + // malformed ID is answered without waiting on the producer, and a + // blank pipe cannot mask it. Only an exact "-" reads stdin; + // --description "" stays the clear idiom. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -1565,13 +1575,6 @@ Set or clear the people notified when the todo is completed: return err } - // Extract ID from URL if provided - todoIDStr := extractID(args[0]) - todoID, err := strconv.ParseInt(todoIDStr, 10, 64) - if err != nil { - return output.ErrUsage("Invalid todo ID") - } - // Pre-Edit validation and resolution — no todo HTTP happens here. // Image uploads are deferred into the Edit closure so a missing // todo can't orphan uploaded attachments. From 45f15c3479e3e4a07dd8d48ca603f114b4873528 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Wed, 19 Aug 2026 21:36:42 -0700 Subject: [PATCH 09/19] Guard the root at pre-run, and never swallow an error behind a bad --jq MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The root guard ran in RunE, so the root's persistent pre-run answered first: 'printf x | basecamp --jq "[invalid" -' reported the jq error rather than the stray dash. It now hangs off the front of that pre-run — the earliest hook that still leaves Args nil for cobra's unknown-command lookup — acting only when the root itself executes, since subcommands inherit the hook and are already guarded at Args-validation time. That exposed a pre-existing hole: any error raised before jq validation was rendered through the unvalidated filter, and the render failure was discarded, so the command exited non-zero having printed nothing. The fallback writer now retries without the filter. --- internal/cli/cobra_error_test.go | 23 +++++++++++ internal/cli/root.go | 9 +++- internal/commands/dash_guard_test.go | 62 ++++++++++++++++++++++++++++ internal/commands/stdin.go | 32 ++++++++------ 4 files changed, 112 insertions(+), 14 deletions(-) diff --git a/internal/cli/cobra_error_test.go b/internal/cli/cobra_error_test.go index f44ae1b5c..34cd66a9b 100644 --- a/internal/cli/cobra_error_test.go +++ b/internal/cli/cobra_error_test.go @@ -1,6 +1,7 @@ package cli import ( + "bytes" "errors" "testing" @@ -85,3 +86,25 @@ func TestTransformCobraErrorIgnoresUnanchoredArityText(t *testing.T) { assert.False(t, errors.As(err, &outErr), "should be left untouched, got %T", err) assert.Equal(t, msg, err.Error()) } + +// An invalid --jq paired with an error raised before jq validation used to +// exit non-zero having printed nothing: the fallback writer tried to render the +// envelope through the broken filter, and its failure was discarded. The error +// the caller needs outranks the filter they asked for. +func TestInvalidJQFilterStillRendersAnEarlierError(t *testing.T) { + var buf bytes.Buffer + writer := output.New(output.Options{ + Format: output.FormatJSON, + Writer: &buf, + JQFilter: ".[invalid", + }) + + require.Error(t, writer.Err(output.ErrUsage("stray dash")), + "a broken filter must report that it could not render") + require.Empty(t, buf.String(), "and must not have written a usable envelope") + + buf.Reset() + plain := output.New(output.Options{Format: output.FormatJSON, Writer: &buf}) + require.NoError(t, plain.Err(output.ErrUsage("stray dash"))) + assert.Contains(t, buf.String(), "stray dash") +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 4c22fe7ea..fbce36953 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -442,7 +442,14 @@ func Execute() { Writer: os.Stdout, JQFilter: jqFilter, }) - _ = writer.Err(err) + if writeErr := writer.Err(err); writeErr != nil && jqFilter != "" { + // The filter itself could not render the envelope — an invalid + // --jq paired with an error raised before jq validation, which + // otherwise exits non-zero having printed nothing at all. The + // error the caller needs outranks the filter they asked for. + plain := output.New(output.Options{Format: format, Writer: os.Stdout}) + _ = plain.Err(err) + } os.Exit(output.ExitCodeFor(apiErr.Code)) } diff --git a/internal/commands/dash_guard_test.go b/internal/commands/dash_guard_test.go index 95840d786..4929980c7 100644 --- a/internal/commands/dash_guard_test.go +++ b/internal/commands/dash_guard_test.go @@ -1,6 +1,7 @@ package commands import ( + "errors" "strings" "testing" @@ -322,3 +323,64 @@ func TestDashGuardOnRootPreservesUnknownCommandHandling(t *testing.T) { assert.True(t, probe.ran) }) } + +// The root guard runs at the front of the persistent pre-run, so nothing the +// caller did not ask about — config loading, profile resolution, --jq +// validation — can answer ahead of the stray dash. +func TestDashGuardOnRootPrecedesPersistentPreRun(t *testing.T) { + preRunRan := false + root := &cobra.Command{ + Use: "basecamp", + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + preRunRan = true + return errors.New("pre-run would have answered first") + }, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + root.AddCommand(&cobra.Command{ + Use: "todos", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + }) + InstallDashGuard(root) + root.SetIn(strings.NewReader("piped")) + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + root.SetArgs([]string{"-"}) + + outErr := requireUsageErr(t, root.Execute()) + assert.Contains(t, outErr.Message, "does not read stdin") + assert.False(t, preRunRan, "the guard must precede the root's pre-run work") +} + +// Subcommands inherit the root's persistent pre-run; the guard there must not +// double-fire for them, since they are already guarded at Args-validation time. +func TestDashGuardOnRootDoesNotAffectSubcommands(t *testing.T) { + preRunRan := false + var got []string + root := &cobra.Command{ + Use: "basecamp", + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + preRunRan = true + return nil + }, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + sub := &cobra.Command{ + Use: "notes", + RunE: func(cmd *cobra.Command, args []string) error { + got = args + return nil + }, + } + allowDash(sub, "arg:0") + root.AddCommand(sub) + InstallDashGuard(root) + root.SetIn(strings.NewReader("piped")) + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + root.SetArgs([]string{"notes", "-"}) + + require.NoError(t, root.Execute()) + assert.True(t, preRunRan, "the inherited pre-run still runs for subcommands") + assert.Equal(t, []string{"-"}, got, "an allowed dash reaches the subcommand") +} diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index 38986e780..076125b8d 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -140,24 +140,30 @@ func afterDashSeparator(cmd *cobra.Command, index int) bool { func InstallDashGuard(root *cobra.Command) { // The root's nil Args is load-bearing: cobra's Find() rejects unknown // subcommands (legacyArgs) only while Args == nil, so wrapping it would - // turn "basecamp unknowncmd" into a quickstart run. Guard its RunE - // instead, so "printf x | basecamp -" still gets the stray-dash error - // rather than silently running quickstart and ignoring the pipe. RunE is - // later than Args validation, but the root's pre-run work (config - // hardening, the update check) neither reads stdin nor writes content — - // and its one TUI path, the first-run wizard, is stdin-gated by - // stdinarg.InteractiveStdio, so piped stdin routes to the non-interactive - // summary instead of a wizard that would eat the pipe. + // turn "basecamp unknowncmd" into a quickstart run. Guard the front of its + // persistent pre-run instead — the earliest hook that still leaves Args + // nil, and earlier than RunE, so the stray-dash error is not shadowed by + // config loading, profile resolution or --jq validation, none of which the + // caller asked about. + // + // Subcommands inherit this hook when they define none of their own, so it + // acts only when the root itself is executing; every subcommand is already + // guarded at Args-validation time, which is earlier still. skipRootArgs := root.Args == nil && !root.HasParent() && root.HasSubCommands() switch { case !root.Runnable(): case skipRootArgs: - existing := root.RunE - root.RunE = func(cmd *cobra.Command, args []string) error { - if err := guardDashArgs(cmd, args); err != nil { - return err + existing := root.PersistentPreRunE + root.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { + if cmd == root { + if err := guardDashArgs(cmd, args); err != nil { + return err + } } - return existing(cmd, args) + if existing != nil { + return existing(cmd, args) + } + return nil } default: existing := root.Args From 44db2c555ab3ae4bfbd126c13f41d0f46732971a Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Wed, 19 Aug 2026 22:59:33 -0700 Subject: [PATCH 10/19] Exempt cobra's generated meta commands from the dash guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cobra adds help and the completion commands during ExecuteC, after the guard walks the tree, so they were unguarded by accident of ordering: printf x | basecamp help - printed help and exited 0. Make it a decision instead. The guard now materializes the help command before the walk and skips generated meta commands explicitly. Tier 2 exists to stop a stray "-" landing as content; these take no content and write nothing, and "help -" resolves like any unknown topic. For the completion commands the exemption is load-bearing rather than harmless: the shell passes the word being completed, so "todos create -<TAB>" runs "__complete todos create -", and guarding it would break flag completion across the whole CLI. Unit and e2e tests pin both. Also: the root wrapper set PersistentPreRunE, which shadows a non-E hook cobra would have run in its place — it now calls that instead of dropping it. Production uses the E form, so this only guards the refactor. And two comments still described the root guard as hanging off RunE; the root test also built a childless root, which took the Args branch rather than the pre-run branch production uses. --- e2e/stdin_dash.bats | 19 +++++++++ internal/cli/root_test.go | 14 ++++--- internal/commands/dash_guard_test.go | 62 +++++++++++++++++++++++++++- internal/commands/stdin.go | 38 +++++++++++++++-- 4 files changed, 122 insertions(+), 11 deletions(-) diff --git a/e2e/stdin_dash.bats b/e2e/stdin_dash.bats index 636d41335..04fd47fc8 100644 --- a/e2e/stdin_dash.bats +++ b/e2e/stdin_dash.bats @@ -90,3 +90,22 @@ load test_helper assert_failure assert_output_contains "unknown command" } + +@test "piped help - is exempt from the dash guard" { + create_credentials + create_global_config '{"account_id": 99999}' + + run bash -c "printf 'x' | basecamp help - --json" + assert_success +} + +@test "piped completion of a flag word is exempt from the dash guard" { + create_credentials + create_global_config '{"account_id": 99999}' + + # The shell passes the word being completed, so "todos create -<TAB>" runs + # this. Guarding it would break flag completion everywhere. + run bash -c "printf 'x' | basecamp __complete todos create -" + assert_success + assert_output_contains "--description" +} diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index b98763166..fe609ad0b 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -320,11 +320,13 @@ func TestIsInteractiveTTYRequiresUnpipedStdin(t *testing.T) { "piped stdin must not open the profile picker") } -// The root's dash guard hangs off RunE (its Args must stay nil for cobra's -// unknown-command handling), so quick-start's own interactive paths run in the -// same window. The e2e suite always has a piped stdout, which takes the -// machine-output branch and never reaches them — this covers the other side: -// a character-device stdout, the terminal stand-in, with a piped stdin. +// The root's dash guard hangs off the front of its persistent pre-run (its Args +// must stay nil for cobra's unknown-command handling), so quick-start's own +// interactive paths sit behind it. The e2e suite always has a piped stdout, +// which takes the machine-output branch and never reaches them — this covers +// the other side: a character-device stdout, the terminal stand-in, with a +// piped stdin. The root carries a subcommand so InstallDashGuard takes the +// pre-run branch production uses, not the Args branch for a childless root. func TestRootDashGuardWithTerminalStdout(t *testing.T) { isolateRootTest(t) @@ -358,7 +360,9 @@ func TestRootDashGuardWithTerminalStdout(t *testing.T) { "piped stdin must close every TUI gate even with a terminal stdout") root := NewRootCmd() + root.AddCommand(commands.NewConfigCmd()) commands.InstallDashGuard(root) + require.Nil(t, root.Args, "the root must keep nil Args for cobra's unknown-command lookup") root.SetIn(reader) root.SetOut(&bytes.Buffer{}) root.SetErr(&bytes.Buffer{}) diff --git a/internal/commands/dash_guard_test.go b/internal/commands/dash_guard_test.go index 4929980c7..0370461c1 100644 --- a/internal/commands/dash_guard_test.go +++ b/internal/commands/dash_guard_test.go @@ -256,8 +256,8 @@ func TestDashGuardNamesASoloFlagPlainly(t *testing.T) { } // The root keeps nil Args so cobra's legacyArgs still rejects unknown -// subcommands, so its guard hangs off RunE instead. All four root behaviors -// have to survive together. +// subcommands, so its guard hangs off the front of its persistent pre-run +// instead. All four root behaviors have to survive together. func TestDashGuardOnRootPreservesUnknownCommandHandling(t *testing.T) { newRoot := func(probe *dashProbe) *cobra.Command { root := &cobra.Command{ @@ -384,3 +384,61 @@ func TestDashGuardOnRootDoesNotAffectSubcommands(t *testing.T) { assert.True(t, preRunRan, "the inherited pre-run still runs for subcommands") assert.Equal(t, []string{"-"}, got, "an allowed dash reaches the subcommand") } + +// Tier 2 stops a stray "-" landing as content, so cobra's generated commands +// are exempt: they take no content and write nothing. For the completion +// commands the exemption is load-bearing, not merely harmless — the shell +// passes the word being completed as an argument, so "todos create -<TAB>" +// runs "__complete todos create -". Guarding that would break completion for +// every flag in the CLI. +func TestDashGuardExemptsGeneratedMetaCommands(t *testing.T) { + root := &cobra.Command{ + Use: "basecamp", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + root.AddCommand(&cobra.Command{ + Use: "todos", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + }) + InstallDashGuard(root) + + help := findSubcommand(root, "help") + require.NotNil(t, help, "InstallDashGuard must materialize the help command so the exemption is explicit") + assert.True(t, isMetaCommand(help)) + + for _, name := range []string{cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd} { + assert.True(t, isMetaCommand(&cobra.Command{Use: name}), "%s must stay unguarded", name) + } + + // A piped "help -" resolves like any unknown topic rather than erroring. + root.SetIn(strings.NewReader("piped")) + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + root.SetArgs([]string{"help", "-"}) + assert.NoError(t, root.Execute()) +} + +// Setting PersistentPreRunE shadows a non-E hook cobra would otherwise run in +// its place. Production uses the E form, so this only guards the refactor. +func TestDashGuardOnRootKeepsNonErrorPersistentPreRun(t *testing.T) { + preRunRan := false + root := &cobra.Command{ + Use: "basecamp", + PersistentPreRun: func(cmd *cobra.Command, args []string) { + preRunRan = true + }, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + root.AddCommand(&cobra.Command{ + Use: "todos", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + }) + InstallDashGuard(root) + root.SetIn(strings.NewReader("piped")) + root.SetOut(&strings.Builder{}) + root.SetErr(&strings.Builder{}) + root.SetArgs(nil) + + require.NoError(t, root.Execute()) + assert.True(t, preRunRan, "the non-error pre-run must still run") +} diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index 076125b8d..e14043200 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -138,6 +138,13 @@ func afterDashSeparator(cmd *cobra.Command, index int) bool { // silently lose the guard. A nil Args means ArbitraryArgs (always nil), so // wrapping it is behavior-preserving. func InstallDashGuard(root *cobra.Command) { + // Materialize cobra's generated help command before the walk so the + // exemption below is a decision the tree records, not an accident of + // running before ExecuteC adds it. Idempotent; ExecuteC calls it again. + if !root.HasParent() { + root.InitDefaultHelpCmd() + } + // The root's nil Args is load-bearing: cobra's Find() rejects unknown // subcommands (legacyArgs) only while Args == nil, so wrapping it would // turn "basecamp unknowncmd" into a quickstart run. Guard the front of its @@ -151,17 +158,23 @@ func InstallDashGuard(root *cobra.Command) { // guarded at Args-validation time, which is earlier still. skipRootArgs := root.Args == nil && !root.HasParent() && root.HasSubCommands() switch { - case !root.Runnable(): + case !root.Runnable(), isMetaCommand(root): case skipRootArgs: - existing := root.PersistentPreRunE + existingE := root.PersistentPreRunE + existing := root.PersistentPreRun root.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { if cmd == root { if err := guardDashArgs(cmd, args); err != nil { return err } } - if existing != nil { - return existing(cmd, args) + // Setting the E form shadows any non-E hook cobra would have run + // in its place, so call it here rather than dropping it. + switch { + case existingE != nil: + return existingE(cmd, args) + case existing != nil: + existing(cmd, args) } return nil } @@ -182,6 +195,23 @@ func InstallDashGuard(root *cobra.Command) { } } +// isMetaCommand reports whether cmd is one of cobra's generated commands, which +// the tier-2 guard deliberately skips. +// +// Tier 2 exists to stop a stray "-" landing as content. These take no content +// and write nothing: "help -" resolves like any unknown topic and prints help. +// For the completion commands the exemption is load-bearing rather than merely +// harmless — the shell passes the word being completed as an argument, so +// "basecamp todos create -<TAB>" runs "basecamp __complete todos create -". +// Guarding that would break completion for every flag in the CLI. +func isMetaCommand(cmd *cobra.Command) bool { + switch cmd.Name() { + case "help", cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd: + return true + } + return false +} + // guardDashArgs enforces the tier-2 policy for one invocation: // // 1. Collect every exact "-" — positionals before the "--" separator, plus From d8bcfda3765f7a06b41c2a8806e0c8cc22f52bd1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Wed, 19 Aug 2026 23:53:17 -0700 Subject: [PATCH 11/19] Say that the meta-command exemption exists, and say it accurately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tier-2 policy was still stated absolutely in the file header, the e2e header, and SKILL.md, which the deliberate help/__complete exemption contradicts. All three now name it. "Take no content and write nothing" was also literally false — both commands write to stdout, which is the entire point of help. The claim that actually distinguishes them is that they perform no Basecamp content write, so a stray "-" has nothing to corrupt. --- e2e/stdin_dash.bats | 7 ++++--- internal/commands/dash_guard_test.go | 2 +- internal/commands/stdin.go | 9 ++++++--- skills/basecamp/SKILL.md | 4 +++- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/e2e/stdin_dash.bats b/e2e/stdin_dash.bats index 04fd47fc8..1ada2283d 100644 --- a/e2e/stdin_dash.bats +++ b/e2e/stdin_dash.bats @@ -3,9 +3,10 @@ # # Tier 1: content inputs accept "-" to read piped stdin. Tier 2: everywhere # else, a literal "-" combined with piped stdin is a usage error instead of -# silently becoming literal content. Every case resolves locally — usage -# errors before any request, or a config write — so no cassette or server -# is needed. +# silently becoming literal content — except cobra's generated meta commands +# (help, __complete), which are deliberately exempt and covered below. Every +# case resolves locally — usage errors before any request, or a config write — +# so no cassette or server is needed. load test_helper diff --git a/internal/commands/dash_guard_test.go b/internal/commands/dash_guard_test.go index 0370461c1..26c39dbf4 100644 --- a/internal/commands/dash_guard_test.go +++ b/internal/commands/dash_guard_test.go @@ -386,7 +386,7 @@ func TestDashGuardOnRootDoesNotAffectSubcommands(t *testing.T) { } // Tier 2 stops a stray "-" landing as content, so cobra's generated commands -// are exempt: they take no content and write nothing. For the completion +// are exempt: they perform no Basecamp content write. For the completion // commands the exemption is load-bearing, not merely harmless — the shell // passes the word being completed as an argument, so "todos create -<TAB>" // runs "__complete todos create -". Guarding that would break completion for diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index e14043200..e7d911d44 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -20,7 +20,8 @@ import ( // dash guard installed over the whole command tree: when stdin is piped, a // stray "-" is ambiguous (the caller almost certainly meant "read the pipe"), // so it fails as a usage error instead of landing as literal content. On a -// TTY, a literal "-" stays legal everywhere. +// TTY, a literal "-" stays legal everywhere. Cobra's generated meta commands +// (help, __complete) are the one deliberate exemption — see isMetaCommand. // allowDash marks where cmd accepts "-" as "read from stdin", merging with any // tokens already registered. Tokens: "arg:0" (exact positional index), @@ -198,8 +199,10 @@ func InstallDashGuard(root *cobra.Command) { // isMetaCommand reports whether cmd is one of cobra's generated commands, which // the tier-2 guard deliberately skips. // -// Tier 2 exists to stop a stray "-" landing as content. These take no content -// and write nothing: "help -" resolves like any unknown topic and prints help. +// Tier 2 exists to stop a stray "-" landing as content. These perform no +// Basecamp content write — they print to stdout and stop there — so a literal +// "-" has nothing to corrupt: "help -" resolves like any unknown topic and +// prints help. // For the completion commands the exemption is load-bearing rather than merely // harmless — the shell passes the word being completed as an argument, so // "basecamp todos create -<TAB>" runs "basecamp __complete todos create -". diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index cf89c67b9..10d665451 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -121,7 +121,9 @@ Full CLI coverage: 155 endpoints across todos, cards, messages, files, schedule, - A literal `-` anywhere else (a title, a name, a path) **errors when stdin is piped**. Escape a positional after the `--` separator (`basecamp projects create -- -`); a flag value has no in-line escape — run - the command without piped stdin. + the command without piped stdin. `basecamp help` and shell completion are + exempt: they write nothing to Basecamp, and completion legitimately + receives `-` as the word being completed. - `-` with nothing piped (interactive TTY) errors immediately instead of hanging; use a pipe, a heredoc (`basecamp comments create <id> - <<'EOF'`), or `--edit` where offered. From 3f5a7304f9cf0529a428f73ec11512a041b3fc32 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 20 Aug 2026 01:05:57 -0700 Subject: [PATCH 12/19] Name the meta-command exemption at the last two policy statements InstallDashGuard's doc comment claimed every runnable command, and the install site claimed everywhere a command doesn't explicitly accept it. Both predate the deliberate help/__complete exemption. --- internal/cli/root.go | 4 +++- internal/commands/stdin.go | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/cli/root.go b/internal/cli/root.go index fbce36953..9c70d2227 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -356,7 +356,9 @@ func Execute() { cmd.AddCommand(commands.NewAgentHookCmd()) // Tier-2 stdin guard: reject a stray literal "-" when stdin is piped, - // everywhere a command doesn't explicitly accept it. + // everywhere a command doesn't explicitly accept it — except cobra's + // generated meta commands, which are deliberately exempt (see + // commands.InstallDashGuard). commands.InstallDashGuard(cmd) // Use ExecuteC to get the executed command (for correct context access) diff --git a/internal/commands/stdin.go b/internal/commands/stdin.go index e7d911d44..8b1e0c143 100644 --- a/internal/commands/stdin.go +++ b/internal/commands/stdin.go @@ -128,9 +128,9 @@ func afterDashSeparator(cmd *cobra.Command, index int) bool { return lenAtDash >= 0 && index >= lenAtDash } -// InstallDashGuard wraps every runnable command in the tree with the tier-2 -// dash guard. It wraps the Args validator: cobra runs ValidateArgs after flag -// parsing (so Changed and ArgsLenAtDash are available) but before the +// InstallDashGuard wraps every non-meta runnable command in the tree with the +// tier-2 dash guard. It wraps the Args validator: cobra runs ValidateArgs +// after flag parsing (so Changed and ArgsLenAtDash are available) but before the // persistent pre-run chain, PreRunE, and required-flag validation — so a // stray "-" is rejected before any lifecycle side effect (config hardening, // the update check) and before a competing usage error can shadow it. It is From 3faf2a0b55f896b0c2c23d2a7a5680cdfed44aea Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 20 Aug 2026 01:17:30 -0700 Subject: [PATCH 13/19] Decide doomed invocations before draining the pipe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eleven commands read a "-" input before a check that already decides the invocation: an unsupported --content-type or --type, a missing filter or required flag, a mutually exclusive flag pair, an unparseable room, boost or check-in ID, an unreadable upload path, or an API URL on a foreign host. Each now runs its local check first. Two needed splitting rather than moving. parsePath needs a resolved account for its account-segment check, so only the host rejection — which needs just the configured base URL — moves ahead of the read; hoisting the rest would put account resolution before stdin, which is the ordering an earlier round deliberately reversed. applySubscribeFlags resolves people over the network, so only its mutually-exclusive check moves out, into rejectSubscribeConflict, which it now calls itself. files update keeps its type switch where it is — the no-op branches read the resolved content — and validates the --type vocabulary early instead. Fourteen cases assert, through a stdin reader that records reads and a counting transport, that nothing is drained and no request is issued. Also: the jq fallback added last round could double-write. writeJQ streams each result, so a filter that fails partway has already written, and retrying plainly emitted two incompatible envelopes. Decide the filter's usability up front instead — parse and compile failures are the only ones knowable before output exists. Buffering the render was the alternative, but the sanitizer is TTY-gated on the writer, so buffering would silently drop terminal-injection escaping. --- internal/cli/root.go | 32 ++++++++--- internal/commands/api.go | 40 +++++++++++++- internal/commands/boost.go | 14 +++++ internal/commands/chat.go | 32 +++++++---- internal/commands/checkins.go | 13 +++++ internal/commands/files.go | 61 +++++++++++++++++---- internal/commands/gauges.go | 10 +++- internal/commands/helpers.go | 16 +++++- internal/commands/messages.go | 3 + internal/commands/schedule.go | 4 ++ internal/commands/stdin_integration_test.go | 58 ++++++++++++++++++++ internal/commands/todos.go | 28 ++++++++-- 12 files changed, 270 insertions(+), 41 deletions(-) diff --git a/internal/cli/root.go b/internal/cli/root.go index 9c70d2227..2e110d3ed 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -439,24 +439,40 @@ func Execute() { format = output.FormatJSON } + // An unusable filter must not swallow the error: --jq is validated in + // the pre-run, so an error raised *before* that check would otherwise + // be rendered through an unparseable filter and exit non-zero having + // printed nothing. Decide here instead of retrying after a failed + // write — writeJQ streams each result as it produces it, so a filter + // that fails partway (".error, error(\"stop\")") has already written, + // and a second pass would emit two incompatible envelopes. + if jqFilter != "" && !jqUsable(jqFilter) { + jqFilter = "" + } + writer := output.New(output.Options{ Format: format, Writer: os.Stdout, JQFilter: jqFilter, }) - if writeErr := writer.Err(err); writeErr != nil && jqFilter != "" { - // The filter itself could not render the envelope — an invalid - // --jq paired with an error raised before jq validation, which - // otherwise exits non-zero having printed nothing at all. The - // error the caller needs outranks the filter they asked for. - plain := output.New(output.Options{Format: format, Writer: os.Stdout}) - _ = plain.Err(err) - } + _ = writer.Err(err) os.Exit(output.ExitCodeFor(apiErr.Code)) } } +// jqUsable reports whether a filter parses and compiles. Only these failures +// are knowable before any output is produced, which is what makes clearing the +// filter safe: a filter that fails at runtime may already have written. +func jqUsable(filter string) bool { + q, err := gojq.Parse(filter) + if err != nil { + return false + } + _, err = gojq.Compile(q, gojq.WithEnvironLoader(os.Environ)) + return err == nil +} + // resolveProfile determines which profile to use. // Resolution order: // 1. --profile / -P flag diff --git a/internal/commands/api.go b/internal/commands/api.go index fa70ee683..2fa4dc3db 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -87,12 +87,19 @@ Use --data - to read the JSON body from stdin: return missingArg(cmd, "--data") } + app := appctx.FromContext(cmd.Context()) + + // Refuse a foreign host before the pipe is drained; the rest of the + // path parse needs the resolved account, so it follows. + if err := rejectForeignAPIPath(args[0], app.Config.BaseURL); err != nil { + return err + } + data, err := resolveContentValue(cmd, data, -1, "--data") if err != nil { return err } - app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { return err } @@ -149,12 +156,19 @@ Use --data - to read the JSON body from stdin: return missingArg(cmd, "--data") } + app := appctx.FromContext(cmd.Context()) + + // Refuse a foreign host before the pipe is drained; the rest of the + // path parse needs the resolved account, so it follows. + if err := rejectForeignAPIPath(args[0], app.Config.BaseURL); err != nil { + return err + } + data, err := resolveContentValue(cmd, data, -1, "--data") if err != nil { return err } - app := appctx.FromContext(cmd.Context()) if err := ensureAccount(cmd, app); err != nil { return err } @@ -256,6 +270,28 @@ var accountSegmentPattern = regexp.MustCompile(`^/([0-9]+)(/.*)?$`) // All leading slashes and a mixed-case scheme are normalized first so neither // "//https://evil/…" nor "HTTPS://evil/…" can smuggle an absolute URL past the // host check (URL schemes are case-insensitive per RFC 3986 §3.1). +// rejectForeignAPIPath answers the half of parsePath that needs only the +// configured base URL: an absolute URL on another host is refused outright, so +// credentials never leave the configured host. The account-segment half needs a +// resolved account, so it stays in parsePath — this exists so the host check can +// run before a "-" drains stdin for an invocation that cannot be sent. +func rejectForeignAPIPath(input, baseURL string) error { + candidate := strings.TrimLeft(input, "/") + lower := strings.ToLower(candidate) + if !strings.HasPrefix(lower, "http://") && !strings.HasPrefix(lower, "https://") { + return nil + } + u, err := url.Parse(candidate) + if err != nil || u.Host == "" { + return output.ErrUsage("invalid API URL: " + input) + } + base, baseErr := url.Parse(baseURL) + if baseErr != nil || base.Host == "" || !sameHostPort(u, base) { + return output.ErrUsage("API path must be relative or a Basecamp URL on the configured host; refusing to send credentials to " + input) + } + return nil +} + func parsePath(input, baseURL, accountID string) (string, error) { candidate := strings.TrimLeft(input, "/") lower := strings.ToLower(candidate) diff --git a/internal/commands/boost.go b/internal/commands/boost.go index 3e64c845e..df1698f77 100644 --- a/internal/commands/boost.go +++ b/internal/commands/boost.go @@ -271,6 +271,20 @@ Use --event to boost a specific event within the item.`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + + // Both identifiers are decidable from the arguments alone, so they + // are checked before the pipe is drained; runBoostCreate parses + // them again once the project is known. + recordingID, _ := extractWithProject(args[0]) + if _, err := strconv.ParseInt(recordingID, 10, 64); err != nil { + return output.ErrUsage("Invalid ID") + } + if eventID != "" { + if _, err := strconv.ParseInt(eventID, 10, 64); err != nil { + return output.ErrUsage("Invalid event ID") + } + } + content, err := resolveContentValue(cmd, args[1], 1, "<content>") if err != nil { return err diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 26e931a00..9faa3ce83 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -330,6 +330,16 @@ Use - as the message argument to read the message from stdin: argIndex, what = 0, "<message>" } + // An explicitly supplied --room must be numeric; when it is absent + // the room is resolved from the project, which needs the network. + // Check the supplied case here so a malformed room does not cost + // the caller a drained pipe. + if *chatID != "" { + if _, err := strconv.ParseInt(*chatID, 10, 64); err != nil { + return output.ErrUsage("Invalid chat room ID") + } + } + var err error messageContent, err = resolveContentValue(cmd, messageContent, argIndex, what) if err != nil { @@ -798,6 +808,18 @@ edit to rich text.`, argIndex, what = 1, "[content]" } + // Validate the content mode before reading stdin, not just before + // the request: an unknown --content-type dooms the invocation, and + // draining the pipe first makes the caller wait on a producer whose + // output is already discarded — or lets a blank pipe answer "stdin + // is empty" instead of naming the bad flag. + ct := *contentType + switch ct { + case "", "text/html", "text/plain": + default: + return output.ErrUsage(fmt.Sprintf("unsupported --content-type %q (expected text/html or text/plain)", ct)) + } + var contentErr error messageContent, contentErr = resolveContentValue(cmd, messageContent, argIndex, what) if contentErr != nil { @@ -808,16 +830,6 @@ edit to rich text.`, return missingArg(cmd, "<content>") } - // Validate the content mode before any request or account setup so an - // unknown --content-type fails fast rather than silently sending raw - // bytes (the SDK no longer validates content type for us). - ct := *contentType - switch ct { - case "", "text/html", "text/plain": - default: - return output.ErrUsage(fmt.Sprintf("unsupported --content-type %q (expected text/html or text/plain)", ct)) - } - if err := ensureAccount(cmd, app); err != nil { return err } diff --git a/internal/commands/checkins.go b/internal/commands/checkins.go index 90d614b12..5107e7f2e 100644 --- a/internal/commands/checkins.go +++ b/internal/commands/checkins.go @@ -1269,6 +1269,13 @@ func newCheckinsAnswerCreateCmd(project *string) *cobra.Command { } questionID := args[0] + + // Decidable from the arguments alone, so it precedes the read; the + // project-resolution path parses it again once the project is known. + if _, err := strconv.ParseInt(questionID, 10, 64); err != nil { + return output.ErrUsage("Invalid question ID") + } + content, err := resolveContentArg(cmd, args[1:], 1) if err != nil { return err @@ -1386,6 +1393,12 @@ You can pass either an answer ID or a Basecamp URL: // Extract ID and project from URL if provided answerIDStr, urlProjectID := extractWithProject(args[0]) + // Decidable from the arguments alone, so it precedes the read; the + // project-resolution path parses it again once the project is known. + if _, err := strconv.ParseInt(answerIDStr, 10, 64); err != nil { + return output.ErrUsage("Invalid answer ID") + } + content, err := resolveContentArg(cmd, args[1:], 1) if err != nil { return err diff --git a/internal/commands/files.go b/internal/commands/files.go index 591fc03a9..d9f26cf69 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -887,11 +887,15 @@ as an upload in the target folder (vault).`, basecamp uploads create ./photo.png --folder 123 --description "Site photo"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - description, err := resolveContentValue(cmd, description, -1, "--description") + filePath, err := validateUploadPath(args[0]) + if err != nil { + return err + } + description, err = resolveContentValue(cmd, description, -1, "--description") if err != nil { return err } - return runUploadFile(cmd, *project, *vaultID, args[0], description, visibleToClients) + return runUploadFile(cmd, *project, *vaultID, filePath, description, visibleToClients) }, } @@ -921,11 +925,15 @@ attachment and then created as an upload in the target folder.`, basecamp upload ./photo.png --folder 123 --description "Site photo"`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - description, err := resolveContentValue(cmd, description, -1, "--description") + filePath, err := validateUploadPath(args[0]) + if err != nil { + return err + } + description, err = resolveContentValue(cmd, description, -1, "--description") if err != nil { return err } - return runUploadFile(cmd, project, vaultID, args[0], description, visibleToClients) + return runUploadFile(cmd, project, vaultID, filePath, description, visibleToClients) }, } @@ -968,6 +976,19 @@ func resolveVaultClientVisibility(cmd *cobra.Command, app *appctx.App, vaultFlag return &value, nil } +// validateUploadPath normalizes a drag/paste path and checks the file is +// readable. It needs no account and no network, so the upload commands run it +// before resolving a "-" description: a missing file should not cost the caller +// a drained pipe, and a blank pipe must not answer "stdin is empty" instead of +// naming the unreadable file. +func validateUploadPath(filePath string) (string, error) { + filePath = richtext.NormalizeDragPath(filePath) + if err := richtext.ValidateFile(filePath); err != nil { + return "", fmt.Errorf("%s: %w", filePath, err) + } + return filePath, nil +} + func runUploadFile(cmd *cobra.Command, project, vaultID, filePath, description string, visibleToClients bool) error { app := appctx.FromContext(cmd.Context()) @@ -975,10 +996,12 @@ func runUploadFile(cmd *cobra.Command, project, vaultID, filePath, description s return err } - // Normalize drag/paste paths and validate - filePath = richtext.NormalizeDragPath(filePath) - if err := richtext.ValidateFile(filePath); err != nil { - return fmt.Errorf("%s: %w", filePath, err) + // Normalize drag/paste paths and validate. Callers that read stdin run + // this first (see validateUploadPath); repeating it is idempotent and + // keeps this function correct on its own. + filePath, err := validateUploadPath(filePath) + if err != nil { + return err } // Resolve project, with interactive fallback @@ -1243,6 +1266,10 @@ 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 { + return err + } + content := "" if len(args) > 1 { var contentErr error @@ -1917,8 +1944,21 @@ You can pass either an item ID or a Basecamp URL: // Syntactic checks first, then "-", then account and network: a // malformed ID is answered without waiting on the producer, and a - // blank pipe cannot mask it. Only an exact "-" reads stdin; - // --content "" stays the clear idiom. + // blank pipe cannot mask it. The --type vocabulary is checked here + // too — the switch below cannot move, since its no-op branches read + // the resolved content, but an unknown type dooms the invocation on + // its own and must not cost the caller a drained pipe. + itemType = strings.ToLower(strings.TrimSpace(itemType)) + switch itemType { + case "", "document", "doc", "vault", "folder", "upload", "file": + default: + return output.ErrUsageHint( + fmt.Sprintf("Invalid type: %s", itemType), + "Use: vault, document, or upload", + ) + } + + // Only an exact "-" reads stdin; --content "" stays the clear idiom. var contentErr error content, contentErr = resolveContentValue(cmd, content, -1, "--content") if contentErr != nil { @@ -1936,7 +1976,6 @@ You can pass either an item ID or a Basecamp URL: docContentSet := contentChanged && (content == "" || contentTrimmed != "") nonDocTitleSet := titleChanged && titleTrimmed != "" nonDocContentSet := contentChanged && contentTrimmed != "" - itemType = strings.ToLower(strings.TrimSpace(itemType)) switch itemType { case "", "document", "doc": if !docTitleSet && !docContentSet { diff --git a/internal/commands/gauges.go b/internal/commands/gauges.go index 26b9b9ff3..ff5a2958d 100644 --- a/internal/commands/gauges.go +++ b/internal/commands/gauges.go @@ -185,6 +185,13 @@ func newGaugesCreateCmd(project *string) *cobra.Command { return output.ErrUsage("--position must be between 0 and 100") } + // --notify custom without --subscriptions cannot succeed, and that + // is knowable from the flags alone, so it is decided before the pipe + // is drained rather than at request-building time. + if notify == "custom" && len(subscriptions) == 0 { + return output.ErrUsage("--subscriptions required when using --notify custom") + } + // Local validation, then "-", then account: a bad stdin gets the // stdin error rather than "--account is required". description, err := resolveContentValue(cmd, description, -1, "--description") @@ -220,9 +227,6 @@ func newGaugesCreateCmd(project *string) *cobra.Command { if notify != "" { req.Notify = notify if notify == "custom" { - if len(subscriptions) == 0 { - return output.ErrUsage("--subscriptions required when using --notify custom") - } req.Subscriptions = subscriptions } } diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 54f2d1e02..2d642c0f5 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -460,9 +460,21 @@ func resolvePersonIDs(ctx context.Context, resolver *names.Resolver, input strin // // subscribeChanged should be true when the --subscribe flag was explicitly // provided on the command line (i.e. cmd.Flags().Changed("subscribe")). -func applySubscribeFlags(ctx context.Context, resolver *names.Resolver, subscribe string, subscribeChanged, noSubscribe bool) (*[]int64, error) { +// rejectSubscribeConflict answers the one part of applySubscribeFlags that +// needs neither the network nor an account, so callers that read stdin can +// 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 { if subscribeChanged && noSubscribe { - return nil, output.ErrUsage("--subscribe and --no-subscribe are mutually exclusive") + return output.ErrUsage("--subscribe and --no-subscribe are mutually exclusive") + } + return nil +} + +func applySubscribeFlags(ctx context.Context, resolver *names.Resolver, subscribe string, subscribeChanged, noSubscribe bool) (*[]int64, error) { + if err := rejectSubscribeConflict(subscribeChanged, noSubscribe); err != nil { + return nil, err } if noSubscribe { empty := []int64{} diff --git a/internal/commands/messages.go b/internal/commands/messages.go index c5fe1116a..49e89a724 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -461,6 +461,9 @@ 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 { + return err + } var err error body, err = resolveContentValue(cmd, body, 1, "[body]") if err != nil { diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index f4f7db832..e1ce93478 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -441,6 +441,10 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { return err } + if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { + return err + } + // Local validation, then "-", then account: a bad stdin gets the // stdin error rather than "--account is required". description, err := resolveContentValue(cmd, description, -1, "--description") diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index 382fa147a..f30f127a7 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -367,3 +367,61 @@ func TestMalformedIDRejectedBeforeReadingStdin(t *testing.T) { }) } } + +// An invocation that is already doomed by its flags or arguments must be +// rejected before the pipe is drained. Otherwise the caller waits on a producer +// whose output is discarded, an unbounded one buffers into memory, and a blank +// one answers "stdin is empty" instead of naming the real problem. +func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { + for _, tc := range []struct { + name string + cmd func() *cobra.Command + args []string + want string + }{ + {"api foreign host", NewAPICmd, + []string{"post", "https://evil.example/x", "--data", "-"}, "configured host"}, + {"chat post bad room", NewChatCmd, + []string{"post", "-", "--room", "nope"}, "Invalid chat room ID"}, + {"chat update bad content-type", NewChatCmd, + []string{"update", "1", "-", "--content-type", "bogus"}, "unsupported --content-type"}, + {"boost bad id", NewBoostsCmd, + []string{"create", "nope", "-"}, "Invalid ID"}, + {"boost bad event", NewBoostsCmd, + []string{"create", "1", "-", "--event", "nope"}, "Invalid event ID"}, + {"checkins bad question", NewCheckinsCmd, + []string{"answer", "create", "nope", "-"}, "Invalid question ID"}, + {"checkins bad answer", NewCheckinsCmd, + []string{"answer", "update", "nope", "-"}, "Invalid answer ID"}, + {"files bad type", NewFilesCmd, + []string{"update", "1", "--type", "nonsense", "--content", "-"}, "Invalid type"}, + {"todos sweep without a filter", NewTodosCmd, + []string{"sweep", "--comment", "-"}, "requires a filter"}, + {"todos loose with a list", NewTodosCmd, + []string{"create", "-", "--loose", "--list", "123"}, "cannot be combined with --list"}, + {"gauges custom notify", NewGaugesCmd, + []string{"create", "--position", "50", "--notify", "custom", "--description", "-"}, "--subscriptions required"}, + {"docs subscribe conflict", NewDocsCmd, + []string{"documents", "create", "Title", "-", "--subscribe", "me", "--no-subscribe"}, "mutually exclusive"}, + {"messages subscribe conflict", NewMessagesCmd, + []string{"create", "Title", "-", "--subscribe", "me", "--no-subscribe"}, "mutually exclusive"}, + {"uploads unreadable file", NewUploadsCmd, + []string{"create", "/nope/missing.txt", "--description", "-"}, "missing.txt"}, + } { + t.Run(tc.name, func(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + + stdin := &trackingReader{r: strings.NewReader("body from stdin")} + cmd := tc.cmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + err := executeCommand(cmd, app, tc.args...) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.want) + assert.False(t, stdin.read, "stdin must not be drained for an invocation that cannot succeed") + assert.Zero(t, transport.calls, "no request may be issued") + }) + } +} diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 0fb7f9052..343af7bd1 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1257,6 +1257,20 @@ Use - as the content argument to read the todo title from stdin: if len(args) == 0 { return missingArg(cmd, "<content>") } + + // --loose and a named list are mutually exclusive, and that is + // knowable from the flags alone. Decide it before the pipe is + // drained: a doomed invocation should not make the caller wait on a + // producer, and a blank pipe must not answer "stdin is empty" + // instead of naming the conflict. The destination resolution below + // still repeats the check, since app.Flags.Todolist is only one of + // its inputs. + if loose && (cmd.Flags().Changed("list") || todolist != "") { + return output.ErrUsageHint( + "--loose creates a todo outside any list, so it cannot be combined with --list", + "Drop --list to create on the to-do set, or drop --loose to create in that list") + } + content, err := resolveContentArg(cmd, args, 0) if err != nil { return err @@ -1898,6 +1912,15 @@ Examples: RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) + // Require at least one filter. This gate decides the invocation on + // its own, so it runs before the pipe is drained: otherwise the + // caller waits on a producer whose output is already discarded, and + // a blank pipe answers "stdin is empty" instead of naming the + // missing filter. + if !overdueOnly && assignee == "" { + return output.ErrUsageHint("Sweep requires a filter", "Use --overdue or --assignee to select todos") + } + comment, err := resolveContentValue(cmd, comment, -1, "--comment") if err != nil { return err @@ -1907,11 +1930,6 @@ Examples: return err } - // Require at least one filter - if !overdueOnly && assignee == "" { - return output.ErrUsageHint("Sweep requires a filter", "Use --overdue or --assignee to select todos") - } - // Require at least one action if comment == "" && !complete { return output.ErrUsageHint("Sweep requires an action", "Use --comment and/or --complete") From 7aeaeddb8b80d6c013d993ce1a3f80d646ed0d48 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 20 Aug 2026 01:20:55 -0700 Subject: [PATCH 14/19] Reject a malformed schedule entry ID instead of sending 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit schedule update was the one command in the family with no local ID check — its ParseInt discarded the error, so 'schedule update nope' drained the pipe and then asked the server to update entry 0. It now parses the extracted ID before the read, like every sibling. --- internal/commands/schedule.go | 16 ++++++++++------ internal/commands/stdin_integration_test.go | 2 ++ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index e1ce93478..c55d02f56 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -629,13 +629,19 @@ You can pass either an entry ID or a Basecamp URL: // Extract ID and project from URL if provided. Purely syntactic, // so it precedes the stdin read like every other target-ID check. - // (This command never rejects a malformed entry ID locally — the - // ParseInt below discards its error and the server answers. That - // predates this change and is left alone.) entryID, urlProjectID := extractWithProject(args[0]) + // This command used to send a malformed ID to the server as 0 — + // the ParseInt further down discarded its error. Reject it here + // instead, like every sibling update command, and before the pipe + // is drained. + entryIDInt, err := strconv.ParseInt(entryID, 10, 64) + if err != nil { + return output.ErrUsage("Invalid schedule entry ID") + } + // Syntactic checks first, then "-", then account and network. - description, err := resolveContentValue(cmd, description, -1, "--description") + description, err = resolveContentValue(cmd, description, -1, "--description") if err != nil { return err } @@ -667,8 +673,6 @@ You can pass either an entry ID or a Basecamp URL: return err } - entryIDInt, _ := strconv.ParseInt(entryID, 10, 64) - // Build request with provided fields only req := &basecamp.UpdateScheduleEntryRequest{} hasChanges := false diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index f30f127a7..2f4f5fb33 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -405,6 +405,8 @@ func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { []string{"documents", "create", "Title", "-", "--subscribe", "me", "--no-subscribe"}, "mutually exclusive"}, {"messages subscribe conflict", NewMessagesCmd, []string{"create", "Title", "-", "--subscribe", "me", "--no-subscribe"}, "mutually exclusive"}, + {"schedule bad entry id", NewScheduleCmd, + []string{"update", "nope", "--description", "-"}, "Invalid schedule entry ID"}, {"uploads unreadable file", NewUploadsCmd, []string{"create", "/nope/missing.txt", "--description", "-"}, "missing.txt"}, } { From ac3f38ea2576d3c872c1169adcd089cf0f19c79d Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 20 Aug 2026 01:55:01 -0700 Subject: [PATCH 15/19] Close the remaining pre-read validations, including two I got wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --attach paths are readable or not regardless of the body, so all eight commands that take both now check them before the read, via a shared validateAttachPaths that uploadAttachments also calls. Six more local checks moved ahead of the read: a named --column without --card-table, an untrusted host in a files replace URL (the account half still follows, as with the API path split), schedule update's timestamp formats, and cards/schedule attachments. Two were incomplete fixes from the previous round rather than new sites. The --loose gate read only the local --list flag, so a global --todolist still reached the destination check after the pipe was drained. And files update rejected an unknown --type early but not --content against a folder — which is decidable from Changed("content") alone, without the resolved value the switch below needs. --- internal/commands/attach.go | 14 +++++++++ internal/commands/cards.go | 11 +++++++ internal/commands/chat.go | 6 ++++ internal/commands/checkins.go | 6 ++++ internal/commands/comment.go | 6 ++++ internal/commands/files.go | 32 ++++++++++++++++++--- internal/commands/messages.go | 6 ++++ internal/commands/schedule.go | 24 ++++++++++++++++ internal/commands/stdin_integration_test.go | 30 +++++++++++++++++++ internal/commands/todos.go | 12 ++++++-- 10 files changed, 140 insertions(+), 7 deletions(-) diff --git a/internal/commands/attach.go b/internal/commands/attach.go index e3bbd413a..c2061fbfb 100644 --- a/internal/commands/attach.go +++ b/internal/commands/attach.go @@ -80,6 +80,20 @@ No project is needed — attachment upload is account-scoped.`, return cmd } +// validateAttachPaths checks every --attach path is readable. It needs no +// account and no network, so commands that also read a "-" content input run it +// first: an unreadable attachment dooms the invocation, and draining the pipe +// for it makes the caller wait on a producer whose output is discarded — or +// lets a blank pipe answer "stdin is empty" instead of naming the file. +func validateAttachPaths(paths []string) error { + for _, path := range paths { + if err := richtext.ValidateFile(richtext.NormalizeDragPath(path)); err != nil { + return fmt.Errorf("%s: %w", path, err) + } + } + return nil +} + // uploadAttachments uploads each path and returns attachment references. // Sequential, fails on first error. func uploadAttachments(cmd *cobra.Command, app *appctx.App, paths []string) ([]richtext.AttachmentRef, error) { diff --git a/internal/commands/cards.go b/internal/commands/cards.go index f6ef184b1..8db523523 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -870,6 +870,17 @@ Use - as the body argument to read the body from stdin: if strings.TrimSpace(title) == "" { return cmd.Help() } + // A named column needs --card-table to resolve against, and that is + // knowable from the flags alone. Attachment paths are readable or + // not regardless of the body. Both precede the read, so a doomed + // invocation never costs the caller a drained pipe. + if column != "" && !isNumericID(column) && *cardTable == "" { + return output.ErrUsage("--card-table is required when using --column with a name") + } + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + var content string if len(args) > 1 { var err error diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 9faa3ce83..337d2e79d 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -340,6 +340,12 @@ Use - as the message argument to read the message from stdin: } } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + var err error messageContent, err = resolveContentValue(cmd, messageContent, argIndex, what) if err != nil { diff --git a/internal/commands/checkins.go b/internal/commands/checkins.go index 5107e7f2e..fff393ae8 100644 --- a/internal/commands/checkins.go +++ b/internal/commands/checkins.go @@ -1276,6 +1276,12 @@ func newCheckinsAnswerCreateCmd(project *string) *cobra.Command { return output.ErrUsage("Invalid question ID") } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + content, err := resolveContentArg(cmd, args[1:], 1) if err != nil { return err diff --git a/internal/commands/comment.go b/internal/commands/comment.go index 27baacf54..16348cb5d 100644 --- a/internal/commands/comment.go +++ b/internal/commands/comment.go @@ -1166,6 +1166,12 @@ busybox-ash) it posts a literal leading $ and keeps \n as backslash-n: return output.ErrUsage("cannot combine --edit and positional content") } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + var content string if len(args) > 1 { var err error diff --git a/internal/commands/files.go b/internal/commands/files.go index d9f26cf69..0fe03cca2 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -1270,6 +1270,12 @@ Use - as the content argument to read the document body from stdin: return err } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + content := "" if len(args) > 1 { var contentErr error @@ -1760,11 +1766,19 @@ You can pass either an upload ID or a Basecamp URL: return fmt.Errorf("%s: %w", filePath, err) } + // The trusted-host check needs only the configured base URL, so it + // runs here rather than with the account-identity checks below — + // refusing a look-alike host must not cost the caller a drained + // pipe. The full rationale for the check is at its sibling below. + if urlarg.IsURL(args[0]) && !hostutil.IsTrustedBasecampHost(args[0], app.Config.BaseURL) { + return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") + } + // Syntactic checks first, then "-", then account and network: a - // malformed ID or missing file is answered without waiting on the - // producer. The URL identity checks below need the session account, - // so they necessarily follow. Only an exact "-" reads stdin; - // --description "" stays the clear idiom. + // malformed ID, missing file or foreign host is answered without + // waiting on the producer. The account-identity checks below need + // the session account, so they necessarily follow. Only an exact + // "-" reads stdin; --description "" stays the clear idiom. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -1958,6 +1972,16 @@ You can pass either an item ID or a Basecamp URL: ) } + // --content is meaningless for a folder, and whether it was given is + // knowable before its value is: the switch below needs the resolved + // content for its no-op branches, this does not. + if cmd.Flags().Changed("content") { + switch itemType { + case "vault", "folder": + return output.ErrUsage("--content can only be used with --type document or upload") + } + } + // Only an exact "-" reads stdin; --content "" stays the clear idiom. var contentErr error content, contentErr = resolveContentValue(cmd, content, -1, "--content") diff --git a/internal/commands/messages.go b/internal/commands/messages.go index 49e89a724..33d4dd558 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -464,6 +464,12 @@ Use - as the body argument to read the body from stdin: if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { return err } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + var err error body, err = resolveContentValue(cmd, body, 1, "[body]") if err != nil { diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index c55d02f56..6935883ac 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -445,6 +445,12 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { return err } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + // Local validation, then "-", then account: a bad stdin gets the // stdin error rather than "--account is required". description, err := resolveContentValue(cmd, description, -1, "--description") @@ -640,6 +646,24 @@ You can pass either an entry ID or a Basecamp URL: return output.ErrUsage("Invalid schedule entry ID") } + // The timestamp formats and the attachment paths are decidable from + // the flags alone, as they already are on the create path — so they + // precede the read rather than following account and project + // resolution. + if startsAt != "" { + if err := validateScheduleTimestamp("starts-at", startsAt); err != nil { + return err + } + } + if endsAt != "" { + if err := validateScheduleTimestamp("ends-at", endsAt); err != nil { + return err + } + } + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + // Syntactic checks first, then "-", then account and network. description, err = resolveContentValue(cmd, description, -1, "--description") if err != nil { diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index 2f4f5fb33..69f6437ec 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -405,6 +405,16 @@ func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { []string{"documents", "create", "Title", "-", "--subscribe", "me", "--no-subscribe"}, "mutually exclusive"}, {"messages subscribe conflict", NewMessagesCmd, []string{"create", "Title", "-", "--subscribe", "me", "--no-subscribe"}, "mutually exclusive"}, + {"messages bad attachment", NewMessagesCmd, + []string{"create", "Title", "-", "--attach", "/nope/missing.png"}, "missing.png"}, + {"cards named column without a table", NewCardsCmd, + []string{"create", "Title", "-", "--column", "Backlog"}, "--card-table is required"}, + {"files replace untrusted host", NewFilesCmd, + []string{"replace", "https://evil.example/123/buckets/456/uploads/789", "stdin.go", "--description", "-"}, "untrusted host"}, + {"files vault with content", NewFilesCmd, + []string{"update", "123", "--type", "vault", "--content", "-"}, "--content can only be used"}, + {"schedule bad timestamp", NewScheduleCmd, + []string{"update", "123", "--starts-at", "invalid", "--description", "-"}, "starts-at"}, {"schedule bad entry id", NewScheduleCmd, []string{"update", "nope", "--description", "-"}, "Invalid schedule entry ID"}, {"uploads unreadable file", NewUploadsCmd, @@ -427,3 +437,23 @@ func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { }) } } + +// The --loose gate has to see a configured todolist too, not just the local +// --list flag: a global --todolist arrives via app.Flags and used to be caught +// only after the pipe was drained and the project resolved. +func TestTodosLooseRejectsAConfiguredTodolistBeforeReadingStdin(t *testing.T) { + transport := &countingTransport{} + app := setupTransportTestApp(t, transport) + app.Flags.Todolist = "123" + + stdin := &trackingReader{r: strings.NewReader("body from stdin")} + cmd := NewTodosCmd() + InstallDashGuard(cmd) + cmd.SetIn(stdin) + + err := executeCommand(cmd, app, "create", "-", "--loose") + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot be combined with --list") + assert.False(t, stdin.read, "stdin must not be drained for an invocation that cannot succeed") + assert.Zero(t, transport.calls) +} diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 343af7bd1..fd3592f5f 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1263,14 +1263,20 @@ Use - as the content argument to read the todo title from stdin: // drained: a doomed invocation should not make the caller wait on a // producer, and a blank pipe must not answer "stdin is empty" // instead of naming the conflict. The destination resolution below - // still repeats the check, since app.Flags.Todolist is only one of - // its inputs. - if loose && (cmd.Flags().Changed("list") || todolist != "") { + // still repeats the check, since a configured todolist is only one + // of its inputs. + if loose && (cmd.Flags().Changed("list") || todolist != "" || app.Flags.Todolist != "") { return output.ErrUsageHint( "--loose creates a todo outside any list, so it cannot be combined with --list", "Drop --list to create on the to-do set, or drop --loose to create in that list") } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + content, err := resolveContentArg(cmd, args, 0) if err != nil { return err From e4d545d2005c9e067bd40dea03356ce9d1ed34ad Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 20 Aug 2026 01:58:28 -0700 Subject: [PATCH 16/19] Finish the pre-read sweep; stop a test depending on the runner's stdin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit chat post never got the --content-type check its update sibling has, and todos update validated --due/--starts-on after the read. Both now precede it; the parsed dates are carried forward rather than re-derived. The interactivity tests stubbed only stdout, so their interactive baseline depended on the ambient stdin — now that both streams are checked, a piped stdin fails them. Extracted stubCharDeviceStdio, which points both at /dev/null. Note the exposure is not reachable through 'go test', which gives the test binary its own stdin; running the compiled binary with a piped stdin is what reproduces it, and does: the old baseline fails there and the new one passes. --- internal/cli/root_test.go | 51 +++++++++------------ internal/commands/chat.go | 8 +++- internal/commands/stdin_integration_test.go | 4 ++ internal/commands/todos.go | 37 ++++++++------- 4 files changed, 52 insertions(+), 48 deletions(-) diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index fe609ad0b..a68c88781 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -166,16 +166,7 @@ func isolateRootTest(t *testing.T) { } func TestIsInteractiveTTYWithNonInteractiveEnv(t *testing.T) { - devNull, err := os.Open(os.DevNull) - if err != nil { - t.Skip(os.DevNull + " not available") - } - origStdout := os.Stdout - os.Stdout = devNull - t.Cleanup(func() { - os.Stdout = origStdout - devNull.Close() - }) + stubCharDeviceStdio(t) t.Setenv("BASECAMP_NONINTERACTIVE", "") require.True(t, isInteractiveTTY(appctx.GlobalFlags{})) @@ -292,16 +283,7 @@ func TestVersionWithJQReturnsUsageError(t *testing.T) { // feeding a "-" content input the picker would eat that body as keystrokes — // so a terminal stdout is not on its own enough to open one. func TestIsInteractiveTTYRequiresUnpipedStdin(t *testing.T) { - devNull, err := os.Open(os.DevNull) - if err != nil { - t.Skip(os.DevNull + " not available") - } - origStdout := os.Stdout - os.Stdout = devNull - t.Cleanup(func() { - os.Stdout = origStdout - devNull.Close() - }) + stubCharDeviceStdio(t) t.Setenv("BASECAMP_NONINTERACTIVE", "") require.True(t, isInteractiveTTY(appctx.GlobalFlags{}), "char-device stdio is interactive") @@ -330,16 +312,7 @@ func TestIsInteractiveTTYRequiresUnpipedStdin(t *testing.T) { func TestRootDashGuardWithTerminalStdout(t *testing.T) { isolateRootTest(t) - devNull, err := os.Open(os.DevNull) - if err != nil { - t.Skip(os.DevNull + " not available") - } - origStdout := os.Stdout - os.Stdout = devNull - t.Cleanup(func() { - os.Stdout = origStdout - devNull.Close() - }) + stubCharDeviceStdio(t) t.Setenv("BASECAMP_NONINTERACTIVE", "") reader, writer, err := os.Pipe() @@ -372,3 +345,21 @@ func TestRootDashGuardWithTerminalStdout(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "does not read stdin") } + +// stubCharDeviceStdio points both stdout and stdin at /dev/null, a character +// device, so interactivity assertions do not depend on how `go test` itself was +// invoked — a piped stdin on the test runner would otherwise fail the +// interactive baseline now that both streams are checked. +func stubCharDeviceStdio(t *testing.T) { + t.Helper() + devNull, err := os.Open(os.DevNull) + if err != nil { + t.Skip(os.DevNull + " not available") + } + origStdout, origStdin := os.Stdout, os.Stdin + os.Stdout, os.Stdin = devNull, devNull + t.Cleanup(func() { + os.Stdout, os.Stdin = origStdout, origStdin + devNull.Close() + }) +} diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 337d2e79d..551932695 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -333,12 +333,18 @@ Use - as the message argument to read the message from stdin: // An explicitly supplied --room must be numeric; when it is absent // the room is resolved from the project, which needs the network. // Check the supplied case here so a malformed room does not cost - // the caller a drained pipe. + // the caller a drained pipe. Same for the content mode, which chat + // update already checks before its own read. if *chatID != "" { if _, err := strconv.ParseInt(*chatID, 10, 64); err != nil { return output.ErrUsage("Invalid chat room ID") } } + switch *contentType { + case "", "text/html", "text/plain": + default: + return output.ErrUsage(fmt.Sprintf("unsupported --content-type %q (expected text/html or text/plain)", *contentType)) + } // Attachment paths are readable or not regardless of the body, so // check them before the pipe is drained. diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index 69f6437ec..045736ba0 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -383,6 +383,10 @@ func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { []string{"post", "https://evil.example/x", "--data", "-"}, "configured host"}, {"chat post bad room", NewChatCmd, []string{"post", "-", "--room", "nope"}, "Invalid chat room ID"}, + {"chat post bad content-type", NewChatCmd, + []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 content-type", NewChatCmd, []string{"update", "1", "-", "--content-type", "bogus"}, "unsupported --content-type"}, {"boost bad id", NewBoostsCmd, diff --git a/internal/commands/todos.go b/internal/commands/todos.go index fd3592f5f..5c3206d49 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1577,9 +1577,27 @@ Set or clear the people notified when the todo is completed: return output.ErrUsage("Invalid todo ID") } + // Date formats are decidable from the flags alone, so they join the + // ID check ahead of the read. The parsed values are carried forward + // rather than re-derived below. + var parsedDue string + if !clearDue && 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)) + } + } + var parsedStarts string + if !clearStarts && !clearDue && strings.TrimSpace(startsOn) != "" { + parsedStarts = dateparse.Parse(startsOn) + if _, err := time.Parse("2006-01-02", parsedStarts); err != nil { + return output.ErrUsage(fmt.Sprintf("Invalid start date: %q", startsOn)) + } + } + // Syntactic checks first, then "-", then account and network: a - // malformed ID is answered without waiting on the producer, and a - // blank pipe cannot mask it. Only an exact "-" reads stdin; + // malformed ID or date is answered without waiting on the producer, + // and a blank pipe cannot mask it. Only an exact "-" reads stdin; // --description "" stays the clear idiom. description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { @@ -1603,21 +1621,6 @@ Set or clear the people notified when the todo is completed: descHTML = richtext.MarkdownToHTML(description) } - var parsedDue string - if !clearDue && 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)) - } - } - var parsedStarts string - if !clearStarts && !clearDue && strings.TrimSpace(startsOn) != "" { - parsedStarts = dateparse.Parse(startsOn) - if _, err := time.Parse("2006-01-02", parsedStarts); err != nil { - return output.ErrUsage(fmt.Sprintf("Invalid start date: %q", startsOn)) - } - } - var assigneeIDs []int64 if assigneeChanged { if assigneeIDs, err = resolveAssigneeIDs(cmd.Context(), app, assignee); err != nil { From 27b86152f54437668a96afb18cba58a0413f51b9 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 20 Aug 2026 02:25:39 -0700 Subject: [PATCH 17/19] Never replay an envelope after a jq-backed write has begun MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fallback writer was not the path that retried — app.Err was. When a filter emits results and then fails, writeJQ has already written, and falling through to the plain writer appended a second, unfiltered envelope. 'todos create --jq ".error, error(\"stop\")"' printed both a bare line and a full JSON document to stdout, which is two incompatible outputs for the machine consumer the filter exists to serve. Once a jq-backed write has begun, stdout is final: report the render failure on stderr and exit with the original code. Without a filter nothing partial can have been written through one, so the plain fallback stays as the last resort for a broken pipe. jqUsable stays — it covers the other path, where no app existed yet — but it was never able to prevent this, and the comment that implied otherwise now describes only the path it governs. The old unit test drove two writers directly rather than Execute, so it could not have caught this. Replaced with a predicate test, plus an e2e case that runs the real binary and asserts stdout holds exactly one document; removing the fix makes it fail. --- e2e/stdin_dash.bats | 32 ++++++++++++++++++++++++ internal/cli/cobra_error_test.go | 42 ++++++++++++++++---------------- internal/cli/root.go | 38 ++++++++++++++++++++--------- 3 files changed, 80 insertions(+), 32 deletions(-) diff --git a/e2e/stdin_dash.bats b/e2e/stdin_dash.bats index 1ada2283d..50fed2c56 100644 --- a/e2e/stdin_dash.bats +++ b/e2e/stdin_dash.bats @@ -110,3 +110,35 @@ load test_helper assert_success assert_output_contains "--description" } + + +# --jq error rendering. Not stdin-specific, but it shares this file's contract: +# exactly one document reaches stdout, so a machine consumer can parse it. + +@test "a jq filter that fails partway leaves one document on stdout" { + create_credentials + create_global_config '{"account_id": 99999}' + + # This filter emits .error, then raises. writeJQ streams results as it + # produces them, so the first line is already on stdout when it fails — + # replaying the envelope would append a second, incompatible document. + run bash -c "basecamp todos create --jq '.error, error(\"stop\")' 2>/dev/null < /dev/null" + assert_failure + [ "${#lines[@]}" -eq 1 ] + assert_output_contains "required" + + # The failure is still reported, on stderr. + run bash -c "basecamp todos create --jq '.error, error(\"stop\")' 2>&1 >/dev/null < /dev/null" + assert_output_contains "--jq" +} + +@test "an unparseable jq filter still renders an error raised before validation" { + create_credentials + create_global_config '{"account_id": 99999}' + + # The stray-dash guard fires before --jq is validated, so the envelope would + # otherwise be rendered through an unparseable filter and print nothing. + run bash -c "printf 'x' | basecamp --jq '.[invalid' - 2>/dev/null" + assert_failure + assert_output_contains "does not read stdin" +} diff --git a/internal/cli/cobra_error_test.go b/internal/cli/cobra_error_test.go index 34cd66a9b..172ba6a66 100644 --- a/internal/cli/cobra_error_test.go +++ b/internal/cli/cobra_error_test.go @@ -1,7 +1,6 @@ package cli import ( - "bytes" "errors" "testing" @@ -87,24 +86,25 @@ func TestTransformCobraErrorIgnoresUnanchoredArityText(t *testing.T) { assert.Equal(t, msg, err.Error()) } -// An invalid --jq paired with an error raised before jq validation used to -// exit non-zero having printed nothing: the fallback writer tried to render the -// envelope through the broken filter, and its failure was discarded. The error -// the caller needs outranks the filter they asked for. -func TestInvalidJQFilterStillRendersAnEarlierError(t *testing.T) { - var buf bytes.Buffer - writer := output.New(output.Options{ - Format: output.FormatJSON, - Writer: &buf, - JQFilter: ".[invalid", - }) - - require.Error(t, writer.Err(output.ErrUsage("stray dash")), - "a broken filter must report that it could not render") - require.Empty(t, buf.String(), "and must not have written a usable envelope") - - buf.Reset() - plain := output.New(output.Options{Format: output.FormatJSON, Writer: &buf}) - require.NoError(t, plain.Err(output.ErrUsage("stray dash"))) - assert.Contains(t, buf.String(), "stray dash") +// jqUsable decides whether the fallback writer may keep a filter. It answers +// only the question that is knowable before any output exists — a filter that +// parses and compiles can still fail partway through producing results, which +// is why the jq-backed path never retries a failed write. That end-to-end +// property needs the real binary and is covered in e2e/stdin_dash.bats; this +// only pins the predicate. +func TestJQUsable(t *testing.T) { + for _, tc := range []struct { + name string + filter string + want bool + }{ + {"valid", ".error", true}, + {"parse failure", ".[invalid", false}, + {"compile failure", ".foo | undefined_function", false}, + {"valid but fails at runtime", `.error, error("stop")`, true}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, jqUsable(tc.filter)) + }) + } } diff --git a/internal/cli/root.go b/internal/cli/root.go index 2e110d3ed..1d0c6eb3e 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -396,12 +396,27 @@ func Execute() { disableJQ := output.IsJQError(err) if !disableJQ { if app := appctx.FromContext(executedCmd.Context()); app != nil { - if writeErr := app.Err(err); writeErr == nil { + writeErr := app.Err(err) + if writeErr == nil { os.Exit(output.ExitCodeFor(apiErr.Code)) } - // app.Err() write failed (e.g. jq runtime error on the error - // envelope, or broken pipe). Disable jq in the fallback writer - // to avoid replaying the same failure. + + // The write failed. When a filter was in play it may already + // have emitted results — writeJQ streams each one as it is + // produced, so a filter like `.error, error("stop")` prints + // before it fails. Replaying the envelope on stdout would + // append a second, unfiltered document to the first, which is + // two incompatible outputs for the machine consumer the filter + // exists to serve. Once a jq-backed write has begun, stdout is + // final: report the failure on stderr and stop. + if jq, _ := cmd.PersistentFlags().GetString("jq"); jq != "" { + fmt.Fprintf(os.Stderr, "error rendering error output through --jq: %v\n", writeErr) + os.Exit(output.ExitCodeFor(apiErr.Code)) + } + + // No filter: nothing partial can have been written through one, + // so the plain fallback below is still the right last resort + // (e.g. a broken pipe). disableJQ = true } } @@ -439,13 +454,14 @@ func Execute() { format = output.FormatJSON } - // An unusable filter must not swallow the error: --jq is validated in - // the pre-run, so an error raised *before* that check would otherwise - // be rendered through an unparseable filter and exit non-zero having - // printed nothing. Decide here instead of retrying after a failed - // write — writeJQ streams each result as it produces it, so a filter - // that fails partway (".error, error(\"stop\")") has already written, - // and a second pass would emit two incompatible envelopes. + // This path runs only when nothing has been written yet: either no app + // was available (an error raised before it was built) or no filter was + // in play. An unusable filter must not swallow the error — --jq is + // validated in the pre-run, so an error raised *before* that check + // would otherwise render through an unparseable filter and exit + // non-zero having printed nothing. Decide usability up front rather + // than retrying after a failed write, which is what the jq-backed path + // above refuses to do. if jqFilter != "" && !jqUsable(jqFilter) { jqFilter = "" } From 58ff6da8df87fc12b10b573d43220cd704bc8c15 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 20 Aug 2026 03:18:44 -0700 Subject: [PATCH 18/19] Fix the ordering backstop, then fix what it found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AST check compared the first recognized check with the first read, so one early check hid every later one — which is why the sweep kept looking finished. It now flags every recognized check that follows the read, and found twelve more sites immediately. Dropped dateparse.Parse and isNumericID from the recognized set: neither can fail, so they decide nothing and only report branch selection, which would be answered with suppressions rather than fixes. What it found: duplicate schedule timestamp and card column checks left behind by earlier hoists, chat update's URL host and shape plus its line and room IDs, and files replace's recording-type check. For the URL cases only the account comparison genuinely needs a resolved account, so that is all that still follows the read. Semantic cases the AST cannot see, from the same review: cards update never validated its attachments (the earlier sweep took one RunE per file), schedule create discarded its ParseInt error and would have created under schedule 0, and explicitly supplied dock IDs were parsed only once a request was being built. getDockToolID returns an explicit value verbatim, so requireNumericID settles them from the flag alone. uploadAttachments now calls validateAttachPaths rather than duplicating it, which is what its comment already claimed. --- e2e/stdin_dash.bats | 9 ++- internal/cli/cobra_error_test.go | 15 +++++ internal/cli/root.go | 18 +++++- internal/commands/attach.go | 10 ++-- internal/commands/cards.go | 19 +++++-- internal/commands/chat.go | 61 ++++++++++++-------- internal/commands/files.go | 62 +++++++++++---------- internal/commands/helpers.go | 15 +++++ internal/commands/messages.go | 3 + internal/commands/schedule.go | 9 +-- internal/commands/stdin_integration_test.go | 22 ++++++++ internal/commands/stdin_ordering_test.go | 59 ++++++++++++++++---- internal/commands/todolists.go | 4 ++ internal/commands/todos.go | 3 + 14 files changed, 224 insertions(+), 85 deletions(-) diff --git a/e2e/stdin_dash.bats b/e2e/stdin_dash.bats index 50fed2c56..7458f5e97 100644 --- a/e2e/stdin_dash.bats +++ b/e2e/stdin_dash.bats @@ -127,9 +127,14 @@ load test_helper [ "${#lines[@]}" -eq 1 ] assert_output_contains "required" - # The failure is still reported, on stderr. - run bash -c "basecamp todos create --jq '.error, error(\"stop\")' 2>&1 >/dev/null < /dev/null" + # The failure is still reported on stderr, but response-selected text in a + # jq runtime error cannot inject terminal controls or forge another line. + run bash -c "basecamp todos create --jq 'error(\"\\u001b[31mPWN\\u001b[0m\\nforged\")' 2>&1 >/dev/null < /dev/null" + assert_failure + [ "${#lines[@]}" -eq 1 ] + [[ "$output" != *$'\033'* ]] assert_output_contains "--jq" + assert_output_contains "PWN forged" } @test "an unparseable jq filter still renders an error raised before validation" { diff --git a/internal/cli/cobra_error_test.go b/internal/cli/cobra_error_test.go index 172ba6a66..7ca7f7866 100644 --- a/internal/cli/cobra_error_test.go +++ b/internal/cli/cobra_error_test.go @@ -108,3 +108,18 @@ func TestJQUsable(t *testing.T) { }) } } + +func TestJQRenderErrorDiagnosticIsTerminalSafeAndSingleLine(t *testing.T) { + err := output.ErrJQRuntime(errors.New("error: \x1b[31mPWN\x1b[0m\r\nforged\tline \x1b]8;;https://evil.example\aLINK\x1b]8;;\a \u009b31mC1\u009b0m")) + + got := jqRenderErrorDiagnostic(err) + + assert.NotContains(t, got, "\x1b") + assert.NotContains(t, got, "\r") + assert.NotContains(t, got, "\n") + assert.NotContains(t, got, "\t") + assert.NotContains(t, got, "\u009b") + assert.Contains(t, got, "PWN") + assert.Contains(t, got, "forged line LINK") + assert.Contains(t, got, "C1") +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 1d0c6eb3e..745791be5 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -22,6 +22,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/harness" "github.com/basecamp/basecamp-cli/internal/hostutil" "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui" "github.com/basecamp/basecamp-cli/internal/version" @@ -410,7 +411,7 @@ func Execute() { // exists to serve. Once a jq-backed write has begun, stdout is // final: report the failure on stderr and stop. if jq, _ := cmd.PersistentFlags().GetString("jq"); jq != "" { - fmt.Fprintf(os.Stderr, "error rendering error output through --jq: %v\n", writeErr) + fmt.Fprintln(os.Stderr, jqRenderErrorDiagnostic(writeErr)) os.Exit(output.ExitCodeFor(apiErr.Code)) } @@ -489,6 +490,21 @@ func jqUsable(filter string) bool { return err == nil } +// jqRenderErrorDiagnostic renders a terminal-safe, single-line explanation of +// a failed jq-backed error write. A jq runtime error can include response data +// selected by the filter, so it must not reach stderr verbatim. +func jqRenderErrorDiagnostic(err error) string { + const prefix = "error rendering error output through --jq" + if err == nil { + return prefix + } + detail := richtext.SanitizeSingleLine(err.Error()) + if detail == "" { + return prefix + } + return prefix + ": " + detail +} + // resolveProfile determines which profile to use. // Resolution order: // 1. --profile / -P flag diff --git a/internal/commands/attach.go b/internal/commands/attach.go index c2061fbfb..ac62cdb04 100644 --- a/internal/commands/attach.go +++ b/internal/commands/attach.go @@ -97,15 +97,17 @@ func validateAttachPaths(paths []string) error { // uploadAttachments uploads each path and returns attachment references. // Sequential, fails on first error. func uploadAttachments(cmd *cobra.Command, app *appctx.App, paths []string) ([]richtext.AttachmentRef, error) { + // Callers that read stdin run this first; repeating it here is idempotent + // and keeps this function correct on its own. + if err := validateAttachPaths(paths); err != nil { + return nil, err + } + refs := make([]richtext.AttachmentRef, 0, len(paths)) for _, path := range paths { normalized := richtext.NormalizeDragPath(path) - if err := richtext.ValidateFile(normalized); err != nil { - return nil, fmt.Errorf("%s: %w", path, err) - } - contentType := richtext.DetectMIME(normalized) filename := filepath.Base(normalized) diff --git a/internal/commands/cards.go b/internal/commands/cards.go index 8db523523..c15a58cf1 100644 --- a/internal/commands/cards.go +++ b/internal/commands/cards.go @@ -877,6 +877,9 @@ Use - as the body argument to read the body from stdin: if column != "" && !isNumericID(column) && *cardTable == "" { return output.ErrUsage("--card-table is required when using --column with a name") } + if err := requireNumericID(*cardTable, "card table ID"); err != nil { + return err + } if err := validateAttachPaths(attachFiles); err != nil { return err } @@ -896,12 +899,6 @@ Use - as the body argument to read the body from stdin: return err } - // Column name (non-numeric) requires --card-table for resolution - // Numeric column IDs can be used directly without card table discovery - if column != "" && !isNumericID(column) && *cardTable == "" { - return output.ErrUsage("--card-table is required when using --column with a name") - } - // Resolve project, with interactive fallback projectID := *project if projectID == "" { @@ -1110,6 +1107,12 @@ You can pass either a card ID or a Basecamp URL: return output.ErrUsage("Invalid card ID") } + // Attachment paths are readable or not regardless of the body, so + // check them before the pipe is drained. + if err := validateAttachPaths(attachFiles); err != nil { + return err + } + // Syntactic checks first, then "-", then account and network: a // malformed ID is answered without waiting on the producer, and a // blank pipe cannot mask it. @@ -2056,6 +2059,10 @@ func newCardsColumnCreateCmd(project, cardTable *string) *cobra.Command { app := appctx.FromContext(cmd.Context()) + if err := requireNumericID(*cardTable, "card table ID"); err != nil { + return err + } + description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err diff --git a/internal/commands/chat.go b/internal/commands/chat.go index 551932695..65e404657 100644 --- a/internal/commands/chat.go +++ b/internal/commands/chat.go @@ -832,27 +832,14 @@ edit to rich text.`, return output.ErrUsage(fmt.Sprintf("unsupported --content-type %q (expected text/html or text/plain)", ct)) } - var contentErr error - messageContent, contentErr = resolveContentValue(cmd, messageContent, argIndex, what) - if contentErr != nil { - return contentErr - } - - if strings.TrimSpace(messageContent) == "" { - return missingArg(cmd, "<content>") - } - - if err := ensureAccount(cmd, app); err != nil { - return err - } - - // Resolve the line reference. A bare numeric ID falls through to - // --room/dock resolution; a URL must be a chat-line URL on a trusted - // host so a pasted card/todo/message URL — or a look-alike on an - // attacker-controlled host — can't be misinterpreted into an edit. + // Resolve the line reference before the read: the URL host and + // shape, an explicitly supplied --room, and a bare numeric line ID + // are all decidable from the arguments and the configured base URL, + // so a bad reference must not cost the caller a drained pipe. lineID := args[0] urlChatID := "" urlProjectID := "" + var parsedURL *urlarg.Parsed if urlarg.IsURL(args[0]) { if !hostutil.IsTrustedBasecampHost(args[0], app.Config.BaseURL) { return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") @@ -865,15 +852,41 @@ edit to rich text.`, if parsed == nil || parsed.Type != "lines" || parsed.IsCollection { return output.ErrUsage("expected a chat-line ID or URL of the form /chats/{c}/lines/{l} or /chats/{c}@{l}") } - // Guard against editing in the wrong account: the URL names an - // account, and if it disagrees with the configured one the safe - // move is to stop rather than silently target a different account. - if parsed.AccountID != "" && app.Config.AccountID != "" && parsed.AccountID != app.Config.AccountID { - return output.ErrUsage(fmt.Sprintf("URL account %s does not match the configured account %s", parsed.AccountID, app.Config.AccountID)) - } lineID = parsed.RecordingID urlChatID = parsed.CampfireID urlProjectID = parsed.ProjectID + parsedURL = parsed + } + if _, err := strconv.ParseInt(lineID, 10, 64); err != nil { + return output.ErrUsage("Invalid chat line ID") + } + if *chatID != "" { + if _, err := strconv.ParseInt(*chatID, 10, 64); err != nil { + return output.ErrUsage("Invalid chat room ID") + } + } + + var contentErr error + messageContent, contentErr = resolveContentValue(cmd, messageContent, argIndex, what) + if contentErr != nil { + return contentErr + } + + if strings.TrimSpace(messageContent) == "" { + return missingArg(cmd, "<content>") + } + + if err := ensureAccount(cmd, app); err != nil { + return err + } + + // The URL's host and shape were settled before the read; only the + // account comparison had to wait for a resolved account. Editing in + // the wrong account is worth stopping for rather than silently + // targeting a different one. + if parsedURL != nil && parsedURL.AccountID != "" && app.Config.AccountID != "" && + parsedURL.AccountID != app.Config.AccountID { + return output.ErrUsage(fmt.Sprintf("URL account %s does not match the configured account %s", parsedURL.AccountID, app.Config.AccountID)) } // Resolve the chat (campfire) ID, and a project only when needed. The diff --git a/internal/commands/files.go b/internal/commands/files.go index 0fe03cca2..c371e81cd 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -1269,6 +1269,9 @@ Use - as the content argument to read the document body from stdin: if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { return err } + if err := requireNumericID(*vaultID, "folder ID"); err != nil { + return err + } // Attachment paths are readable or not regardless of the body, so // check them before the pipe is drained. @@ -1766,12 +1769,31 @@ You can pass either an upload ID or a Basecamp URL: return fmt.Errorf("%s: %w", filePath, err) } - // The trusted-host check needs only the configured base URL, so it - // runs here rather than with the account-identity checks below — - // refusing a look-alike host must not cost the caller a drained - // pipe. The full rationale for the check is at its sibling below. - if urlarg.IsURL(args[0]) && !hostutil.IsTrustedBasecampHost(args[0], app.Config.BaseURL) { - return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") + // A URL-shaped argument must live on a trusted Basecamp host: the + // URL router is host-agnostic, so a look-alike on an + // attacker-controlled host would otherwise pass the identity checks + // and retarget the configured account's upload — the confused-deputy + // case hostutil exists to prevent. That, and whether the URL names a + // single upload at all, need only the configured base URL, so they + // run before the read; only the account comparison waits. + var parsedURL *urlarg.Parsed + if urlarg.IsURL(args[0]) { + if !hostutil.IsTrustedBasecampHost(args[0], app.Config.BaseURL) { + return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") + } + parsedURL = urlarg.Parse(args[0]) + } + if parsedURL != nil { + if parsedURL.Type != "uploads" { + return output.ErrUsage(fmt.Sprintf("URL identifies a %s recording, not an upload", parsedURL.Type)) + } + // A collection URL (/vaults/456/uploads, /buckets/456/uploads) + // also parses as type "uploads", but its extracted ID is the + // PARENT's — the identity predicate needs the recording half + // too, or extractID retargets a same-numbered upload. + if parsedURL.IsCollection || parsedURL.RecordingID == "" { + return output.ErrUsage("URL identifies an uploads listing, not a single upload") + } } // Syntactic checks first, then "-", then account and network: a @@ -1791,30 +1813,10 @@ You can pass either an upload ID or a Basecamp URL: // A pasted URL names its own account; refuse one that isn't the // session's before anything is staged — extractID keeps only the // numeric ID, which would silently retarget the configured - // account's same-numbered upload on a mutating request. - // A URL-shaped argument must live on a trusted Basecamp host: - // the URL router is host-agnostic, so a look-alike on an - // attacker-controlled host would otherwise pass the identity - // checks below and retarget the configured account's upload — - // the confused-deputy case hostutil exists to prevent. - if urlarg.IsURL(args[0]) && !hostutil.IsTrustedBasecampHost(args[0], app.Config.BaseURL) { - return output.ErrUsage("refusing untrusted host in URL — expected a Basecamp URL") - } - - if parsed := urlarg.Parse(args[0]); parsed != nil { - if parsed.AccountID != "" && parsed.AccountID != app.Config.AccountID { - return output.ErrUsage(fmt.Sprintf("URL is for account %s, but this session uses account %s", parsed.AccountID, app.Config.AccountID)) - } - if parsed.Type != "uploads" { - return output.ErrUsage(fmt.Sprintf("URL identifies a %s recording, not an upload", parsed.Type)) - } - // A collection URL (/vaults/456/uploads, /buckets/456/uploads) - // also parses as type "uploads", but its extracted ID is the - // PARENT's — the identity predicate needs the recording half - // too, or extractID retargets a same-numbered upload. - if parsed.IsCollection || parsed.RecordingID == "" { - return output.ErrUsage("URL identifies an uploads listing, not a single upload") - } + // account's same-numbered upload on a mutating request. Host and + // shape were settled before the read; only this needed the account. + if parsedURL != nil && parsedURL.AccountID != "" && parsedURL.AccountID != app.Config.AccountID { + return output.ErrUsage(fmt.Sprintf("URL is for account %s, but this session uses account %s", parsedURL.AccountID, app.Config.AccountID)) } // Resolve the description first: its local-image references can diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 2d642c0f5..4c21a10fb 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -148,6 +148,21 @@ func dockToolNotFoundError(all []DockTool, dockName, projectID, friendlyName str // // When exactly one tool exists, its ID is returned. // When no tools of the type exist, a not found error is returned. +// requireNumericID rejects an explicitly supplied dock or container ID that is +// not numeric. getDockToolID returns an explicit value verbatim — there is no +// name resolution for these — so the check needs neither an account nor the +// network, and commands that also read a "-" input run it before the read +// rather than discovering it after a request is already being built. +func requireNumericID(value, label string) error { + if value == "" { + return nil + } + if _, err := strconv.ParseInt(value, 10, 64); err != nil { + return output.ErrUsage("Invalid " + label) + } + return nil +} + func getDockToolID(ctx context.Context, app *appctx.App, projectID, dockName, explicitID, friendlyName, flagName string) (string, error) { // If explicit ID provided, use it directly if explicitID != "" { diff --git a/internal/commands/messages.go b/internal/commands/messages.go index 33d4dd558..3e6c1b94c 100644 --- a/internal/commands/messages.go +++ b/internal/commands/messages.go @@ -464,6 +464,9 @@ Use - as the body argument to read the body from stdin: if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { return err } + if err := requireNumericID(*messageBoard, "message board ID"); err != nil { + return err + } // Attachment paths are readable or not regardless of the body, so // check them before the pipe is drained. if err := validateAttachPaths(attachFiles); err != nil { diff --git a/internal/commands/schedule.go b/internal/commands/schedule.go index 6935883ac..31f7a5e95 100644 --- a/internal/commands/schedule.go +++ b/internal/commands/schedule.go @@ -444,6 +444,9 @@ func newScheduleCreateCmd(project, scheduleID *string) *cobra.Command { if err := rejectSubscribeConflict(cmd.Flags().Changed("subscribe"), noSubscribe); err != nil { return err } + if err := requireNumericID(*scheduleID, "schedule ID"); err != nil { + return err + } // Attachment paths are readable or not regardless of the body, so // check them before the pipe is drained. @@ -706,16 +709,10 @@ You can pass either an entry ID or a Basecamp URL: hasChanges = true } if startsAt != "" { - if err := validateScheduleTimestamp("starts-at", startsAt); err != nil { - return err - } req.StartsAt = basecamp.Ptr(startsAt) hasChanges = true } if endsAt != "" { - if err := validateScheduleTimestamp("ends-at", endsAt); err != nil { - return err - } req.EndsAt = basecamp.Ptr(endsAt) hasChanges = true } diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index 045736ba0..50889da20 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -419,6 +419,28 @@ func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { []string{"update", "123", "--type", "vault", "--content", "-"}, "--content can only be used"}, {"schedule bad timestamp", NewScheduleCmd, []string{"update", "123", "--starts-at", "invalid", "--description", "-"}, "starts-at"}, + {"cards update bad attachment", NewCardsCmd, + []string{"update", "1", "--body", "-", "--attach", "/nope/missing.png"}, "missing.png"}, + {"chat update untrusted host", NewChatCmd, + []string{"update", "https://evil.example/1/buckets/2/chats/3/lines/4", "-"}, "untrusted host"}, + {"chat update wrong recording type", NewChatCmd, + []string{"update", "https://3.basecamp.com/1/buckets/2/todos/4", "-"}, "expected a chat-line"}, + {"chat update bad line id", NewChatCmd, + []string{"update", "nope", "-"}, "Invalid chat line ID"}, + {"files replace wrong recording type", NewFilesCmd, + []string{"replace", "https://3.basecamp.com/1/buckets/2/todos/4", "stdin.go", "--description", "-"}, "not an upload"}, + {"schedule create bad schedule id", NewScheduleCmd, + []string{"create", "Title", "--starts-at", "2026-01-01T10:00:00Z", "--ends-at", "2026-01-01T11:00:00Z", "--schedule", "nope", "--description", "-"}, "Invalid schedule ID"}, + {"messages create bad board id", NewMessagesCmd, + []string{"create", "Title", "-", "--message-board", "nope"}, "Invalid message board ID"}, + {"docs create bad folder id", NewDocsCmd, + []string{"documents", "create", "Title", "-", "--folder", "nope"}, "Invalid folder ID"}, + {"todolists create bad todoset id", NewTodolistsCmd, + []string{"create", "Title", "--description", "-", "--todoset", "nope"}, "Invalid todoset ID"}, + {"cards column create bad table id", NewCardsCmd, + []string{"column", "create", "Title", "--description", "-", "--card-table", "nope"}, "Invalid card table ID"}, + {"todos create bad todoset id", NewTodosCmd, + []string{"create", "-", "--todoset", "nope"}, "Invalid todoset ID"}, {"schedule bad entry id", NewScheduleCmd, []string{"update", "nope", "--description", "-"}, "Invalid schedule entry ID"}, {"uploads unreadable file", NewUploadsCmd, diff --git a/internal/commands/stdin_ordering_test.go b/internal/commands/stdin_ordering_test.go index 006883231..ed29d8a93 100644 --- a/internal/commands/stdin_ordering_test.go +++ b/internal/commands/stdin_ordering_test.go @@ -48,29 +48,38 @@ func TestSyntacticArgChecksPrecedeStdinReads(t *testing.T) { return true } - firstStdinRead, firstArgCheck := token.NoPos, token.NoPos + firstStdinRead := token.NoPos + var lateChecks []token.Pos ast.Inspect(kv.Value, func(inner ast.Node) bool { call, ok := inner.(*ast.CallExpr) if !ok { return true } switch { - case stdinResolver(call) && !firstStdinRead.IsValid(): - firstStdinRead = call.Pos() - case syntacticArgUse(call) && !firstArgCheck.IsValid(): - firstArgCheck = call.Pos() + case stdinResolver(call): + if !firstStdinRead.IsValid() { + firstStdinRead = call.Pos() + } + case deterministicCheck(call): + // Every recognized check must precede the read, not just + // the earliest one: comparing first-to-first lets a single + // early check hide every later one. + if firstStdinRead.IsValid() && call.Pos() > firstStdinRead { + lateChecks = append(lateChecks, call.Pos()) + } } return true }) - if !firstStdinRead.IsValid() || !firstArgCheck.IsValid() { + if !firstStdinRead.IsValid() { return true } checked++ - assert.Less(t, int(firstArgCheck), int(firstStdinRead), - "%s: this command reads stdin at %s before validating its arguments at %s — "+ - "hoist the argument check above the resolver", - name, fset.Position(firstStdinRead), fset.Position(firstArgCheck)) + for _, late := range lateChecks { + assert.Fail(t, "stdin is read before a check that decides the invocation", + "%s: reads stdin at %s but checks its arguments at %s — hoist the check above the resolver", + name, fset.Position(firstStdinRead), fset.Position(late)) + } return true }) } @@ -84,10 +93,36 @@ func stdinResolver(call *ast.CallExpr) bool { return ok && (name.Name == "resolveContentValue" || name.Name == "resolveContentArg") } -// syntacticArgUse matches a call that derives something from args without any -// account, config, or network dependency — the checks that must come first. +// deterministicCheck matches a call that can decide the invocation without the +// network: a syntactic use of args, or one of the local validation helpers. // Recognizing a form by name is exactly as wide as the names listed; a new // helper needs adding here, which is why the coverage claim above is bounded. +// +// Normalizers are deliberately absent. dateparse.Parse and isNumericID cannot +// fail, so they decide nothing on their own — listing them would report call +// sites that are branch selection rather than validation, and the noise would +// be answered by suppressions instead of fixes. +func deterministicCheck(call *ast.CallExpr) bool { + if id, ok := call.Fun.(*ast.Ident); ok { + switch id.Name { + case "validateAttachPaths", "validateUploadPath", "validateScheduleTimestamp", + "rejectSubscribeConflict", "rejectForeignAPIPath": + return true + } + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok { + if pkg, ok := sel.X.(*ast.Ident); ok { + switch pkg.Name + "." + sel.Sel.Name { + case "hostutil.IsTrustedBasecampHost", "urlarg.Parse", "urlarg.IsURL": + return true + } + } + } + return syntacticArgUse(call) +} + +// syntacticArgUse matches a call that derives something from args without any +// account, config, or network dependency. func syntacticArgUse(call *ast.CallExpr) bool { name, ok := call.Fun.(*ast.Ident) if !ok { diff --git a/internal/commands/todolists.go b/internal/commands/todolists.go index b6c271e30..dbd20f635 100644 --- a/internal/commands/todolists.go +++ b/internal/commands/todolists.go @@ -288,6 +288,10 @@ func newTodolistsCreateCmd(project, todosetID *string) *cobra.Command { return fmt.Errorf("app not initialized") } + if err := requireNumericID(*todosetID, "todoset ID"); err != nil { + return err + } + description, err := resolveContentValue(cmd, description, -1, "--description") if err != nil { return err diff --git a/internal/commands/todos.go b/internal/commands/todos.go index 5c3206d49..560930639 100644 --- a/internal/commands/todos.go +++ b/internal/commands/todos.go @@ -1276,6 +1276,9 @@ Use - as the content argument to read the todo title from stdin: if err := validateAttachPaths(attachFiles); err != nil { return err } + if err := requireNumericID(todoset, "todoset ID"); err != nil { + return err + } content, err := resolveContentArg(cmd, args, 0) if err != nil { From a6bcf291adebcae11594896a23c30ce2f329e527 Mon Sep 17 00:00:00 2001 From: Jeremy Daer <jeremy@37signals.com> Date: Thu, 20 Aug 2026 17:58:08 -0700 Subject: [PATCH 19/19] Close the upload folder-ID hole and make the backstop able to see it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit uploads create and the top-level upload shortcut read --description - before rejecting a malformed --folder, so a doomed invocation drained the producer first. Both now validate it before the read. Two backstop corrections, without which that class stays invisible: requireNumericID was not in the recognized set, so moving one after a read reported nothing — verified by doing exactly that, which now fails the check. And urlarg.IsURL is out: like isNumericID it only selects a branch and cannot reject an invocation, so listing it reports branch selection as validation. requireNumericID was also sitting inside getDockToolID's doc comment, orphaning half of it. Moved above the block. --- internal/commands/files.go | 6 ++++++ internal/commands/helpers.go | 18 +++++++++--------- internal/commands/stdin_integration_test.go | 4 ++++ internal/commands/stdin_ordering_test.go | 10 +++++----- 4 files changed, 24 insertions(+), 14 deletions(-) diff --git a/internal/commands/files.go b/internal/commands/files.go index c371e81cd..b00a9f417 100644 --- a/internal/commands/files.go +++ b/internal/commands/files.go @@ -891,6 +891,9 @@ as an upload in the target folder (vault).`, if err != nil { return err } + if err := requireNumericID(*vaultID, "folder ID"); err != nil { + return err + } description, err = resolveContentValue(cmd, description, -1, "--description") if err != nil { return err @@ -929,6 +932,9 @@ attachment and then created as an upload in the target folder.`, if err != nil { return err } + if err := requireNumericID(vaultID, "folder ID"); err != nil { + return err + } description, err = resolveContentValue(cmd, description, -1, "--description") if err != nil { return err diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 4c21a10fb..d4164d4e4 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -139,15 +139,6 @@ func dockToolNotFoundError(all []DockTool, dockName, projectID, friendlyName str return output.ErrNotFoundHint(friendlyName, projectID, fmt.Sprintf("Project has no %s", friendlyName)) } -// getDockToolID retrieves a dock tool ID from a project, handling the multi-dock case. -// -// When multiple tools of the same type exist in the project: -// - If explicitID is provided, it is returned as-is -// - Otherwise, an error is returned listing the available tools; -// if flagName is non-empty, the hint directs users to that flag -// -// When exactly one tool exists, its ID is returned. -// When no tools of the type exist, a not found error is returned. // requireNumericID rejects an explicitly supplied dock or container ID that is // not numeric. getDockToolID returns an explicit value verbatim — there is no // name resolution for these — so the check needs neither an account nor the @@ -163,6 +154,15 @@ func requireNumericID(value, label string) error { return nil } +// getDockToolID retrieves a dock tool ID from a project, handling the multi-dock case. +// +// When multiple tools of the same type exist in the project: +// - If explicitID is provided, it is returned as-is +// - Otherwise, an error is returned listing the available tools; +// if flagName is non-empty, the hint directs users to that flag +// +// When exactly one tool exists, its ID is returned. +// When no tools of the type exist, a not found error is returned. func getDockToolID(ctx context.Context, app *appctx.App, projectID, dockName, explicitID, friendlyName, flagName string) (string, error) { // If explicit ID provided, use it directly if explicitID != "" { diff --git a/internal/commands/stdin_integration_test.go b/internal/commands/stdin_integration_test.go index 50889da20..2332e422a 100644 --- a/internal/commands/stdin_integration_test.go +++ b/internal/commands/stdin_integration_test.go @@ -443,6 +443,10 @@ func TestDeterministicFailuresRejectedBeforeReadingStdin(t *testing.T) { []string{"create", "-", "--todoset", "nope"}, "Invalid todoset ID"}, {"schedule bad entry id", NewScheduleCmd, []string{"update", "nope", "--description", "-"}, "Invalid schedule entry ID"}, + {"uploads create bad folder id", NewUploadsCmd, + []string{"create", "stdin.go", "--folder", "nope", "--description", "-"}, "Invalid folder ID"}, + {"upload shortcut bad folder id", NewUploadCmd, + []string{"stdin.go", "--folder", "nope", "--description", "-"}, "Invalid folder ID"}, {"uploads unreadable file", NewUploadsCmd, []string{"create", "/nope/missing.txt", "--description", "-"}, "missing.txt"}, } { diff --git a/internal/commands/stdin_ordering_test.go b/internal/commands/stdin_ordering_test.go index ed29d8a93..8ec6ef00a 100644 --- a/internal/commands/stdin_ordering_test.go +++ b/internal/commands/stdin_ordering_test.go @@ -98,22 +98,22 @@ func stdinResolver(call *ast.CallExpr) bool { // Recognizing a form by name is exactly as wide as the names listed; a new // helper needs adding here, which is why the coverage claim above is bounded. // -// Normalizers are deliberately absent. dateparse.Parse and isNumericID cannot -// fail, so they decide nothing on their own — listing them would report call -// sites that are branch selection rather than validation, and the noise would +// Normalizers and branch selectors are deliberately absent. dateparse.Parse, +// isNumericID and urlarg.IsURL cannot reject an invocation, so listing them +// would report branch selection as if it were validation, and the noise would // be answered by suppressions instead of fixes. func deterministicCheck(call *ast.CallExpr) bool { if id, ok := call.Fun.(*ast.Ident); ok { switch id.Name { case "validateAttachPaths", "validateUploadPath", "validateScheduleTimestamp", - "rejectSubscribeConflict", "rejectForeignAPIPath": + "rejectSubscribeConflict", "rejectForeignAPIPath", "requireNumericID": return true } } if sel, ok := call.Fun.(*ast.SelectorExpr); ok { if pkg, ok := sel.X.(*ast.Ident); ok { switch pkg.Name + "." + sel.Sel.Name { - case "hostutil.IsTrustedBasecampHost", "urlarg.Parse", "urlarg.IsURL": + case "hostutil.IsTrustedBasecampHost", "urlarg.Parse": return true } }