This PR does exactly what MIR-1823 describes: it pins the "nicer unknown command" behaviour with a regression test suite, and bumps miren.dev/mflags (from 20260910 → 20260911) to the version that actually implements the improvement. The implementation lives in the upstream library; the runtime repo's job here is to test the contract.
What I read through:
The test file (cli/commands/unknown_command_test.go) covers the meaningful cases cleanly:
depoy) — must name the wrong word, suggest the right one, and link to --help.app lst, runner upgrde) — same treatment, with the parent path in the error.version foo) — should use unexpected argument "…" rather than the old error parsing flags: …[foo] message.TestKnownCommandsAreNotMistakenForTypos) — --help, help, pass-through args, and value-taking global flags must not be flagged as typos.The dispatchErr helper is tidy and correctly mirrors what the other test files (section_help_test.go, global_test.go) already do with RegisterAll.
One minor observation — not a blocker:
All three top-level tests call labs.EnableAll() at the top of the function with no corresponding t.Cleanup(labs.Reset). The existing tests in help_groups_test.go and section_help_test.go do the same thing — this is the established pattern in this package, not something introduced here. The labs state is package-global and Go tests in the same package run in a single process, so the feature flag leaks across tests. That's already accepted practice here, and since EnableAll is idempotent and these are all tests that want all features on, it doesn't cause any test to fail incorrectly. I'm not raising it as a concern — just noting it matches the existing codebase.
The go.mod/go.sum bump is small and scoped to a single dependency; both the old and new entries share the same /go.mod hash, which is the expected pattern for a patch update to an existing module version.
The test structure, naming, and coverage are solid. The three scenarios I most care about — (1) the test actually exercises the new mflags version, (2) the false-positive guard is present, and (3) the "no guess when nothing is close" case is covered — are all there. This is ready to merge.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1209,
"verdict": "ready",
"event": "approve",
"summary": "This PR does exactly what MIR-1823 describes: it pins the \"nicer unknown command\" behaviour with a regression test suite, and bumps `miren.dev/mflags` (from `20260910` → `20260911`) to the version that actually implements the improvement. The implementation lives in the upstream library; the runtime repo's job here is to test the contract.\n\n**What I read through:**\n\nThe test file (`cli/commands/unknown_command_test.go`) covers the meaningful cases cleanly:\n- A top-level typo (`depoy`) — must name the wrong word, suggest the right one, and link to `--help`.\n- A sub-command typo inside a section (`app lst`, `runner upgrde`) — same treatment, with the parent path in the error.\n- A completely unrecognisable command — error without a guess, no \"Did you mean?\".\n- A leaf command given an unexpected positional (`version foo`) — should use `unexpected argument \"…\"` rather than the old `error parsing flags: …[foo]` message.\n- Unknown flags with and without a close enough match.\n- The false-positive guard (`TestKnownCommandsAreNotMistakenForTypos`) — `--help`, `help`, pass-through args, and value-taking global flags must not be flagged as typos.\n\nThe `dispatchErr` helper is tidy and correctly mirrors what the other test files (`section_help_test.go`, `global_test.go`) already do with `RegisterAll`.\n\n**One minor observation — not a blocker:**\n\nAll three top-level tests call `labs.EnableAll()` at the top of the function with no corresponding `t.Cleanup(labs.Reset)`. The existing tests in `help_groups_test.go` and `section_help_test.go` do the same thing — this is the established pattern in this package, not something introduced here. The `labs` state is package-global and Go tests in the same package run in a single process, so the feature flag leaks across tests. That's already accepted practice here, and since `EnableAll` is idempotent and these are all tests that *want* all features on, it doesn't cause any test to fail incorrectly. I'm not raising it as a concern — just noting it matches the existing codebase.\n\nThe `go.mod`/`go.sum` bump is small and scoped to a single dependency; both the old and new entries share the same `/go.mod` hash, which is the expected pattern for a patch update to an existing module version.\n\nThe test structure, naming, and coverage are solid. The three scenarios I most care about — (1) the test actually exercises the new mflags version, (2) the false-positive guard is present, and (3) the \"no guess when nothing is close\" case is covered — are all there. This is ready to merge.",
"posted_to_pr": true
}