diff --git a/docs/changes/unreleased/1845-explicit-work-orders.md b/docs/changes/unreleased/1845-explicit-work-orders.md new file mode 100644 index 000000000..b4e2a9484 --- /dev/null +++ b/docs/changes/unreleased/1845-explicit-work-orders.md @@ -0,0 +1,29 @@ +--- +kind: fixed +title: automatic task starts require a clear request to act now +pr: 1845 +surface: [chat, engine] +invalidates: + - >- + Questions and product direction could be mistaken for permission to start a + task. Both route judges now distinguish clear polite requests from + feasibility questions, ambiguous questions and future direction. Only a + wake exclusively reporting known stopped results for the current request, + without fresh human input, skips automatic task routing. +--- + +Clear polite requests can start work even when phrased as questions. Questions +about feasibility, ambiguous questions and future direction stay in the +conversation. The stopped-result guard applies only to an exclusively result-driven +wake where every result is known to be stopped for the current request and no fresh +human message was consumed. It makes no classifier calls and admits nothing. + +Every other case keeps normal routing inputs, limits and rulings: fresh human messages +(including acknowledgements during a wake), mixed results or attempts, different +requests, background news and missing provenance. Mixed wakes have no new guarantee +that stopped work will stay excluded. Restoring a saved task preserves its stopped +flag; the guard still requires provenance on the delivered result. Ordinary recovery +notices without that provenance follow normal routing. + +The authorization and stopped-result rules each have their own searchable manual +section, so retrieval returns the rule instead of truncating it inside a longer topic. diff --git a/internal/manual/chat/tasks.md b/internal/manual/chat/tasks.md index fd7ad5a2f..a5a71be26 100644 --- a/internal/manual/chat/tasks.md +++ b/internal/manual/chat/tasks.md @@ -616,9 +616,28 @@ partial block disappears because that attempt's half-arrived call was discarded; proposal fragment starts a new block. If the call is refused before it ever becomes a question, it settles as `not started · the call was refused` instead of moving forever. +## Will a polite request start a task — feasibility, explanation, suggestions and future direction + +**Automatic task routing needs a clear request to do work now.** An imperative or a +polite question can authorize work: "Could you fix the failing tests?" and "Can you +implement headless screenshots and add tests?" both ask codeaf to act. They remain +eligible for normal routing; a small job that takes only a few tool calls stays here. + +A feasibility question, such as "Is headless screenshot capture possible on CI?", or an +explanation request, such as "How would headless screenshots work?", stays in chat. +"Can we get screenshots without a display?" is ambiguous without context, so codeaf +answers or asks what you mean before starting. "We should support screenshots eventually" +is future direction, not authorization. Suggestions, goals and plans remain discussion +until you clearly ask codeaf to act now. The intended action and timing matter, not +whether the sentence ends in a question mark. + +Both route readings follow this rule: the one alongside your message and the one after +a reply in words alone. Both also require the work to be worth handing off, a substantial +message of at least six words, and two models agreeing. Ambiguous messages stay in chat. + ## Why a task started on its own — codeaf started work I did not ask for, this looked like work, so task N started -**Sometimes work starts without you asking for it, and you are told after.** After a turn +**Sometimes codeaf starts a task without you explicitly asking for a task, and tells you after.** After a turn that answered a substantial message in **words alone** — no tool call — a cheap model reads what you asked and the first two lines of the reply, and decides one thing: should that have been work? The same judge also starts reading your message **the moment you send it**, @@ -627,7 +646,8 @@ alongside the reply rather than in front of it, which is its own section below. **A yes is asked twice.** The cheap model only screens — it reads every substantial message and every wordy turn, which is why it is cheap — and it cannot start anything on its own. Before a task exists, the same question is put once more to your **planner** model, in the same words, with none of the -first answer in front of it. Only if both say yes does the work start, and then one dim line +first answer in front of it. Both must recognize a clear request to do work now, and both +must agree the work belongs on a task. Only then does the work start, and one dim line goes into the transcript: ``` @@ -686,6 +706,25 @@ type disarms it for the session. What bounds it is the list above, and the stop. Nodes cut by an adaptive run also appear as rows under the run's own row — see *Adaptive runs*, which explains what those rows can and cannot do. +## Can a stopped task start again — a result wake and a fresh human request + +When a conversation wakes exclusively to report task results, every result is known to +be stopped for the current request, and no fresh human message was consumed in that turn, +answering that result does not start the same request again. Neither route reading runs. +This includes a stopped task restored from a saved session when its delivered result carries +that provenance. Recovery notices without result provenance follow normal routing. + +A new instruction you type is a fresh request and can be judged normally. That includes +a new request that repeats the same words, or a message consumed while the stopped result +is being answered. Mixed stopped and non-stopped results, different requests, multiple +attempts of the same task, other background news and uncertain provenance also follow +normal routing. These cases have no extra protection against restarting stopped work. + +The usual routing inputs and limits still apply. An unattended session uses its original +goal as the routing request, including when a human message arrives during a result wake. +Work must be worth handing off, with at least six words, two models agreeing, and at most +one start every three turns. + ## A message that reads like work makes codeaf look sooner — nothing else happens **The judge that starts work on its own looks at two moments.** One is after a turn that @@ -710,11 +749,11 @@ four jobs, and reading the request cannot tell them apart. The answer doing the this moment now only decides **how soon the answer is told what it has run up**, and the answer decides for itself from there. -**What the judge is looking for is a request whose fastest correct answer is not a -conversation:** several independent deliverables in one message, a sweep over many files or -many sources, or an answer you would otherwise sit and watch a spinner for. A question, a -discussion, one obvious edit and a few tool calls are all answered here, however large the -subject sounds. +**The judge needs both authorization and work worth handing off.** The fastest correct answer +must not be a conversation: several independent deliverables in one message, a sweep over many +files or sources, or an answer you would otherwise sit and watch a spinner for. Feasibility +questions, discussion, suggestions and product direction are answered here, however large the +subject sounds. A clear request whose fastest answer takes only a few tool calls stays here too. **Everything that bounds the auto-start above bounds this too** — one start every three turns across both moments, six words, two models agreeing, silence when either cannot diff --git a/internal/manual/chat_test.go b/internal/manual/chat_test.go index c0524e43d..8ec1caede 100644 --- a/internal/manual/chat_test.go +++ b/internal/manual/chat_test.go @@ -4116,3 +4116,22 @@ func TestChangedInputQuestionReachesRetainedHistory(t *testing.T) { t.Errorf("%q does not reach what-i-remember · %q; it reached %v", question, title, where) } } + +// A rule beyond the section cap is absent from the answer a model actually reads, +// even when searching finds the correct page. Stopped-result handling therefore +// needs its own searchable section and both halves of the rule in the context. +func TestTheChatManualExplainsStoppedResultRouting(t *testing.T) { + text := strings.Join(strings.Fields(Chat().Context("can a stopped task's result start the same request again", DefaultResults)), " ") + for _, want := range []string{ + "answering that result does not start the same request again", + "A new instruction you type is a fresh request and can be judged normally", + "every result is known to be stopped for the current request", + "no fresh human message was consumed", + "Recovery notices without result provenance follow normal routing", + "These cases have no extra protection against restarting stopped work", + } { + if !strings.Contains(text, want) { + t.Errorf("the retrieved stopped-result answer omits %q", want) + } + } +} diff --git a/internal/session/agent.go b/internal/session/agent.go index 9f020618d..b58e81e69 100644 --- a/internal/session/agent.go +++ b/internal/session/agent.go @@ -1191,6 +1191,9 @@ type userMessage struct { // otherResults preserves untagged background outcomes when a batch also // carries task results. Their reply obligation must not be lost in folding. otherResults bool + // nonTaskNews preserves untagged news inside a batch for the conservative + // stopped-result routing guard, without changing the turn's owed asks. + nonTaskNews bool // wake marks a note the model OWES AN ANSWER FOR: a task's completion // (task_run.go's reportTaskNode), a background job's exit (jobs.go's reap), @@ -3378,12 +3381,14 @@ func batchSessionNotes(notes []userMessage) userMessage { var ( wake bool otherResults bool + nonTaskNews bool tags []TaskReplyTag delivered []durableDelivery ) for _, note := range notes { wake = wake || note.wake otherResults = otherResults || note.otherResults + nonTaskNews = nonTaskNews || note.nonTaskNews || len(note.replyTags) == 0 tags = append(tags, note.replyTags...) // The batch is the record these notes end up in, so it carries what // settles each of them ([durableDelivery]); dropped here, every landing in @@ -3428,6 +3433,7 @@ func batchSessionNotes(notes []userMessage) userMessage { message: textMessage("user", text), replyTags: tags, otherResults: otherResults, + nonTaskNews: nonTaskNews, delivered: delivered, wake: wake, authored: true, diff --git a/internal/session/route_judge.go b/internal/session/route_judge.go index 9b13aec17..eee160185 100644 --- a/internal/session/route_judge.go +++ b/internal/session/route_judge.go @@ -283,7 +283,7 @@ type routeVerdict struct { // is a condition nobody can check (see [routeVerdict.Acceptance]). const routeJudgeBrief = `You judge ONE turn of a coding assistant, after the fact. The assistant answered the person in WORDS ALONE — it called no tool. You decide one thing: should that turn have been WORK? -WORDS are a question, a discussion, advice, an opinion, a fact, an explanation, a plan somebody asked to read. SMALL WORK is words too, for this purpose: a few tool calls, one obvious edit, a file read and an answer. Handing small work off is slower than doing it, so it is not work. +WORDS include questions about feasibility, discussion, advice, opinions, facts, explanations and plans somebody asked to read. A polite question that clearly asks you to act now can still authorize work, as the work rule below explains. SMALL WORK is words too, for this purpose: a few tool calls, one obvious edit, a file read and an answer. Handing small work off is slower than doing it, so it is not work. WORK is research across several sources, changes across several files, a goal with several independent parts, or anything the person would otherwise watch a spinner for. @@ -309,6 +309,19 @@ or {"work": true, "wide": true, "goal": "...", "acceptance": "...", "checks": ["..."], "why": "..."} + work true only when the person clearly asks codeaf to do work now. This can + be an imperative or a polite question whose intended action is clear: + "Could you fix the failing tests?" and "Can you implement headless + screenshots and add tests?" both authorize work. A feasibility question, + such as "Is headless screenshot capture possible on CI?", or an + explanation request, such as "How would headless screenshots work?", + does not authorize work. "Can we get screenshots without a display?" is + ambiguous without context, so answer or clarify; do not start work. + "We should support screenshots eventually" is future direction, not + authorization. Judge intended action and timing, not grammar. Product + direction, suggestions, goals and plans do not authorize work unless + the person clearly asks you to act now. Judge authorization before + effort: a useful or lengthy job is not permission. wide true when the work is BROAD — many files, many sources, several independent parts — so the one worker that starts on it is allowed to hand the parts out once it has opened the material. Leave it out for @@ -379,6 +392,7 @@ func (a *Agent) readRouteJudge(ctx context.Context, user userMessage, usedTools a.mu.Lock() turn, offered := a.routeTurns, a.routeOffered model, closed := a.model, a.closed + freshPerson := a.personHeard != 0 && a.personHeard == a.turnSeq a.mu.Unlock() if closed || usedTools { return judgeRuling{} @@ -430,6 +444,11 @@ func (a *Agent) readRouteJudge(ctx context.Context, user userMessage, usedTools if offered > 0 && turn-offered < routeJudgeGap { return judgeRuling{} } + // Only an exclusively stopped-result wake is report-only. Every other + // turn keeps the ordinary ask, floors and readings, including human steering. + if user.wake && !user.answersPerson && !freshPerson && a.onlyStoppedResults(asked) { + return judgeRuling{} + } // The judge is a ROLE, so an install with no tiers configured resolves it to // the conversation's own model rather than refusing — and an install with // nothing anywhere gets no judge at all, which is this feature absent rather @@ -469,6 +488,26 @@ func (a *Agent) readRouteJudge(ctx context.Context, user userMessage, usedTools return judgeRuling{start: true, verdict: verdict, turn: turn, ahead: ahead} } +// onlyStoppedResults requires positive provenance for every arrival. Missing +// metadata, other news and repeated task identities leave normal routing alone; +// in particular, two attempts of one task are not a stopped-only batch. +func (a *Agent) onlyStoppedResults(asked string) bool { + a.mu.Lock() + defer a.mu.Unlock() + if a.turnNonTaskNews || len(a.turnResultTags) == 0 { + return false + } + seen := make(map[uint64]bool, len(a.turnResultTags)) + for _, tag := range a.turnResultTags { + if tag.ID == 0 || seen[tag.ID] || !tag.State.settled() || !tag.Stopped || + strings.TrimSpace(tag.Request) != strings.TrimSpace(asked) { + return false + } + seen[tag.ID] = true + } + return true +} + // applyRouteJudge is the other half of [Agent.readRouteJudge]: the decision, // spent. The TURN calls it, and it calls it at the one moment it knows the turn // is really over — after the reader that may re-open the turn has said it will @@ -766,7 +805,7 @@ func routeJudgeQuestion(asked, answered string) string { // to surface features is a keyword rule with a bill attached. const routeAheadBrief = `You judge ONE message a person has just typed to a coding assistant, BEFORE it is answered. Nothing has been done yet: you are reading the REQUEST, not a reply. You decide one thing: is this WORK? -WORDS are a question, a discussion, advice, an opinion, a fact, an explanation, a plan somebody asked to read. SMALL WORK is words too, for this purpose: a few tool calls, one obvious edit, a file read and an answer. Handing small work off is slower than doing it, so it is not work. +WORDS include questions about feasibility, discussion, advice, opinions, facts, explanations and plans somebody asked to read. A polite question that clearly asks you to act now can still authorize work, as the work rule below explains. SMALL WORK is words too, for this purpose: a few tool calls, one obvious edit, a file read and an answer. Handing small work off is slower than doing it, so it is not work. WORK is research across several sources, changes across several files, a goal with several independent parts, or anything the person would otherwise watch a spinner for. Three shapes of request are almost always work: one message that asks for SEVERAL INDEPENDENT DELIVERABLES, a SWEEP across many files or many sources, and anything whose answer is MINUTES OF TOOL CALLS the person can only sit and watch. diff --git a/internal/session/route_judge_dev_test.go b/internal/session/route_judge_dev_test.go new file mode 100644 index 000000000..03befca9a --- /dev/null +++ b/internal/session/route_judge_dev_test.go @@ -0,0 +1,297 @@ +package session + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/roles" +) + +// Keep these probes source-compatible with current dev: no new tag fields or helpers. +func r1845Node(t *testing.T, a *Agent, title string) *TaskNode { + t.Helper() + g := a.graph() + id := g.reserve() + g.admit(id, taskSpec{title: title, request: routeAsk, brief: title, acceptance: "done"}) + return g.node(id) +} + +func TestOldFailedAttemptAndNewStopKeepDevRouting(t *testing.T) { + c := &routeCompleter{answer: "The audit was stopped.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + a.mu.Lock() + a.opened = false // Queue both real deliveries before any wake can consume either. + a.personAsk = routeAsk + a.mu.Unlock() + n := r1845Node(t, a, "pricing audit") + g := a.graph() + g.mu.Lock() + n.state, n.report = TaskFailed, "failed audit" + g.mu.Unlock() + a.reportTaskNode(n) // Real resultOf -> deliverTaskNote -> postTaskMessage. + if err := a.RetryTask(n.id); err != nil { + t.Fatal(err) + } + if _, err := a.Cancel(fmt.Sprint(n.id)); err != nil { + t.Fatal(err) + } + if !n.wasStopped() { + t.Fatal("fixture did not stop the retry") + } + a.drainSteering(nil) + a.mu.Lock() + ids := append([]uint64(nil), a.turnResults...) + a.mu.Unlock() + if len(ids) != 1 { + t.Fatalf("unexpected task ids: %v", ids) + } + r := a.readRouteJudge(context.Background(), wakeNote("the results arrived"), false, c.answer) + defer r.release() + t.Logf("start=%t screen=%d confirm=%d question=%s", r.start, c.asked(), c.confirms(), c.sawQuestion()) + if r.start { + a.applyRouteJudge(newEventHub(), r) + t.Logf("admitted tasks after applying ruling=%d", admitted(g)) + if admitted(g) != 2 { + t.Fatal("mixed attempts changed dev's admission") + } + } + if !r.start { + t.Fatal("mixed attempts changed dev's ruling") + } + assertDevRouteReadings(t, c, 1) +} + +func TestRecoveryWithoutResultProvenanceKeepsDevRouting(t *testing.T) { + journal, checkpoint := journalIn(t) + writeCheckpoint(t, checkpoint, taskDocument{ + Type: taskDocumentType, Version: taskFileVersion, Seq: 1, + Nodes: []taskRecord{{ID: 1, Title: "pricing audit", Request: routeAsk, Brief: "audit", Acceptance: "done", State: TaskFailed, Ending: TaskEndingStopped, Report: "stopped", Noted: false}}, + }) + c := &routeCompleter{answer: "The audit was stopped.", verdict: routeYes} + a, _ := newTestAgent(t, c, func(config *Config) { + config.SessionFile = journal + config.AskConsent = true + config.RolesSource = tierSettings(map[string]string{roles.TierKey(roles.TierLow): routeScreenModel, roles.TierKey(roles.TierMastermind): routeConfirmModel}) + }) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + a.mu.Lock() + a.personAsk = routeAsk + a.mu.Unlock() + stubbedGraph(a, func(*TaskNode) {}) + a.drainSteering(nil) // Consume the actual non-waking recovery notice. + // This is the same boundary drain used by a wake. + a.mu.Lock() + ids := append([]uint64(nil), a.turnResults...) + a.mu.Unlock() + t.Logf("restored stopped=%t, remembered result IDs=%v", a.graph().node(1).wasStopped(), ids) + r := a.readRouteJudge(context.Background(), wakeNote("a background event wakes this session"), false, c.answer) + defer r.release() + t.Logf("start=%t screen=%d confirm=%d", r.start, c.asked(), c.confirms()) + if !r.start || len(ids) != 0 { + t.Fatal("recovery fixture or dev ruling changed") + } + assertDevRouteReadings(t, c, 1) +} + +func TestUnrevisedMixedResultsKeepDevRouteAsk(t *testing.T) { + c := &routeCompleter{answer: "The UI audit stays stopped; finish the storage audit.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + ui := r1845Node(t, a, "UI audit") + storage := r1845Node(t, a, "storage audit") + g := a.graph() + g.mu.Lock() + ui.state, ui.stopped = TaskFailed, true + ui.spec.brief = "Audit only pricing UI views and produce UI-ONLY-FINDINGS.md" + storage.state = TaskFailed + storage.spec.brief = "Audit only pricing storage transactions and produce STORAGE-ONLY-FINDINGS.md" + storage.spec.acceptance = "STORAGE-ONLY-FINDINGS.md exists" + g.mu.Unlock() + note := wakeNote("one audit stopped; the other failed") + note.replyTags = []TaskReplyTag{ui.resultTag(), storage.resultTag()} + a.mu.Lock() + a.rememberOwedLocked(note) + a.mu.Unlock() + r := a.readRouteJudge(context.Background(), note, false, c.answer) + defer r.release() + if !r.start { + t.Fatal("mixed results changed dev's ruling") + } + assertDevRouteReadings(t, c, 1) +} + +func TestFreshThanksAndMixedResultsKeepDevRouting(t *testing.T) { + c := &routeCompleter{answer: "The cancelled UI audit stays cancelled. The failed storage audit needs repair.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + ui := r1845Node(t, a, "cancelled UI audit") + storage := r1845Node(t, a, "failed storage audit") + g := a.graph() + g.mu.Lock() + ui.state, ui.stopped = TaskFailed, true + storage.state = TaskFailed + g.mu.Unlock() + c.onAnswer = func(round int) { + if round != 1 { + return + } + ch, err := a.Submit(context.Background(), "thanks") + if err != nil { + t.Error(err) + return + } + // Finish the production boundary drain before this scripted answer + // returns, so a discarded pre-steer reading cannot satisfy the counter. + a.drainSteering(nil) + go func() { + for range ch { + } + }() + } + wakes, leave := a.WatchWakes() + defer leave() + note := wakeNote("The UI audit stopped; the storage audit failed.") + note.replyTags = []TaskReplyTag{ui.resultTag(), storage.resultTag()} + if !a.enqueueNote(note) { + t.Fatal("wake refused") + } + select { + case stream := <-wakes: + collect(t, stream) + case <-time.After(10 * time.Second): + t.Fatal("no wake") + } + a.mu.Lock() + heard, turn := a.personHeard, a.turnSeq + a.mu.Unlock() + t.Logf("personHeard=%d turn=%d screen=%d confirm=%d", heard, turn, c.asked(), c.confirms()) + if heard != turn { + t.Fatal("fixture did not consume the human message") + } + assertDevRouteReadings(t, c, 2) +} + +// The expected calls and complete reader input are measured on dev a4dff1168 +// using these same production wake, Submit, retry, cancel and recovery paths. +// Compatibility controls deliberately pass on both versions; stop-only tests +// separately prove the guard changes behavior. +func assertDevRouteReadings(t *testing.T, c *routeCompleter, calls int) { + t.Helper() + c.mu.Lock() + screen, confirm, question, confirmQ := c.judged, c.confirmed, c.question, c.confirmQ + c.mu.Unlock() + want := routeJudgeQuestion(routeAsk, c.answer) + if screen != calls || confirm != calls || question != want || confirmQ != want { + t.Fatalf("dev compatibility: screen=%d confirm=%d, want %d each\nquestion=%q\nconfirm=%q\nwant=%q", screen, confirm, calls, question, confirmQ, want) + } + t.Logf("dev compatibility: screen=%d confirm=%d ask=%q", screen, confirm, routeAsk) +} + +func TestAmbiguousResultWakesKeepDevRouting(t *testing.T) { + for _, scenario := range []string{ + "no provenance", "legacy tag", "unknown task", "background outcome", + "untagged news batched first", "untagged news batched last", "nested batch", + "untagged news consumed later", "other request", "mixed requests", + "two stopped attempts", "non-settled tag", "answers earlier human", + } { + t.Run(scenario, func(t *testing.T) { + c := &routeCompleter{answer: "Here is what happened.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + n := reviewResultNode(t, a, true, "stopped audit") + note := wakeNote("the result arrived") + note.replyTags = []TaskReplyTag{n.resultTag()} + switch scenario { + case "no provenance": + note.replyTags = nil + case "legacy tag": + note.replyTags = []TaskReplyTag{{ID: n.id, Request: routeAsk}} + case "unknown task": + note.replyTags = append(note.replyTags, TaskReplyTag{ID: 999, Request: routeAsk}) + case "background outcome": + note.otherResults = true + case "answers earlier human": + note.answersPerson = true + case "untagged news batched first": + note = batchSessionNotes([]userMessage{wakeNote("other news"), note}) + case "untagged news batched last": + note = batchSessionNotes([]userMessage{note, wakeNote("other news")}) + case "nested batch": + note = batchSessionNotes([]userMessage{batchSessionNotes([]userMessage{note, wakeNote("other news")})}) + case "other request", "mixed requests": + g := a.graph() + g.mu.Lock() + n.spec.request = "implement headless screenshots across the app and add tests" + g.mu.Unlock() + if scenario == "other request" { + note.replyTags = nil + } + note.replyTags = append(note.replyTags, n.resultTag()) + case "two stopped attempts": + g := a.graph() + g.mu.Lock() + n.attempt++ + g.mu.Unlock() + note.replyTags = append(note.replyTags, n.resultTag()) + case "non-settled tag": + g := a.graph() + g.mu.Lock() + n.state = TaskRunning + g.mu.Unlock() + note.replyTags = []TaskReplyTag{n.resultTag()} + } + a.mu.Lock() + a.rememberOwedLocked(note) + if scenario == "untagged news consumed later" { + a.rememberOwedLocked(wakeNote("other news")) + } + a.mu.Unlock() + r := a.readRouteJudge(context.Background(), note, false, c.answer) + defer r.release() + if !r.start { + t.Fatal("ambiguous wake changed dev's ruling") + } + assertDevRouteReadings(t, c, 1) + }) + } +} + +func TestFreshHumanWakesKeepDevFloors(t *testing.T) { + for _, scenario := range []string{"Person", "short Steward ask", "trivial Steward ask"} { + t.Run(scenario, func(t *testing.T) { + c := &routeCompleter{answer: "I will report the result.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + if scenario == "short Steward ask" { + a.principal = NewSteward("thanks", Budget{Wall: time.Hour}, nil) + } else if scenario == "trivial Steward ask" { + a.principal = NewSteward("please commit all of the changes now", Budget{Wall: time.Hour}, nil) + } + n := reviewResultNode(t, a, true, "stopped audit") + c.onAnswer = func(round int) { + if round != 1 { + return + } + stream, err := a.Submit(context.Background(), routeAsk) + if err != nil { + t.Error(err) + return + } + a.drainSteering(nil) + go func() { + for range stream { + } + }() + } + reviewWake(t, a, []TaskReplyTag{n.resultTag()}) + a.mu.Lock() + heard, turn := a.personHeard, a.turnSeq + a.mu.Unlock() + if heard == 0 || heard != turn || c.asked() != 0 || c.confirms() != 0 { + t.Fatalf("dev floor changed: heard=%d turn=%d screen=%d confirm=%d", heard, turn, c.asked(), c.confirms()) + } + }) + } +} diff --git a/internal/session/route_judge_mixed_results_test.go b/internal/session/route_judge_mixed_results_test.go new file mode 100644 index 000000000..44089f531 --- /dev/null +++ b/internal/session/route_judge_mixed_results_test.go @@ -0,0 +1,103 @@ +package session + +import ( + "context" + "fmt" + "testing" + "time" +) + +func TestReviewMixedResultsKeepIndependentRepairEligible(t *testing.T) { + c := &routeCompleter{answer: "The cancelled UI audit stays cancelled. I will complete the independent storage audit.", verdict: `{"work":true,"goal":"Complete the independent pricing storage audit; leave the cancelled UI audit stopped","acceptance":"Storage audit report exists","why":"finish the still-authorized independent audit"}`} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + a.personAsk = routeAsk + stopped := reviewResultNode(t, a, true, "cancelled pricing UI audit") + failed := reviewResultNode(t, a, false, "failed pricing storage audit") + reviewWake(t, a, []TaskReplyTag{stopped.resultTag(), failed.resultTag()}) + a.mu.Lock() + results := append([]uint64(nil), a.turnResults...) + a.mu.Unlock() + t.Logf("result IDs=%v; screen=%d confirm=%d", results, c.asked(), c.confirms()) + if len(results) != 2 { + t.Fatal("fixture did not deliver both results") + } + if c.asked() == 0 { + t.Fatal("a different failed task's repair was suppressed by the stopped sibling's inherited request") + } + assertDevRouteReadings(t, c, 2) +} + +func TestStoppedResultRoutingUsesTheDeliveredAttempt(t *testing.T) { + for _, stoppedAtDelivery := range []bool{true, false} { + t.Run(fmt.Sprintf("stopped_at_delivery_%t", stoppedAtDelivery), func(t *testing.T) { + c := &routeCompleter{answer: "Here is what happened.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + node := reviewResultNode(t, a, stoppedAtDelivery, "delayed audit") + tag := node.resultTag() + g := a.graph() + g.mu.Lock() + // Another attempt changes the live node before this result is consumed. + node.attempt++ + node.stopped = !stoppedAtDelivery + g.mu.Unlock() + a.mu.Lock() + note := wakeNote("the old attempt arrived") + note.replyTags = []TaskReplyTag{tag} + a.rememberOwedLocked(note) + a.mu.Unlock() + ruling := a.readRouteJudge(context.Background(), note, false, c.answer) + defer ruling.release() + if ruling.start == stoppedAtDelivery { + t.Fatalf("start=%t followed the live node instead of delivered stopped=%t", ruling.start, stoppedAtDelivery) + } + wantCalls := 1 + if stoppedAtDelivery { + wantCalls = 0 + } + if c.asked() != wantCalls || c.confirms() != wantCalls { + t.Fatalf("screen=%d confirm=%d, want %d each", c.asked(), c.confirms(), wantCalls) + } + // The next wake cannot inherit this attempt's cancellation snapshot. + a.mu.Lock() + a.forgetOwedLocked() + a.mu.Unlock() + fresh := a.readRouteJudge(context.Background(), wakeNote("new event"), false, c.answer) + defer fresh.release() + if !fresh.start { + t.Fatal("a previous turn's stopped result suppressed a later wake") + } + }) + } +} + +func TestMixedResultRoutingKeepsDevAskWithRevisedTargets(t *testing.T) { + for _, reverse := range []bool{false, true} { + t.Run(fmt.Sprintf("reversed_%t", reverse), func(t *testing.T) { + c := &routeCompleter{answer: "Only the storage audit needs work.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + stopped := reviewResultNode(t, a, true, "cancelled UI audit") + failed := reviewResultNode(t, a, false, "storage audit") + remaining := failed.resultTag() + const target = "audit storage transactions only and produce the storage report" + remaining.Obligation, remaining.Revision = target, 2 + tags := []TaskReplyTag{stopped.resultTag(), remaining} + if reverse { + tags[0], tags[1] = tags[1], tags[0] + } + note := wakeNote("the two results arrived") + note.replyTags = tags + a.mu.Lock() + a.rememberOwedLocked(note) + a.mu.Unlock() + ruling := a.readRouteJudge(context.Background(), note, false, c.answer) + defer ruling.release() + if !ruling.start { + t.Fatal("the independent result was suppressed") + } + assertDevRouteReadings(t, c, 1) + }) + } +} diff --git a/internal/session/route_judge_restored_result_test.go b/internal/session/route_judge_restored_result_test.go new file mode 100644 index 000000000..57b57c433 --- /dev/null +++ b/internal/session/route_judge_restored_result_test.go @@ -0,0 +1,61 @@ +package session + +import ( + "context" + "testing" + "time" +) + +func TestReviewRestoredStoppedResultCannotRestart(t *testing.T) { + c := &routeCompleter{answer: "The audit was stopped.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + a.personAsk = routeAsk + stopped := reviewResultNode(t, a, true, "cancelled audit") + g := a.graph() + g.mu.Lock() + record := stopped.recordLocked() + g.mu.Unlock() + restored := restoreNode(g, record) + g.mu.Lock() + g.nodes[stopped.id] = restored + g.mu.Unlock() + t.Logf("checkpoint ending=%q, restored stopped=%t", record.Ending, restored.wasStopped()) + reviewWake(t, a, []TaskReplyTag{restored.resultTag()}) + if c.asked() != 0 || c.confirms() != 0 || admitted(g) != 1 { + t.Fatal("a restored stopped result reached the classifier and can restart the stopped request") + } +} + +func TestRestoredNonStoppedResultsRemainEligible(t *testing.T) { + for _, ending := range []TaskEnding{"", TaskEndingInterrupted, TaskEndingWire, TaskEndingRefused} { + t.Run(string(ending), func(t *testing.T) { + c := &routeCompleter{answer: "The audit needs a repair.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + node := reviewResultNode(t, a, false, "failed audit") + g := a.graph() + g.mu.Lock() + node.ending = ending + record := node.recordLocked() + g.mu.Unlock() + restored := restoreNode(g, record) + if restored.wasStopped() { + t.Fatalf("ending %q restored as a person's cancellation", ending) + } + g.mu.Lock() + g.nodes[node.id] = restored + g.mu.Unlock() + note := wakeNote("the failed result arrived") + note.replyTags = []TaskReplyTag{restored.resultTag()} + a.mu.Lock() + a.rememberOwedLocked(note) + a.mu.Unlock() + ruling := a.readRouteJudge(context.Background(), note, false, c.answer) + defer ruling.release() + if !ruling.start || c.asked() != 1 || c.confirms() != 1 { + t.Fatalf("restored non-stopped result was suppressed: start=%t screen=%d confirm=%d", ruling.start, c.asked(), c.confirms()) + } + }) + } +} diff --git a/internal/session/route_judge_stopped_result_test.go b/internal/session/route_judge_stopped_result_test.go new file mode 100644 index 000000000..efea8e1ce --- /dev/null +++ b/internal/session/route_judge_stopped_result_test.go @@ -0,0 +1,121 @@ +package session + +import ( + "context" + "fmt" + "testing" + "time" +) + +func reviewResultNode(t *testing.T, a *Agent, stopped bool, title string) *TaskNode { + t.Helper() + g := a.graph() + id := g.reserve() + g.admit(id, taskSpec{title: title, request: routeAsk, brief: title, acceptance: "done"}) + n := g.node(id) + g.mu.Lock() + n.state = TaskFailed + n.stopped = stopped + if stopped { + n.ending = TaskEndingStopped + } + g.mu.Unlock() + return n +} + +func reviewWake(t *testing.T, a *Agent, tags []TaskReplyTag) { + t.Helper() + wakes, leave := a.WatchWakes() + defer leave() + note := wakeNote("The stopped audit was cancelled. A different audit failed and needs a repair.") + note.replyTags = tags + if !a.enqueueNote(note) { + t.Fatal("wake not accepted") + } + select { + case stream := <-wakes: + collect(t, stream) + case <-time.After(10 * time.Second): + t.Fatal("no wake") + } +} + +func TestReviewFreshHumanRepeatDuringStoppedWake(t *testing.T) { + c := &routeCompleter{answer: "I will fix the audit now.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + a.personAsk = routeAsk + stopped := reviewResultNode(t, a, true, "cancelled old audit") + c.onAnswer = func(round int) { + if round != 1 { + return + } + ch, err := a.Submit(context.Background(), routeAsk) + if err != nil { + t.Error(err) + return + } + // Finish the production boundary drain before this scripted answer + // returns, so the steer is consumed in this turn deterministically. + a.drainSteering(nil) + go func() { + for range ch { + } + }() + } + reviewWake(t, a, []TaskReplyTag{stopped.resultTag()}) + a.mu.Lock() + heard, turn := a.personHeard, a.turnSeq + a.mu.Unlock() + t.Logf("fresh human message consumed in turn=%d; current turn=%d; screen=%d confirm=%d", heard, turn, c.asked(), c.confirms()) + if heard != turn { + t.Fatal("fixture did not consume the human message in the result turn") + } + assertDevRouteReadings(t, c, 2) +} + +func TestFreshHumanSteeringKeepsDevRouteAsk(t *testing.T) { + c := &routeCompleter{answer: "I will implement the screenshots now.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + stopped := reviewResultNode(t, a, true, "cancelled old audit") + const fresh = "implement headless screenshots across the app and add tests" + c.onAnswer = func(round int) { + if round != 1 { + return + } + ch, err := a.Submit(context.Background(), fresh) + if err != nil { + t.Error(err) + return + } + // Finish the production boundary drain before this scripted answer + // returns, so the steer is consumed in this turn deterministically. + a.drainSteering(nil) + go func() { + for range ch { + } + }() + } + reviewWake(t, a, []TaskReplyTag{stopped.resultTag()}) + assertDevRouteReadings(t, c, 2) +} + +func TestStoppedOnlyResultWakeAdmitsNothing(t *testing.T) { + for _, multiple := range []bool{false, true} { + t.Run(fmt.Sprint(multiple), func(t *testing.T) { + c := &routeCompleter{answer: "The audit was stopped.", verdict: routeYes} + a, _, _ := routeAgent(t, c) + a.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + stopped := reviewResultNode(t, a, true, "stopped audit") + tags := []TaskReplyTag{stopped.resultTag()} + if multiple { + tags = append(tags, reviewResultNode(t, a, true, "another stopped audit").resultTag()) + } + reviewWake(t, a, tags) + if c.asked() != 0 || c.confirms() != 0 || admitted(a.graph()) != len(tags) { + t.Fatalf("stopped-only wake: screen=%d confirm=%d admitted=%d", c.asked(), c.confirms(), admitted(a.graph())) + } + }) + } +} diff --git a/internal/session/route_judge_test.go b/internal/session/route_judge_test.go index cef231432..78f1bce7f 100644 --- a/internal/session/route_judge_test.go +++ b/internal/session/route_judge_test.go @@ -10,6 +10,7 @@ import ( "github.com/Agent-Field/agentfield/sdk/go/ai" "github.com/Agent-Field/codeaf/internal/approval" + "github.com/Agent-Field/codeaf/internal/manual" "github.com/Agent-Field/codeaf/internal/roles" "github.com/Agent-Field/codeaf/internal/splitgate" ) @@ -1902,6 +1903,125 @@ func TestBothBriefsAskForOneObjectAndOnlyOneOfThemMentionsAnAnswer(t *testing.T) } } +func TestRouteJudgesDistinguishWorkOrdersFromQuestions(t *testing.T) { + contract := strings.Join(strings.Fields(routeVerdictContract), " ") + page := strings.Join(strings.Fields(manual.Chat().Context("will a polite request start a task", manual.DefaultResults)), " ") + for _, example := range routeIntentExamples { + text := strings.Join(strings.Fields(example.request), " ") + if !strings.Contains(page, text) { + t.Errorf("the retrieved task-routing manual omits %s", example.request) + } + if !strings.Contains(contract, text) { + t.Errorf("the shared route contract omits %s", example.request) + } + if !strings.Contains(contract, example.rule) { + t.Errorf("the shared route contract does not explain why %s is work=%t: missing %q", example.request, example.wantWork, example.rule) + } + for name, brief := range map[string]string{"pre-turn": routeAheadBrief, "post-turn": routeJudgeBrief} { + brief = strings.Join(strings.Fields(brief), " ") + if !strings.Contains(brief, text) || !strings.Contains(brief, example.rule) { + t.Errorf("the %s route prompt does not carry the %s classification", name, example.request) + } + } + } + for _, want := range []string{ + "clearly asks codeaf to do work now", + "Judge intended action and timing, not grammar", + "authorization before effort", + } { + if !strings.Contains(contract, strings.Join(strings.Fields(want), " ")) { + t.Errorf("the shared route contract does not state %q", want) + } + } + if !strings.Contains(routeAheadBrief, routeVerdictContract) || !strings.Contains(routeJudgeBrief, routeVerdictContract) { + t.Fatal("one route judge can start work without the authorization rule") + } +} + +// routeIntentExamples pins each example together with its classification in +// both prompts. This is a prompt-text regression, not a model evaluation: there +// is no deterministic intent parser whose semantic accuracy it could assert. +var routeIntentExamples = []struct { + request string + wantWork bool + rule string +}{ + {`"Could you fix the failing tests?"`, true, `"Could you fix the failing tests?" and "Can you implement headless screenshots and add tests?" both authorize work.`}, + {`"Can you implement headless screenshots and add tests?"`, true, `"Could you fix the failing tests?" and "Can you implement headless screenshots and add tests?" both authorize work.`}, + {`"Is headless screenshot capture possible on CI?"`, false, `A feasibility question, such as "Is headless screenshot capture possible on CI?", or an explanation request, such as "How would headless screenshots work?", does not authorize work.`}, + {`"How would headless screenshots work?"`, false, `A feasibility question, such as "Is headless screenshot capture possible on CI?", or an explanation request, such as "How would headless screenshots work?", does not authorize work.`}, + {`"Can we get screenshots without a display?"`, false, `"Can we get screenshots without a display?" is ambiguous without context, so answer or clarify; do not start work.`}, + {`"We should support screenshots eventually"`, false, `"We should support screenshots eventually" is future direction, not authorization.`}, +} + +func TestAStoppedTaskResultCannotAutoStartItsRequestAgain(t *testing.T) { + for _, tc := range []struct { + name string + request string + stopped bool + arrived bool + wake bool + want bool + }{ + {"stopped result", routeAsk, true, true, true, false}, + {"stopped result with surrounding whitespace", " " + routeAsk + "\n", true, true, true, false}, + {"fresh human request", routeAsk, true, true, false, true}, + {"failed result can still start a repair", routeAsk, false, true, true, true}, + {"unrelated stopped result", "implement headless screenshots across the app and add tests", true, true, true, true}, + {"stopped task did not arrive this turn", routeAsk, true, false, true, true}, + } { + t.Run(tc.name, func(t *testing.T) { + completer := &routeCompleter{answer: "I can report what happened.", verdict: routeYes} + agent, _, nodes := routeAgent(t, completer) + graph := agent.graph() + id := graph.reserve() + graph.admit(id, taskSpec{title: "the requested audit", request: tc.request, brief: "audit", acceptance: "done"}) + if tc.stopped { + graph.mu.Lock() + graph.nodes[id].state = TaskFailed + graph.mu.Unlock() + graph.node(id).markStopped() + } + agent.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + if tc.arrived { + agent.mu.Lock() + note := wakeNote("the task result arrived") + note.replyTags = []TaskReplyTag{graph.node(id).resultTag()} + agent.rememberOwedLocked(note) + agent.mu.Unlock() + } + user := userText(routeAsk) + if tc.wake { + user = userMessage{wake: true} + } + ruling := agent.readRouteJudge(context.Background(), user, false, completer.answer) + defer ruling.release() + if ruling.start != tc.want { + t.Fatalf("start = %t, want %t", ruling.start, tc.want) + } + wantCalls := 0 + if tc.want { + wantCalls = 1 + } + if completer.asked() != wantCalls || completer.confirms() != wantCalls { + t.Fatalf("screen calls = %d, confirms = %d, want %d each", completer.asked(), completer.confirms(), wantCalls) + } + if !tc.want { + agent.applyRouteJudge(newEventHub(), ruling) + if nodes.count() != 0 || admitted(graph) != 1 { + t.Fatal("reporting a stopped result admitted or ran another task") + } + // A person can repeat the same request after the stopped wake. + fresh := agent.readRouteJudge(context.Background(), userText(routeAsk), false, completer.answer) + defer fresh.release() + if !fresh.start || completer.asked() != 1 || completer.confirms() != 1 { + t.Fatal("a fresh human request was suppressed after the stopped result") + } + } + }) + } +} + // ── the order of the two readers at a turn's end ──────────────────────────── // A JUDGE THAT HAS ALREADY ANSWERED HAS STILL STARTED NOTHING. diff --git a/internal/session/session.go b/internal/session/session.go index dfc0666a2..b59a6fc37 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -664,6 +664,11 @@ type TaskReplyTag struct { // neither and is read exactly as it always was. Obligation string `json:"obligation,omitempty"` Revision uint64 `json:"revision,omitempty"` + // State and Stopped belong to the result's own attempt. A delayed + // delivery must not borrow a re-armed node's new cancellation state. Empty + // State identifies older tags without positive stop provenance. + State TaskState `json:"state,omitempty"` + Stopped bool `json:"stopped,omitempty"` } // Event is one observable thing in a turn. A Submit returns a channel of @@ -2827,6 +2832,11 @@ type Agent struct { // [Agent.deliveringOwnedResult]), and that question has to survive a result // whose words were empty. turnResults []uint64 + // turnResultTags keeps cancellation provenance for the stopped-only guard. + // It retains every arrival, so mixed attempts cannot look stopped-only. + // turnNonTaskNews marks arrivals without exclusive task-result provenance. + turnResultTags []TaskReplyTag + turnNonTaskNews bool // personTurns is the same answer for the turns BEFORE the newest one, kept // for the same reason and bounded (admission_compile.go). personAsk answers // "what is the current ask"; this answers "what else have they told us", diff --git a/internal/session/task_store.go b/internal/session/task_store.go index cc4de10f8..7b963ba84 100644 --- a/internal/session/task_store.go +++ b/internal/session/task_store.go @@ -2175,29 +2175,32 @@ func restoreNode(graph *TaskGraph, record taskRecord) *TaskNode { landed: record.landedHalf(), late: append([]string(nil), record.Late...), ending: record.Ending, - kind: record.Kind, - claim: record.Claim, - checkedOn: record.CheckedOn, - produced: resultFromRecord(record.Result), - changed: record.Changed, - wrote: record.Wrote, - branch: record.Branch, - worktree: record.Worktree, - merge: record.Merge, - journal: record.Journal, - elapsed: time.Duration(record.ElapsedMS) * time.Millisecond, - started: record.StartedAt, - ended: record.EndedAt, - cost: record.CostUSD, - input: record.Input, - output: record.Output, - cacheRead: record.CacheRead, - cacheWrite: record.CacheWrite, - noted: record.Noted, - notedRead: record.Noted, - notedState: record.NotedState, - attempt: record.Attempt, - interrupted: record.Interrupted, + // The ending already persists a person's cancellation. Rebuild the + // runtime flag so restored results retain the same routing protection. + stopped: record.Ending == TaskEndingStopped, + kind: record.Kind, + claim: record.Claim, + checkedOn: record.CheckedOn, + produced: resultFromRecord(record.Result), + changed: record.Changed, + wrote: record.Wrote, + branch: record.Branch, + worktree: record.Worktree, + merge: record.Merge, + journal: record.Journal, + elapsed: time.Duration(record.ElapsedMS) * time.Millisecond, + started: record.StartedAt, + ended: record.EndedAt, + cost: record.CostUSD, + input: record.Input, + output: record.Output, + cacheRead: record.CacheRead, + cacheWrite: record.CacheWrite, + noted: record.Noted, + notedRead: record.Noted, + notedState: record.NotedState, + attempt: record.Attempt, + interrupted: record.Interrupted, // AND WHO WAS HOLDING ITS DECISION, which is read back exactly as it was // written and then put right by the floor a few lines above this node's // arrival ([TaskGraph.handBackOnLoad], called out of [TaskGraph.rehydrate]). diff --git a/internal/session/wakecause.go b/internal/session/wakecause.go index 73d84d611..779a119e4 100644 --- a/internal/session/wakecause.go +++ b/internal/session/wakecause.go @@ -71,6 +71,9 @@ const ( // places a message reaches the transcript — the turn's opening and the steering // drain — so a landing that arrives mid-turn is owed by the turn it lands in. func (a *Agent) rememberOwedLocked(user userMessage) { + // Untagged news cannot establish a stopped-only wake. The empty opening + // envelope contributes no news; its actual notes arrive in the first drain. + a.turnNonTaskNews = a.turnNonTaskNews || user.nonTaskNews || user.otherResults || len(user.replyTags) == 0 && !user.empty() a.rememberProgramOutcomeLocked(user) if question := strings.TrimSpace(user.landingQuestion); question != "" { a.oweLocked(owedAsk{text: question, from: owedByPerson}) @@ -102,6 +105,7 @@ func (a *Agent) rememberOwedLocked(user userMessage) { // text and the write seam needs the node rather than the sentence // (writeseam.go's [Agent.deliveringOwnedResult]). a.arrivedLocked(tag.ID) + a.turnResultTags = append(a.turnResultTags, tag) } } @@ -152,6 +156,8 @@ func (a *Agent) oweLocked(ask owedAsk) { // arrived with. Called once, where a turn opens. func (a *Agent) forgetOwedLocked() { a.owedAsks, a.landingOutcomes, a.turnResults, a.programOutcomeNow, a.personCardAnswers = nil, nil, nil, nil, nil + a.turnResultTags = nil + a.turnNonTaskNews = false } // turnAsk is the ask this turn's endings are read against. @@ -236,6 +242,7 @@ func (n *TaskNode) resultTagLocked() TaskReplyTag { return TaskReplyTag{ ID: n.id, Title: n.spec.title, Request: n.spec.request, Obligation: obligation, Revision: revision, + State: n.state, Stopped: n.stopped, } }