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

[jeremy]

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 — - reads stdin on every content input

Content-kind positionals: comments create/update, checkins answer create/update, todos create (join-all pattern); messages create [body], cards create [body], docs create [content], chat post/update, boost create, notes set (exact-positional pattern).

Content flags: --data (api post/put), --body (messages/cards update), --content (chat, files update), --description (todos, schedule, projects, todolists, templates, gauges, cards column, uploads create, upload, files replace), --comment (todos sweep), --file (notes set).

Resolution lives in internal/commands/stdin.go. The shared vocabulary — the allow_dash annotation and pipe detection — is the new internal/stdinarg leaf package, because internal/cli needs it too: --agent help now auto-synthesizes a per-command note ("Pass - to read from stdin: [body], --description") from the annotation, so tier-1 coverage self-documents, including for future commands.

Tier 2 — stray literal - + piped stdin = usage error

A central guard covers every runnable command in the assembled tree (~380 commands, aliases and future ones included). On subcommands it wraps the Args validator: cobra runs ValidateArgs after flag parsing (so Changed and ArgsLenAtDash are live) but before the persistent pre-run chain, PreRunE, and required-flag validation — so a stray - is rejected before any lifecycle side effect and before a competing usage error can shadow it. It is not a PersistentPreRunE hook across the tree because cobra runs only the innermost one, and the agent hook already shadows the root's.

The root is the one exception: its Args must stay nil for cobra's legacyArgs unknown-subcommand check, so its guard hangs off the front of its own PersistentPreRunE, acting only when the root itself executes. That is still ahead of config loading, profile resolution and --jq validation.

When stdin is piped and an exact - appears anywhere not annotated (positional or string/stringArray flag value), the command fails with a usage error naming the offender, pointing at where it does accept stdin, and teaching the -- escape. On a TTY, a literal - stays legal everywhere. Two allowed - in one invocation can never both be satisfied, so that errors regardless of pipe state. --out - (attachments/files download) is exempted as the stdout idiom. Cobra's generated meta commands — help and the completion commands — are exempt too: they perform no Basecamp content write, and completion legitimately receives - as the word being completed (basecamp todos create -<TAB> runs basecamp __complete todos create -), so guarding it would break flag completion across the CLI.

Behavior changes

1. comments create 123 - extra — was a silent literal "- extra" comment, now a usage error.
2. - with TTY stdin — was hang-until-Ctrl-D, now an immediate usage error teaching the escapes: pipe it, heredoc (… - <<'EOF'), cat | … - (type + Ctrl-D), or --edit where it exists.
3. Bare-pipe auto-read removed from comments create and notes set — piped stdin without - now errors with a hint instead of being silently consumed. Corollary: an unclaimed pipe alongside a named source (generate | notes set --file x.md) is ignored rather than raising the old ambiguity error — pipes are only ever a source through an explicit -, uniformly.
4. notes set - (piped) — was a bogus "two sources" error, now works; notes set --file - — was ENOENT on a file named -, now stdin.
5. Piped scripts passing literal - as a title/name/path now error; -- is the documented escape. TTY usage unaffected.
6. Stdin content gets trailing newlines trimmed — Markdown doesn't care, but titles and boost's 16-rune limit do (printf '🎉\n' | boost create <id> - no longer burns a rune).

Flag for review

No --stdin flag. A precedent survey settled on - as the universal content-from-stdin idiom; --stdin in the wild means other things (git plumbing = list-of-items, kubectl = attach container stdin), and heredoc/cat | give interactive humans the classic TTY path with zero new surface. This was an open question during planning — veto welcome if you still want the flag.

Tests

• internal/stdinarg: annotation parsing, pipe detection (char-device TTY stand-in per the established edit_test.go seam).
• Resolver semantics table + -- escape through real parses.
• Guard: unlisted positional (projects create -), unlisted flag (todos update --title -), TTY passthrough, --out - exemption, double-dash rejection, --attach - alongside an allowed body, -- escape.
• Per-pattern integration through mock transports: messages create body -, api post --data -, todos create -, boost create - (+ over-limit stdin), todos update --description -, notes set both forms.
• New e2e/stdin_dash.bats: empty-pipe rejection, TTY no-hang, bare-pipe hint, tier-2 rejection with -- escape, -- passthrough — all pre-network, no cassette needed. (The planned "posts body against cassette" e2e isn't recordable without live credentials — the happypath cassette set is read-only — so wire-level posting is covered by the mock-transport integration tests instead.)

bin/ci green: fmt, vet, lint, unit, e2e, surface snapshot (no Use-string or flag renames, so no regen), skill drift, smoke coverage, provenance. SKILL.md's - idiom is generalized in the same PR — it previously over-promised; now it's true.

---

Summary by cubic

Adds uniform “-” (stdin) support to all content inputs and installs a dash guard that rejects stray “-” when stdin is piped, so pipes aren’t misinterpreted as literal content. Also fixes jq-backed output to avoid double writes on filter errors and classifies Cobra arity errors as usage.

• Coverage: accepts “-” on content positionals (comments create/update, check-ins answers, todos create, messages/cards/docs create body, chat post/update, boost create, notes set) and content flags (--data, --body, --content, --description, --comment, --file). --out - (download) keeps the stdout idiom. Cobra meta commands (help, __complete) are exempt.
• Guard and ordering: runs at Args-validation time, rejects two stdin consumers and empty stdin, and dedupes alias flags; the root is guarded in pre-run. Run syntactic/local checks and attachment path validation before reading stdin; TUIs require character‑device stdin and stdout. An AST backstop enforces this, and additional pre-read checks were hoisted (chat update line/room IDs and URL host/shape, files replace recording-type, schedule create ID parsing, cards update attachment validation, explicit dock IDs via numeric validation).
• Behavior changes: implicit bare‑pipe reads are removed; stdin is read only when “-” is present. Chat post/update now reject combining a positional message with --content. Messages/cards/docs create bound at two args to avoid draining stdin and dropping extras. Stdin content trims trailing CRLF/LF.
• Required actions:
  • Pass “-” wherever stdin should be consumed; the CLI never reads stdin implicitly.
  • For a literal “-” positional, use “--” to escape it. For flags with a literal “-”, run without piped stdin.

^{Written for commit 58ff6da. Summary will update on new commits.}

[Copilot]

Pull request overview

Standardizes explicit - stdin handling across content-bearing CLI commands and rejects ambiguous stray dashes.

Changes:

• Adds shared stdin resolution and command-tree guard logic.
• Enables stdin for supported positional arguments and flags.
• Adds unit, integration, E2E, agent-help, and skill documentation updates.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
skills/basecamp/SKILL.md	Documents stdin conventions.
internal/stdinarg/stdinarg.go	Adds annotations and pipe detection.
internal/stdinarg/stdinarg_test.go	Tests shared stdin utilities.
internal/commands/stdin.go	Implements resolution and dash guard.
internal/commands/stdin_test.go	Tests resolver behavior.
internal/commands/stdin_integration_test.go	Tests request-level stdin handling.
internal/commands/dash_guard_test.go	Tests central guard behavior.
internal/commands/api.go	Supports stdin JSON bodies.
internal/commands/attachments.go	Exempts stdout output syntax.
internal/commands/boost.go	Supports positional stdin content.
internal/commands/cards.go	Supports card content stdin.
internal/commands/chat.go	Supports chat content stdin.
internal/commands/checkins.go	Supports answer content stdin.
internal/commands/comment.go	Makes comment stdin explicit.
internal/commands/comment_test.go	Updates comment stdin tests.
internal/commands/commands_test.go	Installs guard in test tree.
internal/commands/files.go	Supports document/upload content stdin.
internal/commands/gauges.go	Supports description stdin.
internal/commands/helpers.go	Removes obsolete pipe reader.
internal/commands/messages.go	Supports message body stdin.
internal/commands/notes.go	Makes note stdin explicit.
internal/commands/notes_test.go	Tests explicit note sources.
internal/commands/projects.go	Supports description stdin.
internal/commands/schedule.go	Supports schedule description stdin.
internal/commands/templates.go	Supports template description stdin.
internal/commands/todolists.go	Supports todolist description stdin.
internal/commands/todos.go	Supports todo content and flag stdin.
internal/cli/root.go	Installs guard and generates agent notes.
e2e/stdin_dash.bats	Exercises CLI stdin behavior.

Suppressed comments (2)

internal/commands/stdin.go:203

• The documented -- escape cannot preserve a literal - used as a flag value. For example, with piped stdin, todos update 1 --title - is rejected, but moving - after -- makes it positional rather than the value of --title; --title=- is still detected by this guard. Please either define a workable flag-value escape (and test it) or avoid rejecting flag values, rather than directing users to an impossible invocation.

		hint := `For a literal "-", pass it after the -- separator`

internal/commands/stdin.go:191

• Changed alias flags that share one destination are double-counted from their final value. For example, templates update 1 --description text --desc - leaves both flag values reporting -, so this increments allowed twice and rejects the invocation even though only one dash was supplied. Count actual dash occurrences or model aliases as one input before enforcing the one-reader rule.

		if allow.Flag(f.Name) {
			allowed += dashes

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b81a070f07

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jeremy]

Addressed the advisory in cfb9959. Per finding:

1 (guard timing) — fixed. The guard now wraps each runnable command's Args validator instead of RunE: cobra runs ValidateArgs after flag parsing (so Changed/ArgsLenAtDash are live) but before the persistent pre-run chain, PreRunE, and required-flag validation, so the stray-dash error fires before any lifecycle side effect or competing usage error. One discovery en route: the root's nil Args is load-bearing — cobra's Find() applies legacyArgs (unknown-subcommand rejection) only while Args == nil, and wrapping the root turned basecamp unknowncmd into a successful quickstart run (core.bats caught it). The root stays unwrapped, losing nothing: its positionals are subcommand names, and a bare basecamp - runs quickstart, which posts no content. New test pins the ordering (guard beats ExactArgs, PreRunE, and MarkFlagRequired).

2 (alias false-positive) — fixed. pflag hands aliases sharing a backing variable the same Value instance, so the guard now dedupes on Value identity: --description old --desc - is one logical stdin input (reads stdin), and --desc - --description old resolves to the literal old. Both orders tested; covers schedule, templates, and every other alias pair for free.

3 (impossible escape) — fixed. --name=- can't be the explicit-literal form — the guard sees only the parsed value, and special-casing the = spelling would need re-scanning os.Args. So the hint is now honest per offender kind: -- is mentioned only for positional offenders; flag offenders get the real remedy, run without piped stdin (</dev/tty). SKILL.md matches.

4 (newlines) — CRLF fixed; the per-input trim policy declined. Trailing \r\n is now trimmed alongside \n (test: a 16-rune boost followed by CRLF passes). But I'm keeping the uniform trailing-newline trim rather than classifying inputs as body-like vs title-like: only trailing newlines are touched (interior breaks preserved, so chat's text/plain "line breaks preserved" promise holds), Markdown→HTML conversion makes trailing newlines invisible for every rich-text body, and a per-site trim knob across ~30 call sites buys correctness only for the case of a chat message whose trailing blank lines are deliberate — which a trailing newline in a pipe almost never is. The uniform rule is also what SKILL.md documents. Happy to revisit if a real case surfaces.

5 (weak e2e) — fixed. The final case is now a deterministic local success — printf 'x' | basecamp config set project_id --json -- - stores a literal -, read back via config show — and the file header's no-network claim is now true.

bin/ci green end to end after the changes.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

Suppressed comments (3)

internal/commands/messages.go:457

• This new stdin path still accepts extra positionals silently. For example, printf body | basecamp messages create Title - unexpected reads stdin and posts the message while dropping unexpected, even though Use declares only <title> [body]. Add a maximum-argument validator so malformed stdin invocations fail instead of losing input.

			// Validate user input first, before checking account. The --edit
			// exclusion runs before "-" resolution so --edit … - errors
			// without consuming stdin.

internal/commands/cards.go:875

• Extra positionals remain silently ignored on the new stdin path: printf body | basecamp cards create Title - unexpected consumes stdin but drops unexpected. Since this command declares exactly <title> [body], cap it at two arguments before resolving -.

				var err error
				content, err = resolveContentValue(cmd, args[1], 1, "[body]")
				if err != nil {
					return err

internal/commands/files.go:1252

• The new - resolver only examines args[1], so printf body | basecamp docs create Title - unexpected succeeds and silently discards unexpected. Enforce the two positionals declared by Use before consuming stdin.

				var contentErr error
				content, contentErr = resolveContentValue(cmd, args[1], 1, "[content]")
				if contentErr != nil {
					return contentErr
				}

[jeremy]

Round 2 addressed in b8f09d7. Both mediums accepted, the low-priority tightening taken, plus one adjacent fix the arity bound exposed.

1. TTY hints are invalid for flag-based stdin — fixed.

stdinEscapeHint now takes the what it was given and repeats the input that actually carried the -. Verified against your exact repro:

$ basecamp api post /foo --data - --json </dev/null
"hint": "Pipe the content (printf '...' | basecamp api post ... --data -), use a heredoc
         (basecamp api post ... --data - <<'EOF'), or run cat | basecamp api post ... --data -
         and type the content, ending with Ctrl-D"

Positionals are unchanged (... -). Covered by a unit test that asserts the flag spelling is present and that a bare ... -) is absent, plus an integration test driving api post --data - through a real Execute with a transport that fails the test if any request escapes. Closes r3809916059.

2. Exact-positional consumers discard extra arguments — fixed, and the class is closed.

MaximumNArgs(2) on messages create, cards create, docs create. I audited every positional resolveContentValue call site rather than just the three you named: boost create (ExactArgs(2)), chat post (MaximumNArgs(1)), chat update (MaximumNArgs(2)), notes set (MaximumNArgs(1)) were already bounded. Those three were the whole remainder.

Validation-before-consumption is proven, not asserted: the test wires stdin to a reader that records whether Read was ever called and the SDK to a transport that counts calls, then runs create Title - unexpected on all three and asserts the arity error, read == false, and zero requests. The ordering holds structurally too — cobra runs ValidateArgs (guard wrapper → original validator) before RunE, so resolveContentValue is unreachable.

3. </dev/tty — removed.

Right; it is wrong on Windows and on headless runners with no controlling terminal. The hint is now For a literal "-" flag value, run the command without piped stdin, and SKILL.md matches. Kept the shape of the remedy, dropped the platform-specific spelling.

One adjacent fix, flag it if you want it split out. Adding the arity bound surfaced that cobra's arity errors were classified api_error (exit 7):

$ printf body | basecamp messages create Title - unexpected --json
{"ok": false, "error": "accepts at most 2 arg(s), received 3", "code": "api_error"}

That tells an agent to retry a call that can never succeed — the opposite of what this PR is for. transformCobraError already rewrites the received 0 case to a usage error; I extended it to the rest of the arity family, keeping cobra's wording (already clear) and fixing only the code. Now usage / exit 1. This is pre-existing and affects other commands too (chat post a b had the same envelope), so it is a behavior change beyond the stated scope — say the word and I will lift it into its own PR.

bin/ci green.

On the Go bump: agreed it is unrelated to this feature and belongs on main, not here. Worth knowing before someone attempts it as a one-liner: everything derives from go-version-file: go.mod, so the pin itself is one line — but the nix-build job exists precisely to catch a go.mod bump outpacing flake.lock (see the comment at test.yml:481, written after #533 did exactly that), so the change is go.mod + a nixpkgs carrying 1.26.6 + make update-nix-hash. Not mine to land from this branch; I will open it separately on request.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.

Suppressed comments (3)

internal/commands/chat.go:790

• The update path has the same silent stdin loss: printf intended | basecamp chat update 123 literal --content - chooses the positional value and never consumes the explicitly requested stdin input. Reject simultaneous positional content and --content before choosing the source.

			if len(args) > 1 {
				messageContent = args[1]
				argIndex, what = 1, "[content]"

internal/commands/chat.go:327

• When a positional message and --content - are both provided, this branch silently wins and the explicit stdin source is never read. For example, printf intended | basecamp chat post literal --content - passes the guard (there is only one -) but posts literal, discarding the pipe. Reject the two content sources together before selecting one.

This issue also appears on line 788 of the same file.

			if len(args) > 0 {
				messageContent = args[0]
				argIndex, what = 0, "<message>"

internal/commands/stdin.go:196

• Deduplicating shared flag values here can misname the offending alias because VisitAll is alphabetical, not invocation order. For example, --in old --project - leaves both aliases changed with the shared value -, but --in is visited first and reported even though --project carried the dash. Preserve/report the changed alias group so the diagnostic does not identify the wrong flag.

	// 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] {

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b8f09d7a7d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 42b536095f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.

[jeremy]

Addressed the body-level (suppressed) findings from the Copilot review rounds in 9310135:

• chat post/update dual content sources — fixed: a positional message combined with --content is now a usage error instead of the positional silently winning; with - in play the losing source would have discarded piped content unread. Unit-tested. (The messages/cards/docs extra-positional arity findings were already fixed in b8f09d7's bounded create arity.)
• Alias diagnostic may name the other spelling — not doing this: when both spellings of one aliased flag carry the dash, the guard names whichever alias VisitAll reaches first. Both names point at the same logical input the user just typed, so the diagnostic still identifies the right thing to fix; tracking invocation order through pflag to fix a cosmetic corner isn't worth the machinery.

Also in this round: pickers are now gated off when stdin is piped (mechanism fix for the read-ordering finding — details in that thread), and the docs create references now use the registered docs documents create path.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7aeaeddb8b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.

Suppressed comments (5)

internal/cli/root.go:450

• Pre-parsing cannot prevent runtime jq failures. A valid filter such as .data, error("stop") passes jqUsable, writeJQ emits the first result and then returns an error, and the existing fallback appends an unfiltered error envelope. Buffer jq output until iteration succeeds, or otherwise prevent the fallback once filtered output may have been written.

		// 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 = ""

internal/commands/chat.go:344

• chat update validates --content-type before reading stdin, but chat post does not. Thus chat post - --content-type bogus drains the pipe and proceeds toward a request with an unsupported content type instead of failing immediately; apply the same vocabulary check before this resolver.

			var err error
			messageContent, err = resolveContentValue(cmd, messageContent, argIndex, what)

internal/commands/schedule.go:644

• The locally decidable --starts-at/--ends-at checks still occur after this stdin read (and after account/project resolution). schedule update 1 --starts-at bogus --description - therefore drains or blocks on the pipe before returning the timestamp usage error. Move both timestamp validations ahead of resolveContentValue, as the create path already does.

			// Syntactic checks first, then "-", then account and network.
			description, err = resolveContentValue(cmd, description, -1, "--description")

internal/commands/cards.go:876

• The --column <name>/missing --card-table conflict is fully knowable from flags, but it is checked only after this stdin read and account setup. Consequently cards create Title - --column Backlog consumes the producer before returning the usage error. Hoist that conflict check above resolveContentValue.

			var content string
			if len(args) > 1 {
				var err error
				content, err = resolveContentValue(cmd, args[1], 1, "[body]")

internal/commands/todos.go:1578

• Due/start date validation remains below this stdin read and account resolution. For example, todos update 1 --due not-a-date --description - drains the pipe before returning Invalid due date, contrary to the fail-fast ordering introduced here. Parse and validate due/startsOn before resolving the description.

[jeremy]

Processed the remaining review bodies. Three of the five suppressed comments were live; two were already fixed in ac3f38e (schedule update timestamps, cards create named column). Fixes in ac3f38e and e4d545d.

chat post missing the --content-type check — correct, I had only done chat update. Now checked before the read, same vocabulary.

todos update date validation after the read — correct. --due/--starts-on are parsed and validated before the read, and the parsed values carried forward rather than re-derived below.

internal/cli/root_test.go depending on the runner's stdin — correct, and worth the detail: the exposure is not reachable through go test, which hands the test binary its own stdin, so piping into go test proves nothing. Running the compiled binary with a piped stdin is what reproduces it, and does:

$ printf 'x' | ./before.test -test.run IsInteractiveTTY
    root_test.go:307: Should be true — char-device stdio is interactive
FAIL
$ printf 'x' | ./after.test  -test.run IsInteractiveTTY
PASS

Both interactivity tests now stub stdout and stdin through a shared stubCharDeviceStdio. (My first commit message for this claimed I had verified the failure when I had only verified the pass; amended after actually checking, which is how the go test distinction above surfaced.)

internal/cli/root.go:450 — this one is stale rather than live. It says "the existing fallback appends an unfiltered error envelope", but the fallback was removed in the same change that added jqUsable; the code is now a single writer.Err(err) with no retry. So a valid-but-runtime-failing filter like .data, error("stop") writes what jq produced and nothing else — one output stream, which is the property asked for. Pre-parsing is not claimed to prevent runtime failures; it decides only whether the filter is usable at all, which is what the other half of that thread was about. Verified:

$ printf 'x' | basecamp --jq '.error, error("stop")' -
basecamp does not read stdin via "-" for argument 1        # one envelope, no duplicate

I did not buffer, as noted before: the terminal-injection sanitizer is TTY-gated on the writer, so buffering would silently disable escape stripping.

bin/ci green.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (4)

internal/commands/cards.go:1119

• cards update accepts --attach, but unlike the create path it resolves and drains --body - before checking whether attachment paths are readable. Thus producer | basecamp cards update 1 --body - --attach /missing waits for/consumes the producer and only then reports the already-known file error, contrary to the fail-before-stdin behavior added for attachment-bearing commands. Validate attachFiles before resolving the body.

			content, err := resolveContentValue(cmd, content, -1, "--body")

internal/commands/chat.go:830

• The target line is not validated until after this stdin read (the ParseInt/URL checks are at lines 843–925). Consequently, producer | basecamp chat update nope - drains the entire producer before returning Invalid line ID; malformed or foreign URLs do the same. Hoist the locally decidable line-reference validation ahead of resolveContentValue, as the other stdin-enabled update commands do.

			case "", "text/html", "text/plain":

skills/basecamp/SKILL.md:119

• This claim is broader than the implementation. For example, printf x | basecamp todos create --json returns missingArg, whose structured hint is only the usage line; check-in answers and boost have the same behavior. Only comments create and notes set explicitly detect a bare pipe and teach -, so agents following this skill will not receive the promised guidance elsewhere. Narrow the statement to those commands (or add the hint consistently).
  internal/commands/todos.go:1584
• Due/start-date validation still occurs after this stdin read at lines 1606–1618. An invocation such as producer | basecamp todos update 1 --description - --due nonsense therefore drains the producer before returning the deterministic invalid-date error. Parse and validate both date flags before resolving --description -, while retaining the parsed values for the request.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/commands/cards.go:1116

• Validate attachFiles before resolving --body. cards update 1 --body - --attach /missing currently drains (and can block on) stdin, then fails in uploadAttachments on a path that was already known to be unreadable. This is the one attachment-capable stdin command missing the new pre-read validation.

			content, err := resolveContentValue(cmd, content, -1, "--body")

internal/commands/chat.go:836

• Validate the target before reading stdin. The existing line-reference checks remain below this resolver, so chat update nope - --room 1 drains the producer before returning Invalid line ID; invalid or untrusted chat-line URLs do the same. Move the local URL/line-ID validation ahead of resolveContentValue, as the other stdin-enabled updates do.

			var contentErr error
			messageContent, contentErr = resolveContentValue(cmd, messageContent, argIndex, what)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e4d545d200

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Validate chat update targets before draining stdin

Unlike the fixed chat post path, chat update still reads the entire message before validating the target URL, line ID, or explicit --room. For example, producer | basecamp chat update https://evil.example/123 - or producer | basecamp chat update nope - --room nope can block or exhaust memory before reaching the deterministic errors at lines 856-872 and 925-931, while an empty producer masks the target error as “stdin is empty.” Parse and validate these argument-only target values before resolving the content.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reject all-invalid comment targets before reading content

When every comma-separated comment target is malformed, the invocation cannot create any comment, but this resolver drains stdin before extractIDs and the ParseInt loop at lines 1214 and 1254-1259. Thus producer | basecamp comments create nope - can block indefinitely or buffer an unbounded stream, and an empty producer reports the stdin error instead of the invalid target. Determine that at least one target is valid before consuming the content while preserving the existing partial-success behavior for mixed batches.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Validate explicit message-board IDs before reading the body

When --message-board is explicitly nonnumeric, this call drains the stdin body before the value is returned unchanged by getMessageBoardID and rejected by ParseInt at lines 523-530. For example, producer | basecamp messages create Title - --message-board nope --in 123 can block or exhaust memory before reporting the already-known invalid board ID, while an empty producer masks it as “stdin is empty.” Validate an explicitly supplied board ID before resolving the body.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Check replacement URL type before draining stdin

Fresh evidence beyond the earlier trusted-host fix is that a URL on the configured host can still deterministically identify the wrong recording type or an uploads collection, but those checks remain after this read at lines 1804-1816. For example, producer | basecamp files replace https://3.basecamp.com/123/buckets/456/todos/789 ./valid --description - can block or exhaust memory before reporting that the URL is not an upload, while an empty producer masks that error. Parse the URL and validate its type and collection status before resolving the description; only the account-identity comparison needs to remain after account resolution.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reject empty subscriptions before reading document content

Fresh evidence beyond the earlier mutual-exclusion fix is that an explicitly empty --subscribe "" is still guaranteed to fail in applySubscribeFlags, but docs documents create Title - --subscribe "" drains stdin here before that validation runs at lines 1294-1297. A slow or unbounded producer can therefore block before the deterministic “requires at least one person” error, and a blank producer masks it as “stdin is empty.” Extend the pre-read subscription validation to reject an explicitly changed but empty value; the messages and schedule create paths have the same ordering.

Useful? React with 👍 / 👎.

[jeremy]

Fixed in 27b8615. You were right, and I had fixed the wrong path.

The defect. I patched the fallback writer at the bottom of Execute and reasoned about it as though it were the only place a second write could happen. It isn't: the retry is in app.Err, above it. jqUsable was never able to prevent this — as you say, it only answers what is knowable before output exists — and my comment asserting the property was describing a path that wasn't the one at fault. Reproduced exactly as reported.

The fix, taking the invariant you stated: once a jq-backed write has begun, stdout is final. When app.Err fails and a filter was in play, the render failure goes to stderr and the process exits with the original code — no replay. Without a filter nothing partial can have been written through one, so the plain fallback stays as the last resort for a broken pipe.

$ basecamp todos create --jq '.error, error("stop")' 2>/dev/null
<content> required                                  # one document, exit 1

$ basecamp todos create --jq '.error, error("stop")' 2>&1 >/dev/null
error rendering error output through --jq: jq filter error: error: stop

I did not buffer. Your point that TTY state can be captured separately is fair and it is a workable design, but it means threading the destination's TTY-ness through output.Options so the sanitizer keeps working — a change to the output package's contract to fix a control-flow bug in Execute. Not replaying is the smaller correct change, and it is the invariant you named as the minimum. Say the word if you want atomic stdout instead and I will do the plumbing.

On the test. Your criticism was exact — it drove two writers directly and never reached Execute, so it asserted something true about the output package and nothing about the decision under review. Replaced with a predicate test for jqUsable (which is honestly all a unit test can cover here) plus an e2e case running the real binary that asserts stdout holds exactly one line. I verified it catches the regression: with the fix removed it fails on the line count, and passes with it restored.

jqUsable stays — it covers the other path, where no app exists yet because the error was raised before one was built — but its comment now describes only that path rather than implying a global property.

Scope note. The 110 uncommitted lines you saw were mid-round work; they landed as ac3f38e and e4d545d (the remaining pre-read validations, --attach paths across all eight commands, and two incomplete fixes of mine from the round before). Your verdict predates them, so they are unreviewed.

And agreed on io.ReadAll — it remains unbounded regardless of ordering, and a cap is a policy call rather than a review fix. Left for a separate decision.

bin/ci green, e2e now 11 cases.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/cli/root.go:467

• A valid filter can still fail at runtime on this no-app fallback path (for example, the dash guard can fail before app creation and .error, error("stop") writes once, then errors). The writer.Err result below is discarded, so unlike the app-backed path, that filter failure is never reported on stderr. Capture it and emit the same stderr diagnostic without retrying stdout.

		if jqFilter != "" && !jqUsable(jqFilter) {
			jqFilter = ""
		}

skills/basecamp/SKILL.md:119

• This parenthetical overstates current behavior. Most required-content paths still use the generic missingArg/Cobra error; for example, piped todos create --json reports only <content> required plus usage and does not mention -. Only the explicitly updated comments/notes paths provide that hint, so either add the hint consistently or remove this claim.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 27b86152f5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Validate card attachments before reading the body

Fresh evidence beyond the prior attachment-path thread is that newCardsUpdateCmd still reaches this stdin resolver without calling validateAttachPaths, despite the earlier fix claiming all affected commands were covered. With producer | basecamp cards update 123 --body - --attach missing.png, the command can block or buffer the entire producer before uploadAttachments reports the already-known missing-file error, while an empty producer masks it as “stdin is empty”; validate attachFiles before resolving --body, as the card-create path already does.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Validate the explicit todoset before reading the description

When --todoset is explicitly malformed, its value is returned unchanged by ensureTodoset and rejected by the ParseInt at lines 327-329, but this resolver consumes stdin first. Thus producer | basecamp todolists create Name --todoset nope --description - can block or buffer indefinitely before reporting the already-known invalid todoset ID, and an empty producer masks that error as “stdin is empty”; parse an explicitly supplied todoset ID before resolving the description.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Validate explicit folder IDs before consuming stdin

Fresh evidence beyond the prior upload-file validation is that a malformed explicit --folder or --vault still reaches this description resolver before runUploadFile parses the folder ID at lines 1036-1038. With a valid upload file, producer | basecamp uploads create ./report.pdf --folder nope --description - can therefore block or exhaust memory before returning the deterministic folder-ID error, while an empty producer reports the wrong stdin error; validate an explicitly supplied folder ID before resolving the description in both upload entry points (and before the analogous document-content read).

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Validate explicit card-table IDs before reading card bodies

Fresh evidence beyond the prior named-column requirement fix is that a nonnumeric explicit --card-table satisfies that pre-read check, so this resolver drains the body before getCardTableID rejects the table value. For example, producer | basecamp cards create Title - --column Backlog --card-table nope --in 123 can block or buffer indefinitely before reporting that the card table is invalid, while an empty producer masks it as “stdin is empty”; reject a syntactically invalid explicit card-table ID before resolving the body (and do the same before the stdin description in column creation).

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

internal/commands/chat.go:839

• The target is still validated only after this stdin read (and after account/project resolution at lines 845-929). Thus producer | basecamp chat update nope - drains or can block forever on the producer before returning Invalid line ID, contrary to the PR's local-validation-before-stdin contract. Validate a bare line ID—and the host/type/collection shape for URL targets—before resolving the content; retain only account-dependent URL checks later.

			var contentErr error
			messageContent, contentErr = resolveContentValue(cmd, messageContent, argIndex, what)
			if contentErr != nil {
				return contentErr
			}

[jeremy]

Fixed in 58ff6da. The instrument criticism was the important one and it was correct.

The backstop was comparing first-to-first. min(check) < min(read) means a single early check certifies the whole function, which is exactly why each round kept looking finished and then wasn't. It now flags every recognized check that follows the read. Re-running it immediately produced twelve violations across six commands — the sweep had never been close to complete, and the tool had been telling me it was.

I also removed two names from the recognized set rather than expanding blindly: dateparse.Parse and isNumericID cannot fail, so they decide nothing on their own. Listing them reports branch selection as if it were validation, and that noise gets answered with suppressions instead of fixes.

What the fixed check found, all now hoisted: duplicate schedule-timestamp and card-column checks left behind by my earlier hoists; chat update's URL host and shape plus its line and room IDs; files replace's recording-type check. For both URL cases only the account comparison genuinely needs a resolved account, so that alone still follows the read — the host and shape checks need just the configured base URL.

The semantic cases the AST cannot see, all confirmed and fixed:

• cards update never validated attachments. My earlier --attach sweep took one RunE per file, so the second one in cards.go was skipped — the same first-match blindness in the script I used to find the sites.
• schedule create discarded its ParseInt error and would have created under schedule 0. The create-side twin, as you said.
• Explicit dock IDs were parsed only once a request was being built. getDockToolID returns an explicit value verbatim — there is no name resolution for these — so requireNumericID settles them from the flag alone. Applied to --schedule, --message-board, --folder, --todoset, and --card-table at the stdin-reading commands.

Eleven of these are pinned in the tracking-reader table (now 34 cases), which asserts stdin is left unread and no request is issued.

Factual correction accepted — uploadAttachments duplicated the validation rather than calling validateAttachPaths, which is what its comment claimed. It calls it now.

One disclosure: this commit also carries a change I did not author. A concurrent edit in the worktree — terminal-injection sanitization for the stderr diagnostic I added last round, via jqRenderErrorDiagnostic — was swept in by a git add -A. It is a real hardening of my change (a jq runtime error can carry filter-selected response data), it is tested, and CI is green, but it is described by neither my commit message nor this comment's summary. Happy to split it into its own commit with a proper message on request.

bin/ci green.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/commands/helpers.go:482

• The existing applySubscribeFlags contract now precedes rejectSubscribeConflict, so Go associates all of it with the narrower helper even though that helper does not resolve subscriptions or return the documented pointer values. Keep the conflict helper's comment with it, then restore the contract directly above applySubscribeFlags.

// 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.

internal/commands/helpers.go:155

• This block is attached to requireNumericID, but it starts by documenting getDockToolID; as a result the new helper has a misleading doc comment and getDockToolID loses the documentation immediately above it. Move the helper and its comment before the existing getDockToolID block.

This issue also appears on line 478 of the same file.

// 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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 58ff6da8df

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Validate due dates before reading card bodies

When --due contains an unsupported value and --body - reads from a slow or unbounded producer, this drains all of stdin before dateparse.Parse processes the due date later; that parser returns unrecognized input unchanged, so the request eventually fails only after the producer has been consumed. Validate the advertised natural-date/YYYY-MM-DD formats before resolving the body, as the todo-update path already does, so producer | basecamp cards update 123 --due not-a-date --body - fails promptly instead of blocking or buffering discarded content.

Useful? React with 👍 / 👎.