Featured image of post When you hand the same key to every call

When you hand the same key to every call

I was building a tutorial, the kind where the whole point is that the reader runs every command and it just works. So I generated a fresh project with go-tool-base, added a command, then added a command underneath that command, and hit build. It didn’t.

pkg/cmd/hello/cmd.go: props.ChildCmd undefined
  (type *props.Props has no field or method ChildCmd)

My own generator, in my own framework, had just written code that referenced a thing that didn’t exist. That is a special kind of embarrassing.

A bug with a two-month alibi

git blame walked me straight to the commit that introduced the command middleware system back in March. Middleware here is the web-style idea of wrapping a command’s run function with cross-cutting behaviour, timing, auth, recovery, that sort of thing. To wire it in, the generator started emitting this for a nested command:

setup.AddCommandWithMiddleware(cmd, child.NewCmdChild(p), props.ChildCmd)

where the line before had simply been:

cmd.AddCommand(child.NewCmdChild(p))

The catch is that third argument. props.ChildCmd is meant to be one of a set of constants, but those constants are hand-declared for the framework’s built-in commands only (UpdateCmd, DocsCmd, and friends). The generator never declares one for a user’s child command, so the generated parent referenced a name that nothing had ever declared. Undefined. Won’t compile.

Here’s the part that should worry you more than the bug. It shipped in March and nobody noticed until late May. Partly because it only bites nested commands, a command under another user command; top-level commands register by a different path and were fine. But mostly because the generator’s tests checked the generated code as text, asserting that it contained the right strings, and never once ran go build on the result. CI was green for two months on code that could not compile. We were grading the essay without ever reading it aloud.

What the key actually was

Once I stopped staring at the missing name, the real problem came into focus, and it wasn’t the missing constant.

That third argument is a middleware lookup key. The framework keeps a table of middleware registered against each key, and the key tells it which to apply. It is not an on/off switch and it is not optional, so the generator had to supply one at every registration site. It was being asked to guess, on every call, a value it had no reliable way to produce for a user command.

And the tell was sitting right there in the same generator: everywhere else, the idiom was props.FeatureCmd("name"), a function that derives a key from a string. The nested-registration path was the one place that assumed a hand-declared constant instead. One call site out of step with all the others.

That is the actual lesson, and it has nothing to do with cobra or codegen. When you find yourself threading the same derived value through every single call site, and getting it wrong, the value is in the wrong place. The feature key was never the caller’s business. It belonged to the command.

Changing my mind about cobra

This is where I had to eat a helping of my own opinion.

go-tool-base is built on cobra, the de-facto Go library for building command trees, and I like it a great deal. I had deliberately not wrapped it. Every abstraction over a good library is a tax the reader pays, so my standing rule was: use cobra directly, don’t hide it behind something of mine.

The trouble is the middleware pattern kept growing, and the bigger it got the more plainly “don’t abstract cobra” was a position I was holding past its evidence. The very thing I’d refused to build was the thing the design had come to need. It helped that, having recently moved the project to GitLab, the version had reset to a 0.x prerelease, which makes a breaking change cheap. The window to stop patching and do it properly was open, and not for long.

Go’s composition model made it almost painless. You can embed a pointer to one struct inside another and the outer type inherits all the inner one’s methods for free, which is about as close to monkey-patching as a statically typed language gets. So a setup.Command became a cobra command plus the feature it belongs to:

type Command struct {
	*cobra.Command
	Feature props.FeatureCmd
}

func Wrap(feature props.FeatureCmd, cmd *cobra.Command) *Command {
	return &Command{Command: cmd, Feature: feature}
}

func (c *Command) Register(children ...*Command) {
	for _, child := range children {
		if child.Command == nil {
			continue
		}
		if child.RunE != nil {
			child.RunE = Chain(child.Feature, child.RunE)
		}
		c.AddCommand(child.Command)
	}
}

Because *cobra.Command is embedded, a setup.Command is a cobra command for every method cobra offers; the one place you need the raw pointer, you reach for .Command. The generated command now carries its own identity:

func NewCmdChild(props *props.Props) *setup.Command {
	cmd := &cobra.Command{Use: "child", RunE: ...}
	return setup.Wrap(props.FeatureCmd("child"), cmd)
}

and the parent registers it with nothing threaded through the call:

parent.Register(child.NewCmdChild(p))

The feature key now lives on the command, derived from the command’s own name, which is the one place the generator can always produce it correctly. The bug isn’t so much fixed as made unsayable: there’s no call site left to write the wrong thing into. The wiring got cleaner on the way past, too, each command’s run is wrapped exactly once with its own feature, instead of the old recursive pass that re-applied the parent’s feature down the whole subtree. And the old free function stays on as a deprecated shim that just calls Register, so nothing downstream breaks before v1.0.

Changing my mind about the tests

The redesign was the satisfying fix. The test was the important one.

The reason a non-compiling generator sailed through CI for two months is that its tests read the generated source as text. A generator is a program that writes a program, and we were checking that it wrote the expected words without ever asking whether the words formed a working program. So the redesign shipped with a different kind of test, one that scaffolds a real project, adds a nested command, and actually builds it. Its own comment says the quiet part out loud:

The previous test suite asserted file-content shapes but never tried to go build the generated module, so the nested-command path that referenced undefined props.<Name>Cmd symbols compiled cleanly in tests and broke only when downstream users built their tools.

It’s gated behind an integration flag, because it shells out to the Go toolchain and that’s too heavy for every unit run, but it closes the exact gap that hid the bug. The only real test of a code generator is whether its output compiles.

What it comes down to

Three times in one bug I had to change my mind. I’d decided cobra shouldn’t be abstracted; the evidence said abstract it. I’d reached for the one-line patch; the evidence said redesign. I’d trusted tests that read the generated code; the evidence said build it. None of those were comfortable, and the version reset is the only reason the timing was kind.

Both technical lessons are worth keeping. When the same derived value is threaded through every call, the abstraction is in the wrong place. And the only proof that something which writes code actually works is to compile what it writes. But the one underneath both, the one I apparently have to keep relearning, is simpler than either: don’t get so attached to an implementation that you can’t change your mind when the evidence says it doesn’t fit. The framework is better for it, and it now has a composition seam to hang the features cobra doesn’t give us natively. A nested command that wouldn’t build was just the thing that finally made me look.

Built with Hugo
Theme Stack designed by Jimmy