cli: add CLI framework package

[forge-coding-environment]

Summary

• Adds a new cli package: a struct-driven CLI framework that replaces Cobra+Viper
• Commands are Go structs with tagged fields that become flags and config file bindings automatically
• Implements Configure-Validate-Run lifecycle with native loader and await integration
• Supports POSIX-style flags, embedded struct composability, middleware, and auto-generated help

Design

• cli:"name,alias" struct tags define flags
• config:"json.path" struct tags bind fields to config file sections
• Precedence: explicit CLI flag > config file value > default
• Validator interface is opt-in (no empty Validate methods)
• Middleware for cross-cutting concerns (logging, tracing)
• WithConfigFlag loads one root config file via loader.LoadConfig

Test plan

• Review struct tag parsing and flag generation
• Review config file loading and precedence logic
• Review help output formatting
• Run go test ./cli/...

[abraithwaite]

Team Review

Product Manager with Taste

Verdict: Approve with suggestions

• The API is dramatically simpler than Cobra+Viper. A command is a struct with tags and a Run method — that's it. A new engineer can understand a command in seconds.
• The example program is excellent — demonstrates globals embedding, config file loading, middleware, positional args, groups, and validation in 117 lines.
• CommandWithOptions is awkward. The []CmdOption slice parameter feels clunky compared to the clean variadic pattern elsewhere. Consider eliminating it and folding options into Command more naturally.

Principal Software Engineer

Verdict: Needs changes

• Overall architecture is sound — clean separation between cli.go (orchestration), flags.go (parsing), reflect.go (struct scanning), config.go (file loading), help.go (output).
• handlerFlagSets global sync.Map (cli.go:73) is a code smell. It maps handler pointers to flag sets so IsSet() works. This couples IsSet() to the execution lifecycle via global state. Cleaner: pass the FlagSet through context, or make IsSet a method on a context-carried type.
• DumpConfig (cli.go:431) returns "(set)" as the value for explicitly-set flags instead of the actual value. This defeats the purpose of a config dump for debugging.
• config.go uses encoding/json for unmarshaling config sections but the config was processed by loader.LoadConfig using segmentio/encoding/json. This inconsistency could cause subtle behavior differences.
• flags.go type switch (~180 lines) for makeFlagDef could be simplified — the pointer-type variants double the code unnecessarily.
• Tests: 693 lines is solid coverage. Config precedence test (TestConfig_CLIFlagOverridesConfig) is present and critical. However, tests aren't table-driven where they could be.

Blocking changes:

1. config.go: Use github.com/segmentio/encoding/json instead of encoding/json for unmarshalIntoField to match loader.
2. DumpConfig should return actual field values, not "(set)".

Devil's Advocate

Verdict: Suggestions

• No test for HuJSON features in config files. The config path goes through loader.LoadConfig which supports comments and trailing commas, but no test verifies this end-to-end.
• No test for env var replacement in config files. This is a key feature of loader.LoadConfig — the test suite should verify "$MY_VAR" replacement works through the CLI framework.
• Embedded struct diamond problem: If two embedded structs both define cli:"verbose", buildFlagSet returns "duplicate flag --verbose" — correct behavior, but the error message doesn't tell you which embedded structs conflict.

Sysadmin Greybeard

Verdict: Approve with suggestions

• Panic recovery with stack trace logging via slog is exactly right.
• Help output is clean and shows defaults.
• Config file not-found behavior is correct: silently skips if not explicitly set, errors if it was.
• Consider adding --dump-config as a built-in flag for operational debugging.

Summary

Overall: Approve with changes

Top priorities (blocking):

1. Fix encoding/json inconsistency in config.go — use segmentio/encoding/json to match loader
2. Fix DumpConfig returning "(set)" instead of actual values

High-value (non-blocking):
3. Eliminate CommandWithOptions — fold options into Command more naturally
4. Add tests for HuJSON config and $ENV_VAR replacement through the framework
5. Replace handlerFlagSets global with context-carried state

Nice-to-haves:
6. Table-driven test refactoring
7. Built-in --dump-config flag
8. Better error messages for duplicate flags from embedded struct conflicts

CI note: Build failure is unrelated — golines requires Go 1.23+ but CI runs 1.21.

[abraithwaite]

Final Team Review (Pre-Merge)

Product Manager

Verdict: Approve

• API is clean and intuitive — struct tags, Command/Group, WithGlobals, lifecycle interfaces. A Go developer will feel at home.
• README accurately reflects the implementation. Code examples match the actual signatures.
• defcon is whimsical but the help footer makes it discoverable. Overridable via WithDefaultConfigCommand for serious contexts.
• Command(...any) silently dropped bad types — fixed: now panics with a descriptive message.

Principal Engineer

Verdict: Approve (blocking items fixed)

Fixed in latest commit:

• Command() now panics on unsupported option types instead of silently swallowing them
• defcon with extra args (e.g., --help) no longer falls through to "unknown command"
• Added aliasing comment on reflect.go:36 slice construction

Non-blocking notes for future iterations:

• io.Closer is only called on globals, not handlers — worth documenting
• []string flag + default tag interaction (appends, doesn't reset) could surprise — consider a test
• config:"key" collision between globals and handler both get populated; undocumented but harmless
• GoDoc coverage is good across all exports
• All README code examples match the implementation

Devil's Advocate

Verdict: Approve

• Globals mutation across multiple app.Run() calls (e.g., in tests) retains state. Not a real-world issue since CLIs run once, but worth a doc comment on WithGlobals eventually.
• No race conditions — App is single-goroutine by design, globals pointer is immutable in context.
• replaceEnvInJSON duplication between cli and loader is documented with a clear comment explaining why both exist (pinned loader version may not have it).

Sysadmin Greybeard

Verdict: Approve

• Error messages are actionable: flag names, file paths, phase names all included.
• Panic recovery with stack traces via slog — production services won't die silently.
• Help footer shows defcon when a default config is registered — good discoverability.
• Completion scripts are correct for bash/zsh. Fish strips descriptions unnecessarily (minor).
• --version only works at top level as sole arg — matches cobra behavior.

Summary

Overall: Approve

All three blocking items from review have been addressed. The package is well-tested (84% coverage), well-documented, and the API surface is clean. The loader integration (List, Describe, Helper) is minimal and well-designed.

Remaining nice-to-haves (post-merge):

1. Document io.Closer only on globals in README lifecycle section
2. Test []string flag + default interaction
3. Fish completion could pass descriptions through instead of stripping