[AI] OSAC-3500: osac CLI help output emits ANSI color codes even when not connected to a terminal (no NO_COLOR/non-TTY detection)

[osac-jira-ai-issue-solver]

Resolves OSAC-3500

Summary

Fixes the osac CLI help output grey/dim color issue and adds a --no-color flag.

The prior fix (commit 1963de9) correctly added NO_COLOR environment variable support and
non-TTY detection, but Glamour's Document.Color still forced body text to render in hardcoded
grey (ANSI 252/234) instead of the terminal's default foreground color. This made help text
hard to read on many terminal color schemes.

Changes

• Clear Document.Color from Glamour's Dark/Light style configs so help body text uses the
  terminal's default foreground color instead of hardcoded grey
• Add --no-color persistent flag to disable colored output from the CLI, complementing the
  existing NO_COLOR environment variable support
• Strengthen test regex from CSI-only (\x1b[...) to any ESC character (\x1b) to catch
  all escape sequences including OSC 8 hyperlinks
• Add tests for --no-color flag behavior on root and subcommand help

Files changed

• fulfillment-service/internal/cmd/cli/help/help_setup.go — flag registration, style fix,
  color logic refactor
• fulfillment-service/internal/cmd/cli/help/help_setup_test.go — regex update, 2 new tests

Test plan

• ginkgo run internal/cmd/cli/help/ — 7/7 pass (5 existing + 2 new)
• ginkgo run -r internal/cmd/cli/ — all 31 suites pass
• gofmt -s -w . — no changes
• uv run dev.py lint — 0 issues
• Manual: ./osac --help | grep -cP '\x1b' → 0 (piped, no escape codes)
• Manual: ./osac --no-color --help | grep -cP '\x1b' → 0 (flag works)
• Manual: NO_COLOR= ./osac --help | grep -cP '\x1b' → 0 (env var works)

[openshift-ci-robot]

@osac-jira-ai-issue-solver[bot]: This pull request references OSAC-3500 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Resolves OSAC-3500

Summary

Fixes the osac CLI help output grey/dim color issue and adds a --no-color flag.

The prior fix (commit 1963de9) correctly added NO_COLOR environment variable support and
non-TTY detection, but Glamour's Document.Color still forced body text to render in hardcoded
grey (ANSI 252/234) instead of the terminal's default foreground color. This made help text
hard to read on many terminal color schemes.

Changes

• Clear Document.Color from Glamour's Dark/Light style configs so help body text uses the
  terminal's default foreground color instead of hardcoded grey
• Add --no-color persistent flag to disable colored output from the CLI, complementing the
  existing NO_COLOR environment variable support
• Strengthen test regex from CSI-only (\x1b[...) to any ESC character (\x1b) to catch
  all escape sequences including OSC 8 hyperlinks
• Add tests for --no-color flag behavior on root and subcommand help

Files changed

• fulfillment-service/internal/cmd/cli/help/help_setup.go — flag registration, style fix,
  color logic refactor
• fulfillment-service/internal/cmd/cli/help/help_setup_test.go — regex update, 2 new tests

Test plan

• ginkgo run internal/cmd/cli/help/ — 7/7 pass (5 existing + 2 new)
• ginkgo run -r internal/cmd/cli/ — all 31 suites pass
• gofmt -s -w . — no changes
• uv run dev.py lint — 0 issues
• Manual: ./osac --help | grep -cP '\x1b' → 0 (piped, no escape codes)
• Manual: ./osac --no-color --help | grep -cP '\x1b' → 0 (flag works)
• Manual: NO_COLOR= ./osac --help | grep -cP '\x1b' → 0 (env var works)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[osac-jira-ai-issue-solver]

AI Session Costs

Session	Cost
New ticket	$4.88
Feedback (1) (no changes)	$0.43
Feedback (1) retry 1 (no changes)	$0.30
Feedback (1) retry 2	$1.44
Feedback (2) (no changes)	$0.89
Feedback (2) retry 1	$1.91
Feedback (3) (no changes)	$0.72
Feedback (3) retry 1 (no changes)	$0.57
Feedback (3) retry 2 (no changes)	$0.83
Feedback (3) retry 3	$1.83
Feedback (4) (no changes)	$0.68
Feedback (4) retry 1 (no changes)	$0.84
Feedback (4) retry 2 (no changes)	$0.57
Feedback (4) retry 3 (unable)	$0.69
Feedback (5) (no changes)	$0.79
Feedback (5) retry 1 (no changes)	$0.80
Feedback (5) retry 2 (no changes)	$0.83
Feedback (5) retry 3 (unable)	$0.73
Feedback (6) (no changes)	$0.66
Feedback (6) retry 1 (no changes)	$0.59
Feedback (6) retry 2 (no changes)	$0.75
Feedback (6) retry 3 (unable)	$0.57
Merge conflict resolution	$0.43
Feedback (7) (no changes)	$0.76
Feedback (7) retry 1 (no changes)	$0.78
Feedback (7) retry 2	$0.91
Feedback (8) (no changes)	$0.78
Feedback (8) retry 1 (no changes)	$0.80
Feedback (8) retry 2 (no changes)	$0.73
Feedback (8) retry 3	$1.26
Feedback (9) (no changes)	$0.74
Feedback (9) retry 1 (no changes)	$0.82
Feedback (9) retry 2 (no changes)	$0.89
Total	$31.20

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:58 AM UTC · Completed 7:13 AM UTC

Commit: 5b0897e · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [error-handling] fulfillment-service/internal/cmd/cli/help/help_setup.go:82 — When GetBool(NoColorFlag) returns an error (e.g., if a future caller of Setup forgets to register the flag), the error is printed to stderr but execution continues with noColorFlagValue = false, silently ignoring the --no-color flag. All three current callers comply with the documented precondition, so this is currently benign — graceful degradation (help with wrong color is better than no help) is reasonable behavior.

• [coupling-contract] fulfillment-service/internal/cmd/cli/help/help_setup.go:39 — Setup() has a runtime contract requiring the root command to pre-register a persistent boolean flag named NoColorFlag. If a new root command omits registration, the error surfaces only at help-render time (stderr message) rather than at startup. The doc comment added in this PR documents the precondition, and the pattern is consistent with hidePrivateSubcommands's similar lookup of the config flag.

Previous run

Review

Findings

Low

• [naming-convention] fulfillment-service/internal/cmd/cli/help/help_setup.go:163 — NoColorFlag and NoColorFlagHelp are exported, but existing flag constants (configFlag, cacheFlag, tenantFlag) use unexported camelCase. The export is justified by cross-package usage but deviates from the local convention.

• [code-organization] fulfillment-service/internal/cmd/cli/help/help_setup.go:163 — Flag constants are conventionally co-located with the registering command (root_cmd.go), not in the help package. The centralized placement is defensible for a cross-cutting flag. See also: [naming-convention] finding at this location.

• [api-consistency] fulfillment-service/internal/cmd/cli/help/help_setup.go:39 — Setup accepts a noColorFlagName parameter, but NoColorFlag is defined in the same package and every caller passes help.NoColorFlag. The parameterization enables testability but adds indirection for a currently-fixed value.

• [test-adequacy] fulfillment-service/internal/cmd/cli/help/help_setup_test.go:68 — The new --no-color tests verify the flag is accepted without error but do not exercise the TTY color-suppression path (bytes.Buffer is not a TTY). The test comments acknowledge this limitation.

• [constant-grouping] fulfillment-service/internal/cmd/cli/help/help_setup.go:163 — The two new constants use standalone const declarations rather than a grouped const (...) block, which is the convention for related constants in this codebase.

• [convention-alignment] fulfillment-service/internal/cmd/service/root_cmd.go — --no-color is registered on the fulfillment-service server binary, though the Jira issue targets the osac CLI specifically. Not harmful but slightly beyond the stated scope.

Previous run (2)

Review

Findings

Medium

• [incomplete-documentation] osac-installer/OSAC-CLI-HOWTO.md:1645 — The 'Global Options' section lists logging and connection flags for the osac CLI but does not include the new --no-color flag. Users consulting this doc will not discover the --no-color flag.
  Remediation: Add --no-color to the Global Options section, e.g.: --no-color Disable colored output (can also be set with NO_COLOR env var)

Low

• [error-handling] fulfillment-service/internal/cmd/cli/help/help_setup.go:82 — When GetBool(noColorFlagName) returns an error, execution continues with noColorFlagValue as false. Current callers all register the flag correctly, but a future caller that omits registration would get no color suppression and only a stderr message.

• [test-adequacy] fulfillment-service/internal/cmd/cli/help/help_setup_test.go:101 — The new --no-color tests use bytes.Buffer as output (not a TTY), so they don't exercise the actual color-suppression path. The test comments honestly acknowledge this limitation.

• [scope-creep] fulfillment-service/internal/cmd/cli/help/help_setup.go:165 — Adding --no-color as a CLI flag goes slightly beyond the strict bug description (ANSI color leak in non-TTY), but the no-color.org convention contemplates both env var and CLI flag mechanisms. Low-risk additive UX.

• [flag-registration-style] fulfillment-service/internal/cmd/cli/root_cmd.go:97 — Other persistent flags use flags.StringVar(...) with struct field binding; the new --no-color uses unbound flags.Bool(...). This is intentional since the flag is consumed at help-render time via GetBool() rather than in PersistentPreRunE.

• [flag-name-constant-placement] fulfillment-service/internal/cmd/cli/help/help_setup.go:165 — NoColorFlag is exported from the help package while other flag name constants are unexported and local. The export is justified by the cross-package sharing requirement (both registration sites and Setup need the name).

Previous run (3)

Review

Findings

Low

• [error-handling] fulfillment-service/internal/cmd/cli/help/help_setup.go:80 — When GetBool returns an error (e.g., the flag was not registered), the code prints an error to stderr but continues with noColorFlagValue = false. This is reasonable degradation given the function's contract states the name must match the registered flag.

• [naming-convention] fulfillment-service/internal/cmd/cli/help/help_setup.go:161 — The exported constants NoColorFlag and NoColorFlagHelp are defined in the help package and imported by three root commands. This is a minor deviation from the prevailing pattern where flag constants are unexported and local to each command file, but it is justified here: the flag must be shared across three binaries and the help package is the natural home since it both defines the flag's behavior and consumes its value.

• [pattern-inconsistency] fulfillment-service/internal/cmd/cli/root_cmd.go:97 — The --no-color flag is registered using flags.Bool() with the returned pointer discarded. The logging package uses the identical pattern (flags.Bool() + later GetBool()), so this follows the established approach for cross-cutting flags not bound to runner struct fields.

• [missing-authorization] No linked GitHub issue. PR references Jira ticket OSAC-3500 (validated by CI via jira/valid-reference label). Authorization is traceable through the Jira ticket even though it cannot be independently verified via GitHub.

Previous run (4)

Review

Findings

Medium

• [pattern-inconsistency] fulfillment-service/internal/cmd/service/root_cmd.go:38 — The --no-color flag in the service root command uses a plain inline string ("Disable colored output") while the same flag in cli/root_cmd.go and osac-dev/root_cmd.go uses a markdown-formatted noColorFlagHelp constant with type hint prefix (_[BOOLEAN]_ - Disable colored output. Can also be set with the NO_COLOR environment variable.). This inconsistency means the fulfillment-service --no-color --help output will render the flag description differently from the other two CLIs.
  Remediation: Add a noColorFlagHelp constant to service/root_cmd.go matching the markdown format used by the other root commands, or extract the constant into the shared help package.

Low

• [error-handling] fulfillment-service/internal/cmd/cli/help/help_setup.go:62 — When GetBool(noColorFlagName) returns an error, the code prints a warning to stderr but continues execution with noColorFlagValue = false (zero value). The flag is silently ignored on error and behavior falls back to NO_COLOR env var alone. In practice the error should never occur since all callers pass a registered flag name, but the control flow on error is non-obvious.

• [test-inadequate] fulfillment-service/internal/cmd/cli/help/help_setup_test.go:99 — The two new --no-color tests do not exercise the color-suppression logic. Because cmd.SetOut(output) uses a bytes.Buffer (not a TTY), isTTY is false and useColor is false regardless of the --no-color flag. The tests verify flag parsing without errors but provide no automated signal that --no-color actually suppresses color. The test comments acknowledge this limitation.

• [duplication] fulfillment-service/internal/cmd/osac-dev/root_cmd.go:152 — The noColorFlagHelp constant is defined identically in cli/root_cmd.go and osac-dev/root_cmd.go, following the existing pattern for cacheFlagHelp. Consider extracting the flag name and help text into the help package (which all three root commands already import) to reduce duplication.

• [code-organization] fulfillment-service/internal/cmd/service/root_cmd.go:82 — The noColorFlag constant is declared as a standalone const rather than inside a grouped const (...) block with a // Names of command line flags: comment, as done in the other root commands.

Previous run (5)

Review

Findings

Medium

• [duplicate constant / drift risk] fulfillment-service/internal/cmd/cli/help/help_setup.go:157 — The constant noColorFlag is defined independently in two packages: help/help_setup.go (line 157) and cli/root_cmd.go (line 240), both set to "no-color". If either value is changed without updating the other, GetBool in help_setup.go will silently return (false, error) because the flag lookup will fail, and the error is discarded on line 79. The --no-color flag would then silently stop working in help output with no indication of failure. Other flag name constants (configFlag, cacheFlag, tenantFlag) are defined only in root_cmd.go.
  Remediation: The help package cannot import the cli package (circular dependency). Either (a) extract the constant to a shared subpackage, (b) pass the flag name into Setup() as a parameter, or (c) keep both but handle the error from GetBool so a mismatch surfaces at runtime.

Low

• [error handling gap] fulfillment-service/internal/cmd/cli/help/help_setup.go:79 — The error returned by GetBool(noColorFlag) is silently discarded. While GetBool returns false on error (safe default — color stays enabled), this masks configuration problems. Related to the constant duplication above — the discarded error is the mechanism by which a constant drift would go undetected.

• [scope creep] fulfillment-service/internal/cmd/cli/help/help_setup.go:87 — The PR delivers three distinct changes: (1) adding a --no-color flag, (2) setting style.Document.Color = nil to override Glamour's default foreground color for all TTY users, and (3) broadening the ANSI regex in tests. Change (2) alters visual appearance for all TTY help output regardless of the --no-color flag. A human reviewer should confirm that the Jira ticket OSAC-3500 intends all three changes.

• [flag registration pattern] fulfillment-service/internal/cmd/cli/root_cmd.go:97 — All existing persistent flags use flags.XxxVar(&runner.args.xxx, ...), binding to a struct field. The new noColorFlag uses flags.Bool() without struct binding. The inconsistency is justified by the different consumption point (help function closure vs. persistentPreRun), but may confuse future maintainers.

• [test setup pattern] fulfillment-service/internal/cmd/cli/help/help_setup_test.go:55 — The --no-color flag is registered in BeforeEach rather than newTestCommand(). All other command shape setup is consolidated in newTestCommand().
  Remediation: Move cmd.PersistentFlags().Bool("no-color", false, "Disable colored output") into newTestCommand().

Previous run (6)

Review

Findings

Medium

• [architectural-coherence] fulfillment-service/internal/cmd/cli/help/help_setup.go:53 — The --no-color persistent flag is registered inside help.Setup(), but all other persistent flags on the root command (--config, --cache, --tenant, logging flags) are registered in root_cmd.go's Root() function. This splits flag registration across two locations, breaking the established pattern where help.Setup() only configures help rendering, not CLI surface area.
  Remediation: Move the flag registration to root_cmd.go alongside the other persistent flags, and have help.Setup() read the flag value when rendering. Alternatively, accept a configuration struct in help.Setup() that conveys whether color is disabled.

Low

• [test-inadequate] fulfillment-service/internal/cmd/cli/help/help_setup_test.go:98 — The two new --no-color tests do not exercise the flag's intended code path. Because cmd.SetOut(output) writes to a *bytes.Buffer (not an *os.File backed by a TTY), isTTY is always false, so the help function always selects NoTTYStyleConfig regardless of whether --no-color is set. These tests verify the flag doesn't break anything, but not that it actually suppresses color on a TTY.

• [test-inadequate] fulfillment-service/internal/cmd/cli/help/help_setup_test.go:107 — The subcommand --no-color test omits the Expect(output.String()).ToNot(BeEmpty()) guard that the root-command --no-color test includes. Without it, empty output would vacuously pass the "no ANSI codes" assertion.

---

Labels: PR fixes CLI help output styling — 'bug' label is the closest match for categorization.

[osac-jira-ai-issue-solver]

In comment, @fullsend-ai-review[bot] said:

🤖 Finished Review · ✅ Success · Started 6:58 AM UTC · Completed 7:13 AM UTC

Commit: 5b0897e · View workflow run →

Addressed in 84e897a.

[osac-jira-ai-issue-solver]

In comment, @fullsend-ai-review[bot] said:

Review

Findings

Medium

• [architectural-coherence] fulfillment-service/internal/cmd/cli/help/help_setup.go:53 — The --no-color persistent flag is registered inside help.Setup(), but all other persistent flags on the root command (--config, --cache, --tenant, logging flags) are registered in root_cmd.go's Root() function. This splits flag registration across two locations, breaking the established pattern where help.Setup() only configures help rendering, not CLI surface area.
  Remediation: Move the flag registration to root_cmd.go alongside the other persistent flags, and have help.Setup() read the flag value when rendering. Alternatively, accept a configuration struct in help.Setup() that conveys whether color is disabled.

Low

• [test-inadequate] fulfillment-service/internal/cmd/cli/help/help_setup_test.go:98 — The two new --no-color tests do not exercise the flag's intended code path. Because cmd.SetOut(output) writes to a *bytes.Buffer (not an *os.File backed by a TTY), isTTY is always false, so the help function always selects NoTTYStyleConfig regardless of whether --no-color is set. These tests verify the flag doesn't break anything, but not that it actually suppresses color on a TTY.

• [test-inadequate] fulfillment-service/internal/cmd/cli/help/help_setup_test.go:107 — The subcommand --no-color test omits the Expect(output.String()).ToNot(BeEmpty()) guard that the root-command --no-color test includes. Without it, empty output would vacuously pass the "no ANSI codes" assertion.

---

Labels: PR fixes CLI help output styling — 'bug' label is the closest match for categorization.

Addressed in 84e897a.

[osac-jira-ai-issue-solver]

In comment, @fullsend-ai-review[bot] said:

See the review comment for full details.

Addressed in 84e897a.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:26 AM UTC · Completed 7:43 AM UTC

Commit: 84e897a · View workflow run →

[osac-jira-ai-issue-solver]

In comment, @fullsend-ai-review[bot] said:

🤖 Finished Review · ✅ Success · Started 7:26 AM UTC · Completed 7:43 AM UTC

Commit: 84e897a · View workflow run →

No action needed — this is a CI status notification with no new findings.

Addressed in d06caea.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:54 AM UTC · Completed 8:11 AM UTC

Commit: d06caea · View workflow run →

[osac-jira-ai-issue-solver]

In comment, @fullsend-ai-review[bot] said:

🤖 Finished Review · ✅ Success · Started 8:30 AM UTC · Completed 8:43 AM UTC

Commit: 44d9742 · View workflow run →

I was unable to produce code changes to address this comment after multiple attempts.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:09 AM UTC · Completed 11:23 AM UTC

Commit: a27a6cd · View workflow run →

[osac-jira-ai-issue-solver]

In comment, @fullsend-ai-review[bot] said:

🤖 Finished Review · ✅ Success · Started 11:09 AM UTC · Completed 11:23 AM UTC

Commit: a27a6cd · View workflow run →

CI status notification — no action needed.

Addressed in 0d6820b.

[osac-jira-ai-issue-solver]

In comment, @fullsend-ai-review[bot] said:

See the review comment for full details.

Reference to review details. Added --no-color to the Global Options section of OSAC-CLI-HOWTO.md to address the [medium] incomplete-documentation finding from the referenced review.

Addressed in 0d6820b.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: osac-jira-ai-issue-solver[bot]
Once this PR has been reviewed and has the lgtm label, please assign omer-vishlitzky for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• fulfillment-service/OWNERS
• osac-installer/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:36 AM UTC · Completed 11:51 AM UTC

Commit: 0d6820b · View workflow run →

[osac-jira-ai-issue-solver]

In comment, @fullsend-ai-review[bot] said:

🤖 Finished Review · ✅ Success · Started 11:36 AM UTC · Completed 11:51 AM UTC

Commit: 0d6820b · View workflow run →

CI status notification — no action needed.

Addressed in de4c44b.

[osac-jira-ai-issue-solver]

In comment, @fullsend-ai-review[bot] said:

Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:

• fulfillment-service/internal/cmd/cli/help/help_setup.go:163: [low] naming-convention

NoColorFlag and NoColorFlagHelp are exported, but existing flag constants (configFlag, cacheFlag, tenantFlag) use unexported camelCase. The export is justified by cross-package usage but deviates from the local convention.

Suggested fix: Rename to noColorFlag and noColorFlagHelp, passing the string value to callers, or document the intentional deviation for cross-package constants.

• fulfillment-service/internal/cmd/cli/help/help_setup.go:163: [low] code-organization

Flag constants are conventionally co-located with the registering command (root_cmd.go), not in the help package. The centralized placement is defensible for a cross-cutting flag but differs from the established pattern.

• fulfillment-service/internal/cmd/cli/help/help_setup.go:39: [low] api-consistency

Setup accepts a noColorFlagName parameter, but NoColorFlag is defined in the same package and every caller passes help.NoColorFlag. The parameterization enables testability but adds indirection for a currently-fixed value.

• fulfillment-service/internal/cmd/cli/help/help_setup_test.go (file-level): Line 68 · [low] test-adequacy

The new --no-color tests verify the flag is accepted without error but do not exercise the TTY color-suppression path (bytes.Buffer is not a TTY). The test comments acknowledge this limitation.

• fulfillment-service/internal/cmd/cli/help/help_setup.go:163: [low] constant-grouping

The two new constants use standalone const declarations rather than a grouped const (...) block, which is the convention for related constants in this codebase.

Addressed two of the five inline findings: (1) constant-grouping — grouped NoColorFlag and NoColorFlagHelp into a const(...) block; (2) api-consistency — removed the noColorFlagName parameter from Setup() since NoColorFlag is defined in the same package and every caller passed help.NoColorFlag. The remaining three findings (naming-convention, code-organization, test-adequacy) are informational — the reviewer acknowledges the current approach is justified.

Addressed in de4c44b.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:00 PM UTC · Completed 12:14 PM UTC

Commit: de4c44b · View workflow run →