From 421ec77f4bf2e7c56925e7667ad48e50a39bd344 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Mon, 23 Feb 2026 22:32:37 -0500 Subject: [PATCH 1/3] Test script --- .task/repro.sh | 132 ++++++++++++++++++++++ internal/temporalcli/commands.env_test.go | 51 +++++++++ internal/temporalcli/commands_test.go | 25 ++++ 3 files changed, 208 insertions(+) create mode 100755 .task/repro.sh diff --git a/.task/repro.sh b/.task/repro.sh new file mode 100755 index 000000000..b6fc3f834 --- /dev/null +++ b/.task/repro.sh @@ -0,0 +1,132 @@ +#!/usr/bin/env bash +# Run: bash .task/repro.sh +# +# Builds the CLI from the currently checked-out commit and runs scenarios +# that demonstrate whether error reporting and user-facing warnings are +# coupled to the structured logger. Run on main to see the bug; run on +# the fix branch to see correct behavior. +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" +cd "$REPO_ROOT" + +BINARY=/tmp/temporal-repro +ENV_FILE=$(mktemp) +STDERR_FILE=$(mktemp) +trap 'rm -f "$ENV_FILE" "$STDERR_FILE" "$BINARY"' EXIT + +echo "Building temporal from $(git rev-parse --short HEAD) ($(git branch --show-current))..." >&2 +go build -o "$BINARY" ./cmd/temporal + +COMMIT="$(git rev-parse --short HEAD)" +BRANCH="$(git branch --show-current)" + +run() { + local stdout rc + stdout=$("$BINARY" "$@" 2>"$STDERR_FILE") && rc=$? || rc=$? + local stderr + stderr=$(cat "$STDERR_FILE") + echo '```bash' + echo "$ temporal $*" + echo '```' + echo "" + if [ -n "$stdout" ]; then + echo "**stdout:**" + echo '```' + echo "$stdout" + echo '```' + fi + echo "**stderr** (exit $rc):" + if [ -z "$stderr" ]; then + echo '```' + echo "(empty)" + echo '```' + else + echo '```' + echo "$stderr" + echo '```' + fi + echo "" +} + +cat </dev/null +\`\`\` + +EOF + +# Seed the env file +"$BINARY" env set --env-file "$ENV_FILE" --env myenv -k foo -v bar 2>/dev/null + +cat <<'EOF' +## Scenario 1: error reporting vs log level + +Attempting `workflow list` against a closed port should produce a clear +error message on stderr regardless of `--log-level`. Expected: an +`Error: ...` message appears at every log level. + +### Default log level +EOF +run workflow list --address 127.0.0.1:1 + +cat <<'EOF' +### `--log-level never` +EOF +run workflow list --address 127.0.0.1:1 --log-level never + +cat <<'EOF' +--- + +## Scenario 2: deprecation warning vs log level + +Using the deprecated positional-argument syntax for `env get` should +produce a plain-text warning on stderr regardless of `--log-level`. +Expected: a `Warning: ...` line appears at every log level, and it is +plain text (not a structured log message with `time=`/`level=` prefixes). + +### Default log level +EOF +run env get --env-file "$ENV_FILE" myenv + +cat <<'EOF' +### `--log-level never` +EOF +run env get --env-file "$ENV_FILE" --log-level never myenv + +cat <<'EOF' +--- + +## Scenario 3: default log level noise + +Running `env set` at the default log level should not dump structured +log lines to stderr. Expected: stderr is empty. + +### Default log level +EOF +rm -f "$ENV_FILE" && touch "$ENV_FILE" +run env set --env-file "$ENV_FILE" --env myenv -k foo -v bar + +cat <<'EOF' +### `--log-level info` (opt-in) +EOF +rm -f "$ENV_FILE" && touch "$ENV_FILE" +run env set --env-file "$ENV_FILE" --env myenv -k foo -v bar --log-level info + +cat <<'EOF' +--- + +## Cleanup + +```bash +rm -f /tmp/temporal-repro $ENV_FILE +``` +EOF diff --git a/internal/temporalcli/commands.env_test.go b/internal/temporalcli/commands.env_test.go index cdd1157a9..1a2a0a45a 100644 --- a/internal/temporalcli/commands.env_test.go +++ b/internal/temporalcli/commands.env_test.go @@ -2,6 +2,7 @@ package temporalcli_test import ( "os" + "strings" "testing" "gopkg.in/yaml.v3" @@ -104,3 +105,53 @@ func TestEnv_InputValidation(t *testing.T) { res = h.Execute("env", "set", "myenv1.foo") h.ErrorContains(res.Err, `no value provided`) } + +func TestEnv_DeprecationWarningBypassesLogger(t *testing.T) { + h := NewCommandHarness(t) + defer h.Close() + + tmpFile, err := os.CreateTemp("", "") + h.NoError(err) + h.Options.DeprecatedEnvConfig.EnvConfigFile = tmpFile.Name() + defer os.Remove(h.Options.DeprecatedEnvConfig.EnvConfigFile) + res := h.Execute("env", "set", "--env", "myenv1", "-k", "foo", "-v", "bar") + h.NoError(res.Err) + + // Using deprecated argument syntax should produce a warning on stderr. + // The warning must bypass the logger so it appears regardless of log level. + for _, logLevel := range []string{"never", "error", "info"} { + t.Run("log-level="+logLevel, func(t *testing.T) { + res = h.Execute("env", "get", "--log-level", logLevel, "myenv1") + h.NoError(res.Err) + + stderr := res.Stderr.String() + h.Contains(stderr, "Warning:") + h.Contains(stderr, "deprecated") + + // Must be a plain-text warning, not a structured log message + h.False(strings.Contains(stderr, "time="), "warning should not be a structured log message") + h.False(strings.Contains(stderr, "level="), "warning should not be a structured log message") + }) + } +} + +func TestEnv_DefaultLogLevelProducesNoLogOutput(t *testing.T) { + h := NewCommandHarness(t) + defer h.Close() + + tmpFile, err := os.CreateTemp("", "") + h.NoError(err) + h.Options.DeprecatedEnvConfig.EnvConfigFile = tmpFile.Name() + defer os.Remove(h.Options.DeprecatedEnvConfig.EnvConfigFile) + + // env set logs "Setting env property" via cctx.Logger.Info(). With the + // default log level ("never"), this should not appear on stderr. + res := h.Execute("env", "set", "--env", "myenv1", "-k", "foo", "-v", "bar") + h.NoError(res.Err) + h.Empty(res.Stderr.String(), "default log level should produce no log output") + + // With --log-level info, the logger output should appear. + res = h.Execute("env", "set", "--env", "myenv1", "-k", "baz", "-v", "qux", "--log-level", "info") + h.NoError(res.Err) + h.Contains(res.Stderr.String(), "Setting env property") +} diff --git a/internal/temporalcli/commands_test.go b/internal/temporalcli/commands_test.go index 2312e442e..749f2c3e6 100644 --- a/internal/temporalcli/commands_test.go +++ b/internal/temporalcli/commands_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "log/slog" + "net" "regexp" "slices" "strings" @@ -625,6 +626,30 @@ func TestUnknownCommandExitsNonzero(t *testing.T) { assert.Contains(t, res.Err.Error(), "unknown command") } +func TestErrorReporting_IndependentOfLogLevel(t *testing.T) { + // Errors must be reported through Fail regardless of --log-level, and + // must not produce structured log output on stderr. + for _, logLevel := range []string{"never", "error", "info"} { + t.Run("log-level="+logLevel, func(t *testing.T) { + h := NewCommandHarness(t) + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + ln.Close() // close immediately so connection is refused + + res := h.Execute( + "workflow", "list", + "--address", ln.Addr().String(), + "--log-level", logLevel, + ) + require.Error(t, res.Err) + + stderr := res.Stderr.String() + assert.NotContains(t, stderr, "level=ERROR", + "errors should not appear as structured log messages on stderr") + }) + } +} + func (s *SharedServerSuite) TestHiddenAliasLogFormat() { _ = s.waitActivityStarted().GetID() res := s.Execute("workflow", "list", "--log-format", "pretty", "--address", s.Address()) From aac3a61d5fc01404116907a700ca68a128c507e6 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Mon, 23 Feb 2026 23:02:02 -0500 Subject: [PATCH 2/3] Decouple error reporting from logging The Fail callback was routing errors through the slog logger, which meant: - Errors appeared as structured log messages (with timestamp and level) - --log-level never silently swallowed errors - --log-format json wrapped errors in JSON Errors are now always printed directly to stderr as "Error: ", independent of log configuration. Default log level changed from "info" to "never"; server start-dev continues to default to "warn" via its existing ChangedFromDefault override. Users can still opt in with --log-level on any command. Two deprecation warnings (namespace arg, env command args) converted from logger calls to direct stderr writes so they remain visible regardless of log level. --- cliext/flags.gen.go | 4 ++-- cliext/option-sets.yaml | 4 ++-- internal/temporalcli/commands.env.go | 2 +- internal/temporalcli/commands.go | 6 +----- internal/temporalcli/commands.operator_namespace.go | 2 +- internal/temporalcli/commands.workflow_exec.go | 2 +- internal/temporalcli/commands.workflow_view.go | 2 +- 7 files changed, 9 insertions(+), 13 deletions(-) diff --git a/cliext/flags.gen.go b/cliext/flags.gen.go index 28668a3ed..c607e4c13 100644 --- a/cliext/flags.gen.go +++ b/cliext/flags.gen.go @@ -38,8 +38,8 @@ func (v *CommonOptions) BuildFlags(f *pflag.FlagSet) { f.StringVar(&v.Profile, "profile", "", "Profile to use for config file. EXPERIMENTAL.") f.BoolVar(&v.DisableConfigFile, "disable-config-file", false, "If set, disables loading environment config from config file. EXPERIMENTAL.") f.BoolVar(&v.DisableConfigEnv, "disable-config-env", false, "If set, disables loading environment config from environment variables. EXPERIMENTAL.") - v.LogLevel = NewFlagStringEnum([]string{"debug", "info", "warn", "error", "never"}, "info") - f.Var(&v.LogLevel, "log-level", "Log level. Default is \"info\" for most commands and \"warn\" for \"server start-dev\". Accepted values: debug, info, warn, error, never.") + v.LogLevel = NewFlagStringEnum([]string{"debug", "info", "warn", "error", "never"}, "never") + f.Var(&v.LogLevel, "log-level", "Log level. Default is \"never\" for most commands and \"warn\" for \"server start-dev\". Accepted values: debug, info, warn, error, never.") v.LogFormat = NewFlagStringEnum([]string{"text", "json", "pretty"}, "text") f.Var(&v.LogFormat, "log-format", "Log format. Accepted values: text, json.") v.Output = NewFlagStringEnum([]string{"text", "json", "jsonl", "none"}, "text") diff --git a/cliext/option-sets.yaml b/cliext/option-sets.yaml index c8472fd03..e7a7ff8cf 100644 --- a/cliext/option-sets.yaml +++ b/cliext/option-sets.yaml @@ -51,8 +51,8 @@ option-sets: - never description: | Log level. - Default is "info" for most commands and "warn" for "server start-dev". - default: info + Default is "never" for most commands and "warn" for "server start-dev". + default: never - name: log-format type: string-enum description: Log format. diff --git a/internal/temporalcli/commands.env.go b/internal/temporalcli/commands.env.go index d98c06f04..5d43843d7 100644 --- a/internal/temporalcli/commands.env.go +++ b/internal/temporalcli/commands.env.go @@ -13,7 +13,7 @@ import ( func (c *TemporalEnvCommand) envNameAndKey(cctx *CommandContext, args []string, keyFlag string) (string, string, error) { if len(args) > 0 { - cctx.Logger.Warn("Arguments to env commands are deprecated; please use --env and --key (or -k) instead") + fmt.Fprintln(cctx.Options.Stderr, "Warning: Arguments to env commands are deprecated; please use --env and --key (or -k) instead") if c.Parent.Env != "default" || keyFlag != "" { return "", "", fmt.Errorf("cannot specify both an argument and flags; please use flags instead") diff --git a/internal/temporalcli/commands.go b/internal/temporalcli/commands.go index a60307b83..1983ebdfa 100644 --- a/internal/temporalcli/commands.go +++ b/internal/temporalcli/commands.go @@ -143,11 +143,7 @@ func (c *CommandContext) preprocessOptions() error { if c.Err() != nil { err = fmt.Errorf("program interrupted") } - if c.Logger != nil { - c.Logger.Error(err.Error()) - } else { - fmt.Fprintln(os.Stderr, err) - } + fmt.Fprintf(c.Options.Stderr, "Error: %v\n", err) os.Exit(1) } } diff --git a/internal/temporalcli/commands.operator_namespace.go b/internal/temporalcli/commands.operator_namespace.go index ebcc96cab..ca4b6c6d5 100644 --- a/internal/temporalcli/commands.operator_namespace.go +++ b/internal/temporalcli/commands.operator_namespace.go @@ -19,7 +19,7 @@ func (c *TemporalOperatorCommand) getNSFromFlagOrArg0(cctx *CommandContext, args } if len(args) > 0 { - cctx.Logger.Warn("Passing the namespace as an argument is now deprecated; please switch to using -n instead") + fmt.Fprintln(cctx.Options.Stderr, "Warning: Passing the namespace as an argument is now deprecated; please switch to using -n instead") return args[0], nil } return c.Namespace, nil diff --git a/internal/temporalcli/commands.workflow_exec.go b/internal/temporalcli/commands.workflow_exec.go index d5ba34899..935504b96 100644 --- a/internal/temporalcli/commands.workflow_exec.go +++ b/internal/temporalcli/commands.workflow_exec.go @@ -94,7 +94,7 @@ func (c *TemporalWorkflowExecuteCommand) run(cctx *CommandContext, args []string // Log print failure and return workflow failure if workflow failed if closeEvent.EventType != enums.EVENT_TYPE_WORKFLOW_EXECUTION_COMPLETED { if err != nil { - cctx.Logger.Error("Workflow failed, and printing the output also failed", "error", err) + fmt.Fprintf(cctx.Options.Stderr, "Warning: printing workflow output failed: %v\n", err) } err = fmt.Errorf("workflow failed") } diff --git a/internal/temporalcli/commands.workflow_view.go b/internal/temporalcli/commands.workflow_view.go index 08fb5c76e..a63ca06ec 100644 --- a/internal/temporalcli/commands.workflow_view.go +++ b/internal/temporalcli/commands.workflow_view.go @@ -560,7 +560,7 @@ func (c *TemporalWorkflowResultCommand) run(cctx *CommandContext, _ []string) er // Log print failure and return workflow failure if workflow failed if closeEvent.EventType != enums.EVENT_TYPE_WORKFLOW_EXECUTION_COMPLETED { if err != nil { - cctx.Logger.Error("Workflow failed, and printing the output also failed", "error", err) + fmt.Fprintf(cctx.Options.Stderr, "Warning: printing workflow output failed: %v\n", err) } err = fmt.Errorf("workflow failed") } From f6ccd122805743c7f94b7c0b4077b8f2928a0d90 Mon Sep 17 00:00:00 2001 From: Dan Davison Date: Sun, 1 Mar 2026 13:33:13 -0500 Subject: [PATCH 3/3] rm repro script --- .task/repro.sh | 132 ------------------------------------------------- 1 file changed, 132 deletions(-) delete mode 100755 .task/repro.sh diff --git a/.task/repro.sh b/.task/repro.sh deleted file mode 100755 index b6fc3f834..000000000 --- a/.task/repro.sh +++ /dev/null @@ -1,132 +0,0 @@ -#!/usr/bin/env bash -# Run: bash .task/repro.sh -# -# Builds the CLI from the currently checked-out commit and runs scenarios -# that demonstrate whether error reporting and user-facing warnings are -# coupled to the structured logger. Run on main to see the bug; run on -# the fix branch to see correct behavior. -set -euo pipefail - -REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" -cd "$REPO_ROOT" - -BINARY=/tmp/temporal-repro -ENV_FILE=$(mktemp) -STDERR_FILE=$(mktemp) -trap 'rm -f "$ENV_FILE" "$STDERR_FILE" "$BINARY"' EXIT - -echo "Building temporal from $(git rev-parse --short HEAD) ($(git branch --show-current))..." >&2 -go build -o "$BINARY" ./cmd/temporal - -COMMIT="$(git rev-parse --short HEAD)" -BRANCH="$(git branch --show-current)" - -run() { - local stdout rc - stdout=$("$BINARY" "$@" 2>"$STDERR_FILE") && rc=$? || rc=$? - local stderr - stderr=$(cat "$STDERR_FILE") - echo '```bash' - echo "$ temporal $*" - echo '```' - echo "" - if [ -n "$stdout" ]; then - echo "**stdout:**" - echo '```' - echo "$stdout" - echo '```' - fi - echo "**stderr** (exit $rc):" - if [ -z "$stderr" ]; then - echo '```' - echo "(empty)" - echo '```' - else - echo '```' - echo "$stderr" - echo '```' - fi - echo "" -} - -cat </dev/null -\`\`\` - -EOF - -# Seed the env file -"$BINARY" env set --env-file "$ENV_FILE" --env myenv -k foo -v bar 2>/dev/null - -cat <<'EOF' -## Scenario 1: error reporting vs log level - -Attempting `workflow list` against a closed port should produce a clear -error message on stderr regardless of `--log-level`. Expected: an -`Error: ...` message appears at every log level. - -### Default log level -EOF -run workflow list --address 127.0.0.1:1 - -cat <<'EOF' -### `--log-level never` -EOF -run workflow list --address 127.0.0.1:1 --log-level never - -cat <<'EOF' ---- - -## Scenario 2: deprecation warning vs log level - -Using the deprecated positional-argument syntax for `env get` should -produce a plain-text warning on stderr regardless of `--log-level`. -Expected: a `Warning: ...` line appears at every log level, and it is -plain text (not a structured log message with `time=`/`level=` prefixes). - -### Default log level -EOF -run env get --env-file "$ENV_FILE" myenv - -cat <<'EOF' -### `--log-level never` -EOF -run env get --env-file "$ENV_FILE" --log-level never myenv - -cat <<'EOF' ---- - -## Scenario 3: default log level noise - -Running `env set` at the default log level should not dump structured -log lines to stderr. Expected: stderr is empty. - -### Default log level -EOF -rm -f "$ENV_FILE" && touch "$ENV_FILE" -run env set --env-file "$ENV_FILE" --env myenv -k foo -v bar - -cat <<'EOF' -### `--log-level info` (opt-in) -EOF -rm -f "$ENV_FILE" && touch "$ENV_FILE" -run env set --env-file "$ENV_FILE" --env myenv -k foo -v bar --log-level info - -cat <<'EOF' ---- - -## Cleanup - -```bash -rm -f /tmp/temporal-repro $ENV_FILE -``` -EOF