From ec7f3d9c1da94102a0e148e079048a6b49a89e62 Mon Sep 17 00:00:00 2001 From: Alex Wilkerson John Date: Thu, 3 Sep 2026 19:15:28 -0400 Subject: [PATCH 1/4] docs: plan tree-sitter scale, cross-file taint, and confidence policy Three design specs and their task-by-task implementation plans, covering the accuracy and performance work queued after 1.9.0: - tree-sitter scan scale: the tree path is capped at 64 files / 256 KiB per scan because trees are retained for the whole scan, so on any real repository most files silently fall back to the regex path the spike measured at 60%/54.5% precision. Replaces retention with a bounded resident cache plus heap-aware parse admission, and counts every fallback. - cross-file taint: Go taint analysis resolves calls only within one file, so a flow through a helper in another file is a false negative. Adds a package-qualified summary index iterated to a fixed point in reverse topological package order, with a package-closure cache fingerprint so cross-file findings cannot go stale. - confidence policy: findings carry a confidence nothing consumes. Makes it a configurable minimum threshold with full accounting, optional demotion, and confidence-aware ordering. Each plan is red-first per task and carries its own acceptance criteria. Co-Authored-By: Claude Opus 5 (1M context) --- .../plans/2026-09-03-confidence-policy.md | 225 +++++++++++++++++ .../plans/2026-09-03-cross-file-taint.md | 236 ++++++++++++++++++ .../plans/2026-09-03-treesitter-scan-scale.md | 198 +++++++++++++++ .../2026-09-03-confidence-policy-design.md | 78 ++++++ .../2026-09-03-cross-file-taint-design.md | 89 +++++++ ...2026-09-03-treesitter-scan-scale-design.md | 57 +++++ 6 files changed, 883 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-03-confidence-policy.md create mode 100644 docs/superpowers/plans/2026-09-03-cross-file-taint.md create mode 100644 docs/superpowers/plans/2026-09-03-treesitter-scan-scale.md create mode 100644 docs/superpowers/specs/2026-09-03-confidence-policy-design.md create mode 100644 docs/superpowers/specs/2026-09-03-cross-file-taint-design.md create mode 100644 docs/superpowers/specs/2026-09-03-treesitter-scan-scale-design.md diff --git a/docs/superpowers/plans/2026-09-03-confidence-policy.md b/docs/superpowers/plans/2026-09-03-confidence-policy.md new file mode 100644 index 0000000..90dec22 --- /dev/null +++ b/docs/superpowers/plans/2026-09-03-confidence-policy.md @@ -0,0 +1,225 @@ +# Confidence As Policy Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Turn the existing but unused finding confidence into a policy input — a configurable minimum threshold with full accounting, optional low-confidence demotion, and confidence-aware ordering — without changing any default behavior. + +**Architecture:** Two new fields on `core.CheckConfig`, validated in `config/validate.go`. Filtering and demotion happen in `FinalizeSectionWithDiagnostics`, before suppression matching and before section status, which keeps the change out of the per-file cache path entirely. Removed findings are tallied by `RuleStatsCollector` under a new reason. + +**Tech Stack:** Go, existing config load/validate, `runner/support` finalize gate, `RuleStatsCollector`, report writers. + +**Spec:** `docs/superpowers/specs/2026-09-03-confidence-policy-design.md` + +**Branch:** `feat/confidence-policy` + +## Global Constraints + +- Default configuration must produce identical findings, counts, statuses, exit codes, and JSON/SARIF bytes; the only permitted difference is within-group text ordering (Task 5). +- Confidence and reported level must not affect exact, context, or content fingerprints. +- Demotion never promotes and never touches medium or high. +- Threshold and demotion changes must not invalidate the per-file findings cache, so neither field may enter the named per-family fingerprints in `SectionConfigHashes`. +- Filtered findings must be counted, never silently dropped. +- No rule's assigned confidence changes in this branch. + +**Local toolchain:** `export GOROOT=/opt/homebrew/opt/go/libexec && export PATH=$GOROOT/bin:$PATH`, or use `make` targets. Use `GOCACHE=/private/tmp/codeguard-go-cache`. + +--- + +### Task 1: Config surface and validation + +**Files:** +- Modify: `internal/codeguard/core/config_types.go` +- Modify: `internal/codeguard/config/validate.go` +- Modify: `internal/codeguard/config/defaults.go` +- Test: `tests/config/confidence_policy_test.go` + +**Interfaces:** +- Produces: `core.ConfidencePolicyConfig{Default string, Sections map[string]string}` on `CheckConfig` as `min_confidence`, plus `ConfidenceDemotion bool` as `confidence_demotion`. +- Produces: `func (c ConfidencePolicyConfig) Threshold(sectionID string) string`. + +- [ ] **Step 1: Write failing config tests** + +Assert an absent block yields the permissive default; assert YAML and JSON both load global and per-section values; assert `Threshold` prefers the section entry then the default; assert validation rejects an unknown level and an unknown section key with a message shaped like the existing `parsers.treesitter` error. + +```go +if err := config.Validate(cfgWithConfidence("sideways")); err == nil { + t.Fatal("expected an error for an unknown confidence level") +} +``` + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/config -run 'Confidence' -count=1` + +- [ ] **Step 3: Implement the config fields, defaults, and validation** + +Place both fields directly on `CheckConfig`. Reuse `core.NormalizedConfidence` for level parsing and the existing section identifier list for key validation. + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/config -count=1` + +--- + +### Task 2: Confidence ranking helper + +**Files:** +- Modify: `internal/codeguard/core/finding_confidence.go` +- Test: `tests/core/finding_confidence_test.go` + +**Interfaces:** +- Produces: `core.ConfidenceRank(level string) int` and `core.MeetsConfidence(finding, threshold string) bool`, with empty confidence treated as medium per the documented mapping. + +- [ ] **Step 1: Write failing ranking tests** + +Cover high > medium > low, empty treated as medium, unknown input treated as medium, and threshold comparison at each of the three thresholds including the permissive one admitting everything. + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/core -run 'Confidence' -count=1` + +- [ ] **Step 3: Implement the helpers** + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/core -count=1` + +--- + +### Task 3: Filter gate with accounting + +**Files:** +- Modify: `internal/codeguard/runner/support/findings_section.go` +- Modify: `internal/codeguard/runner/support/rule_stats.go` +- Modify: `internal/codeguard/runner/support/suppressions.go` (reason constant) +- Modify: `internal/codeguard/core/report_artifact_rule_stats_types.go` +- Test: `tests/support/confidence_filter_test.go` +- Test: `tests/support/rule_stats_test.go` + +**Interfaces:** +- Adds: `SuppressionReasonConfidence`, a `confidence` tally on `ruleTally`, and the filter stage in `FinalizeSectionWithDiagnostics` before `MatchSuppression`. + +- [ ] **Step 1: Write failing filter and accounting tests** + +Assert a below-threshold finding is absent from `section.Findings`, counted in the section's suppressed accounting, and tallied under the confidence reason; assert a per-section threshold applies only to that section; assert the removed count plus the emitted count equals the pre-filter count; assert `IncludeSuppressed` surfaces the finding with its confidence reason; assert section status ignores filtered findings. + +```go +if emitted+filtered != total { + t.Fatalf("confidence filter lost findings: %d emitted + %d filtered != %d total", emitted, filtered, total) +} +``` + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/support -run 'Confidence|RuleStats' -count=1` + +- [ ] **Step 3: Implement the filter stage** + +Insert it after diff scoping and before waiver audit and suppression matching. Keep the reason distinct from the three suppression mechanisms in both the collector and the report artifact. + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/support -count=1 -race` + +--- + +### Task 4: Demotion + +**Files:** +- Modify: `internal/codeguard/runner/support/findings_section.go` +- Test: `tests/support/confidence_demotion_test.go` +- Test: `tests/support/context_fingerprint_test.go` + +- [ ] **Step 1: Write failing demotion and identity tests** + +Assert a low-confidence `fail` reports as `warn` with demotion on and drives section status to warn; assert medium and high are untouched; assert `warn` is never promoted; assert `Level` and `Severity` stay consistent; assert all three fingerprints are identical with demotion on and off. + +```go +if demoted.Fingerprint != undemoted.Fingerprint || demoted.ContextFingerprint != undemoted.ContextFingerprint || demoted.ContentFingerprint != undemoted.ContentFingerprint { + t.Fatal("confidence demotion changed finding identity") +} +``` + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/support -run 'Demotion|Fingerprint' -count=1` + +- [ ] **Step 3: Implement demotion before the status switch** + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/support -count=1` + +--- + +### Task 5: Ordering and report surfaces + +**Files:** +- Modify: `internal/codeguard/report/text_helpers.go` (`groupTextFindings` only) +- Test: `tests/report/ordering_test.go` +- Test: `tests/report/confidence_report_test.go` + +**Interfaces:** +- Changes: within-group ordering in the text renderer only. Section finding order is untouched, so JSON and SARIF serialization is unaffected. + +- [ ] **Step 1: Write failing ordering and report tests** + +Assert a rule group mixing confidences lists high before medium before low; assert the sort is stable so equal-confidence findings keep scan order; assert group identity and group order are unchanged; assert JSON and SARIF bytes are unchanged for the default config; assert the text report states how many findings the confidence threshold removed when any were. + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/report -run 'Ordering|Confidence' -count=1` + +- [ ] **Step 3: Implement the within-group stable sort and the threshold summary line** + +Sort inside `groupTextFindings` with `sort.SliceStable` on `core.ConfidenceRank`. Do not sort `section.Findings`. + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/report -count=1` + +--- + +### Task 6: Cache invariance and default-behavior regression + +**Files:** +- Test: `tests/support/cache_confidence_test.go` +- Test: `tests/corpus/` (default-config regression) + +- [ ] **Step 1: Write the cache invariance test** + +Scan, then rescan with a different `min_confidence` and with demotion toggled: assert per-file cache hits (no recomputation) and that the rendered findings differ as configured. Assert neither field appears in any named per-family fingerprint from `SectionConfigHashes`. + +- [ ] **Step 2: Write the default-behavior regression** + +With no confidence configuration, assert the corpus suite's findings, counts, statuses, and exit code are identical to the pre-change baseline. + +- [ ] **Step 3: Run both and verify** + +Run: `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/support ./tests/corpus -count=1` + +--- + +### Task 7: Docs and full gate + +**Files:** +- Modify: `docs/checks.md` (config reference for both fields) +- Modify: `docs/features.md` (baseline governance / policy surface) +- Modify: `README.md` (one line in the capability paragraph) +- Modify: `.claude/knowledge/architecture-boundaries.md` + +- [ ] **Step 1: Document the config with a worked YAML example** + +Follow the existing `docs/features.md` config-example style, including a per-section override and a note that thresholds re-render rather than re-run a scan. + +- [ ] **Step 2: Record the self-scan deltas** + +Run the self-scan at each threshold and record the finding counts and the confidence-filtered tallies, so the accounting reconciliation is documented, not just tested. + +- [ ] **Step 3: Full gate** + +Run: `make fmt && make lint && make test && make codeguard-ci`, then strict lint: `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache GOLANGCI_LINT_CACHE=/private/tmp/golangci-lint-cache /Users/alex/.go/bin/golangci-lint run` (0 issues), then `go test ./... -race`. + +- [ ] **Step 4: Capture the identity rule in knowledge** + +Note that confidence and reported level are excluded from finding identity, alongside prose and metadata, so future policy knobs inherit the constraint. diff --git a/docs/superpowers/plans/2026-09-03-cross-file-taint.md b/docs/superpowers/plans/2026-09-03-cross-file-taint.md new file mode 100644 index 0000000..eca76dd --- /dev/null +++ b/docs/superpowers/plans/2026-09-03-cross-file-taint.md @@ -0,0 +1,236 @@ +# Cross-File Taint Analysis Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Detect Go source-to-sink flows that cross file and in-repo package boundaries, without guessing at unresolved hops and without stale cached verdicts. + +**Architecture:** A target-level pre-pass builds `goFuncSummary` values for every non-test Go function, keyed by package-qualified identity, iterating to a fixed point in reverse topological package order. The per-file reporting pass keeps its current structure but resolves call sites against that index after exhausting same-file lookup. Taint gets its own cache section id whose entries carry a package-closure dependency hash. + +**Tech Stack:** Go, `go/ast`, `support.BuildGoPackageImportGraph`, `dependency_graph_tarjan.go`, the shared per-scan corpus, the persisted findings cache. + +**Spec:** `docs/superpowers/specs/2026-09-03-cross-file-taint-design.md` + +**Branch:** `feat/cross-file-taint` + +## Global Constraints + +- Go only; Python and C++ analyzers are untouched, but the index seam must not be Go-shaped in its exported names. +- An unresolved hop never produces a finding. No allowlists, no assumed propagation. +- Findings anchor at the sink file and line; rule IDs, levels, and fix text are unchanged. +- Fingerprints of currently-detected intra-file flows must not change. +- Chain prose must not participate in fingerprint identity. +- Analysis stays bounded on iterations, hop depth, and indexed functions, and degrades to intra-file behavior with a diagnostic rather than emitting partial chains. +- Diff mode indexes the whole target and reports only changed lines. + +**Local toolchain:** `export GOROOT=/opt/homebrew/opt/go/libexec && export PATH=$GOROOT/bin:$PATH`, or use `make` targets. Use `GOCACHE=/private/tmp/codeguard-go-cache`. + +--- + +### Task 1: Cross-file fixtures and expectations (red first) + +**Files:** +- Add: `tests/corpus/` fixtures for cross-file flows +- Modify: `tests/corpus/expectations.yaml` +- Test: `tests/checks/security_taint_cross_file_test.go` + +**Interfaces:** +- Produces: the ground truth this branch is built against, expressed before any implementation. + +- [ ] **Step 1: Write the positive fixtures** + +Same-package two-file flow; cross-package flow through an in-repo import; flow through a package cycle; flow through a method on a receiver; flow whose hop is three functions deep. + +```go +// handler.go +func Handle(r *http.Request) { run(r.URL.Query().Get("cmd")) } +// exec.go +func run(arg string) { exec.Command("sh", "-c", arg).Run() } +``` + +- [ ] **Step 2: Write the negative fixtures** + +Sanitized flow (`shlex`-style quoting or `strconv` parse before the sink); interface-dispatch hop; function-value-parameter hop; call into a package outside the target. Each asserts zero findings. + +- [ ] **Step 3: Run and verify red on positives, green on negatives** + +Run: `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/corpus ./tests/checks -run 'Taint.*CrossFile|TestCorpusExpectations' -count=1 -v` + +Positives must fail now — that failure is the false negative this branch exists to remove. Negatives passing now must still pass at the end. + +--- + +### Task 2: Package-qualified summary index + +**Files:** +- Add: `internal/codeguard/checks/security/security_taint_index.go` +- Modify: `internal/codeguard/checks/security/security_taint_go_types.go` +- Test: `tests/checks/security_taint_index_test.go` + +**Interfaces:** +- Produces: `taintSummaryIndex` keyed by `.` and `.().`, holding `*goFuncSummary`. +- Consumes: `support.Context`, `core.TargetConfig`, `ParseGoFile` via the shared corpus, `support.BuildGoPackageImportGraph`. + +- [ ] **Step 1: Write failing index tests** + +Assert identity keying for plain functions, pointer-receiver methods, and value-receiver methods; that `_test.go` files are excluded; that generated and vendored paths follow the existing exclusion rules; and that the index is built once per target per scan. + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/checks -run 'TestTaintSummaryIndex' -count=1` + +- [ ] **Step 3: Implement index construction** + +Walk the target through the corpus, parse each non-test Go file, and record one summary slot per declaration. Reuse `goTaintAnalyzer.analyzeFunction` for summary computation so intra-function semantics stay identical; only the function map's scope changes. + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/checks -run 'TestTaintSummaryIndex' -count=1 -race` + +--- + +### Task 3: Fixed point in package order + +**Files:** +- Modify: `internal/codeguard/checks/security/security_taint_index.go` +- Modify: `internal/codeguard/checks/security/security_taint_go.go` +- Test: `tests/checks/security_taint_index_test.go` + +**Interfaces:** +- Consumes: the package graph's SCCs from `dependency_graph_tarjan.go`. +- Produces: summaries that are final for callees before callers are analyzed, with a bounded iteration count per SCC. + +- [ ] **Step 1: Write failing order and bound tests** + +Assert a callee's summary is final before its caller's is computed for an acyclic pair; assert a two-package cycle converges; assert a synthetic pathological cycle stops at the iteration bound and records a degradation diagnostic instead of looping. + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/checks -run 'TestTaintSummaryFixedPoint' -count=1` + +- [ ] **Step 3: Implement ordered iteration** + +Compute SCCs, process in reverse topological order, iterate within an SCC until summaries stabilize or the bound is hit. Preserve the existing three-pass behavior inside a single package. + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/checks -count=1` + +--- + +### Task 4: Cross-file call resolution and chain rendering + +**Files:** +- Modify: `internal/codeguard/checks/security/security_taint_go_calls.go` +- Modify: `internal/codeguard/checks/security/security_taint_go_walk.go` +- Modify: `internal/codeguard/checks/security/security_taint.go` (file-qualified chain steps) +- Test: `tests/checks/security_taint_cross_file_test.go` +- Test: `tests/support/context_fingerprint_test.go` + +**Interfaces:** +- Consumes: `taintSummaryIndex`, the file's import declarations and alias bindings. +- Produces: findings anchored at the sink with file-qualified chain steps and `confidence: high`. + +- [ ] **Step 1: Write failing resolution and fingerprint tests** + +Assert the precedence order same-file → same-package → imported in-repo package; assert an unresolved hop yields no finding and increments an unresolved counter; assert chain messages name the file once the chain leaves the reporting file; assert a currently-detected intra-file flow keeps its exact, context, and content fingerprints. + +```go +if crossFile.Fingerprint != intraFileBaseline.Fingerprint { + t.Fatal("cross-file chain rendering changed intra-file finding identity") +} +``` + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/checks ./tests/support -run 'Taint.*CrossFile|Fingerprint' -count=1` + +- [ ] **Step 3: Implement resolution and rendering** + +Extend call-site handling to consult the index after same-file lookup, thread the callee's file into the chain step, and stop chains at unresolved hops with a counted reason. Keep the dedupe key as sink line, sink name, and source. + +- [ ] **Step 4: Verify green including the corpus positives from Task 1** + +Run: `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/corpus ./tests/checks ./tests/support -count=1` + +--- + +### Task 5: Cache dependency fingerprint + +**Files:** +- Modify: `internal/codeguard/runner/support/cache_types.go` +- Modify: `internal/codeguard/runner/support/findings_scan.go` +- Modify: `internal/codeguard/runner/support/cache_helpers.go` +- Modify: `internal/codeguard/checks/security/security.go` (taint scans under its own section id) +- Test: `tests/support/cache_taint_test.go` + +**Interfaces:** +- Adds: `cacheEntry.DepsHash`, populated for the taint section from the transitive package-closure file hashes. +- Bumps: `scanCacheVersion` from 8 to 9. + +- [ ] **Step 1: Write failing cache invalidation tests** + +Scan, mutate a helper in a dependency package, rescan: assert the caller's taint entry is recomputed and the new cross-file finding appears. Mutate an unrelated package: assert the caller's entry is a cache hit. Assert a v8 cache on disk is discarded rather than reused. + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/support -run 'TestTaintCache' -count=1` + +- [ ] **Step 3: Implement the dependency hash** + +Give taint its own section id, compute the closure hash from the package graph plus per-file content hashes, store it on the entry, and require it to match for a hit. Bump the cache version. + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/support ./tests/checks -count=1` + +--- + +### Task 6: Bounds, degradation, diagnostics + +**Files:** +- Modify: `internal/codeguard/checks/security/security_taint_index.go` +- Modify: `internal/codeguard/checks/security/security_taint_go.go` +- Test: `tests/checks/security_taint_bounds_test.go` +- Test: `internal/codeguard/report/diagnostics_test.go` + +- [ ] **Step 1: Write failing bounds tests** + +Assert that exceeding indexed-function, hop-depth, or iteration bounds degrades the target to intra-file analysis with a diagnostic and no partial cross-file finding; assert corpus AST budget exhaustion degrades the same way; assert unresolved-hop counts are reported by reason and are not findings. + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/checks ./internal/codeguard/report -run 'Taint.*(Bounds|Degrad)|Diagnostic' -count=1` + +- [ ] **Step 3: Implement bounds and degradation** + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/... -count=1` + +--- + +### Task 7: Docs, benchmarks, full gate + +**Files:** +- Modify: `docs/checks.md` (taint rule descriptions now state cross-file scope and its limits) +- Modify: `docs/features.md` +- Modify: `docs/benchmarks.md` (refreshed detector-quality numbers) +- Modify: `.claude/knowledge/architecture-boundaries.md` + +- [ ] **Step 1: Re-run the detector-quality lane and record the numbers** + +Run: `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/corpus -run TestCorpusExpectations -v` + +Record TP / FN / FP against the 68 / 0 / 1 baseline. Any new false positive blocks the branch. + +- [ ] **Step 2: Measure the pre-pass cost** + +Full scan with and without the branch on the reference repositories; assert wall clock growth under 15%. + +- [ ] **Step 3: Update docs to state the scope honestly** + +Cross-file and cross-package within a target; interface dispatch and function values unresolved; out-of-target calls unresolved. + +- [ ] **Step 4: Full gate** + +Run: `make fmt && make lint && make test && make codeguard-ci`, then strict lint: `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache GOLANGCI_LINT_CACHE=/private/tmp/golangci-lint-cache /Users/alex/.go/bin/golangci-lint run` (0 issues), then `go test ./... -race`. diff --git a/docs/superpowers/plans/2026-09-03-treesitter-scan-scale.md b/docs/superpowers/plans/2026-09-03-treesitter-scan-scale.md new file mode 100644 index 0000000..d52dfca --- /dev/null +++ b/docs/superpowers/plans/2026-09-03-treesitter-scan-scale.md @@ -0,0 +1,198 @@ +# Tree-Sitter Scan Scale Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the tree-sitter parsing path reachable across a whole repository by replacing scan-lifetime tree retention with a bounded resident cache and heap-aware parse admission, and by counting every fallback. + +**Architecture:** `fileCorpus` keeps the `parseScript` signature and its content-hash keying, but its `scripts` map becomes an LRU bounded by measured retained tree bytes, and parse admission reserves transient heap instead of taking a capacity-1 token. Checks are untouched: `support.ScriptSyntaxTree` still returns nil on any refusal and every rule keeps its regex fallback. + +**Tech Stack:** Go, `github.com/odvcencio/gotreesitter`, existing corpus diagnostics, `tests/corpus` expectation groups. + +**Spec:** `docs/superpowers/specs/2026-09-03-treesitter-scan-scale-design.md` + +**Branch:** `feat/treesitter-scan-scale` + +## Global Constraints + +- No rule semantics change; no rule migrates in this branch. +- `parsers.treesitter` default stays `off`, and the `off` path must be byte-identical to today. +- Tree cache keying stays `path + language + content hash`. +- A tree handed to a section must stay valid for that section's use even if evicted from the cache. +- Fallback must stay silent in findings but visible in diagnostics. +- Per-file `MaxTreeSitterFileBytes` stays 256 KiB. +- Run concurrency-touching tests with `-race`. +- New tests go under `tests/` as external packages (CLAUDE.md). The two exceptions in this plan are the *existing* in-package files `corpus_memory_test.go` and `corpus_script_test.go`, which already assert unexported corpus budgets and are permitted by `ci_rules.allowed_test_paths` in `.codeguard/codeguard.yaml`; extend them rather than adding new in-package test files. + +**Local toolchain:** `export GOROOT=/opt/homebrew/opt/go/libexec && export PATH=$GOROOT/bin:$PATH`, or use `make` targets which run `env -u GOROOT go`. Use `GOCACHE=/private/tmp/codeguard-go-cache`. + +--- + +### Task 1: Measure retained tree cost + +**Files:** +- Add: `tests/checks/treeprovider_cost_test.go` (measurement needs only the exported `ParseScriptSource`, so it belongs in the external test tree per CLAUDE.md) +- Modify: `docs/superpowers/specs/2026-09-03-treesitter-scan-scale-design.md` (record the measured factors) + +**Interfaces:** +- Produces: measured `retainedBytesPerSourceByte` and `transientBytesPerSourceByte` factors for the TypeScript, TSX, JavaScript, and Python grammars. + +- [ ] **Step 1: Write a benchmark that separates retained from transient cost** + +Parse fixtures of ~1 KB, ~10 KB, ~100 KB and ~250 KB. For transient cost, sample `runtime.MemStats.TotalAlloc` delta across one parse. For retained cost, hold the tree, force `runtime.GC()`, and read `HeapAlloc` against a baseline with the tree dropped. Report bytes per source byte for both. + +- [ ] **Step 2: Run it and record the numbers** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./tests/checks -run TestTreeRetainedCost -v -count=1` + +- [ ] **Step 3: Write the measured factors into the spec's Resident Cache Model section** + +Replace the derivation placeholders with the measured values and the date, so the ceiling defaults below are traceable. + +--- + +### Task 2: Bounded resident tree cache + +**Files:** +- Modify: `internal/codeguard/runner/support/corpus.go` +- Modify: `internal/codeguard/runner/support/corpus_access.go` (only if the parse entry point needs a lease-aware signature) +- Test: `internal/codeguard/runner/support/corpus_memory_test.go` +- Test: `internal/codeguard/runner/support/corpus_script_test.go` + +**Interfaces:** +- Replaces: `maxTreeSitterScanBytes`, `maxTreeSitterScanFiles`. +- Adds: `maxResidentTreeBytes` ceiling plus LRU eviction over `c.scripts`. +- Preserves: `parseScript(path, data, lang) (*checkSupport.SyntaxTree, error)`. + +- [ ] **Step 1: Write failing tests for coverage past the old budgets** + +Assert that parsing 300 distinct 8 KiB script files all succeed (no budget error), that the resident set stays at or under the ceiling, that a re-request of an evicted key re-parses and returns an equivalent tree, and that two goroutines racing a cold key parse once. + +```go +for i := range 300 { + tree, err := corpus.parseScript(fmt.Sprintf("file%d.ts", i), source, checkSupport.ScriptLangTypeScript) + if err != nil { + t.Fatalf("file %d refused by a scan-wide budget: %v", i, err) + } + _ = tree +} +if resident := corpus.residentTreeBytes(); resident > maxResidentTreeBytes { + t.Fatalf("resident tree bytes %d exceed ceiling %d", resident, maxResidentTreeBytes) +} +``` + +- [ ] **Step 2: Run the focused tests and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./internal/codeguard/runner/support -run 'TestCorpus.*(Script|Tree)' -count=1 -race` + +- [ ] **Step 3: Implement the LRU** + +Track resident bytes using the Task 1 retained factor. On admission, evict least-recently-used entries until the new tree fits. Eviction drops the cache's reference only; a caller that already holds the tree keeps it alive. Keep the per-slot `sync.Once` semantics for cold-key races, and keep entries keyed by content hash. Record an eviction/re-parse counter for Task 4. + +- [ ] **Step 4: Verify green with race detection** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./internal/codeguard/runner/support -count=1 -race` + +--- + +### Task 3: Heap-aware parse admission + +**Files:** +- Modify: `internal/codeguard/runner/support/corpus.go` +- Test: `internal/codeguard/runner/support/corpus_memory_test.go` + +**Interfaces:** +- Replaces: `scriptParse chan struct{}` of capacity 1. +- Adds: an in-flight transient-heap reservation ceiling and a worker cap. + +- [ ] **Step 1: Write failing concurrency and admission tests** + +Assert that several small-file parses overlap (observed concurrency > 1), that a parse whose reservation exceeds the remaining ceiling waits rather than proceeding, and that total in-flight reserved bytes never exceed the ceiling. + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./internal/codeguard/runner/support -run 'TestScriptParseAdmission' -count=1 -race` + +- [ ] **Step 3: Implement reservation-based admission** + +Reserve `len(data) * transientFactor` from Task 1 against the ceiling, block until it fits, release on completion. Cap concurrent parses at `min(4, runtime.NumCPU())` independently of the byte ceiling. A reservation larger than the whole ceiling is admitted alone rather than deadlocking. + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./internal/codeguard/runner/support -count=1 -race` + +--- + +### Task 4: Fallback telemetry + +**Files:** +- Modify: `internal/codeguard/runner/support/corpus.go` +- Modify: `internal/codeguard/checks/support/treeprovider_parse.go` (typed refusal reasons) +- Modify: `internal/codeguard/checks/support/treeprovider.go` (`ScriptSyntaxTree` reason pass-through) +- Test: `internal/codeguard/runner/support/corpus_script_test.go` +- Test: `internal/codeguard/report/diagnostics_test.go` + +**Interfaces:** +- Produces: per-scan counts of tree-path refusals by reason (`oversize`, `parse_error`, `error_heavy`, `grammar_missing`, `reparse`) and language. +- Consumes: existing `core.Diagnostic` emission via `recordBudget`'s sibling path. + +- [ ] **Step 1: Write failing telemetry tests** + +Assert that an oversized file, a syntactically broken file, and an error-heavy file each increment their own counter with the right language, that counters aggregate across sections, and that they surface as informational diagnostics which do not become findings and do not change exit status. + +- [ ] **Step 2: Run and verify red** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./internal/codeguard/runner/support ./internal/codeguard/report -run 'Tree.*(Fallback|Diagnostic)' -count=1` + +- [ ] **Step 3: Implement typed refusal reasons and counting** + +Return a typed reason from `ParseScriptSource` refusals instead of bare `fmt.Errorf` strings, thread it through `parseScript`, and tally it on the corpus. Keep `ScriptSyntaxTree`'s nil-on-refusal contract so no check changes. + +- [ ] **Step 4: Verify green** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./internal/codeguard/runner/support ./internal/codeguard/report -count=1` + +--- + +### Task 5: Coverage and performance evidence + +**Files:** +- Test: `tests/corpus/` (extend the `typescript-treesitter` group) +- Modify: `docs/treesitter-spike.md` (Phase 2 → Phase 3 status and the coverage numbers) +- Modify: `docs/features.md` (Parsers section) +- Modify: `docs/checks.md` (tree-path coverage note) + +- [ ] **Step 1: Grow the corpus group past the old budgets** + +Add enough TypeScript fixtures to the `typescript-treesitter` group that the old 64-file / 256 KiB budgets would have been exhausted, with expectations that only hold on the tree path. + +- [ ] **Step 2: Measure coverage and wall clock on a reference repo** + +Scan a vendored snapshot of a real TS repository with `off` and with `auto`. Record: tree-served file count, refusals by reason, wall clock for both modes, peak RSS for both modes. + +- [ ] **Step 3: Assert the acceptance thresholds** + +Every file at or under 256 KiB is tree-served; zero refusals attributed to a scan-wide budget; `auto` wall clock under 2x `off`; peak retained tree bytes under the ceiling. + +- [ ] **Step 4: Update the docs with the measured numbers** + +Record the coverage and timing evidence in the spike doc as the Phase 3 readiness input, and note in `docs/features.md` that `auto` now covers whole repositories. + +--- + +### Task 6: Full gate + +- [ ] **Step 1: Format, lint, test, self-scan** + +Run: `make fmt && make lint && make test && make codeguard-ci` + +- [ ] **Step 2: Strict lint** + +Run: `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache GOLANGCI_LINT_CACHE=/private/tmp/golangci-lint-cache /Users/alex/.go/bin/golangci-lint run` (must be 0 issues) + +- [ ] **Step 3: Race pass over the scan path** + +Run: `GOCACHE=/private/tmp/codeguard-go-cache go test ./internal/codeguard/runner/... ./tests/... -count=1 -race` + +- [ ] **Step 4: Capture knowledge** + +Record the measured heap factors and the resident-ceiling rationale in `.claude/knowledge/architecture-boundaries.md` under the scanner hardening invariants. diff --git a/docs/superpowers/specs/2026-09-03-confidence-policy-design.md b/docs/superpowers/specs/2026-09-03-confidence-policy-design.md new file mode 100644 index 0000000..6ae6007 --- /dev/null +++ b/docs/superpowers/specs/2026-09-03-confidence-policy-design.md @@ -0,0 +1,78 @@ +# Confidence As Policy Design + +## Scope + +Findings already carry a confidence level that nothing consumes. This change makes confidence a policy input: a configurable minimum threshold that filters findings with full accounting, an optional level demotion for low-confidence hits, and confidence as a deterministic ordering key in reports. + +It does not change which confidence any rule assigns, does not add or remove rules, and does not alter finding identity. Defaults reproduce today's behavior exactly. + +## Problem + +`core.ConfidenceHigh`, `ConfidenceMedium`, and `ConfidenceLow` exist, `NormalizedConfidence` maps unspecified to medium, and 177 call sites set the field. `NewFinding` stores it, `report/sarif_builders.go` emits it as a SARIF result property, and `report/text_helpers.go` appends "(low confidence)" to the text renderer's `why` line. That is the whole of it: no filtering, no ordering, no effect on section status or exit code. + +The gap matters most where accuracy work has already landed. Tree-sitter-derived findings are tagged `high` and their regex fallbacks are not, and structured taint analysis is tagged `high` while regex line scans are effectively medium — but a consumer cannot ask for only the trustworthy half. A team that wants coverage without gate noise has to disable rules wholesale, which loses the signal entirely, or baseline the noise, which hides it. + +## Configuration + +One struct on `CheckConfig`, holding the global default and per-section overrides: + +```yaml +checks: + min_confidence: + default: low # low | medium | high; low is today's behavior + sections: + security: high + confidence_demotion: false +``` + +Section overrides sit under their own `sections` key rather than beside +`default`, so the block decodes as a plain struct with no reserved-key +handling in the codec. + +`default: low` admits everything, which is why the default configuration is behavior-preserving. Section keys are the ids that appear in scan output and in `checks.disabled` — note that supply chain is `supply_chain` there, while the runner's internal registry id is `supply-chain`. Validation rejects unknown levels and unknown section keys with the same message style as `parsers.treesitter` validation in `internal/codeguard/config/validate.go`. + +Both fields live directly on `CheckConfig` rather than inside any rule struct. `SectionConfigHashes` additionally strips them before fingerprinting, so they stay out of every family including the conservative all-checks fallback, which is required for the caching property below. + +## Filter Placement + +Filtering happens in `FinalizeSectionWithDiagnostics` (`internal/codeguard/runner/support/findings_section.go`), the single gate every section's findings already pass through, immediately before suppression matching and before section status is computed. + +That placement gives two properties: + +- Findings are filtered after the per-file cache (`cachedFileFindings`), so changing a threshold never invalidates cached findings. A threshold change re-renders a scan; it does not re-run one. +- Diff-mode scoping, waiver auditing, suppression precedence, and status computation keep their existing order and semantics. + +## Accounting + +A filtered finding is never silently dropped. It is counted through the existing `RuleStatsCollector` under a new reason alongside baseline, waiver, and inline suppression, and it is included in the section's suppressed accounting so the report can state how many findings the threshold removed and for which rules. + +Confidence filtering is reported as its own mechanism, not folded into suppression counts, because the two answer different questions: a suppression says a team accepted a finding, a confidence filter says the tool was not sure enough to show it. `--include-suppressed` surfaces confidence-filtered findings with their reason, as it does for the other mechanisms. + +## Demotion + +With `confidence_demotion: true`, a `low`-confidence finding on a rule whose level is `fail` reports as `warn`. Demotion never promotes, never applies to `medium` or `high`, and applies before section status is computed so a demoted finding cannot fail a gate. It is off by default. + +Demotion and thresholds compose in one direction: the threshold decides whether a finding is shown, then demotion decides how loudly. A finding removed by the threshold is not demoted, it is gone. + +## Ordering + +Findings today carry no explicit sort: they reach the report in file-scan order, and the text renderer groups them by rule title through `groupTextFindings`. Sorting at the section level would therefore reorder JSON and SARIF output as a side effect, which the byte-identical default rules out. + +Confidence ordering is instead applied as a stable sort by confidence rank *within* each existing text finding group. Group identity and group order are unchanged, JSON and SARIF field order is untouched, and the only visible difference is that a rule group mixing confidences lists its high-confidence hits first. The sort is stable, so equal-confidence findings keep their scan order and output stays reproducible. + +## Identity + +Confidence must not affect the exact, context, or content fingerprint. Neither may demotion: a demoted finding keeps the identity it would have had at its catalog level, so an existing baseline entry keeps matching after `confidence_demotion` is switched on. This follows the identity rule established for structural findings — prose, metadata, and now confidence and reported level are excluded from identity. + +## Non-Goals + +Mapping confidence onto SARIF `rank` is not part of this change; confidence stays a result property so external consumers see no schema churn. Auditing the 177 emission sites to replace the empty-means-medium default with explicit values is also out of scope: the empty mapping is documented and the accuracy-relevant contrast (tree path `high` versus regex fallback default) already holds. + +## Acceptance + +- With no `min_confidence` configured, findings, counts, section statuses, exit codes, and JSON/SARIF bytes are identical to the current implementation across the corpus suite and the self-scan. Text output may differ only in within-group ordering where a rule group mixes confidences, and that difference is pinned by a test. +- `min_confidence.default: high` on the self-scan reduces the finding count, and every removed finding is accounted for in rule stats under the confidence reason, with the two totals reconciling exactly. +- A per-section override applies to that section only. +- `confidence_demotion: true` turns a low-confidence `fail` into a `warn`, changes section status accordingly, and leaves all three fingerprints unchanged. +- Changing a threshold or the demotion flag produces cache hits, not recomputation, proven by a cache test. +- Ordering tests pin the confidence tiebreak. diff --git a/docs/superpowers/specs/2026-09-03-cross-file-taint-design.md b/docs/superpowers/specs/2026-09-03-cross-file-taint-design.md new file mode 100644 index 0000000..4d53ac3 --- /dev/null +++ b/docs/superpowers/specs/2026-09-03-cross-file-taint-design.md @@ -0,0 +1,89 @@ +# Cross-File Taint Analysis Design + +## Scope + +This change extends Go source-to-sink taint analysis from one file to the target's package closure. It adds a target-level function summary index, resolves call sites across files and imported in-repo packages, and keeps the per-file findings cache correct under cross-file dependence. + +Go only. The summary index seam is language-neutral so Python and C++ can follow, but neither is migrated here. No new rules: `security.taint.go`, `security.ssrf.go`, and the other Go taint rule IDs keep their identity, level, and fix guidance. + +## Problem + +`goTaintAnalyzer` (`internal/codeguard/checks/security/security_taint_go.go`) documents its own limit: intra-file analysis with call-site resolution for functions declared in the same file. It indexes `parsed.Decls` from one file, runs three fixed passes to a summary fixed point, and reports on the third. + +The result is a false negative for the most common real shape, where the flow crosses a file boundary: + +```go +// handler.go +func Handle(r *http.Request) { run(r.URL.Query().Get("cmd")) } + +// exec.go +func run(arg string) { exec.Command("sh", "-c", arg).Run() } +``` + +Both halves are analyzed, neither sees a flow. `run` is not in `handler.go`'s function map, so the argument's taint stops at the call; `exec.go` has no source, so its parameter sink never fires. Splitting a handler and its helper across files is enough to hide the vulnerability entirely. + +## Summary Index + +A target-level pre-pass builds one summary per function declaration, keyed by resolved identity rather than bare name: + +- package-level function: `.` +- method: `.().` + +The summary value is the existing `goFuncSummary` — `returnTaint`, `paramsToReturn`, `paramsToSink` — unchanged in shape. What changes is its scope and lifetime: summaries are computed for every non-test Go file in the target and shared, instead of being rebuilt per file and discarded. + +The pre-pass reads and parses through the shared per-scan corpus (`ParseGoFile`), so files already parsed by another section are not re-parsed, and the pass inherits the corpus AST budgets. + +## Fixed Point Order + +Summaries iterate to a fixed point in reverse topological order of the package import graph, so a callee's summary is final before its callers are analyzed. Both inputs already exist: `support.BuildGoPackageImportGraph` gives the target-local package graph and the file-to-package map, and `dependency_graph_tarjan.go` gives the strongly-connected components. Packages in a cycle iterate together until their summaries stop changing or the iteration bound is reached. + +Within a package the existing three-pass structure is retained for intra-package recursion. + +## Call Resolution + +A call site resolves in a fixed precedence: same file, then same package, then an imported in-repo package via the file's import declarations and alias bindings. Standard library and third-party calls continue to resolve against the existing source/sink/sanitizer models, not the summary index. + +Only fully resolved chains produce findings. When a hop cannot be resolved — dynamic dispatch through an interface value, a function value passed as a parameter, a call into a package outside the target — the chain stops and no finding is emitted for it. Unresolved hops are counted as diagnostics by reason, following the house rule that unknown symbols never become findings through a guessed relationship. + +Interface method calls are explicitly unresolved in this change. Devirtualization is a follow-up. + +## Finding Shape + +Findings stay anchored at the sink: the reported path and line are the sink's file and line, so a finding's location is where the dangerous call is. The rendered chain becomes file-qualified once it leaves the reporting file, so the message shows the route: + +``` +tainted data from http request query (handler.go line 4) reaches exec.Command (exec.go line 3) via run(arg0) -> exec.go:3 exec.Command +``` + +Cross-file findings carry `confidence: high`, matching the existing intra-file taint findings, because emission still requires a fully resolved chain. Deduplication keeps its current key of sink line, sink name, and taint source, scoped per reporting file. + +Fingerprints must not change for flows that are already detected intra-file, so an existing baseline entry for such a finding stays matched. Chain prose is excluded from fingerprint identity. + +## Cache Correctness + +`cachedFileFindings` keys entries on section, target, path, content hash, and section config hash. Cross-file taint breaks the assumption behind that key: a file's findings now depend on other files' contents, so an unchanged file can have a stale cached verdict after a helper changes. + +Taint scanning moves to its own section id so its entries can carry an extra dependency fingerprint: the hash of the file hashes of every file in the transitive package closure of the reporting file's package. A change to a helper invalidates exactly the files whose closure contains it, which keeps diff-mode cache hit rates high because a PR usually touches few packages. `scanCacheVersion` is bumped so existing caches are discarded rather than reused with the old key semantics. + +## Bounds and Degradation + +Whole-program analysis is not in scope, and the analysis must stay bounded: + +- maximum summary iterations per SCC +- maximum chain hop depth +- maximum indexed functions per target +- the existing corpus AST entry and byte budgets + +Exceeding any bound degrades to the current intra-file behavior for the affected target and records an informational diagnostic. Degradation must never emit a partial cross-file finding. + +Diff mode builds the summary index over the whole target, because a flow's source and sink can sit in unchanged files while the changed line is a hop in between; reporting stays scoped to changed lines as it is today. + +## Acceptance + +- The existing security corpus baseline holds: 68 true positives, 0 false negatives, at most 1 false positive (`docs/benchmarks.md`). +- New corpus fixtures cover source-to-helper-to-sink across files in the same package, across in-repo packages, through a package cycle, through a method, and through a sanitizer that neutralizes the flow (negative case). +- Unresolved-hop fixtures — interface dispatch, function-value parameter, call into an out-of-target package — produce no findings and increment the unresolved counters. +- Fingerprints for currently-detected intra-file flows are unchanged, proven by a baseline regression test. +- Cache test: changing a helper invalidates the caller's cached taint entry; changing an unrelated package does not. +- The self-scan (`make codeguard-ci`) gains no new findings. +- Full-scan wall clock growth from the pre-pass stays under 15% on the reference repositories. diff --git a/docs/superpowers/specs/2026-09-03-treesitter-scan-scale-design.md b/docs/superpowers/specs/2026-09-03-treesitter-scan-scale-design.md new file mode 100644 index 0000000..49fe5e4 --- /dev/null +++ b/docs/superpowers/specs/2026-09-03-treesitter-scan-scale-design.md @@ -0,0 +1,57 @@ +# Tree-Sitter Scan Scale Design + +## Scope + +Phase 2 of the tree-sitter migration (`docs/treesitter-spike.md` §9) shipped the parsing seam, four migrated TypeScript rules, and one migrated Python rule behind `parsers.treesitter: auto`. This change makes that path reachable on real repositories. It replaces the scan-lifetime tree retention model with a bounded resident cache plus heap-aware parse admission, and it makes every refusal of the tree path observable. + +It does not migrate additional rules, does not change any rule's semantics, does not flip the `parsers.treesitter` default, and does not alter behavior when the mode is `off`. + +## Problem + +`fileCorpus.parseScript` (`internal/codeguard/runner/support/corpus.go`) retains every parsed tree in `c.scripts` for the lifetime of the scan so that N rules across N sections pay for one parse. Retention makes peak memory a function of total scanned bytes, which forces the two scan-wide budgets that bound it: + +```go +const maxTreeSitterScanBytes = 256 * 1024 +const maxTreeSitterScanFiles = 64 +``` + +256 KiB is a whole-scan allowance, not a per-file one, so a repository is covered by the tree path only until the first few files exhaust it. Past that point `parseScript` returns a budget error, `support.ScriptSyntaxTree` returns nil, and each migrated rule silently takes its regex fallback — the path the spike measured at 60% / 54.5% precision against 100% / 100% for the tree path. Parsing is additionally serialized through a capacity-1 channel, so the tree path cannot use more than one core even within its budget. + +The consequence is that the precision win Phase 2 paid for is unrealized on any repository larger than a handful of script files, and nothing in the output says so. + +## Resident Cache Model + +Trees become a bounded resident cache rather than a scan-lifetime map. A tree is retained after parse so concurrent consumers in different sections share it; it is evicted when the resident budget is exceeded, least-recently-used first. A consumer that requests an evicted tree re-parses it. There is no per-scan file count or total-bytes ceiling: coverage is bounded by per-file size only. + +The resident budget is expressed in retained tree bytes, not source bytes, and its default is derived from a measurement task rather than assumed. Eviction accounting uses the measured retained cost of each tree so the ceiling means what it says. + +Correctness properties the cache must preserve from the current implementation: + +- Keying stays `path + language + content hash`, so diff-mode patched content parses separately from on-disk content. +- Concurrent callers racing a cold entry parse exactly once and observe the same tree. +- Returned trees stay immutable and safe for concurrent queries. +- A tree still in use by one section is never freed underneath it; eviction removes the cache's reference only, and the holder's reference keeps it alive. + +## Parse Admission + +Parse concurrency is governed by transient heap rather than a fixed count. Each admitted parse reserves `len(source) * treeSitterHeapFactor` bytes against a global in-flight ceiling, where the factor comes from the spike's measured ~0.5–0.6 MB of transient heap per KB of TypeScript. A parse waits until its reservation fits. Small files therefore parse concurrently up to a worker cap, while a single large file parses alone. + +The per-file refusal at `MaxTreeSitterFileBytes` (256 KiB, `internal/codeguard/checks/support/treeprovider_parse.go`) is unchanged and remains justified by the same heap factor. Raising it is out of scope. + +## Observability + +Every refusal of the tree path is counted per scan and attributed by reason: file oversize, parse failure, error-heavy tree, grammar not embedded in this build, and resident-cache re-parse. Counts are reported through the existing scan-diagnostic mechanism used by `fileCorpus.recordBudget`, aggregated by language, and are informational: they are not findings, are not baselinable, and do not affect exit status. + +This telemetry is the Phase 3 gate. Flipping the `parsers.treesitter` default requires evidence that tree coverage is near-total on reference repositories and that fallback is confined to files the tree path legitimately refuses. + +## Non-Goals + +Per-file batching of tree-consuming rules — one pass per file that runs every tree rule against one parse and then drops the tree — has a strictly better memory profile than a resident cache, but it requires restructuring how sections wire rules to files. It stays a documented follow-up, taken only if profiling shows re-parse thrash the LRU cannot absorb. The `ParserProvider` seam is unchanged either way. + +## Acceptance + +- With `parsers.treesitter: off`, findings are byte-identical to the current implementation on the corpus suite and the self-scan. +- With `parsers.treesitter: auto` on a reference TypeScript corpus of at least 200 script files, every file at or under 256 KiB is served by the tree path; the fallback counters attribute every remaining file to a per-file reason, with zero attributed to a scan-wide budget. +- Peak retained tree bytes stay under the configured ceiling under a synthetic corpus larger than that ceiling, asserted in the style of the existing `corpus_memory_test.go` budget tests. +- Full-scan wall clock with `auto` grows less than 2x against `off` on the reference repositories, the Phase 3 exit criterion from the spike. +- `go test ./...` passes with `-race`, since both section and per-file scanning are parallel. From 53c7e5d2cabb1d45213a9cb54d0619d92f42b009 Mon Sep 17 00:00:00 2001 From: Alex Wilkerson John Date: Thu, 3 Sep 2026 19:16:54 -0400 Subject: [PATCH 2/4] feat(config): add a minimum-confidence policy for findings Findings have always carried a confidence of high, medium, or low, set by 177 call sites, but nothing consumed it: no filtering, no ordering, no effect on status. Structural analyses (source-to-sink taint, the tree-sitter rule paths) set high while regex line scans leave it unspecified, so a team could not ask for only the trustworthy half without disabling rules outright or baselining the noise. checks.min_confidence.default drops findings below a level, and checks.min_confidence.sections.
overrides it per section. Omitting the block, or setting low, admits everything, so the default configuration is behavior-preserving. checks.confidence_demotion additionally reports a low-confidence failing finding as a warning; it never promotes. Filtering happens in FinalizeSectionWithDiagnostics, before waiver auditing, suppression matching, and status computation, and therefore after the per-file findings cache: changing a threshold re-renders a scan rather than re-running one. SectionConfigHashes strips both settings through findingRelevantChecks so they stay out of every fingerprint family, including the conservative all-checks fallback. Removed findings are never silent. Each is counted per rule as confidence_filtered, per section as confidence_filtered_count, and listed with reason "confidence" under --include-suppressed. Confidence is deliberately not folded into Suppressed() or suppression_ratio, which keep meaning "findings teams work around". Demotion leaves all three fingerprints untouched, so a demoted finding still matches its baseline entry. Section keys are the ids that appear in scan output and in checks.disabled. Those diverge from the runner's registry ids (supply_chain vs supply-chain), so core.SectionKeys() holds the report-facing list and a test asserts every section a real scan reports is a configurable key. Text output sorts by confidence within each rule group with a stable sort; section finding order is untouched, so JSON and SARIF keep exact scan order. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/knowledge/architecture-boundaries.md | 4 + .claude/knowledge/testing-patterns.md | 7 +- README.md | 2 +- docs/checks.md | 23 +++ docs/features.md | 42 +++++ internal/codeguard/config/validate.go | 22 +++ internal/codeguard/core/config_types.go | 37 ++++ internal/codeguard/core/finding_confidence.go | 33 ++++ .../core/report_artifact_rule_stats_types.go | 14 +- internal/codeguard/core/report_types.go | 5 + internal/codeguard/core/section_keys.go | 57 ++++++ internal/codeguard/report/text_helpers.go | 16 +- .../codeguard/runner/support/cache_helpers.go | 14 +- .../runner/support/findings_section.go | 32 ++++ .../codeguard/runner/support/rule_stats.go | 22 ++- .../codeguard/runner/support/suppressions.go | 4 + pkg/codeguard/sdk_types_config_root.go | 14 ++ .../confidence_policy_config_test.go | 154 +++++++++++++++ .../codeguard/confidence_policy_scan_test.go | 112 +++++++++++ tests/core/finding_confidence_test.go | 61 ++++++ tests/report/confidence_ordering_test.go | 153 +++++++++++++++ tests/support/confidence_cache_test.go | 55 ++++++ tests/support/confidence_demotion_test.go | 114 ++++++++++++ tests/support/confidence_filter_test.go | 175 ++++++++++++++++++ 24 files changed, 1159 insertions(+), 13 deletions(-) create mode 100644 internal/codeguard/core/section_keys.go create mode 100644 tests/codeguard/confidence_policy_config_test.go create mode 100644 tests/codeguard/confidence_policy_scan_test.go create mode 100644 tests/core/finding_confidence_test.go create mode 100644 tests/report/confidence_ordering_test.go create mode 100644 tests/support/confidence_cache_test.go create mode 100644 tests/support/confidence_demotion_test.go create mode 100644 tests/support/confidence_filter_test.go diff --git a/.claude/knowledge/architecture-boundaries.md b/.claude/knowledge/architecture-boundaries.md index 427f786..50c00ce 100644 --- a/.claude/knowledge/architecture-boundaries.md +++ b/.claude/knowledge/architecture-boundaries.md @@ -33,3 +33,7 @@ Key architectural decisions, service boundaries, data flow, integration points, - **PERF: the quality AI checks read through the corpus via `checks/support.Context.ListTargetFiles`/`ReadTargetFile`** (wired in `runner/checks/checks.go`, backed by `runnersupport.ListTargetFiles`/`ReadTargetFile`; `checks/quality/quality_ai_corpus.go` holds the nil-safe fallbacks for unit-test contexts). GOTCHA when routing more reads through the corpus: `corpusRead` caps at 32 MiB (`readCappedFile`) while `os.ReadFile` does not — walk-enumerated files are already under the cap so behavior is identical, but fixed-filename reads that never went through the walk (`go.mod`, root `package.json`, python manifests) deliberately stay direct `os.ReadFile` to preserve semantics. The Go repo-dominant style signals (test framework, error style, naming) are computed in ONE corpus pass (`goRepoStyleProfile` in `quality_ai_target_go.go`); each signal is an independent per-file sum, which is what makes the fold behavior-identical. - **GOTCHA: `goFuncEligibleForUnusedCheck` must not consult `fn.Doc`.** The AI dead-code pass historically parsed with `parser.ParseFile(..., 0)`, so `fn.Doc` was always nil and the "go: directive implies external use" exemption NEVER fired. The pass now reuses the shared ParseComments corpus AST (`support.ParseGoSource`), where `fn.Doc` IS populated — keeping that check would have silently exempted unused functions carrying `//go:generate`-style comments and changed findings. It was removed to keep output identical (see the NOTE in `quality_ai_dead_code.go`); activating it is a candidate behavior fix, to be made deliberately with tests. - **PERF: clone-detector hashing is not behavior-bearing.** `cloneToken` now stores the original-case source slice plus a per-token FNV-1a hash of the ASCII-lowercased text (tokens are ASCII-only by the regex); window grouping uses a rolling polynomial hash (`cloneWindowIndex`) and equality is verified token-by-token in `sharedCloneLength` (hash short-circuit + ASCII case-fold compare). Because every candidate is value-verified, ANY deterministic hash yields identical clone candidates — collisions only cost time. `tests/checks/quality_clone_differential_test.go` pins findings captured from the pre-rewrite algorithm; don't regenerate its expectations with the current code. + +- **Finding identity excludes presentation.** Exact, context, and content fingerprints derive from rule ID, path, line, and source identity only (`runner/support/findings_section.go`) — never message prose, metadata, confidence, or the *reported* level. Confidence demotion (`checks.confidence_demotion`) rewrites `Level`/`Severity` after `NewFinding`, so a demoted finding still matches its baseline entry. Any future policy that changes how a finding is reported must preserve this. +- **Post-cache policy must be stripped from cache fingerprints.** `SectionConfigHashes` calls `findingRelevantChecks` to zero settings that cannot change per-file findings (currently `checks.min_confidence` and `checks.confidence_demotion`), including for the conservative `""` all-checks family. Filtering happens in `FinalizeSectionWithDiagnostics`, i.e. after `cachedFileFindings`, so a threshold change must re-render a scan, never invalidate cached findings. Add any new finalize-time-only setting to that strip list. +- **Section ids diverge between the registry and reports.** `runner/checks/registry.go` uses `supply-chain`, but the section finalizes as `supply_chain`, which is what appears in `SectionResult.ID`, in `checks.disabled`, and in `checks.min_confidence.sections`. `core.SectionKeys()` holds the report-facing list; `tests/codeguard` asserts every id a real scan reports is in it. diff --git a/.claude/knowledge/testing-patterns.md b/.claude/knowledge/testing-patterns.md index 41b28ec..76ded29 100644 --- a/.claude/knowledge/testing-patterns.md +++ b/.claude/knowledge/testing-patterns.md @@ -4,7 +4,7 @@ Testing strategies, test infrastructure quirks, how to run/debug specific test s - The trust policy (`internal/codeguard/trust`) is a process-global. Tests that exercise command-driven checks or local (127.0.0.1) AI endpoints must enable it. `tests/checks` and `tests/codeguard` each have a `TestMain` (`trust_main_test.go`) that sets `Policy{AllowConfigCommands: true, AllowConfigAIEndpoints: true}`. If you add a new test package that runs config commands or hits a loopback test server, add a similar `TestMain` or the tests will fail with "refusing to run config-supplied command" / "ssrf guard" errors. - `tests/security` deliberately has NO trust `TestMain`: it verifies the secure *default*. Tests there set/restore the policy locally with `trust.Set(...)` + `t.Cleanup(trust.ResetFromEnv)`. Do not add a package-wide opt-in there. -- All tests live under `tests/` as external (`_test`) packages; there are no white-box tests inside `internal/`/`pkg/`. Internal packages are still importable from `tests/` because they are within the module. This is enforced by `ci.test-file-location` (the repo's own scan fails on `_test.go` files outside `tests/**`), so do not add tests next to the code even for unexported access — import the internal package from `tests/` and test via its exported API instead. +- New tests go under `tests/` as external (`_test`) packages. A small number of legacy in-package tests do exist (e.g. `internal/codeguard/runner/support/corpus_memory_test.go`, `internal/codeguard/runner/pr_summary_test.go`, `internal/codeguard/config/profile_test.go`) and are permitted because `ci_rules.allowed_test_paths` in `.codeguard/codeguard.yaml` lists `internal/**` and `cmd/**` alongside `tests/**` — so the self-scan will NOT catch a new in-package test. The `tests/`-only rule is convention enforced by review, not by the scan; extend those existing files when you need unexported access, don't add new ones. Internal packages are still importable from `tests/` because they are within the module. This is enforced by `ci.test-file-location` (the repo's own scan fails on `_test.go` files outside `tests/**`), so do not add tests next to the code even for unexported access — import the internal package from `tests/` and test via its exported API instead. - **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///{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). @@ -14,3 +14,8 @@ Testing strategies, test infrastructure quirks, how to run/debug specific test s - **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. - Defensive precision positive fixtures should avoid UI-ish names such as `render*` unless the test is explicitly covering UI suppression. The defensive boundary/null rules intentionally skip React/UI helper contexts, so a fixture named like a renderer can stop emitting the server-side defensive finding the test expects. - Precision-rule retunes should include both the false-positive fixture and a nearby positive that still fires. Dogfood the local binary against a real target repo before committing; several naming/mutation rules only showed meaningful movement after scanning a mixed React, route, and integration-heavy TypeScript monorepo. +- **`go test ./...` is red on `main` (as of 1205484).** `TestSecuritySemanticAnalyzerScansNodeModulesWithinTarget` (`tests/checks/typescript_semantic_test.go:182`) fails with `Security status = "pass", want "fail"`. Its `requireTypeScriptSemanticRuntime` guard does *not* skip, so this is a real failure, not a missing-runtime skip; it plausibly relates to the vendored/`node_modules` scan bounding in 24cb12c but has not been diagnosed. Reproduce a baseline in a clean worktree (`git worktree add --detach HEAD`) before attributing a `tests/checks` failure to your own change. +- **Local strict lint cannot typecheck the current Go stdlib.** `golangci-lint run` reports 2 `typecheck` issues inside `crypto/internal/randutil` / `math/rand/v2` because its bundled Go is older than the Homebrew toolchain (1.27.1). These are stdlib paths, not repo code — check that no reported path is under `cmd/`, `internal/`, `pkg/`, or `tests/` and rely on CI's pinned version for the real gate. +- **Report rendering is tested through the SDK.** `internal/codeguard/report` has no external test hooks, so `tests/report` drives it via `codeguard.WriteReport(w, report, format)` with a hand-built `codeguard.Report`. Note the text writer emits a large braille/ANSI banner, so assert on substring *positions* rather than diffing whole output. `tests/core` and `tests/report` were added as external packages for `core` and `report` respectively. +- **Verifying a self-scan didn't add findings:** `make codeguard-ci` prints only a summary count, so compare against a clean worktree at HEAD and diff the `N. at:` location lines (`grep -E '^\s+[0-9]+\. at:' | sed 's/^ *[0-9]*\. at: //' | sort`). The repo's own `quality.ai.narrative-comment` rule fires on doc comments that describe what code is rather than why it exists, so new comments can add findings. + diff --git a/README.md b/README.md index ec934bb..61ef732 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ `codeguard` is a standalone Go service and CLI for repository checks across code quality, production reliability, data correctness, design boundaries, security, CI/CD hygiene, AI prompt governance, and repo-specific policy rules. -It now supports repository exclusions, baselines, waivers, changed-lines diff scans, SARIF output, GitHub annotations, custom rule packs, natural-language custom rules through an optional AI runtime, policy profiles, scan caching, doctor checks, rule discovery from the CLI, native TypeScript/Python quality, design, security, reliability, and data-correctness heuristics, and language-specific command checks. +It now supports repository exclusions, baselines, waivers, changed-lines diff scans, SARIF output, GitHub annotations, custom rule packs, natural-language custom rules through an optional AI runtime, policy profiles, per-section minimum-confidence policy, scan caching, doctor checks, rule discovery from the CLI, native TypeScript/Python quality, design, security, reliability, and data-correctness heuristics, and language-specific command checks. For a user-facing glossary of every check family and subsection, see [docs/checks.md](docs/checks.md). diff --git a/docs/checks.md b/docs/checks.md index 1d67a45..4361a1a 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -426,6 +426,29 @@ Tree-sitter parsing (opt-in): - oversized files (> 256 KiB), parse failures, and error-heavy trees fall back to the regex path per file +Confidence policy (opt-in): +- every finding carries a confidence of `high`, `medium`, or `low`; an + unspecified confidence is treated as `medium`. Structural analyses set + `high` (source-to-sink taint, tree-sitter rule paths), while regex line + scans generally leave it unspecified +- `checks.min_confidence.default` drops findings below the given level, and + `checks.min_confidence.sections.
` overrides it for one section. + Section keys are the ids reported in scan output and accepted by + `checks.disabled` (supply chain is `supply_chain`). Omitting the block, or + setting `low`, admits every finding — the default behavior +- removed findings are never silent: each is counted per rule as + `confidence_filtered` in the `rule_stats` artifact, reported per section as + `confidence_filtered_count`, and listed with reason `confidence` under + `--include-suppressed`. They are excluded from `suppression_ratio`, which + keeps meaning "findings teams work around" +- `checks.confidence_demotion: true` reports a `low`-confidence finding on a + failing rule as a warning instead. It never promotes and never applies to + `medium` or `high`. Off by default +- filtering and demotion are applied when a section is finalized, after the + per-file findings cache, so changing either setting re-renders a scan + instead of re-running one, and neither affects a finding's fingerprint or + its baseline match + ## Quality Purpose: diff --git a/docs/features.md b/docs/features.md index 30ae1c0..74dfcb4 100644 --- a/docs/features.md +++ b/docs/features.md @@ -215,6 +215,48 @@ JSON: } ``` +### Minimum confidence policy + +Drop findings the scanner is not confident enough about, globally or per +section, and optionally report low-confidence failures as warnings. Omitting +the block admits every finding, which is the default behavior. Section keys are +the ids reported in scan output (the same names `checks.disabled` accepts). + +Both settings are applied when a section is finalized, after the per-file +findings cache, so changing a threshold re-renders a scan rather than +re-running one, and neither changes a finding's fingerprint or its baseline +match. Every removed finding is counted per rule as `confidence_filtered` and +per section as `confidence_filtered_count`. + +YAML: + +```yaml +checks: + min_confidence: + default: medium + sections: + security: high + quality: low + confidence_demotion: true +``` + +JSON: + +```json +{ + "checks": { + "min_confidence": { + "default": "medium", + "sections": { + "security": "high", + "quality": "low" + } + }, + "confidence_demotion": true + } +} +``` + ### Enable AI change risk YAML: diff --git a/internal/codeguard/config/validate.go b/internal/codeguard/config/validate.go index 9e0066b..c2d2a3a 100644 --- a/internal/codeguard/config/validate.go +++ b/internal/codeguard/config/validate.go @@ -44,6 +44,7 @@ func Validate(cfg core.Config) error { validatePerformanceRules(cfg.Checks.PerformanceRules), validateSecretsRules(cfg.Checks.SecurityRules.Secrets), validateParsers(cfg.Parsers), + validateConfidencePolicy(cfg.Checks.MinConfidence), validateRulePacks(cfg.RulePacks), validateExternalReports(cfg.ExternalReports), ) @@ -141,6 +142,27 @@ func validateParsers(parsers core.ParsersConfig) error { } } +// validateConfidencePolicy rejects unknown confidence levels and unknown +// section keys. An empty level is "unspecified" and stays valid, so a config may +// name a section without pinning it. +func validateConfidencePolicy(policy core.ConfidencePolicyConfig) error { + if strings.TrimSpace(policy.Default) != "" && core.NormalizedConfidence(policy.Default) == "" { + return fmt.Errorf("checks.min_confidence.default must be %q, %q, or %q", + core.ConfidenceLow, core.ConfidenceMedium, core.ConfidenceHigh) + } + for section, level := range policy.Sections { + if !core.KnownSectionKey(section) { + return fmt.Errorf("checks.min_confidence.sections has unknown section %q; known sections are %s", + section, strings.Join(core.SectionKeys(), ", ")) + } + if strings.TrimSpace(level) != "" && core.NormalizedConfidence(level) == "" { + return fmt.Errorf("checks.min_confidence.sections[%q] must be %q, %q, or %q", + section, core.ConfidenceLow, core.ConfidenceMedium, core.ConfidenceHigh) + } + } + return nil +} + func validateNameAndProfile(cfg core.Config) error { if strings.TrimSpace(cfg.Name) == "" { return errors.New("config name is required") diff --git a/internal/codeguard/core/config_types.go b/internal/codeguard/core/config_types.go index f093a65..5e2c13f 100644 --- a/internal/codeguard/core/config_types.go +++ b/internal/codeguard/core/config_types.go @@ -132,6 +132,43 @@ type CheckConfig struct { ContractRules ContractRulesConfig `json:"contract_rules" yaml:"contract_rules"` ContextRules ContextRulesConfig `json:"context_rules" yaml:"context_rules"` ProductionRisk ProductionRiskConfig `json:"production_risk,omitempty" yaml:"production_risk,omitempty"` + // MinConfidence drops findings whose confidence sits below the configured + // level, globally or per section. Omitted (or "low") admits every finding, + // which is the historical behavior. Filtering happens when a section is + // finalized, after the per-file findings cache, so changing a threshold + // re-renders a scan rather than re-running one. + MinConfidence ConfidencePolicyConfig `json:"min_confidence,omitempty" yaml:"min_confidence,omitempty"` + // ConfidenceDemotion reports a low-confidence finding on a failing rule as + // a warning instead. It never promotes, and never applies to medium or + // high confidence. Off by default. + ConfidenceDemotion bool `json:"confidence_demotion,omitempty" yaml:"confidence_demotion,omitempty"` +} + +// ConfidencePolicyConfig is the minimum-confidence policy: one default plus +// optional per-section overrides keyed by section id. +type ConfidencePolicyConfig struct { + Default string `json:"default,omitempty" yaml:"default,omitempty"` + Sections map[string]string `json:"sections,omitempty" yaml:"sections,omitempty"` +} + +// Threshold resolves the minimum confidence for a section: its own override +// when present, otherwise the policy default, otherwise ConfidenceLow, which +// admits every finding. Levels and section keys are normalized, so casing and +// surrounding space in a config file do not silently disable the policy. +func (c ConfidencePolicyConfig) Threshold(sectionID string) string { + section := NormalizedSectionKey(sectionID) + for key, level := range c.Sections { + if NormalizedSectionKey(key) != section { + continue + } + if normalized := NormalizedConfidence(level); normalized != "" { + return normalized + } + } + if normalized := NormalizedConfidence(c.Default); normalized != "" { + return normalized + } + return ConfidenceLow } type OutputConfig struct { diff --git a/internal/codeguard/core/finding_confidence.go b/internal/codeguard/core/finding_confidence.go index 48b46c7..0ad0692 100644 --- a/internal/codeguard/core/finding_confidence.go +++ b/internal/codeguard/core/finding_confidence.go @@ -26,3 +26,36 @@ func NormalizedConfidence(value string) string { return "" } } + +// Confidence ranks, ordered so a higher rank is more trustworthy. Unspecified +// confidence ranks as medium, matching how consumers are documented to treat +// an empty value. +const ( + confidenceRankLow = iota + 1 + confidenceRankMedium + confidenceRankHigh +) + +// ConfidenceRank maps a confidence value onto its comparable rank. Empty and +// unrecognized values rank as medium. +func ConfidenceRank(value string) int { + switch NormalizedConfidence(value) { + case ConfidenceHigh: + return confidenceRankHigh + case ConfidenceLow: + return confidenceRankLow + default: + return confidenceRankMedium + } +} + +// MeetsConfidence reports whether a finding's confidence satisfies a minimum +// threshold. An empty or unrecognized threshold admits every finding, so an +// unconfigured policy cannot filter anything. +func MeetsConfidence(confidence string, threshold string) bool { + normalized := NormalizedConfidence(threshold) + if normalized == "" { + return true + } + return ConfidenceRank(confidence) >= ConfidenceRank(normalized) +} diff --git a/internal/codeguard/core/report_artifact_rule_stats_types.go b/internal/codeguard/core/report_artifact_rule_stats_types.go index b1949bc..2234cca 100644 --- a/internal/codeguard/core/report_artifact_rule_stats_types.go +++ b/internal/codeguard/core/report_artifact_rule_stats_types.go @@ -14,11 +14,15 @@ type RuleStatsArtifact struct { // SuppressionRatio is suppressed/(emitted+suppressed); a persistently high // ratio signals a rule teams work around rather than act on. type RuleStatsEntry struct { - RuleID string `json:"rule_id"` - Emitted int `json:"emitted"` - BaselineSuppressed int `json:"baseline_suppressed"` - WaiverSuppressed int `json:"waiver_suppressed"` - InlineSuppressed int `json:"inline_suppressed"` + RuleID string `json:"rule_id"` + Emitted int `json:"emitted"` + BaselineSuppressed int `json:"baseline_suppressed"` + WaiverSuppressed int `json:"waiver_suppressed"` + InlineSuppressed int `json:"inline_suppressed"` + // ConfidenceFiltered counts findings removed by the minimum-confidence + // policy. It is excluded from Suppressed and from SuppressionRatio so the + // ratio keeps meaning "findings teams work around". + ConfidenceFiltered int `json:"confidence_filtered,omitempty"` SuppressionRatio float64 `json:"suppression_ratio"` } diff --git a/internal/codeguard/core/report_types.go b/internal/codeguard/core/report_types.go index 163786d..0596a8a 100644 --- a/internal/codeguard/core/report_types.go +++ b/internal/codeguard/core/report_types.go @@ -47,6 +47,11 @@ type SectionResult struct { Findings []Finding `json:"findings"` Diagnostics []Diagnostic `json:"diagnostics,omitempty"` SuppressedCount int `json:"suppressed_count,omitempty"` + // ConfidenceFilteredCount counts findings removed by the + // checks.min_confidence policy. It is deliberately separate from + // SuppressedCount: a suppression records a team accepting a finding, while + // a confidence filter records the scanner not being sure enough to show it. + ConfidenceFilteredCount int `json:"confidence_filtered_count,omitempty"` } // Diagnostic describes scanner operation or informational classification. It diff --git a/internal/codeguard/core/section_keys.go b/internal/codeguard/core/section_keys.go new file mode 100644 index 0000000..2a51310 --- /dev/null +++ b/internal/codeguard/core/section_keys.go @@ -0,0 +1,57 @@ +package core + +import ( + "sort" + "strings" +) + +// Section ids as they appear in scan output (SectionResult.ID) and in +// per-section configuration such as checks.disabled and +// ConfidencePolicyConfig.Sections. +// +// These are the ids sections finalize with, which is what users see in a +// report — note that supply chain finalizes as "supply_chain" while the +// runner's internal registry id is "supply-chain". tests/codeguard asserts +// that every section a real scan reports is present here, so a new section +// cannot silently become unconfigurable. +var sectionKeys = map[string]struct{}{ + "change": {}, + "quality": {}, + "performance": {}, + "reliability": {}, + "data": {}, + "observability": {}, + "operations": {}, + "design": {}, + "security": {}, + "prompts": {}, + "ci": {}, + "delivery": {}, + "supply_chain": {}, + "context": {}, + "contracts": {}, + "custom": {}, +} + +// NormalizedSectionKey trims and lowercases a section id so configuration keys +// match the runner's ids regardless of casing or surrounding space. +func NormalizedSectionKey(sectionID string) string { + return strings.TrimSpace(strings.ToLower(sectionID)) +} + +// KnownSectionKey reports whether sectionID names a registered check section. +func KnownSectionKey(sectionID string) bool { + _, ok := sectionKeys[NormalizedSectionKey(sectionID)] + return ok +} + +// SectionKeys returns the registered section ids in sorted order, for error +// messages and documentation. +func SectionKeys() []string { + keys := make([]string, 0, len(sectionKeys)) + for key := range sectionKeys { + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} diff --git a/internal/codeguard/report/text_helpers.go b/internal/codeguard/report/text_helpers.go index 21a505a..0226ec2 100644 --- a/internal/codeguard/report/text_helpers.go +++ b/internal/codeguard/report/text_helpers.go @@ -3,6 +3,7 @@ package report import ( "fmt" "io" + "sort" "strconv" "strings" @@ -83,6 +84,11 @@ func writeTextSection(w io.Writer, section core.SectionResult) error { return err } } + if section.ConfidenceFilteredCount > 0 { + if _, err := fmt.Fprintf(w, "\n confidence filtered: %d\n", section.ConfidenceFilteredCount); err != nil { + return err + } + } return nil } @@ -103,9 +109,17 @@ func groupTextFindings(findings []core.Finding) []textFindingGroup { } out := make([]textFindingGroup, 0, len(order)) for _, name := range order { + grouped := groups[name] + // Presentation only: the most trustworthy findings in a group are read + // first. The sort is stable, so equal-confidence findings keep scan + // order, and section.Findings itself is never reordered — machine + // surfaces such as JSON and SARIF keep exact scan order. + sort.SliceStable(grouped, func(i, j int) bool { + return core.ConfidenceRank(grouped[i].Confidence) > core.ConfidenceRank(grouped[j].Confidence) + }) out = append(out, textFindingGroup{ name: name, - findings: groups[name], + findings: grouped, }) } return out diff --git a/internal/codeguard/runner/support/cache_helpers.go b/internal/codeguard/runner/support/cache_helpers.go index 6cf0b4b..8c5a167 100644 --- a/internal/codeguard/runner/support/cache_helpers.go +++ b/internal/codeguard/runner/support/cache_helpers.go @@ -43,7 +43,7 @@ func SectionConfigHashes(cfg core.Config, catalog map[string]core.RuleMetadata, // v4: findings gained ContentFingerprint, so cached per-file findings need // to be regenerated with the path-insensitive fingerprint. prefix := "section-config-v4|" + strings.Join(extras, "|") + "|" - checks := cfg.Checks + checks := findingRelevantChecks(cfg.Checks) return map[string]string{ // quality reads both QualityRules and DesignRules, and its AI-quality // findings depend on the AI config. quality, performance, and security @@ -60,6 +60,18 @@ func SectionConfigHashes(cfg core.Config, catalog map[string]core.RuleMetadata, } } +// findingRelevantChecks strips settings that cannot change any per-file +// finding, so they never invalidate cached findings. The minimum-confidence +// policy and confidence demotion are applied when a section is finalized — +// after the cache — so a threshold change re-renders a scan rather than +// re-running one, including for sections that fall back to the all-checks +// fingerprint. +func findingRelevantChecks(checks core.CheckConfig) core.CheckConfig { + checks.MinConfidence = core.ConfidencePolicyConfig{} + checks.ConfidenceDemotion = false + return checks +} + // sectionConfigFamily maps a per-file cache section id to the config family // whose settings can change that section's findings. Compound ids share the // family of their prefix (e.g. "quality-clone" and "security-secrets"); any diff --git a/internal/codeguard/runner/support/findings_section.go b/internal/codeguard/runner/support/findings_section.go index 25583bc..ca28e1d 100644 --- a/internal/codeguard/runner/support/findings_section.go +++ b/internal/codeguard/runner/support/findings_section.go @@ -108,6 +108,21 @@ func FinalizeSectionWithDiagnostics(sc Context, id string, name string, findings if sc.Opts.Mode == core.ScanModeDiff && finding.Path != "" && !matchesDiff(sc, finding) { continue } + // The confidence policy decides whether a finding is shown at all, so it + // runs before waiver auditing and suppression matching: a finding the + // scanner is not confident enough to report is not a finding a team + // waived or baselined. + if !core.MeetsConfidence(finding.Confidence, sc.Cfg.Checks.MinConfidence.Threshold(id)) { + section.ConfidenceFilteredCount++ + sc.RuleStats.RecordConfidenceFiltered(finding.RuleID) + if sc.Opts.IncludeSuppressed { + finding.Suppressed = true + finding.SuppressionReason = SuppressionReasonConfidence + finding.Suppression = &core.Suppression{Kind: SuppressionReasonConfidence} + sc.Suppressed.Add(finding) + } + continue + } sc.WaiverAudit.RecordMatches(MatchingWaivers(sc, finding), finding) if suppression := MatchSuppression(sc, finding); suppression != nil { section.SuppressedCount++ @@ -121,6 +136,7 @@ func FinalizeSectionWithDiagnostics(sc Context, id string, name string, findings continue } sc.RuleStats.RecordEmitted(finding.RuleID) + finding = demoteLowConfidence(sc, finding) active = append(active, finding) switch finding.Level { case "fail": @@ -152,6 +168,22 @@ func FinalizeSectionWithDiagnostics(sc Context, id string, name string, findings return section } +// demoteLowConfidence reports a low-confidence failing finding as a warning +// when checks.confidence_demotion is on. It only ever lowers a level, and it +// leaves finding identity untouched: fingerprints derive from rule, path, and +// source, so a demoted finding still matches its baseline entry. +func demoteLowConfidence(sc Context, finding core.Finding) core.Finding { + if !sc.Cfg.Checks.ConfidenceDemotion { + return finding + } + if finding.Confidence != core.ConfidenceLow || finding.Level != "fail" { + return finding + } + finding.Level = "warn" + finding.Severity = "warn" + return finding +} + func matchesDiff(sc Context, finding core.Finding) bool { scope, ok := sc.Diff[finding.Path] if !ok { diff --git a/internal/codeguard/runner/support/rule_stats.go b/internal/codeguard/runner/support/rule_stats.go index dedc38f..e695eb5 100644 --- a/internal/codeguard/runner/support/rule_stats.go +++ b/internal/codeguard/runner/support/rule_stats.go @@ -17,10 +17,11 @@ type RuleStatsCollector struct { } type ruleTally struct { - emitted int - baseline int - waiver int - inline int + emitted int + baseline int + waiver int + inline int + confidence int } func NewRuleStatsCollector() *RuleStatsCollector { @@ -37,6 +38,18 @@ func (collector *RuleStatsCollector) RecordEmitted(ruleID string) { collector.lockedTally(ruleID).emitted++ } +// RecordConfidenceFiltered counts one finding removed from ruleID by the +// minimum-confidence policy. It is tracked apart from the suppression +// mechanisms so rule health can tell "not shown" from "worked around". +func (collector *RuleStatsCollector) RecordConfidenceFiltered(ruleID string) { + if collector == nil || ruleID == "" { + return + } + collector.mu.Lock() + defer collector.mu.Unlock() + collector.lockedTally(ruleID).confidence++ +} + // RecordSuppressed counts one suppressed finding for ruleID, attributed by the // reason string returned from IsSuppressed. func (collector *RuleStatsCollector) RecordSuppressed(ruleID string, reason string) { @@ -92,6 +105,7 @@ func newRuleStatsEntry(ruleID string, tally ruleTally) core.RuleStatsEntry { BaselineSuppressed: tally.baseline, WaiverSuppressed: tally.waiver, InlineSuppressed: tally.inline, + ConfidenceFiltered: tally.confidence, } if total := entry.Emitted + entry.Suppressed(); total > 0 { entry.SuppressionRatio = float64(entry.Suppressed()) / float64(total) diff --git a/internal/codeguard/runner/support/suppressions.go b/internal/codeguard/runner/support/suppressions.go index 183a1a7..e9955d1 100644 --- a/internal/codeguard/runner/support/suppressions.go +++ b/internal/codeguard/runner/support/suppressions.go @@ -24,6 +24,10 @@ const ( SuppressionReasonBaseline = "baseline" SuppressionReasonWaiver = "waiver" SuppressionReasonInline = "inline suppression" + // SuppressionReasonConfidence labels findings removed by the + // checks.min_confidence policy. It is reported as its own mechanism and is + // never counted as a suppression. + SuppressionReasonConfidence = "confidence" ) func IsSuppressed(sc Context, finding core.Finding) (bool, string) { diff --git a/pkg/codeguard/sdk_types_config_root.go b/pkg/codeguard/sdk_types_config_root.go index e73c865..808bd1f 100644 --- a/pkg/codeguard/sdk_types_config_root.go +++ b/pkg/codeguard/sdk_types_config_root.go @@ -13,4 +13,18 @@ type TargetConfig = core.TargetConfig type CheckConfig = core.CheckConfig type ParsersConfig = core.ParsersConfig + +// ConfidencePolicyConfig is the minimum-confidence policy applied to findings, +// with one default level and optional per-section overrides. +type ConfidencePolicyConfig = core.ConfidencePolicyConfig + +// Confidence levels a finding can carry, and the values accepted by +// checks.min_confidence. An empty confidence must be read as medium rather than +// as a weak finding: it means the check did not state one. +const ( + ConfidenceHigh = core.ConfidenceHigh + ConfidenceMedium = core.ConfidenceMedium + ConfidenceLow = core.ConfidenceLow +) + type ExternalReportConfig = core.ExternalReportConfig diff --git a/tests/codeguard/confidence_policy_config_test.go b/tests/codeguard/confidence_policy_config_test.go new file mode 100644 index 0000000..0646e5d --- /dev/null +++ b/tests/codeguard/confidence_policy_config_test.go @@ -0,0 +1,154 @@ +package codeguard_test + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/devr-tools/codeguard/pkg/codeguard" +) + +// A configuration that says nothing about confidence must behave exactly as it +// does today: every finding is admitted and nothing is demoted. +func TestConfidencePolicyDefaultsToPermissive(t *testing.T) { + var policy codeguard.ConfidencePolicyConfig + if got := policy.Threshold("security"); got != codeguard.ConfidenceLow { + t.Fatalf("zero-value threshold = %q, want %q", got, codeguard.ConfidenceLow) + } + + cfg := codeguard.ExampleConfig() + if got := cfg.Checks.MinConfidence.Threshold("security"); got != codeguard.ConfidenceLow { + t.Fatalf("example config threshold = %q, want %q", got, codeguard.ConfidenceLow) + } + if cfg.Checks.ConfidenceDemotion { + t.Fatal("confidence_demotion must default to off") + } +} + +func TestConfidencePolicyThresholdPrecedence(t *testing.T) { + policy := codeguard.ConfidencePolicyConfig{ + Default: codeguard.ConfidenceMedium, + Sections: map[string]string{"security": codeguard.ConfidenceHigh}, + } + tests := []struct { + name string + section string + want string + }{ + {name: "section override wins", section: "security", want: codeguard.ConfidenceHigh}, + {name: "unlisted section falls back to default", section: "quality", want: codeguard.ConfidenceMedium}, + {name: "empty section falls back to default", section: "", want: codeguard.ConfidenceMedium}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := policy.Threshold(tt.section); got != tt.want { + t.Fatalf("Threshold(%q) = %q, want %q", tt.section, got, tt.want) + } + }) + } +} + +// Levels and section keys are normalized the way the rest of the config is, so +// a stray capital or space is not a silent misconfiguration. +func TestConfidencePolicyThresholdNormalizesInput(t *testing.T) { + policy := codeguard.ConfidencePolicyConfig{ + Default: " HIGH ", + Sections: map[string]string{" Security ": " medium "}, + } + if got := policy.Threshold("security"); got != codeguard.ConfidenceMedium { + t.Fatalf("Threshold(security) = %q, want %q", got, codeguard.ConfidenceMedium) + } + if got := policy.Threshold("quality"); got != codeguard.ConfidenceHigh { + t.Fatalf("Threshold(quality) = %q, want %q", got, codeguard.ConfidenceHigh) + } +} + +func TestValidateConfidencePolicyRejectsUnknownValues(t *testing.T) { + tests := []struct { + name string + policy codeguard.ConfidencePolicyConfig + want string + }{ + { + name: "unknown default level", + policy: codeguard.ConfidencePolicyConfig{Default: "sideways"}, + want: "checks.min_confidence.default", + }, + { + name: "unknown section level", + policy: codeguard.ConfidencePolicyConfig{Sections: map[string]string{"security": "certain"}}, + want: "checks.min_confidence.sections", + }, + { + name: "unknown section key", + policy: codeguard.ConfidencePolicyConfig{Sections: map[string]string{"securty": codeguard.ConfidenceHigh}}, + want: "unknown section", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateConfidencePolicy(tt.policy) + if err == nil || !strings.Contains(err.Error(), tt.want) { + t.Fatalf("error = %v, want containing %q", err, tt.want) + } + }) + } +} + +func TestValidateConfidencePolicyAcceptsCompletePolicy(t *testing.T) { + policy := codeguard.ConfidencePolicyConfig{ + Default: codeguard.ConfidenceMedium, + Sections: map[string]string{ + "security": codeguard.ConfidenceHigh, + "quality": codeguard.ConfidenceLow, + "supply_chain": codeguard.ConfidenceMedium, + }, + } + if err := validateConfidencePolicy(policy); err != nil { + t.Fatalf("validate complete policy: %v", err) + } +} + +// An empty level anywhere means "unspecified" and must stay valid, so a config +// can name a section without pinning it. +func TestValidateConfidencePolicyAcceptsEmptyLevels(t *testing.T) { + policy := codeguard.ConfidencePolicyConfig{ + Sections: map[string]string{"security": ""}, + } + if err := validateConfidencePolicy(policy); err != nil { + t.Fatalf("validate empty levels: %v", err) + } +} + +func TestConfidencePolicyYAMLRoundTrip(t *testing.T) { + cfg := codeguard.ExampleConfig() + cfg.Checks.MinConfidence = codeguard.ConfidencePolicyConfig{ + Default: codeguard.ConfidenceMedium, + Sections: map[string]string{"security": codeguard.ConfidenceHigh}, + } + cfg.Checks.ConfidenceDemotion = true + + path := filepath.Join(t.TempDir(), "codeguard.yaml") + if err := codeguard.WriteConfigFile(path, cfg); err != nil { + t.Fatalf("write config: %v", err) + } + loaded, err := codeguard.LoadConfigFile(path) + if err != nil { + t.Fatalf("load config: %v", err) + } + if got := loaded.Checks.MinConfidence.Threshold("security"); got != codeguard.ConfidenceHigh { + t.Fatalf("loaded security threshold = %q, want %q", got, codeguard.ConfidenceHigh) + } + if got := loaded.Checks.MinConfidence.Threshold("quality"); got != codeguard.ConfidenceMedium { + t.Fatalf("loaded quality threshold = %q, want %q", got, codeguard.ConfidenceMedium) + } + if !loaded.Checks.ConfidenceDemotion { + t.Fatal("loaded confidence_demotion = false, want true") + } +} + +func validateConfidencePolicy(policy codeguard.ConfidencePolicyConfig) error { + cfg := codeguard.ExampleConfig() + cfg.Checks.MinConfidence = policy + return codeguard.ValidateConfig(cfg) +} diff --git a/tests/codeguard/confidence_policy_scan_test.go b/tests/codeguard/confidence_policy_scan_test.go new file mode 100644 index 0000000..bc205df --- /dev/null +++ b/tests/codeguard/confidence_policy_scan_test.go @@ -0,0 +1,112 @@ +package codeguard_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + + "github.com/devr-tools/codeguard/internal/codeguard/core" + "github.com/devr-tools/codeguard/pkg/codeguard" +) + +func confidenceScanConfig(t testing.TB) codeguard.Config { + t.Helper() + dir := t.TempDir() + for i := 0; i < 6; i++ { + var src strings.Builder + src.WriteString("package repo\n\n") + src.WriteString(fmt.Sprintf("func Long%02d() int {\n", i)) + for line := 0; line < 12; line++ { + src.WriteString(fmt.Sprintf("\tv%d := %d\n\t_ = v%d\n", line, line, line)) + } + src.WriteString("\treturn 0\n}\n") + path := filepath.Join(dir, fmt.Sprintf("file%02d.go", i)) + if err := os.WriteFile(path, []byte(src.String()), 0o644); err != nil { + t.Fatalf("write fixture: %v", err) + } + } + + cacheDisabled := false + cfg := codeguard.ExampleConfig() + cfg.Name = "confidence-policy" + cfg.Targets = []codeguard.TargetConfig{{Name: "repo", Path: dir, Language: "go"}} + cfg.Cache.Enabled = &cacheDisabled + cfg.Checks.QualityRules.MaxFunctionLines = 5 + cfg.Checks.SecurityRules.GovulncheckMode = "off" + return cfg +} + +// An omitted policy and an explicit permissive policy must be indistinguishable, +// which is what makes the default upgrade path a no-op. +func TestConfidencePolicyDefaultMatchesExplicitPermissive(t *testing.T) { + cfg := confidenceScanConfig(t) + omitted, err := codeguard.Run(context.Background(), cfg) + if err != nil { + t.Fatalf("run without policy: %v", err) + } + if omitted.Summary.TotalFindings == 0 { + t.Fatal("fixture produced no findings; the comparison would be vacuous") + } + + cfg.Checks.MinConfidence = codeguard.ConfidencePolicyConfig{Default: codeguard.ConfidenceLow} + explicit, err := codeguard.Run(context.Background(), cfg) + if err != nil { + t.Fatalf("run with permissive policy: %v", err) + } + if !reflect.DeepEqual(omitted.Sections, explicit.Sections) { + t.Fatal("an explicit low threshold changed the scan; the default must admit everything") + } +} + +// End-to-end proof that the threshold reaches every section through the real +// runner, and that what it removes is accounted for rather than lost. +func TestConfidencePolicyFiltersThroughRealScan(t *testing.T) { + cfg := confidenceScanConfig(t) + permissive, err := codeguard.Run(context.Background(), cfg) + if err != nil { + t.Fatalf("permissive run: %v", err) + } + + cfg.Checks.MinConfidence = codeguard.ConfidencePolicyConfig{Default: codeguard.ConfidenceHigh} + strict, err := codeguard.Run(context.Background(), cfg) + if err != nil { + t.Fatalf("strict run: %v", err) + } + + if strict.Summary.TotalFindings >= permissive.Summary.TotalFindings { + t.Fatalf("strict findings = %d, permissive = %d: a high threshold must remove low-confidence findings", + strict.Summary.TotalFindings, permissive.Summary.TotalFindings) + } + + filtered := 0 + for _, section := range strict.Sections { + filtered += section.ConfidenceFilteredCount + } + removed := permissive.Summary.TotalFindings - strict.Summary.TotalFindings + if filtered < removed { + t.Fatalf("confidence-filtered count = %d, but %d findings disappeared", filtered, removed) + } +} + +// The policy is keyed on the section ids that appear in scan output. This +// pins that contract: a section whose id is not a known key could never be +// configured, and the divergence would be silent. +func TestReportedSectionIDsAreConfigurableKeys(t *testing.T) { + report, err := codeguard.Run(context.Background(), confidenceScanConfig(t)) + if err != nil { + t.Fatalf("run: %v", err) + } + if len(report.Sections) == 0 { + t.Fatal("scan reported no sections") + } + for _, section := range report.Sections { + if !core.KnownSectionKey(section.ID) { + t.Fatalf("section %q is reported by a scan but is not a configurable section key (known: %s)", + section.ID, strings.Join(core.SectionKeys(), ", ")) + } + } +} diff --git a/tests/core/finding_confidence_test.go b/tests/core/finding_confidence_test.go new file mode 100644 index 0000000..9ead241 --- /dev/null +++ b/tests/core/finding_confidence_test.go @@ -0,0 +1,61 @@ +package core_test + +import ( + "testing" + + "github.com/devr-tools/codeguard/internal/codeguard/core" +) + +func TestConfidenceRankOrdersLevels(t *testing.T) { + if core.ConfidenceRank(core.ConfidenceHigh) <= core.ConfidenceRank(core.ConfidenceMedium) { + t.Fatal("high must rank above medium") + } + if core.ConfidenceRank(core.ConfidenceMedium) <= core.ConfidenceRank(core.ConfidenceLow) { + t.Fatal("medium must rank above low") + } +} + +// Empty and unrecognized confidence are documented as medium, so ranking must +// place them exactly where an explicit medium sits. +func TestConfidenceRankTreatsUnspecifiedAsMedium(t *testing.T) { + medium := core.ConfidenceRank(core.ConfidenceMedium) + for _, value := range []string{"", " ", "unknown", "CERTAIN"} { + if got := core.ConfidenceRank(value); got != medium { + t.Fatalf("ConfidenceRank(%q) = %d, want medium rank %d", value, got, medium) + } + } +} + +func TestConfidenceRankNormalizesCasingAndSpace(t *testing.T) { + if core.ConfidenceRank(" HIGH ") != core.ConfidenceRank(core.ConfidenceHigh) { + t.Fatal("ConfidenceRank must normalize casing and surrounding space") + } +} + +func TestMeetsConfidence(t *testing.T) { + tests := []struct { + name string + confidence string + threshold string + want bool + }{ + {name: "low threshold admits low", confidence: core.ConfidenceLow, threshold: core.ConfidenceLow, want: true}, + {name: "low threshold admits unspecified", confidence: "", threshold: core.ConfidenceLow, want: true}, + {name: "medium threshold rejects low", confidence: core.ConfidenceLow, threshold: core.ConfidenceMedium, want: false}, + {name: "medium threshold admits unspecified", confidence: "", threshold: core.ConfidenceMedium, want: true}, + {name: "medium threshold admits medium", confidence: core.ConfidenceMedium, threshold: core.ConfidenceMedium, want: true}, + {name: "medium threshold admits high", confidence: core.ConfidenceHigh, threshold: core.ConfidenceMedium, want: true}, + {name: "high threshold rejects medium", confidence: core.ConfidenceMedium, threshold: core.ConfidenceHigh, want: false}, + {name: "high threshold rejects unspecified", confidence: "", threshold: core.ConfidenceHigh, want: false}, + {name: "high threshold admits high", confidence: core.ConfidenceHigh, threshold: core.ConfidenceHigh, want: true}, + {name: "empty threshold admits everything", confidence: core.ConfidenceLow, threshold: "", want: true}, + {name: "unknown threshold admits everything", confidence: core.ConfidenceLow, threshold: "sideways", want: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := core.MeetsConfidence(tt.confidence, tt.threshold); got != tt.want { + t.Fatalf("MeetsConfidence(%q, %q) = %v, want %v", tt.confidence, tt.threshold, got, tt.want) + } + }) + } +} diff --git a/tests/report/confidence_ordering_test.go b/tests/report/confidence_ordering_test.go new file mode 100644 index 0000000..e79aff5 --- /dev/null +++ b/tests/report/confidence_ordering_test.go @@ -0,0 +1,153 @@ +package report_test + +import ( + "bytes" + "strings" + "testing" + + "github.com/devr-tools/codeguard/pkg/codeguard" +) + +func orderingFinding(ruleID string, title string, line int, confidence string) codeguard.Finding { + return codeguard.Finding{ + RuleID: ruleID, + Title: title, + Level: "warn", + Severity: "warn", + Section: "Security", + Message: "finding at line " + itoa(line), + Why: "finding at line " + itoa(line), + Path: "app/main.go", + Line: line, + Confidence: confidence, + } +} + +func itoa(value int) string { + digits := "" + if value == 0 { + return "0" + } + for value > 0 { + digits = string(rune('0'+value%10)) + digits + value /= 10 + } + return digits +} + +func renderText(t testing.TB, report codeguard.Report) string { + t.Helper() + var buf bytes.Buffer + if err := codeguard.WriteReport(&buf, report, "text"); err != nil { + t.Fatalf("write text report: %v", err) + } + return buf.String() +} + +func lineOrder(t testing.TB, rendered string, needles ...string) []int { + t.Helper() + positions := make([]int, 0, len(needles)) + for _, needle := range needles { + idx := strings.Index(rendered, needle) + if idx < 0 { + t.Fatalf("rendered report missing %q:\n%s", needle, rendered) + } + positions = append(positions, idx) + } + return positions +} + +func confidenceReport(findings []codeguard.Finding, filtered int) codeguard.Report { + return codeguard.Report{ + Sections: []codeguard.SectionResult{{ + ID: "security", + Name: "Security", + Status: "warn", + Findings: findings, + ConfidenceFilteredCount: filtered, + }}, + } +} + +// Within one rule group the most trustworthy findings are listed first, so a +// reader meets the high-confidence hits before the speculative ones. +func TestTextReportOrdersFindingsByConfidenceWithinGroup(t *testing.T) { + rendered := renderText(t, confidenceReport([]codeguard.Finding{ + orderingFinding("security.demo", "Demo rule", 10, codeguard.ConfidenceLow), + orderingFinding("security.demo", "Demo rule", 20, codeguard.ConfidenceHigh), + orderingFinding("security.demo", "Demo rule", 30, codeguard.ConfidenceMedium), + }, 0)) + + positions := lineOrder(t, rendered, "line 20", "line 30", "line 10") + if !(positions[0] < positions[1] && positions[1] < positions[2]) { + t.Fatalf("findings not ordered high, medium, low:\n%s", rendered) + } +} + +// Equal confidence keeps scan order, so output stays reproducible. +func TestTextReportConfidenceSortIsStable(t *testing.T) { + rendered := renderText(t, confidenceReport([]codeguard.Finding{ + orderingFinding("security.demo", "Demo rule", 30, codeguard.ConfidenceHigh), + orderingFinding("security.demo", "Demo rule", 10, codeguard.ConfidenceHigh), + orderingFinding("security.demo", "Demo rule", 20, codeguard.ConfidenceHigh), + }, 0)) + + positions := lineOrder(t, rendered, "line 30", "line 10", "line 20") + if !(positions[0] < positions[1] && positions[1] < positions[2]) { + t.Fatalf("equal-confidence findings were reordered:\n%s", rendered) + } +} + +// Groups keep their first-appearance order: confidence sorts inside a group, +// never across groups. +func TestTextReportKeepsGroupOrder(t *testing.T) { + rendered := renderText(t, confidenceReport([]codeguard.Finding{ + orderingFinding("security.first", "First rule", 10, codeguard.ConfidenceLow), + orderingFinding("security.second", "Second rule", 20, codeguard.ConfidenceHigh), + }, 0)) + + positions := lineOrder(t, rendered, "First rule", "Second rule") + if positions[0] > positions[1] { + t.Fatalf("group order changed:\n%s", rendered) + } +} + +func TestTextReportStatesConfidenceFilteredCount(t *testing.T) { + rendered := renderText(t, confidenceReport([]codeguard.Finding{ + orderingFinding("security.demo", "Demo rule", 10, codeguard.ConfidenceHigh), + }, 3)) + if !strings.Contains(rendered, "confidence filtered: 3") { + t.Fatalf("rendered report does not state the confidence-filtered count:\n%s", rendered) + } +} + +func TestTextReportOmitsConfidenceLineWhenNothingFiltered(t *testing.T) { + rendered := renderText(t, confidenceReport([]codeguard.Finding{ + orderingFinding("security.demo", "Demo rule", 10, codeguard.ConfidenceHigh), + }, 0)) + if strings.Contains(rendered, "confidence filtered") { + t.Fatalf("rendered report mentions confidence filtering with nothing filtered:\n%s", rendered) + } +} + +// JSON is a machine surface: section finding order must stay exactly as the +// scan produced it, so the confidence sort is presentation-only. +func TestJSONReportPreservesScanOrder(t *testing.T) { + report := confidenceReport([]codeguard.Finding{ + orderingFinding("security.demo", "Demo rule", 10, codeguard.ConfidenceLow), + orderingFinding("security.demo", "Demo rule", 20, codeguard.ConfidenceHigh), + }, 0) + var buf bytes.Buffer + if err := codeguard.WriteReport(&buf, report, "json"); err != nil { + t.Fatalf("write json report: %v", err) + } + rendered := buf.String() + first := strings.Index(rendered, "finding at line 10") + second := strings.Index(rendered, "finding at line 20") + if first < 0 || second < 0 { + t.Fatalf("json report missing findings:\n%s", rendered) + } + if first > second { + t.Fatalf("json findings were reordered by confidence:\n%s", rendered) + } +} diff --git a/tests/support/confidence_cache_test.go b/tests/support/confidence_cache_test.go new file mode 100644 index 0000000..cf373c9 --- /dev/null +++ b/tests/support/confidence_cache_test.go @@ -0,0 +1,55 @@ +package support_test + +import ( + "testing" + + "github.com/devr-tools/codeguard/internal/codeguard/core" + runnersupport "github.com/devr-tools/codeguard/internal/codeguard/runner/support" +) + +// The confidence policy is applied when a section is finalized, which is after +// the per-file findings cache. Changing a threshold must therefore leave every +// section config fingerprint untouched: a threshold change re-renders a scan, +// it never re-runs one. +func TestConfidencePolicyDoesNotInvalidateCachedFindings(t *testing.T) { + catalog := map[string]core.RuleMetadata{ + "security.demo": {ID: "security.demo", Section: "Security", DefaultLevel: "fail"}, + } + base := core.Config{Name: "cache"} + baseline := runnersupport.SectionConfigHashes(base, catalog) + + withPolicy := base + withPolicy.Checks.MinConfidence = core.ConfidencePolicyConfig{ + Default: core.ConfidenceHigh, + Sections: map[string]string{"security": core.ConfidenceHigh}, + } + withPolicy.Checks.ConfidenceDemotion = true + changed := runnersupport.SectionConfigHashes(withPolicy, catalog) + + if len(baseline) != len(changed) { + t.Fatalf("family count changed: %d -> %d", len(baseline), len(changed)) + } + for family, want := range baseline { + if got := changed[family]; got != want { + t.Fatalf("config fingerprint for family %q changed when the confidence policy changed; cached findings would be discarded", family) + } + } +} + +// A setting that does affect per-file findings must still change the +// fingerprint, so the test above cannot pass by fingerprinting nothing. +func TestSectionConfigHashesStillTrackFindingRelevantSettings(t *testing.T) { + catalog := map[string]core.RuleMetadata{ + "security.demo": {ID: "security.demo", Section: "Security", DefaultLevel: "fail"}, + } + base := core.Config{Name: "cache"} + baseline := runnersupport.SectionConfigHashes(base, catalog) + + changed := base + changed.Parsers.TreeSitter = core.TreeSitterModeAuto + updated := runnersupport.SectionConfigHashes(changed, catalog) + + if updated["security"] == baseline["security"] { + t.Fatal("changing parsers.treesitter must change the security fingerprint") + } +} diff --git a/tests/support/confidence_demotion_test.go b/tests/support/confidence_demotion_test.go new file mode 100644 index 0000000..3499a70 --- /dev/null +++ b/tests/support/confidence_demotion_test.go @@ -0,0 +1,114 @@ +package support_test + +import ( + "testing" + + "github.com/devr-tools/codeguard/internal/codeguard/core" + runnersupport "github.com/devr-tools/codeguard/internal/codeguard/runner/support" +) + +func demotionContext(t testing.TB, demote bool) runnersupport.Context { + t.Helper() + sc := confidenceContext(t, core.ConfidencePolicyConfig{}) + sc.Cfg.Checks.ConfidenceDemotion = demote + return sc +} + +func TestConfidenceDemotionLowersFailingLowConfidenceFindings(t *testing.T) { + sc := demotionContext(t, true) + findings := []core.Finding{confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow)} + section := runnersupport.FinalizeSection(sc, "security", "Security", findings) + if len(section.Findings) != 1 { + t.Fatalf("findings = %d, want 1: demotion must not remove findings", len(section.Findings)) + } + got := section.Findings[0] + if got.Level != "warn" || got.Severity != "warn" { + t.Fatalf("level/severity = %q/%q, want warn/warn", got.Level, got.Severity) + } + if section.Status != core.StatusWarn { + t.Fatalf("status = %q, want %q", section.Status, core.StatusWarn) + } +} + +func TestConfidenceDemotionOffByDefault(t *testing.T) { + sc := demotionContext(t, false) + findings := []core.Finding{confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow)} + section := runnersupport.FinalizeSection(sc, "security", "Security", findings) + if section.Findings[0].Level != "fail" || section.Status != core.StatusFail { + t.Fatalf("level = %q status = %q, want fail/fail", section.Findings[0].Level, section.Status) + } +} + +// Demotion applies only to low confidence, and only downward. +func TestConfidenceDemotionLeavesOtherFindingsAlone(t *testing.T) { + sc := demotionContext(t, true) + tests := []struct { + name string + confidence string + level string + want string + }{ + {name: "medium confidence keeps fail", confidence: core.ConfidenceMedium, level: "fail", want: "fail"}, + {name: "high confidence keeps fail", confidence: core.ConfidenceHigh, level: "fail", want: "fail"}, + {name: "unspecified confidence keeps fail", confidence: "", level: "fail", want: "fail"}, + {name: "low confidence warn is not promoted", confidence: core.ConfidenceLow, level: "warn", want: "warn"}, + {name: "low confidence pass is untouched", confidence: core.ConfidenceLow, level: "pass", want: "pass"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + finding := runnersupport.NewFinding(sc, runnersupport.FindingInput{ + RuleID: confidenceRuleLow, + Level: tt.level, + Path: "app/main.go", + Line: 10, + Message: "demo finding", + Confidence: tt.confidence, + }) + section := runnersupport.FinalizeSection(sc, "security", "Security", []core.Finding{finding}) + if got := section.Findings[0].Level; got != tt.want { + t.Fatalf("level = %q, want %q", got, tt.want) + } + }) + } +} + +// Demotion changes how loudly a finding is reported, never what it is, so an +// existing baseline entry must keep matching after the flag is switched on. +func TestConfidenceDemotionPreservesFindingIdentity(t *testing.T) { + plain := demotionContext(t, false) + demoting := demotionContext(t, true) + input := runnersupport.FindingInput{ + RuleID: confidenceRuleLow, + Path: "app/main.go", + Line: 10, + Message: "demo finding", + Confidence: core.ConfidenceLow, + } + undemoted := runnersupport.FinalizeSection(plain, "security", "Security", + []core.Finding{runnersupport.NewFinding(plain, input)}).Findings[0] + demoted := runnersupport.FinalizeSection(demoting, "security", "Security", + []core.Finding{runnersupport.NewFinding(demoting, input)}).Findings[0] + + if demoted.Level == undemoted.Level { + t.Fatalf("demotion did not change the reported level (both %q)", demoted.Level) + } + if demoted.Fingerprint != undemoted.Fingerprint || + demoted.ContextFingerprint != undemoted.ContextFingerprint || + demoted.ContentFingerprint != undemoted.ContentFingerprint { + t.Fatal("confidence demotion changed finding identity") + } +} + +// Suppression is matched on identity, so a baselined finding stays suppressed +// whether or not it is demoted. +func TestConfidenceDemotionKeepsBaselineMatching(t *testing.T) { + sc := demotionContext(t, true) + finding := confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow) + sc.Baseline = map[string]core.BaselineEntry{ + finding.Fingerprint: {Fingerprint: finding.Fingerprint}, + } + section := runnersupport.FinalizeSection(sc, "security", "Security", []core.Finding{finding}) + if len(section.Findings) != 0 || section.SuppressedCount != 1 { + t.Fatalf("findings = %d suppressed = %d, want 0/1", len(section.Findings), section.SuppressedCount) + } +} diff --git a/tests/support/confidence_filter_test.go b/tests/support/confidence_filter_test.go new file mode 100644 index 0000000..a3ad705 --- /dev/null +++ b/tests/support/confidence_filter_test.go @@ -0,0 +1,175 @@ +package support_test + +import ( + "testing" + + "github.com/devr-tools/codeguard/internal/codeguard/core" + runnersupport "github.com/devr-tools/codeguard/internal/codeguard/runner/support" +) + +const ( + confidenceRuleHigh = "security.demo-high" + confidenceRuleLow = "security.demo-low" +) + +func confidenceContext(t testing.TB, policy core.ConfidencePolicyConfig) runnersupport.Context { + t.Helper() + cfg := core.Config{Name: "test"} + cfg.Checks.MinConfidence = policy + return runnersupport.Context{ + Cfg: cfg, + RuleStats: runnersupport.NewRuleStatsCollector(), + RuleCatalog: map[string]core.RuleMetadata{ + confidenceRuleHigh: {ID: confidenceRuleHigh, Section: "Security", DefaultLevel: "fail"}, + confidenceRuleLow: {ID: confidenceRuleLow, Section: "Security", DefaultLevel: "fail"}, + }, + } +} + +func confidenceFinding(sc runnersupport.Context, ruleID string, confidence string) core.Finding { + return runnersupport.NewFinding(sc, runnersupport.FindingInput{ + RuleID: ruleID, + Path: "app/main.go", + Line: 10, + Column: 1, + Message: "demo finding", + Confidence: confidence, + }) +} + +func statsEntry(t testing.TB, sc runnersupport.Context, ruleID string) core.RuleStatsEntry { + t.Helper() + for _, entry := range sc.RuleStats.Snapshot() { + if entry.RuleID == ruleID { + return entry + } + } + t.Fatalf("no rule stats entry for %q", ruleID) + return core.RuleStatsEntry{} +} + +// An unconfigured policy must admit every finding, which is what keeps the +// default scan byte-identical to the pre-policy behavior. +func TestConfidenceFilterDefaultAdmitsEverything(t *testing.T) { + sc := confidenceContext(t, core.ConfidencePolicyConfig{}) + findings := []core.Finding{ + confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow), + confidenceFinding(sc, confidenceRuleHigh, core.ConfidenceHigh), + } + section := runnersupport.FinalizeSection(sc, "security", "Security", findings) + if len(section.Findings) != 2 { + t.Fatalf("findings = %d, want 2", len(section.Findings)) + } + if section.ConfidenceFilteredCount != 0 { + t.Fatalf("ConfidenceFilteredCount = %d, want 0", section.ConfidenceFilteredCount) + } +} + +func TestConfidenceFilterDropsBelowThreshold(t *testing.T) { + sc := confidenceContext(t, core.ConfidencePolicyConfig{Default: core.ConfidenceHigh}) + findings := []core.Finding{ + confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow), + confidenceFinding(sc, confidenceRuleHigh, core.ConfidenceHigh), + } + section := runnersupport.FinalizeSection(sc, "security", "Security", findings) + if len(section.Findings) != 1 || section.Findings[0].RuleID != confidenceRuleHigh { + t.Fatalf("findings = %+v, want only %q", section.Findings, confidenceRuleHigh) + } + if section.ConfidenceFilteredCount != 1 { + t.Fatalf("ConfidenceFilteredCount = %d, want 1", section.ConfidenceFilteredCount) + } + if section.SuppressedCount != 0 { + t.Fatalf("SuppressedCount = %d, want 0: a confidence filter is not a suppression", section.SuppressedCount) + } + + filtered := statsEntry(t, sc, confidenceRuleLow) + if filtered.ConfidenceFiltered != 1 || filtered.Emitted != 0 { + t.Fatalf("filtered rule stats = %+v, want ConfidenceFiltered 1 and Emitted 0", filtered) + } + if filtered.Suppressed() != 0 { + t.Fatalf("Suppressed() = %d, want 0: confidence must not inflate the suppression ratio", filtered.Suppressed()) + } + if emitted := statsEntry(t, sc, confidenceRuleHigh); emitted.Emitted != 1 { + t.Fatalf("admitted rule stats = %+v, want Emitted 1", emitted) + } +} + +// Every finding must land in exactly one bucket, so a threshold can never make +// findings disappear without a trace. +func TestConfidenceFilterAccountsForEveryFinding(t *testing.T) { + sc := confidenceContext(t, core.ConfidencePolicyConfig{Default: core.ConfidenceHigh}) + findings := []core.Finding{ + confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow), + confidenceFinding(sc, confidenceRuleLow, core.ConfidenceMedium), + confidenceFinding(sc, confidenceRuleHigh, core.ConfidenceHigh), + } + runnersupport.FinalizeSection(sc, "security", "Security", findings) + + total := 0 + for _, entry := range sc.RuleStats.Snapshot() { + total += entry.Emitted + entry.ConfidenceFiltered + entry.Suppressed() + } + if total != len(findings) { + t.Fatalf("accounted findings = %d, want %d", total, len(findings)) + } +} + +func TestConfidenceFilterHonoursPerSectionOverride(t *testing.T) { + policy := core.ConfidencePolicyConfig{Sections: map[string]string{"security": core.ConfidenceHigh}} + sc := confidenceContext(t, policy) + low := []core.Finding{confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow)} + + if section := runnersupport.FinalizeSection(sc, "security", "Security", low); len(section.Findings) != 0 { + t.Fatalf("security findings = %d, want 0", len(section.Findings)) + } + if section := runnersupport.FinalizeSection(sc, "quality", "Code Quality", low); len(section.Findings) != 1 { + t.Fatalf("quality findings = %d, want 1: the override must not leak across sections", len(section.Findings)) + } +} + +// A filtered finding must not fail or warn the gate it was filtered out of. +func TestConfidenceFilterLeavesSectionStatusClean(t *testing.T) { + sc := confidenceContext(t, core.ConfidencePolicyConfig{Default: core.ConfidenceHigh}) + findings := []core.Finding{confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow)} + section := runnersupport.FinalizeSection(sc, "security", "Security", findings) + if section.Status != core.StatusPass { + t.Fatalf("status = %q, want %q", section.Status, core.StatusPass) + } +} + +func TestConfidenceFilterSurfacesUnderIncludeSuppressed(t *testing.T) { + sc := confidenceContext(t, core.ConfidencePolicyConfig{Default: core.ConfidenceHigh}) + sc.Opts = core.ScanOptions{IncludeSuppressed: true} + sc.Suppressed = &runnersupport.SuppressedFindingCollector{} + findings := []core.Finding{confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow)} + runnersupport.FinalizeSection(sc, "security", "Security", findings) + + snapshot := sc.Suppressed.Snapshot() + if len(snapshot) != 1 { + t.Fatalf("suppressed snapshot = %d findings, want 1", len(snapshot)) + } + if !snapshot[0].Suppressed || snapshot[0].SuppressionReason != runnersupport.SuppressionReasonConfidence { + t.Fatalf("snapshot finding = %+v, want suppressed with reason %q", snapshot[0], runnersupport.SuppressionReasonConfidence) + } +} + +// The filter runs before suppression matching, so a finding removed by the +// threshold is never also attributed to a baseline entry. +func TestConfidenceFilterPrecedesSuppressionMatching(t *testing.T) { + sc := confidenceContext(t, core.ConfidencePolicyConfig{Default: core.ConfidenceHigh}) + finding := confidenceFinding(sc, confidenceRuleLow, core.ConfidenceLow) + sc.Baseline = map[string]core.BaselineEntry{ + finding.Fingerprint: {Fingerprint: finding.Fingerprint}, + } + section := runnersupport.FinalizeSection(sc, "security", "Security", []core.Finding{finding}) + if section.SuppressedCount != 0 { + t.Fatalf("SuppressedCount = %d, want 0", section.SuppressedCount) + } + if section.ConfidenceFilteredCount != 1 { + t.Fatalf("ConfidenceFilteredCount = %d, want 1", section.ConfidenceFilteredCount) + } + entry := statsEntry(t, sc, confidenceRuleLow) + if entry.BaselineSuppressed != 0 || entry.ConfidenceFiltered != 1 { + t.Fatalf("rule stats = %+v, want only ConfidenceFiltered 1", entry) + } +} From 7d8ccd48898bbfec61ed0b34d5194146bb950e02 Mon Sep 17 00:00:00 2001 From: Alex Wilkerson John Date: Thu, 3 Sep 2026 19:17:33 -0400 Subject: [PATCH 3/4] feat(config): stamp the codeguard version into written configs A config checked into a repository gave no indication of which codeguard produced it. WriteFile now records the writing release as a top-level codeguard_version, so codeguard init and every SDK write path stamp it. Stamping happens at the write boundary rather than in ApplyDefaults, so a loaded config reports what its file recorded instead of the running binary; that is what makes the field useful for spotting a config produced by a different release. Rewriting refreshes it. The stamp is provenance, not a compatibility gate. It is never validated, so an absent, older, newer, or unparseable value always loads, and decoding was already non-strict, so older binaries ignore the field. It is also absent from SectionConfigHashes, so a release upgrade cannot discard cached findings; a test pins that. It does reach ConfigHash, which only feeds waiver-audit history dedupe, where recording a new observation after a config change is correct. Release builds write v{{.Version}} via the GoReleaser ldflag, so the repository config and the shipped JSON example use the v-prefixed form. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/knowledge/architecture-boundaries.md | 2 + .codeguard/codeguard.yaml | 1 + docs/features.md | 16 +++ examples/codeguard.json | 1 + internal/codeguard/config/io.go | 6 + internal/codeguard/core/config_types.go | 17 ++- tests/codeguard/config_version_test.go | 118 +++++++++++++++++++ tests/support/confidence_cache_test.go | 21 ++++ 8 files changed, 176 insertions(+), 6 deletions(-) create mode 100644 tests/codeguard/config_version_test.go diff --git a/.claude/knowledge/architecture-boundaries.md b/.claude/knowledge/architecture-boundaries.md index 50c00ce..64f5e6a 100644 --- a/.claude/knowledge/architecture-boundaries.md +++ b/.claude/knowledge/architecture-boundaries.md @@ -37,3 +37,5 @@ Key architectural decisions, service boundaries, data flow, integration points, - **Finding identity excludes presentation.** Exact, context, and content fingerprints derive from rule ID, path, line, and source identity only (`runner/support/findings_section.go`) — never message prose, metadata, confidence, or the *reported* level. Confidence demotion (`checks.confidence_demotion`) rewrites `Level`/`Severity` after `NewFinding`, so a demoted finding still matches its baseline entry. Any future policy that changes how a finding is reported must preserve this. - **Post-cache policy must be stripped from cache fingerprints.** `SectionConfigHashes` calls `findingRelevantChecks` to zero settings that cannot change per-file findings (currently `checks.min_confidence` and `checks.confidence_demotion`), including for the conservative `""` all-checks family. Filtering happens in `FinalizeSectionWithDiagnostics`, i.e. after `cachedFileFindings`, so a threshold change must re-render a scan, never invalidate cached findings. Add any new finalize-time-only setting to that strip list. - **Section ids diverge between the registry and reports.** `runner/checks/registry.go` uses `supply-chain`, but the section finalizes as `supply_chain`, which is what appears in `SectionResult.ID`, in `checks.disabled`, and in `checks.min_confidence.sections`. `core.SectionKeys()` holds the report-facing list; `tests/codeguard` asserts every id a real scan reports is in it. +- **`config.WriteFile` stamps `codeguard_version`, `ApplyDefaults` does not.** The stamp is applied at the write boundary so a *loaded* config keeps whatever its file recorded — that is what makes it useful for spotting a config produced by another release. It is never validated (any value, including an unparseable one, must still load), and it is absent from `SectionConfigHashes`, so a release upgrade cannot discard cached findings. Release builds write `v{{.Version}}` (GoReleaser `-X internal/version.Number=v{{.Version}}`), so hand-maintained configs use the `v`-prefixed form; a local dirty build writes e.g. `v1.9.0+dirty`. + diff --git a/.codeguard/codeguard.yaml b/.codeguard/codeguard.yaml index e2363c6..81b7031 100644 --- a/.codeguard/codeguard.yaml +++ b/.codeguard/codeguard.yaml @@ -1,3 +1,4 @@ +codeguard_version: v1.9.0 name: codeguard-repo-ci exclude: - .claude/** diff --git a/docs/features.md b/docs/features.md index 74dfcb4..2e8848d 100644 --- a/docs/features.md +++ b/docs/features.md @@ -215,6 +215,22 @@ JSON: } ``` +### Config provenance + +Every config codeguard writes (`codeguard init`, `WriteConfigFile`) records the +release that produced it as a top-level `codeguard_version`. Loading reports +what the file says rather than the running binary, so a config written by a +different release is visible on inspection. + +The stamp is provenance only: it is never validated, so a config written by an +older or newer release always loads, and it is excluded from the per-file cache +fingerprints, so a release upgrade does not discard cached findings. + +```yaml +codeguard_version: v1.9.0 +name: my-repo +``` + ### Minimum confidence policy Drop findings the scanner is not confident enough about, globally or per diff --git a/examples/codeguard.json b/examples/codeguard.json index cc511e0..240d741 100644 --- a/examples/codeguard.json +++ b/examples/codeguard.json @@ -1,4 +1,5 @@ { + "codeguard_version": "v1.9.0", "name": "codeguard-default", "profile": "startup", "targets": [ diff --git a/internal/codeguard/config/io.go b/internal/codeguard/config/io.go index 627b1be..f5d20b7 100644 --- a/internal/codeguard/config/io.go +++ b/internal/codeguard/config/io.go @@ -10,6 +10,7 @@ import ( "github.com/devr-tools/codeguard/internal/codeguard/cachefile" "github.com/devr-tools/codeguard/internal/codeguard/core" + "github.com/devr-tools/codeguard/internal/version" ) // maxConfigFileBytes caps how much of a config file is read into memory, @@ -220,6 +221,11 @@ func WriteFile(path string, cfg core.Config) error { if err := Validate(cfg); err != nil { return err } + // Stamp at the write boundary rather than in ApplyDefaults, so a loaded + // config keeps the version its file recorded and only rewriting refreshes + // it. That is what makes the field usable for spotting a config produced by + // a different release. + cfg.CodeguardVersion = version.Number data, err := marshalConfig(path, cfg) if err != nil { diff --git a/internal/codeguard/core/config_types.go b/internal/codeguard/core/config_types.go index 5e2c13f..e5528b9 100644 --- a/internal/codeguard/core/config_types.go +++ b/internal/codeguard/core/config_types.go @@ -1,12 +1,17 @@ package core type Config struct { - Name string `json:"name" yaml:"name"` - Profile string `json:"profile,omitempty" yaml:"profile,omitempty"` - Targets []TargetConfig `json:"targets" yaml:"targets"` - Checks CheckConfig `json:"checks" yaml:"checks"` - AI AIConfig `json:"ai,omitempty" yaml:"ai,omitempty"` - RulePacks []RulePackConfig `json:"rule_packs,omitempty" yaml:"rule_packs,omitempty"` + // CodeguardVersion records which codeguard release last wrote this file. + // It is stamped on write and never validated: a config written by an older + // or newer release must always still load, so the field is provenance for + // a human reading the file, not a compatibility gate. + CodeguardVersion string `json:"codeguard_version,omitempty" yaml:"codeguard_version,omitempty"` + Name string `json:"name" yaml:"name"` + Profile string `json:"profile,omitempty" yaml:"profile,omitempty"` + Targets []TargetConfig `json:"targets" yaml:"targets"` + Checks CheckConfig `json:"checks" yaml:"checks"` + AI AIConfig `json:"ai,omitempty" yaml:"ai,omitempty"` + RulePacks []RulePackConfig `json:"rule_packs,omitempty" yaml:"rule_packs,omitempty"` // ExternalReports imports findings produced by already-run scanners. CodeGuard // only reads these files; it never executes the configured tools. ExternalReports []ExternalReportConfig `json:"external_reports,omitempty" yaml:"external_reports,omitempty"` diff --git a/tests/codeguard/config_version_test.go b/tests/codeguard/config_version_test.go new file mode 100644 index 0000000..06f4ff6 --- /dev/null +++ b/tests/codeguard/config_version_test.go @@ -0,0 +1,118 @@ +package codeguard_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/devr-tools/codeguard/internal/version" + "github.com/devr-tools/codeguard/pkg/codeguard" +) + +func writeConfigYAML(t testing.TB, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "codeguard.yaml") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + return path +} + +// A written config records which codeguard produced it, so a config checked +// into a repository carries its own provenance. +func TestWriteConfigStampsCodeguardVersion(t *testing.T) { + path := filepath.Join(t.TempDir(), "codeguard.yaml") + if err := codeguard.WriteConfigFile(path, codeguard.ExampleConfig()); err != nil { + t.Fatalf("write config: %v", err) + } + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read config: %v", err) + } + if !strings.Contains(string(data), "codeguard_version: "+version.Number) { + t.Fatalf("written yaml does not record the codeguard version:\n%s", data) + } + + loaded, err := codeguard.LoadConfigFile(path) + if err != nil { + t.Fatalf("load config: %v", err) + } + if loaded.CodeguardVersion != version.Number { + t.Fatalf("loaded version = %q, want %q", loaded.CodeguardVersion, version.Number) + } +} + +// Writing is what stamps, so rewriting a config produced by an older release +// refreshes the record rather than preserving a stale one. +func TestWriteConfigRefreshesStaleVersionStamp(t *testing.T) { + cfg := codeguard.ExampleConfig() + cfg.CodeguardVersion = "0.0.1" + path := filepath.Join(t.TempDir(), "codeguard.yaml") + if err := codeguard.WriteConfigFile(path, cfg); err != nil { + t.Fatalf("write config: %v", err) + } + loaded, err := codeguard.LoadConfigFile(path) + if err != nil { + t.Fatalf("load config: %v", err) + } + if loaded.CodeguardVersion != version.Number { + t.Fatalf("loaded version = %q, want the writing binary's %q", loaded.CodeguardVersion, version.Number) + } +} + +// Loading must report what the file says, not what the running binary is, so +// the stamp can be used to spot a config written by a different release. +func TestLoadConfigPreservesRecordedVersion(t *testing.T) { + path := writeConfigYAML(t, `codeguard_version: 0.0.1 +name: recorded +targets: + - name: repo + path: . + language: go +output: + format: text +`) + loaded, err := codeguard.LoadConfigFile(path) + if err != nil { + t.Fatalf("load config: %v", err) + } + if loaded.CodeguardVersion != "0.0.1" { + t.Fatalf("loaded version = %q, want the recorded 0.0.1", loaded.CodeguardVersion) + } +} + +// The stamp is informational: a config without one, or one written by a newer +// release, must never fail to load or validate. +func TestConfigVersionStampNeverBlocksLoading(t *testing.T) { + tests := []struct { + name string + stamp string + }{ + {name: "absent", stamp: ""}, + {name: "older release", stamp: "codeguard_version: 0.0.1\n"}, + {name: "newer release", stamp: "codeguard_version: 99.0.0-rc1\n"}, + {name: "development build", stamp: "codeguard_version: devel\n"}, + {name: "unparseable", stamp: "codeguard_version: not-a-version\n"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := writeConfigYAML(t, tt.stamp+`name: stamped +targets: + - name: repo + path: . + language: go +output: + format: text +`) + loaded, err := codeguard.LoadConfigFile(path) + if err != nil { + t.Fatalf("load config: %v", err) + } + if err := codeguard.ValidateConfig(loaded); err != nil { + t.Fatalf("validate config: %v", err) + } + }) + } +} diff --git a/tests/support/confidence_cache_test.go b/tests/support/confidence_cache_test.go index cf373c9..ba976c7 100644 --- a/tests/support/confidence_cache_test.go +++ b/tests/support/confidence_cache_test.go @@ -53,3 +53,24 @@ func TestSectionConfigHashesStillTrackFindingRelevantSettings(t *testing.T) { t.Fatal("changing parsers.treesitter must change the security fingerprint") } } + +// The codeguard version stamp is provenance, not a scan input: it must not +// change any per-file cache fingerprint, or every release upgrade would +// silently discard every cached finding. +func TestVersionStampDoesNotInvalidateCachedFindings(t *testing.T) { + catalog := map[string]core.RuleMetadata{ + "security.demo": {ID: "security.demo", Section: "Security", DefaultLevel: "fail"}, + } + base := core.Config{Name: "cache"} + baseline := runnersupport.SectionConfigHashes(base, catalog) + + stamped := base + stamped.CodeguardVersion = "99.0.0" + changed := runnersupport.SectionConfigHashes(stamped, catalog) + + for family, want := range baseline { + if got := changed[family]; got != want { + t.Fatalf("config fingerprint for family %q changed with the version stamp; cached findings would be discarded", family) + } + } +} From ccd8224a5ecd35dfba8193adc1ae49638cbc2f36 Mon Sep 17 00:00:00 2001 From: Alex Wilkerson John Date: Thu, 3 Sep 2026 20:17:49 -0400 Subject: [PATCH 4/4] test: satisfy staticcheck in the confidence policy tests CI lint caught four staticcheck findings in the new test files: two WriteString(fmt.Sprintf(...)) calls that should be fmt.Fprintf (QF1012), and two negated conjunctions that read better under De Morgan (QF1001). Also drops a hand-rolled integer formatter in favor of strconv.Itoa. These were missed locally because golangci-lint suppresses staticcheck findings in repository code when its typecheck pass fails, and it fails here on the Homebrew Go 1.27.1 stdlib since the binary is built with go1.26.3. The local run therefore exited 0 while CI reported four issues. Pinning GOTOOLCHAIN to the linter's build version reproduces CI exactly and now reports 0 issues; the knowledge note records the corrected command. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/knowledge/testing-patterns.md | 9 +++++++- .../codeguard/confidence_policy_scan_test.go | 4 ++-- tests/report/confidence_ordering_test.go | 21 +++++-------------- 3 files changed, 15 insertions(+), 19 deletions(-) diff --git a/.claude/knowledge/testing-patterns.md b/.claude/knowledge/testing-patterns.md index 76ded29..48cf4cc 100644 --- a/.claude/knowledge/testing-patterns.md +++ b/.claude/knowledge/testing-patterns.md @@ -15,7 +15,14 @@ Testing strategies, test infrastructure quirks, how to run/debug specific test s - Defensive precision positive fixtures should avoid UI-ish names such as `render*` unless the test is explicitly covering UI suppression. The defensive boundary/null rules intentionally skip React/UI helper contexts, so a fixture named like a renderer can stop emitting the server-side defensive finding the test expects. - Precision-rule retunes should include both the false-positive fixture and a nearby positive that still fires. Dogfood the local binary against a real target repo before committing; several naming/mutation rules only showed meaningful movement after scanning a mixed React, route, and integration-heavy TypeScript monorepo. - **`go test ./...` is red on `main` (as of 1205484).** `TestSecuritySemanticAnalyzerScansNodeModulesWithinTarget` (`tests/checks/typescript_semantic_test.go:182`) fails with `Security status = "pass", want "fail"`. Its `requireTypeScriptSemanticRuntime` guard does *not* skip, so this is a real failure, not a missing-runtime skip; it plausibly relates to the vendored/`node_modules` scan bounding in 24cb12c but has not been diagnosed. Reproduce a baseline in a clean worktree (`git worktree add --detach HEAD`) before attributing a `tests/checks` failure to your own change. -- **Local strict lint cannot typecheck the current Go stdlib.** `golangci-lint run` reports 2 `typecheck` issues inside `crypto/internal/randutil` / `math/rand/v2` because its bundled Go is older than the Homebrew toolchain (1.27.1). These are stdlib paths, not repo code — check that no reported path is under `cmd/`, `internal/`, `pkg/`, or `tests/` and rely on CI's pinned version for the real gate. +- **Local strict lint needs a pinned Go toolchain, or it silently under-reports.** Plain `golangci-lint run` fails with 2 `typecheck` issues inside `crypto/internal/randutil` / `math/rand/v2`: the binary is built with go1.26.3 but the Homebrew toolchain is 1.27.1, whose stdlib it cannot parse. It still exits 0, and — the dangerous part — `staticcheck` findings in your own code are **suppressed** when typecheck fails, so a clean-looking local run can still fail CI. Reproduce CI exactly with: + + ``` + env -u GOROOT GOTOOLCHAIN=go1.26.3 GOCACHE=/private/tmp/codeguard-go-cache \ + GOLANGCI_LINT_CACHE=/private/tmp/golangci-lint-cache /Users/alex/.go/bin/golangci-lint run + ``` + + Match `GOTOOLCHAIN` to the Go version the linter was built with (`golangci-lint --version`), not to go.mod. CI pins golangci-lint v2.12.2 with `go-version-file: go.mod`. - **Report rendering is tested through the SDK.** `internal/codeguard/report` has no external test hooks, so `tests/report` drives it via `codeguard.WriteReport(w, report, format)` with a hand-built `codeguard.Report`. Note the text writer emits a large braille/ANSI banner, so assert on substring *positions* rather than diffing whole output. `tests/core` and `tests/report` were added as external packages for `core` and `report` respectively. - **Verifying a self-scan didn't add findings:** `make codeguard-ci` prints only a summary count, so compare against a clean worktree at HEAD and diff the `N. at:` location lines (`grep -E '^\s+[0-9]+\. at:' | sed 's/^ *[0-9]*\. at: //' | sort`). The repo's own `quality.ai.narrative-comment` rule fires on doc comments that describe what code is rather than why it exists, so new comments can add findings. diff --git a/tests/codeguard/confidence_policy_scan_test.go b/tests/codeguard/confidence_policy_scan_test.go index bc205df..ec767f2 100644 --- a/tests/codeguard/confidence_policy_scan_test.go +++ b/tests/codeguard/confidence_policy_scan_test.go @@ -19,9 +19,9 @@ func confidenceScanConfig(t testing.TB) codeguard.Config { for i := 0; i < 6; i++ { var src strings.Builder src.WriteString("package repo\n\n") - src.WriteString(fmt.Sprintf("func Long%02d() int {\n", i)) + fmt.Fprintf(&src, "func Long%02d() int {\n", i) for line := 0; line < 12; line++ { - src.WriteString(fmt.Sprintf("\tv%d := %d\n\t_ = v%d\n", line, line, line)) + fmt.Fprintf(&src, "\tv%d := %d\n\t_ = v%d\n", line, line, line) } src.WriteString("\treturn 0\n}\n") path := filepath.Join(dir, fmt.Sprintf("file%02d.go", i)) diff --git a/tests/report/confidence_ordering_test.go b/tests/report/confidence_ordering_test.go index e79aff5..16aff15 100644 --- a/tests/report/confidence_ordering_test.go +++ b/tests/report/confidence_ordering_test.go @@ -2,6 +2,7 @@ package report_test import ( "bytes" + "strconv" "strings" "testing" @@ -15,26 +16,14 @@ func orderingFinding(ruleID string, title string, line int, confidence string) c Level: "warn", Severity: "warn", Section: "Security", - Message: "finding at line " + itoa(line), - Why: "finding at line " + itoa(line), + Message: "finding at line " + strconv.Itoa(line), + Why: "finding at line " + strconv.Itoa(line), Path: "app/main.go", Line: line, Confidence: confidence, } } -func itoa(value int) string { - digits := "" - if value == 0 { - return "0" - } - for value > 0 { - digits = string(rune('0'+value%10)) + digits - value /= 10 - } - return digits -} - func renderText(t testing.TB, report codeguard.Report) string { t.Helper() var buf bytes.Buffer @@ -79,7 +68,7 @@ func TestTextReportOrdersFindingsByConfidenceWithinGroup(t *testing.T) { }, 0)) positions := lineOrder(t, rendered, "line 20", "line 30", "line 10") - if !(positions[0] < positions[1] && positions[1] < positions[2]) { + if positions[0] >= positions[1] || positions[1] >= positions[2] { t.Fatalf("findings not ordered high, medium, low:\n%s", rendered) } } @@ -93,7 +82,7 @@ func TestTextReportConfidenceSortIsStable(t *testing.T) { }, 0)) positions := lineOrder(t, rendered, "line 30", "line 10", "line 20") - if !(positions[0] < positions[1] && positions[1] < positions[2]) { + if positions[0] >= positions[1] || positions[1] >= positions[2] { t.Fatalf("equal-confidence findings were reordered:\n%s", rendered) } }