From 1f415ecc98928d72a58177f372a162a77c6dda5c Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 31 Aug 2026 02:25:42 -0700 Subject: [PATCH] Make IsMachineOutput honor an explicit --styled/--md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ApplyFlags lets --styled and --md rebuild the writer over a configured json/quiet format, but IsMachineOutput never read those flags — so `basecamp --styled ` 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. --- internal/appctx/context.go | 6 +++ internal/appctx/context_test.go | 29 +++++++++++++ internal/commands/helpers_test.go | 71 +++++++++++++++++++++++++++++++ internal/commands/wizard.go | 4 +- internal/commands/wizard_test.go | 4 +- 5 files changed, 112 insertions(+), 2 deletions(-) diff --git a/internal/appctx/context.go b/internal/appctx/context.go index bdcbada8b..856eb5500 100644 --- a/internal/appctx/context.go +++ b/internal/appctx/context.go @@ -318,6 +318,12 @@ func (a *App) IsMachineOutput() bool { if a.Flags.Agent || a.Flags.Quiet || a.Flags.IDsOnly || a.Flags.Count || a.Flags.JSON || a.Flags.JQFilter != "" { return true } + // An explicit --styled/--md is what ApplyFlags honors over a configured + // machine format; this predicate must agree, or the same invocation is + // human to the renderer and machine to every gate. + if a.Flags.Styled || a.Flags.MD { + return false + } // Config-driven machine output formats if a.Config != nil { switch a.Config.Format { diff --git a/internal/appctx/context_test.go b/internal/appctx/context_test.go index 3bbeb3724..65f56e49a 100644 --- a/internal/appctx/context_test.go +++ b/internal/appctx/context_test.go @@ -586,6 +586,35 @@ func TestIsMachineOutputConfigFormat(t *testing.T) { } } +// Test that an explicit --styled/--md overrides a configured machine format, +// matching ApplyFlags: the flag rebuilds the writer as styled/markdown, so the +// predicate must stand down too, or the same invocation is human to the +// renderer and machine to every gate. Machine flags still win over style flags, +// mirroring ApplyFlags' JSON-first ordering. +func TestIsMachineOutputStyleFlagOverridesConfig(t *testing.T) { + tests := []struct { + name string + format string + setFlags func(*App) + expected bool + }{ + {"styled over config json", "json", func(a *App) { a.Flags.Styled = true }, false}, + {"md over config quiet", "quiet", func(a *App) { a.Flags.MD = true }, false}, + {"config json with no style flag stays machine", "json", func(a *App) {}, true}, + {"json flag beats styled flag", "json", func(a *App) { a.Flags.JSON = true; a.Flags.Styled = true }, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &config.Config{Format: tt.format} + app := NewApp(cfg) + tt.setFlags(app) + + assert.Equal(t, tt.expected, app.IsMachineOutput()) + }) + } +} + // Test that app.Err doesn't print stats in machine output modes func TestAppErrMachineOutputNoStats(t *testing.T) { tests := []struct { diff --git a/internal/commands/helpers_test.go b/internal/commands/helpers_test.go index 5e860b68f..b4f07e261 100644 --- a/internal/commands/helpers_test.go +++ b/internal/commands/helpers_test.go @@ -6,6 +6,7 @@ import ( "errors" "io" "net/http" + "os" "strings" "testing" @@ -209,6 +210,76 @@ func TestIsMachineOutput_JSONFlag(t *testing.T) { assert.True(t, isMachineOutput(cmd)) } +// TestMachinePredicatesAgree is the regression net for app.IsMachineOutput and +// machineReadsThisOutput drifting apart. They answer the same question through +// different lenses — flags+config vs the writer ApplyFlags built — and the chat +// delete --force corner came from them disagreeing on `--styled` over a +// configured json. Every flag/config combination must agree, with one +// deliberate exception: FormatAuto on a pipe, where the renderer emits JSON +// (machineReadsThisOutput true) while gates keep treating `basecamp foo | grep` +// as human (IsMachineOutput false) so notices, progress, and wizard gating +// don't flip on redirection alone. That row is asserted as a divergence, not +// skipped, so a change to either side of it shows up here. +func TestMachinePredicatesAgree(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + + // Neither predicate may consult a real terminal: pin all three stdio + // streams to pipes and assert the precondition, so the FormatAuto rows + // resolve deterministically wherever the test runs. + r, w, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { r.Close(); w.Close() }) + origIn, origOut, origErr := os.Stdin, os.Stdout, os.Stderr + os.Stdin, os.Stdout, os.Stderr = r, w, w + t.Cleanup(func() { os.Stdin, os.Stdout, os.Stderr = origIn, origOut, origErr }) + fi, err := os.Stdout.Stat() + require.NoError(t, err) + require.Zero(t, fi.Mode()&os.ModeCharDevice, "precondition: stdout must not be a terminal") + + configFormats := []string{"", "json", "quiet", "markdown"} + flagSets := []struct { + name string + set func(*appctx.App) + }{ + {"none", func(a *appctx.App) {}}, + {"json", func(a *appctx.App) { a.Flags.JSON = true }}, + {"agent", func(a *appctx.App) { a.Flags.Agent = true }}, + {"quiet", func(a *appctx.App) { a.Flags.Quiet = true }}, + {"ids-only", func(a *appctx.App) { a.Flags.IDsOnly = true }}, + {"count", func(a *appctx.App) { a.Flags.Count = true }}, + {"jq", func(a *appctx.App) { a.Flags.JQFilter = ".x" }}, + {"styled", func(a *appctx.App) { a.Flags.Styled = true }}, + {"md", func(a *appctx.App) { a.Flags.MD = true }}, + {"styled+json", func(a *appctx.App) { a.Flags.Styled = true; a.Flags.JSON = true }}, + } + + for _, format := range configFormats { + for _, flags := range flagSets { + name := "config=" + format + "/flags=" + flags.name + t.Run(name, func(t *testing.T) { + app := appctx.NewApp(&config.Config{Format: format}) + flags.set(app) + app.ApplyFlags() + + cmd := &cobra.Command{Use: "test"} + cmd.SetContext(appctx.WithApp(context.Background(), app)) + + gate := app.IsMachineOutput() + renderer := machineReadsThisOutput(cmd) + + if format == "" && flags.name == "none" { + // The FormatAuto-on-a-pipe exception described above. + assert.False(t, gate, "IsMachineOutput must not flip on redirection alone") + assert.True(t, renderer, "FormatAuto on a pipe renders JSON") + return + } + assert.Equal(t, gate, renderer, + "IsMachineOutput=%v but machineReadsThisOutput=%v for %s", gate, renderer, name) + }) + } + } +} + func TestIsNonInteractiveCommand_NonInteractiveEnv(t *testing.T) { t.Setenv("BASECAMP_NONINTERACTIVE", "1") diff --git a/internal/commands/wizard.go b/internal/commands/wizard.go index 617710494..5cc611b0b 100644 --- a/internal/commands/wizard.go +++ b/internal/commands/wizard.go @@ -487,7 +487,9 @@ func fetchProjectName(cmd *cobra.Command, app *appctx.App, projectID string) str // no one of them sees everything: IsInteractive covers non-terminal // stdin/stdout, the machine-output flags and the BASECAMP_NONINTERACTIVE escape // hatch; IsMachineOutput adds the config-driven json/quiet formats it does not -// look at; InteractivePrompt adds stderr, which is where huh actually draws. +// look at (standing down when an explicit --styled/--md overrides them, since +// ApplyFlags renders those human); InteractivePrompt adds stderr, which is +// where huh actually draws. // // Two callers, deliberately different responses. `basecamp setup` was asked for // by name, so it refuses out loud. Bare `basecamp` never asked for a wizard at diff --git a/internal/commands/wizard_test.go b/internal/commands/wizard_test.go index 0fc59accf..986456e4a 100644 --- a/internal/commands/wizard_test.go +++ b/internal/commands/wizard_test.go @@ -756,7 +756,9 @@ func TestSetupRefusesUnderNonInteractiveEnv(t *testing.T) { // Terminal stdio is not enough: a caller that asked for machine output has // declared it is not there to answer questions, and the wizard is nothing but // questions. Config-driven json/quiet counts too — app.IsInteractive() does not -// look at it, which is why the gate also asks IsMachineOutput(). +// look at it, which is why the gate also asks IsMachineOutput(). An explicit +// --styled/--md overrides a configured machine format there, so that pairing +// prompts like any human invocation. func TestSetupRefusesMachineOutputOnATerminal(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("no /dev/ptmx on Windows")