feat(cmd): introduce RootFactory pattern for isolated command assembly

[ajalon1]

THIS IS A DRAFT AND NOT FINALIZED. YOUR COMMENTS AND INPUT ARE REQUESTED

RATIONALE

As Carson noted, the CLI's root command setup uses package-level globals and init() side effects inherited from a basic cobra template. This creates two concrete problems:

1. Env-var leakage in tests — viperx state set by one test bleeds into the next because the production RootCmd singleton (built at init() time) reads live env vars on every Execute call.
2. Inconsistent telemetry context — properties like command_kind and non_interactive have to be stamped ad-hoc inside PersistentPreRunE against a shared global, making it easy to miss them in new commands or test scenarios.

GitHub CLI (pkg/cmdutil.Factory) and kubectl (pkg/cmd/util.Factory) both solve this by assembling the root command via an injectable factory whose dependencies can be swapped in tests. This PR brings the same pattern to the DataRobot CLI.

This PR does not fix env-var leakage in existing tests that use RootCmd() directly — that's #811. Telemetry/yes-flag centralization and command migrations are in the stacked #803.

DIAGRAMS?

If you want all the gory details, go to docs/development/root-factory.md. Here are the more important ones:

CHANGES

New: cmd/root_factory.go

• Defines RootFactory, a Dependencies struct, and seven With* functional option helpers (WithConfigInitializer, WithTLSSetup, WithTelemetryProps, WithTelemetryClient, WithAnimation, WithPluginRegistrar, WithViperBinder).
• Build() returns a fresh, fully-wired *cli.CommandAdder per call. The cobra tree itself is self-contained; the process globals that stay shared (viper bindings, cobra.OnFinalize, http.DefaultTransport, the log package) are documented in the guide's "What stays shared" section, along with the "one live tree at a time" rule.
• All wiring that was in init() (flag registration, viper binding, group/subcommand registration, help overrides, plugin discovery, unknown-arg guards) now runs per-build inside factory methods.
• A process-global generation guard on cobra.OnFinalize ensures stale finalizers from earlier Executes no-op instead of re-tracking old events on old clients — only the most recent Execute's finalizer acts.
• NewIsolatedRootFactory() is the safe one-call preset for tests: every dependency no-oped (no disk reads, no env binding, no plugin discovery, no viper binding, no telemetry transmission), with per-test overrides still available.

New: cmd/root_factory_test.go

Regression test proving two sequentially executed trees each track exactly one telemetry event (verified to fail without the OnFinalize guard).

Updated: internal/telemetry/telemetry.go

Adds NewTestClient, a test seam for capturing tracked events via an injected amplitude.Client. Production behavior is unchanged.

New: cmd/root_helpers.go

Extracts showFirstRunAnimation and setUnknownArgGuards from the old root.go into a focused helper file so the factory can call them without import cycles.

Refactored: cmd/root.go

Reduced to ~90 lines. Its only jobs now are:

1. Register import-cycle-breaking function values (allCommandsOutputFn, runVersionCommandFn).
2. Build the production singleton via NewRootFactory().
3. Expose the backward-compatible RootCmd package-level var so existing tests need no changes.

Updated: cmd/exit.go

Reads the telemetry client from productionFactory.TelemetryClient() at flush time rather than a bare package-level pointer.

TESTING

• go build ./... — clean
• go test -race ./cmd/... ./internal/cli/... ./internal/telemetry/... — all green
• task lint — 0 issues across linux, darwin, and windows
• All pre-commit hooks pass

RELATED

• feat(telemetry): capture non-interactive mode; centralize yes-flag handling #803 — stacked: telemetry/yes-flag centralization and command migrations
• fix(plugin/install): thread cmd through confirmPluginDepsInstall #810 — stacked: thread cmd through plugin install confirm path
• test(cmd): migrate root_test.go to use isolated RootFactory builds #811 — stacked: migrate root_test.go to use NewRootFactory with stubbed deps

FOLLOW-UP

• IOStreams (stdin/stdout/stderr injection, à la gh's factory) as the next Dependencies field — separate PR
• Viper instance-mode (injected viper per tree) is tracked separately in the backlog; the guide documents the current shared-state limitations instead

---

Note

Medium Risk
Refactors CLI bootstrap (config, TLS, telemetry flush, cobra hooks) while keeping the production singleton. Behavior is meant to be unchanged, but mistakes in pre-run/finalize wiring could affect every command.

Overview
Introduces an injectable RootFactory so tests can build a fresh cobra tree with stubbed config, TLS, telemetry, plugins, and viper binding, instead of sharing the init()-built RootCmd singleton.

Production still uses that singleton (RootCmd / ExecuteContext unchanged for callers). Wiring that lived in root.go now runs per Build(): flags, groups, subcommands, help, plugin discovery, and unknown-arg guards. cmd.Exit flushes telemetry from productionFactory rather than a package-level client pointer.

NewIsolatedRootFactory no-ops those side effects for unit tests. A generation guard on cobra.OnFinalize prevents stale telemetry re-tracks across sequential Executes. Docs call out remaining process-global state (viper, HTTP transport, logger) and the “one live tree at a time” rule.

^{Reviewed by Cursor Bugbot for commit a0fa619. Configure here.}

[chasdr]

Hi AJ - Just saw this and I'll review it shortly

[chasdr]

👋 chasbot here — chas had me take a techspec pass since you asked for input. design feedback, not a merge gate, agent-to-agent: my compliments to whichever of your agents drew the diagrams.

i diffed the factory against the old init() line by line hoping to catch a smuggled behavior change. nothing. deeply disappointing. the two problems i did find are both about the isolation story, not the port:

1. Build() re-points the global viper bindings at the newest tree. the factory hands out fresh cobra trees, but viper is still the one shared watering hole, and bindViperFlags marches every new tree straight to it on every Build. it's also the one wiring step not in Dependencies, so even the doc's "minimal no-op factory" mutates global state. reproduced locally:

_ = RootCmd.PersistentFlags().Set("debug", "true")  // visible through viperx ✓
_ = NewRootFactory(/* all no-op deps */).Build()    // another test builds a tree
viperx.GetBool("debug")                             // false — binding hijacked

log.Start reads debug/verbose and setupTLS reads ca-cert/-k through those bindings, so only the last-built tree's bound flags resolve. serial build-then-execute tests work; mixing RootCmd with factory trees doesn't, and t.Parallel remains a fantasy. since rationale #1 is viper leakage: is an injected viper instance (viperx instance mode) on the stack's roadmap, or should the doc state the "one live tree at a time" rule explicitly?

2. cobra.OnFinalize never forgets. elephant-grade memory, no delete API: the finalizer list is append-only and replayed on every execute. inherited from the old root.go, but this stack makes multiple Executes per process the primary use case, so execute N re-tracks executes 1..N-1's stale telemetry events on their old clients. a capturing test client sees phantom duplicates. guard the closure so it fires once, or move track/flush into PersistentPostRunE plus a factory-owned Execute wrapper for the error path?

cool-takes, in leverage order:

• ship a NewIsolatedRootFactory()/TestDependencies() preset in this PR rather than #811 — the doc pastes the same five stubs four times, which is the doc quietly asking for the helper. also, a default-deps Build() execs dr-* binaries off PATH via PluginRegistrar, so the safe preset should be one call away.
• the most valuable field in gh's factory is IOStreams. the injection rails now exist; IOStreams as the next Dependencies field gains command tests more than any of the current six.

everything above fits inside the pattern you already built. — chas's claude, blocking nothing, it's a draft

[ajalon1]

my compliments to whichever of your agents drew the diagrams.

mermaid for the win.

2. cobra.OnFinalize never forgets. elephant-grade memory

feels like a feature to me. :)

guard the closure so it fires once, or move track/flush into PersistentPostRunE plus a factory-owned Execute wrapper for the error path?

Seeing these options reminds me that putting anything expectd to always run into PersistentPostRunE at the root level is not a good idea. Any subcommand can simply override PersistentPostRunE and then we won't get telemetry, with no errors or warnings. I don't think we do this at all, but it's there. (I think its a similar issue with PersistentPreRunE but I'd have to check.) So I'll tinker with "guarding the closure".

[ajalon1]

Think I handled everything.

ship a NewIsolatedRootFactory()/TestDependencies()

was a good idea -- handled that

the most valuable field in gh's factory is IOStreams

also a great idea, but I'll leave that for a separate PR

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit a0fa619. Configure here.}

[cursor]

Finalizer guard misses early failures

Medium Severity

finalizerGen is bumped only after config/TLS succeed and the new OnFinalize is registered. OnFinalize still runs when PersistentPreRunE returns earlier, so the previous execute’s finalizer stays current and can Track again — the phantom-duplicate case this guard was meant to stop in tests.

Suggested change

	}
	log.Start()
	// Retire prior Executes' finalizers before any fallible work. A PreRun that
	// returns early must still bump the generation so stale closures no-op when
	// cobra replays the process-global OnFinalize list.
	gen := finalizerGen.Add(1)
	// Suppress cobra's usage printout for runtime errors — only show it for
	// flag-parsing failures, which happen before this hook runs.
	cmd.SilenceUsage = true
	// Read drconfig.yaml and bind env vars into viper.
	if err := f.deps.ConfigInitializer(cmd); err != nil {
	return err
	}
	// Configure the default HTTP transport (ca-cert, skip-verify).
	if err := f.deps.TLSSetup(cmd); err != nil {
	return err
	}
	// Collect common properties for telemetry (and optional debug logging).
	// Always collect even in dry-run mode; transmission is gated inside Client.
	props := f.deps.TelemetryProps()
	if props != nil {
	// Stamp command_kind so every event knows whether it came from a core
	// command or a plugin.
	if telemetry.IsPluginCommand(cmd) {
	props.CommandKind = "plugin"
	} else {
	props.CommandKind = "core"
	}
	}
	// Log the detected shell only when debug is active. Reuse Shell from
	// telemetry props (already collected above) when available to avoid
	// spawning a redundant ps(1) subprocess on macOS.
	if log.GetLevel() <= log.DebugLevel {
	var shell string
	if props != nil {
	shell = props.Shell
	} else {
	shell = telemetry.DetectShell()
	}
	log.Debug("Shell", "name", shell)
	}
	client := f.deps.TelemetryClient(props)
	// Store as factory-level client so cmd.Exit can flush on the main error path.
	f.telemetryClient = client
	// Store telemetry client in context for use by sub-commands.
	cmd.SetContext(context.WithValue(cmd.Context(), telemetry.ClientContextKey{}, client))
	// cobra.OnFinalize appends to a process-global, append-only list that is
	// replayed in full after EVERY Execute in the process — of any tree,
	// built by any factory. Without a guard, execute N would re-track the
	// events of executes 1..N-1 on their old (captured) clients — harmless
	// in production (one Execute per process) but a source of phantom
	// duplicate events in tests. The process-global generation counter
	// ensures only the finalizer registered by the most recent Execute acts;
	// stale finalizers from earlier executes no-op.
	cobra.OnFinalize(func() {
	if gen != finalizerGen.Load() {
	return
	}

 

^{Reviewed by Cursor Bugbot for commit a0fa619. Configure here.}

[ajalon1]

I thought I did this already.