KofTwentyTwo/license-tool
2
0 Fork 0
Code Issues 2 Pull Requests 9 Actions Packages Projects Releases 6 Wiki Activity

check fail condition flag is ignored #17

New Issue
Closed
opened 2026-06-04 16:19:21 +00:00 by KofTwentyTwo · 2 comments
KofTwentyTwo commented 2026-06-04 16:19:21 +00:00 (Migrated from github.com)
Copy link
Copy source

Finding

The check command exposes a per-run fail condition override, but the parsed values never affect the policy used to decide the exit code.

Evidence

  • cmd/license-tool/commands.go:73 to cmd/license-tool/commands.go:80 stores fail conditions on auditFlags.
  • cmd/license-tool/commands.go:151 to cmd/license-tool/commands.go:159 binds the flag only on check.
  • cmd/license-tool/commands.go:120 to cmd/license-tool/commands.go:135 resolves config and runs report.Check, but never merges f.failOn into cfg.Policy.FailOn.
  • internal/report/report.go:374 to internal/report/report.go:385 uses the report's Passed value, which was computed from cfg.Policy.FailOn.
  • internal/config/config.go:552 onward has parsing logic for fail condition tokens, but that path is only wired through config files.

Reproduction

  1. Create a repo with a headerless supported source file and no unresolved dependencies.
  2. Run license-tool check <repo> with fail conditions restricted to unresolved dependencies.
  3. Expected: exit 0, because the only finding is missing headers.
  4. Actual: exit 1, because the command still uses default fail conditions.

Why It Matters

CI jobs can appear to enforce one policy while actually enforcing another. This can cause false failures, false passes, and confusing policy rollout behavior.

Suggested Fix

Parse the command-line fail condition list into model.FailCondition values and apply it after config resolution. Add command tests that assert the flag changes check's exit behavior.

## Finding The `check` command exposes a per-run fail condition override, but the parsed values never affect the policy used to decide the exit code. ## Evidence - `cmd/license-tool/commands.go:73` to `cmd/license-tool/commands.go:80` stores fail conditions on `auditFlags`. - `cmd/license-tool/commands.go:151` to `cmd/license-tool/commands.go:159` binds the flag only on `check`. - `cmd/license-tool/commands.go:120` to `cmd/license-tool/commands.go:135` resolves config and runs `report.Check`, but never merges `f.failOn` into `cfg.Policy.FailOn`. - `internal/report/report.go:374` to `internal/report/report.go:385` uses the report's `Passed` value, which was computed from `cfg.Policy.FailOn`. - `internal/config/config.go:552` onward has parsing logic for fail condition tokens, but that path is only wired through config files. ## Reproduction 1. Create a repo with a headerless supported source file and no unresolved dependencies. 2. Run `license-tool check <repo>` with fail conditions restricted to unresolved dependencies. 3. Expected: exit `0`, because the only finding is missing headers. 4. Actual: exit `1`, because the command still uses default fail conditions. ## Why It Matters CI jobs can appear to enforce one policy while actually enforcing another. This can cause false failures, false passes, and confusing policy rollout behavior. ## Suggested Fix Parse the command-line fail condition list into `model.FailCondition` values and apply it after config resolution. Add command tests that assert the flag changes check's exit behavior.
KofTwentyTwo commented 2026-06-04 16:29:52 +00:00 (Migrated from github.com)
Copy link
Copy source

Status: confirmed — check's --fail-on value is parsed into auditFlags.failOn but never merged into cfg.Policy.FailOn, so the gate always uses the config/default conditions. Reproduced end-to-end against a clean build.

Severity: high (label: bug). This silently defeats a CI safety control: a job can declare a narrower or broader gate via --fail-on and get the default policy instead — false passes and false failures with no error. Not critical only because the config-file path (policy.fail_on) works correctly, so a workaround exists.

Verified root cause: cmd/license-tool/commands.go:newCheckCmd (RunE, lines 120-143). cfg comes from config.Resolve at commands.go:122 and is passed straight to report.Check at commands.go:130 with f.failOn never consulted. The value captured at commands.go:158 (bindAuditFlags, the StringArrayVar for --fail-on) is therefore dead. Downstream the chain is correct: report.Audit (internal/report/report.go:179) calls policy.Passed(verdict, cfg.Policy.FailOn), report.Check (internal/report/report.go:382) returns the exit code from r.Passed, and policy.Passed (internal/policy/policy.go:315) honors whatever set it is handed. The defect is purely the missing flag→config merge in the CLI layer.

Reproduction (confirmed): temp git repo, headerless main.go, .license-tool.yaml with license/holder only.

  • check . → exit 1 (default fail_on includes missing-header) ✓
  • check . --fail-on unresolved-dependency → exit 1 (expected 0; flag ignored) ✓ bug
  • equivalent config policy.fail_on: [unresolved-dependency] → exit 0 ✓ (proves only the flag path is broken)

Suggested fix:

  1. Add a FailOn []string field to config.Flags (internal/config/config.go:51) and have config.Resolve apply it as the top precedence layer (above the repo/user config files, mirroring how --license/--holder already flow through applyToFlags). Reuse the existing validation in parseFailConditions (internal/config/config.go:554) — currently unexported, so either export it or route the CLI tokens through Resolve so unknown tokens are still rejected (a typo must not silently weaken the gate). Note parseFailConditions is the helper; the issue body's "config.go:552 onward" points at its doc comment, the parse body starts at 554.
  2. In newCheckCmd (commands.go:120), pass f.failOn into the config.Flags builder. Consider only overriding when the flag was explicitly set (cmd.Flags().Changed("fail-on")) so the bound default doesn't clobber a richer policy.fail_on from config; decide the desired flag-vs-config precedence (see open questions).
  3. Flag type / docs mismatch: the flag uses StringArrayVar (repeatable, no comma split — verified: --fail-on a,b,c yields the single element "a,b,c"), but README.md:67 and docs/license-tool-design.md:25 document the comma-separated form --fail-on missing-header,unknown-license,policy-violation. Switch to StringSliceVar or split on commas before parsing, and align docs, otherwise the documented invocation will produce an "unknown fail_on condition" once wiring lands.
  4. Coverage gate: this is analogous to the v0.2.1 universal-shebang fix in that the existing test only proved the flag was bound (commands_test.go:291 asserts Lookup("fail-on") is non-nil), never that it changed behavior — which is exactly why a dead flag held the 100% gate green. Add a command-level test asserting check --fail-on unresolved-dependency exits 0 on a headerless tree (and a negative case), paralleling the existing config-driven test at commands_test.go:261. This is a CLI-layer + config-layer fix; it does not touch the render/detect preserve-boundary code, so the dual-boundary concern from header bugs does not apply here.

Corrections to the issue: (1) Line ref nit — the parse logic body is internal/config/config.go:554 (parseFailConditions); :552 is its doc comment. (2) The "Suggested Fix" says "parse the command-line fail condition list into model.FailCondition values and apply it after config resolution" — applying after config.Resolve would bypass parseFailConditions's typo rejection; cleaner to thread it through Resolve as a flag layer so validation and precedence stay centralized. (3) The body omits the StringArrayVar-vs-documented-comma-syntax mismatch, which any fix must also resolve. Otherwise the evidence and root-cause localization are accurate.

Open questions: (a) Precedence — should an explicit --fail-on fully replace policy.fail_on from config, or union with it? The bound default [missing-header, unknown-license, policy-violation] means a naive merge would always override config unless gated on Changed("fail-on"). (b) Flag syntax — adopt comma-separated (StringSliceVar) to match docs, keep repeatable, or support both? (c) Should audit gain read-only --fail-on reflected in its (non-exit) output, or stay check-only as today (bindAuditFlags(..., false))?

Overlaps: none found among the cited code paths.

Automated maintainer review.

**Status**: confirmed — `check`'s `--fail-on` value is parsed into `auditFlags.failOn` but never merged into `cfg.Policy.FailOn`, so the gate always uses the config/default conditions. Reproduced end-to-end against a clean build. **Severity**: high (label: bug). This silently defeats a CI safety control: a job can declare a narrower or broader gate via `--fail-on` and get the default policy instead — false passes and false failures with no error. Not critical only because the config-file path (`policy.fail_on`) works correctly, so a workaround exists. **Verified root cause**: `cmd/license-tool/commands.go:newCheckCmd` (RunE, lines 120-143). `cfg` comes from `config.Resolve` at `commands.go:122` and is passed straight to `report.Check` at `commands.go:130` with `f.failOn` never consulted. The value captured at `commands.go:158` (`bindAuditFlags`, the `StringArrayVar` for `--fail-on`) is therefore dead. Downstream the chain is correct: `report.Audit` (`internal/report/report.go:179`) calls `policy.Passed(verdict, cfg.Policy.FailOn)`, `report.Check` (`internal/report/report.go:382`) returns the exit code from `r.Passed`, and `policy.Passed` (`internal/policy/policy.go:315`) honors whatever set it is handed. The defect is purely the missing flag→config merge in the CLI layer. **Reproduction (confirmed)**: temp git repo, headerless `main.go`, `.license-tool.yaml` with `license/holder` only. - `check .` → exit 1 (default `fail_on` includes `missing-header`) ✓ - `check . --fail-on unresolved-dependency` → exit **1** (expected 0; flag ignored) ✓ bug - equivalent config `policy.fail_on: [unresolved-dependency]` → exit **0** ✓ (proves only the flag path is broken) **Suggested fix**: 1. Add a `FailOn []string` field to `config.Flags` (`internal/config/config.go:51`) and have `config.Resolve` apply it as the top precedence layer (above the repo/user config files, mirroring how `--license`/`--holder` already flow through `applyToFlags`). Reuse the existing validation in `parseFailConditions` (`internal/config/config.go:554`) — currently unexported, so either export it or route the CLI tokens through `Resolve` so unknown tokens are still rejected (a typo must not silently weaken the gate). Note `parseFailConditions` is the helper; the issue body's "config.go:552 onward" points at its doc comment, the parse body starts at 554. 2. In `newCheckCmd` (`commands.go:120`), pass `f.failOn` into the `config.Flags` builder. Consider only overriding when the flag was explicitly set (`cmd.Flags().Changed("fail-on")`) so the bound default doesn't clobber a richer `policy.fail_on` from config; decide the desired flag-vs-config precedence (see open questions). 3. Flag type / docs mismatch: the flag uses `StringArrayVar` (repeatable, no comma split — verified: `--fail-on a,b,c` yields the single element `"a,b,c"`), but `README.md:67` and `docs/license-tool-design.md:25` document the comma-separated form `--fail-on missing-header,unknown-license,policy-violation`. Switch to `StringSliceVar` or split on commas before parsing, and align docs, otherwise the documented invocation will produce an "unknown fail_on condition" once wiring lands. 4. Coverage gate: this is analogous to the v0.2.1 universal-shebang fix in that the existing test only proved the flag was *bound* (`commands_test.go:291` asserts `Lookup("fail-on")` is non-nil), never that it changed behavior — which is exactly why a dead flag held the 100% gate green. Add a command-level test asserting `check --fail-on unresolved-dependency` exits 0 on a headerless tree (and a negative case), paralleling the existing config-driven test at `commands_test.go:261`. This is a CLI-layer + config-layer fix; it does not touch the render/detect preserve-boundary code, so the dual-boundary concern from header bugs does not apply here. **Corrections to the issue**: (1) Line ref nit — the parse logic body is `internal/config/config.go:554` (`parseFailConditions`); `:552` is its doc comment. (2) The "Suggested Fix" says "parse the command-line fail condition list into model.FailCondition values and apply it after config resolution" — applying *after* `config.Resolve` would bypass `parseFailConditions`'s typo rejection; cleaner to thread it *through* `Resolve` as a flag layer so validation and precedence stay centralized. (3) The body omits the `StringArrayVar`-vs-documented-comma-syntax mismatch, which any fix must also resolve. Otherwise the evidence and root-cause localization are accurate. **Open questions**: (a) Precedence — should an explicit `--fail-on` fully replace `policy.fail_on` from config, or union with it? The bound default `[missing-header, unknown-license, policy-violation]` means a naive merge would always override config unless gated on `Changed("fail-on")`. (b) Flag syntax — adopt comma-separated (`StringSliceVar`) to match docs, keep repeatable, or support both? (c) Should `audit` gain read-only `--fail-on` reflected in its (non-exit) output, or stay check-only as today (`bindAuditFlags(..., false)`)? **Overlaps**: none found among the cited code paths. _Automated maintainer review._
KofTwentyTwo commented 2026-06-04 20:28:13 +00:00 (Migrated from github.com)
Copy link
Copy source

Resolved on develop in commit 742c124e6ec2afa9312075db3112bc2def3c91ff.

Validation:

Closing as fixed.

Resolved on `develop` in commit `742c124e6ec2afa9312075db3112bc2def3c91ff`. Validation: - CI: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977377097 - CodeQL: success, https://github.com/KofTwentyTwo/license-tool/actions/runs/26977376421 - Coverage gate: 100% (2087/2087) Closing as fixed.
Sign in to join this conversation.
No Branch/Tag Specified
Labels
Clear labels
bug
Something isn't working
 dependencies
Pull requests that update a dependency file
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 github_actions
Pull requests that update GitHub Actions code
 go
Pull requests that update go code
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No labels bug
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
forge-recovery (Forge Recovery) james.maes (James Maes)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: KofTwentyTwo/license-tool#17