refactor(cmd/morphic): dispatch subcommands via a command table

[fuad-daoud]

Summary

Groundwork for the help fix in the next PR, split out because it is a structural change that can be read on its own.

The single compile subcommand moves into a commands table that dispatch resolves against, and its flag binding is separated from its execution. The point of the table is the flagSet func() *flag.FlagSet field: help rendering and argument parsing draw from the same FlagSet, so a documented flag list can never drift from the flags that are actually accepted.

Silencing the flag package's own output is part of this move — compile's FlagSet now writes to io.Discard and runCompile renders the parse error itself. A bad flag therefore prints one reason line and one usage block instead of flag's error, flag's usage dump, and the CLI's usage.

runCompile went from one 44-line function doing flag binding, validation, engine construction and output to runCompile plus compileSpec.

Two things this PR does not leave in a good state

Both are fixed by #205, which is why that PR is stacked directly on this one:

• -h and --help get worse. Discarding the FlagSet's output also discards the flag table -h used to print, so a help request now surfaces the stdlib sentinel morphic: flag: help requested. That is not a reason anyone can act on. --bogus also changes, for the better. The claim that this PR changes no CLI behaviour was wrong and has been removed.
• usage and description on the table entry are unread. Nothing renders them until the help PR lands, so they cannot be taken as live help text. That is also why usage says [flags] while main.go's usage const spells them out — the two are already out of step, and fix(cmd/morphic): print help on stdout and exit 0 #205 deletes the const.

Test plan

• Every pre-existing test in cmd/morphic passes unmodified — that was the acceptance criterion for this refactor.
• New TestLookup_KnownAndUnknown covers table resolution.
• New TestCommands_TableIsWellFormed holds the table invariants lookup relies on. Verified by planting each defect: a blank name fails on Should NOT be empty, a duplicate compile entry fails on duplicate command "compile".
• TestNewCompileFlags_DefinesEveryFlag and TestCommand_FlagSetBindsTheCommandsOwnFlags compare the whole flag set with ElementsMatch rather than checking a subset. Verified by planting a fifth flag that no list mentions: both go red, where the subset form left both green.
• gofmt -l, go vet ./..., golangci-lint run (0 issues), go build ./..., ./scripts/check-coverage.sh — 4198/4198 statements.

[OmarAlJarrah]

Findings inline. Two with no line to hang them on:

• Title is over the cap. 67 chars + GitHub's (#204) = 74. refactor(cmd/morphic): dispatch subcommands via a command table is 70.
• Strip the AI trailers. d3d9948 carries Co-Authored-By: and Claude-Session:, and the body ends with the generated-with line and a session URL. Needs an amend + force-push, not just a body edit.

Mechanics are otherwise fine: test-merges clean onto main, and the full gate passes on the merged state — golangci-lint 0 issues including #226's new caps, coverage 4201/4201.

[OmarAlJarrah]

-h and --help land here, so they now print morphic: flag: help requested. That's a stdlib sentinel, not a reason anyone can act on. Before this PR they got the flag table.

Suggested change

	emitf(stderr, "morphic: %v\n", err)
	if !errors.Is(err, flag.ErrHelp) {
	emitf(stderr, "morphic: %v\n", err)
	}

Needs "errors" in the import block. That's outside the diff so I can't suggest it from here.

Separately, the body says this changes no CLI behavior. It changes this path and --bogus. Fix that line and call out that -h stays broken until the help PR.

Worth a test either way. Nothing in the suite touches -h, which is how the output could change without a single red line:

func TestRunCompile_HelpFlagPrintsUsageOnly(t *testing.T) {
	t.Parallel()
	var stdout, stderr bytes.Buffer

	code := run([]string{"compile", "-h"}, &stdout, &stderr)

	assert.Equal(t, 2, code)
	assert.True(t, strings.HasPrefix(stderr.String(), "usage:"),
		"flag.ErrHelp must not surface as a reason line, got: %s", stderr.String())
}

[fuad-daoud]

Fixed the body claim rather than the code. The errors.Is branch is #205's job — adding it here only to have #205 restructure it around writeCommandHelp would make this PR harder to read, not easier. The body now says outright that -h is worse than before on this commit, and the commit message says the same.

[OmarAlJarrah]

This says [flags]. The usage const in main.go spells them out. So there are two help texts already out of step, and nothing renders this one yet.

Fine as groundwork, but put a line in the PR body saying it's unread until the help change lands, so the next reader doesn't take it for live.

[fuad-daoud]

Added to the body: usage and description are unread until #205 renders them, so they can't be taken as live help text. That also covers why this one says [flags] while main.go's usage const spells them out — #205 deletes the const.

[OmarAlJarrah]

The \n is baked into the data, so this text can never rewrap. Break the source line instead of the string:

Suggested change

	description: "Lower an API spec (OpenAPI 3.x) into Morphic IR JSON on stdout, and write\n" +
	"diagnostics to stderr.",
	description: "Lower an API spec (OpenAPI 3.x) into Morphic IR JSON on stdout, " +
	"and write diagnostics to stderr.",

[fuad-daoud]

Pushing back on this one. Your rewrite unwraps the first sentence to ~99 chars but leaves the --explain paragraph below it hard-wrapped, so the description ends up inconsistently wrapped — one sentence relying on the terminal, three lines not. And the \n\n between them is load-bearing regardless, so the data carries some layout either way.

Given #207 pins the render as a golden, I read the wrapping as deliberate rather than accidental: the output is the same at any terminal width, and the golden diff stays readable. Happy to unwrap the whole description — all four lines, plus the \n\n becoming something structural — if you think that trade is better, but I don't want to half-do it. Left as is for now.

[OmarAlJarrah]

Drop this. c.name == "" is already false for every entry, so lookup("") misses without it. I deleted the guard locally and the package suite plus the 100% coverage gate both stayed green.

Suggested change

	if name == "" {
	return command{}, false
	}

The doc comment two lines up still promises "The empty name never resolves", so trim that sentence with it.

If the point was catching a blank entry in the table, that's a table invariant and belongs in the test where it can actually fail:

seen := make(map[string]bool, len(commands))
for _, cmd := range commands {
	require.NotEmpty(t, cmd.name)
	require.False(t, seen[cmd.name], "duplicate command %q", cmd.name)
	seen[cmd.name] = true
}

That picks up duplicates too, which the linear search currently resolves to the first entry without a word.

[fuad-daoud]

Dropped, and the doc sentence with it. Confirmed the guard was dead: removing it leaves the package suite green at 100% coverage, since no table entry has an empty name.

Took the table-invariant test too, as TestCommands_TableIsWellFormed. Verified both halves by planting the defect — a blank name fails on Should NOT be empty, a duplicate compile entry on duplicate command "compile".

[OmarAlJarrah]

This only checks the listed names are a subset of what's defined, so it can't fail when a flag is added, which is what the name promises. I added a fifth flag to newCompileFlags and left it off the list; both flag tests stayed green.

Suggested change

	for _, name := range compileFlagNames {
	assert.NotNil(t, fs.Lookup(name), "flag %q must be defined", name)
	}
	var got []string
	fs.VisitAll(func(f *flag.Flag) { got = append(got, f.Name) })
	assert.ElementsMatch(t, compileFlagNames, got)

Needs "flag" in the imports.

[fuad-daoud]

Done, both here and in TestCommand_FlagSetBindsTheCommandsOwnFlags. Reproduced your mutation first: a fifth flag left off compileFlagNames kept all three flag tests green. With ElementsMatch both go red on elements differ.

[OmarAlJarrah]

c.flagSet is newCompileFlags, and runCompile calls the constructor directly rather than going through the table, so nothing can drift yet. Say what the test actually holds:

Suggested change

	// The table's flagSet is what help rendering reads. It must produce the same
	// flags Parse accepts, or documentation drifts from behaviour.
	// The table's flagSet is what help rendering reads. Today it just calls
	// newCompileFlags; this holds the line if the table ever gets its own.

[fuad-daoud]

Reworded to your version.

[OmarAlJarrah]

A few simplifications on top of the earlier pass. All three are verified against the tree, not just read.

[OmarAlJarrah]

Dead line. SetOutput(io.Discard) already swallows the default usage dump, so this adds nothing; with ContinueOnError the flag package writes its usage to Output(), which is io.Discard. I dropped it and the output on -h, --bogus and --fail-on with no value is byte for byte the same.

Suggested change

fs.Usage = func() {}

Keeping both means the next reader has to work out which one actually silences it.

[fuad-daoud]

Dropped. Confirmed the same way you did — -h, --bogus and --fail-on with no value are byte-identical without it.

Also rewrote the newCompileFlags doc to name io.Discard as the whole of the silencing and say why, so the next reader doesn't re-add the override.

[OmarAlJarrah]

Handing out a *flag.FlagSet is wider than help rendering needs, and it's a live footgun: anyone can call Parse on it, which writes into a compileOptions nobody is holding, and the values vanish silently.

Help only ever needs the flag table, so the field could say that:

// printFlags writes the command's flag table to w.
printFlags func(w io.Writer)

printFlags: func(w io.Writer) {
	fs, _ := newCompileFlags()
	fs.SetOutput(w)
	fs.PrintDefaults()
},

Same single source as today, narrower contract, and it can't be misused. It also drops the awkwardness where runCompile has to ignore the table's flagSet and call newCompileFlags itself, because it needs the options handle too. Your call whether that belongs here or in the help PR.

[OmarAlJarrah]

These three blocks and the unknown-command block in main.go are the same three lines four times over: reason, usage, exit 2. Worth single-sourcing while you're in here:

// usageError renders an optional reason line plus the usage block and returns
// the exit code for a malformed invocation. An empty reason prints usage alone.
func usageError(stderr io.Writer, reason string) int {
	if reason != "" {
		emitf(stderr, "morphic: %s\n", reason)
	}
	printUsage(stderr)
	return 2
}

Call sites collapse to one line each:

if errors.Is(err, flag.ErrHelp) {
	return usageError(stderr, "")
}
return usageError(stderr, err.Error())
...
return usageError(stderr, fmt.Sprintf("invalid --fail-on %q (want error or warning)", opts.failOn))
...
return usageError(stderr, "compile requires exactly one spec file")

I ran it: existing tests pass untouched, lint clean, and cmd/morphic drops 6 statements. No suggestion block because the helper lands next to printUsage, outside the diff.

One thing it exposes, which is the real argument for it: written this way the ErrHelp branch is its own statement, and the coverage gate fails on it until -h has a test. That's the gap working as intended.

[fuad-daoud]

Force-pushed with the review items addressed; the stack is rebased on top so #205 and #207 carry the same base.

Also stripped the session trailer and the generated-with line, which were plain violations of the repo's "no LLM/session artifacts" rule. Left Co-Authored-By: for now — say the word and it goes too.

Gate on the merged state: gofmt clean, go vet clean, golangci-lint 0 issues, build clean, coverage 4198/4198 (down 3 statements from the two dead branches you found).

One thing I found while checking your title measurement: #207's own title was over the cap too — 68 chars + (#207) = 75. Retitled to test(cmd/morphic): pin the rendered help text with goldens (65).