From ec692e057b30e123c96d6e256c5325991b1eedeb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 6 Jul 2026 05:19:19 +0000 Subject: [PATCH] fix: close two silent false-clean gates in completion + security scanning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_013iyEhzaabQP6s7yv7mwW9a --- CHANGELOG.md | 2 +- internal/agent/frontend_test.go | 2 +- internal/engine/verification_loop.go | 18 ++++++- internal/engine/verification_loop_test.go | 65 +++++++++++++++++++++++ internal/security/scanners.go | 53 +++++++++++++++++- internal/security/scanners_test.go | 47 ++++++++++++++++ 6 files changed, 182 insertions(+), 5 deletions(-) create mode 100644 internal/engine/verification_loop_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d2464b0..6a901c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,7 @@ _(no entries yet — open a PR to add a line under the relevant subsection.)_ - Auto-commit before rebase to prevent unstaged changes failure - Graceful failure handling — artifact filter, re-planner guardrails, sub-story validation - Command injection, path traversal, and input validation hardening -- Port 3 VXD fixes — split suffix, gitDiff master, duplicate validation +- Port 3 upstream fixes — split suffix, gitDiff master, duplicate validation ### Improved - Engine test coverage: 66% → 72% diff --git a/internal/agent/frontend_test.go b/internal/agent/frontend_test.go index 95a0d96..5fc20d7 100644 --- a/internal/agent/frontend_test.go +++ b/internal/agent/frontend_test.go @@ -6,7 +6,7 @@ import ( ) // The frontend design brief is injected into the goal prompt only for -// UI-facing stories (ctx.IsFrontend). It is the vxd factory's design skill: +// UI-facing stories (ctx.IsFrontend). It is the factory's design skill: // token-first planning, one signature element, the named anti-pattern looks, // and a non-negotiable accessibility floor. func TestGoalPrompt_FrontendBriefInjectedWhenFlagSet(t *testing.T) { diff --git a/internal/engine/verification_loop.go b/internal/engine/verification_loop.go index 518fc7d..c907399 100644 --- a/internal/engine/verification_loop.go +++ b/internal/engine/verification_loop.go @@ -197,13 +197,27 @@ func parseGoTestJSON(output string) (passing, failing, total int) { Action string `json:"Action"` Test string `json:"Test"` } - if err := json.Unmarshal([]byte(line), &evt); err != nil || evt.Test == "" { + if err := json.Unmarshal([]byte(line), &evt); err != nil { continue } switch evt.Action { case "pass": - passing++ + if evt.Test != "" { + passing++ + } case "fail": + if evt.Test != "" { + failing++ + } + case "build-fail": + // A package whose test binary fails to COMPILE emits a build-fail + // event with no Test (and no Package) field. `go build ./...` does + // not compile _test.go files, so checkBuild stays green and this is + // the only signal that the composed mainline's tests are red — e.g. + // when one story removes a symbol another story's test references. + // Without counting it, parseGoTestJSON returns 0/0/0, ShouldRunFixCycle + // sees no failures, and the completion gate emits REQ_COMPLETED on a + // mainline whose tests do not even build. failing++ } } diff --git a/internal/engine/verification_loop_test.go b/internal/engine/verification_loop_test.go new file mode 100644 index 0000000..ed5b489 --- /dev/null +++ b/internal/engine/verification_loop_test.go @@ -0,0 +1,65 @@ +package engine + +import "testing" + +// TestParseGoTestJSON_BuildFailCountsAsFailing pins the regression where a test +// suite that does not COMPILE was reported 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. If that event is not counted, the completion gate emits +// REQ_COMPLETED on a mainline whose tests do not build. +func TestParseGoTestJSON_BuildFailCountsAsFailing(t *testing.T) { + // Verbatim `go test -count=1 -json ./...` output for a package whose test + // file references an undefined symbol (captured from the real toolchain). + out := `{"ImportPath":"demo [demo.test]","Action":"build-output","Output":"# demo [demo.test]\n"} +{"ImportPath":"demo [demo.test]","Action":"build-output","Output":"./foo_test.go:3:32: undefined: Bar\n"} +{"ImportPath":"demo [demo.test]","Action":"build-fail"} +{"Time":"2026-07-06T05:10:31.594769294Z","Action":"start","Package":"demo"} +{"Time":"2026-07-06T05:10:31.594812561Z","Action":"output","Package":"demo","Output":"FAIL\tdemo [build failed]\n"} +{"Time":"2026-07-06T05:10:31.594817791Z","Action":"fail","Package":"demo","Elapsed":0,"FailedBuild":"demo [demo.test]"} +` + passing, failing, total := parseGoTestJSON(out) + if failing < 1 { + t.Fatalf("build-fail must count as a test failure: got passing=%d failing=%d total=%d", passing, failing, total) + } + if passing != 0 { + t.Errorf("no tests ran, expected 0 passing, got %d", passing) + } + + // The gate must not permit completion on this result. + if !ShouldRunFixCycle(VerificationResult{BuildPasses: true, TestsPassing: passing, TestsFailing: failing, TestsTotal: total}) { + t.Error("ShouldRunFixCycle must return true when the test suite fails to build") + } +} + +// TestParseGoTestJSON_CountsTestLevelOnly guards the existing behavior: only +// per-test pass/fail events are counted, so a package-level pass/fail (Test=="") +// that always accompanies test events does not double-count. +func TestParseGoTestJSON_CountsTestLevelOnly(t *testing.T) { + out := `{"Action":"run","Test":"TestA"} +{"Action":"pass","Test":"TestA"} +{"Action":"run","Test":"TestB"} +{"Action":"fail","Test":"TestB"} +{"Action":"fail","Package":"demo"} +{"Action":"output","Package":"demo","Output":"FAIL\n"} +` + passing, failing, total := parseGoTestJSON(out) + if passing != 1 || failing != 1 || total != 2 { + t.Fatalf("expected 1 passing / 1 failing / 2 total, got %d/%d/%d", passing, failing, total) + } +} + +// TestParseGoTestJSON_AllGreen confirms a fully passing suite still reads clean. +func TestParseGoTestJSON_AllGreen(t *testing.T) { + out := `{"Action":"pass","Test":"TestA"} +{"Action":"pass","Test":"TestB"} +{"Action":"pass","Package":"demo"} +` + passing, failing, _ := parseGoTestJSON(out) + if passing != 2 || failing != 0 { + t.Fatalf("expected 2 passing / 0 failing, got %d/%d", passing, failing) + } + if ShouldRunFixCycle(VerificationResult{BuildPasses: true, TestsPassing: passing, TestsFailing: failing}) { + t.Error("ShouldRunFixCycle must return false for a green suite") + } +} diff --git a/internal/security/scanners.go b/internal/security/scanners.go index b9d6aff..d3cc9c8 100644 --- a/internal/security/scanners.go +++ b/internal/security/scanners.go @@ -5,6 +5,8 @@ import ( "bytes" "context" "encoding/json" + "errors" + "fmt" "log" "os/exec" "path/filepath" @@ -365,12 +367,28 @@ func (s Scanner) Run(ctx context.Context, repoDir string) ([]Finding, error) { return nil, nil } cmd.Dir = repoDir - out, _ := cmd.CombinedOutput() // exit code intentionally ignored; parse output + // Exit code is intentionally ignored for the JSON scanners: they exit + // non-zero when they find issues, and a genuine run failure corrupts the + // JSON so the parser returns an error (→ recorded as failed). govulncheck + // is the exception — its text output is empty both when it finds nothing + // AND when it never ran (offline, no go.mod, load error), so a clean parse + // cannot tell the two apart; we must inspect its exit code (see below). + out, runErr := cmd.CombinedOutput() switch s.Kind { case ScannerGosec: return parseGosec(out, repoDir) case ScannerGovulncheck: + // Exit 0 (no vulnerabilities) and exit 3 (vulnerabilities found) are + // both successful analyses. Any other outcome — exit 1 (load/network/ + // config error, e.g. NXD's offline-first host cannot reach vuln.go.dev), + // exit 2 (usage), or a non-exit failure such as a timeout — means the + // scan never inspected the code. Returning an error here routes it to + // RunScanners' `failed` list instead of masquerading as a clean run, + // which is the whole point of that list. + if !govulncheckCompleted(runErr) { + return nil, fmt.Errorf("govulncheck did not complete (dependency-CVE coverage lost): %s", scannerFailureDetail(out, runErr)) + } return parseGovulncheck(out) case ScannerGitleaks: return parseGitleaks(out, repoDir) @@ -382,3 +400,36 @@ func (s Scanner) Run(ctx context.Context, repoDir string) ([]Finding, error) { return nil, nil } } + +// govulncheckCompleted reports whether a govulncheck invocation actually ran a +// full analysis. govulncheck exits 0 when it finds no vulnerabilities and 3 when +// it does (both successful runs, per golang.org/x/vuln internal/scan/errors.go). +// Every other outcome — exit 1 (package load / network / config error), exit 2 +// (usage), or a non-*exec.ExitError failure such as a context timeout — means it +// never inspected the code and its empty text output must not be read as clean. +func govulncheckCompleted(runErr error) bool { + if runErr == nil { + return true // exit 0: analysis completed, no vulnerabilities + } + var ee *exec.ExitError + if errors.As(runErr, &ee) && ee.ExitCode() == 3 { + return true // exit 3: analysis completed, vulnerabilities found + } + return false +} + +// scannerFailureDetail summarizes why a scanner run failed for logging: the +// first non-empty line of tool output (usually the diagnostic), falling back to +// the exec error string. +func scannerFailureDetail(out []byte, runErr error) string { + sc := bufio.NewScanner(bytes.NewReader(out)) + for sc.Scan() { + if line := strings.TrimSpace(sc.Text()); line != "" { + return line + } + } + if runErr != nil { + return runErr.Error() + } + return "no output" +} diff --git a/internal/security/scanners_test.go b/internal/security/scanners_test.go index b1e8031..886203d 100644 --- a/internal/security/scanners_test.go +++ b/internal/security/scanners_test.go @@ -1,12 +1,59 @@ package security import ( + "fmt" "os" + "os/exec" "path/filepath" "sort" "testing" ) +// exitErr runs `sh -c "exit N"` to obtain a real *exec.ExitError carrying the +// given code — the same error type os/exec returns for a scanner subprocess. +func exitErr(t *testing.T, code int) error { + t.Helper() + err := exec.Command("sh", "-c", fmt.Sprintf("exit %d", code)).Run() + if err == nil { + t.Fatalf("expected non-nil error for exit %d", code) + } + return err +} + +// TestGovulncheckCompleted pins the exit-code contract that keeps a govulncheck +// run which never inspected the code (offline host, no go.mod, load error) from +// being recorded as a clean scan. Exit 0 (no vulns) and 3 (vulns found) are +// successful analyses; exit 1/2 and non-exit failures are coverage loss. +func TestGovulncheckCompleted(t *testing.T) { + if !govulncheckCompleted(nil) { + t.Error("nil error (exit 0, no vulnerabilities) must count as a completed scan") + } + if !govulncheckCompleted(exitErr(t, 3)) { + t.Error("exit 3 (vulnerabilities found) must count as a completed scan") + } + if govulncheckCompleted(exitErr(t, 1)) { + t.Error("exit 1 (load/network/config error) must NOT count as completed — coverage was lost") + } + if govulncheckCompleted(exitErr(t, 2)) { + t.Error("exit 2 (usage error) must NOT count as completed") + } + if govulncheckCompleted(fmt.Errorf("context deadline exceeded")) { + t.Error("a non-exit failure (e.g. timeout) must NOT count as completed") + } +} + +// TestScannerFailureDetail confirms the diagnostic prefers the first non-empty +// output line (the tool's own error) over the bare exec status. +func TestScannerFailureDetail(t *testing.T) { + out := []byte("\n\ngovulncheck: fetching vulnerabilities: Forbidden\nmore\n") + if got := scannerFailureDetail(out, exitErr(t, 1)); got != "govulncheck: fetching vulnerabilities: Forbidden" { + t.Errorf("unexpected detail: %q", got) + } + if got := scannerFailureDetail(nil, fmt.Errorf("boom")); got != "boom" { + t.Errorf("empty output should fall back to the error string, got %q", got) + } +} + func TestDetectLanguages(t *testing.T) { dir := t.TempDir() write := func(name, body string) {