Reject whitespace-only content in mutation commands by robzolkos · Pull Request #238 · basecamp/basecamp-cli

robzolkos · 2026-03-10T21:47:31Z

Summary

Whitespace-only input (e.g. --content " ") bypassed empty string validation
and hit the API, which returned malformed JSON causing an exit code 7: "unexpected end of JSON input" crash
Added strings.TrimSpace before emptiness checks across all content mutation
commands: campfire post, comment, messages create/update, todos create/todo,
cards create/update, and files update
Empty string validation (--content "") was already working correctly — this
closes the gap for whitespace-only strings

Summary by cubic

Reject whitespace-only content across create and update commands to prevent malformed API calls that crashed with exit code 7. We now trim only for validation, blocking whitespace-only input while preserving intentional whitespace in values sent to the API.

Bug Fixes
- Use strings.TrimSpace only in condition checks; do not reassign inputs.
- Behavior: create shows help for card, cards create, message, messages create, todo, todos create; returns usage errors for campfire post and comment. Updates treat whitespace-only as “no changes”; comment --edit still returns a usage error.
- Coverage: campfire posts, comments, messages (create/update + shortcut), todos (todo, todos create), cards (create/update + shortcut), files (update). Added 8 e2e tests for whitespace-only cases.

^{Written for commit 81c545b. Summary will update on new commits.}

cubic-dev-ai

1 issue found across 6 files

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/commands/campfire.go">

<violation number="1" location="internal/commands/campfire.go:268">
P2: `strings.TrimSpace` result is assigned back to `messageContent`, which silently strips leading/trailing whitespace from all messages before sending them to the API. Use `TrimSpace` only in the condition to reject whitespace-only input without altering valid content.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

internal/commands/campfire.go

Copilot

Pull request overview

This PR tightens CLI validation for mutation commands so whitespace-only inputs are treated as empty and rejected before hitting the Basecamp API (preventing malformed JSON responses and the resulting crash).

Changes:

Trim whitespace (strings.TrimSpace) on positional/flag content fields before emptiness checks across multiple commands.
Add additional “show help” guards when trimmed content becomes empty (e.g., todos/todo positional content, card/message titles).
Apply trimming on update commands (messages/cards/files) so whitespace-only --title/--body/--content doesn’t count as an update.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
internal/commands/todos.go	Trim and reject whitespace-only todo content for `todo` shortcut and `todos create`.
internal/commands/messages.go	Trim title/body for create/update; add help guard for whitespace-only titles/updates.
internal/commands/files.go	Trim title/content for `files update` before deciding whether to show help.
internal/commands/comment.go	Trim comment content before “content required” validation.
internal/commands/cards.go	Trim card title on create and trim title/body flags on update before validation.
internal/commands/campfire.go	Trim campfire post message content before validation.

Comments suppressed due to low confidence (1)

internal/commands/files.go:988

files update currently has no focused tests, and this new TrimSpace-based guard is important to prevent whitespace-only updates from reaching the API. Please add tests that --title " " / --content " " (with/without --type) results in help/usage and avoids making update requests.

			if strings.TrimSpace(title) == "" && strings.TrimSpace(content) == "" && itemType == "" {
				return cmd.Help()
			}

			app := appctx.FromContext(cmd.Context())

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/commands/messages.go

internal/commands/todos.go

internal/commands/campfire.go

internal/commands/cards.go

cubic-dev-ai

3 issues found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/commands/todos.go">

<violation number="1" location="internal/commands/todos.go:85">
P2: `content` is no longer trimmed before being sent to the API. The old code assigned the trimmed result to `content`, so downstream usage (line 144: `Content: content`) received clean input. Now only the validation check trims, but the untrimmed value is passed to `CreateTodoRequest`. Note that the old code already rejected whitespace-only input correctly, so this change only introduces a regression.

Keep the trimmed value and check it:</violation>
</file>

<file name="internal/commands/messages.go">

<violation number="1" location="internal/commands/messages.go:354">
P2: Whitespace-only `--title` or `--body` can still reach the API when the other flag has content. The guard only rejects the case where *both* are empty after trimming, but doesn't strip the individual values before building the request. Consider trimming the values sent to the request (not the validation variables), e.g. assigning `title = strings.TrimSpace(title)` and `body = strings.TrimSpace(body)` after the guard, so a whitespace-only `--title " "` becomes `""` in the `UpdateMessageRequest`.</violation>
</file>

<file name="internal/commands/cards.go">

<violation number="1" location="internal/commands/cards.go:447">
P1: Whitespace-only `--title` or `--body` still reaches the API when another flag (e.g. `--due`) is also set. The guard uses `strings.TrimSpace` but the later `if title != ""` / `if content != ""` checks don't, so `" "` passes through to the request.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

internal/commands/cards.go

internal/commands/todos.go

internal/commands/messages.go

cubic-dev-ai

6 issues found across 6 files (changes from recent commits).

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/commands/cards.go">

<violation number="1" location="internal/commands/cards.go:450">
P2: These reassignments strip legitimate whitespace from `title` and `content` before sending to the API, contradicting the PR's stated goal of preserving intentional whitespace. The emptiness guard above already catches whitespace-only input correctly — these lines should be removed.</violation>

<violation number="2" location="internal/commands/cards.go:741">
P2: The trimmed value is assigned to `title` and used for the API request, stripping intentional whitespace. Use the same pattern as `newCardsCreateCmd`: keep the original value and only use `TrimSpace` in the condition.</violation>
</file>

<file name="e2e/errors.bats">

<violation number="1" location="e2e/errors.bats:161">
P2: Whitespace-only input should produce the same structured JSON error as missing content, not silently show help. Today `basecamp todos create` fails with `{"error":"<content> required","code":"usage"}`, but `basecamp todos create " "` succeeds with help text. Assert `assert_failure` + `assert_json_value` here (and in the other four tests) so the behaviour is consistent and the error envelope is machine-readable.

(Based on your team's feedback about asserting the structured JSON error envelope using assert_json_value for both '.error' and '.code' instead of substring output checks.) [FEEDBACK_USED]</violation>
</file>

<file name="internal/commands/messages.go">

<violation number="1" location="internal/commands/messages.go:357">
P2: `title` and `body` are reassigned to trimmed values, stripping intentional whitespace from the API payload. The emptiness guard on line 354 already uses `TrimSpace` correctly — these reassignments should be removed so valid input like `--title "  Hello  "` reaches the API unmodified, consistent with `newMessagesCreateCmd`.</violation>

<violation number="2" location="internal/commands/messages.go:531">
P2: Assigning `strings.TrimSpace(args[0])` to `subject` strips whitespace from the value sent to the API. Use TrimSpace only in the emptiness check to preserve the original input, matching how `newMessagesCreateCmd` handles it.</violation>
</file>

<file name="internal/commands/files.go">

<violation number="1" location="internal/commands/files.go:981">
P2: These reassignments strip leading/trailing whitespace from the values actually sent to the API, contradicting the PR's stated intent to preserve original input. The emptiness check on line 978 already correctly uses `TrimSpace` in a read-only way. Remove these reassignments so the original user input is forwarded to the API.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

internal/commands/cards.go

e2e/errors.bats

internal/commands/messages.go

internal/commands/files.go

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

internal/commands/comment.go:303

Whitespace-only comment content is now rejected via strings.TrimSpace(content) == "", but there’s no test covering this behavior (existing e2e coverage in e2e/errors.bats only checks missing content, not whitespace-only). Adding an e2e case like basecamp comment 123 " " asserting a usage error would prevent regressions back to the API-crash path.

			if strings.TrimSpace(content) == "" {
				if edit {
					return output.ErrUsage("Comment content required")
				}
				return missingArg(cmd, "<content>")
			}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/commands/files.go

e2e/errors.bats

internal/commands/files.go

internal/commands/messages.go

internal/commands/cards.go

Whitespace-only input (e.g. --content " ") bypassed empty string validation and reached the API, causing malformed JSON responses. TrimSpace before checking emptiness across all content mutation commands: campfire post, comment, messages, todos, cards, and files.

Avoid mutating input values — use strings.TrimSpace only in the emptiness check so legitimate leading/trailing whitespace is preserved when sent to the API.

…tests Trim title/body/content after the noChanges guard in update commands (cards, messages, files) so whitespace-only values don't reach the API. Add whitespace validation to card and message shortcut commands. Add 7 e2e tests covering whitespace-only input across campfire, cards, todos, and messages commands.

TrimSpace is now only used in condition checks, never reassigned to the variable. This preserves intentional whitespace in values sent to the API. Also adds comment whitespace-only e2e test.

jeremy

Makes sense to give an early rejection here. We should also update the SDK so it gracefully handles the empty Rails response (assume it was blowing up trying to parse JSON from a 204 No Content blank response).

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-11T08:06:38Z

internal/commands/cards.go

+			if strings.TrimSpace(title) == "" && strings.TrimSpace(content) == "" && due == "" && assignee == "" {
 				return noChanges(cmd)
 			}


The new no-changes guard trims whitespace, but request construction below still checks title != "" / content != "". As a result, a whitespace-only --title " " or --body " " will still be sent when combined with another field (e.g. --due ...), so whitespace-only updates are not actually rejected. Consider using strings.TrimSpace(...) when deciding whether to set req.Title/req.Content so whitespace-only values are treated as “no input.”

Copilot · 2026-03-11T08:06:38Z

internal/commands/files.go

+			if strings.TrimSpace(title) == "" && strings.TrimSpace(content) == "" && itemType == "" {
 				return noChanges(cmd)
 			}



The new no-changes guard trims whitespace, but whitespace-only values can still be sent when itemType is specified (e.g. files update 789 --type vault --title " "). If the goal is to reject whitespace-only mutations, consider treating whitespace-only title/content as “not provided” when building the per-type update requests (i.e., avoid sending raw whitespace strings).

Suggested change

if strings.TrimSpace(title) == "" && strings.TrimSpace(content) == "" && itemType == "" {

return noChanges(cmd)

}

trimmedTitle := strings.TrimSpace(title)

trimmedContent := strings.TrimSpace(content)

if trimmedTitle == "" && trimmedContent == "" && itemType == "" {

return noChanges(cmd)

}

// Use normalized values for the rest of the function so that

// whitespace-only inputs are treated as "not provided".

title = trimmedTitle

content = trimmedContent

Copilot · 2026-03-11T08:06:38Z

e2e/errors.bats

+@test "todos create with whitespace-only content shows help" {
+  create_credentials
+  create_global_config '{"account_id": 99999, "project_id": 123}'
+
+  run basecamp todos create " "
+  assert_success
+  assert_output_contains "basecamp todos create"
+}
+
+@test "cards create with whitespace-only title shows help" {
+  create_credentials
+  create_global_config '{"account_id": 99999, "project_id": 123}'
+
+  run basecamp cards create " "
+  assert_success
+  assert_output_contains "basecamp cards create"
+}
+
+@test "messages create with whitespace-only title shows help" {
+  create_credentials
+  create_global_config '{"account_id": 99999, "project_id": 123}'
+
+  run basecamp messages create " "
+  assert_success
+  assert_output_contains "basecamp messages create"
+}
+
+@test "message with whitespace-only title shows help" {
+  create_credentials
+  create_global_config '{"account_id": 99999, "project_id": 123}'
+
+  run basecamp message " "
+  assert_success
+  assert_output_contains "basecamp message"


These new tests assert that whitespace-only required args show help and exit successfully. Because help-on-success bypasses the existing missingArg() behavior (which returns a structured usage error + non-zero exit in machine output), this makes invalid input look like success to scripts and differs from the existing “missing content” tests above. Consider asserting a usage error (JSON envelope) for whitespace-only required args instead, matching how missingArg() behaves for other invalid input.

Suggested change

@test "todos create with whitespace-only content shows help" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp todos create " "

assert_success

assert_output_contains "basecamp todos create"

}

@test "cards create with whitespace-only title shows help" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp cards create " "

assert_success

assert_output_contains "basecamp cards create"

}

@test "messages create with whitespace-only title shows help" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp messages create " "

assert_success

assert_output_contains "basecamp messages create"

}

@test "message with whitespace-only title shows help" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp message " "

assert_success

assert_output_contains "basecamp message"

@test "todos create with whitespace-only content shows error" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp todos create " "

assert_failure

assert_json_value '.error' '<content> required'

assert_json_value '.code' 'usage'

}

@test "cards create with whitespace-only title shows error" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp cards create " "

assert_failure

assert_json_value '.error' '<title> required'

assert_json_value '.code' 'usage'

}

@test "messages create with whitespace-only title shows error" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp messages create " "

assert_failure

assert_json_value '.error' '<title> required'

assert_json_value '.code' 'usage'

}

@test "message with whitespace-only title shows error" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp message " "

assert_failure

assert_json_value '.error' '<title> required'

assert_json_value '.code' 'usage'

Copilot · 2026-03-11T08:06:38Z

e2e/cards_columns_steps.bats

+@test "card with whitespace-only title shows help" {
+  create_credentials
+  create_global_config '{"account_id": 99999, "project_id": 123}'
+
+  run basecamp card " "
+  assert_success
+  assert_output_contains "basecamp card"


This test expects whitespace-only title to return help with exit 0. If the CLI intends to reject whitespace-only required args consistently (especially in piped/CI contexts), consider expecting a usage error (non-zero + JSON envelope) similar to the “card without title” case.

Suggested change

@test "card with whitespace-only title shows help" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp card " "

assert_success

assert_output_contains "basecamp card"

@test "card with whitespace-only title shows error" {

create_credentials

create_global_config '{"account_id": 99999, "project_id": 123}'

run basecamp card " "

assert_failure

assert_json_value '.error' '<title> required'

assert_json_value '.code' 'usage'

Copilot · 2026-03-11T08:06:39Z

internal/commands/todos.go

 			}
 			content := strings.Join(args, " ")
+			if strings.TrimSpace(content) == "" {
+				return cmd.Help()


Whitespace-only input is treated as missing content, but this path returns cmd.Help() directly. That bypasses the missingArg() helper’s machine-output behavior (JSON usage error + non-zero exit) and results in exit 0/help even in non-TTY/--json/--agent contexts. Consider using missingArg(cmd, "") here so interactive users still see help, while machine consumers get a structured usage error.

Suggested change

return cmd.Help()

return missingArg(cmd, "<content>")

Copilot · 2026-03-11T08:06:39Z

internal/commands/messages.go

 			}

+			if strings.TrimSpace(title) == "" {
+				return cmd.Help()


Whitespace-only titles are treated as missing input, but returning cmd.Help() directly bypasses missingArg()’s machine-output behavior (structured usage error + non-zero exit). Consider using missingArg(cmd, "<title>") here so scripts/--json/--agent runs don’t see a success exit code for invalid input.

Suggested change

return cmd.Help()

return missingArg(cmd, "<title>")

Copilot · 2026-03-11T08:06:39Z

internal/commands/messages.go

 			}
 			title := args[0]
+			if strings.TrimSpace(title) == "" {
+				return cmd.Help()


Same issue for the message shortcut: cmd.Help() on whitespace-only title bypasses missingArg() and will exit 0 in machine-output contexts. Use missingArg(cmd, "<title>") for consistent rejection semantics.

Suggested change

return cmd.Help()

return missingArg(cmd, "<title>")

Copilot · 2026-03-11T08:06:39Z

internal/commands/messages.go

+			if strings.TrimSpace(title) == "" && strings.TrimSpace(body) == "" {
 				return noChanges(cmd)
 			}



The new no-changes guard trims whitespace, but the update request still uses raw flag values. This means a whitespace-only title/body can still be sent if the other field is non-empty (e.g. --title " " --body "hi"), which contradicts “whitespace-only fields are treated as no input” and may still hit the API with invalid values. Consider building UpdateMessageRequest by only setting Subject/Content when strings.TrimSpace(...) != "" (similar to how the TUI updates only provided fields).

Copilot · 2026-03-11T08:06:40Z

internal/commands/cards.go


 			title := args[0]
+			if strings.TrimSpace(title) == "" {
+				return cmd.Help()


Whitespace-only titles are treated as missing input, but returning cmd.Help() bypasses missingArg() and will exit 0 in machine-output contexts. Using missingArg(cmd, "<title>") would keep interactive behavior while returning a structured usage error for scripts/--json/--agent.

Suggested change

return cmd.Help()

return missingArg(cmd, "<title>")

* origin/main: Polish boost display and copy (#262) Reject whitespace-only content in mutation commands (#238)

robzolkos requested a review from a team as a code owner March 10, 2026 21:47

Copilot AI review requested due to automatic review settings March 10, 2026 21:47

github-actions bot added commands CLI command implementations bug Something isn't working labels Mar 10, 2026

Copilot started reviewing on behalf of robzolkos March 10, 2026 21:47 View session

cubic-dev-ai bot reviewed Mar 10, 2026

View reviewed changes

internal/commands/campfire.go Outdated Show resolved Hide resolved

Copilot AI reviewed Mar 10, 2026

View reviewed changes

internal/commands/messages.go Show resolved Hide resolved

internal/commands/messages.go Show resolved Hide resolved

internal/commands/todos.go Show resolved Hide resolved

internal/commands/campfire.go Show resolved Hide resolved

internal/commands/cards.go Show resolved Hide resolved

cubic-dev-ai bot reviewed Mar 10, 2026

View reviewed changes

internal/commands/cards.go Show resolved Hide resolved

internal/commands/todos.go Show resolved Hide resolved

internal/commands/messages.go Show resolved Hide resolved

robzolkos force-pushed the rz/trim-whitespace-content branch from d5d3aaa to 35bc5ee Compare March 11, 2026 00:30

Copilot AI review requested due to automatic review settings March 11, 2026 00:40

github-actions bot added the tests Tests (unit and e2e) label Mar 11, 2026

Copilot started reviewing on behalf of robzolkos March 11, 2026 00:41 View session

cubic-dev-ai bot reviewed Mar 11, 2026

View reviewed changes

Copilot AI reviewed Mar 11, 2026

View reviewed changes

robzolkos requested a review from jeremy March 11, 2026 01:30

robzolkos added 3 commits March 11, 2026 00:41

Use TrimSpace in condition only, preserve original content

ccc9cad

Avoid mutating input values — use strings.TrimSpace only in the emptiness check so legitimate leading/trailing whitespace is preserved when sent to the API.

jeremy force-pushed the rz/trim-whitespace-content branch from 75b3b1d to 90ea750 Compare March 11, 2026 07:53

Remove TrimSpace reassignments, preserve original input for API calls

81c545b

TrimSpace is now only used in condition checks, never reassigned to the variable. This preserves intentional whitespace in values sent to the API. Also adds comment whitespace-only e2e test.

Copilot AI review requested due to automatic review settings March 11, 2026 07:57

jeremy force-pushed the rz/trim-whitespace-content branch from 90ea750 to 81c545b Compare March 11, 2026 07:57

Copilot started reviewing on behalf of jeremy March 11, 2026 07:58 View session

jeremy approved these changes Mar 11, 2026

View reviewed changes

jeremy merged commit 2b91ffd into main Mar 11, 2026
27 checks passed

jeremy deleted the rz/trim-whitespace-content branch March 11, 2026 08:04

Copilot AI reviewed Mar 11, 2026

View reviewed changes

jeremy added a commit that referenced this pull request Mar 11, 2026

Merge remote-tracking branch 'origin/main' into sdk-0.4.0

bad35e3

* origin/main: Polish boost display and copy (#262) Reject whitespace-only content in mutation commands (#238)

-			if strings.TrimSpace(title) == "" && strings.TrimSpace(content) == "" && itemType == "" {
-				return noChanges(cmd)
-			}
+			trimmedTitle := strings.TrimSpace(title)
+			trimmedContent := strings.TrimSpace(content)
+			if trimmedTitle == "" && trimmedContent == "" && itemType == "" {
+				return noChanges(cmd)
+			}
+			// Use normalized values for the rest of the function so that
+			// whitespace-only inputs are treated as "not provided".
+			title = trimmedTitle
+			content = trimmedContent

Conversation

robzolkos commented Mar 10, 2026 • edited by cubic-dev-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by cubic

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeremy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

robzolkos commented Mar 10, 2026 •

edited by cubic-dev-ai bot

Loading