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

CLI errors are silent and documented exit codes are collapsed #16

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

Finding

Returned command errors are not printed, and most documented exit code categories are collapsed into usage exit code 2.

Evidence

  • cmd/license-tool/commands.go:42 to cmd/license-tool/commands.go:43 sets SilenceUsage and SilenceErrors on the root command.
  • cmd/license-tool/main.go:18 to cmd/license-tool/main.go:26 exits 2 for any returned error and does not print the error.
  • README.md:113 to README.md:121 documents distinct exit codes for policy failure, usage error, write refusal, and internal error.
  • docs/license-tool-design.md:29 documents the same contract.
  • internal/report/report.go:377 to internal/report/report.go:385 can distinguish check internal errors, but the CLI does not surface that distinction.

Reproduction

  1. Build the binary.
  2. Run an invalid format value for audit.
  3. Observe exit code 2 with empty stdout and stderr.
  4. Run a write-enabled apply in a non-git directory without force.
  5. Observe exit code 2 instead of the documented write-refusal code, with no diagnostic.

Why It Matters

Users and CI cannot diagnose failures from stderr, and automation cannot distinguish usage mistakes from unsafe write refusals or internal failures.

Suggested Fix

Introduce a command error type or command execution result that carries the intended exit code. Print returned errors to stderr when Cobra is silenced, and map write refusals and internal failures to the documented codes.

## Finding Returned command errors are not printed, and most documented exit code categories are collapsed into usage exit code `2`. ## Evidence - `cmd/license-tool/commands.go:42` to `cmd/license-tool/commands.go:43` sets `SilenceUsage` and `SilenceErrors` on the root command. - `cmd/license-tool/main.go:18` to `cmd/license-tool/main.go:26` exits `2` for any returned error and does not print the error. - `README.md:113` to `README.md:121` documents distinct exit codes for policy failure, usage error, write refusal, and internal error. - `docs/license-tool-design.md:29` documents the same contract. - `internal/report/report.go:377` to `internal/report/report.go:385` can distinguish check internal errors, but the CLI does not surface that distinction. ## Reproduction 1. Build the binary. 2. Run an invalid format value for `audit`. 3. Observe exit code `2` with empty stdout and stderr. 4. Run a write-enabled apply in a non-git directory without force. 5. Observe exit code `2` instead of the documented write-refusal code, with no diagnostic. ## Why It Matters Users and CI cannot diagnose failures from stderr, and automation cannot distinguish usage mistakes from unsafe write refusals or internal failures. ## Suggested Fix Introduce a command error type or command execution result that carries the intended exit code. Print returned errors to stderr when Cobra is silenced, and map write refusals and internal failures to the documented codes.
KofTwentyTwo commented 2026-06-04 16:29:06 +00:00 (Migrated from github.com)
Copy link
Copy source

Status: confirmed — both halves reproduce. With SilenceErrors: true set on the root command, returned errors are never printed, and main.go collapses every returned error to exit 2, swallowing the documented 1/3/4 codes on the error path.

Severity: high (label is bug/unspecified). It is not critical (no data loss / no incorrect writes), but it defeats the tool's CI purpose: automation cannot distinguish a usage mistake from an unsafe write refusal or an internal failure, and operators get zero diagnostic text. High is justified for a CI-gating tool whose entire value is machine-readable failure signaling.

Verified root cause:

  • cmd/license-tool/commands.go:42-43 (newRootCmd) — SilenceUsage: true and SilenceErrors: true. The SilenceErrors flag is the silence root cause; cobra would otherwise print returned errors itself.
  • cmd/license-tool/main.go:18-26 (main) — if err := root.Execute(); err != nil { os.Exit(2) }. Blanket exit 2, no print. The inline comment here ("Cobra has already printed the error") is factually wrong given SilenceErrors: true.
  • Exit-code collapse on the error path is structural, not just main:
    • apply write refusal originates at internal/applier/applier.go:298 (gateWrite: errors.New("applier: refusing to write in a non-git directory without --force")). newApplyCmd RunE (commands.go:198-200) just return err, so it lands on main.go's exit 2 instead of the documented 3.
    • check internal error: report.Check returns (4, err) (internal/report/report.go:382), but newCheckCmd RunE (commands.go:136-137) does if err != nil { return err }, discarding the 4. The os.Exit(code) at commands.go:140 only runs on the err == nil branch, so the 4 never reaches the process.

Reproduction (built at HEAD, temp dir):

  1. audit --format bogus . → exit 2, empty stdout, empty stderr.
  2. write-enabled apply --write in a non-git dir without --force → exit 2 (documented: 3), empty stdout/stderr — error text from applier.go:298 is swallowed.
  3. genuine cobra usage error (audit --nope .) → exit 2, also silent (this is the SilenceErrors over-reach: it suppresses cobra's own usage/flag errors too, not just RunE-returned errors).

Suggested fix: Mirror the v0.2.1 universal-shebang precedent of fixing the behavior at every relevant site rather than one. Define a small typed command error (e.g. type exitError struct { code int; err error }) and have RunE bodies return it:

  • apply/license: wrap the gateWrite refusal as code 3. Cleanest is for gateWrite/applier.Apply to return a typed refusal that the RunE maps to 3.
  • check: stop discarding the returned code — return exitError{4, err} when report.Check returns a non-zero code with an error, and keep the existing os.Exit(code) for the 1 case (or fold that into the typed error too).
  • usage/validation errors (ParseFormat, bad flags, config.Resolve) → code 2.
  • internal/unexpected → code 4.
    Then in main.go: keep SilenceUsage but either drop SilenceErrors or print the error to stderr explicitly before exiting, and read the code off the typed error (errors.As) defaulting to 2. Because main.go is the only covered-excluded CLI file (.testcoverage.yml:45), put the code-mapping logic in commands.go (which is gated at 100%) and keep main.go a thin errors.As + os.Exit shim — otherwise the new branches in main.go are untestable and the gate notes for main.go stay honest. New commands.go branches (refusal→3, internal→4, usage→2) must each get a unit test to hold the 100% total.

Corrections to the issue:

  • The body cites internal/report/report.go:377-385 as where check "can distinguish internal errors." The return 4, err is at line 382 (the Check func spans ~376-388); the cited range is close but the load-bearing line is 382. Minor.
  • The body's framing ("most documented exit code categories are collapsed into 2") is accurate but slightly understates one detail worth recording: SilenceErrors: true also silences cobra's own usage/flag errors (repro #3), so the silence is broader than RunE-returned errors. Not an over-claim, an under-claim.
  • Otherwise the file:line references (commands.go:42-43, main.go:18-26, README 113-121, design :29) are accurate.

Open questions:

  • For check policy failure today the process exits 1 via os.Exit(code) with no stderr text — should a policy failure also print a one-line summary to stderr, or stay silent and rely on the rendered report on stdout? Affects whether 1 flows through the new typed-error path or keeps its direct os.Exit.
  • Should --quiet suppress the new stderr error line, or is an error always printed regardless of --quiet? (Recommend: errors always print.)
  • Confirm the intended code for config.Resolve failures (missing license/holder in CI): usage 2 or internal 4? README lumps "missing required fields … hard-error in CI" without assigning a code.

Overlaps: none found from this issue's body. A maintainer should confirm no separate open issue already tracks the apply write-refusal exit code specifically, since the fix here must touch that path too.

Automated maintainer review.

**Status**: confirmed — both halves reproduce. With `SilenceErrors: true` set on the root command, returned errors are never printed, and `main.go` collapses every returned error to exit `2`, swallowing the documented `1/3/4` codes on the error path. **Severity**: high (label is `bug`/unspecified). It is not critical (no data loss / no incorrect writes), but it defeats the tool's CI purpose: automation cannot distinguish a usage mistake from an unsafe write refusal or an internal failure, and operators get zero diagnostic text. High is justified for a CI-gating tool whose entire value is machine-readable failure signaling. **Verified root cause**: - `cmd/license-tool/commands.go:42-43` (`newRootCmd`) — `SilenceUsage: true` **and** `SilenceErrors: true`. The `SilenceErrors` flag is the silence root cause; cobra would otherwise print returned errors itself. - `cmd/license-tool/main.go:18-26` (`main`) — `if err := root.Execute(); err != nil { os.Exit(2) }`. Blanket exit `2`, no print. The inline comment here ("Cobra has already printed the error") is **factually wrong** given `SilenceErrors: true`. - Exit-code collapse on the error path is structural, not just `main`: - `apply` write refusal originates at `internal/applier/applier.go:298` (`gateWrite`: `errors.New("applier: refusing to write in a non-git directory without --force")`). `newApplyCmd` RunE (`commands.go:198-200`) just `return err`, so it lands on `main.go`'s exit `2` instead of the documented `3`. - `check` internal error: `report.Check` returns `(4, err)` (`internal/report/report.go:382`), but `newCheckCmd` RunE (`commands.go:136-137`) does `if err != nil { return err }`, discarding the `4`. The `os.Exit(code)` at `commands.go:140` only runs on the `err == nil` branch, so the `4` never reaches the process. **Reproduction** (built at HEAD, temp dir): 1. `audit --format bogus .` → exit `2`, empty stdout, empty stderr. 2. write-enabled `apply --write` in a non-git dir without `--force` → exit `2` (documented: `3`), empty stdout/stderr — error text from `applier.go:298` is swallowed. 3. genuine cobra usage error (`audit --nope .`) → exit `2`, **also silent** (this is the `SilenceErrors` over-reach: it suppresses cobra's own usage/flag errors too, not just RunE-returned errors). **Suggested fix**: Mirror the v0.2.1 universal-shebang precedent of fixing the behavior at every relevant site rather than one. Define a small typed command error (e.g. `type exitError struct { code int; err error }`) and have RunE bodies return it: - `apply`/`license`: wrap the `gateWrite` refusal as code `3`. Cleanest is for `gateWrite`/`applier.Apply` to return a typed refusal that the RunE maps to `3`. - `check`: stop discarding the returned code — return `exitError{4, err}` when `report.Check` returns a non-zero code with an error, and keep the existing `os.Exit(code)` for the `1` case (or fold that into the typed error too). - usage/validation errors (`ParseFormat`, bad flags, `config.Resolve`) → code `2`. - internal/unexpected → code `4`. Then in `main.go`: keep `SilenceUsage` but **either drop `SilenceErrors` or print the error to stderr explicitly** before exiting, and read the code off the typed error (`errors.As`) defaulting to `2`. Because `main.go` is the *only* covered-excluded CLI file (`.testcoverage.yml:45`), put the code-mapping logic in `commands.go` (which is gated at 100%) and keep `main.go` a thin `errors.As` + `os.Exit` shim — otherwise the new branches in `main.go` are untestable and the gate notes for `main.go` stay honest. New `commands.go` branches (refusal→3, internal→4, usage→2) must each get a unit test to hold the 100% total. **Corrections to the issue**: - The body cites `internal/report/report.go:377-385` as where check "can distinguish internal errors." The `return 4, err` is at **line 382** (the `Check` func spans ~376-388); the cited range is close but the load-bearing line is 382. Minor. - The body's framing ("most documented exit code categories are collapsed into `2`") is accurate but slightly understates one detail worth recording: `SilenceErrors: true` also silences cobra's *own* usage/flag errors (repro #3), so the silence is broader than RunE-returned errors. Not an over-claim, an under-claim. - Otherwise the file:line references (`commands.go:42-43`, `main.go:18-26`, README `113-121`, design `:29`) are accurate. **Open questions**: - For `check` policy failure today the process exits `1` via `os.Exit(code)` *with no stderr text* — should a policy failure also print a one-line summary to stderr, or stay silent and rely on the rendered report on stdout? Affects whether `1` flows through the new typed-error path or keeps its direct `os.Exit`. - Should `--quiet` suppress the new stderr error line, or is an error always printed regardless of `--quiet`? (Recommend: errors always print.) - Confirm the intended code for `config.Resolve` failures (missing license/holder in CI): usage `2` or internal `4`? README lumps "missing required fields … hard-error in CI" without assigning a code. **Overlaps**: none found from this issue's body. A maintainer should confirm no separate open issue already tracks the `apply` write-refusal exit code specifically, since the fix here must touch that path too. _Automated maintainer review._
KofTwentyTwo commented 2026-06-04 20:28:12 +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#16