diff --git a/CLAUDE.md b/CLAUDE.md index 3ac09a7..22fa724 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -135,6 +135,12 @@ kill rm -f ~/.nxd/nxd.lock ~/.nxd/events.jsonl ~/.nxd/nxd.db ``` +## Current State (2026-06-24) — audit hardening + +- **Planner degenerate-plan guard**: `Planner.plan` (`internal/engine/planner.go`) now rejects an empty story list and any story with an empty id/title *before* emitting REQ_PLANNED/STORY_CREATED, so a requirement can no longer be stranded with nothing to dispatch. Tests: `internal/engine/planner_validation_test.go`. +- **Conflict path quoting fix**: `git.ConflictedFiles` (`internal/git/conflict.go`) uses `git status --porcelain -z` and parses NUL-delimited records, returning raw paths instead of git's octal-escaped `core.quotepath` form. Filenames with spaces/non-ASCII (e.g. `résumé draft.txt`) now flow correctly into SniffBinary/StageFiles/conflict resolver. Test: `internal/git/conflict_quotepath_test.go`. +- **Post-merge integration fixer wired**: `runResume` (`internal/cli/resume.go`) now constructs `engine.NewTechLeadFixer` (Tech-Lead model/stage) and attaches it via `WithMonTechLeadFixer`. The feature + monitor logic existed but the setter was never called, so the post-merge build-validation stage never ran. Source-scan regression test: `internal/cli/resume_wiring_test.go`. + ## Current State (2026-06-24) - **Human-readable acceptance criteria**: `internal/criteria/format.go` (`Format`/`FormatMarkdown`, pure, fully unit-tested) segments a run-on acceptance-criteria blob into discrete items — sentence-splitting that guards abbreviations (`e.g.`) and identifier periods (`WorldState.copy()`) and strips `-`/`*`/`•`/`1.` markers. Surfaces: `nxd review ` prints a `Description:` block + a bulleted `Acceptance Criteria:` block; the web snapshot exposes `acceptance_criteria_items` (story_id → []string) and the dashboard expands a detail row (description + criteria checklist) when a story title is clicked (`storyDetailRow`/`toggleStoryDetail` in `app.js`). The Tech-Lead prompt (`prompts.go`) and planner tool schema (`planner_tools.go`) now require 3-6 intent-first criteria, one per line. diff --git a/internal/cli/resume.go b/internal/cli/resume.go index 9eaeba6..dd8f53f 100644 --- a/internal/cli/resume.go +++ b/internal/cli/resume.go @@ -454,12 +454,24 @@ func runResume(cmd *cobra.Command, args []string) error { conflictResolver = engine.NewConflictResolver(mergerClient, seniorModel.Model, seniorModel.MaxTokens, s.Events) } + // Optional post-merge integration-build validation. After a wave merges, if + // main no longer compiles the Tech Lead diagnoses the cross-story break and + // records a focused fix suggestion. The feature was implemented and + // unit-tested but never wired here, so the stage never ran in production. + var techLeadFixer *engine.TechLeadFixer + if llmClient != nil { + techLead := s.Config.Models.TechLead + fixerClient := metrics.LabelStage(llmClient, "tech_lead") + techLeadFixer = engine.NewTechLeadFixer(fixerClient, techLead.Model, techLead.MaxTokens, s.Events, s.Proj) + } + monitor.Configure( engine.WithMonMemPalace(mp), engine.WithMonBayesianRouter(bayesianRouter), engine.WithMonArtifactStore(artStore), engine.WithMonCodeGraph(cg), engine.WithMonConflictResolver(conflictResolver), + engine.WithMonTechLeadFixer(techLeadFixer), engine.WithMonDryRun(dryRun), // Auto-resume: when a wave completes, dispatch the next ready wave // without waiting for the user to re-run "nxd resume". diff --git a/internal/cli/resume_wiring_test.go b/internal/cli/resume_wiring_test.go new file mode 100644 index 0000000..d437a4c --- /dev/null +++ b/internal/cli/resume_wiring_test.go @@ -0,0 +1,27 @@ +package cli + +import ( + "os" + "strings" + "testing" +) + +// TestResume_WiresTechLeadFixer guards against a dead-wire regression: the +// post-merge integration-build feature (WithMonTechLeadFixer + TechLeadFixer) +// was fully implemented and unit-tested, but runResume never wired the fixer +// into the monitor, so the stage never ran in production. The option's own +// wiring test could not catch this. This test scans the resume source to +// confirm the fixer is actually constructed and attached. +func TestResume_WiresTechLeadFixer(t *testing.T) { + src, err := os.ReadFile("resume.go") + if err != nil { + t.Fatalf("read resume.go: %v", err) + } + code := string(src) + + for _, want := range []string{"NewTechLeadFixer(", "WithMonTechLeadFixer("} { + if !strings.Contains(code, want) { + t.Errorf("resume.go must wire the post-merge integration fixer: missing %q", want) + } + } +} diff --git a/internal/engine/planner.go b/internal/engine/planner.go index 3e72ab3..2ca8f67 100644 --- a/internal/engine/planner.go +++ b/internal/engine/planner.go @@ -255,6 +255,26 @@ architecture and conventions when planning stories.`, profileContext) } } + // Reject a degenerate plan before any event is emitted. An empty story + // list (the LLM returned `[]`) would otherwise emit REQ_PLANNED with no + // stories, stranding the requirement forever with nothing to dispatch. + if len(stories) == 0 { + return PlanResult{}, fmt.Errorf("tech lead returned zero stories — requirement cannot be planned") + } + + // Reject any story missing an id or a title. A blank story object (`{}`) + // from a small model carries no work; auto-assigning it an ID below would + // dispatch an agent against nothing. Validate before the auto-ID fill so an + // empty id is caught rather than masked. + for i, s := range stories { + if strings.TrimSpace(s.ID) == "" { + return PlanResult{}, fmt.Errorf("story %d has an empty id", i) + } + if strings.TrimSpace(s.Title) == "" { + return PlanResult{}, fmt.Errorf("story %s has an empty title", s.ID) + } + } + // Ensure all stories have IDs. Smaller models sometimes omit them. for i, s := range stories { if s.ID == "" { diff --git a/internal/engine/planner_validation_test.go b/internal/engine/planner_validation_test.go new file mode 100644 index 0000000..b144226 --- /dev/null +++ b/internal/engine/planner_validation_test.go @@ -0,0 +1,78 @@ +package engine_test + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/tzone85/nexus-dispatch/internal/config" + "github.com/tzone85/nexus-dispatch/internal/engine" + "github.com/tzone85/nexus-dispatch/internal/llm" + "github.com/tzone85/nexus-dispatch/internal/state" +) + +func newPlannerStores(t *testing.T, dir string) (state.EventStore, state.ProjectionStore) { + t.Helper() + _ = os.WriteFile(filepath.Join(dir, "go.mod"), []byte("module test"), 0644) + eventStore, err := state.NewFileStore(filepath.Join(dir, "events.jsonl")) + if err != nil { + t.Fatalf("create event store: %v", err) + } + t.Cleanup(func() { _ = eventStore.Close() }) + projStore, err := state.NewSQLiteStore(":memory:") + if err != nil { + t.Fatalf("create proj store: %v", err) + } + t.Cleanup(func() { _ = projStore.Close() }) + return eventStore, projStore +} + +// TestPlan_RejectsEmptyStoryList pins the fix for the silent "0 stories" plan: +// when the Tech-Lead LLM returns an empty array, Plan must error instead of +// emitting REQ_PLANNED with no stories (which strands the requirement forever). +func TestPlan_RejectsEmptyStoryList(t *testing.T) { + dir := t.TempDir() + eventStore, projStore := newPlannerStores(t, dir) + + client := llm.NewReplayClient(llm.CompletionResponse{Content: "[]"}) + cfg := config.DefaultConfig() + planner := engine.NewPlanner(client, cfg, eventStore, projStore) + + _, err := planner.Plan(context.Background(), "r-empty", "Build nothing", dir) + if err == nil { + t.Fatal("expected error when the planner returns zero stories") + } + + // No REQ_PLANNED event must have been emitted for a failed plan. + events, lerr := eventStore.List(state.EventFilter{}) + if lerr != nil { + t.Fatalf("list events: %v", lerr) + } + for _, e := range events { + if e.Type == state.EventReqPlanned { + t.Errorf("REQ_PLANNED should not be emitted when planning yields 0 stories") + } + } +} + +// TestPlan_RejectsStoryWithEmptyID pins per-story boundary validation: an empty +// or fieldless story object from the LLM must be rejected, not dispatched +// against nothing. +func TestPlan_RejectsStoryWithEmptyID(t *testing.T) { + dir := t.TempDir() + eventStore, projStore := newPlannerStores(t, dir) + + // One well-formed story and one empty object. + response := `[ + {"id": "s-001", "title": "Real", "description": "d", "acceptance_criteria": "ac", "complexity": 2, "owned_files": ["a.go"]}, + {} + ]` + client := llm.NewReplayClient(llm.CompletionResponse{Content: response}) + cfg := config.DefaultConfig() + planner := engine.NewPlanner(client, cfg, eventStore, projStore) + + if _, err := planner.Plan(context.Background(), "r-emptyid", "Has a blank story", dir); err == nil { + t.Fatal("expected error for a story with an empty id/title") + } +} diff --git a/internal/git/conflict.go b/internal/git/conflict.go index 90baf6a..de6dab4 100644 --- a/internal/git/conflict.go +++ b/internal/git/conflict.go @@ -26,11 +26,18 @@ func StartRebase(worktreePath, upstream string) error { } // ConflictedFiles returns the list of files with unresolved merge conflicts -// in the given worktree. It uses `git status --porcelain` which reliably +// in the given worktree. It uses `git status --porcelain -z` which reliably // detects all unmerged states (UU, AA, DD, AU, UA, DU, UD), unlike // `git diff --diff-filter=U` which can miss some conflict types. +// +// The `-z` flag emits NUL-terminated records with raw (unquoted) paths. +// Default porcelain output quotes and octal-escapes paths containing spaces +// or non-ASCII bytes (e.g. `"r\303\251sum\303\251 draft.txt"`), which would +// flow as a bogus path into SniffBinary/StageFiles and break the conflict +// resolver. Each record is "XYpath"; the path is rec[3:] verbatim — +// no TrimSpace, since a real path may legitimately contain trailing spaces. func ConflictedFiles(worktreePath string) ([]string, error) { - cmd := exec.Command("git", "status", "--porcelain") + cmd := exec.Command("git", "status", "--porcelain", "-z") cmd.Dir = worktreePath out, err := cmd.CombinedOutput() if err != nil { @@ -38,15 +45,15 @@ func ConflictedFiles(worktreePath string) ([]string, error) { } var files []string - for _, line := range strings.Split(string(out), "\n") { - if len(line) < 4 { + for _, rec := range strings.Split(string(out), "\x00") { + if len(rec) < 4 { continue } // Unmerged status codes: UU, AA, DD, AU, UA, DU, UD - xy := line[:2] + xy := rec[:2] if xy == "UU" || xy == "AA" || xy == "DD" || xy == "AU" || xy == "UA" || xy == "DU" || xy == "UD" { - files = append(files, strings.TrimSpace(line[3:])) + files = append(files, rec[3:]) } } return files, nil diff --git a/internal/git/conflict_quotepath_test.go b/internal/git/conflict_quotepath_test.go new file mode 100644 index 0000000..39ca9e6 --- /dev/null +++ b/internal/git/conflict_quotepath_test.go @@ -0,0 +1,64 @@ +package git + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +// TestConflictedFiles_NonASCIIPath verifies that a conflict on a filename with a +// space and a non-ASCII character is returned as its real path — not git's +// default core.quotepath-escaped form (e.g. `"r\303\251sum\303\251 a.txt"`). +// Without the fix, the escaped string flows into SniffBinary/StageFiles and the +// Tech-Lead conflict resolver silently fails. +func TestConflictedFiles_NonASCIIPath(t *testing.T) { + dir := t.TempDir() + helperRun(t, dir, "git", "init") + helperRun(t, dir, "git", "config", "user.email", "test@test.com") + helperRun(t, dir, "git", "config", "user.name", "Test") + // Intentionally leave core.quotepath at its default (true). + + const name = "résumé draft.txt" + write := func(content string) { + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0644); err != nil { + t.Fatalf("write: %v", err) + } + } + + write("initial\n") + helperRun(t, dir, "git", "add", ".") + helperRun(t, dir, "git", "commit", "-m", "init") + mainBranch := helperRun(t, dir, "git", "rev-parse", "--abbrev-ref", "HEAD") + + helperRun(t, dir, "git", "checkout", "-b", "topic") + write("topic change\n") + helperRun(t, dir, "git", "add", ".") + helperRun(t, dir, "git", "commit", "-m", "topic") + + helperRun(t, dir, "git", "checkout", mainBranch) + write("main change\n") + helperRun(t, dir, "git", "add", ".") + helperRun(t, dir, "git", "commit", "-m", "main") + + helperRun(t, dir, "git", "checkout", "topic") + // Trigger the conflict (rebase exits non-zero — that's expected). + rebase := exec.Command("git", "rebase", mainBranch) + rebase.Dir = dir + _, _ = rebase.CombinedOutput() + + files, err := ConflictedFiles(dir) + if err != nil { + t.Fatalf("ConflictedFiles: %v", err) + } + if len(files) != 1 { + t.Fatalf("expected exactly 1 conflicted file, got %d: %#v", len(files), files) + } + if files[0] != name { + t.Errorf("ConflictedFiles returned %q, want the real path %q", files[0], name) + } + // The returned path must resolve to a real file on disk. + if _, statErr := os.Stat(filepath.Join(dir, files[0])); statErr != nil { + t.Errorf("returned path does not resolve to a file: %v", statErr) + } +}