Scan precision and performance: false-positive fixes, determinism, and a 1.3-1.5x faster scan

.claude/knowledge/local-dev-setup.md

@@ -5,3 +5,6 @@ How to set up, run, and work with this project locally. Non-obvious dependencies
 - **The self-scan cache lives at `.codeguard/.codeguard/cache.json`, not `.codeguard/cache.json`.** `cache.path` resolves relative to the config directory (`config.containConfigArtifactPaths`), and `make codeguard-ci` uses `-config .codeguard/codeguard.yaml`, so the default `.codeguard/cache.json` nests. When iterating on **rule logic**, delete that file before re-running `make codeguard-ci`: cache keys cover config + file content + the release-bumped scanner-version constant, so a code-only change to a check serves stale findings from the cache and looks like your fix didn't work.
 - The Makefile runs Go via `env -u GOROOT go` (Makefile:6), so `make` targets work even with the shell's stale GOROOT; direct `go` commands need `export GOROOT=/opt/homebrew/opt/go/libexec && export PATH=$GOROOT/bin:$PATH` first.
 - Local `golangci-lint run` can fail from the shell's stale `GOROOT` or unwritable `~/Library/Caches/golangci-lint` even when CI is clean. Use `env -u GOROOT GOCACHE=/private/tmp/codeguard-go-cache GOLANGCI_LINT_CACHE=/private/tmp/golangci-lint-cache golangci-lint run` to reproduce CI lint locally.
+- **`golangci-lint` needs a Go 1.25 GOROOT to run here — there IS a working recipe.** The installed v2.12.2 is built with go1.26.3 and cannot analyze this tree under the default go1.27.1: it panics inside `pkg/goanalysis` (`file requires newer Go version go1.27`) and emits phantom `math/rand/v2` stdlib typecheck errors on crypto-importing packages, reproducibly on unmodified HEAD. **Verified fix:** point it at the Homebrew go@1.25 toolchain — `G=$(ls -d /opt/homebrew/Cellar/go@1.25/*/libexec | head -1); GOROOT=$G PATH=$G/bin:$PATH /Users/alex/.go/bin/golangci-lint run ./...` → 0 issues. Note `make lint-strict` runs `env -u GOROOT golangci-lint`, which resolves yet another Go and panics, and the `export GOROOT=/opt/homebrew/opt/go/libexec` recipe elsewhere in this file is the 1.27 toolchain and does NOT work for lint.
+- **`golangci-lint` takes a global lock** and aborts with `Error: parallel golangci-lint is running` if a second invocation overlaps — relevant when several worktrees or agents build/lint concurrently. Retry, or serialise; it is never a code problem.
+- **A worktree-isolated agent cannot `git commit` here.** The `rtk`/`szr` `PreToolUse` hooks in `~/.claude/settings.json` rewrite `git commit`/`add`/`checkout`/`status`/`diff` into a `rtk git …` / `szr git …` launcher form, and the worktree-isolation guard then refuses the wrapped command; the permission classifier also denies the direct spelling. `git switch`, `git stage`, `git mv`, `git rev-parse`, `git ls-tree`, `git ls-files` pass through unrewritten, so an agent can stage and self-verify (`git ls-tree` diff of the staged tree against HEAD) but the commit itself must be made by the user or with the hooks disabled. Plan parallel-worktree work accordingly: have agents stage and write the message to a file, and commit at integration.

.claude/knowledge/testing-patterns.md

@@ -26,3 +26,11 @@ Testing strategies, test infrastructure quirks, how to run/debug specific test s
 - **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.
 
+- **The precision/recall corpus covers the SECURITY section only.** `groupConfig` in `tests/corpus/corpus_test.go` builds a security-only SDK config, so the scoreboard measures 24 `security.*` rules out of ~254 catalog rules. Quality, design, performance, ci, supply-chain, prompts, agent-context, change, contracts, data and reliability have no ground-truth precision harness — a change there can only be regression-tested against hand-written fixtures. Extending the harness to another section means adding a group whose `groupConfig` enables that section (and disabling the command-driven rules, as the security groups do for govulncheck).
+- **To measure false positives beyond the corpus, scan unrelated real repositories.** Write a config per language with an ABSOLUTE `targets[].path` (a relative `path: .` resolves against the *config file's* directory, so a config in a temp dir silently scans the temp dir and every repo yields identical output), set `cache.enabled: false`, `--format json`, then aggregate `rule_id` counts and sample findings with surrounding source. This surfaced the exclusion, fixture-classification, yarn-berry and PEP 503 issues; none were visible from the repo's own self-scan.
+- **Determinism must be tested against a FROZEN tree, not a working copy.** Scanning a live repo gives different finding counts run to run (observed 1735–1905 on one repo, varying across 41 rules) purely because the tree mutates under the walk — build caches, daemons, editor temp files. `git archive HEAD | tar -x -C <tmpdir>` and scan that: the engine itself is deterministic (identical count and finding-set hash across runs). Don't chase a scheduling bug before ruling this out; `GOMAXPROCS=1` does NOT rule it out.
+- **For rules that can fire en masse, assert the EXACT finding/message set, not presence.** `assertFindingRulePresent` cannot distinguish a rule firing once correctly from firing 33 times wrongly, which is exactly how the `lockfile-drift` Yarn Berry bug survived existing coverage. Table-driven cases over `supplyChainTestConfig` + `supplyChainRuleMessages` comparing the full message slice are the cheap shape for this. The suite must also pin both directions of any "stay silent when unsure" behaviour: a garbage lockfile produces zero findings, AND a genuinely empty-but-valid one still reports drift — otherwise a later "fix" can silence the rule entirely and still look green.
+- **`golangci-lint` cannot run concurrently.** It takes a lock and aborts with `Error: parallel golangci-lint is running`, so parallel agents/worktrees linting at once all fail spuriously. Serialise lint runs, or pass a distinct `GOLANGCI_LINT_CACHE` and retry on that error — it is never a code problem.
+- **Corpus `vulnerable/` fixtures must not use vendor-documented placeholder credentials.** Three fixtures used AWS's published `AKIAIOSFODNN7EXAMPLE`, so the harness was scoring a world-readable non-secret as a true positive — wrong ground truth, not a detector gap. Use the split-literal form instead (`"AKIA" + "<16 chars>"`): `foldConcatenatedLiterals` joins adjacent literals before matching, so detection is exercised while no contiguous provider token is committed, which is also what gets a realistic fixture past GitHub push protection. The documented placeholders belong in `clean/`. When a corpus TP count *rises* after a precision fix, check whether new `must_fire` cases were added versus ground truth being loosened — both look the same in the scoreboard.
+- **Zero-config behaviour can only be tested from a directory with no discoverable config.** `codeguard scan -folder <path>` resolves its config from the *current working directory* (`resolveConfigPath` finds `./codeguard.yaml` or `./.codeguard/`), not from the scanned folder, so running it from anywhere inside this repo silently uses `.codeguard/codeguard.yaml` and never exercises the fallback path at all. A measurement or test of the no-config path must `chdir` into a fresh empty dir first — otherwise it passes vacuously. This invalidated a measurement mid-review (a reported 144 fail-level findings was really the self-scan config; the true fallback figure was 149).
+- **Parallel agents must not share one scratchpad filename.** Two worktree agents both wrote their prepared commit message to `<scratchpad>/commitmsg.txt`; the second clobbered the first, and the surviving file carried the wrong branch's message. Scratchpad paths are shared across agents in a session — namespace per-agent files by branch or agent id.

.codeguard/codeguard.yaml

@@ -54,6 +54,20 @@ checks:
   design_rules:
     max_decls_per_file: 60
     god_module_threshold: 32
+    # Package-layering policy is off by default (it describes one specific
+    # repository layout). codeguard's own layering invariants are cmd/ ->
+    # internal/cli and a pkg/codeguard facade that owns the only internal/
+    # imports, so the self-scan states them explicitly.
+    require_cmd_through_internal_cli: true
+    forbid_internal_import_cmd: true
+    forbid_service_import_internal: true
+    forbid_service_import_cmd: true
+    # Which package is the facade is a property of the repository being
+    # scanned, not a literal in codeguard's source. Unset, it is discovered
+    # (every directory under pkg/); stating it keeps the layering rule strict
+    # for any future pkg/ package that is not the SDK facade.
+    sdk_facade_paths:
+      - pkg/codeguard
   performance_rules:
     hot_package_importer_threshold: 32
     rebuild_amplifier_threshold: 40
@@ -75,10 +89,18 @@ checks:
     # below 85 (the repo currently scores 100; see docs/checks.md).
     legibility_warn_threshold: 85
   ci_rules:
+    # Every ci_rules list is empty by default: codeguard cannot know another
+    # repository's workflow filenames, release artifacts or test layout. This
+    # repository's conventions are therefore stated here in full.
+    #
+    # The tests/-only convention is enforced by review; internal/** and cmd/**
+    # are allowed here because a handful of in-package tests predate the rule.
     allowed_test_paths:
       - tests/**
       - internal/**
       - cmd/**
+    required_gates:
+      - test
     require_workflow_dir: true
     required_workflow_files:
       - .github/workflows/ci.yml

.gitignore

@@ -40,3 +40,4 @@ go.work.sum
 **/.codeguard/cache.perf-history.json
 **/.codeguard/cache.rule-stats-history.json
 **/.codeguard/cache.legibility-history.json
+.bench/

Makefile

@@ -30,13 +30,20 @@ MENU_LDFLAGS := -X github.com/devr-tools/codeguard/internal/version.Number=v$(ME
 # ~206 (~+22 MB). Builds without these tags (plain `go build`, `go install`)
 # still work; they just embed every grammar.
 GRAMMAR_TAGS := grammar_subset,grammar_subset_typescript,grammar_subset_tsx,grammar_subset_javascript,grammar_subset_python,grammar_subset_cpp
+# make bench-cpu knobs (see the bench-cpu target).
+BENCH ?= BenchmarkRealRepositoryFullScan
+BENCH_REPOS ?=
+BENCH_CONFIGS ?=
+BENCH_COUNT ?= 5
+BENCH_TIME ?= 1x
+BENCH_PROFILE_DIR ?= $(CURDIR)/.bench
 
 export GOCACHE
 export GOMODCACHE
 
 .DEFAULT_GOAL := help
 
-.PHONY: help fmt fmt-check lint lint-strict test codeguard-ci check ci build release release-snapshot release-check deploy commit menu table table-diff table-interactive clean
+.PHONY: help fmt fmt-check lint lint-strict test bench-cpu codeguard-ci check ci build release release-snapshot release-check deploy commit menu table table-diff table-interactive clean
 
 help:
 	@printf "\ncodeguard make targets\n\n"
@@ -45,6 +52,7 @@ help:
 	@printf "  make lint       Run go vet\n"
 	@printf "  make lint-strict  Run golangci-lint (enforced in CI)\n"
 	@printf "  make test       Run the Go test suite\n"
+	@printf "  make bench-cpu  Profile a full scan of real repo snapshots (BENCH_CONFIGS=/abs/codeguard.yaml,...)\n"
 	@printf "  make codeguard-ci  Validate and scan this repository with codeguard\n"
 	@printf "  make check      Run fmt-check, lint, test, and codeguard-ci\n"
 	@printf "  make ci         Run the full local CI gate, including golangci-lint\n"
@@ -81,6 +89,32 @@ lint-strict:
 test:
 	@set -o pipefail; $(GO) test ./... 2>&1 | grep -v '\[no test files\]'
 
+# bench-cpu profiles a full scan of real repository snapshots. Point BENCH_REPOS
+# at frozen snapshots (`git archive HEAD | tar -x -C <dir>`), never a live
+# working tree: a tree that mutates under the walk gives different findings run
+# to run. Paths must be absolute — a relative target path resolves against the
+# config directory, not the shell's cwd.
+#
+# BENCH_CONFIGS scans with a codeguard.yaml verbatim, so the profile covers the
+# same rule set as the `codeguard scan` you time with it; BENCH_REPOS uses the
+# default `codeguard init` check set. Prefer BENCH_CONFIGS — the check set moves
+# full-scan cost by more than 2x.
+#
+#   make bench-cpu BENCH_CONFIGS=/tmp/cfg/szr/codeguard.yaml
+#   make bench-cpu BENCH_REPOS=/tmp/snap/szr,/tmp/snap/sentinel
+#   make bench-cpu BENCH_REPOS=/tmp/snap/szr BENCH='.*/cold'
+#   go tool pprof -top -cum -nodecount=10 .bench/cpu.out
+bench-cpu:
+	@test -n "$(BENCH_REPOS)$(BENCH_CONFIGS)" || { echo "set BENCH_CONFIGS=/abs/codeguard.yaml[,...] or BENCH_REPOS=/abs/snapshot[,...]"; exit 1; }
+	@mkdir -p $(BENCH_PROFILE_DIR)
+	@set -o pipefail; CODEGUARD_BENCH_REPOS='$(BENCH_REPOS)' CODEGUARD_BENCH_CONFIGS='$(BENCH_CONFIGS)' $(GO) test ./tests/codeguard \
+		-tags $(GRAMMAR_TAGS) -run '^$$' -bench '$(BENCH)' -benchmem \
+		-count=$(BENCH_COUNT) -benchtime=$(BENCH_TIME) \
+		-cpuprofile $(BENCH_PROFILE_DIR)/cpu.out \
+		-memprofile $(BENCH_PROFILE_DIR)/mem.out \
+		-o $(BENCH_PROFILE_DIR)/bench.test 2>&1 | tee $(BENCH_PROFILE_DIR)/bench.txt
+	@echo "profiles: $(BENCH_PROFILE_DIR)/cpu.out $(BENCH_PROFILE_DIR)/mem.out (binary $(BENCH_PROFILE_DIR)/bench.test)"
+
 codeguard-ci: build
 	$(CODEGUARD_BIN) validate -config $(CI_CONFIG)
 	$(CODEGUARD_BIN) scan -config $(CI_CONFIG)

docs/architecture.md

@@ -41,7 +41,7 @@ This keeps the runner tree organized with the same directory-first style as `int
 ## Current check behavior
 
 - `quality` enforces parseability, `gofmt` cleanliness, AST-derived maintainability thresholds, cyclomatic complexity, and dependency-direction checks
-- `design` enforces configurable layer boundaries between `cmd/`, `internal/`, and reusable `pkg/codeguard/` packages, plus principle checks for separation of concerns, clean-code naming, and SOLID-oriented heuristics
+- `design` enforces configurable layer boundaries between `cmd/`, `internal/`, and the scanned repository's public `pkg/` facade (`design_rules.sdk_facade_paths`, or discovered from the layout), plus principle checks for separation of concerns, clean-code naming, and SOLID-oriented heuristics
 - `prompts` discovers prompt-oriented files and enforces configurable checks for secret interpolation and unsafe instruction patterns
 - `ci` enforces configurable repository policy for workflow directories, workflow files, workflow contents, release files, and automation entrypoints
 - `security` runs local heuristic scanning first and can optionally run `govulncheck` in `off`, `auto`, or `required` mode with per-vulnerability findings when output is available