Make IsMachineOutput honor an explicit --styled/--md - #670
Conversation
ApplyFlags lets --styled and --md rebuild the writer over a configured json/quiet format, but IsMachineOutput never read those flags — so `basecamp --styled <cmd>` on a format=json machine was human to the renderer and machine to every gate. That split surfaced as the declined review thread on #654: chat delete demanded --force for an invocation whose confirmation a person could see and answer, and chat.go grew humanFormatOverride to state the rule at one call site because the predicate couldn't. The clause sits between the machine-flag block and the config switch, mirroring ApplyFlags' ordering: --json --styled resolves JSON-first there, so machine flags still win here. Deliberately not delegated to EffectiveFormat(): that would also pull in FormatAuto-on-a-pipe, flipping upgrade's stdout, attachment progress, the skill-update nudge, and quickstart/wizard gating for every `basecamp foo | grep`. TestMachinePredicatesAgree pins the whole flag/config matrix against machineReadsThisOutput, asserting that one divergence explicitly instead of skipping it.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Pull request overview
Aligns machine-output detection with explicit human-format flags and renderer precedence.
Changes:
- Treats
--styled/--mdas overriding configured JSON/quiet output. - Adds unit and predicate-consistency coverage.
- Updates wizard test documentation.
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 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/appctx/context.go |
Adds human-format override handling. |
internal/appctx/context_test.go |
Tests format precedence. |
internal/commands/helpers_test.go |
Adds predicate consistency matrix. |
internal/commands/wizard.go |
Clarifies gate documentation. |
internal/commands/wizard_test.go |
Clarifies test documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Follow-through on the one declined thread from #654:
IsMachineOutputread flags and config but neverFlags.Styled/Flags.MD, whileApplyFlagslets those flags overwrite a configuredjson/quietwriter. The same invocation was human to the renderer and machine to every gate, andhumanFormatOverrideinchat.gois this exact rule written once at one call site because the predicate couldn't say it.Change
One clause in
IsMachineOutput, between the machine-flag block and the config switch — machine flags must stay above it, since--json --styledresolves JSON-first inApplyFlagsand the predicate mirrors that ordering.Deliberately not delegated to
EffectiveFormat(). That would also pull in FormatAuto-on-a-pipe (EffectiveFormat()==FormatJSONwhereIsMachineOutput()isfalsetoday), flippingupgradestdout→stderr, attachment progress, the skill-update nudge, and quickstart/skill/wizard gating for everybasecamp foo | grep. It would also be order-sensitive: the existingTestIsMachineOutput*tables never callApplyFlags, so every row would read the writer's JSON default and pass vacuously.Callers whose behavior changes
Only when an explicit
--styled/--mdoverrides a configuredjson/quiet— all flip toward "treat as human", which is what the flag asked for:shouldIncludeStatsInError/shouldPrintStatsToStderr(context.go)root.go:207,221)attachments.go)quickstart.gorouting,skill.godump→picker (still TTY-gated),upgrade.gowizardCanRun,setupCanRunafter Streamline first-time setup with recommended defaults #660)humanFormatOverrideinchat.gostays — the breadcrumb must still carry the flag for the future invocation it suggests.Tests
context_test.go:--styledover configjsonand--mdover configquiet→ not machine; configjsonalone and--json --styled→ machine. Existing rows unchanged.TestMachinePredicatesAgreeininternal/commands: the regression net for "these must not drift again" — every flag×config combination assertsapp.IsMachineOutput() == machineReadsThisOutput(cmd), with all three stdio streams pinned to pipes and the precondition asserted. The one deliberate divergence (FormatAuto on a pipe) is asserted as a divergence rather than skipped, so a change to either side of it shows up.TestChatDeleteConfirmationMatrixpasses with and without.Note: #660 also touches
wizard.go(renames the gate tosetupCanRunand moves its doc comment). The comment amendment here will need a trivial rebase whichever lands second.bin/cigreen.Summary by cubic
Makes
IsMachineOutputhonor explicit--styled/--mdflags, so an invocation that overrides a configuredjson/quietformat is treated as human by all gates instead of being routed as machine while rendering human.Behavior changes
IsMachineOutputnow flip to human for--styled/--mdover machine config, covering error stats, post-run notices, attachment progress, quickstart/skill/upgrade routing, and wizard gating.--json,--quiet, etc.) still win over style flags, matchingApplyFlagsordering.Testing
TestMachinePredicatesAgreeregression net that asserts the full flag×config matrix against the renderer, with the one deliberateFormatAuto-on-pipe divergence enforced explicitly.Written for commit 1f415ec. Summary will update on new commits.