diff --git a/docs/changes/unreleased/1801-senior-dev-ending-no-checks.md b/docs/changes/unreleased/1801-senior-dev-ending-no-checks.md new file mode 100644 index 000000000..4b495440d --- /dev/null +++ b/docs/changes/unreleased/1801-senior-dev-ending-no-checks.md @@ -0,0 +1,13 @@ +--- +kind: fixed +title: senior-dev's ending no longer says the build and tests passed when it ran none +pr: 1801 +surface: [engine, docs] +invalidates: + - "senior-dev ended `submitted a change, and the project's own build and tests passed` whenever its check came back clean, even in a folder with nothing to run. With zero commands it now ends `submitted a change; the project has no build or tests it could find to run`." + - "The run's reason for a zero-command pass, which codeaf appends to the landing note, said `submitted, and its build and tests passed`. It now says `submitted, and found no build or tests to run`." +--- + +A real run on a folder holding only a README printed the pass sentence one line +above `senior-dev observed: the project has no build or tests it could find to run`. +The inner status is still `pass`; only what the ending claims changed. diff --git a/internal/manual/chat/senior-dev.md b/internal/manual/chat/senior-dev.md index 675410b54..805e7168f 100644 --- a/internal/manual/chat/senior-dev.md +++ b/internal/manual/chat/senior-dev.md @@ -381,7 +381,8 @@ When it submits, its submission receipt names the change, for example `across 3 file(s), tree `. In a plain folder it counts changed files but does not produce patch text, so no byte size is shown; a measured patch in a git repository says, for example, `128 bytes across 3 file(s), tree `. The run's last line is its ending -sentence, such as that it submitted a change and the project's own build and tests passed. +sentence, such as `submitted a change, and the project's own build and tests passed`, +which it says only when at least one of their commands ran. **On Windows it is absent**: there is no `/senior-dev` and no `codeaf senior-dev`. Its engine needs a Unix shell, process groups and file locks, so Windows builds leave it out @@ -501,6 +502,32 @@ have found. senior-dev runs them itself on the submitted tree, with the same str settings and time limit, and a non-zero exit fails the check. Naming one never excuses the other. Only the command line sets them; senior-dev's own model cannot. +## senior-dev found no build or tests to run — a folder with only a README, nothing was checked, did it pass + +A folder that does not look like a project — no manifest, no `Makefile`, no +`CMakeLists.txt`, no `test/` folder, only a README or loose files — has no build or +tests for senior-dev to find, so its own check runs no command. Its `verify` step says +`found no build or tests to run`. + +A change it submits there is still finished work, and its ending says only what was +checked: +`finished: submitted a change; the project has no build or tests it could find to run`. +At a shell, the observation printed beneath it says the same: +`senior-dev observed: the project has no build or tests it could find to run`. +It never says the project's build and tests passed when none ran; that sentence, +`submitted a change, and the project's own build and tests passed`, is kept for a run +where at least one of their commands did. + +To the chat the run still counts as passed (see what codeaf does when senior-dev ends): +it reads the change and offers to merge the branch without any check of its own having +run, so look at the change yourself before taking it. + +A folder that does look like a project but has no command senior-dev can find is +different: its check fails with `no build entrypoint could be discovered` (or `test`), +and the change is still handed in (see how senior-dev finds a project's build and +tests). To have a folder checked by something it cannot find, name the check yourself +with `--verify-test` or `--verify-build` on `codeaf senior-dev run`. + ## Can I run senior-dev in a folder that is not a git repo — a plain folder, no git, --in-place, operation not permitted, .Trash Yes. **senior-dev uses git only if it is there.** A folder with no git history — a plain @@ -1204,8 +1231,9 @@ senior-dev had finished but that codeaf closed under before the run was over rea A run ends in one of these ways, and the task's ending says which: - `finished: …` — it submitted a change, and the words after say what its own check of the - project's build and tests found on the frozen tree, passed or not: a change handed in is - finished work, and a check that did not pass is looked into by the chat, not acted on; + project's build and tests found on the frozen tree, passed or not, or that it found none + to run: a change handed in is finished work, and a check that did not pass is looked + into by the chat, not acted on; - `senior-dev did not finish: …` — it ended without submitting. It is not drawn as a fault, what it made is still on its branch, and the chat acts on it (see what codeaf does when senior-dev ends); diff --git a/internal/manual/chat_test.go b/internal/manual/chat_test.go index 8f15437de..1c9955629 100644 --- a/internal/manual/chat_test.go +++ b/internal/manual/chat_test.go @@ -3214,6 +3214,30 @@ func TestGeneralTaskCostPagesNameSeniorDevCeiling(t *testing.T) { } } +// A senior-dev run in a folder with nothing to check ends saying so, where it +// once said the project's build and tests passed. Whoever reads that ending, or +// doubts an older one, asks in these words and must reach the section that says +// what was checked. +func TestSeniorDevNothingToRunQuestionsReachItsSection(t *testing.T) { + for _, asked := range []string{ + "senior-dev says the project has no build or tests it could find to run", + "senior-dev found no build or tests to run", + "my folder only has a README, did senior-dev check its change", + "senior-dev said the build and tests passed but my folder has no tests", + } { + found := false + for _, section := range Chat().Search(asked, DefaultResults) { + if section.Page == "senior-dev" && strings.Contains(section.Title, "found no build or tests to run") { + found = true + break + } + } + if !found { + t.Errorf("%q does not reach the section on a folder with nothing to run", asked) + } + } +} + // TestC13UpdateQuestionsReachTheNewManualSection proves C13. func TestC13UpdateQuestionsReachTheNewManualSection(t *testing.T) { for _, asked := range []string{ diff --git a/internal/seniordev/app/ending_message_test.go b/internal/seniordev/app/ending_message_test.go new file mode 100644 index 000000000..509cd43bf --- /dev/null +++ b/internal/seniordev/app/ending_message_test.go @@ -0,0 +1,101 @@ +//go:build !windows + +package app + +import ( + "context" + "io" + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/delegate" +) + +// A pass is said as passed only when the project's own build and tests ran. +// A real run on a folder holding only a README printed +// +// senior-dev finished: submitted a change, and the project's own build and tests passed +// senior-dev observed: the project has no build or tests it could find to run +// +// two witnesses contradicting each other about one check, because the ending's +// sentence read the status and never the count of commands behind it. +func TestPassEndingSaysOnlyWhatWasChecked(t *testing.T) { + const passed = "submitted a change, and the project's own build and tests passed" + for _, test := range []struct { + name string + data map[string]any + message string + observed string + }{ + { + name: "commands ran", + data: map[string]any{"status": "pass", "verification_commands": 3, "verification_failing": 0}, + message: passed, + observed: "the project's 3 build and test commands all passed", + }, + { + // The same record after it has crossed JSON, which is how codeaf + // holds it once the run has ended. + name: "commands ran, read back from JSON", + data: map[string]any{"status": "pass", "verification_commands": float64(2), "verification_failing": float64(0)}, + message: passed, + observed: "the project's 2 build and test commands all passed", + }, + { + name: "no command ran", + data: map[string]any{"status": "pass", "verification_commands": 0, "verification_failing": 0}, + message: "submitted a change; the project has no build or tests it could find to run", + observed: "the project has no build or tests it could find to run", + }, + } { + t.Run(test.name, func(t *testing.T) { + ending := endingOf(pipelineResult{Status: delegate.StatusPass, Terminal: test.data}) + if ending.Message != test.message { + t.Errorf("message = %q, want %q", ending.Message, test.message) + } + if ending.Observed != test.observed { + t.Errorf("observed = %q, want %q", ending.Observed, test.observed) + } + }) + } +} + +// The same ending from the real road: a submitted change in a folder with +// nothing discoverable to run, through the project's own check and the +// terminal it writes. Neither the sentence nor the longer reason beside it may +// say anything passed. +func TestFolderWithNothingToRunEndsWithoutSayingItPassed(t *testing.T) { + workspace := gitWorkspace(t, map[string]string{"README.md": "Notes about this folder.\n"}) + base := strings.TrimSpace(gitOutput(context.Background(), workspace, "rev-parse", "HEAD")) + runner := newPipeline(cliArgs{}, workspace, pipelineDeps{ + Backend: &soloScriptedBackend{}, Events: newEventWriter(io.Discard), Notes: io.Discard, + }) + defer runner.runtime.Close() + + outcome, err := runner.runSolo(context.Background(), "Add the feature.", base) + if err != nil { + t.Fatal(err) + } + status, reason := soloResultStatus(outcome) + if status != delegate.StatusPass || outcome.Status != "pass" { + t.Fatalf("status = %q (inner %q), want a pass: %#v", status, outcome.Status, outcome.TerminalData) + } + if commands, ok := outcome.TerminalData["verification_commands"]; !ok || wholeNumber(commands) != 0 { + t.Fatalf("verification_commands = %#v, want 0 for a folder with nothing to run", commands) + } + ending := endingOf(pipelineResult{Status: status, Reason: reason, Terminal: outcome.TerminalData}) + if want := "submitted a change; the project has no build or tests it could find to run"; ending.Message != want { + t.Errorf("message = %q, want %q", ending.Message, want) + } + if want := "the project has no build or tests it could find to run"; ending.Observed != want { + t.Errorf("observed = %q, want %q", ending.Observed, want) + } + for name, said := range map[string]string{"message": ending.Message, "reason": ending.Reason} { + if strings.Contains(said, "passed") { + t.Errorf("%s = %q, which says something passed when no command ran", name, said) + } + } + if !strings.Contains(ending.Reason, "found no build or tests to run") { + t.Errorf("reason = %q, want it to say no build or tests were found to run", ending.Reason) + } +} diff --git a/internal/seniordev/app/run.go b/internal/seniordev/app/run.go index c51fba5b0..eedfb9fee 100644 --- a/internal/seniordev/app/run.go +++ b/internal/seniordev/app/run.go @@ -371,13 +371,27 @@ func endingOf(result pipelineResult) delegate.Ending { return ending } +// nothingToCheck is what senior-dev saw of a project whose build and tests it +// could not find, so ran none. The ending's sentence and its observation both +// say it, in these words. +const nothingToCheck = "the project has no build or tests it could find to run" + // messageOf is the ending in one sentence. A run that submitted is said in // terms of what its own check of the project found, which is the fact the // status projects; everything else keeps the reason the run gave. +// +// A PASS WITH NO COMMAND BEHIND IT PASSED NOTHING. A folder that does not look +// like a project — a README and nothing else — has no build or tests to find, +// and its check comes back clean having run zero commands. Saying its build +// and tests passed there would be a claim nothing made, so the sentence says +// what was checked instead. func messageOf(result pipelineResult, data map[string]any) string { inner, _ := data["status"].(string) switch { case result.Status == delegate.StatusPass && inner == "pass": + if commands, checked := data["verification_commands"]; checked && wholeNumber(commands) == 0 { + return "submitted a change; " + nothingToCheck + } return "submitted a change, and the project's own build and tests passed" case result.Status == delegate.StatusPass && inner == "pass-unverified": return "submitted a change, and nothing finished checking it" @@ -410,7 +424,7 @@ func observedOf(data map[string]any) string { case commands > 0: said = append(said, fmt.Sprintf("the project's %d build and test commands all passed", commands)) default: - said = append(said, "the project has no build or tests it could find to run") + said = append(said, nothingToCheck) } if data["suite_dead"] == true { said = append(said, "its test suite could not even start") diff --git a/internal/seniordev/app/solo_ship.go b/internal/seniordev/app/solo_ship.go index 363989132..31ea6a32b 100644 --- a/internal/seniordev/app/solo_ship.go +++ b/internal/seniordev/app/solo_ship.go @@ -86,9 +86,16 @@ func (runner *pipeline) soloShip( // while recording no command at all, so counting commands would call a // project whose suite was never found -- the vacuous-green shape -- a // verified pass. + // A project with nothing discoverable to run passes having run no + // command, and its reason says that rather than that anything passed + // (run.go's messageOf says the same of the ending's sentence). outcome.Status = "pass" + checked := "its build and tests passed" + if len(verification.Commands) == 0 { + checked = "found no build or tests to run" + } endingReason = fmt.Sprintf( - "submitted, and its build and tests passed: %s (%s)", candidate.describe(), candidate.Reason, + "submitted, and %s: %s (%s)", checked, candidate.describe(), candidate.Reason, ) default: // The candidate does not verify. It is still what ships: it is the only