chore(audit): weekly cloud audit — fix 4 fail-open / data-loss defects

CLAUDE.md

@@ -83,7 +83,7 @@ Tier 4: Pause (human intervention required)
 `internal/engine/conflict_resolver.go` uses a 3-tier strategy during rebase:
 
 1. **Binary detection** (`internal/git/conflict.go`): `IsBinaryConflict` runs `git diff --numstat HEAD -- <file>`; if the output is `-\t-\t<path>`, the file is binary. `SniffBinary` is the fallback (null-byte check in first 8 KB) for newly-added unmerged files not yet in HEAD. Binary files are NEVER sent to an LLM.
-2. **Binary policy**: Compiled/oversized files (`server`, `main`, `*.exe`, >500 KB) are removed via `git rm` and emit `STORY_CONFLICT_BINARY_REMOVED`. Smaller binaries are resolved with `git checkout --ours` (story branch version wins) and emit `STORY_CONFLICT_BINARY`.
+2. **Binary policy**: Compiled/oversized files (`server`, `main`, `*.exe`, >500 KB) are removed via `git rm` and emit `STORY_CONFLICT_BINARY_REMOVED`. Smaller binaries are resolved with `git checkout --theirs` (story branch version wins — during a rebase `--theirs` is the replayed story commit, `--ours` is the base) and emit `STORY_CONFLICT_BINARY`. (Previously `--ours`, which during a rebase is the base side — that silently discarded the story's binary change when the base also touched the file.)
 3. **Senior fast-path**: Text conflicts are sent to the Senior model. If the resolved content still contains `<<<<<<<` markers, the result is discarded and the Tech Lead is tried.
 4. **Tech Lead escalation**: Triggered when (a) the Senior fails, (b) resolved content still has conflict markers, or (c) the conflict spans >3 files (integration-level). The Tech Lead prompt includes the requirement title/text, story acceptance criteria, `depends_on` story titles, sibling story titles, and the last 3 `git log` subjects for the file. Emits `STORY_CONFLICT_ESCALATED`.
 

internal/engine/conflict_resolver.go

@@ -151,8 +151,11 @@ func (cr *ConflictResolver) RebaseWithResolution(ctx context.Context, storyID, w
 				continue
 			}
 
-			// Generated lock-file check: resolve deterministically (story branch
-			// version via --ours, then staged by the bulk StageFiles below).
+			// Generated lock-file check: resolve deterministically by keeping the
+			// base side (--ours, which during a rebase is the upstream/base), then
+			// staged by the bulk StageFiles below. Which side wins is immaterial
+			// here — a lock conflict means both sides diverged from the merged
+			// manifest, so the post-merge build regenerates the lock regardless.
 			// Lock files like package-lock.json are huge and machine-generated —
 			// sending them to the LLM blows the pipeline timeout for no benefit;
 			// the post-merge build/QA validates dependencies. This was the root
@@ -463,7 +466,8 @@ func deepMergeJSON(ours, theirs any) any {
 // handleBinaryConflict applies a deterministic policy for binary-file conflicts
 // without invoking the LLM:
 //   - Oversized (>500 KB) or compiled binary names (server, main, *.exe) → git rm
-//   - All others → git checkout --ours (story branch version wins)
+//   - All others → git checkout --theirs (story branch version wins; during a
+//     rebase --theirs is the replayed story commit, --ours is the base)
 // generatedLockFiles are package-manager lock files that are machine-generated
 // and must never be LLM-resolved — they are large and deterministic, so a
 // conflict is resolved by taking one side and letting the build regenerate.
@@ -503,15 +507,21 @@ func (cr *ConflictResolver) handleBinaryConflict(storyID, worktreePath, absPath,
 		return nil
 	}
 
-	// Take the story branch version (--ours during rebase = the branch being rebased).
-	log.Printf("[conflict-resolver] taking --ours for binary %s in %s", file, storyID)
-	cmd := exec.Command("git", "checkout", "--ours", "--", file)
+	// Keep the story branch version. During `git rebase <upstream>` the story
+	// commit is replayed ONTO the base, so the conflict sides are inverted
+	// relative to a merge: --ours = base/upstream, --theirs = the story commit
+	// being replayed (pinned by git.TestConflictSides_OursIsBaseTheirsIsStory,
+	// and matching the text/JSON deterministic paths, which all keep theirs).
+	// Using --ours here silently discarded the story's binary asset change
+	// whenever the base also touched the file — real data loss.
+	log.Printf("[conflict-resolver] taking --theirs (story branch version) for binary %s in %s", file, storyID)
+	cmd := exec.Command("git", "checkout", "--theirs", "--", file)
 	cmd.Dir = worktreePath
 	if out, err := cmd.CombinedOutput(); err != nil {
-		return fmt.Errorf("git checkout --ours %s: %w (%s)", file, err, strings.TrimSpace(string(out)))
+		return fmt.Errorf("git checkout --theirs %s: %w (%s)", file, err, strings.TrimSpace(string(out)))
 	}
 	cr.emitBinaryEvent(storyID, file, state.EventStoryConflictBinary,
-		"binary conflict: took --ours (story branch version)")
+		"binary conflict: took --theirs (story branch version)")
 	return nil
 }
 

internal/engine/conflict_resolver_extended_test.go

@@ -1,6 +1,7 @@
 package engine
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"os"
@@ -665,3 +666,80 @@ func TestBinaryConflict_NoLLMCall(t *testing.T) {
 		t.Error("expected STORY_CONFLICT_BINARY or STORY_CONFLICT_BINARY_REMOVED event to be emitted")
 	}
 }
+
+// TestBinaryConflict_SmallBinaryKeepsStoryVersion pins the data-loss regression:
+// a small (kept) binary asset changed by BOTH the story and the base must resolve
+// to the STORY's version. During a rebase the sides are inverted (--ours=base,
+// --theirs=story), so the previous `git checkout --ours` silently reverted the
+// story's asset change to the base version and the requirement merged with the
+// story's work lost. The resolver must take --theirs.
+func TestBinaryConflict_SmallBinaryKeepsStoryVersion(t *testing.T) {
+	bareDir := filepath.Join(t.TempDir(), "remote.git")
+	if err := exec.Command("git", "init", "--bare", bareDir).Run(); err != nil {
+		t.Fatalf("init bare: %v", err)
+	}
+	cloneDir := filepath.Join(t.TempDir(), "clone")
+	if err := exec.Command("git", "clone", bareDir, cloneDir).Run(); err != nil {
+		t.Fatalf("clone: %v", err)
+	}
+	runGit := func(args ...string) {
+		t.Helper()
+		cmd := exec.Command("git", args...)
+		cmd.Dir = cloneDir
+		if out, err := cmd.CombinedOutput(); err != nil {
+			t.Fatalf("git %v: %v (%s)", args, err, out)
+		}
+	}
+	runGit("config", "user.email", "test@test.com")
+	runGit("config", "user.name", "Test")
+
+	// A small binary asset (null bytes => detected binary) with a non-compiled
+	// name, so it takes the kept-small-binary path, not the git-rm removal path.
+	asset := filepath.Join(cloneDir, "logo.png")
+	base := []byte{0x89, 'P', 'N', 'G', 0x00, 0x01, 0x02}
+	if err := os.WriteFile(asset, base, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	runGit("add", ".")
+	runGit("commit", "-m", "init")
+	runGit("push", "origin", "main")
+
+	// Main modifies the asset.
+	if err := os.WriteFile(asset, append(append([]byte{}, base...), 0xFF), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	runGit("add", ".")
+	runGit("commit", "-m", "main updates logo")
+	runGit("push", "origin", "main")
+
+	// Story branch (from before main's change) makes a DIFFERENT change.
+	storyVersion := append(append([]byte{}, base...), 0xAA, 0xBB)
+	runGit("checkout", "-b", "feature", "HEAD~1")
+	if err := os.WriteFile(asset, storyVersion, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	runGit("add", ".")
+	runGit("commit", "-m", "story updates logo")
+	runGit("fetch", "origin", "main")
+
+	senior := llm.NewReplayClient()   // zero responses — errors if called
+	techLead := llm.NewReplayClient() // binary must never reach an LLM
+	es := newEventStoreForTest(t)
+	cr := NewConflictResolver(senior, "senior-model", techLead, "tl-model", 4096, nil, es)
+
+	if err := cr.RebaseWithResolution(context.Background(), "s-binary-keep", cloneDir, "origin/main"); err != nil {
+		t.Fatalf("RebaseWithResolution: %v", err)
+	}
+	if senior.CallCount() > 0 || techLead.CallCount() > 0 {
+		t.Errorf("binary conflict must not reach an LLM (senior=%d, tl=%d)", senior.CallCount(), techLead.CallCount())
+	}
+
+	got, err := os.ReadFile(asset)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !bytes.Equal(got, storyVersion) {
+		t.Errorf("small binary conflict must keep the STORY version %v, got %v (base was %v+0xFF) — story asset change was lost",
+			storyVersion, got, base)
+	}
+}

internal/engine/verification_loop.go

@@ -154,35 +154,48 @@ func checkTests(repoDir string) (passing, failing, total int) {
 	}
 
 	cmd.Dir = repoDir
-	out, _ := cmd.CombinedOutput()
+	out, runErr := cmd.CombinedOutput()
 	output := string(out)
 
 	if fileExists(filepath.Join(repoDir, "go.mod")) {
-		return parseGoTestJSON(output)
-	}
-
-	// Parse test results (simplified — count PASS/FAIL lines)
-	for _, line := range strings.Split(output, "\n") {
-		if strings.Contains(line, "\"numPassedTests\"") || strings.Contains(line, "PASS:") {
-			passing++
-		}
-		if strings.Contains(line, "\"numFailedTests\"") || strings.Contains(line, "FAIL:") {
-			failing++
+		passing, failing, total = parseGoTestJSON(output)
+	} else {
+		// Parse test results (simplified — count PASS/FAIL lines)
+		for _, line := range strings.Split(output, "\n") {
+			if strings.Contains(line, "\"numPassedTests\"") || strings.Contains(line, "PASS:") {
+				passing++
+			}
+			if strings.Contains(line, "\"numFailedTests\"") || strings.Contains(line, "FAIL:") {
+				failing++
+			}
 		}
-	}
 
-	// Fallback: parse Jest summary line
-	if strings.Contains(output, "Tests:") {
-		for _, line := range strings.Split(output, "\n") {
-			if strings.Contains(line, "Tests:") && strings.Contains(line, "passed") {
-				// Parse "Tests: X failed, Y passed, Z total"
-				_, _ = fmt.Sscanf(line, "Tests: %d failed, %d passed, %d total", &failing, &passing, &total) // partial parse keeps zero counters
-				break
+		// Fallback: parse Jest summary line
+		if strings.Contains(output, "Tests:") {
+			for _, line := range strings.Split(output, "\n") {
+				if strings.Contains(line, "Tests:") && strings.Contains(line, "passed") {
+					// Parse "Tests: X failed, Y passed, Z total"
+					_, _ = fmt.Sscanf(line, "Tests: %d failed, %d passed, %d total", &failing, &passing, &total) // partial parse keeps zero counters
+					break
+				}
 			}
 		}
+		total = passing + failing
+	}
+
+	// Fail closed: a non-zero test-runner exit with no attributable per-test
+	// failure means the suite could not be EVALUATED — a compile error (test
+	// files aren't built by `go build ./...`), a panic before any test ran, a
+	// vet failure, or the runner (jest/vitest) failing to launch. The per-test
+	// parsers see nothing to count and would otherwise report (0 failing) =
+	// green, so the completion gate would emit REQ_COMPLETED on a suite that
+	// never ran. Treat it as a failure so the gate blocks instead.
+	if runErr != nil && failing == 0 {
+		log.Printf("[verify] test runner exited non-zero (%v) with 0 per-test failures — treating as compile/infra failure, not a clean run", runErr)
+		failing = 1
+		total = passing + failing
 	}
 
-	total = passing + failing
 	log.Printf("[verify] tests: %d passing, %d failing, %d total", passing, failing, total)
 	return passing, failing, total
 }
@@ -197,7 +210,21 @@ 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
+		}
+		// Package-level events have an empty Test. Almost all are ignorable
+		// (start/output/pass), but "build-fail" is critical: `go test -json`
+		// emits it when a test binary fails to COMPILE — e.g. a merged story's
+		// _test.go references a symbol another story renamed or removed. Because
+		// `go build ./...` (checkBuild) does NOT compile test files, this is the
+		// ONLY signal that cross-story drift broke the test suite. Counting it as
+		// a failure is what stops the completion gate from reporting a suite that
+		// does not even compile as green.
+		if evt.Test == "" {
+			if evt.Action == "build-fail" {
+				failing++
+			}
 			continue
 		}
 		switch evt.Action {

internal/engine/verification_loop_test.go

@@ -1,6 +1,11 @@
 package engine
 
-import "testing"
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+)
 
 func TestShouldRunFixCycle_WhenTestsFail(t *testing.T) {
 	result := VerificationResult{
@@ -22,3 +27,48 @@ func TestParseGoTestJSONCountsIndividualTests(t *testing.T) {
 		t.Fatalf("expected 1 pass, 1 fail, 2 total; got pass=%d fail=%d total=%d", passing, failing, total)
 	}
 }
+
+// TestParseGoTestJSON_BuildFailIsCounted pins the fail-open fix: a test binary
+// that fails to COMPILE emits a package-level "build-fail" (empty Test) and no
+// per-test events. Previously all such events were skipped, so a non-compiling
+// suite parsed as (0,0,0) = clean. It must now count as failing.
+func TestParseGoTestJSON_BuildFailIsCounted(t *testing.T) {
+	output := `{"ImportPath":"pkg [pkg.test]","Action":"build-output","Output":"# pkg\n"}
+{"ImportPath":"pkg [pkg.test]","Action":"build-output","Output":"./x_test.go:4:8: undefined: Foo\n"}
+{"ImportPath":"pkg [pkg.test]","Action":"build-fail"}
+{"Action":"fail","Package":"pkg","FailedBuild":"pkg [pkg.test]"}`
+	_, failing, _ := parseGoTestJSON(output)
+	if failing == 0 {
+		t.Fatalf("a package build-fail must count as failing, got failing=0 (fail-open)")
+	}
+}
+
+// TestCheckTests_BrokenTestFileIsNotClean is the end-to-end regression for the
+// completion-gate fail-open. Production code builds (go build ./... ignores test
+// files), but a _test.go references an undefined symbol — exactly the cross-story
+// drift the completion gate exists to catch. checkTests must report failing > 0
+// so ShouldRunFixCycle blocks REQ_COMPLETED instead of reporting green.
+func TestCheckTests_BrokenTestFileIsNotClean(t *testing.T) {
+	if _, err := exec.LookPath("go"); err != nil {
+		t.Skip("go toolchain not on PATH")
+	}
+	dir := t.TempDir()
+	write := func(name, content string) {
+		t.Helper()
+		if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil {
+			t.Fatal(err)
+		}
+	}
+	write("go.mod", "module example.com/x\n\ngo 1.26\n")
+	write("main.go", "package x\n\n// Add is real production code and compiles fine.\nfunc Add(a, b int) int { return a + b }\n")
+	// Broken test: references a symbol that does not exist (cross-story drift).
+	write("x_test.go", "package x\n\nimport \"testing\"\n\nfunc TestAdd(t *testing.T) {\n\t_ = NonexistentSymbol\n}\n")
+
+	if !checkBuild(dir) {
+		t.Fatalf("go build ./... must pass — it does not compile test files")
+	}
+	_, failing, _ := checkTests(dir)
+	if failing == 0 {
+		t.Fatalf("a test file that fails to compile must count as failing, got failing=0 (fail-open: REQ_COMPLETED on an un-compilable suite)")
+	}
+}

internal/engine/wiring_test.go

@@ -2309,7 +2309,7 @@ func TestWiring_StoryConflictBinary_ProjectsWithoutError(t *testing.T) {
 	evt := state.NewEvent(state.EventStoryConflictBinary, "conflict-resolver", "s-wiring-bin-1",
 		map[string]any{
 			"file":   "server",
-			"reason": "binary conflict: took --ours (story branch version)",
+			"reason": "binary conflict: took --theirs (story branch version)",
 		})
 
 	if err := store.Project(evt); err != nil {

internal/security/coverage_gaps_test.go

@@ -193,14 +193,23 @@ func TestParseNpmAudit_FallsBackToMapKeyForName(t *testing.T) {
 	}
 }
 
-func TestParseGovulncheck_MalformedLinesSkipped(t *testing.T) {
-	out := []byte("Vulnerability #1 without colon\nVulnerability #2:   \nVulnerability #3: GO-2025-999\n")
-	fs, err := parseGovulncheck(out)
+func TestParseGovulncheckJSON_ImportOnlyFindingsNotCalled(t *testing.T) {
+	// Module- and package-level findings (no function in the top trace frame) are
+	// import-only and must NOT be reported; only the symbol-level (called) finding
+	// counts. A clean handshake (config) with no called vuln is a clean scan.
+	out := []byte(`{"config":{"scanner_name":"govulncheck"}}
+{"finding":{"osv":"GO-2025-111","trace":[{"module":"example.com/dep","package":"example.com/dep"}]}}
+{"finding":{"osv":"GO-2025-111","trace":[{"module":"example.com/dep","package":"example.com/dep","function":"Vulnerable"}]}}
+{"finding":{"osv":"GO-2025-222","trace":[{"module":"example.com/other","package":"example.com/other"}]}}`)
+	fs, sawConfig, err := parseGovulncheckJSON(out)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(fs) != 1 || fs[0].RuleID != "GO-2025-999" {
-		t.Errorf("malformed lines must be skipped, valid ones kept: %+v", fs)
+	if !sawConfig {
+		t.Error("config handshake must be detected")
+	}
+	if len(fs) != 1 || fs[0].RuleID != "GO-2025-111" {
+		t.Errorf("only the called (symbol-level) vuln must be reported: %+v", fs)
 	}
 }