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..0fceeaabe --- /dev/null +++ b/docs/changes/unreleased/1845-explicit-work-orders.md @@ -0,0 +1,26 @@ +--- +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; a stopped + task result cannot restart the same request. +--- + +Clear polite requests can start work even when phrased as questions. Questions +about feasibility, ambiguous questions and future direction stay in the +conversation. A stopped task's result can be reported without starting the same +request again. + +A fresh human instruction remains eligible even when it arrives while the stopped +result is being answered. +An independent non-stopped result in the same wake also remains eligible; sharing +the original request with a stopped task does not cancel the sibling's work. +Restoring a saved task preserves whether it was stopped by a person. + +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..8d510ba3a 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,22 @@ 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 + +If a stopped task's result wakes the conversation, answering that result does not start the +same request again. A new instruction you type is a fresh request and can be judged normally. +That includes a new request that repeats the same words. The normal task rules still +apply: work worth handing off, at least six words, two models agreeing, and at most one start +every three turns. + +You can also type a new instruction while codeaf is answering the stopped result. Once +that instruction reaches the conversation, its own words can be judged normally in that turn. + +If the same wake also reports an independent task that was not stopped, that task's +remaining work can still be judged normally, even when both tasks came from the same +original request. The stopped task stays excluded from that work. +This stop protection also applies to stopped tasks restored from a saved session. + ## 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 +746,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..f02682f8e 100644 --- a/internal/manual/chat_test.go +++ b/internal/manual/chat_test.go @@ -4116,3 +4116,18 @@ 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", + } { + if !strings.Contains(text, want) { + t.Errorf("the retrieved stopped-result answer omits %q", want) + } + } +} diff --git a/internal/session/route_judge.go b/internal/session/route_judge.go index 9b13aec17..755143183 100644 --- a/internal/session/route_judge.go +++ b/internal/session/route_judge.go @@ -122,6 +122,7 @@ package session import ( "context" "encoding/json" + "fmt" "strconv" "strings" "time" @@ -283,7 +284,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 +310,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 +393,8 @@ 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 + personAsk := a.personAsk a.mu.Unlock() if closed || usedTools { return judgeRuling{} @@ -411,7 +427,11 @@ func (a *Agent) readRouteJudge(ctx context.Context, user userMessage, usedTools // floor, the gap, the cheap screen, the mastermind confirm, and both of them // having to say yes. asked := user.text() - if user.empty() || user.wake || user.authored { + if user.wake && freshPerson { + // The opening note stays a wake after steering is consumed. Judge the + // person's new words, even when they repeat the cancelled request. + asked = personAsk + } else if user.empty() || user.wake || user.authored { steward := a.steward() if steward == nil { return judgeRuling{} @@ -430,6 +450,15 @@ func (a *Agent) readRouteJudge(ctx context.Context, user userMessage, usedTools if offered > 0 && turn-offered < routeJudgeGap { return judgeRuling{} } + // A stopped task's result is news to report, not a fresh instruction. + // Independent results in the same wake retain only their own work. + if user.wake && !freshPerson { + var blocked bool + asked, blocked = a.routeResultAsk(asked) + if blocked { + 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 +498,61 @@ func (a *Agent) readRouteJudge(ctx context.Context, user userMessage, usedTools return judgeRuling{start: true, verdict: verdict, turn: turn, ahead: ahead} } +// routeResultAsk excludes cancelled work without vetoing an independent result +// that inherited the same original request. Only a result-only wake uses this; +// the person's fresh instructions take precedence in readRouteJudge. +func (a *Agent) routeResultAsk(asked string) (string, bool) { + a.mu.Lock() + results := append([]uint64(nil), a.turnResults...) + tags := append([]TaskReplyTag(nil), a.turnResultTags...) + a.mu.Unlock() + if len(results) == 0 { + return asked, false + } + graph := a.tasker() + // Older callers carry only ids, and older persisted tags have no state. + // Current deliveries use their frozen snapshots, never the live attempt. + if len(tags) == 0 { + for _, id := range results { + tags = append(tags, TaskReplyTag{ID: id}) + } + } + var eligible, cancelled []TaskReplyTag + stopped := false + for _, tag := range tags { + if tag.State == "" && graph != nil { + if node := graph.node(tag.ID); node != nil { + tag = node.resultTag() + } + } + if tag.Stopped { + cancelled = append(cancelled, tag) + if strings.TrimSpace(tag.Request) == strings.TrimSpace(asked) { + stopped = true + } + } else if tag.State != "" && strings.TrimSpace(tag.owed()) != "" { + eligible = append(eligible, tag) + } + } + if !stopped { + return asked, false + } + if len(eligible) == 0 { + return "", true + } + // The shared request is context for each part, not authority to revive the + // cancelled sibling. Both readers receive the same restricted question. + var out strings.Builder + out.WriteString("Only the non-stopped task results below remain eligible for work. Do not restart stopped tasks or repeat the whole original request.\n") + for _, tag := range eligible { + fmt.Fprintf(&out, "\nTask %d (attempt %d), %s\nIts own target: %s\n", tag.ID, tag.Attempt, tag.Title, tag.owed()) + } + for _, tag := range cancelled { + fmt.Fprintf(&out, "\nStopped task %d, report only: %s\n", tag.ID, tag.Title) + } + return out.String(), false +} + // 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 +850,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_mixed_results_test.go b/internal/session/route_judge_mixed_results_test.go new file mode 100644 index 000000000..42be10554 --- /dev/null +++ b/internal/session/route_judge_mixed_results_test.go @@ -0,0 +1,119 @@ +package session + +import ( + "context" + "fmt" + "strings" + "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") + } + c.mu.Lock() + question, confirm := c.question, c.confirmQ + c.mu.Unlock() + if question != confirm { + t.Fatalf("screen and confirm read different obligations:\n%s\n%s", question, confirm) + } + for _, want := range []string{"Only the non-stopped task results", "Do not restart stopped tasks", "Task 2 (attempt 0), failed pricing storage audit", "Stopped task 1, report only: cancelled pricing UI audit"} { + if !strings.Contains(question, want) { + t.Errorf("the mixed-result question omits %q:\n%s", want, question) + } + } +} + +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 TestMixedResultRoutingRetainsEachTasksEffectiveTarget(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") + } + c.mu.Lock() + question, confirm := c.question, c.confirmQ + c.mu.Unlock() + if question != confirm || !strings.Contains(question, target) || strings.Contains(question, "Its own target: "+routeAsk) { + t.Fatalf("routing lost the eligible task's own effective target:\n%s\n%s", question, confirm) + } + }) + } +} 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..c813f2809 --- /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 { + 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..b91810fcf --- /dev/null +++ b/internal/session/route_judge_stopped_result_test.go @@ -0,0 +1,105 @@ +package session + +import ( + "context" + "strings" + "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 + } + 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") + } + if c.asked() == 0 { + t.Fatal("fresh human repeat was consumed but the stopped-result guard suppressed routing") + } +} + +func TestFreshHumanSteeringSuppliesTheRouteAsk(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 + } + go func() { + for range ch { + } + }() + } + reviewWake(t, a, []TaskReplyTag{stopped.resultTag()}) + c.mu.Lock() + question, confirm := c.question, c.confirmQ + c.mu.Unlock() + for _, got := range []string{question, confirm} { + if !strings.Contains(got, fresh) || strings.Contains(got, routeAsk) { + t.Fatalf("routing did not read the new human instruction: %q", got) + } + } +} diff --git a/internal/session/route_judge_test.go b/internal/session/route_judge_test.go index cef231432..2a33b5cdf 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,120 @@ 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.node(id).markStopped() + } + agent.principal = NewSteward(routeAsk, Budget{Wall: time.Hour}, nil) + if tc.arrived { + agent.mu.Lock() + agent.turnResults = []uint64{id} + 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..9a9aefc46 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -664,6 +664,12 @@ type TaskReplyTag struct { // neither and is read exactly as it always was. Obligation string `json:"obligation,omitempty"` Revision uint64 `json:"revision,omitempty"` + // State, Stopped and Attempt 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 that still need the node as a fallback. + State TaskState `json:"state,omitempty"` + Stopped bool `json:"stopped,omitempty"` + Attempt int `json:"attempt,omitempty"` } // Event is one observable thing in a turn. A Submit returns a channel of @@ -2827,6 +2833,10 @@ type Agent struct { // [Agent.deliveringOwnedResult]), and that question has to survive a result // whose words were empty. turnResults []uint64 + // turnResultTags keeps each arrival's target and cancellation snapshot for + // routing. Unlike owedAsks, siblings with the same request stay distinct. + // It has the same turn lifetime as turnResults. + turnResultTags []TaskReplyTag // 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..49360cabd 100644 --- a/internal/session/wakecause.go +++ b/internal/session/wakecause.go @@ -102,9 +102,21 @@ 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.rememberResultTagLocked(tag) } } +// rememberResultTagLocked retains the first snapshot of each delivered attempt, +// even when another task inherited the same request. The caller holds a.mu. +func (a *Agent) rememberResultTagLocked(tag TaskReplyTag) { + for _, already := range a.turnResultTags { + if already.ID == tag.ID && already.Attempt == tag.Attempt { + return + } + } + a.turnResultTags = append(a.turnResultTags, tag) +} + // arrivedLocked records one result that landed in this turn, once. The caller // holds a.mu. func (a *Agent) arrivedLocked(id uint64) { @@ -152,6 +164,7 @@ 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 } // turnAsk is the ask this turn's endings are read against. @@ -236,6 +249,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, Attempt: n.attempt, } }