[PPSC-402] feat(cli): improve CLI usability with color support and update checks #69
+1,514
−266
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…date checks - Add color output management with TTY detection and NO_COLOR support - Implement version update checking with GitHub releases API and local caching - Update root command with --color flag and version check integration - Enhance scan commands with better progress handling and user feedback - Add comprehensive tests for new features
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
🛡️ Armis Security Scan Results🟠 HIGH issues found
Total: 5 View all 5 findings🟠 HIGH (4)
|
Test Coverage Reporttotal: (statements) 79.9% Coverage by function |
There was a problem hiding this comment.
Pull request overview
This PR improves armis-cli usability by adding centralized color handling for stderr output and introducing a background version update check against GitHub releases, plus spinner/status messaging improvements during scans.
Changes:
- Add
internal/clicolor utilities (--colorflag, NO_COLOR/TTY detection) and migrate warnings/errors to use them. - Add
internal/updatewith async GitHub “latest release” checking + on-disk caching, and print update notifications after scan commands. - Enhance scan progress UX by updating spinner messages based on ingest status callbacks.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
internal/update/update.go |
Implements GitHub release polling, TTL cache, and notification formatting. |
internal/update/update_test.go |
Adds unit tests for version parsing/comparison, caching, network/error paths, and async checks. |
internal/cli/color.go |
Adds centralized color enablement logic + PrintWarning/Error helpers. |
internal/cli/color_test.go |
Tests color mode precedence and colored/plain stderr output. |
internal/cmd/root.go |
Adds --color/--no-update-check, initializes color state, and starts update checks in the background. |
internal/cmd/scan_repo.go |
Adds command examples and prints update notification before exiting. |
internal/cmd/scan_image.go |
Adds command examples and prints update notification before exiting. |
internal/cmd/context.go |
Routes cancellation messaging through the new warning printer. |
internal/api/client.go |
Extends WaitForIngest with an optional per-poll status callback. |
internal/api/client_test.go |
Updates WaitForIngest call sites and adds callback coverage. |
internal/scan/repo/repo.go |
Uses cli warnings and updates spinner text via ingest status callback. |
internal/scan/repo/repo_test.go |
Adds tests for new repo scan status formatting helper. |
internal/scan/image/image.go |
Uses cli warnings and updates spinner text via ingest status callback. |
internal/scan/image/helpers_test.go |
Adds tests for new image scan status formatting helper. |
internal/scan/sbom_vex.go |
Migrates SBOM/VEX warning output to cli printers. |
internal/output/human.go |
Adds SyncColors() to align formatter colors with centralized CLI color state. |
internal/output/output.go |
Clarifies stderr writer usage for testability during stdout flush failures. |
internal/output/output_test.go |
Adds tests to verify output.SyncColors() behavior. |
internal/output/sarif.go |
Routes SARIF sanitization warnings through cli printers. |
cmd/armis-cli/main.go |
Initializes colors early and prints top-level errors via cli.PrintError. |
go.mod |
Adds golang.org/x/term as a direct dependency for TTY detection. |
.goreleaser.yaml |
Generates shell completions during release builds and includes them in archives. |
.gitignore |
Ignores dist/ and generated completions/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/update/update.go
Outdated
| } | ||
|
|
||
| // readCache attempts to read a cached check result. | ||
| // Returns nil if cache is missing, corrupt, or expired. |
There was a problem hiding this comment.
The comment says readCache returns nil if the cache is "expired", but this function doesn’t check TTL/expiry (expiry is handled in Checker.check). Please update the comment to avoid misleading future readers (e.g., say missing/corrupt only).
Suggested change
| // Returns nil if cache is missing, corrupt, or expired. | |
| // Returns nil if cache is missing or corrupt. |
Comment on lines
+118
to
+122
| // Write to cache (best-effort) | ||
| c.writeCache(&cacheFile{ | ||
| LatestVersion: latest, | ||
| CheckedAt: time.Now(), | ||
| }) |
There was a problem hiding this comment.
Checker.check writes to the cache even when fetchLatestVersion returns an empty tag. Caching an empty LatestVersion can suppress future update checks for the full TTL; consider treating empty tag as an error (skip caching and return nil) so the next run can retry.
Comment on lines
67
to
75
| // Skip update check if: | ||
| // - explicitly disabled via flag or env var | ||
| // - running in CI | ||
| // - version is "dev" (development build) | ||
| // - running meta-commands | ||
| if noUpdateCheck || os.Getenv("ARMIS_NO_UPDATE_CHECK") != "" || | ||
| progress.IsCI() || version == "dev" || | ||
| cmd.Name() == "help" || cmd.Name() == "__complete" { | ||
| return nil |
There was a problem hiding this comment.
Update checks are only skipped for help and __complete, but Cobra’s built-in "completion" command (used by the goreleaser hooks) will still start a GitHub update check. Consider also skipping update checks for cmd.Name()=="completion" (and possibly other meta commands like "version") to avoid unnecessary network calls/rate limits during completion generation.
| var buf bytes.Buffer | ||
| if _, err := buf.ReadFrom(r); err != nil { | ||
| t.Errorf("failed to read from pipe: %v", err) | ||
| } |
There was a problem hiding this comment.
captureStderr closes the write end of the pipe but never closes the read end (r). Please close r as well to avoid leaking file descriptors across the test suite.
Suggested change
| } | |
| } | |
| if err := r.Close(); err != nil { | |
| t.Errorf("failed to close pipe reader: %v", err) | |
| } |
internal/update/update_test.go
Outdated
Comment on lines
119
to
130
| func contains(s, substr string) bool { | ||
| return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsHelper(s, substr)) | ||
| } | ||
|
|
||
| func containsHelper(s, substr string) bool { | ||
| for i := 0; i <= len(s)-len(substr); i++ { | ||
| if s[i:i+len(substr)] == substr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
This test file reimplements substring search via contains/containsHelper. Using strings.Contains would simplify the test and remove custom logic that’s easy to get subtly wrong (especially around edge cases like empty substrings).
internal/scan/repo/repo.go
Outdated
Comment on lines
818
to
825
| // formatScanStatus returns a human-readable message for the current scan phase. | ||
| // Status values from ArtifactScanStatus enum in Project-Moose API. | ||
| func formatScanStatus(scanStatus string) string { | ||
| switch strings.ToUpper(scanStatus) { | ||
| case "INITIATED": | ||
| return "Scan initiated, preparing analysis..." | ||
| case "IN_PROGRESS": | ||
| return "Analyzing code for vulnerabilities..." |
There was a problem hiding this comment.
formatScanStatus is duplicated (repo and image scanners) with only minor message differences. Consider extracting a shared helper (e.g., internal/scan/status.go) to avoid the two copies drifting as new statuses are added.
internal/scan/image/image.go
Outdated
Comment on lines
480
to
493
| switch strings.ToUpper(scanStatus) { | ||
| case "INITIATED": | ||
| return "Scan initiated, preparing analysis..." | ||
| case "IN_PROGRESS": | ||
| return "Scanning image for vulnerabilities..." | ||
| case "COMPLETED": | ||
| return "Scan completed, preparing results..." | ||
| case "FAILED": | ||
| return "Scan encountered an error" | ||
| case "STOPPED": | ||
| return "Scan was stopped" | ||
| default: | ||
| return fmt.Sprintf("Scanning... [%s]", strings.ToUpper(scanStatus)) | ||
| } |
There was a problem hiding this comment.
formatScanStatus is duplicated (repo and image scanners) with only minor message differences. Consider extracting a shared helper (e.g., internal/scan/status.go) to avoid the two copies drifting as new statuses are added.
Suggested change
| switch strings.ToUpper(scanStatus) { | |
| case "INITIATED": | |
| return "Scan initiated, preparing analysis..." | |
| case "IN_PROGRESS": | |
| return "Scanning image for vulnerabilities..." | |
| case "COMPLETED": | |
| return "Scan completed, preparing results..." | |
| case "FAILED": | |
| return "Scan encountered an error" | |
| case "STOPPED": | |
| return "Scan was stopped" | |
| default: | |
| return fmt.Sprintf("Scanning... [%s]", strings.ToUpper(scanStatus)) | |
| } | |
| statusKey := strings.ToUpper(scanStatus) | |
| statusMessages := map[string]string{ | |
| "INITIATED": "Scan initiated, preparing analysis...", | |
| "IN_PROGRESS": "Scanning image for vulnerabilities...", | |
| "COMPLETED": "Scan completed, preparing results...", | |
| "FAILED": "Scan encountered an error", | |
| "STOPPED": "Scan was stopped", | |
| } | |
| if msg, ok := statusMessages[statusKey]; ok { | |
| return msg | |
| } | |
| return fmt.Sprintf("Scanning... [%s]", statusKey) |
Security fixes: - CWE-73: Add path validation in update.go using util.SanitizePath() - CWE-770: Add io.LimitReader to all io.ReadAll calls in api/client.go Code quality improvements: - Skip update checks for completion commands and subcommands - Don't cache empty version tags from GitHub API - Fix misleading readCache comment (removed "expired" mention) Test fixes: - Close pipe reader in color_test.go to prevent FD leak - Replace custom contains() helper with strings.Contains in update_test.go Refactoring: - Extract shared scan helpers (FormatScanStatus, FormatElapsed, MapSeverity) to internal/scan/status.go to eliminate code duplication between repo and image scanners
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
84
to
86
| with: | ||
| version: v2.7.2 | ||
| version: latest | ||
| args: --timeout=5m |
There was a problem hiding this comment.
Using golangci-lint version latest makes CI non-deterministic and can break builds unexpectedly when new linter releases add checks or change defaults. Please pin this to a specific known-good golangci-lint version (and update it intentionally when desired).
CWE-770: Replace json.NewDecoder with io.LimitReader + json.Unmarshal - StartIngest: limit IngestUploadResponse decoding - GetIngestStatus: limit IngestStatusResponse decoding - GetScanResult: limit ScanResult decoding CWE-73: Add inline path validation in update.go - readCache: validate path with util.SanitizePath before os.ReadFile - writeCache: validate path with util.SanitizePath before os.WriteFile CI: Pin golangci-lint to v2.1.6 for reproducible builds
| _ = ctx // unused but kept for API consistency | ||
| if errors.Is(err, context.Canceled) { | ||
| fmt.Fprintln(os.Stderr, "\nScan cancelled") | ||
| fmt.Fprintln(os.Stderr, "") // newline before warning |
Check warning
Code scanning / Armis Security Scanner
The return value (error) from fmt.Fprintln is ignored, so failures when writi... Medium
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Issue
Type of Change
Problem
The armis-cli needed improved usability features to better handle output formatting and keep users informed about available updates.
Solution
This PR adds two major features to improve CLI usability:
Color Output Management (
internal/cli/color.go)Version Update Checking (
internal/update/update.go)Additional improvements:
Testing
Automated Tests
Manual Testing
Verified by running:
make test- All tests passmake lint- No linting errorsarmis-cli --help,armis-cli --color=never, etc.Reviewer Notes
golang.org/x/termfor cross-platform TTY detectionChecklist