Skip to content

fix(configuration): coerce typed flag values via spf13/cast#281

Open
matuszeg wants to merge 2 commits intogo-gremlins:mainfrom
matuszeg:fix/threshold-flag-cli-issue-216
Open

fix(configuration): coerce typed flag values via spf13/cast#281
matuszeg wants to merge 2 commits intogo-gremlins:mainfrom
matuszeg:fix/threshold-flag-cli-issue-216

Conversation

@matuszeg
Copy link
Copy Markdown

@matuszeg matuszeg commented May 1, 2026

Proposed changes

Closes #216.

--threshold-efficacy and --threshold-mcover never produced a non-zero exit code, even when the actual efficacy/coverage was below the threshold. The same thresholds set via .gremlins.yaml worked correctly, so the bug only affected the CLI-flag path.

Root cause

configuration.Get[T] used an unchecked type assertion against viper.Get:

r, _ = viper.Get(k).(T)

When a value is bound into Viper through viper.BindPFlag, pflag-resolved float64 values surface as their string representation (via pflag.Value.String()), so the assertion to float64 silently failed and returned the zero value. The threshold check in internal/report/report.go then saw et == 0 and skipped enforcement.

Config-file values bypassed this because Viper stores YAML scalars as native Go types, so the assertion succeeded.

Fix

Coerce float64, int, bool, and string through spf13/cast (already an indirect dependency), and fall back to the assertion for slices and other types. This normalizes both pflag-sourced and config-sourced values to the native Go type before they leave Get[T].

Tests

Added three layered regression tests so the bug stays fixed regardless of where a future refactor lands:

  • internal/configuration: TestGetFromBoundPFlagGet[T] over a bound pflag in isolation.
  • cmd/internal/flags: TestSetBindsTypedValueFromCLI — full flags.Set → cobra parse → configuration.Get[T].
  • cmd: TestUnleashFlagsPropagateToConfiguration — exercises the real unleash command surface.

All three fail without the fix and pass with it.

End-to-end verified by rebuilding the binary and running gremlins unleash --threshold-efficacy 50 against a small module with 0% efficacy: exit code is now 10 (was 0).

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes (make all)
  • I have added tests that prove my fix is effective or that my feature works
  • [x ] I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

spf13/cast is promoted from indirect to direct in go.mod; it was already pulled in transitively through viper, so no new dependency is added to the module graph.

The original feature request in #216 also asked for --fail-lived / --fail-not-covered flags. That part is intentionally out of scope for this PR — this change only fixes the existing threshold flags so they actually work as documented. Per-status fail flags can be a follow-up.

…ins#216)

Get[T] used an unchecked type assertion against viper.Get, which
silently returned the zero value when the bound value originated from a
pflag (pflag surfaces float64 values through Value.String()). The
threshold-efficacy and threshold-mcover CLI flags therefore never
triggered the threshold exit code, even though the same thresholds
worked from a config file.

Coerce numeric and string types through spf13/cast so flag-sourced and
config-sourced values both resolve to the native Go type.
@pull-request-size pull-request-size Bot added the s/L Size: Denotes a Pull Request that changes 100-499 lines label May 1, 2026
Satisfies the CRT-P0001 'can combine chain of 2 appends into one' lint
on the PR's external code-review bot. Pre-existing pattern in the file;
collapsed for parity with the bot's expectations now that the file is
in the PR diff.
@matuszeg
Copy link
Copy Markdown
Author

matuszeg commented May 1, 2026

After my first commit, I saw that "DeepSource: Go" was failing because of some append optimizations in code I didn't touch in onfiguration_test.go. I went ahead and fixed them to make it green, but if you don't want that as part of this PR to keep the changes isolated I can remove them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s/L Size: Denotes a Pull Request that changes 100-499 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add a flag to have unleash return a nonzero value on failure

1 participant