diff --git a/.claude/knowledge/architecture-boundaries.md b/.claude/knowledge/architecture-boundaries.md index e6be7f7..6868e4c 100644 --- a/.claude/knowledge/architecture-boundaries.md +++ b/.claude/knowledge/architecture-boundaries.md @@ -15,6 +15,10 @@ Key architectural decisions, service boundaries, data flow, integration points, - **The secret scanner exposes a pure API for reuse: `security.BuildScanner(cfg) Scanner` + `Scanner.ScanContent(content) []Match` + `Scanner.SkipPath(file)`.** `secretFindingsForFile` wraps `ScanContent` with `env.NewFinding`. Tiers in `scanLine` (one match per line, priority order): private-key → `credentialPatterns` (known formats + DB/Bearer/Azure, fail) → config `custom_patterns` → name-based (warn) → opt-in entropy (`security.high-entropy-string`, warn). Values are masked in messages via `maskSecret` (first4…last4). Entropy = `shannonEntropy` (bits/char) over whitespace-free quoted literals, gated by `secrets.entropy` (off by default; min_length 20, threshold 4.5). GitHub-token CRC32 checksum validation was deliberately NOT added — getting the algorithm slightly wrong yields false negatives (worse than a false positive for a security tool). - **Git-history secret scan lives in its own top-level package `internal/codeguard/history` (shells `git log -p -U0`), exposed via `pkg/codeguard.ScanGitHistory` and the `scan-history` CLI command.** It MUST be a separate package, not `runner/support`: `checks/security` imports `runner/support` (for `MatchPattern`), so `runner/support` importing `checks/security` would cycle. `history` imports `checks/security` (above it) and is consumed by `pkg` + CLI (both top-level). The parser tracks `+++ b/`, hunk headers (`@@ … +c`), and `+`/`-` line prefixes to attribute added lines to commits; dedupes by rule+path+masked-message keeping the newest commit (log is newest-first). - **GOTCHA: `support.ScanTargetFiles` caches per-file findings keyed by `(sectionID, target.Path, rel)`.** If two passes scan the same files with the same `sectionID`, the second pass gets a cache hit and returns the FIRST pass's findings. The secret pass therefore uses a distinct cache id `"security-secrets"` (not `"security"`) so it doesn't collide with the language pass. `sectionID` here only affects the cache key — findings still land in the real section via their RuleID/`FinalizeSection`. When adding any additional `ScanTargetFiles` call to an existing section, give it a unique cache id. +- **GOTCHA: `ScanTargetFiles`'s per-file cache only works for PURE evaluators** (evaluators whose sole output is the returned `[]core.Finding`). On a cache hit the evaluator is NOT called, so any evaluator that builds state via a captured closure and returns `nil` will silently produce nothing once a warm cache exists. Clone detection hit exactly this (all `quality.duplicate-code` findings vanished on the second scan). Cross-file/stateful passes MUST use `VisitTargetFiles` (bypasses the findings cache, visits every file) instead of `ScanTargetFiles`. See `checks/quality/quality_clone.go:cloneDocumentsForTarget`. +- **PERF: the scan is parallelized at the section level** (`runner/checks/checks.go:Build`) on a `runtime.NumCPU()`-bounded worker pool; results are written into position-indexed slots so output order stays deterministic. This required making the shared mutable state concurrency-safe: `ScanCache` (mutex in `cache_types.go`), `ArtifactStore` (mutex in `artifacts.go`), and the per-scan file corpus. `OnSectionComplete` is wrapped in a mutex (`withSynchronizedSectionCallback`) since sections now finish concurrently. File-level parallelism was deliberately NOT used — some evaluators mutate closure state (clone), which is only safe when a section runs serially. +- **PERF: files are walked, read, and Go-parsed once per scan via a shared `fileCorpus`** (`runner/support/corpus.go`), held by pointer on the by-value `Context` so all sections share it. `ScanTargetFiles`/`VisitTargetFiles` read bytes through it; `support.ParseGoSource(env, file, data)` returns a shared `*ast.File`/`*token.FileSet` (parsed with `ParseComments`) so quality and design no longer re-parse the same file. Cached ASTs are read-only — never mutate them. Tradeoff: peak memory is higher (the corpus retains all read bytes + ASTs for the scan), but wall-clock improves ~1.2x (small repos) to ~1.7x (large repos). Taint/graph/perf parsers keep their own cheaper parse modes (`SkipObjectResolution`/`ImportsOnly`) and are intentionally not routed through the shared cache. +- **The per-file cache fingerprint is per-config-family, not global** (`SectionConfigHashes` in `cache_helpers.go`; cache version bumped to 7). Each family (`quality`/`design`/`security`/`prompts`/`ci`/`contracts`) hashes only the config that can change its findings (quality also hashes `DesignRules` + `AI`; the rule catalog is in every family since a metadata override changes `NewFinding` output). `sectionConfigFamily` maps a cache section id to its family by prefix; unknown ids fall back to the `""` all-checks hash so a new section can never silently serve stale entries. Editing an unrelated field (output format, target names, exclude list) no longer invalidates the whole cache. - **The MCP server is hand-rolled (no SDK) and transport-agnostic.** Shared JSON-RPC message builders + the synchronous method router live in `internal/cli/mcp_dispatch.go` (`buildResultMessage`/`buildErrorMessage`/`buildProgressMessage`, `serverCapabilities`, `dispatchSyncMethod`). Both transports reuse them. stdio (`mcp_run.go`/`mcp_requests.go`) keeps an async goroutine + cancel-map for `tools/call` (cancellation via `notifications/cancelled`); HTTP (`mcp_http.go`) runs `tools/call` synchronously per-request and streams progress over SSE, cancellation driven by the request context. When adding a capability, wire it into `dispatchSyncMethod` (HTTP picks it up automatically) AND the stdio method switch in `mcp_run.go`. - **MCP server→client requests (sampling/roots)** flow through `internal/cli/mcp_client.go`: `clientCaller`/`clientBridge` + a `serverRequester` (pending-id→chan registry). The transport supplies a `send` closure and feeds inbound responses (classified by `isResponseMessage`: id present, no method) back via `serverRequester.deliver`. stdio demuxes responses in the read loop (`mcp_run.go handleLine`); HTTP needs the client to open the `GET {mcp-path}` SSE stream — server-initiated requests are written there and answered on later POSTs, correlated per session in `internal/cli/mcp_http_session.go`. Client capabilities are captured at `initialize` (`parseClientCapabilities`). The `clientCaller`/progress emitter reach tools via context (`withClientCaller`/`withProgress`), not signatures. - **MCP fix tools** (`internal/cli/mcp_fix.go`): `verify_fix` wraps `service.VerifyFix` (caller diff); `propose_fix` wraps `service.GenerateVerifiedFix` with a generator resolved as sampling-first (`samplingGenerator` calls the client LLM) then `internalfix.NewAIGenerator(cfg.AI)`; `apply_fix` verifies then writes the tree via `runnersupport.ApplyUnifiedDiff` (the one `destructiveHint` tool), confirming via `clientCaller.elicit` (`elicitation/create`) when supported. Verification test execution is trust-gated, so it needs `CODEGUARD_ALLOW_CONFIG_COMMANDS=1` or it fails closed. Fix failures return `toolErrorResultData` (isError + structuredContent with attempted diff + remaining findings). diff --git a/.codeguard/codeguard.yaml b/.codeguard/codeguard.yaml index 79bfdec..41cbb6b 100644 --- a/.codeguard/codeguard.yaml +++ b/.codeguard/codeguard.yaml @@ -15,6 +15,9 @@ waivers: - rule: ci.test-without-assertion path: tests/**/trust_main_test.go reason: Go TestMain functions bootstrap package-wide trust policy and are not assertion-bearing tests + - rule: quality.unbounded-goroutines-in-loop + path: internal/codeguard/runner/checks/checks.go + reason: section workers are bounded by a NumCPU-sized semaphore before each goroutine is launched targets: - name: repository path: .. diff --git a/internal/codeguard/checks/design/design_go.go b/internal/codeguard/checks/design/design_go.go index 15666ce..8cd7c09 100644 --- a/internal/codeguard/checks/design/design_go.go +++ b/internal/codeguard/checks/design/design_go.go @@ -3,7 +3,6 @@ package design import ( "fmt" "go/ast" - "go/parser" "go/token" "path/filepath" "strings" @@ -21,8 +20,7 @@ func goTargetFindings(env support.Context, target core.TargetConfig) []core.Find } func goFindingsForFile(env support.Context, file string, data []byte) []core.Finding { - fset := token.NewFileSet() - parsed, err := parser.ParseFile(fset, file, data, parser.ParseComments) + fset, parsed, err := support.ParseGoSource(env, file, data) if err != nil { return nil } diff --git a/internal/codeguard/checks/quality/quality_clone.go b/internal/codeguard/checks/quality/quality_clone.go index c68cdc5..69da252 100644 --- a/internal/codeguard/checks/quality/quality_clone.go +++ b/internal/codeguard/checks/quality/quality_clone.go @@ -50,14 +50,17 @@ func cloneFindingsForTarget(env support.Context, target core.TargetConfig) []cor func cloneDocumentsForTarget(env support.Context, target core.TargetConfig) []cloneDocument { docs := make([]cloneDocument, 0) include := cloneIncludeForLanguage(target.Language) - env.ScanTargetFiles(target, "quality-clone", func(rel string) bool { + // Clone detection builds cross-file state (the document list) rather than + // per-file findings, so it must visit every file directly. Routing it + // through the per-file findings cache would skip the tokenizer on a cache + // hit and silently drop every clone once a warm cache exists. + env.VisitTargetFiles(target, func(rel string) bool { return include(rel) && !cloneExcludedPath(target.Language, rel) - }, func(file string, data []byte) []core.Finding { + }, func(file string, data []byte) { tokens := tokenizeNormalizedCloneText(string(data)) if len(tokens) > 0 { docs = append(docs, cloneDocument{Path: file, Tokens: tokens}) } - return nil }) return docs } @@ -81,24 +84,39 @@ func cloneWindowIndex(docs []cloneDocument, threshold int) cloneIndex { } func collectCloneCandidates(index cloneIndex, docs []cloneDocument, threshold int) []cloneCandidate { - candidates := make([]cloneCandidate, 0) + // Candidates are partitioned by their (LeftDoc, RightDoc) pair. The overlap + // merge only ever compares candidates that share a pair, so bucketing by + // pair turns the merge from a linear scan of every candidate found so far + // into a scan of just the (usually tiny) bucket for that file pair. + byPair := make(map[[2]int][]cloneCandidate) for _, occurrences := range index { - candidates = appendClonePairs(candidates, occurrences, docs, threshold) + addClonePairs(byPair, occurrences, docs, threshold) } - return candidates + return flattenCloneCandidates(byPair) } -func appendClonePairs(candidates []cloneCandidate, occurrences []cloneOccurrence, docs []cloneDocument, threshold int) []cloneCandidate { +func addClonePairs(byPair map[[2]int][]cloneCandidate, occurrences []cloneOccurrence, docs []cloneDocument, threshold int) { if len(occurrences) < 2 { - return candidates + return } for i := 0; i < len(occurrences); i++ { for j := i + 1; j < len(occurrences); j++ { if next, ok := cloneCandidateForPair(occurrences[i], occurrences[j], docs, threshold); ok { - candidates = appendOrMergeCloneCandidate(candidates, next) + addOrMergeCloneCandidate(byPair, next) } } } +} + +func flattenCloneCandidates(byPair map[[2]int][]cloneCandidate) []cloneCandidate { + total := 0 + for _, bucket := range byPair { + total += len(bucket) + } + candidates := make([]cloneCandidate, 0, total) + for _, bucket := range byPair { + candidates = append(candidates, bucket...) + } return candidates } @@ -120,17 +138,23 @@ func cloneCandidateForPair(left cloneOccurrence, right cloneOccurrence, docs []c } func sortCloneCandidates(candidates []cloneCandidate, docs []cloneDocument) { + // Normalize each document path once up front rather than 2-4 times inside + // every comparator call. + slashPaths := make([]string, len(docs)) + for i := range docs { + slashPaths[i] = filepath.ToSlash(docs[i].Path) + } sort.Slice(candidates, func(i, j int) bool { - leftDoc := filepath.ToSlash(docs[candidates[i].LeftDoc].Path) - rightDoc := filepath.ToSlash(docs[candidates[j].LeftDoc].Path) + leftDoc := slashPaths[candidates[i].LeftDoc] + rightDoc := slashPaths[candidates[j].LeftDoc] if leftDoc != rightDoc { return leftDoc < rightDoc } if candidates[i].LeftStart != candidates[j].LeftStart { return candidates[i].LeftStart < candidates[j].LeftStart } - otherLeft := filepath.ToSlash(docs[candidates[i].RightDoc].Path) - otherRight := filepath.ToSlash(docs[candidates[j].RightDoc].Path) + otherLeft := slashPaths[candidates[i].RightDoc] + otherRight := slashPaths[candidates[j].RightDoc] if otherLeft != otherRight { return otherLeft < otherRight } @@ -138,22 +162,21 @@ func sortCloneCandidates(candidates []cloneCandidate, docs []cloneDocument) { }) } -func appendOrMergeCloneCandidate(candidates []cloneCandidate, next cloneCandidate) []cloneCandidate { +func addOrMergeCloneCandidate(byPair map[[2]int][]cloneCandidate, next cloneCandidate) { if next.LeftDoc > next.RightDoc { next.LeftDoc, next.RightDoc = next.RightDoc, next.LeftDoc next.LeftStart, next.RightStart = next.RightStart, next.LeftStart } - for idx, existing := range candidates { - if existing.LeftDoc != next.LeftDoc || existing.RightDoc != next.RightDoc { - continue - } + key := [2]int{next.LeftDoc, next.RightDoc} + bucket := byPair[key] + for idx, existing := range bucket { if cloneRangesOverlap(existing.LeftStart, existing.Length, next.LeftStart, next.Length) && cloneRangesOverlap(existing.RightStart, existing.Length, next.RightStart, next.Length) { if next.Length > existing.Length { - candidates[idx] = next + bucket[idx] = next } - return candidates + return } } - return append(candidates, next) + byPair[key] = append(bucket, next) } diff --git a/internal/codeguard/checks/quality/quality_go.go b/internal/codeguard/checks/quality/quality_go.go index 2fa6672..f8d3349 100644 --- a/internal/codeguard/checks/quality/quality_go.go +++ b/internal/codeguard/checks/quality/quality_go.go @@ -4,7 +4,6 @@ import ( "fmt" "go/ast" "go/format" - "go/parser" "go/token" "path/filepath" "strings" @@ -39,8 +38,7 @@ func goFindingsForFile(env support.Context, file string, data []byte) []core.Fin })) } - fset := token.NewFileSet() - parsed, err := parser.ParseFile(fset, file, data, parser.ParseComments) + fset, parsed, err := support.ParseGoSource(env, file, data) if err != nil { findings = append(findings, env.NewFinding(support.FindingInput{ RuleID: "quality.parse-error", diff --git a/internal/codeguard/checks/security/security_regex_cache.go b/internal/codeguard/checks/security/security_regex_cache.go new file mode 100644 index 0000000..587c0ab --- /dev/null +++ b/internal/codeguard/checks/security/security_regex_cache.go @@ -0,0 +1,25 @@ +package security + +import ( + "regexp" + "sync" +) + +// dynamicPatternCache memoizes regexes compiled from runtime-derived text such +// as import aliases, namespaces, and module names. The same handful of aliases +// and modules recur across many files in a project, so compiling each distinct +// pattern once and reusing it avoids recompiling identical regexes per file. +var dynamicPatternCache sync.Map // map[string]*regexp.Regexp + +// compileDynamicPattern returns the compiled form of expr, reusing a previously +// compiled instance when one exists. The expressions passed here are always +// valid (fixed fragments plus regexp.QuoteMeta-escaped input), so it mirrors the +// regexp.MustCompile contract and panics on a genuinely malformed pattern. +func compileDynamicPattern(expr string) *regexp.Regexp { + if cached, ok := dynamicPatternCache.Load(expr); ok { + return cached.(*regexp.Regexp) + } + compiled := regexp.MustCompile(expr) + actual, _ := dynamicPatternCache.LoadOrStore(expr, compiled) + return actual.(*regexp.Regexp) +} diff --git a/internal/codeguard/checks/security/security_typescript.go b/internal/codeguard/checks/security/security_typescript.go index 1180ffb..5ebfc1b 100644 --- a/internal/codeguard/checks/security/security_typescript.go +++ b/internal/codeguard/checks/security/security_typescript.go @@ -63,7 +63,7 @@ func typeScriptAliasedShellFindings(ctx typeScriptScanContext) []core.Finding { message := support.ScriptLabelForPath(ctx.file) + " shell execution primitive should be reviewed" for alias := range execAliases { - pattern := regexp.MustCompile(`\b` + regexp.QuoteMeta(alias) + `\s*\(`) + pattern := compileDynamicPattern(`\b` + regexp.QuoteMeta(alias) + `\s*\(`) findings = append(findings, regexTypeScriptSecurityFindings(ctx, support.ScriptRegexSpec{Pattern: pattern, RuleID: ruleID, Level: "warn", Message: "shell execution primitive should be reviewed"})...) } for alias := range spawnAliases { @@ -72,7 +72,7 @@ func typeScriptAliasedShellFindings(ctx typeScriptScanContext) []core.Finding { } } for namespace := range childProcessNamespaces { - pattern := regexp.MustCompile(`\b` + regexp.QuoteMeta(namespace) + `\s*\.\s*(?:exec|execSync)\s*\(`) + pattern := compileDynamicPattern(`\b` + regexp.QuoteMeta(namespace) + `\s*\.\s*(?:exec|execSync)\s*\(`) findings = append(findings, regexTypeScriptSecurityFindings(ctx, support.ScriptRegexSpec{Pattern: pattern, RuleID: ruleID, Level: "warn", Message: "shell execution primitive should be reviewed"})...) for _, line := range typeScriptCallLinesWithShellOption(ctx, namespace, true) { findings = append(findings, newTypeScriptSecurityFinding(ctx, ruleID, line, message)) @@ -88,15 +88,15 @@ func typeScriptVMFindings(ctx typeScriptScanContext) []core.Finding { vmNamespaces := collectTypeScriptNamespaceBindings(ctx.source, "vm") for alias, original := range directAliases { - pattern := regexp.MustCompile(`\b` + regexp.QuoteMeta(alias) + `\s*\(`) + pattern := compileDynamicPattern(`\b` + regexp.QuoteMeta(alias) + `\s*\(`) if original == "Script" { - pattern = regexp.MustCompile(`\bnew\s+` + regexp.QuoteMeta(alias) + `\s*\(`) + pattern = compileDynamicPattern(`\bnew\s+` + regexp.QuoteMeta(alias) + `\s*\(`) } findings = append(findings, regexTypeScriptSecurityFindings(ctx, support.ScriptRegexSpec{Pattern: pattern, RuleID: securityRuleID(ctx.file, "vm-dynamic-code"), Level: "warn", Message: "Node vm dynamic code execution should be reviewed"})...) } for namespace := range vmNamespaces { - methodPattern := regexp.MustCompile(`\b` + regexp.QuoteMeta(namespace) + `\s*\.\s*(?:runInContext|runInNewContext|runInThisContext|compileFunction)\s*\(`) - scriptPattern := regexp.MustCompile(`\bnew\s+` + regexp.QuoteMeta(namespace) + `\s*\.\s*Script\s*\(`) + methodPattern := compileDynamicPattern(`\b` + regexp.QuoteMeta(namespace) + `\s*\.\s*(?:runInContext|runInNewContext|runInThisContext|compileFunction)\s*\(`) + scriptPattern := compileDynamicPattern(`\bnew\s+` + regexp.QuoteMeta(namespace) + `\s*\.\s*Script\s*\(`) findings = append(findings, regexTypeScriptSecurityFindings(ctx, support.ScriptRegexSpec{Pattern: methodPattern, RuleID: securityRuleID(ctx.file, "vm-dynamic-code"), Level: "warn", Message: "Node vm dynamic code execution should be reviewed"})...) findings = append(findings, regexTypeScriptSecurityFindings(ctx, support.ScriptRegexSpec{Pattern: scriptPattern, RuleID: securityRuleID(ctx.file, "vm-dynamic-code"), Level: "warn", Message: "Node vm dynamic code execution should be reviewed"})...) } diff --git a/internal/codeguard/checks/security/security_typescript_bindings.go b/internal/codeguard/checks/security/security_typescript_bindings.go index 17c31e6..a51c3ef 100644 --- a/internal/codeguard/checks/security/security_typescript_bindings.go +++ b/internal/codeguard/checks/security/security_typescript_bindings.go @@ -33,7 +33,7 @@ func collectTypeScriptNamedModuleBindings(source string, module string, allowed func collectTypeScriptNamespaceBindings(source string, module string) map[string]struct{} { namespaces := make(map[string]struct{}) for _, pattern := range []*regexp.Regexp{tsNamespaceImportPattern, tsDefaultImportPattern, tsNamespaceRequirePattern} { - re := regexp.MustCompile(strings.ReplaceAll(pattern.String(), "%s", regexp.QuoteMeta(module))) + re := compileDynamicPattern(strings.ReplaceAll(pattern.String(), "%s", regexp.QuoteMeta(module))) for _, match := range re.FindAllStringSubmatch(source, -1) { if len(match) > 1 { namespaces[match[1]] = struct{}{} @@ -46,7 +46,7 @@ func collectTypeScriptNamespaceBindings(source string, module string) map[string func collectTypeScriptBindingSpecs(source string, module string, patterns ...*regexp.Regexp) []string { specs := make([]string, 0) for _, pattern := range patterns { - re := regexp.MustCompile(strings.ReplaceAll(pattern.String(), "%s", regexp.QuoteMeta(module))) + re := compileDynamicPattern(strings.ReplaceAll(pattern.String(), "%s", regexp.QuoteMeta(module))) for _, match := range re.FindAllStringSubmatch(source, -1) { if len(match) > 1 { specs = append(specs, splitTypeScriptBindingSpecs(match[1])...) @@ -88,7 +88,7 @@ func typeScriptCallLinesWithShellOption(ctx typeScriptScanContext, alias string, } else { patternText += `\s*\(` } - pattern := regexp.MustCompile(patternText) + pattern := compileDynamicPattern(patternText) for _, call := range support.FindScriptCalls(ctx.source, ctx.code, pattern) { if !scriptCallHasShellTrue(call.Args) { continue diff --git a/internal/codeguard/checks/support/context.go b/internal/codeguard/checks/support/context.go index 3ff899c..4905694 100644 --- a/internal/codeguard/checks/support/context.go +++ b/internal/codeguard/checks/support/context.go @@ -3,6 +3,7 @@ package support import ( "context" "go/ast" + "go/token" "time" "github.com/devr-tools/codeguard/internal/codeguard/core" @@ -30,6 +31,7 @@ type Context struct { DiffScope func() map[string]core.ChangedLineRanges VisitTargetFiles func(target core.TargetConfig, include func(string) bool, visit func(rel string, data []byte)) ScanTargetFiles func(target core.TargetConfig, sectionID string, include func(string) bool, evaluator func(string, []byte) []core.Finding) []core.Finding + ParseGoFile func(path string, data []byte) (*token.FileSet, *ast.File, error) NewFinding func(FindingInput) core.Finding FinalizeSection func(id string, name string, findings []core.Finding) core.SectionResult PutArtifact func(core.Artifact) diff --git a/internal/codeguard/checks/support/section_helpers.go b/internal/codeguard/checks/support/section_helpers.go index e412464..d3fe064 100644 --- a/internal/codeguard/checks/support/section_helpers.go +++ b/internal/codeguard/checks/support/section_helpers.go @@ -3,11 +3,28 @@ package support import ( "context" "fmt" + "go/ast" + "go/parser" + "go/token" "strings" "github.com/devr-tools/codeguard/internal/codeguard/core" ) +// ParseGoSource returns a Go AST for the given source, reusing the shared +// per-scan parse cache when the runner wired one in (ParseGoFile), and falling +// back to a fresh parse otherwise (e.g. in unit tests that build a Context +// directly). Callers must treat the result as read-only, since a cached tree is +// shared across sections. +func ParseGoSource(env Context, file string, data []byte) (*token.FileSet, *ast.File, error) { + if env.ParseGoFile != nil { + return env.ParseGoFile(file, data) + } + fset := token.NewFileSet() + parsed, err := parser.ParseFile(fset, file, data, parser.ParseComments) + return fset, parsed, err +} + func NormalizedLanguage(language string) string { return strings.ToLower(strings.TrimSpace(language)) } diff --git a/internal/codeguard/runner/checks/checks.go b/internal/codeguard/runner/checks/checks.go index 8b21170..4afb1b2 100644 --- a/internal/codeguard/runner/checks/checks.go +++ b/internal/codeguard/runner/checks/checks.go @@ -3,6 +3,10 @@ package checks import ( "context" "fmt" + "go/ast" + "go/token" + "runtime" + "sync" checkSupport "github.com/devr-tools/codeguard/internal/codeguard/checks/support" "github.com/devr-tools/codeguard/internal/codeguard/core" @@ -10,18 +14,71 @@ import ( runnersupport "github.com/devr-tools/codeguard/internal/codeguard/runner/support" ) +// Build runs every enabled section and returns their results in the fixed +// registry order. Independent sections run concurrently on a worker pool bounded +// by the CPU count; the shared scan cache, artifact store, and file corpus are +// all concurrency-safe, and results are written into position-indexed slots so +// the output order is deterministic regardless of completion order. func Build(ctx context.Context, sc runnersupport.Context) []core.SectionResult { - sections := make([]core.SectionResult, 0, len(sectionRegistry)) + sc = withSynchronizedSectionCallback(sc) checkEnv := buildCheckContext(sc) //nolint:contextcheck // git helpers use a contained timeout; deeper ctx threading is a tracked follow-up + + enabled := make([]sectionDef, 0, len(sectionRegistry)) for _, def := range sectionRegistry { - if !def.enabled(sc) { - continue + if def.enabled(sc) { + enabled = append(enabled, def) } - sections = append(sections, safeRun(def.id, def.name, func() core.SectionResult { - return def.run(ctx, sc, checkEnv) - })) } - return sections + + results := make([]core.SectionResult, len(enabled)) + sem := make(chan struct{}, sectionConcurrency(len(enabled))) + var wg sync.WaitGroup + for i, def := range enabled { + wg.Add(1) + sem <- struct{}{} + go func() { + defer wg.Done() + defer func() { <-sem }() + results[i] = safeRun(def.id, def.name, func() core.SectionResult { + return def.run(ctx, sc, checkEnv) + }) + }() + } + wg.Wait() + return results +} + +// sectionConcurrency bounds the number of sections running at once to the CPU +// count (and never more than there are sections to run). +func sectionConcurrency(sections int) int { + if sections <= 1 { + return 1 + } + limit := runtime.NumCPU() + if limit < 1 { + limit = 1 + } + if limit > sections { + limit = sections + } + return limit +} + +// withSynchronizedSectionCallback wraps the caller's OnSectionComplete streaming +// callback in a mutex so concurrent sections never invoke it simultaneously, +// preserving the single-threaded contract callers were written against. +func withSynchronizedSectionCallback(sc runnersupport.Context) runnersupport.Context { + callback := sc.Opts.OnSectionComplete + if callback == nil { + return sc + } + var mu sync.Mutex + sc.Opts.OnSectionComplete = func(section core.SectionResult) { + mu.Lock() + defer mu.Unlock() + callback(section) + } + return sc } // safeRun executes one check section, recovering from any panic so that a single @@ -85,6 +142,9 @@ func buildCheckContext(sc runnersupport.Context) checkSupport.Context { ScanTargetFiles: func(target core.TargetConfig, sectionID string, include func(string) bool, evaluator func(string, []byte) []core.Finding) []core.Finding { return runnersupport.ScanTargetFiles(sc, target, sectionID, include, evaluator) }, + ParseGoFile: func(path string, data []byte) (*token.FileSet, *ast.File, error) { + return runnersupport.ParseGoFile(sc, path, data) + }, NewFinding: func(input checkSupport.FindingInput) core.Finding { return runnersupport.NewFinding(sc, runnersupport.FindingInput{ RuleID: input.RuleID, diff --git a/internal/codeguard/runner/support/artifacts.go b/internal/codeguard/runner/support/artifacts.go index 94b6a12..0561234 100644 --- a/internal/codeguard/runner/support/artifacts.go +++ b/internal/codeguard/runner/support/artifacts.go @@ -1,14 +1,16 @@ package support import ( - "os" - "path/filepath" "sort" + "sync" "github.com/devr-tools/codeguard/internal/codeguard/core" ) +// ArtifactStore collects the artifacts produced across all sections. Its methods +// are safe for concurrent use so that sections may run in parallel. type ArtifactStore struct { + mu sync.Mutex items map[string]core.Artifact } @@ -20,6 +22,8 @@ func (store *ArtifactStore) Put(artifact core.Artifact) { if store == nil || artifact.ID == "" { return } + store.mu.Lock() + defer store.mu.Unlock() store.items[artifact.ID] = artifact } @@ -27,12 +31,19 @@ func (store *ArtifactStore) Get(id string) (core.Artifact, bool) { if store == nil { return core.Artifact{}, false } + store.mu.Lock() + defer store.mu.Unlock() artifact, ok := store.items[id] return artifact, ok } func (store *ArtifactStore) List() []core.Artifact { - if store == nil || len(store.items) == 0 { + if store == nil { + return nil + } + store.mu.Lock() + defer store.mu.Unlock() + if len(store.items) == 0 { return nil } ids := make([]string, 0, len(store.items)) @@ -49,11 +60,15 @@ func (store *ArtifactStore) List() []core.Artifact { // VisitTargetFiles walks target files like ScanTargetFiles but bypasses the // findings cache, so callers that build cross-file state (such as import -// graphs) always observe every file. +// graphs) always observe every file. It reuses the shared per-scan corpus, so +// files are still walked and read only once across the whole scan. func VisitTargetFiles(sc Context, target core.TargetConfig, include func(string) bool, visit func(rel string, data []byte)) { - files, _ := WalkFiles(target.Path, sc.Cfg.Exclude, include) + files, _ := sc.corpusFiles(target.Path) for _, file := range files { - data, err := os.ReadFile(filepath.Join(target.Path, file)) //nolint:gosec // file enumerated by WalkFiles under target.Path + if !include(file) { + continue + } + data, err := sc.corpusRead(target.Path, file) if err != nil { continue } diff --git a/internal/codeguard/runner/support/cache_helpers.go b/internal/codeguard/runner/support/cache_helpers.go index 6f4c17a..ee1e0d5 100644 --- a/internal/codeguard/runner/support/cache_helpers.go +++ b/internal/codeguard/runner/support/cache_helpers.go @@ -24,10 +24,74 @@ func ConfigFingerprint(cfg core.Config, extras ...string) string { if err != nil { return "" } - prefix := "scanner-version-6|" + strings.Join(extras, "|") + "|" + prefix := "scanner-version-7|" + strings.Join(extras, "|") + "|" return hashBytes(append([]byte(prefix), data...)) } +// SectionConfigHashes builds a fingerprint per config family, hashing only the +// settings that can change that family's per-file findings. This lets an edit to +// one section's rules (or to a finding-irrelevant field such as the output +// format, target names, or the exclude list) reuse cached findings for every +// unaffected section. The rule catalog is included in every family because a +// rule-metadata override can change any finding's level or title through +// NewFinding. The "" key is the conservative all-checks fallback used for any +// section id that sectionConfigFamily does not recognize, so a newly added +// section can never silently serve stale cache entries. +func SectionConfigHashes(cfg core.Config, catalog map[string]core.RuleMetadata, extras ...string) map[string]string { + prefix := "section-config-v1|" + strings.Join(extras, "|") + "|" + checks := cfg.Checks + return map[string]string{ + // quality reads both QualityRules and DesignRules, and its AI-quality + // findings depend on the AI config. + "quality": sectionFingerprint(prefix, "quality", catalog, cfg.AI, checks.QualityRules, checks.DesignRules), + "design": sectionFingerprint(prefix, "design", catalog, checks.DesignRules), + "security": sectionFingerprint(prefix, "security", catalog, checks.SecurityRules), + "prompts": sectionFingerprint(prefix, "prompts", catalog, checks.PromptRules), + "ci": sectionFingerprint(prefix, "ci", catalog, checks.CIRules), + "contracts": sectionFingerprint(prefix, "contracts", catalog, checks.ContractRules), + "": sectionFingerprint(prefix, "all", catalog, cfg.AI, 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 +// unrecognized id maps to "" so it falls back to the all-checks fingerprint. +func sectionConfigFamily(sectionID string) string { + if i := strings.IndexByte(sectionID, '-'); i >= 0 { + sectionID = sectionID[:i] + } + switch sectionID { + case "quality", "design", "security", "prompts", "ci", "contracts": + return sectionID + default: + return "" + } +} + +// sectionConfigHash resolves the fingerprint for a section id, preferring the +// scoped per-family hash and falling back to the all-checks hash, then to the +// legacy ConfigHash for Contexts assembled without SectionConfigHash. +func (sc Context) sectionConfigHash(sectionID string) string { + if sc.SectionConfigHash != nil { + if hash, ok := sc.SectionConfigHash[sectionConfigFamily(sectionID)]; ok { + return hash + } + if hash, ok := sc.SectionConfigHash[""]; ok { + return hash + } + } + return sc.ConfigHash +} + +func sectionFingerprint(prefix string, family string, components ...any) string { + data, err := json.Marshal(components) + if err != nil { + return "" + } + return hashBytes(append([]byte(prefix+family+"|"), data...)) +} + func cloneFindings(findings []core.Finding) []core.Finding { out := make([]core.Finding, len(findings)) copy(out, findings) @@ -38,6 +102,8 @@ func (cache *ScanCache) GetTriageVerdict(contentHash string) (core.AITriageCache if cache == nil { return core.AITriageCacheVerdict{}, false } + cache.mu.Lock() + defer cache.mu.Unlock() verdict, ok := cache.triageVerdict[contentHash] return verdict, ok } @@ -46,6 +112,8 @@ func (cache *ScanCache) PutTriageVerdict(contentHash string, verdict core.AITria if cache == nil || strings.TrimSpace(contentHash) == "" { return } + cache.mu.Lock() + defer cache.mu.Unlock() if cache.triageVerdict == nil { cache.triageVerdict = map[string]core.AITriageCacheVerdict{} } @@ -57,6 +125,8 @@ func (cache *ScanCache) GetNLRuleVerdict(key string) (core.AINLRuleCacheVerdict, if cache == nil { return core.AINLRuleCacheVerdict{}, false } + cache.mu.Lock() + defer cache.mu.Unlock() verdict, ok := cache.nlRuleVerdict[key] return verdict, ok } @@ -65,6 +135,8 @@ func (cache *ScanCache) PutNLRuleVerdict(key string, verdict core.AINLRuleCacheV if cache == nil || strings.TrimSpace(key) == "" { return } + cache.mu.Lock() + defer cache.mu.Unlock() if cache.nlRuleVerdict == nil { cache.nlRuleVerdict = map[string]core.AINLRuleCacheVerdict{} } diff --git a/internal/codeguard/runner/support/cache_io.go b/internal/codeguard/runner/support/cache_io.go index d6d9812..375b128 100644 --- a/internal/codeguard/runner/support/cache_io.go +++ b/internal/codeguard/runner/support/cache_io.go @@ -35,7 +35,12 @@ func LoadScanCache(path string) *ScanCache { } func (cache *ScanCache) Save() error { - if cache == nil || !cache.dirty || strings.TrimSpace(cache.path) == "" { + if cache == nil { + return nil + } + cache.mu.Lock() + defer cache.mu.Unlock() + if !cache.dirty || strings.TrimSpace(cache.path) == "" { return nil } payload := cacheFile{ diff --git a/internal/codeguard/runner/support/cache_types.go b/internal/codeguard/runner/support/cache_types.go index 8d048d2..a2cefa6 100644 --- a/internal/codeguard/runner/support/cache_types.go +++ b/internal/codeguard/runner/support/cache_types.go @@ -1,8 +1,16 @@ package support -import "github.com/devr-tools/codeguard/internal/codeguard/core" +import ( + "sync" + "github.com/devr-tools/codeguard/internal/codeguard/core" +) + +// ScanCache holds the persisted per-file findings cache plus the AI triage and +// natural-language rule verdict caches. Its maps are guarded by mu so sections +// can read and write concurrently while running in parallel. type ScanCache struct { + mu sync.Mutex path string entries map[string]cacheEntry triageVerdict map[string]core.AITriageCacheVerdict @@ -23,4 +31,8 @@ type cacheEntry struct { Findings []core.Finding `json:"findings"` } -const scanCacheVersion = 6 +// scanCacheVersion is bumped whenever the on-disk cache layout or the meaning of +// a stored fingerprint changes, so stale caches are discarded wholesale rather +// than reused with mismatched semantics. v7 introduced per-section config +// fingerprints (see SectionConfigHashes). +const scanCacheVersion = 7 diff --git a/internal/codeguard/runner/support/context.go b/internal/codeguard/runner/support/context.go index 1c35d0a..7d057a8 100644 --- a/internal/codeguard/runner/support/context.go +++ b/internal/codeguard/runner/support/context.go @@ -24,9 +24,17 @@ type Context struct { CustomRules []CompiledCustomRule NLRuntime nlrule.Runtime Cache *ScanCache - ConfigHash string - DiffCommand map[string]diffCommandEnv - cleanup func() + // ConfigHash is the conservative all-checks fingerprint used as a fallback + // when a section has no scoped entry in SectionConfigHash. + ConfigHash string + // SectionConfigHash maps a config "family" (see sectionConfigFamily) to a + // fingerprint of only the settings that can change that family's per-file + // findings, so editing one section's rules no longer invalidates cached + // findings for unrelated sections. The "" key holds the all-checks fallback. + SectionConfigHash map[string]string + DiffCommand map[string]diffCommandEnv + corpus *fileCorpus + cleanup func() } func NormalizeScanOptions(opts core.ScanOptions) core.ScanOptions { @@ -50,16 +58,18 @@ func NewContext(cfg core.Config, opts core.ScanOptions) (Context, error) { runtime := nlrule.NewRuntime(cfg.AI) sc := Context{ - Cfg: cfg, - Opts: opts, - Artifacts: NewArtifactStore(), - Today: time.Now(), - RuleCatalog: ruleCatalog, - CustomRules: customRules, - NLRuntime: runtime, - ConfigHash: ConfigFingerprint(cfg, runtime.Fingerprint()), - DiffCommand: map[string]diffCommandEnv{}, - cleanup: func() {}, + Cfg: cfg, + Opts: opts, + Artifacts: NewArtifactStore(), + Today: time.Now(), + RuleCatalog: ruleCatalog, + CustomRules: customRules, + NLRuntime: runtime, + ConfigHash: ConfigFingerprint(cfg, runtime.Fingerprint()), + SectionConfigHash: SectionConfigHashes(cfg, ruleCatalog, runtime.Fingerprint()), + DiffCommand: map[string]diffCommandEnv{}, + corpus: newFileCorpus(), + cleanup: func() {}, } if strings.TrimSpace(opts.DiffText) != "" { patchedCfg, diffCommand, cleanup, err := MaterializePatchedTargets(cfg, opts.DiffText) @@ -71,6 +81,7 @@ func NewContext(cfg core.Config, opts core.ScanOptions) (Context, error) { sc.DiffCommand = diffCommand sc.cleanup = cleanup sc.ConfigHash = ConfigFingerprint(patchedCfg, runtime.Fingerprint()) + sc.SectionConfigHash = SectionConfigHashes(patchedCfg, ruleCatalog, runtime.Fingerprint()) } if cfg.Baseline.Path != "" { baseline, err := loadBaselineFile(cfg.Baseline.Path) diff --git a/internal/codeguard/runner/support/corpus.go b/internal/codeguard/runner/support/corpus.go new file mode 100644 index 0000000..483ccfa --- /dev/null +++ b/internal/codeguard/runner/support/corpus.go @@ -0,0 +1,145 @@ +package support + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sync" +) + +// fileCorpus memoizes, for the lifetime of a single scan, the expensive work +// that would otherwise be repeated by every check section: the per-target +// directory walk, the individual file reads, and Go AST parses. Every by-value +// copy of Context shares one *fileCorpus, so a file is walked, read, and parsed +// at most once per scan no matter how many sections inspect it. +// +// All methods are safe for concurrent use so that sections can run in parallel. +// Each cached slot carries its own sync.Once, so concurrent callers racing on a +// cold slot compute it exactly once and every caller observes the same result. +type fileCorpus struct { + mu sync.Mutex + targets map[string]*targetListing + reads map[string]*fileRead + asts map[string]*goParse +} + +type targetListing struct { + once sync.Once + files []string + err error +} + +type fileRead struct { + once sync.Once + data []byte + err error +} + +type goParse struct { + once sync.Once + fset *token.FileSet + file *ast.File + err error +} + +func newFileCorpus() *fileCorpus { + return &fileCorpus{ + targets: map[string]*targetListing{}, + reads: map[string]*fileRead{}, + asts: map[string]*goParse{}, + } +} + +// list returns every non-excluded file under root, walking the tree only once +// per target. Callers apply their own include filter to the returned slice; the +// walk itself is identical regardless of the filter, so sharing it is safe. +func (c *fileCorpus) list(root string, excludes []string) ([]string, error) { + key := filepath.Clean(root) + c.mu.Lock() + entry, ok := c.targets[key] + if !ok { + entry = &targetListing{} + c.targets[key] = entry + } + c.mu.Unlock() + + entry.once.Do(func() { + entry.files, entry.err = WalkFiles(root, excludes, includeAll) + }) + return entry.files, entry.err +} + +// read returns the bytes of root/rel, reading each file at most once per scan. +func (c *fileCorpus) read(root string, rel string) ([]byte, error) { + key := filepath.Clean(root) + "\x00" + rel + c.mu.Lock() + entry, ok := c.reads[key] + if !ok { + entry = &fileRead{} + c.reads[key] = entry + } + c.mu.Unlock() + + entry.once.Do(func() { + entry.data, entry.err = os.ReadFile(filepath.Join(root, rel)) //nolint:gosec // rel enumerated by WalkFiles under root + }) + return entry.data, entry.err +} + +// parseGo returns a shared, read-only Go AST for the given source. The cache key +// includes the content hash so patched (diff-mode) content is reparsed rather +// than serving a stale tree. Callers must treat the returned *ast.File and +// *token.FileSet as immutable, which the AST inspection in the check sections +// already does. +func (c *fileCorpus) parseGo(path string, data []byte) (*token.FileSet, *ast.File, error) { + key := path + "\x00" + hashBytes(data) + c.mu.Lock() + entry, ok := c.asts[key] + if !ok { + entry = &goParse{} + c.asts[key] = entry + } + c.mu.Unlock() + + entry.once.Do(func() { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, data, parser.ParseComments) + entry.fset, entry.file, entry.err = fset, file, err + }) + return entry.fset, entry.file, entry.err +} + +func includeAll(string) bool { return true } + +// corpusFiles lists every non-excluded file under root, using the shared +// per-scan corpus when present and falling back to a direct walk otherwise +// (e.g. for a Context assembled in a unit test). +func (sc Context) corpusFiles(root string) ([]string, error) { + if sc.corpus != nil { + return sc.corpus.list(root, sc.Cfg.Exclude) + } + return WalkFiles(root, sc.Cfg.Exclude, includeAll) +} + +// corpusRead returns the bytes of root/rel via the shared per-scan corpus, +// falling back to a direct read when no corpus is attached. +func (sc Context) corpusRead(root string, rel string) ([]byte, error) { + if sc.corpus != nil { + return sc.corpus.read(root, rel) + } + return os.ReadFile(filepath.Join(root, rel)) //nolint:gosec // rel enumerated by WalkFiles under root +} + +// ParseGoFile returns a shared, read-only Go AST for the given source, parsed at +// most once per scan across every section. It falls back to a fresh parse when +// no corpus is attached. +func ParseGoFile(sc Context, path string, data []byte) (*token.FileSet, *ast.File, error) { + if sc.corpus != nil { + return sc.corpus.parseGo(path, data) + } + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, data, parser.ParseComments) + return fset, file, err +} diff --git a/internal/codeguard/runner/support/findings.go b/internal/codeguard/runner/support/findings.go index c440cb0..8208f3e 100644 --- a/internal/codeguard/runner/support/findings.go +++ b/internal/codeguard/runner/support/findings.go @@ -3,7 +3,6 @@ package support import ( "crypto/sha256" "encoding/hex" - "os" "path/filepath" "strconv" "strings" @@ -29,10 +28,13 @@ type fileScanInput struct { } func ScanTargetFiles(sc Context, target core.TargetConfig, sectionID string, include func(string) bool, evaluator func(string, []byte) []core.Finding) []core.Finding { - files, _ := WalkFiles(target.Path, sc.Cfg.Exclude, include) + files, _ := sc.corpusFiles(target.Path) findings := make([]core.Finding, 0) for _, file := range files { - data, err := os.ReadFile(filepath.Join(target.Path, file)) //nolint:gosec // file enumerated by WalkFiles under target.Path + if !include(file) { + continue + } + data, err := sc.corpusRead(target.Path, file) if err != nil { continue } @@ -52,18 +54,27 @@ func cachedFileFindings(sc Context, input fileScanInput, compute func() []core.F if sc.Cache == nil { return compute() } + configHash := sc.sectionConfigHash(input.sectionID) key := cacheKey(input.sectionID, input.target.Path, input.rel) fileHash := hashBytes(input.data) - if entry, ok := sc.Cache.entries[key]; ok && entry.FileHash == fileHash && entry.ConfigHash == sc.ConfigHash { + + sc.Cache.mu.Lock() + entry, ok := sc.Cache.entries[key] + sc.Cache.mu.Unlock() + if ok && entry.FileHash == fileHash && entry.ConfigHash == configHash { return cloneFindings(entry.Findings) } + findings := compute() + + sc.Cache.mu.Lock() sc.Cache.entries[key] = cacheEntry{ FileHash: fileHash, - ConfigHash: sc.ConfigHash, + ConfigHash: configHash, Findings: cloneFindings(findings), } sc.Cache.dirty = true + sc.Cache.mu.Unlock() return findings }