feat(commands): retire talm dmesg ahead of upstream + merge test plans

main.go

@@ -32,6 +32,12 @@ const (
 	// appears both in skipConfigCommands and in cobra's exported
 	// API.
 	completionInternal = "__complete"
+	// dmesgSubcommandName labels the hidden migration stub for the
+	// retired `talm dmesg` command. The stub errors with a hint
+	// pointing at `talm logs kernel --tail=N`; it must skip
+	// Chart.yaml loading so the migration hint surfaces even when
+	// the operator runs it outside a talm project.
+	dmesgSubcommandName = "dmesg"
 )
 
 // cmdNameTalm is the binary name used both as the cobra root
@@ -49,9 +55,10 @@ var Version = "dev"
 // - init: creates the config, so it doesn't exist yet
 // - completion: generates shell completion scripts
 // - __complete: cobra's internal command for shell autocompletion (Tab key).
+// - dmesg: retired migration stub; must error with the hint regardless of cwd.
 //
 //nolint:gochecknoglobals // immutable lookup table consulted by isCommandOrParent during PersistentPreRunE; init-time literal.
-var skipConfigCommands = []string{initSubcommandName, completionSubcommand, completionInternal}
+var skipConfigCommands = []string{initSubcommandName, completionSubcommand, completionInternal, dmesgSubcommandName}
 
 // rootCmd represents the base command when called without any subcommands.
 //

main_test.go

@@ -248,6 +248,17 @@ func TestSkipConfigCommands(t *testing.T) {
 			cmdPath:  []string{"talm", "init"},
 			expected: true,
 		},
+		{
+			// dmesg is the retired migration stub — must skip
+			// Chart.yaml loading so the "talm dmesg has been
+			// removed" hint surfaces even outside a project
+			// root. Without this membership the operator would
+			// see an "error reading configuration file" instead
+			// of the migration hint.
+			name:     "dmesg (retired migration stub)",
+			cmdPath:  []string{"talm", "dmesg"},
+			expected: true,
+		},
 		{
 			name:     "apply command should load config",
 			cmdPath:  []string{"talm", "apply"},

pkg/commands/dmesg_handler.go

@@ -15,54 +15,59 @@
 package commands
 
 import (
-	"strings"
-
 	"github.com/cockroachdb/errors"
 	"github.com/spf13/cobra"
 )
 
-// dmesgCmdName labels the wrapped dmesg subcommand in the dispatch.
-// Hoisted so a future rename touches one site.
+// dmesgCmdName labels the talm stub that replaces the wrapped
+// upstream dmesg command. Hoisted so a future rename touches one
+// site.
 const dmesgCmdName = "dmesg"
 
-// wrapDmesgCommand installs a FlagErrorFunc that rewrites the
-// cryptic ParseBool error from a numeric --tail value into an
-// operator-friendly hint.
+// dmesgCmd is a hidden migration stub for the retired `talm dmesg`
+// surface.
+//
+// Upstream Talos is retiring the `dmesg` command in favor of
+// `talm logs kernel`, which supports `--tail=N` as a line count
+// directly (the semantics operators reach for; the original
+// `dmesg --tail` was a boolean toggle for tail-mode under
+// --follow, a frequent source of `strconv.ParseBool` confusion).
+// The upstream maintainer's response on siderolabs/talos#13333
+// pointed at `logs kernel` as the replacement.
 //
-// Upstream talosctl registers --tail as a BoolVarP toggling
-// tail-mode for --follow (`Dmesg(ctx, follow, tail bool)` on the
-// wire). Operators' first instinct is `tail(1)`-style line count;
-// `talm dmesg --tail=3` then surfaces
-// `strconv.ParseBool: parsing "3": invalid syntax` with no hint
-// at the actual contract.
+// Removing the command proactively (rather than waiting for
+// upstream's removal) means operators with scripts that call
+// `talm dmesg ...` get an immediate, actionable migration hint
+// instead of a silently-deprecated command that may disappear
+// on the next Talos bump. The stub is hidden from `talm --help`
+// so the command does not surface to new operators.
+// DisableFlagParsing lets operator-supplied flags pass through
+// to RunE without pflag erroring first — the migration hint
+// runs regardless of what arguments accompanied the call.
 //
-// The original ParseBool error is wrapped (not replaced) so it
-// stays reachable via errors.Unwrap and verbose fmt.Sprintf("%+v",
-// err) rendering — debugging the underlying pflag failure remains
-// possible while the operator-facing top line describes what --tail
-// actually does and what to do instead.
-func wrapDmesgCommand(wrappedCmd *cobra.Command) {
-	wrappedCmd.SetFlagErrorFunc(func(_ *cobra.Command, err error) error {
-		// Substring detection on pflag's error format. Two tokens
-		// — `--tail` plus `ParseBool` — together are specific
-		// enough to avoid false positives (pflag only emits
-		// ParseBool errors for bool flags). Matching on `--tail`
-		// without surrounding quotes is durable to pflag's
-		// rendering difference between shorthand-present and
-		// shorthand-absent flags: today's `BoolVarP(..., "tail",
-		// "", ...)` emits `"--tail"`, but a future upstream that
-		// adds a shorthand would emit `-T, --tail` and would
-		// otherwise silently bypass the cushion. pflag does not
-		// export a typed error for invalid-argument failures, so
-		// substring is the only available detection path.
-		msg := err.Error()
-		if strings.Contains(msg, "--tail") && strings.Contains(msg, "ParseBool") {
-			return errors.WithHint(
-				errors.Wrap(err, "talm dmesg --tail is a boolean toggle (tail-mode for --follow), not a line count"),
-				"for the last N lines, pipe to tail(1): `talm dmesg --nodes <node> | tail -n N`; to stream only new messages, use `--follow --tail`",
-			)
-		}
+// Side effect of DisableFlagParsing: cobra does NOT intercept
+// `--help` either — `talm dmesg --help` reaches RunE and
+// surfaces the same migration hint instead of cobra's help
+// renderer. Intentional: every invocation of the retired
+// command should reach the same nudge. A future maintainer
+// who flips DisableFlagParsing to false would silently
+// re-introduce the original pflag failure shape on `--tail=N`
+// for any operator who didn't migrate yet.
+//
+//nolint:gochecknoglobals // cobra command registration requires a package-level value
+var dmesgCmd = &cobra.Command{
+	Use:                dmesgCmdName,
+	Short:              "[removed] use `talm logs kernel` instead",
+	Hidden:             true,
+	DisableFlagParsing: true,
+	RunE: func(_ *cobra.Command, _ []string) error {
+		return errors.WithHint(
+			errors.New("talm dmesg has been removed"),
+			"use `talm logs kernel --tail=N --nodes <node>` for the last N kernel-log lines, or `talm logs kernel --follow --nodes <node>` to stream new messages. Upstream Talos is retiring the `dmesg` command (see siderolabs/talos#13333) and talm drops it ahead of time to avoid a silent break on the next Talos bump.",
+		)
+	},
+}
 
-		return err
-	})
+func init() {
+	addCommand(dmesgCmd)
 }

pkg/commands/talosctl_wrapper.go

@@ -301,13 +301,6 @@ func wrapTalosCommand(cmd *cobra.Command, cmdName string) *cobra.Command {
 		wrapCrashdumpCommand(wrappedCmd)
 	}
 
-	// Special handling for dmesg: rewrite the cryptic ParseBool
-	// error from a numeric --tail value into a hint that describes
-	// upstream's actual --tail contract.
-	if baseCmdName == dmesgCmdName {
-		wrapDmesgCommand(wrappedCmd)
-	}
-
 	// Special handling for the interactive-only commands
 	// (dashboard, edit): refuse non-tty stdin up front so the
 	// operator gets a clear hint instead of a no-output failure.
@@ -344,6 +337,7 @@ func init() {
 		"config":        true, // talm manages config differently
 		"patch":         true, // not needed in talm
 		"upgrade-k8s":   true, // not needed in talm
+		dmesgCmdName:    true, // retired upstream (siderolabs/talos#13333); talm registers a hidden migration stub pointing at `talm logs kernel --tail=N`
 		talosconfigName: true, // talm has its own talosconfig command
 	}
 

pkg/commands/talosctl_wrapper_test.go

@@ -332,71 +332,117 @@ func TestWrapKubeconfigCommand_PositionalPathErrorMessageMatchesContract(t *test
 	}
 }
 
-// TestWrapDmesgCommand_TailEqualsNumeric_RewritesError pins the
-// FlagErrorFunc cushion for `--tail=<n>` typo. Upstream talosctl
-// registers --tail as a BoolVarP (toggling tail-mode for --follow), but
-// operators' first instinct is tail(1)-style line count. `--tail=3`
-// trips pflag's ParseBool and surfaces a cryptic
-// 'strconv.ParseBool: parsing "3": invalid syntax' error.
+// TestDmesgRemoved_StubSurfacesMigrationHint pins the proactive
+// removal of `talm dmesg`. Upstream Talos is retiring the `dmesg`
+// command in favor of `talm logs kernel`, which supports
+// `--tail=N` as a line count directly (the exact semantics
+// operators reach for). Per siderolabs/talos#13333 the upstream
+// maintainer pointed at `logs kernel` as the replacement.
 //
-// The wrapper installs a FlagErrorFunc that detects this specific
-// pattern and rewrites it with a hint pointing at the actual
-// upstream contract + a pipe-to-tail(1) suggestion for the
-// line-count case operators usually want.
-func TestWrapDmesgCommand_TailEqualsNumeric_RewritesError(t *testing.T) {
-	var dmesgCmd *cobra.Command
-
-	for _, cmd := range taloscommands.Commands {
+// talm short-circuits any `talm dmesg ...` invocation with a
+// hidden stub command that always errors. The error names the
+// migration path so operators with scripts get an immediate
+// nudge to the right surface rather than a silent "unknown
+// command" or — worse — a working-but-deprecated command.
+//
+// Contract pins:
+//
+//   - The stub exists at the `talm dmesg` command path.
+//   - It is HIDDEN from `talm --help` (operators shouldn't see
+//     dmesg as available).
+//   - Any invocation (with or without flags) errors and the
+//     error's hint names `logs kernel --tail=N` as the
+//     replacement.
+//   - The upstream taloscommands.Commands entry for `dmesg` is
+//     NOT wrapped (the wrapper's excludedCommands map skips it).
+//     Without this, the stub would collide with the wrapped
+//     upstream command and cobra would error on duplicate.
+func TestDmesgRemoved_StubSurfacesMigrationHint(t *testing.T) {
+	t.Parallel()
+
+	// Find the registered talm `dmesg` stub among package
+	// commands. Iterate Commands rather than rootCmd.Commands
+	// because rootCmd assembly happens in main.go's init, which
+	// the test package doesn't drive.
+	var stub *cobra.Command
+
+	for _, cmd := range Commands {
 		if cmd.Name() == "dmesg" {
-			dmesgCmd = cmd
+			stub = cmd
 
 			break
 		}
 	}
 
-	if dmesgCmd == nil {
-		t.Skip("upstream taloscommands.Commands has no 'dmesg' command — schema changed")
+	if stub == nil {
+		t.Fatal("talm must register a `dmesg` stub command so `talm dmesg ...` invocations surface a migration hint; got no command with that name in Commands")
 	}
 
-	wrapped := wrapTalosCommand(dmesgCmd, "dmesg")
-
-	// FlagErrorFunc is invoked by cobra's Execute() lifecycle on a
-	// ParseFlags failure. Reproduce that path by capturing the
-	// ParseFlags error and feeding it through the registered hook —
-	// this is exactly what cobra does internally in command.execute.
-	rawErr := wrapped.ParseFlags([]string{"--tail=3"})
-	if rawErr == nil {
-		t.Fatal("--tail=3 must error from pflag (upstream --tail is bool); got nil")
+	if !stub.Hidden {
+		t.Error("dmesg stub must be hidden from --help so operators don't see a retired command as available")
 	}
 
-	hook := wrapped.FlagErrorFunc()
-	if hook == nil {
-		t.Fatal("wrapped dmesg must register a FlagErrorFunc to rewrite ParseBool errors")
+	// Invoke RunE to verify the migration error fires with the
+	// hint pointing at `logs kernel`. RunE invocation skips
+	// cobra's flag parsing layer; the stub uses
+	// DisableFlagParsing so any operator-supplied flags pass
+	// through and don't error before reaching RunE.
+	if stub.RunE == nil {
+		t.Fatal("stub must have a RunE that returns the migration error")
 	}
 
-	err := hook(wrapped, rawErr)
+	err := stub.RunE(stub, []string{"--nodes", "1.2.3.4"})
+	if err == nil {
+		t.Fatal("dmesg stub must error on every invocation")
+	}
 
 	msg := err.Error()
+	if !strings.Contains(msg, "dmesg") || !strings.Contains(strings.ToLower(msg), "removed") {
+		t.Errorf("error must name the dmesg-removal explicitly; got: %q", msg)
+	}
 
-	// Top-level message must describe the actual contract
-	// (boolean toggle for --follow) so the operator sees it first
-	// before any inherited ParseBool noise.
-	if !strings.Contains(strings.ToLower(msg), "boolean") && !strings.Contains(strings.ToLower(msg), "follow") {
-		t.Errorf("rewritten error must describe --tail's actual contract (boolean toggle for --follow); got: %q", msg)
+	hints := strings.Join(errors.GetAllHints(err), "\n")
+	// The hint must cover both operator use cases that the
+	// retired dmesg surface served: --tail=N for the last N
+	// lines (the line-count case operators reach for), AND
+	// --follow for live streaming (the original valid usage of
+	// `talm dmesg --follow` that wasn't a parse-error trap).
+	// Operators with either pattern in scripts get a complete
+	// migration target instead of a hint that only covers one.
+	for _, want := range []string{"logs kernel", "--tail", "--follow"} {
+		if !strings.Contains(hints, want) {
+			t.Errorf("hint must point at `talm logs kernel` covering both `--tail=N` (last N lines) and `--follow` (stream); missing %q in hints:\n%s", want, hints)
+		}
 	}
+}
 
-	// Chain must preserve the original ParseBool error via
-	// errors.Wrap so the underlying pflag failure remains
-	// reachable for verbose debugging (errors.Unwrap,
-	// fmt.Sprintf("%+v", err), etc.). Without wrap-not-replace,
-	// downstream tooling that walks the chain or matches against
-	// the upstream error type loses information.
-	if !strings.Contains(msg, "ParseBool") {
-		t.Errorf("rewritten error must wrap (not replace) the original ParseBool error so the chain stays inspectable; got: %q", msg)
+// TestDmesgExcludedFromTalosctlWrap pins the other half of the
+// removal contract: the upstream taloscommands.Commands entry
+// for `dmesg` MUST NOT be wrapped through wrapTalosCommand.
+// Without this guard the talm-owned stub and the wrapped
+// upstream command would collide on cobra registration. The
+// excludedCommands map in talosctl_wrapper.go's init is the
+// load-bearing piece; this test surfaces a regression that
+// removes "dmesg" from the map while leaving the stub.
+func TestDmesgExcludedFromTalosctlWrap(t *testing.T) {
+	t.Parallel()
+
+	// The talm Commands slice is built from two sources:
+	// upstream taloscommands.Commands (excluding the
+	// excludedCommands map) and talm-native registrations
+	// (apply, init, template, talosconfig, plus our new dmesg
+	// stub). Exactly one entry named `dmesg` should exist (the
+	// stub). If two are present, the exclusion failed.
+	count := 0
+
+	for _, cmd := range Commands {
+		if cmd.Name() == "dmesg" {
+			count++
+		}
 	}
 
-	if unwrapped := errors.Unwrap(err); unwrapped == nil {
-		t.Errorf("rewritten error must be unwrappable to the original; errors.Unwrap returned nil for %q", msg)
+	if count != 1 {
+		t.Errorf("Commands slice must contain exactly one entry named 'dmesg' (the talm stub); got %d — upstream taloscommands.Commands entry was not excluded", count)
 	}
 }