chore(audit): weekly cloud audit — two silent false-clean gates (completion + govulncheck)#90
Open
tzone85 wants to merge 1 commit into
Open
chore(audit): weekly cloud audit — two silent false-clean gates (completion + govulncheck)#90tzone85 wants to merge 1 commit into
tzone85 wants to merge 1 commit into
Conversation
…ning Both defects let a failed check report success, defeating a gate whose entire purpose is to block on that failure. 1. engine/parseGoTestJSON: a test suite that fails to COMPILE was counted as 0 failing. `go build ./...` does not compile _test.go files, so a cross-story break in a test's dependency keeps checkBuild green while `go test -json` emits a build-fail event with no Test field, which the parser skipped. Result: 0/0/0, ShouldRunFixCycle sees no failure, and the completion gate emits REQ_COMPLETED on a mainline whose tests do not build — the exact drift the gate exists to catch. Now build-fail events count as failures. 2. security/Scanner.Run (govulncheck): govulncheck's text output is empty both when it finds nothing AND when it never ran (offline host cannot reach vuln.go.dev, no go.mod, load error), and the exit code was discarded — so a scan that never inspected the code was recorded in `ran` (clean) instead of `failed`, the precise false-clean that list exists to prevent. Now gated on the exit-code contract: 0 (clean) and 3 (vulns found) are successful analyses; any other outcome routes to `failed` with the diagnostic. Also scrub two stray sibling-project name references (CHANGELOG line, test comment). Tests added for both fixes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013iyEhzaabQP6s7yv7mwW9a
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.
Weekly security/quality audit. Fanned out four parallel category sweeps (concurrency, silent failures, event-sourcing wiring, security/name-leaks) plus a second targeted pass. Concurrency, event-sourcing wiring, and the security-code surface (injection, path traversal, SQLi, secret leakage, prompt injection) all came back clean after adversarial verification. Two genuine silent-failure defects survived verification and are fixed here — both are false-clean gates: a check that failed to run reports success, defeating the gate's entire purpose.
Findings fixed
1.
internal/engine/verification_loop.go·parseGoTestJSON(→CompletionGate.Run) — a non-compiling test suite completes greenWhy it's a real bug:
go build ./...does not compile_test.gofiles, socheckBuildstays green when one story removes/renames a symbol that another story's test references.go test -json ./...reports that compile break as abuild-failevent (and a package-levelfail) that carry noTestfield, and the parser skipped every event whereTest == "". So it returnedpassing=0, failing=0;ShouldRunFixCyclesaw no failure andCompletionGate.RunemittedREQ_COMPLETED— on a mainline whose tests do not even build. This is exactly the cross-story drift the gate exists to catch (CLAUDE.md: "completing on a red build is impossible regardless of wiring").Reproduced empirically:
go build ./...→ exit 0;go test -json ./...→{"Action":"build-fail"}+{"Action":"fail","Package":...,"FailedBuild":...}, parser counted 0/0/0.Fix: count
build-failevents as failures; the per-testTest != ""guard is preserved forpass/failso package-level events don't double-count.2.
internal/security/scanners.go·Scanner.Run(govulncheck) — a govulncheck run that never inspected the code is recorded as "clean"Why it's a real bug:
RunScannersmaintains afailedlist precisely so "a tool that failed to inspect the code is distinguishable from one that found nothing." The four JSON scanners honor this (an error corrupts the JSON →Unmarshalfails →failed). But govulncheck is parsed from plain text and its exit code was discarded (out, _ := cmd.CombinedOutput()). When govulncheck cannot run — NXD's offline-first host cannot reachvuln.go.dev, nogo.mod, or a package-load error — it prints a diagnostic, exits non-zero, and emits noVulnerability #lines, soparseGovulncheckreturned(nil, nil)and the scanner landed inran(clean). The security gate then reports the story/repo as scanned-clean for dependency CVEs when govulncheck inspected nothing — the exact false-clean thefailedlist was added to prevent.Reproduced empirically in this very sandbox:
govulncheck ./...→fetching vulnerabilities: ... Forbidden, exit 1, zero findings parsed.Fix: gate on govulncheck's documented exit-code contract (
golang.org/x/vulninternal/scan/errors.go) — exit 0 (no vulns) and 3 (vulns found) are successful analyses; any other outcome (exit 1 load/network/config, exit 2 usage, or a non-exit failure such as a timeout) returns an error soRunScannersrecords it infailedwith the diagnostic. The pureparseGovulncheckparser is unchanged.Also scrubbed (sibling-project name leaks)
CHANGELOG.md— a changelog line named the sibling project; reworded to "upstream" (no functional purpose, public doc).internal/agent/frontend_test.go— a source comment named the sibling; reworded.Not changed (flagged for maintainer decision):
internal/engine/integration_fix_test.godeliberately asserts NXD prompts do not contain the sibling"…req"literal — that literal is the guard against the leak, so removing it would weaken a protective test. The deliberately-keptdocs/history/archive (an ARCHIVED reference file + porting notes) also carries sibling references; deleting maintainer-authored archive docs is out of scope for an autonomous audit and left for a human call.Verification
go build ./...— passgo vet ./...— passgo test ./... -count=1— pass (full suite; includes the 5 new tests:TestParseGoTestJSON_BuildFailCountsAsFailing,TestParseGoTestJSON_CountsTestLevelOnly,TestParseGoTestJSON_AllGreen,TestGovulncheckCompleted,TestScannerFailureDetail)golangci-lint run ./...— 0 issues (had to rebuild the tool with the go1.26 toolchain; the preinstalled binary was built with go1.25 and refuses a go1.26 module)govulncheck ./...— could not run in this environment: the network policy deniesCONNECT vuln.go.dev:443(403), so the vuln DB is unreachable. Fittingly, that offline-failure path is exactly what finding ci: add weekly traffic stats collection workflow #2 fixes. Recommend running govulncheck in CI wherevuln.go.devis reachable.🤖 Generated with Claude Code
https://claude.ai/code/session_013iyEhzaabQP6s7yv7mwW9a
Generated by Claude Code