Wo 4 5/spectre family review

.github/workflows/ci.yml

@@ -70,3 +70,24 @@ jobs:
         with:
           version: latest
           args: --timeout=5m
+
+  windows:
+    name: Windows build + test
+    runs-on: windows-latest
+    steps:
+      - name: Check out code
+        uses: actions/checkout@v4
+
+      - name: Set up Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: '1.26'
+
+      - name: Get dependencies
+        run: go mod download
+
+      - name: Build
+        run: go build ./...
+
+      - name: Test
+        run: go test ./...

CHANGELOG.md

@@ -2,6 +2,29 @@
 
 ## [Unreleased]
 
+### Fixed
+- Windows build+test coverage in CI, plus a README Windows quick-start
+- Config keys (`provider`, `timeout`, `exclude.tags`) that were parsed but never
+  wired to scan behavior now actually take effect
+- Explicit CLI flags now correctly override config file values when the flag
+  is set to its own default (e.g. `--idle-days=14`)
+- Text reporter now sorts findings by severity instead of scan order
+- GCP unused-read-replica finding no longer reports a confirmed monthly-waste
+  figure without a real usage signal
+- `selectReporter`'s output file handle is now closed, fixing a Windows CI
+  failure where an open handle blocked temp-directory cleanup
+
+### Changed
+- Deduplicated exclusion-check and progress-reporting logic shared by the
+  AWS and GCP scanners
+
+## [0.1.1] - 2026-02-28
+
+### Fixed
+- SpectreHub reporter now uses the `spectre/v1` schema
+
+## [0.1.0] - 2026-02-28
+
 ### Added
 - AWS RDS scanner with 11 finding types
 - GCP Cloud SQL scanner with 6 config-based findings

README.md

@@ -37,6 +37,16 @@ cd rdsspectre
 make build
 ```
 
+### Windows
+
+Download the `rdsspectre_<version>_windows_<arch>.zip` archive from the
+[latest release](https://github.com/ppiankov/rdsspectre/releases/latest),
+extract it, and run the binary from PowerShell or Command Prompt:
+
+```powershell
+.\rdsspectre.exe aws --region us-east-1 --format json
+```
+
 ### Usage
 
 ```sh

internal/cloudsql/client.go

@@ -32,6 +32,8 @@ type Instance struct {
 	MasterInstanceName string
 	CreateTime         time.Time
 	SelfLink           string
+	// WO-7: user labels, for tag-based exclusion matching.
+	Labels map[string]string
 }
 
 // Client wraps a Cloud SQL Admin API service.
@@ -91,6 +93,8 @@ func convertInstance(db *sqladmin.DatabaseInstance) Instance {
 		inst.DataDiskSizeGB = db.Settings.DataDiskSizeGb
 		inst.DataDiskType = db.Settings.DataDiskType
 		inst.DeletionProtection = db.Settings.DeletionProtectionEnabled
+		// WO-7: populate labels for tag-based exclusion.
+		inst.Labels = db.Settings.UserLabels
 
 		if db.Settings.BackupConfiguration != nil {
 			inst.BackupEnabled = db.Settings.BackupConfiguration.Enabled

internal/cloudsql/scanner.go

@@ -46,7 +46,12 @@ func (s *CloudSQLScanner) Scan(ctx context.Context, cfg database.ScanConfig, pro
 		if inst.State != "RUNNABLE" {
 			continue
 		}
-		if cfg.Exclude.ResourceIDs[inst.Name] {
+		// WO-9: use shared ExcludeConfig.IsExcluded helper instead of inline map lookup.
+		if cfg.Exclude.IsExcluded(inst.Name) {
+			continue
+		}
+		// WO-7: skip instances matching a configured exclude.tags rule.
+		if len(cfg.Exclude.Tags) > 0 && cfg.Exclude.MatchesExcludedTags(inst.Labels) {
 			continue
 		}
 		result.ResourcesScanned++
@@ -138,20 +143,25 @@ func (s *CloudSQLScanner) analyzeInstance(cfg database.ScanConfig, inst Instance
 		})
 	}
 
-	// UNUSED_READ_REPLICA: config-based detection (no metrics)
+	// WO-11: UNUSED_READ_REPLICA config-based detection only, no connection signal
+	// available (Cloud Monitoring deferred). Unlike the AWS path
+	// (rds/scanner.go), which only fires on a confirmed zero-connection
+	// window, usage here is genuinely unknown, so this reports Low severity
+	// with no claimed EstimatedMonthlyWaste rather than presenting the full
+	// instance cost as confirmed savings.
 	if inst.IsReplica {
 		findings = append(findings, database.Finding{
-			ID:                    database.FindingUnusedReadReplica,
-			Severity:              database.SeverityHigh,
-			ResourceType:          database.ResourceReplica,
-			ResourceID:            inst.Name,
-			Region:                region,
-			Message:               "Read replica detected (connection metrics unavailable without Cloud Monitoring)",
-			EstimatedMonthlyWaste: monthlyCost,
+			ID:           database.FindingUnusedReadReplica,
+			Severity:     database.SeverityLow,
+			ResourceType: database.ResourceReplica,
+			ResourceID:   inst.Name,
+			Region:       region,
+			Message:      "Read replica present; usage unknown (connection metrics unavailable without Cloud Monitoring) — verify before deleting",
 			Metadata: map[string]any{
-				"master_instance":  inst.MasterInstanceName,
-				"database_version": inst.DatabaseVersion,
-				"tier":             inst.Tier,
+				"master_instance":        inst.MasterInstanceName,
+				"database_version":       inst.DatabaseVersion,
+				"tier":                   inst.Tier,
+				"estimated_monthly_cost": monthlyCost,
 			},
 		})
 	}
@@ -170,12 +180,6 @@ func hasPublicAccess(inst Instance) bool {
 }
 
 func (s *CloudSQLScanner) reportProgress(progress func(database.ScanProgress), msg string) {
-	if progress != nil {
-		progress(database.ScanProgress{
-			Region:    s.project,
-			Scanner:   "cloudsql",
-			Message:   msg,
-			Timestamp: time.Now(),
-		})
-	}
+	// WO-9: delegate to the shared database.ReportProgress helper.
+	database.ReportProgress(progress, "cloudsql", s.project, msg)
 }

internal/cloudsql/scanner_test.go

@@ -181,8 +181,61 @@ func TestScanReadReplica(t *testing.T) {
 	if len(hits) != 1 {
 		t.Errorf("expected 1 UNUSED_READ_REPLICA finding, got %d", len(hits))
 	}
-	if hits[0].EstimatedMonthlyWaste <= 0 {
-		t.Error("replica finding should have cost estimate")
+	// WO-11: usage is unconfirmed without Cloud Monitoring data, so this must
+	// not report High severity or claim a definite EstimatedMonthlyWaste.
+	if hits[0].Severity != database.SeverityLow {
+		t.Errorf("replica finding severity = %q, want %q (unconfirmed usage)", hits[0].Severity, database.SeverityLow)
+	}
+	if hits[0].EstimatedMonthlyWaste != 0 {
+		t.Errorf("replica finding should not claim confirmed waste, got %.2f", hits[0].EstimatedMonthlyWaste)
+	}
+	if cost, ok := hits[0].Metadata["estimated_monthly_cost"]; !ok || cost.(float64) <= 0 {
+		t.Error("replica finding should surface an informational estimated_monthly_cost in metadata")
+	}
+}
+
+// WO-7: exercises tag/label-based exclusion.
+func TestScanExcludeByLabel(t *testing.T) {
+	mock := newMockClient()
+	mock.instances = []Instance{
+		makeInstance("tagged-db", "db-f1-micro", "POSTGRES_17", func(i *Instance) {
+			i.Labels = map[string]string{"env": "temporary"}
+			i.DeletionProtection = false // would normally be flagged
+		}),
+	}
+
+	cfg := defaultCfg()
+	cfg.Exclude.Tags = map[string]string{"env": "temporary"}
+
+	s := newTestScanner(mock)
+	result := s.Scan(context.Background(), cfg, nil)
+
+	if len(result.Findings) != 0 {
+		t.Errorf("expected 0 findings for label-excluded instance, got %d", len(result.Findings))
+	}
+	if result.ResourcesScanned != 0 {
+		t.Errorf("ResourcesScanned = %d, want 0 (label-excluded)", result.ResourcesScanned)
+	}
+}
+
+// WO-7: exercises tag/label-based exclusion.
+func TestScanLabelExcludeNoMatch(t *testing.T) {
+	mock := newMockClient()
+	mock.instances = []Instance{
+		makeInstance("keep-db", "db-f1-micro", "POSTGRES_17", func(i *Instance) {
+			i.Labels = map[string]string{"env": "production"}
+			i.DeletionProtection = false
+		}),
+	}
+
+	cfg := defaultCfg()
+	cfg.Exclude.Tags = map[string]string{"env": "temporary"}
+
+	s := newTestScanner(mock)
+	result := s.Scan(context.Background(), cfg, nil)
+
+	if len(findByID(result.Findings, database.FindingNoDeletionProtect)) != 1 {
+		t.Error("expected instance to still be flagged (label does not match exclusion)")
 	}
 }
 

internal/commands/aws.go

@@ -3,6 +3,7 @@ package commands
 import (
 	"context"
 	"fmt"
+	"io"
 	"log/slog"
 	"os"
 	"strings"
@@ -57,20 +58,25 @@ func init() {
 }
 
 func runAWS(cmd *cobra.Command, _ []string) error {
+	// WO-7: load config and apply defaults before building the timeout
+	// context, so a config-file timeout can fall back into effect.
+	cfg, err := config.Load(".")
+	if err != nil {
+		slog.Warn("Failed to load config file", "error", err)
+	}
+	// WO-7: reject a config provider that doesn't match the invoked subcommand.
+	if cfg.Provider != "" && cfg.Provider != "aws" {
+		return fmt.Errorf("config provider %q does not match the invoked \"aws\" subcommand", cfg.Provider)
+	}
+	applyAWSConfigDefaults(cmd, cfg)
+
 	ctx := cmd.Context()
 	if awsFlags.timeout > 0 {
 		var cancel context.CancelFunc
 		ctx, cancel = context.WithTimeout(ctx, awsFlags.timeout)
 		defer cancel()
 	}
 
-	// Load config and apply defaults
-	cfg, err := config.Load(".")
-	if err != nil {
-		slog.Warn("Failed to load config file", "error", err)
-	}
-	applyAWSConfigDefaults(cfg)
-
 	// Resolve profile and region
 	profile := awsFlags.profile
 	if profile == "" {
@@ -94,10 +100,8 @@ func runAWS(cmd *cobra.Command, _ []string) error {
 	slog.Info("Scanning RDS", "region", resolvedRegion)
 
 	// Build scan config
-	excludeIDs := make(map[string]bool, len(cfg.Exclude.ResourceIDs))
-	for _, id := range cfg.Exclude.ResourceIDs {
-		excludeIDs[id] = true
-	}
+	// WO-9: shared helper instead of an inline map-building loop.
+	excludeIDs := buildExcludeIDs(cfg.Exclude.ResourceIDs)
 	excludeTags := parseExcludeTags(cfg.Exclude.Tags, awsFlags.excludeTags)
 
 	scanCfg := database.ScanConfig{
@@ -153,55 +157,86 @@ func runAWS(cmd *cobra.Command, _ []string) error {
 	}
 
 	// Select and run reporter
-	reporter, err := selectReporter(awsFlags.format, awsFlags.outputFile)
+	// WO-12: close the output file after Generate so temp-dir cleanup
+	// (and any later read of the file) doesn't race an open handle,
+	// which is fatal on Windows.
+	reporter, closer, err := selectReporter(awsFlags.format, awsFlags.outputFile)
 	if err != nil {
 		return err
 	}
+	if closer != nil {
+		defer func() {
+			if cerr := closer.Close(); cerr != nil {
+				slog.Warn("Failed to close output file", "error", cerr)
+			}
+		}()
+	}
 	return reporter.Generate(data)
 }
 
-func applyAWSConfigDefaults(cfg config.Config) {
-	if awsFlags.format == "text" && cfg.Format != "" {
+// applyAWSConfigDefaults fills unset flags from the config file.
+// An explicit CLI flag always wins over config, even when its value
+// equals the flag's built-in default; only cmd.Flags().Changed() can tell
+// "explicitly set to the default" apart from "never set".
+func applyAWSConfigDefaults(cmd *cobra.Command, cfg config.Config) {
+	// WO-8: cmd.Flags() drives the Changed()-based precedence checks below.
+	flags := cmd.Flags()
+	// WO-8: cmd.Flags().Changed() replaces the old flag==default sentinel
+	// for every check below, so an explicit flag always wins over config.
+	if !flags.Changed("format") && cfg.Format != "" {
 		awsFlags.format = cfg.Format
 	}
-	if awsFlags.idleDays == 14 && cfg.IdleDays > 0 {
+	// WO-8: see above.
+	if !flags.Changed("idle-days") && cfg.IdleDays > 0 {
 		awsFlags.idleDays = cfg.IdleDays
 	}
-	if awsFlags.staleDays == 90 && cfg.StaleDays > 0 {
+	// WO-8: see above.
+	if !flags.Changed("stale-days") && cfg.StaleDays > 0 {
 		awsFlags.staleDays = cfg.StaleDays
 	}
-	if awsFlags.cpuThreshold == 20.0 && cfg.CPUThreshold > 0 {
+	// WO-8: see above.
+	if !flags.Changed("cpu-threshold") && cfg.CPUThreshold > 0 {
 		awsFlags.cpuThreshold = cfg.CPUThreshold
 	}
-	if awsFlags.metricDays == 14 && cfg.MetricDays > 0 {
+	// WO-8: see above.
+	if !flags.Changed("metric-days") && cfg.MetricDays > 0 {
 		awsFlags.metricDays = cfg.MetricDays
 	}
-	if awsFlags.minMonthlyCost == 0.10 && cfg.MinMonthlyCost > 0 {
+	// WO-8: see above.
+	if !flags.Changed("min-monthly-cost") && cfg.MinMonthlyCost > 0 {
 		awsFlags.minMonthlyCost = cfg.MinMonthlyCost
 	}
+	// WO-7: config-file timeout falls back into effect only if --timeout wasn't explicit.
+	if !flags.Changed("timeout") && cfg.TimeoutDuration() > 0 {
+		awsFlags.timeout = cfg.TimeoutDuration()
+	}
 }
 
-func selectReporter(format, outputFile string) (report.Reporter, error) {
-	w := os.Stdout
+// WO-12: also returns an io.Closer (nil for stdout) so callers can close the
+// output file after Generate instead of leaking the handle until process exit.
+func selectReporter(format, outputFile string) (report.Reporter, io.Closer, error) {
+	var w io.Writer = os.Stdout
+	var closer io.Closer
 	if outputFile != "" {
 		f, err := os.Create(outputFile)
 		if err != nil {
-			return nil, fmt.Errorf("create output file: %w", err)
+			return nil, nil, fmt.Errorf("create output file: %w", err)
 		}
 		w = f
+		closer = f
 	}
 
 	switch format {
 	case "json":
-		return &report.JSONReporter{Writer: w}, nil
+		return &report.JSONReporter{Writer: w}, closer, nil
 	case "text":
-		return &report.TextReporter{Writer: w}, nil
+		return &report.TextReporter{Writer: w}, closer, nil
 	case "sarif":
-		return &report.SARIFReporter{Writer: w}, nil
+		return &report.SARIFReporter{Writer: w}, closer, nil
 	case "spectrehub":
-		return &report.SpectreHubReporter{Writer: w}, nil
+		return &report.SpectreHubReporter{Writer: w}, closer, nil
 	default:
-		return nil, fmt.Errorf("unsupported format: %s (use text, json, sarif, or spectrehub)", format)
+		return nil, closer, fmt.Errorf("unsupported format: %s (use text, json, sarif, or spectrehub)", format)
 	}
 }