Chore/code quality improvements

.claude/knowledge/testing-patterns.md

@@ -8,7 +8,7 @@ Testing strategies, test infrastructure quirks, how to run/debug specific test s
 - **Never commit a contiguous real-format secret as a test fixture.** GitHub push protection rejects the push (it flags GitLab/Stripe/SendGrid/Twilio/etc. tokens even in `_test.go` files), and it is poor practice in a secret-detection repo. Assemble fixtures at runtime from a prefix + body via the `cred(prefix, body)` helper in `tests/checks/test_helpers_test.go` (e.g. `cred("AKIA", "1234567890ABCDEF")`), splitting the recognizable provider prefix from the rest so no full token literal appears in source. The reconstructed value still exercises the scanner identically. `goConst(value)` wraps a value as a minimal Go source fixture.
 - The MCP server smoke tests (`tests/mcp/`) drive the **real binary in a subprocess**, not the in-process handler, to keep tests external. Pattern: a `Test...HelperProcess` func gated by an env var (`GO_WANT_MCP_HELPER_PROCESS` for stdio, `GO_WANT_MCP_HTTP_HELPER_PROCESS` for HTTP) re-execs `os.Args[0]` and calls `cli.Run(...)`. stdio replays NDJSON transcripts over stdin; HTTP reserves a free `127.0.0.1:0` port, launches `serve --mcp --http`, polls `/healthz` for readiness, then issues real HTTP requests. Add new MCP behavior as a transcript + assertion (stdio) or a subtest in `http_test.go` (HTTP).
 - **Detector precision corpus** (`tests/corpus/`): fixtures live under `testdata/<language-group>/<rule>/{vulnerable,clean}/` with ground truth in `expectations.yaml`. `known_gaps` entries assert a documented FP/FN *still exists* — when you improve a detector the corpus test fails on purpose with a "promote it to must_fire / remove the known_gaps entry" message; update the manifest in the same change. Fixture credentials must be synthetic but pattern-shaped (see the secret-fixture rule above).
-- **Every catalog rule must ship a `FixTemplate`** with a valid `Kind` (`deterministic`|`guided`) — `TestSDKRuleMetadataFixTemplatesPopulated` iterates the full catalog and fails on any rule without one. When adding a rule, add its template to the family map in `internal/codeguard/rules/catalog_fix_templates_*.go` (or inline in the catalog entry; inline wins over the registry).
+- **Every catalog rule must ship a `FixTemplate`** with a valid `Kind` (`deterministic`|`guided`) — `TestSDKRuleMetadataFixTemplatesPopulated` iterates the full catalog and fails on any rule without one. When adding a rule, add its template to the family map in `internal/codeguard/rules/catalog_fix_templates_*.go` (or inline in the catalog entry; inline wins over the registry). Security rules (`security.*`) additionally need an OWASP Top 10 mapping in `internal/codeguard/rules/catalog_security_owasp.go` — `TestEverySecurityRuleHasOWASPCategory` fails otherwise (only `security.command-check` is exempt, since its category depends on the external tool).
 - `gofmt -l .` at repo root is polluted by `.claude/worktrees/` (live agent worktrees) and `.gomodcache/`; scope it to `gofmt -l cmd internal pkg tests changelog.go` or use `make fmt-check`.
 - **Bidirectional (server→client) MCP tests** live in `tests/mcp/sampling_test.go`: the test acts as the MCP client, advertises `sampling`/`roots` at `initialize`, and answers the server's server-initiated requests. stdio uses interactive `StdinPipe`/`StdoutPipe` (not the replay harness). HTTP opens the `GET /mcp` SSE stream (waits for the `: ready` comment to avoid the attach race), reads the request off the stream, and POSTs the response with the matching `Mcp-Session-Id`. propose_fix verification is expected to fail on the throwaway diff — assert the round trip fired, not a verified patch. The HTTP helper passes `-config` via `CODEGUARD_TEST_HTTP_CONFIG`.
 - **TS tests can be hijacked by the Node semantic engine**: on hosts with a discoverable `typescript.js` (e.g. VS Code installed), TypeScript targets route through the Node runner instead of the per-file Go path. Tests that must exercise the per-file path (tree-sitter differential tests, corpus TS groups) set `CODEGUARD_TYPESCRIPT_LIB_PATH` to an existing-but-invalid lib to force the fallback.

docs/checks.md

@@ -529,6 +529,11 @@ Config keys (under `checks.security_rules.secrets`):
 - `entropy` enables the high-entropy heuristic (off by default); tune `min_length`
   (default 20), `threshold` in bits/char (default 4.5), and `level` (default `warn`).
 
+Invalid `allow_patterns`/`custom_patterns` entries are rejected by config validation. If
+an unvalidated config reaches the scan anyway (e.g. through the SDK), the unusable
+pattern is skipped and reported as a `security.secrets-config` (**fail**) finding rather
+than silently reducing coverage.
+
 Existing `exclude`, `waivers`, `baseline`, and inline `codeguard:ignore` suppressions
 also apply to these findings.
 

internal/cli/commands.go

@@ -50,9 +50,8 @@ func runValidate(args []string, stdout io.Writer, stderr io.Writer) int {
 		return code
 	}
 
-	cfg, err := loadConfigWithProfile(*configPath, *profile)
-	if err != nil {
-		_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
+	cfg, ok := loadConfigOrFail(*configPath, *profile, stderr)
+	if !ok {
 		return exitError
 	}
 	if err := service.ValidateConfig(cfg); err != nil {
@@ -88,9 +87,8 @@ func runScan(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer)
 		return exitError
 	}
 
-	cfg, err := loadConfigWithProfile(*inputs.configPath, *flags.profile)
-	if err != nil {
-		_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
+	cfg, ok := loadConfigOrFail(*inputs.configPath, *flags.profile, stderr)
+	if !ok {
 		return exitError
 	}
 	if trimmedFormat := strings.TrimSpace(*format); trimmedFormat != "" {
@@ -125,9 +123,8 @@ func runValidatePatch(args []string, stdin io.Reader, stdout io.Writer, stderr i
 		return exitError
 	}
 
-	cfg, err := loadConfigWithProfile(*configPath, *profile)
-	if err != nil {
-		_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
+	cfg, ok := loadConfigOrFail(*configPath, *profile, stderr)
+	if !ok {
 		return exitError
 	}
 	if trimmedFormat := strings.TrimSpace(*format); trimmedFormat != "" {
@@ -164,9 +161,8 @@ func runBaseline(args []string, stdout io.Writer, stderr io.Writer) int {
 	}
 	flags.applyTrustPolicy()
 
-	cfg, err := loadConfigWithProfile(*flags.configPath, *flags.profile)
-	if err != nil {
-		_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
+	cfg, ok := loadConfigOrFail(*flags.configPath, *flags.profile, stderr)
+	if !ok {
 		return exitError
 	}
 	cfg.Baseline.Path = ""

internal/cli/fix.go

@@ -22,22 +22,21 @@ func runFix(args []string, stdout io.Writer, stderr io.Writer) int {
 	line := fs.Int("line", 0, "optional 1-based line to target")
 	format := fs.String("format", "text", "output format: text or json")
 	if err := fs.Parse(args); err != nil {
-		return 1
+		return exitError
 	}
 	flags.applyTrustPolicy()
 	if !*enableAI {
 		_, _ = fmt.Fprintln(stderr, "fix requires -ai so unverified AI patch generation is never implicit")
-		return 1
+		return exitError
 	}
 	scanMode, err := parseScanMode(*flags.mode)
 	if err != nil {
 		_, _ = fmt.Fprintln(stderr, err)
-		return 1
+		return exitError
 	}
-	cfg, err := loadConfigWithProfile(*flags.configPath, *flags.profile)
-	if err != nil {
-		_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
-		return 1
+	cfg, ok := loadConfigOrFail(*flags.configPath, *flags.profile, stderr)
+	if !ok {
+		return exitError
 	}
 	report, err := service.RunWithOptions(context.Background(), cfg, service.ScanOptions{
 		Mode:     scanMode,
@@ -46,21 +45,21 @@ func runFix(args []string, stdout io.Writer, stderr io.Writer) int {
 	})
 	if err != nil {
 		_, _ = fmt.Fprintf(stderr, "scan failed: %v\n", err)
-		return 1
+		return exitError
 	}
 	finding, ok := selectFixFinding(report, strings.TrimSpace(*ruleID), strings.TrimSpace(*path), *line)
 	if !ok {
 		_, _ = fmt.Fprintln(stderr, "no matching finding available for fix generation")
-		return 1
+		return exitError
 	}
 	generator, available, err := internalfix.NewAIGenerator(cfg.AI)
 	if err != nil {
 		_, _ = fmt.Fprintf(stderr, "initialize ai generator: %v\n", err)
-		return 1
+		return exitError
 	}
 	if !available {
 		_, _ = fmt.Fprintln(stderr, "no AI provider is configured for fix generation")
-		return 1
+		return exitError
 	}
 	result, err := service.GenerateVerifiedFix(context.Background(), service.FixGenerateRequest{
 		Config:    cfg,
@@ -74,7 +73,7 @@ func runFix(args []string, stdout io.Writer, stderr io.Writer) int {
 	})
 	if err != nil {
 		_, _ = fmt.Fprintf(stderr, "generate verified fix: %v\n", err)
-		return 1
+		return exitError
 	}
 	return writeFixResult(stdout, stderr, result, strings.TrimSpace(*format))
 }
@@ -115,18 +114,18 @@ func writeFixResult(stdout io.Writer, stderr io.Writer, result service.VerifiedF
 				_, _ = fmt.Fprintf(stdout, "- %s (%s)\n", firstNonEmpty(step.CheckName, step.Command), step.TargetName)
 			}
 		}
-		return 0
+		return exitOK
 	case "json":
 		data, err := json.MarshalIndent(result, "", "  ")
 		if err != nil {
 			_, _ = fmt.Fprintf(stderr, "marshal fix result: %v\n", err)
-			return 1
+			return exitError
 		}
 		_, _ = stdout.Write(append(data, '\n'))
-		return 0
+		return exitOK
 	default:
 		_, _ = fmt.Fprintf(stderr, "unsupported fix output format %q\n", format)
-		return 1
+		return exitError
 	}
 }
 

internal/cli/helpers.go

@@ -27,6 +27,18 @@ func promptString(reader *bufio.Reader, stdout io.Writer, label string, fallback
 	return line, nil
 }
 
+// loadConfigOrFail wraps loadConfigWithProfile with the shared handler
+// convention: on failure it reports "load config: <err>" on stderr and returns
+// ok=false so the handler can return exitError.
+func loadConfigOrFail(path string, profile string, stderr io.Writer) (service.Config, bool) {
+	cfg, err := loadConfigWithProfile(path, profile)
+	if err != nil {
+		_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
+		return service.Config{}, false
+	}
+	return cfg, true
+}
+
 func loadConfigWithProfile(path string, profile string) (service.Config, error) {
 	cfg, err := service.LoadConfigFile(path)
 	if err != nil {

internal/cli/info.go

@@ -16,15 +16,14 @@ func runRules(args []string, stdout io.Writer, stderr io.Writer) int {
 	configPath := fs.String("config", "", "optional config path to include custom rule packs")
 	profile := fs.String("profile", "", "optional policy profile override")
 	if err := fs.Parse(args); err != nil {
-		return 1
+		return exitError
 	}
 
 	rules := service.Rules()
 	if strings.TrimSpace(*configPath) != "" {
-		cfg, err := loadConfigWithProfile(*configPath, *profile)
-		if err != nil {
-			_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
-			return 1
+		cfg, ok := loadConfigOrFail(*configPath, *profile, stderr)
+		if !ok {
+			return exitError
 		}
 		rules = service.RulesForConfig(cfg)
 	}
@@ -47,7 +46,7 @@ func runRules(args []string, stdout io.Writer, stderr io.Writer) int {
 		}
 		_, _ = fmt.Fprintln(stdout, line.String())
 	}
-	return 0
+	return exitOK
 }
 
 func runExplain(args []string, stdout io.Writer, stderr io.Writer) int {
@@ -57,22 +56,22 @@ func runExplain(args []string, stdout io.Writer, stderr io.Writer) int {
 	format := fs.String("format", "text", "output format: text, agent")
 	profile := fs.String("profile", "", "optional policy profile override")
 	if err := fs.Parse(args); err != nil {
-		return 1
+		return exitError
 	}
 	if fs.NArg() == 0 {
 		_, _ = fmt.Fprintln(stderr, "explain requires a rule id")
-		return 1
+		return exitError
 	}
 
 	ruleID := fs.Arg(0)
 	rule, ok, err := resolveExplainRule(*configPath, *profile, ruleID)
 	if err != nil {
 		_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
-		return 1
+		return exitError
 	}
 	if !ok {
 		_, _ = fmt.Fprintf(stderr, "unknown rule %q\n", ruleID)
-		return 1
+		return exitError
 	}
 
 	switch strings.TrimSpace(*format) {
@@ -81,13 +80,13 @@ func runExplain(args []string, stdout io.Writer, stderr io.Writer) int {
 	case "agent":
 		if err := writeExplainAgent(stdout, rule); err != nil {
 			_, _ = fmt.Fprintf(stderr, "write explain output: %v\n", err)
-			return 1
+			return exitError
 		}
 	default:
 		_, _ = fmt.Fprintf(stderr, "invalid explain format %q\n", *format)
-		return 1
+		return exitError
 	}
-	return 0
+	return exitOK
 }
 
 func resolveExplainRule(configPath string, profile string, ruleID string) (service.RuleMetadata, bool, error) {
@@ -175,5 +174,5 @@ func runProfiles(stdout io.Writer) int {
 	for _, profile := range service.Profiles() {
 		_, _ = fmt.Fprintf(stdout, "%s\t%s\n", profile.Name, profile.Description)
 	}
-	return 0
+	return exitOK
 }

internal/cli/owasp.go

@@ -19,15 +19,14 @@ func runOWASP(args []string, stdout io.Writer, stderr io.Writer) int {
 	profile := fs.String("profile", "", "optional policy profile override")
 	format := fs.String("format", "text", "output format: text or json")
 	if err := fs.Parse(args); err != nil {
-		return 1
+		return exitError
 	}
 
 	coverage := service.OWASPCoverage()
 	if strings.TrimSpace(*configPath) != "" {
-		cfg, err := loadConfigWithProfile(*configPath, *profile)
-		if err != nil {
-			_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
-			return 1
+		cfg, ok := loadConfigOrFail(*configPath, *profile, stderr)
+		if !ok {
+			return exitError
 		}
 		coverage = service.OWASPCoverageForConfig(cfg)
 	}
@@ -40,13 +39,13 @@ func runOWASP(args []string, stdout io.Writer, stderr io.Writer) int {
 		encoder.SetIndent("", "  ")
 		if err := encoder.Encode(coverage); err != nil {
 			_, _ = fmt.Fprintf(stderr, "write owasp output: %v\n", err)
-			return 1
+			return exitError
 		}
 	default:
 		_, _ = fmt.Fprintf(stderr, "invalid owasp format %q\n", *format)
-		return 1
+		return exitError
 	}
-	return 0
+	return exitOK
 }
 
 func writeOWASPText(stdout io.Writer, coverage []service.OWASPCoverageEntry) {

internal/cli/report.go

@@ -18,17 +18,16 @@ func runReport(args []string, stdout io.Writer, stderr io.Writer) int {
 	slopHistory := fs.Bool("slop-history", false, "print the persisted slop-score trend per target")
 	limit := fs.Int("limit", 0, "maximum history entries to print per target (0 = all)")
 	if err := fs.Parse(args); err != nil {
-		return 1
+		return exitError
 	}
 	if !*slopHistory {
 		_, _ = fmt.Fprintln(stderr, "report requires a mode flag: -slop-history")
-		return 1
+		return exitError
 	}
 
-	cfg, err := loadConfigWithProfile(*configPath, *profile)
-	if err != nil {
-		_, _ = fmt.Fprintf(stderr, "load config: %v\n", err)
-		return 1
+	cfg, ok := loadConfigOrFail(*configPath, *profile, stderr)
+	if !ok {
+		return exitError
 	}
 	return writeSlopHistoryReport(stdout, cfg, *limit)
 }
@@ -38,7 +37,7 @@ func writeSlopHistoryReport(stdout io.Writer, cfg service.Config, limit int) int
 	history := service.LoadSlopHistory(path)
 	if len(history) == 0 {
 		_, _ = fmt.Fprintf(stdout, "no slop-score history recorded at %s\n", path)
-		return 0
+		return exitOK
 	}
 	keys := make([]string, 0, len(history))
 	for key := range history {

internal/cli/scan_history.go

@@ -20,7 +20,7 @@ func runScanHistory(args []string, stdout io.Writer, stderr io.Writer) int {
 	allRefs := fs.Bool("all", false, "scan all refs rather than just HEAD history")
 	format := fs.String("format", "text", "output format: text or json")
 	if err := fs.Parse(args); err != nil {
-		return 1
+		return exitError
 	}
 
 	// Config is optional: it only supplies secret allowlist/custom-pattern/entropy
@@ -37,20 +37,20 @@ func runScanHistory(args []string, stdout io.Writer, stderr io.Writer) int {
 	})
 	if err != nil {
 		_, _ = fmt.Fprintf(stderr, "scan-history failed: %v\n", err)
-		return 1
+		return exitError
 	}
 
 	if err := writeHistoryReport(stdout, report, *format); err != nil {
 		_, _ = fmt.Fprintf(stderr, "scan-history output: %v\n", err)
-		return 1
+		return exitError
 	}
 
 	for _, finding := range report.Findings {
 		if finding.Level == "fail" {
-			return 1
+			return exitError
 		}
 	}
-	return 0
+	return exitOK
 }
 
 func writeHistoryReport(stdout io.Writer, report service.HistoryReport, format string) error {

internal/codeguard/checks/quality/quality_coverage_go.go

@@ -37,8 +37,10 @@ func goCoverageProfile(ctx context.Context, env support.Context, target core.Tar
 		Args:    args,
 	})
 	if err != nil {
+		// Keep err in the chain so callers can tell a timeout/cancellation from
+		// a test run that failed.
 		if strings.TrimSpace(output) != "" {
-			return nil, fmt.Errorf("go test: %s", output)
+			return nil, fmt.Errorf("go test: %w: %s", err, output)
 		}
 		return nil, fmt.Errorf("go test: %w", err)
 	}

internal/codeguard/checks/quality/quality_coverage_lcov.go

@@ -25,10 +25,12 @@ func commandCoverageProfile(ctx context.Context, env support.Context, target cor
 		Args:    command.Args,
 	})
 	if err != nil {
+		// Keep err in the chain so callers can tell a timeout/cancellation from
+		// a command that ran and reported failures.
 		if strings.TrimSpace(output) != "" {
-			return nil, fmt.Errorf("%s: %s", name, output)
+			return nil, fmt.Errorf("%s: %w: %s", name, err, output)
 		}
-		return nil, err
+		return nil, fmt.Errorf("%s: %w", name, err)
 	}
 	reportPath := command.ReportPath
 	if !filepath.IsAbs(reportPath) {