Stdin - support everywhere sensible; usage error for stray - elsewhere

e2e/stdin_dash.bats

@@ -0,0 +1,149 @@
+#!/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 — 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
+
+
+# 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("<name>")' 'true'
+  assert_json_value '.hint | contains("after the -- separator")' 'true'
+}
+
+@test "a -- - after the separator passes the guard and lands literally" {
+  create_credentials
+  create_global_config '{"account_id": 99999}'
+
+  # 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' '-'
+}
+
+@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"
+}
+
+@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"
+}
+
+
+# --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, 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" {
+  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"
+}

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.

internal/cli/cobra_error_test.go

@@ -0,0 +1,125 @@
+package cli
+
+import (
+	"errors"
+	"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/output"
+)
+
+// Cobra's arity messages are usage failures by construction. They used to fall
+// 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{
+		"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)
+}
+
+// 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())
+}
+
+// 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))
+		})
+	}
+}
+
+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")
+}