fix: stop codeguard silently passing on broken scans (high-priority review fixes) by alxxjohn · Pull Request #189 · devr-tools/codeguard

alxxjohn · 2026-10-30T19:21:17Z

Summary

High-priority correctness fixes from a codebase review. The common thread is codeguard silently passing when it should fail. Each fix was developed on its own branch with regression tests, then merged here.

#	Fix	Commit
H1	GitHub Action PR comment never posted. `validateOutput` rejected `github-comment`, and the comment step ran in `github.action_path` (codeguard's own source) instead of the user's workspace. `set +e` swallowed both. The step now scans `$GITHUB_WORKSPACE`, fails with `::error` on an empty report, and takes inputs via `env:` rather than `${{ }}` in `run:`.	`b2c9020`
H2	A missing or unreadable target path passed with exit 0. `files, _ := sc.corpusFiles(target)` dropped walk errors. `validate` and `scan` now reject missing or non-directory targets. Walk errors produce a `scan.target-walk` (fail) diagnostic and read errors a `scan.file-read` (warn) one. New SDK `ValidateTargetPaths`; `ValidateConfig` stays pure.	`00a6a48`
H3	A section panic became a malformed `"warning"` finding and exited 0. It is now a `scan.section.panic` diagnostic (not baselinable or waivable) plus a failing section status, so a crashed section fails the run. Crashed sections now also reach `OnSectionComplete`.	`35349a5`
H4	The per-file findings cache replayed stale cross-file results. Go quality (package index, go.mod/go.work) and Go design (SDK facade) read sibling files the cache key ignored. Evaluators can now declare `FileScanDependencies`, folded into a per-directory digest that is computed only when caching is on. Also fixes the local-abstraction design pass sharing the `"design"` cache id with the per-language passes, which dropped its findings under caching.	`5bed0ad`
H5	The variadic `warnFinding(env, args ...any)` used runtime type assertions across 41 call sites. Migrated to the typed `support.NewWarnFinding(WarnFindingInput{...})`. Self-scan JSON is byte-identical before and after.	`3365396`
H6	The self-scan didn't dogfood `security` or `prompts`. Both are now enabled; triage details are below.	`f7f8423` `ae38101` `87f3fcc` `018089c`
H7	The Security section panicked on a lone-quote log argument (`raw[1:0]` in `scanArgLiterals`), which discarded every security finding. Found while profiling a large TS repo. Without this fix, H3 would turn that into failed scans. Also fixes unterminated literals losing their last character.	`a6b1615`

H6 triage (126 findings)

4 real issues fixed: MCP tests re-exec'd os.Args[0] (now os.Executable()), and an AKIA-shaped literal sat in a production comment.
7 detector false positives fixed in the checks:
- insecure-deserialization now matches masked source.
- The prompts check treated Go source under mcp/ dirs as MCP config.
76 justified exceptions:
- 18 via existing //nolint:gosec directives.
- 12 via inline codeguard:ignore <rule> -- reason.
- 46 via 16 scoped, reasoned config waivers. There is no baseline.
New behaviour to review: a justified //nolint:gosec // reason or #nosec G204 -- reason now suppresses security.shell-execution, and only that rule.
- It requires a non-empty reason.
- It covers its own line, or the next line when the directive sits alone on its line.
- It is documented in docs/checks.md.

Test plan

go build ./..., go vet ./..., gofmt -l cmd internal pkg tests changelog.go
go test ./...: 30 packages pass
go test -race on internal/codeguard/runner/..., tests/support, tests/codeguard, tests/checks
make codeguard-ci: exit 0, now with security and prompts enabled
Regression tests fail before and pass after for H1, H2, H4 and H7
Exercise the Action on a real PR to confirm the sticky comment posts (only action.yml structure is tested)
CI golangci-lint (the local install is broken, so it was not run)

Follow-ups (not in this PR)

The Go taint check misses the chained sink exec.Command(os.Args[1]).Run().
security-secrets diagnostics are lost on a cache hit.
go.work use dirs outside the target are not in the cache digest.
The scan.target-walk diagnostic attaches to every section that finalizes after the failure, including ones that never read files.

🤖 Generated with Claude Code

…WarnFinding The quality and performance sections built warn findings through a variadic warnFinding(env, args ...any) helper that type-asserted each positional argument at runtime. A swapped argument compiled cleanly and panicked mid-scan, which section-panic recovery then downgraded to a warning. Migrate all 41 call sites (29 quality, 12 performance) to the typed support.NewWarnFinding(env, support.WarnFindingInput{...}) builder and delete both variadic helpers. Quality call sites go through a thin typed newQualityWarnFinding wrapper that keeps the aiRuleConfidence lookup in one place, so findings output is unchanged (self-scan JSON is identical before and after apart from version/timestamp). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The comment step ran `codeguard scan -format github-comment`, but config validation rejected github-comment even though the report writer renders it, so the scan printed nothing. The step ran under `set +e`, logged "comment report was empty; skipping", and the workflow stayed green. It also ran from github.action_path, so a working scan would have covered codeguard's own source instead of the caller's repository. - Accept github-comment in output validation; list it (and cyclonedx-json) in the error message, the -format flag help, and the docs. - Run the comment scan from the default working directory ($GITHUB_WORKSPACE), and build the comment helper with `go -C "$GITHUB_ACTION_PATH"`. - Pass inputs to the step through env: instead of interpolating ${{ inputs.* }} into the shell script. - When the report is empty, emit ::error and a stderr message and exit 1. The exit status cannot tell a broken scan from one with findings (both exit 1), so the report being empty is the test. - Add regression tests: config validation, an end-to-end `scan -format github-comment` run, and structural checks on action.yml. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

A recovered section panic was reported as a hand-built finding with level "warning" (not the canonical "warn"), no fingerprint, severity or catalog entry, and a warn section status. A crashing Security section therefore exited 0 and passed CI. SafeRun now reports the crash as an operational fail-level core.Diagnostic (scan.section.panic) on a failing section, for every section: a crashed section checked nothing. A diagnostic, not a finding, because findings can be baselined, waived or confidence-filtered, which would silence the crash on every later run. The user-facing message stays clean; the panic value and a short stack excerpt go in metadata and evidence. The recovered section is also streamed to OnSectionComplete, which it previously skipped. The catalog emission coverage test now also walks runner/checks and holds checks.* ids to the catalog. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Go repositories that gate on gosec via golangci-lint already record each reviewed subprocess call site as `//nolint:gosec // reason` or `#nosec G204 -- reason`. Accept those as inline suppressions of security.shell-execution so the review does not have to be written twice. The translation is deliberately narrow: - Go source only, and a justification containing letters is required - //nolint must list gosec explicitly; #nosec must name G204 - G204 maps to security.shell-execution only; a linter-wide nolint:gosec never suppresses taint, secret, private-key or crypto findings - trailing directives cover their own line, comment-only directives the next line (never both) - suppressed findings carry {"kind":"inline","match":"gosec-justification"} Also document the reason convention for codeguard:ignore and the MCP config files the prompts section governs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Every insecure-deserialization alternative is a call or type name, never a string value, yet it was matched against the raw line. A loader named in a string literal or comment fired: the detector's own gate table, fix-template examples, and Go test fixtures. Match the masked line instead; languages without a masker keep raw matching, and the safe-loader filter still reads the raw line. The string-content rules in the same file (bind-all-interfaces, weak-cipher, ...) cannot be fixed this way because their signal lives in string literals, so the detector's own gate literals and finding text carry line-bound codeguard:ignore directives instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

isMCPConfigFile treated every file under a directory named mcp/ as an MCP config, so MCP server source and its tests became prompt assets: a Go test carrying a `${OPENAI_API_KEY}` diff fixture failed prompts.secret-interpolation. Restrict the directory heuristic to config formats (.json, .jsonc, .yaml, .yml, .toml). The heuristic now also matches a top-level mcp/ directory. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Enable checks.security and checks.prompts in .codeguard/codeguard.yaml and resolve every fail-level finding: - tests/mcp: re-exec the test binary via os.Executable instead of os.Args[0] (clears three security.taint.go fails) - security_fixture_classification.go: spell out the filler-run example instead of committing an AKIA-prefixed literal in a production comment - private-key fixtures and the fix-template example: line-bound codeguard:ignore with reasons; one waiver for the line-number-sensitive scanner fixture that cannot carry an inline directive - triage mock counter: codeguard:ignore security.taint.go with a reason (operator-controlled env path, test instrumentation) - patch.go: codeguard:ignore for the constant-argv git apply call Waivers are scoped per rule and path class: shell-execution and the string-content OWASP rules in tests (mirrors the golangci gosec exclusion for tests/), the same OWASP rules in in-package _test.go files, pattern quotes in docs/, and the deliberately public Devin MCP HTTP example. Secret, private-key and taint rules are never waived by path. local_precision stays off: enabling it adds ~2060 warnings across 53 rules, almost all in production code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

A target path that did not exist walked as empty, produced no findings, and reported every file-driven section as a clean pass with exit 0; `codeguard validate` also accepted it. Walk errors and per-file read errors were dropped (`files, _ :=`, `return nil`, `continue`). - Add config.ValidateTargetPaths (and the SDK ValidateTargetPaths): each target path must be an existing, readable directory. config.Validate and SDK ValidateConfig stay pure. RunWithOptions and `codeguard validate` call the new check, so both now error with a non-zero exit. - Record target walk errors in the shared corpus as an operational fail diagnostic, scan.target-walk. It is not drained like the other corpus diagnostics: every section that finalizes after the failure reports it, so no section that consumed the partial listing passes. - Report files that were listed but could not be read as an operational warn diagnostic, scan.file-read, capped at 20 itemised files plus one summary diagnostic. - Document target path requirements and the scan diagnostics in docs/checks.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The per-file findings cache keyed an entry on section, target, path, the file's own content hash and the section config hash. Several evaluators read more than that file, so editing a sibling or a manifest replayed stale findings for unchanged files: - Go quality: with local_precision on (the default) the mutation resolver sees package types and variables from a package-wide index built from sibling files; quality.dependency-direction resolves import paths from go.mod/go.work. - Go design: design.cmd-through-internal-cli and design.service-import-internal match against the SDK facade, resolved from go.mod and the pkg/ layout. Evaluators can now declare those inputs with checks/support.FileScanDependencies (an in-memory Key, same-directory PackageFiles, target-wide SharedFiles) via ScanTargetFilesWithDependencies. The runner computes one digest per directory once per scan pass, from the shared corpus listing and reads, and stores it as DepsHash on the cache entry. Evaluators that declare nothing store an empty DepsHash, so their entries and hit rate are unchanged. Nothing is computed when the cache is off. Also give the local-abstraction design pass its own cache section id. It scanned the same Go/Python/TS/C++ files as the per-language design pass under "design", so whichever pass ran second replayed the first pass's cached findings, even within one cold cached scan, and the local-abstraction findings disappeared. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # docs/checks.md

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

literalEnd returned len(raw)-1 for an unterminated literal, so an argument span that was just an opening quote (a multi-line template or string continued on the next line) sliced raw[1:0] and panicked. The section-level recovery then discarded every Security finding for the repository; with the section-panic change on this branch it fails the scan instead. Return len(raw) for an unterminated literal so raw[open+1:end] is always valid and also keeps the literal's final character, which was previously dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The codeguard job ran the pinned Marketplace action (v1.10.0), so the self-scan checked each PR with an older binary than the code under review. Enabling the prompts section here tripped a false positive that this same branch fixes (Go source under tests/mcp/ treated as MCP config), failing the job. Build and run the PR's own binary via make codeguard-ci instead, and drop the duplicate direct go test step (make test already runs it). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Resolve ci.yml: keep the in-repo make codeguard-ci self-scan over the Dependabot-bumped Marketplace action pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…aths (#191) ## Summary Three profiled hot paths in the quality section, each prototyped first and then verified to be output-identical. Follow-up to #189. **Self-scan, cache off (Apple M3), measured against the #189 tree:** | | Before | After | |---|---|---| | Wall time | 1.21s | **0.65s** (−46%) | | Instructions retired | 42.9B | **29.5B** (−31%) | | Findings JSON | 87 findings | byte-identical | | # | Change | Measured | Commit | |---|---|---|---| | P1 | **Single-pass TS/JS module-scope detection.** `sourceMutableGlobalFindings` called `scriptSourceLineAtModuleScope` once per line, and each call rescanned the file from the top (O(lines²)). The flags are now computed once per file. | Daintree's 51k-line `tsWorker.js` goes from 232s to 13.2s; a 20k-line JS file from 14.5s to 2.0s | `310df11` | | P2 | **Byte lexer replaces the clone-tokenizer regex.** A 256-entry byte-class table produces the same spans; the regex only ever matches ASCII, so byte scanning is exact on any input, invalid UTF-8 included. Also drops singleton window hashes before sorting, which is provably output-neutral. | 456ms to 22ms per 1.1M tokens (21×); 1.11M allocs to 2 | `1e24245` | | P3 | **Literal pre-gates on the style-profile regexes.** `goPkgErrorsPattern` and others had no literal prefix, so RE2 attempted a match at every byte. Each gate is justified in a comment; `func` names are matched only at line starts. | `goRepoStyleProfile` goes from 212ms to 21ms | `4852024` | ## Equivalence evidence - **P1:** - A 26-file pin test (nested braces, strings, regex literals, `${}` templates, CRLF, JSX) passes on both the old and new implementations. - A temporary 200k random-input fuzz comparison found no mismatch. - **P2:** - The original regex is kept in tests as a reference. - Spans are identical on the whole repo (1.1M tokens), on adversarial inputs (all 256 bytes, broken UTF-8, operator overlaps) and on a 60s fuzz run. - An out-of-tree check covered 39M tokens across 8 languages. - At `clone_token_threshold=40` (1,408 duplicate-code findings) the scans are identical, and so is the Daintree quality scan. - **P3:** - An oracle test compiles the original expressions from the production patterns and compares results across every repo `.go` file plus about 50 edge cases. - It was mutation-checked: three deliberate breaks were each caught. ## Test plan - [x] `go build ./...`, `go vet ./...`, `gofmt -l cmd internal pkg tests changelog.go` - [x] `go test ./...`: 30 packages pass - [x] `make codeguard-ci`: exit 0, no new findings - [x] Self-scan JSON byte-identical to the #189 tree - [ ] CI `golangci-lint` ## Notes - New `...ForTest` seams were added to `internal/codeguard/checks/quality/export_test_helpers.go`, following the existing `SplitIdentifierWordsForTest` convention. - P2's token slice is sized at `len(source)/5+1` instead of an exact count, so it may hold up to ~25% spare capacity (~5 MB on codeguard). A counting pre-pass would make it exact at some lexing cost. - `quality_precision.go` is now 1,082 lines (it was already over the 1,000-line warn threshold). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

alxxjohn and others added 18 commits October 30, 2026 14:29

Merge branch 'refactor/typed-warn-finding' into fix/high-priority

cb53acc

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge branch 'fix/section-panic-handling' into fix/high-priority

a643fc0

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge branch 'fix/missing-target-path' into fix/high-priority

689f81c

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # docs/checks.md

Merge branch 'fix/cache-cross-file-deps' into fix/high-priority

f6431be

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge branch 'fix/action-pr-comment' into fix/high-priority

51e7054

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge branch 'chore/dogfood-security-prompts' into fix/high-priority

14c4ac6

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge origin/main into fix/high-priority

2832252

Resolve ci.yml: keep the in-repo make codeguard-ci self-scan over the Dependabot-bumped Marketplace action pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

alxxjohn merged commit 4104604 into main Oct 30, 2026
16 checks passed

alxxjohn deleted the fix/high-priority branch October 30, 2026 20:05

This was referenced Oct 30, 2026

chore(main): release 1.11.0 #190

Open

perf(quality): cut self-scan time ~46% by fixing three profiled hot paths #191

Merged

Navigation Menu

Repository navigation

fix: stop codeguard silently passing on broken scans (high-priority review fixes) - #189

fix: stop codeguard silently passing on broken scans (high-priority review fixes)#189
alxxjohn merged 18 commits into
mainfrom
fix/high-priority

alxxjohn commented Oct 30, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

alxxjohn commented Oct 30, 2026

Summary

H6 triage (126 findings)

Test plan

Follow-ups (not in this PR)

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant