Repository navigation
Conversation
…d lockfiles `supply_chain.lockfile-drift` is a fail-level rule, and three parsing defects made it report every dependency in a manifest as missing. Yarn Berry lockfiles parsed to zero packages. `parseYarnLockState` only read the Yarn v1 spelling of the version field (`version "2.6.0"`); Yarn 2+ writes `version: 2.6.0`. It now reads both, splits selectors at the first non-scope `@` so Berry's protocol form (`@linear/sdk@npm:^2.6.0`) and classic npm aliases are read correctly, and treats a non-`npm:` protocol (workspace:, link:, portal:, file:, patch:, git/URL) as present-but-not-a-registry-range rather than as a comparable requirement. pnpm's pre-v6 `/name/1.2.3` key shape is read too; it had the same zero-package failure. A zero-package parse was reported as total drift. Every branch of `parseLockfileState` hardcoded `ok = true`, defeating the `parsedAny` guard that exists to stay silent on a lockfile the parser cannot read. Each parser now decides for itself, and a parser that recognised nothing reports nothing. "Empty" is told apart from "unparsed" by the format's own evidence: npm lockfiles self-identify structurally; Cargo/Poetry/uv count as empty only when every substantive line is a known header key, which a truncated or conflict-marked file fails; yarn, go.sum and bun require either a package or a file holding nothing but comments. Python names were compared without PEP 503 normalisation, so a `pyproject.toml` entry for `Jinja2` or `typing_extensions` never matched the `jinja2` / `typing-extensions` entry Poetry writes. Names are now compared through the normalised form for the Python ecosystem only: npm names are case- and separator-sensitive, and Cargo, Go and Conan record the manifest spelling, so normalising those would match the wrong package instead of fixing anything. On two real repositories this takes `lockfile-drift` from 33 findings to 0 (a Yarn Berry monorepo) and from 2 to 0 (a Poetry backend), while a deliberately stale Berry lockfile still reports exactly the one missing dependency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nst binary content Two false-positive sources, both in the shared scan plumbing. 1. Default exclusions were far too narrow. The built-in list was five globs anchored at the target root (`.git/**`, `.gocache/**`, `.gomodcache/**`, `.codeguard/**`, `dist/**`) plus three segment-matched names. Two defects followed: nested build output was scanned (`dist/app.js` was excluded but `services/app/dist/app.js` was not, so every monorepo analyzed its own output), and the common generated trees were not excluded at all. Measured on real repositories: 97% of findings on one Next.js monorepo and 78% on another came from `.next/`, and a Go repo with a `.build/go-cache` tree produced 566 security findings against compiled build-cache objects. The generated/dependency/cache names now live in `isDefaultDependencyOrBuildPath`, so they are segment-matched and depth-independent, and minified bundles plus source maps are skipped by file suffix. Over-exclusion is treated as the equal risk, because a silently skipped directory is reported as a pass. Names that are sometimes real source (`build`, `out`, `bin`, `obj`, `target`, `coverage`, `_build`) are NOT excluded on the name alone: `walkEntryDecision` prunes one only when a sibling manifest corroborates it (`Cargo.toml`/`pom.xml` beside `target/`, a `.csproj` beside `bin/`+`obj/`, `CMakeLists.txt`/`build.gradle` beside `build/`). A Go package named `build`, a Rust crate's `src/bin`, and coverage.py's own `coverage/` source package therefore stay scanned. The unconditional names get an escape hatch: an `exclude` entry prefixed with `!` re-includes matching paths and wins over the built-in defaults. Directory pruning is suppressed when a re-include could match inside the directory, otherwise the negation would be unreachable. 2. `looksBinary` guarded only the secrets pass (one caller). Every other repository-wide detector - weak hash, weak cipher, and the per-language shell-execution, dynamic-code and unsafe-HTML-sink patterns - regexed compiled binaries and reported findings at offsets inside object code. The heuristic moves to `runnersupport.LooksBinary` and is applied in `scanTargetFiles`, the shared per-file findings path, so no detector can opt out by accident. It is deliberately NOT pushed into `corpusRead` or `VisitTargetFiles`: several passes use `VisitTargetFiles` purely to enumerate repository paths, and supply-chain/CI passes legitimately read non-source text (lockfiles, Dockerfiles, YAML, Markdown), so a guard at the read layer would quietly shrink those inventories. Adds `tests/support/runner_support_exclusions_test.go` (table test over `ShouldExclude` covering both the new exclusions and the must-not-exclude cases, plus walk-level tests for the corroborated build dirs, the `!` re-include, and `LooksBinary`) and `tests/security/binary_scan_guard_test.go` (a binary-content file produces no line findings while the identical text still does). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
security.hardcoded-credential bypassed fixture classification entirely: classifyFixtureCandidate short-circuited to `confirmed` on the rule ID before isFixturePath was ever consulted, so a credential-shaped literal in a test file or a documentation placeholder was reported at fail level with high confidence and blocked CI. The rule now participates in classification, with suppression kept strictly separate from path evidence: - A fixture or documentation path DEMOTES a credential finding to warn with medium confidence and an annotated message. It is still a reported, baselinable, SARIF-exported finding — a real leaked key does not stop being real because of the file it was committed in — it just no longer fails CI on a fixture. - Suppression to an informational diagnostic always requires positive evidence about the VALUE: a vendor's own published documentation placeholder (AWS's AKIAIOSFODNN7EXAMPLE and friends, recognized on any path since they are world-readable non-secrets), an EXAMPLE/FAKE/REDACTED marker or repeated filler run in the body, or a credential aimed at a reserved (RFC 2606) or loopback host. - PEM private-key material and high-entropy hits are still never path-demoted. - security_rules.demote_fixture_findings is wired up for the first time (it was documented as inert): setting it false reports every match exactly as the detector produced it. allow_paths remains the only full-suppression lever. Fixture path signals are widened well beyond the four suffixes and three directories they covered. The structure now supports base-name PREFIXES (pytest's dominant test_*.py, conftest.py), case-sensitive PascalCase suffixes for the JVM/.NET conventions (AccountTest.java, AccountTests.cs — matched case-sensitively so Latest.java stays production code), many more dot/snake suffixes (.test.tsx, .spec.js, .stories.tsx, _spec.rb, _test.rs, _test.cc, ...), and segment-matched directories (tests, test, spec, __tests__, __mocks__, mocks, e2e, cypress, stories, ...). Documentation is a separate scope (docs/, examples/, .md/.mdx/.rst/.adoc) that demotes but never suppresses, because a token pasted into a committed README is a real leak vector. Corpus: security.hardcoded-credential goes 8 TP/0 FP to 11 TP/0 FP with precision and recall still 1.000. Two vulnerable fixtures and the TS one used AKIAIOSFODNN7EXAMPLE, i.e. the corpus was scoring AWS's own published example as a true positive; they now use a split-literal AWS-shaped key (folded by the scanner, no contiguous token committed) and the documented placeholders moved to a clean fixture. New fixtures measure the graded outcomes: a production-looking credential in a _test.go and in a test_*.py still fires, loopback/filler/vendor-placeholder values do not, and a bearer token in a .md still fires. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Measured on frozen `git archive HEAD` snapshots of four real Go
repositories (szr, slop, cleanr, merger) with cache disabled and
generated directories excluded. Per-rule totals before -> after:
quality.dependency-direction 631 -> 0
naming.cardinality-mismatch 586 -> 1
defensive.null-assumption 420 -> 0
defensive.fail-open-authorization 14 -> 0
defensive.unsafe-default 4 -> 0
Every hand-inspected finding in the "before" column was a false
positive; the single survivor is `collectCodeOwners(codeOwners string)`,
where the plural name does hold one string of file text.
quality.dependency-direction
`allowsInternalImport` flagged any file outside internal/, cmd/,
tests/, and pkg/codeguard/ that imported a path containing
"/internal/", once per import. Repositories using test/ (singular) had
every external test file flagged, and so did any pkg/<name>/ facade,
because the SDK exemption was hardcoded to codeguard's own path.
The rule now implements Go's actual internal-visibility rule: an
internal tree is importable only by the package rooted at its parent
directory and that package's descendants. The importing package's path
is resolved through the existing goModuleResolver (extended with the
module's directory) and compared against the tree's owner. Same-module
internal imports - the entire measured population - are legal and are
no longer reported; a cross-tree import in a multi-module repository
still is.
Test code is exempted by naming convention across languages rather
than by a hardcoded "tests/" prefix, with ci_rules.allowed_test_paths
consulted as the configured test-path policy for support files that
carry no test-naming marker.
naming.cardinality-mismatch
Two defects. First, `isPluralName` delegated its unit check to
`unitSuffixPattern`, which lists a bare "s" among its alternatives, so
every name ending in "s" looked unit-suffixed and every name not
ending in "s" failed the plural test: the function returned false for
every input, and the plural-name-on-scalar half of the rule had never
fired in the rule's history. Second, the surviving half reduced to
"flag any collection-typed identifier whose name does not end in
s/list/set/map", which fired on byte strings (`payload []byte`), argv
vectors (`command []string`), maps keyed by id
(`index map[string]Ref`), comparison pairs (`left, right []Source`),
Latin plurals (`criteria []string`), and mass nouns
(`evidence map[string]any`).
`isPluralName` now strips a trailing ordinal digit (so `outputs1`
reads as plural) and uses a unit pattern without the bare second
alias. The singular direction is narrowed to the one high-precision
case: the identifier is the element type of the collection it holds
(`user []User`). The newly-live plural direction is limited to
non-numeric, non-boolean scalars, because a plural noun on a number
names a count (`maxLines int`) and on a flag names a mode
(`lineNumbers bool`) - enabling it without those carve-outs added 481
counts and 13 flags and no true positives.
defensive.null-assumption
`nullableParam` treated any pointer-typed parameter as nullable. In
Go, C, C++, and Rust a bare pointer is the pass-by-reference idiom and
carries no nullability information, so the rule fired on out
parameters, accumulators, builders, options structs, and protobuf
request pointers whose Get*() accessors are nil-safe by construction.
Nullability is now taken only from an explicit declaration ("?",
"| null", "| undefined", "| None", Optional, Maybe, std::optional),
which keeps the TypeScript, Python, and C++ true positives. The
message no longer claims the value crossed a boundary, because nothing
established that.
defensive.* confidence calibration
The regex-over-lowercased-body rules in this family rated themselves
medium, with two at high. Both high-rated ones were measured at 100%
false positives and both had a concrete cause:
- fail-open-authorization matched `err != nil { ... return nil }`,
which in Go is the fail-CLOSED idiom when a non-nil error
accompanies it, gated only on the body containing "auth" or
"policy". The nil or true must now be the sole return value;
confidence high -> medium.
- unsafe-default seeded on the bare substring "config" and matched
safety tokens inside unrelated words, so any line naming a Config
type or a ConfigFields key could match. The seed must now be a
config read and the tokens whole words; an environment read keeps
the positional-default and comparison forms, which are real
defaulting syntax. Confidence high -> low.
- null-assumption's residual path is declaration-driven but its guard
detection is still textual: medium -> low.
The AST-driven members of the family (unchecked-type-assertion,
unsafe-numeric-conversion) keep high, which they earn.
goModuleResolver is now concurrency-safe. Its go.mod cache was only ever
reached from the serial AI pipeline; the dependency-direction rule
consults it from inside ScanTargetFiles, which evaluates files on a
worker pool, and an unguarded map write there aborted the process with
"concurrent map writes". The lock is a *sync.Mutex so the by-value
resolver stays copyable for go vet's copylocks check while every copy
shares one lock.
Catalog descriptions and docs/checks.md are updated to state what the
rules now detect; quality.dependency-direction's entry had described
something the implementation never did.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… target TestSecuritySemanticAnalyzerScansNodeModulesWithinTarget asserted that a node_modules tree inside a target is scanned, which scanner hardening deliberately removed: isDefaultDependencyOrBuildPath skips node_modules at any depth and the policy is applied to the logical target root too (see TestWalkFilesAppliesDependencyPolicyToLogicalTargetRoot). The test has been failing at HEAD as a result. Rename it to describe the behavior that is actually intended, give the target a real source file so it is not empty, and assert the finding is absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… entries Captured from a false-positive audit across seven external repositories (23k findings) plus minimal reproductions: - default exclusions were root-anchored and missed most build output, which caused false NEGATIVES as well as positives: a Next.js .next/ tree exhausted the shared corpus budget and suppressed real-source findings - looksBinary guarded only the secrets pass, so every other line scanner regexed compiled binaries - the lockfile parser contract (recognised != found packages), and which ecosystems may normalise package names - credential fixture classification is two axes: path evidence may only demote, value evidence may suppress - ApplyDefaults resolved omitted keys against the init starter config, so house style was global default policy; the config-less scan -folder path still does - isPluralName returned false for every input, so half of naming.cardinality-mismatch had never fired - the defensive.* family's honest confidence line is analysis mechanism, not rule family Corrections to existing entries, both verified: - ScanTargetFiles IS file-parallel (parallel: true, 4 workers); the claim that file-level parallelism was deliberately avoided is stale and led to a concurrent map write that did not reproduce until the third run - golangci-lint needs a go@1.25 GOROOT here; the env -u GOROOT recipe panics against the local go1.27.1 toolchain Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`config.ApplyDefaults` resolved omitted keys against `baseExampleConfig()` —
the same constructor `codeguard init` uses to write its starter config — so
every opinion written for the starter was the silent global default:
- `ci_rules.allowed_test_paths: ["tests/**"]` made `ci.test-file-location`
active at fail level for every repository, hard-failing idiomatic Go
`_test.go`, `*.spec.ts` beside the source, pytest `test_*.py`, and a
`test/` (singular) directory
- `required_workflow_files`, `required_gates`, `workflow_content_rules`
(requiring the literal strings `actions/checkout` and `go test ./...`),
`required_release_files: [".goreleaser.yaml"]` and
`required_automation_paths: ["Makefile"]` enforced codeguard's own release
tooling on everyone, at fail level
- the four `design_rules` layering toggles encoded codeguard's cmd/ ->
internal/cli and pkg/ facade layout, also at fail level.
`design.cmd-through-internal-cli` only ever matches an import containing
"/pkg/codeguard", so it could not fire outside this repository at all
The two roles are now distinct constructors. `baseDefaultConfig()`
(defaults_base*.go) is the conservative baseline `ApplyDefaults` resolves
against: scan mechanics, off-or-scan-mode section activation, and thresholds
with no meaningful unset value. `baseExampleConfig()` (example.go) is that
baseline plus `applyStarterPolicy()` — targets, the recommended section set
and codeguard's CI and layering conventions — so `init` still writes a rich,
opinionated, editable config. Every removed default is off via an empty list
or a false toggle its rule already treats as "not configured", so the
mechanism is intact and nothing was re-levelled. `applyCIDefaults` and
`applyDesignDefaults` now read those toggles from the baseline instead of
hardcoding true.
The config-less `scan -folder` path bypassed `ApplyDefaults` entirely,
resolving through `ExampleConfig()`, so it kept the house style even after
the split. It now builds from `config.DefaultScanConfig`: the baseline plus
`checks.use_recommended_defaults`, which reuses the single existing
definition of the recommended set rather than copying it into the CLI. The
target no longer asserts a language or carries codeguard's own
`entrypoints: [cmd/codeguard]`. The init-only constructor is renamed
`starterConfigForProfile` so a scan cannot fall back to it by accident.
A profile's plain-bool section activations were inert whenever the profile
arrived via `profile:` in a config or `-profile`, because `applyCheckDefaults`
only defaults the `*bool` sections — `enterprise` never enabled supply_chain,
`ai-safe` never enabled prompts/security. `applyProfileSectionActivation`
now promotes only `true`, which is unambiguous only because the thinned
baseline enables nothing. Note the consequence: an explicit
`supply_chain: false` alongside `profile: enterprise` no longer disables the
section, because a plain bool has no unset state; `checks.disabled` is the
escape hatch and is still applied last.
Measured on a snapshot of an ordinary Go repository. Minimal config with only
`checks: {ci: true}` and one target: 143 fail-level findings -> 33. Config-less
`scan -folder` from a directory with no discoverable config: 149 -> 36, with
warn-level findings bit-identical at 1747 and all six sections still running.
The remaining fails are genuine, 33 of them `ci.mutable-deployment-reference`,
which has no config knob.
This repository's own `.codeguard/codeguard.yaml` now states the CI gate and
layering rules it previously inherited. The generated policy-profile
comparison table is rendered from the defaults path instead of the starter, so
its "Baseline" column is the real default policy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ions Two process findings from the false-positive audit: a scan -folder measurement only exercises the no-config fallback when run from a directory with no discoverable config (cwd, not the scanned path, drives discovery), and parallel worktree agents sharing one scratchpad filename clobber each other's prepared commit messages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…per call
Profiling a full scan of seven frozen repository snapshots (pprof, `make
bench-cpu`) put ~45% of CPU inside the regexp engine and showed
`regexp.MustCompile` running *during* the scan, accounting for 32% of every
byte a scan allocated. None of the changes here alter a single finding.
What the profile said, and what changed:
* 23 detectors in `checks/quality` compiled a regex on every call. Patterns
with constant text became package-level vars; the ones that interpolate scan
input (an identifier name) go through a new bounded `cachedPattern` cache
(`quality_pattern_cache.go`). `duplicatedKnowledgeLineIsEnumOrTypeLiteral`
alone — one compile per source line — was half of that cost.
* `naming.cross-layer-inconsistency` was ~15% of scan CPU and ~20% of its
allocations on its own: it asked `containsAnyIdentifierWord` /
`identifierContainsWord` up to 42 questions per identifier and each one
re-split the identifier from scratch. Identifiers are now split once per
file and shared with the other two naming detectors via `sourceIdentifiers`.
* `identifiersInSource` and `splitIdentifierWords` dropped their regexes for
byte scans. Both character classes are pure ASCII, so a byte scan yields the
same matches RE2 does; `splitCamelWords` gained an ASCII path that slices
the input instead of round-tripping through `[]rune`.
* `scriptSourceLineAtModuleScope` split the whole file into a `[]string` on
every call while only ever reading lines up to its target (~10% of scan
allocations). It now walks lines lazily, with the same `\r\n` handling.
* Literal pre-gates where an alternation's `\b`/`(?i)` prefix defeats RE2's
literal-prefix scan — `mixedAbstractionLevel`,
`assignmentLooksLocalAccumulator` — mirroring the secret scanner's
`builtinGatePasses`. `roleSuffixOveruseFinding`'s `(?i)(...)$` became a
suffix test and `environmentBranchLineIsDeploymentDecision`'s
`\b(if|switch|case|when)\b` a word-boundary scan.
* `unknownAbbreviation` rebuilt a 45-entry map from config for every
identifier; the cheap suspicious-shortening test now runs first.
Measured interleaved against a binary built from the merge base (the machine
is shared, so A and B alternate every repetition), median of 7 cold scans:
szr 1.32x slop 1.40x cleanr 1.37x merger 1.53x
sentinel 1.11x guardhouse 1.25x opex 1.02x
The warm-cache path is unchanged (0.97-1.09x, inside the noise): on a cache
hit the evaluators never run, so none of this work happens.
Allocation per cold scan drops ~2.4x: szr 2.48 GB / 35.6M allocs ->
1.34 GB / 8.0M, guardhouse 4.31 GB / 57.2M -> 1.22 GB / 5.6M, sentinel
976 MB / 13.3M -> 261 MB / 1.0M.
Adds `make bench-cpu`, which profiles a full scan of real repository
snapshots (`tests/codeguard/real_repo_scan_benchmark_test.go`). The existing
synthetic full-scan benchmark disables dead code, local precision and the AI
quality passes, which hides exactly the cost this change removes.
Verified byte-identical findings (rule_id, path, line, message) across seven
snapshots before and after. The one exception is `quality.duplicate-code` on
one repository, which already differs between two runs of the *same* binary —
reported separately, not touched here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…scan-precision-and-perf
Merges six false-positive branches plus the quality-section CPU work onto
one branch so main takes a single, verified change.
Two conflicts needed a judgement call, both where the perf branch and the
quality branch had independently hoisted the same inline regexes out of
checks/quality:
- quality_defensive.go: both branches lifted the two fail-open-authorization
patterns that main compiled inline on every call. The perf branch hoisted
them unchanged; the quality branch hoisted AND tightened them so the
returned nil/true must be the sole return value. Kept the quality branch's
patterns - main's loose form matched "if err != nil { return nil, err }",
the fail-CLOSED idiom, which is why that rule measured 14/14 false
positives - and dropped the perf branch's duplicate declarations.
- quality_precision_workstreams_cd.go: took the quality branch's var block,
which supersedes the perf branch's with the isPluralName rewrite's typed
container patterns, keeping the perf branch's file split and identifier
caching.
tests/cli/scan_zero_config_test.go needed a fixture change, not an
assertion change. The fixture was built so its only defects were codeguard's
own conventions; the defaults split correctly stops enforcing those and the
quality branch correctly stops quality.dependency-direction firing on a legal
same-module internal import, so the merged scan found nothing and
TestZeroConfigScanStillEnablesRecommendedSections failed on an empty result.
That assertion is the guard against "fix" the zero-config path by making it
report nothing, so it stays; the fixture gains a genuine non-house-style
defect (an MD5 digest helper) instead.
Verified on the merge: go test ./... 29 packages ok, 0 failures; gofmt
clean; go vet clean; make codeguard-ci 2 pass / 4 warn / 0 fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shared per-scan fileCorpus bounds how much source and how many Go ASTs one scan retains. When a budget was reached it returned an error, and every caller treats a read or parse error as "skip this file" - so exceeding a *cache* budget removed files from the scan and reported fewer findings. The only signal was an info-level diagnostic that the default text report did not render at all. For a security scanner that is a correctness bug: a repository that floods the walk with generated files starves analysis of the real source. It was worse than silent. Which files won the remaining budget depended on goroutine scheduling, because concurrently running sections draw on one shared corpus, so the same frozen tree produced different findings run to run. Three scans of one unchanged 26,305-file target reported 22,921 / 21,941 / 21,498 unique findings - a 6.2% spread, 1,974 fingerprints flapping across 69 rules, not a clone-detector quirk. That breaks baselines (a finding present in two runs of three flaps in and out), makes CI irreproducible, and makes any before/after measurement on a budget-exhausting repository meaningless. The same three runs now report 107,854 / 107,855 / 107,856, with the remaining 343 unstable fingerprints confined to two rules whose own selection is unordered (quality.duplicate-code and smell.data-clump, tracked separately). Cross-file passes took the worst of it. VisitTargetFiles skips any file whose read errors, so clone detection, import graphs and the agent-context inventory were fed a truncated document stream: with a starved budget a 40-file target visited 2 files. Their own budgets were never the trigger; the corpus was. The corpus is a cache, so a miss now degrades instead of refusing: - read() past the retention budget still reads the file and returns its bytes, it just drops the slot. Retention stays bounded exactly as before; the cost of exceeding a budget is CPU and I/O, not findings. Dropping the slot rather than latching a scan-wide "exhausted" flag also keeps admission best-fit, so a later small file can use headroom an oversized one could not. - parseGo() past the AST budget parses without retaining, so AST-driven rules no longer lose files to a full cache. - Reads and parses keep their existing semaphores on the uncached path, so an overflowing scan cannot multiply the number of large buffers in flight. Because nothing is dropped, the file stream equals the walk, which is sorted and single-threaded, and a degraded scan is reproducible rather than merely slower. Three accounting fixes come with it: - The Go AST budget counted SOURCE bytes while retaining trees. A retained ParseComments tree measures ~9x its source (9.0x over this repository, 8.4x over a 13.8k-file module cache), so the nominal 64 MiB budget admitted roughly 576 MiB of live AST. Budgets are now expressed in retained bytes with the expansion factor applied; the new 256 MiB ceiling is a real bound and a reduction on the old effective one. - The budget diagnostic misreported its cause. Once the byte budget latched the exhausted flag, every later miss was recorded as an entry-budget overflow, so a scan that had cached 3,824 files was told its 50,000-entry limit was reached. Each overflow now names the budget that actually tripped. - Retention budgets scale down to a share of an explicit GOMEMLIMIT (1/8 for reads, 1/4 for Go ASTs, with floors). They never scale up: Go does not see a container limit that was not expressed as GOMEMLIMIT, so inferring headroom from host memory would be wrong exactly where memory is tightest. Walk truncation is the one corpus bound that still costs coverage - walkFilesBounded stops at maxScanFileCount and those files reach no check - so it is split out as scan.file-limit at warn level, marked operational so the section reports warn rather than a clean pass it has not earned. The text report now renders warn and fail diagnostics; previously it rendered none, so "the walk stopped early" and "govulncheck could not run" reached a terminal reader not at all. Informational diagnostics stay out of the default report. Findings are unchanged on every repository that stays inside the budgets (seven measured repositories, 46 to 2,755 unique findings each, identical before and after across repeated runs) and on this repository's own self-scan. On the overflowing target the scan now reports ~107,855 unique findings against the old 21,498-22,921, including 2,099 at fail level; spot checks confirm they are ordinary rule matches on real source that was previously never read. Peak RSS is unchanged except where a budget overflows, since reading the previously skipped files costs memory: +35% on one 43k-file repository. Under GOMEMLIMIT the scan degrades predictably - 677 to 194 MiB of RSS from unset down to a 128 MiB limit, +25% wall time, identical findings at every limit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two independent findings-order bugs, both from iterating a Go map and then making a choice that depends on the order. Go randomizes map iteration, so the same input produced different findings between runs. collectCloneCandidates walked the window-hash index as a map and stopped early at maxClonePairComparisons/maxCloneCandidates, so the iteration order decided *which* hashes were examined before the cap. sortCloneCandidates orders the result but cannot recover candidates that were never collected. It now walks the hashes in ascending order; any fixed order is sufficient and the key order is the cheapest available. dataClumpFindings returned the first parameter group with three or more occurrences straight out of a map range, so a file with several qualifying groups reported a different group, line and message each run - and could report a weaker clump than one it had already passed over. It now reports the clump appearing in the most functions, tie-broken by group key so the ordering is total. Measured on a 120-file synthetic repo that saturates the clone cap, five runs each: before, three distinct finding sets (all capped at 2000 duplicate-code findings, differing in which); after, one. On a file with a four-function and a three-function clump, five runs each: before, the weaker clump reported in one run and the stronger in four; after, the stronger every time. Also drops roleSuffixPattern and identifierWordSplitPattern, left unused by this branch's merge of the regex-hoisting and rule-narrowing work, which golangci-lint's unused check flagged. Verified: go test ./... 29 packages ok; golangci-lint run ./... 0 issues; gofmt clean; make codeguard-ci 2 pass / 4 warn / 0 fail; szr and guardhouse full-tree scans byte-identical across three runs each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
performance.go.nested-loop-scan, performance.go.slice-membership-in-loop, performance.go.lock-held-across-blocking-call, performance.cpp.range-for-copy and performance.cpp.flush-in-loop emitted findings with no catalog entry, so the scanner printed a finding and `codeguard explain` then answered "unknown rule". They also carried no metadata, no fix template, no OWASP mapping and no SARIF rule descriptor. All five were already documented in docs/checks.md and already carried weights in performance_score.go; only the catalog was missing them. Descriptions follow the existing docs text, including each rule's documented narrowness, so `explain` tells a reader the same thing the reference does. Catalog-to-implementation was already enforced; nothing enforced the reverse, which is how five rules shipped this way. tests/rules adds that invariant: it walks checks/** for rule-id literals and fails on any that rules.Catalog() does not resolve. Diagnostics are excluded structurally - they are deliberately separate from findings, never eligible for baselines, and correctly have no catalog entry - by matching core.Diagnostic literals rather than by an allowlist that would need updating for each new diagnostic. Prefix fragments used with strings.HasPrefix are excluded by their trailing separator. Confirmed non-vacuous by removing one entry and seeing the test name it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing pkg/codeguard
`support.IsSDKFacadeFile` was `strings.HasPrefix(path, "pkg/codeguard/")`, so
the exemption that keeps `design.service-import-internal` (fail) quiet applied
to exactly one repository on earth: this one. Two fixtures identical but for
the directory name, both doing the legal, idiomatic same-module
`pkg/<name>` -> `internal/` import, measured before this change:
pkg/myapp -> design.service-import-internal (fail)
pkg/codeguard -> no findings
`design.cmd-through-internal-cli` (fail) was the same hardcoding in mirror
image: it required `/pkg/codeguard` in the *imported* path, so it could only
ever fire inside this tree and shipped as a guaranteed no-op for every other
user.
The facade is now a property of the scanned repository:
- `runner/support.SDKFacadeResolver` resolves one `SDKFacade` per target,
memoized for the scan (sections run concurrently). It matches both files
(target-relative directory prefix) and Go import paths (module-qualified
from go.mod, with the old path-segment behaviour as fallback).
- New config key `design_rules.sdk_facade_paths` names the facade package
directories explicitly; a glob tail is trimmed, absolute/escaping paths are
rejected.
- Unset, the facade is discovered: every directory directly under `pkg/`.
Permissive on purpose - within a single Go module `pkg/<anything>`
importing `<module>/internal/...` is legal, idiomatic Go, so treating a
public package as a non-facade costs a fail-level finding on correct code.
- `design.service-import-cmd` loses the facade exemption entirely: a public
package importing `cmd/` is a layering inversion under any layout.
- `checks/change` dropped its `IsSDKFacadeFile` call; it was a strict subset
of the `IsPublicPackageFile` test on the next line.
Measured on frozen snapshots with the four layering toggles on:
merger 513 -> 506 findings (7 fail-level false positives removed)
cleanr 1578 -> 1578 (no pkg/ layout)
slop 1348 -> 1346 (3 fail false positives removed, 1 genuine
design.cmd-through-internal-cli gained: cmd/slop/main.go imports
github.com/devr-tools/slop/pkg/slop)
This repository's own self-scan still passes; `.codeguard/codeguard.yaml` now
states `sdk_facade_paths: [pkg/codeguard]` so the rule stays strict for any
future `pkg/` package that is not the SDK facade.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestSDKRuleMetadataFixTemplatesPopulated asserts every catalog rule carries a populated fix template with a kind; the five entries added in the previous commit had none, so adding them to the catalog broke that invariant. Deterministic for the two mechanical C++ fixes (bind by const reference; move the flush after the loop) and guided for the three Go ones, where the change is a shape rather than an exact rewrite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The flush-in-loop description embedded '\n' as a Go escape, so `codeguard explain` printed a real line break mid-sentence. Describe the character in words instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-checks The per-line detectors in checks/security are (?i)/\b alternations, the shape RE2 cannot accelerate: a leading \b or (?i) defeats its literal-prefix fast path, so the engine walks every line for every pattern of every scanned file. A CPU profile of a full scan across seven real repositories put appendOWASPExtraLineFindings, isInsecureDeserialization and foldConcatenatedLiterals at the top of regexp.MatchString time. Apply the technique the secret scanner already uses (builtinGatePasses): guard each regex with a cheap literal pre-check, since a line holding none of a pattern's required literals cannot match it. Covered: the OWASP-gap line rules (CORS, debug-enabled, bind-all, weak-hash, weak-cipher, insecure deserialization, Dockerfile root), the Go line-scan fallback (which runs on every line of every non-Go file), the Python/Rust/Java/C#/Ruby/C++/TypeScript per-language line rules, and the concatenated-literal fold in the secret scanner. Supporting changes: - sourceLine carries a line's raw and masked forms with a precomputed ASCII flag, so the non-ASCII scan the fold gates need is paid once per line rather than once per pattern. Lines holding a non-ASCII byte always pass a fold gate: Go's (?i) folds s->U+017F and k->U+212A, and admitting those lines costs a wasted regex run where modelling them wrongly would cost a finding. - appendOWASPExtraLineFindings strips a trailing comment only when some gate admits a pattern. The stripped line is a prefix of the raw line, so a gate that rejects the raw line rejects the stripped one too. - appendLanguageLineFindings returns nil instead of preallocating a three-element slice per line, which was ~769 MB of immediately-discarded allocations across the seven-repository scan. The gates must be supersets of the patterns they guard, or a rejected line silently never reaches the regex and the rule goes quiet. Two mechanisms enforce that: gatedPattern owns its *regexp.Regexp so it cannot be evaluated ungated, and newGatedPattern registers every instance, which TestSecurityGatedPatternsAreSupersets walks. That test derives sample lines from each pattern's own regexp/syntax tree -- every alternation branch, every member of a small character class, the zero/one repetition forms of optional elements -- and asserts the gate admits every sample the pattern matches, so a new pattern or branch is checked without anyone maintaining a literal list. The same generator now backs TestBuiltinGateCoversPatterns for the secret scanner's gate, which the architecture notes described but which did not exist. Findings are unchanged: full JSON finding sets (rule id, path, line, column, level, severity, confidence, fingerprint, message) over seven real repositories are byte-identical before and after, and the ground-truth corpus scoreboard holds at 71 TP / 0 FN / 1 FP. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| } | ||
| var words []string | ||
| start := 0 | ||
| for i := 0; i <= len(trimmed); i++ { |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ten branches of false-positive and performance work, merged and verified together so
maintakes one change. Each component branch is also pushed if you'd rather review them separately.Why
A measured false-positive audit across seven unrelated real repositories (23,347 findings) plus a systematic rule-portfolio audit. Every item below was reproduced before it was fixed, and re-measured after.
Correctness
Default exclusions missed nearly all build output. The globs were anchored at the target root, so nested
dist/was scanned and.next,build,target,.venv,__pycache__,coverageand friends were never excluded. On one Next.js monorepo 97% of findings came from.next/. Exclusions are now segment-matched and depth-independent, with corroboration for ambiguous names (build,out,bin,target,coverage) so a Go package namedbuild, a Rust crate'ssrc/binand coverage.py's owncoverage/source package all stay scanned.This also fixed false negatives: the generated-file flood was exhausting the shared corpus budget, and one repo went from 255 real-source findings to 665 once it stopped.
A corpus budget miss was becoming a work skip.
VisitTargetFilescontinued past any read error, so over-budget files were skipped entirely rather than merely uncached — a starved 40-file target visited 2 files, and the truncation point was a scheduling race. Over-budget work is now served uncached: retention stays bounded, nothing is skipped. The Go AST budget was also ~9× miscounted (it capped source bytes while retaining trees, which expand 8.4–9.0×).Binary content reached every line scanner.
looksBinaryguarded only the secrets pass; one compiled binary dropped into a source tree produced 33 phantom security findings. The guard now covers every per-file findings path.security.hardcoded-credentialbypassed fixture classification, firing at fail/high on test fixtures and documentation placeholders. Path evidence now demotes (fail/high → warn/medium, still reported); only value evidence (vendor-documented placeholder, unroutable host) suppresses. A real key in a_test.gois still a leaked key, so path alone never suppresses.supply_chain.lockfile-driftmis-parsed two common formats. Yarn Berry'sversion: 2.6.0was unreadable by a parser expecting classicversion "2.6.0", so every dependency reported as drift at fail level — 33 on one repo. Poetry names were compared without PEP 503 normalisation (Jinja2vsjinja2). A parser that recognises nothing now reports nothing.codeguard init's starter config was also the defaults source, so codeguard's own house style was global default policy:allowed_test_paths: ["tests/**"]hard-failed idiomatic Go, NestJS, pytest andtest/layouts. A minimalchecks: {ci: true}config went 143 fail-level findings → 33; the config-lessscan -folderpath went 149 → 36 with warn-level findings bit-identical.codeguard exempted itself from rules it failed everyone else for.
IsSDKFacadeFilewas literallyHasPrefix(path, "pkg/codeguard/"). Two fixtures identical but for the directory name:pkg/myappgot a fail,pkg/codeguardgot nothing. The facade is now resolved per repository. Its mirror image,design.cmd-through-internal-cli, could only ever fire inside this repo; it is now module-qualified and works everywhere.Findings were nondeterministic in two places. Budget exhaustion made clone detection scheduling-dependent (1992 / 1988 / 1986 across three runs of a frozen tree). Separately, two map-iteration sites chose a result by iteration order: clone candidate selection under its cap, and data-clump selection, which could report a weaker clump than one it had already passed. A 120-file synthetic repo gave three distinct finding sets over five runs before, one after.
Three quality rules were dominated by false positives.
quality.dependency-directionduplicated a compiler-enforced invariant (631 findings, all legal same-module imports);naming.cardinality-mismatch'sisPluralNamereturnedfalsefor every possible input, so half the rule had never fired in its history;defensive.null-assumptiontreated any pointer as nullable. Twodefensive.*rules ratedhighmeasured 100% FP and are recalibrated.Five rules emitted findings the tool could not explain.
codeguard explain performance.go.nested-loop-scanansweredunknown rulefor a finding it had just printed. Catalog→implementation was enforced; nothing enforced the reverse.tests/rulesnow does, excluding diagnostics structurally rather than by allowlist.Performance
Root cause was 23 detectors calling
regexp.MustCompileon every invocation — 32.5% of all bytes allocated was runtime regex compilation, one compiling a regex per source line. Security line scanners are now gated behind literal pre-checks (the technique already proven by the secret scanner):findingsForFile15.88s → 1.28s,appendLanguageLineFindings1044.9 MB → 0.The warm-cache path is unchanged (~1.00×) and that is expected: a cache hit never calls the evaluator.
Verification
go test ./...— 30 packages, 0 failuresgolangci-lint run ./...— 0 issuesmake codeguard-ci— 2 pass, 4 warn, 0 failWorth a look before merging
design.cmd-through-internal-clinow works outside this repo, so projects opting into the layering rules can gain fail findings they have never seen. It added one onslop— verified a true positive (cmd/slop/main.goimportspkg/slopwhileinternal/cli/exists), but it is a policy change in effect.supply_chain: falsealongsideprofile: enterpriseno longer disables the section — a plain bool has no unset state.checks.disabledremains the escape hatch. Making those six fields*boolwould restore a real unset state and is worth doing before release.quality.dependency-directionis recommended for retirement (expected true-positive yield ~0). Narrowed here rather than retired, since that is a product decision.🤖 Generated with Claude Code