feat(root): add --no-color persistent flag

[rianjs]

Summary

• New --no-color persistent flag on the gro root.
• Existing PersistentPreRunE at internal/cmd/root/root.go:44 extended (not replaced) — preserves log.Verbose and WireBackendSelection wiring.
• When --no-color is set, switches lipgloss.DefaultRenderer() to termenv.Ascii so every Style.Render call in this process emits plain text.
• NO_COLOR env behavior (handled natively by lipgloss/termenv) is unchanged — the flag is an additive, discoverable opt-out.

Why

§8 of cli-common output-and-rendering.md requires --no-color as the documented opt-out alongside NO_COLOR env. gro was the only CLI in the family missing the flag (per the divergence catalog).

Test plan

• make build — bin/gro clean.
• make check (tidy+lint+test) — 24 tests green across view + root packages.
• New view tests:
  • TestSuccessUnderAsciiProfileEmitsNoANSI — proves Ascii profile suppresses ANSI.
  • TestSuccessUnderANSIProfileEmitsANSI — regression proving the test setup isn't env-determined.
  • TestErrorUnderAsciiProfileEmitsNoANSI — same for Error (stderr).
• New root tests:
  • TestNoColorFlagRegistered — flag exists, type bool, persistent.
  • TestPersistentPreRunE_NoColorTrueFlipsToAscii — end-to-end: forces ANSI baseline, sets noColor = true, calls PersistentPreRunE, asserts ColorProfile() == Ascii.
  • TestPersistentPreRunE_NoColorFalseLeavesRendererUntouched — regression: noColor = false doesn't mutate the renderer.
• All renderer-mutating tests use t.Cleanup to restore lipgloss.SetDefaultRenderer(saved) and reset the noColor package var — no state leak across tests.

Out of scope (per §8)

• No isatty gating. §8 is explicit: "No CLI gates color on isatty. This is intentional."

Closes #143

[rianjs]

Review

Minor: The production code matches the ticket, but the root tests are weaker than the PR description says. TestPersistentPreRunE_NoColorTrueFlipsToAscii manually sets the package variable noColor = true and calls rootCmd.PersistentPreRunE(...); it does not prove that Cobra parses the persistent --no-color flag and wires it into that variable through the real command path.

For this ticket, the load-bearing behavior is the root flag. I’d prefer one probe-style test that adds a no-op child command, runs rootCmd.SetArgs([]string{"probe", "--no-color"}) or []string{"--no-color", "probe"}, executes the command, and asserts the renderer becomes termenv.Ascii. That test should also clean up the probe child, rootCmd.SetArgs(nil), noColor, and any persistent flag state to avoid order-dependent leakage with the existing backend flag tests.

Everything else looks aligned: the existing PersistentPreRunE is extended rather than replaced, there is no isatty gating, NO_COLOR behavior remains untouched, and moving termenv to a direct dependency is appropriate now that production code imports it.

[rianjs]

Coverage is solid — all critical paths are tested with proper isolation hygiene.

Minor

• TestPersistentPreRunE_NoColorTrueFlipsToAscii calls rootCmd.PersistentPreRunE directly, bypassing cobra's flag parser. This is intentional (the Cobra-wiring path is covered by TestNoColorFlagThroughCobra), but the two tests together leave a gap: there's no assertion that the renderer is still Ascii after TestNoColorFlagThroughCobra cleans up (i.e., that withRenderer's t.Cleanup actually restores ANSI). Not a real risk given Go's cleanup ordering guarantees, but a future reader may wonder.
• view_test.go tests only Success and Error under Ascii; Info emits color via infoStyle and is untested under the Ascii profile. Low-risk (same render path), but leaves a gap if Info is ever styled differently.

Nit

• withRenderer is duplicated verbatim between root and view packages. Could live in internal/testutil if this pattern spreads further.

[monit-reviewer]

Automated PR Review

Reviewed commit: b253302

Approved with 2 non-blocking suggestions below. Address at your discretion.

Summary

Reviewer	Findings
harness-engineering:harness-architecture-reviewer	1
harness-engineering:harness-enforcement-reviewer	1

harness-engineering:harness-architecture-reviewer (1 findings)

💡 Suggestion - internal/cmd/root/root_test.go:116

withRenderer is duplicated verbatim in internal/view/view_test.go (~line 71). The project already uses internal/testutil/ as the canonical location for shared test infrastructure. Duplicate helpers risk silently diverging — e.g., one copy picks up t.Helper() or a different cleanup order. Should be extracted to internal/testutil/ and imported by both test packages.

harness-engineering:harness-enforcement-reviewer (1 findings)

💡 Suggestion - internal/cmd/root/root_test.go:162

TestNoColorFlagThroughCobra cleanup resets the noColor Go variable but not cobra/pflag's internal flag state. After Execute(), the pflag.Flag for --no-color retains Changed=true. On any subsequent Execute() call that omits the flag, pflag internal value stays stale — flag.Changed stays true and help output would show the default as "true". Fix: add rootCmd.PersistentFlags().Set("no-color", "false") to the t.Cleanup block.

3 info-level observations excluded. Run with --verbose to include.

2 PR discussion threads considered.

---

Completed in 3m 39s | $1.01 | sonnet | daemon 0.2.121 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	3m 39s wall · 9m 58s compute (Reviewers: 2m 49s · Synthesis: 48s)
Cost	$1.01
Tokens	209.3k in / 35.6k out
Turns	10

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	29.7k	2.9k	13.2k	16.4k (1h)	$0.12
harness-engineering:harness-architecture-reviewer	sonnet	42.1k	8.7k	21.0k	21.2k (1h)	$0.22
harness-engineering:harness-enforcement-reviewer	sonnet	41.5k	9.5k	6.9k	34.6k (1h)	$0.28
harness-engineering:harness-knowledge-reviewer	sonnet	38.9k	5.4k	21.0k	18.0k (1h)	$0.16
harness-engineering:harness-self-documenting-code-reviewer	sonnet	40.1k	7.5k	20.5k	19.6k (1h)	$0.20
security:security-code-auditor	haiku	16.9k	1.5k	0	16.9k (1h)	$0.03

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

[monit-reviewer]

🔵 Low (harness-engineering:harness-architecture-reviewer): withRenderer is duplicated verbatim in internal/view/view_test.go (~line 71). The project already uses internal/testutil/ as the canonical location for shared test infrastructure. Duplicate helpers risk silently diverging — e.g., one copy picks up t.Helper() or a different cleanup order. Should be extracted to internal/testutil/ and imported by both test packages.

Reply to this thread when addressed.

[monit-reviewer]

🔵 Low (harness-engineering:harness-enforcement-reviewer): TestNoColorFlagThroughCobra cleanup resets the noColor Go variable but not cobra/pflag's internal flag state. After Execute(), the pflag.Flag for --no-color retains Changed=true. On any subsequent Execute() call that omits the flag, pflag internal value stays stale — flag.Changed stays true and help output would show the default as "true". Fix: add rootCmd.PersistentFlags().Set("no-color", "false") to the t.Cleanup block.

Reply to this thread when addressed.

[rianjs]

No architectural drift.

The daemon-driven changes stayed in the expected lane: production code still only adds the root --no-color persistent flag and extends the existing PersistentPreRunE without disturbing verbose/backend wiring. The added Cobra-path test improves confidence that the flag binding actually reaches the renderer. The extra Info view test is still directly tied to §8 color behavior.

The two daemon Low findings are test hygiene only: helper duplication and persistent pflag state cleanup. They do not change the architectural shape or cross-ticket assumptions.