diff --git a/cmd/flynn/driveopts.go b/cmd/flynn/driveopts.go index e48dacbe..cf1c3ef6 100644 --- a/cmd/flynn/driveopts.go +++ b/cmd/flynn/driveopts.go @@ -83,7 +83,8 @@ func withPlanning() driveOption { // It is separate from withPlanning because the two carry different risk. Planning always // runs the producer, so a run's items visibly flip to proven and a run's report can say // how many were proven by execution; this adds the refusal on top, which is the part that -// can stop a run whose plan wrote checks no machine can run. It is staged, not optional: +// can stop a run whose plan wrote checks no machine can run. Without it an item whose +// check keeps failing is left unproven and the run moves on (see goal.LedgerState). It is staged, not optional: // the refusal is the point of keeping a ledger, and a build that never turns it on has a // plan that decides nothing. func withLedgerProof() driveOption { diff --git a/cmd/flynn/ledgerline_test.go b/cmd/flynn/ledgerline_test.go index 9d8f43f9..d02004a7 100644 --- a/cmd/flynn/ledgerline_test.go +++ b/cmd/flynn/ledgerline_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/ionalpha/flynn/goal" "github.com/ionalpha/flynn/harness" "github.com/ionalpha/flynn/llm/llmtest" ) @@ -78,3 +79,41 @@ func TestAPlannedRunSaysWhatItsLedgerWillDo(t *testing.T) { t.Fatalf("a planned run did not say its ledger proof is available and off:\n%s", out.String()) } } + +// TestLedgerOutcomeNamesWhatWasLeftUnproven: a run that does not require proof can finish +// with items unproven, and the operator learns that from the run's own output, with the +// failure that made the run move past each one, rather than from the model's answer. +func TestLedgerOutcomeNamesWhatWasLeftUnproven(t *testing.T) { + ledger, err := goal.AppendItems(nil, + goal.LedgerItem{Item: "add the endpoint", Verify: "curl --fail localhost/health"}, + goal.LedgerItem{Item: "write the report file", Verify: "findstr ok report.txt"}, + goal.LedgerItem{Item: "cover it with a test", Verify: "go test ./api/..."}, + ) + if err != nil { + t.Fatal(err) + } + var st goal.Status + st.SyncLedger(ledger) + if err := st.MarkProven(ledger[0].ID, "1", time.Unix(0, 0).UTC()); err != nil { + t.Fatal(err) + } + st.Ledger[1].Abandoned = true + st.Ledger[1].AbandonedReason = "its check failed the same way on 2 consecutive runs: exit 1" + + got := strings.Join(ledgerOutcomeLines(goal.Spec{Ledger: ledger}, st), "\n") + for _, want := range []string{ + "1 of 3 planned item(s) proven", + "unproven, moved past: write the report file (its check failed the same way on 2 consecutive runs: exit 1)", + "unproven: cover it with a test", + } { + if !strings.Contains(got, want) { + t.Fatalf("outcome missing %q:\n%s", want, got) + } + } + if strings.Contains(got, "add the endpoint") { + t.Fatalf("a proven item was listed as unproven:\n%s", got) + } + if lines := ledgerOutcomeLines(goal.Spec{}, goal.Status{}); lines != nil { + t.Fatalf("a goal with no ledger printed %q", lines) + } +} diff --git a/cmd/flynn/run.go b/cmd/flynn/run.go index 18776fd6..5291b6f4 100644 --- a/cmd/flynn/run.go +++ b/cmd/flynn/run.go @@ -95,6 +95,9 @@ func (r driveRequest) execute(ctx context.Context) (result, source string, trans } result, transcript, _, runErr := renderStream(w, events, r.verbose, r.cfg.observe) + for _, line := range r.ledgerOutcome(ctx, run.ID()) { + _, _ = fmt.Fprintln(w, line) + } if r.cfg.extAgent != nil { r.declareProvenance(runCtx, w, run) } @@ -104,6 +107,55 @@ func (r driveRequest) execute(ctx context.Context) (result, source string, trans return result, run.ID(), transcript, runErr } +// ledgerOutcome reads the run's goal after it settles and says how much of its plan was +// proven. A run that does not require proof can finish with items left unproven, and the +// model's answer is not where an operator should have to learn that: the model is told to +// name them, and this line is what holds when it does not. +func (r driveRequest) ledgerOutcome(ctx context.Context, id string) []string { + if r.rstore == nil { + return nil + } + rec, err := r.rstore.Get(context.WithoutCancel(ctx), goal.Kind, resource.Scope{}, id) + if err != nil { + return nil + } + spec, serr := goal.DecodeSpec(rec) + status, derr := goal.DecodeStatus(rec) + if serr != nil || derr != nil { + return nil + } + return ledgerOutcomeLines(spec, status) +} + +// ledgerOutcomeLines renders a settled goal's ledger: how many planned items were proven, +// then each item left unproven, with the failure that gave it up when the run moved past +// it. A goal with no ledger says nothing. +func ledgerOutcomeLines(spec goal.Spec, status goal.Status) []string { + if len(spec.Ledger) == 0 { + return nil + } + settled := make(map[string]goal.LedgerState, len(status.Ledger)) + proven := 0 + for _, st := range status.Ledger { + settled[st.ID] = st + if st.Proven { + proven++ + } + } + lines := []string{fmt.Sprintf(" ledger: %d of %d planned item(s) proven", proven, len(spec.Ledger))} + for _, it := range spec.Ledger { + st := settled[it.ID] + switch { + case st.Proven: + case st.Abandoned: + lines = append(lines, fmt.Sprintf(" unproven, moved past: %s (%s)", it.Item, st.AbandonedReason)) + default: + lines = append(lines, " unproven: "+it.Item) + } + } + return lines +} + // assembly is the request as the governed-run assembly takes it. A caller-supplied // toolset, an external agent CLI, fan-out and the plain conversation each get their own // mode of it, and all of them record the same session and apply the same governance. @@ -209,5 +261,5 @@ func ledgerLine(planned, proof bool) string { case proof: return " ledger: each item's check runs, and an item with no passing check on the record stops the run" } - return " ledger: each item's check runs and is recorded; --require-proof also stops the run on an item it cannot prove" + return " ledger: each item's check runs and is recorded, and an item whose check keeps failing is left unproven; --require-proof stops the run on it instead" } diff --git a/docs/HOST_BOUNDARY.md b/docs/HOST_BOUNDARY.md index a9b04f60..d8cd78b1 100644 --- a/docs/HOST_BOUNDARY.md +++ b/docs/HOST_BOUNDARY.md @@ -170,6 +170,16 @@ run's sandbox and its verdict goes on the record. The flag adds the refusal that those verdicts. A run prints which of the two applies before it starts (`ledgerLine` in `cmd/flynn/run.go`). +Without the flag the ledger is a record and never a stop. The checks are the model's own, +written before any work, and some can never pass (a quoting mistake) or check for +something the objective never asked for. An item whose executed check fails the same way +on consecutive runs, or names a program the host does not have, is marked abandoned: +still unproven on the record, with the failure that gave it up, and the run moves to the +next item and is told to name it in its answer. A planner that finds nothing checkable +leaves an empty ledger and the run builds. With the flag, the same item stops the run and +an empty ledger stalls it (`TestAnAdvisoryRunMovesPastAnItemWhoseCheckKeepsFailing`, +`TestARunThatRequiresProofStillStopsOnTheItem`). + `goal.Cleaner`: a nil cleaner means there is nothing external to tear down, which is true of the standalone binary. Child goals are removed through owner references. A delete with no cleaner completes instead of waiting on a finalizer diff --git a/docs/releases/v0.2.0.md b/docs/releases/v0.2.0.md index e4105bf9..b4bd3852 100644 --- a/docs/releases/v0.2.0.md +++ b/docs/releases/v0.2.0.md @@ -70,6 +70,7 @@ Run `flynn version` afterwards; it should name a `v0.2.0-rc` release. v0.1.3 can - `go test`, `npm install` and `pip install` failed inside the sandbox: a confined command could not write the caches these tools keep under the home directory, on macOS it could not write its own `TMPDIR`, and on Windows its `TEMP` was the workspace, where Go ignores a `go.mod`. Each sandbox now has a private scratch directory outside the workspace, writable by its commands and deleted when the sandbox closes. The temp variables, `GOCACHE`, `GOMODCACHE`, `npm_config_cache` and `PIP_CACHE_DIR` point into it unless you set them yourself. On Linux it is under `/var/tmp`, so the caches last from one command to the next. - On Windows a command the agent ran had no memory cap unless you passed `--max-memory`, and its process cap (4096) did not bound memory. A command that kept starting copies of itself, here a `go` shim on `PATH` that resolved back to itself, used all of a 95 GB machine's memory at a few hundred processes. With no `--max-memory`, a command and everything it starts may now commit at most half the machine's physical memory, in `flynn goal`, the interactive session, `flynn serve` and the MCP server. Pass `--max-memory` to set another cap. Local model servers are not affected. - `flynn resume` rebuilt a run without the guards it was started with. A planned goal resumed after a crash or kill then stalled at its first item check with `goal_no_verifier`, and a resumed run had no `--max-cost` or `--max-tokens` cap, no `--require-approval` gate, no `--irreversible` gate and no `--require-proof`. A resume now reads how the run was first assembled from its record and wires the same guards, with the spend already recorded still counted against the cap. A run driven by an external agent CLI is refused, since resume drives a native model. +- Without `--require-proof` a goal still stopped when a check it wrote for itself kept failing, though the ledger is documented as a record in that mode. A wrong check (a quoting mistake, or a check for a file the objective never asked for) ended the run after two cycles, and the model spent those cycles reshaping the work to fit the check. Such an item is now left unproven on the record with the failure that gave it up, the run moves to the next item, and the model is told to name it in its answer and not to bend the work to a check it thinks is wrong. An objective with nothing checkable, such as "run this and report", is no longer planned with invented checks, and an empty plan no longer stops the run. With `--require-proof` both still stop the run. A planned goal now ends by printing how many of its items were proven and naming each one that was not. - A confined signer on Windows can read its sealed key. - The interactive session could hang reading input when a multi-byte character arrived with no room left for it in the read buffer. - Markdown in the terminal could render a line one cell wider than the screen, so it wrapped. diff --git a/goal/environment_test.go b/goal/environment_test.go index c501b0ee..ca6fcfc7 100644 --- a/goal/environment_test.go +++ b/goal/environment_test.go @@ -172,7 +172,9 @@ func TestAMissingProgramStopsAsAnEnvironmentProblem(t *testing.T) { } ev := &fakeEvidence{} ver := &fakeVerifier{verdicts: []ItemVerdict{missing}} - h, w := producerHarness(t, ver, ev) + // Proof is required: an advisory run moves past the item instead (see + // TestAnAdvisoryRunMovesPastAnItemWhoseProgramIsMissing). + h, w := producerHarness(t, ver, ev, WithLedgerConvergence()) ledger := twoItemLedger(t) ref := h.plannedGoal(t, "missing-program", ledger) @@ -297,12 +299,13 @@ func TestCommandWords(t *testing.T) { } // runChecksTo drives EnvironmentRepeatLimit rounds of build, verify and observe against a -// planned two-item ledger whose verifier returns the given verdict every time. -func runChecksTo(t *testing.T, v ItemVerdict, rounds int) (Status, *fakeEvidence) { +// planned two-item ledger whose verifier returns the given verdict every time. opts go to +// the reconciler, as for producerHarness. +func runChecksTo(t *testing.T, v ItemVerdict, rounds int, opts ...Option) (Status, *fakeEvidence) { t.Helper() ev := &fakeEvidence{} ver := &fakeVerifier{verdicts: []ItemVerdict{v}} - h, w := producerHarness(t, ver, ev) + h, w := producerHarness(t, ver, ev, opts...) ref := h.plannedGoal(t, "checks", twoItemLedger(t)) h.setStatus(t, ref, func(st *Status) { st.Planned = true }) for range rounds { @@ -329,7 +332,7 @@ func TestAnAgentToolNamedInACheckStopsAsAnEnvironmentProblem(t *testing.T) { Executed: true, ExitCode: 1, Output: "'skill_read' is not recognized as an internal or external command,\noperable program or batch file.", Detail: "exit 1: 'skill_read' is not recognized as an internal or external command", - }, EnvironmentRepeatLimit) + }, EnvironmentRepeatLimit, WithLedgerConvergence()) if len(ev.recs) == 0 || ev.recs[0].Provenance == ProvenanceExecuted { t.Fatalf("recorded %+v: a check that never ran must not be on the record as executed", ev.recs) @@ -354,7 +357,7 @@ func TestAnAgentToolNamedInACheckStopsAsAnEnvironmentProblem(t *testing.T) { func TestACheckThatRanAndFailedAtExitOneStillCountsAsNotConverging(t *testing.T) { st, ev := runChecksTo(t, ItemVerdict{ Executed: true, ExitCode: 1, Output: "FAIL: TestHealth: got 500, want 200", Detail: "exit 1: FAIL", - }, VerdictRepeatLimit) + }, VerdictRepeatLimit, WithLedgerConvergence()) if len(ev.recs) == 0 || ev.recs[0].Provenance != ProvenanceExecuted { t.Fatalf("recorded %+v: a check that ran must stay on the record as executed", ev.recs) diff --git a/goal/ledger.go b/goal/ledger.go index cf9d8ad0..d077ca9c 100644 --- a/goal/ledger.go +++ b/goal/ledger.go @@ -63,6 +63,15 @@ type LedgerState struct { // note instead. ProvenAt *time.Time `json:"provenAt,omitempty"` Evidence string `json:"evidence,omitempty"` + // Abandoned marks an item the run stopped working without proving it, and + // AbandonedReason is the check failure that made it stop. Only a run that does not + // require proof abandons an item (see [Reconciler] and [WithLedgerConvergence]): its + // checks are the model's own guess at what done looks like, written before any work, + // and a check that can never pass would otherwise end a run that was told the record + // is only advisory. The item stays unproven, so the record still says the plan was + // not proven in full; the mark only moves the run on to the next item. + Abandoned bool `json:"abandoned,omitempty"` + AbandonedReason string `json:"abandonedReason,omitempty"` } // ItemID returns the content address of an item: the first ledgerIDLen hex characters of @@ -233,25 +242,57 @@ func (s Status) Unproven() []string { return out } -// CurrentItem returns the item the run is working: the first ledger item still unproven. -// It reports false when the ledger is empty or every item is proven. +// CurrentItem returns the item the run is working: the first ledger item neither proven +// nor abandoned. It reports false when the ledger is empty or every item is settled one +// way or the other. // // The selection is derived and not stored on the status, so there is no second // representation of "where the run is" to drift from the record. The reconciler, the // executor and the producer all compute the same answer from the same two facts. func (s Status) CurrentItem(ledger []LedgerItem) (LedgerItem, bool) { - proven := make(map[string]bool, len(s.Ledger)) + done := make(map[string]bool, len(s.Ledger)) for _, st := range s.Ledger { - proven[st.ID] = st.Proven + done[st.ID] = st.Proven || st.Abandoned } for _, it := range ledger { - if !proven[it.ID] { + if !done[it.ID] { return it, true } } return LedgerItem{}, false } +// Abandoned returns the items the run moved past without proving, in ledger order. +func (s Status) Abandoned() []LedgerState { + var out []LedgerState + for _, st := range s.Ledger { + if st.Abandoned && !st.Proven { + out = append(out, st) + } + } + return out +} + +// abandonCurrent marks the current item abandoned with reason and clears every count +// that was about its check, so the next item starts from nothing. The counts must go +// with it: left in place, the next item's first failure would read as the abandoned +// item's third and stop the run. +func (s *Status) abandonCurrent(ledger []LedgerItem, reason string) { + item, ok := s.CurrentItem(ledger) + if !ok { + return + } + for i := range s.Ledger { + if s.Ledger[i].ID == item.ID { + s.Ledger[i].Abandoned = true + s.Ledger[i].AbandonedReason = reason + } + } + s.ItemFeedback = "" + s.MissingCommand, s.MissingRepeat = "", 0 + s.LastVerdict, s.VerdictMark, s.VerdictRepeat = "", "", 0 +} + // LedgerSettled reports whether the ledger has items and all of them are proven. An empty // ledger is not settled: a goal with nothing planned has not started. func (s Status) LedgerSettled() bool { diff --git a/goal/ledger_test.go b/goal/ledger_test.go index 298d6227..35abe1c5 100644 --- a/goal/ledger_test.go +++ b/goal/ledger_test.go @@ -46,7 +46,7 @@ type planHarness struct { p *fakePlanner } -func newPlanHarness(t *testing.T, stop StopEvaluator, planner *fakePlanner) *planHarness { +func newPlanHarness(t *testing.T, stop StopEvaluator, planner *fakePlanner, opts ...Option) *planHarness { t.Helper() m := clock.NewManual(time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)) reg := resource.NewRegistry() @@ -58,7 +58,7 @@ func newPlanHarness(t *testing.T, stop StopEvaluator, planner *fakePlanner) *pla } store := resource.NewMemory(reg, resource.WithClock(m)) q := jobs.NewMemory(jobs.WithClock(m)) - gr := NewReconciler(store, q, m, stop, WithPlanning()) + gr := NewReconciler(store, q, m, stop, append([]Option{WithPlanning()}, opts...)...) w := NewWorker(store, q, m, &fakeExec{}, WithPlanner(planner), WithLease(time.Minute)) h := &harness{ctx: context.Background(), store: store, jobs: q, gr: gr, clk: m} return &planHarness{harness: h, w: w, p: planner} @@ -471,11 +471,11 @@ func TestPlanningDoesNotSpendTheBuildBudget(t *testing.T) { } } -// TestAnEmptyPlanStallsTheGoal: a planner that produced nothing leaves no -// definition of done. Building anyway is how a run claims success against a record -// that never said what success was. -func TestAnEmptyPlanStallsTheGoal(t *testing.T) { - h := newPlanHarness(t, neverStop{}, &fakePlanner{}) +// TestAnEmptyPlanStallsAGoalThatRequiresProof: a planner that produced nothing leaves +// no definition of done. Building anyway is how a run claims success against a record +// that never said what success was, and a run that requires proof has nothing to prove. +func TestAnEmptyPlanStallsAGoalThatRequiresProof(t *testing.T) { + h := newPlanHarness(t, neverStop{}, &fakePlanner{}, WithLedgerConvergence()) ref := h.createGoal(t, "g", Spec{Objective: "o", StopCondition: "c"}) h.reconcile(t, ref) @@ -493,6 +493,27 @@ func TestAnEmptyPlanStallsTheGoal(t *testing.T) { } } +// TestAnEmptyPlanBuildsAGoalThatDoesNotRequireProof: completion in such a run is the +// model's call whatever the ledger says, and an objective whose result is only a report +// has nothing for a check to read. Stopping it for an empty plan would stop exactly the +// runs the planner was told to leave unplanned. +func TestAnEmptyPlanBuildsAGoalThatDoesNotRequireProof(t *testing.T) { + h := newPlanHarness(t, neverStop{}, &fakePlanner{}) + ref := h.createGoal(t, "g", Spec{Objective: "o", StopCondition: "c"}) + + h.reconcile(t, ref) + if _, err := h.w.ProcessOnce(h.ctx); err != nil { + t.Fatalf("worker: %v", err) + } + h.reconcile(t, ref) + + st := h.status(t, ref) + if st.Phase == PhaseStalled { + t.Fatalf("an advisory run stalled on an empty plan: %q", st.Message) + } + h.completeJob(t, StepJobKind) // the next dispatch is a build step +} + // TestPlanningRunsOnce pins that the planning mark, not the ledger's emptiness, is // what ends the phase, and that a re-reconcile does not plan the goal again. func TestPlanningRunsOnce(t *testing.T) { diff --git a/goal/ledgerloop.go b/goal/ledgerloop.go index 44feb4f7..4d47403b 100644 --- a/goal/ledgerloop.go +++ b/goal/ledgerloop.go @@ -61,3 +61,30 @@ func (g *Reconciler) refuseCompletion(status *Status, recorded []Verification) { status.stall("LedgerUnproven", fmt.Sprintf("completion reported with %d planned item(s) unproven: %s", len(reasons), strings.Join(reasons, "; ")), g.clk.Now()) } + +// moveOffUnprovableItem abandons the current item, in a run that does not require proof, +// once its check has failed the same way twice or named a missing program twice. feedback +// is the executed check failure this cycle was counted on, or "" when none ran. +// +// The checks are the model's own, written before any work, and some can never pass (a +// quoting mistake) or check for a file the objective never asked for. Left to the +// non-convergence guard, either one stops a run whose ledger the operator was told is +// only a record. The item stays unproven, with the failure that gave it up. +// +// A run that requires proof stops on the item instead, and a repeated refusal with no +// check failure in it is still the non-convergence stop. +func (g *Reconciler) moveOffUnprovableItem(spec Spec, status *Status, feedback string) { + if g.ledgerConverge || !g.ledgerGated() || !status.Planned { + return + } + var reason string + switch { + case status.StalledForEnvironment(): + reason = status.EnvironmentReason() + case feedback != "" && status.StalledForNonConvergence(): + reason = fmt.Sprintf("its check failed the same way on %d consecutive runs: %s", status.VerdictRepeat, feedback) + default: + return + } + status.abandonCurrent(spec.Ledger, reason) +} diff --git a/goal/ledgerloop_advisory_test.go b/goal/ledgerloop_advisory_test.go new file mode 100644 index 00000000..8dd49653 --- /dev/null +++ b/goal/ledgerloop_advisory_test.go @@ -0,0 +1,174 @@ +package goal + +// A run that does not require proof treats its ledger as a record, not a stop: an item +// whose check keeps failing the same way, or whose check names a program the host does +// not have, is left unproven and the run moves to the next item. A run that requires +// proof stops on the same item, and a repeated refusal with no check behind it stops +// either kind of run. + +import ( + "strings" + "testing" +) + +// runAdvisory drives rounds of build, verify and observe against a planned two-item +// ledger whose verifier returns v every time, and returns the status and the items the +// verifier was asked about, in order. +func runAdvisory(t *testing.T, v ItemVerdict, rounds int, opts ...Option) (Status, []string, []LedgerItem) { + t.Helper() + ev := &fakeEvidence{} + ver := &fakeVerifier{verdicts: []ItemVerdict{v}} + h, w := producerHarness(t, ver, ev, opts...) + ledger := twoItemLedger(t) + ref := h.plannedGoal(t, "advisory", ledger) + for range rounds { + h.reconcile(t, ref) + if _, err := w.ProcessOnce(h.ctx); err != nil { + t.Fatalf("build step: %v", err) + } + h.reconcile(t, ref) + if _, err := w.ProcessOnce(h.ctx); err != nil { + t.Fatalf("verify step: %v", err) + } + h.reconcile(t, ref) + } + return h.status(t, ref), ver.asked, ledger +} + +// failingCheck is a check that ran and failed with ordinary output: the shape of a check +// the model wrote wrong, which no amount of work will make pass. +var failingCheck = ItemVerdict{Executed: true, ExitCode: 1, Output: "no such line", Detail: "exit 1: no such line"} + +// TestAnAdvisoryRunMovesPastAnItemWhoseCheckKeepsFailing is the field failure: a check +// that can never pass ended a run that was told its ledger was only a record. The item is +// left unproven with the failure that gave it up, and the next item's check runs. +func TestAnAdvisoryRunMovesPastAnItemWhoseCheckKeepsFailing(t *testing.T) { + st, asked, ledger := runAdvisory(t, failingCheck, VerdictRepeatLimit+1) + + if st.Phase == PhaseStalled { + t.Fatalf("an advisory run stopped on its own check: %s", st.Message) + } + first := st.Ledger[0] + if !first.Abandoned || first.Proven { + t.Fatalf("first item = %+v, want it left unproven and marked abandoned", first) + } + if !strings.Contains(first.AbandonedReason, "no such line") { + t.Fatalf("abandon reason %q does not say what the check reported", first.AbandonedReason) + } + if got := asked[len(asked)-1]; got != ledger[1].ID { + t.Fatalf("last check run was for %q, want the next item %q", got, ledger[1].ID) + } + if st.VerdictRepeat > 1 { + t.Fatalf("repeat count %d carried over from the abandoned item onto the next", st.VerdictRepeat) + } +} + +// TestAnAdvisoryRunMovesPastAnItemWhoseProgramIsMissing: the same promise for a check +// that names a program this host does not have. +func TestAnAdvisoryRunMovesPastAnItemWhoseProgramIsMissing(t *testing.T) { + missing := ItemVerdict{ + Executed: true, ExitCode: 9009, + Output: "'test' is not recognized as an internal or external command,\noperable program or batch file.", + Detail: "exit 9009: 'test' is not recognized as an internal or external command", + } + st, _, _ := runAdvisory(t, missing, EnvironmentRepeatLimit) + + if st.Phase == PhaseStalled { + t.Fatalf("an advisory run stopped on a missing program: %s", st.Message) + } + if !st.Ledger[0].Abandoned || !strings.Contains(st.Ledger[0].AbandonedReason, `"test"`) { + t.Fatalf("first item = %+v, want it abandoned naming the missing program", st.Ledger[0]) + } + if st.MissingCommand != "" || st.MissingRepeat != 0 { + t.Fatalf("missing-program count %q x%d survived the abandon", st.MissingCommand, st.MissingRepeat) + } +} + +// TestAnAdvisoryRunWithEveryCheckFailingKeepsBuilding: once every item is abandoned there +// is nothing left to check, so the run builds toward its answer rather than stalling or +// dispatching a verification for no item. +func TestAnAdvisoryRunWithEveryCheckFailingKeepsBuilding(t *testing.T) { + st, _, _ := runAdvisory(t, failingCheck, 2*VerdictRepeatLimit) + + if st.Phase == PhaseStalled { + t.Fatalf("stalled with every item abandoned: %s", st.Message) + } + if got := len(st.Abandoned()); got != 2 { + t.Fatalf("abandoned %d items, want both", got) + } + if _, ok := st.CurrentItem(twoItemLedger(t)); ok { + t.Fatal("a current item remained with every item abandoned") + } +} + +// TestARunThatRequiresProofStillStopsOnTheItem is the control: with proof required the +// unproven item is the reason to stop, so the run stalls naming the failure and nothing is +// marked abandoned. +func TestARunThatRequiresProofStillStopsOnTheItem(t *testing.T) { + st, _, _ := runAdvisory(t, failingCheck, VerdictRepeatLimit, WithLedgerConvergence()) + + if st.Phase != PhaseStalled { + t.Fatalf("phase = %q, want a run that requires proof stopped on its failing item", st.Phase) + } + if reason := stalledReason(st); reason != "NotConverging" { + t.Fatalf("stalled reason = %q, want NotConverging (message %q)", reason, st.Message) + } + if len(st.Abandoned()) != 0 { + t.Fatalf("a run that requires proof abandoned %d items", len(st.Abandoned())) + } +} + +// TestARepeatedRefusalWithNoCheckBehindItStillStops: moving past an item is about a +// failing check. An evaluator saying the same thing about the whole goal, with no check +// failure in it, is the non-convergence the guard was built for, advisory or not. +func TestARepeatedRefusalWithNoCheckBehindItStillStops(t *testing.T) { + ev := &fakeEvidence{} + // The check could not be run, so the cycle carries no executed failure. + ver := &fakeVerifier{verdicts: []ItemVerdict{{Detail: "no sandbox on this host"}}} + h := newHarness(t, refusing{"the deploy target does not exist"}, WithLedgerGate(ev, newGate(t, RequireExecuted()))) + w := NewWorker(h.store, h.jobs, h.clk, &fakeExec{}, WithItemVerification(ver, ev)) + ref := h.plannedGoal(t, "refused", twoItemLedger(t)) + for range VerdictRepeatLimit { + h.reconcile(t, ref) + if _, err := w.ProcessOnce(h.ctx); err != nil { + t.Fatalf("build step: %v", err) + } + h.reconcile(t, ref) + if _, err := w.ProcessOnce(h.ctx); err != nil { + t.Fatalf("verify step: %v", err) + } + h.reconcile(t, ref) + } + + st := h.status(t, ref) + if reason := stalledReason(st); reason != "NotConverging" { + t.Fatalf("stalled reason = %q, want NotConverging (phase %q, message %q)", reason, st.Phase, st.Message) + } + if len(st.Abandoned()) != 0 { + t.Fatalf("abandoned %d items on a refusal that had no failing check in it", len(st.Abandoned())) + } +} + +// TestAnAbandonMarkSurvivesTheDispatchWrite: the reconciler sets the mark and a worker's +// copy written in the same window has never seen it. The merge carries it, and reports +// that it did, so the caller does not restore the abandoned item's feedback. +func TestAnAbandonMarkSurvivesTheDispatchWrite(t *testing.T) { + ledger := twoItemLedger(t) + mine := []LedgerState{{ID: ledger[0].ID, Abandoned: true, AbandonedReason: "kept failing"}, {ID: ledger[1].ID}} + fresh := []LedgerState{{ID: ledger[0].ID}, {ID: ledger[1].ID}} + + merged, added := mergeLedger(mine, fresh) + if !added { + t.Fatal("the merge did not report carrying an abandon mark the record lacked") + } + if !merged[0].Abandoned || merged[0].AbandonedReason != "kept failing" { + t.Fatalf("merged = %+v, want the abandon mark and its reason kept", merged) + } + + // A proof on the record outranks a stale abandon mark: an item proven is proven. + proven := []LedgerState{{ID: ledger[0].ID, Proven: true, Evidence: "1"}, {ID: ledger[1].ID}} + merged, _ = mergeLedger(mine, proven) + if !merged[0].Proven || merged[0].Abandoned { + t.Fatalf("merged = %+v, want the record's proof kept over an abandon mark", merged) + } +} diff --git a/goal/ledgerloop_test.go b/goal/ledgerloop_test.go index 2ac72735..b587dee1 100644 --- a/goal/ledgerloop_test.go +++ b/goal/ledgerloop_test.go @@ -153,10 +153,11 @@ func (h *harness) completeJob(t *testing.T, kind string) { // producerHarness assembles a reconciler with the ledger loop closed and a worker that // shares its record, so a test drives the real dispatch-and-observe round trip. Passing a // nil verifier builds the worker without a producer, which is the misconfiguration the -// loud-failure test is about. -func producerHarness(t *testing.T, ver ItemVerifier, ev Evidence) (*harness, *Worker) { +// loud-failure test is about. opts are added to the reconciler, so a test can run the +// loop with proof required (WithLedgerConvergence) or advisory. +func producerHarness(t *testing.T, ver ItemVerifier, ev Evidence, opts ...Option) (*harness, *Worker) { t.Helper() - h := newHarness(t, neverMet{}, WithLedgerGate(ev, newGate(t, RequireExecuted()))) + h := newHarness(t, neverMet{}, append([]Option{WithLedgerGate(ev, newGate(t, RequireExecuted()))}, opts...)...) wopts := []WorkerOption{} if ver != nil { wopts = append(wopts, WithItemVerification(ver, ev)) diff --git a/goal/persist.go b/goal/persist.go index fca31d94..af0cb875 100644 --- a/goal/persist.go +++ b/goal/persist.go @@ -46,15 +46,15 @@ func (g *Reconciler) recordDispatch(ctx context.Context, r resource.Resource, st // rather than taken from either side. Carrying the worker's copy wholesale // would drop a proof this very pass admitted; carrying ours would drop an item // a concurrent planning step just appended. - var proved bool - status.Ledger, proved = mergeLedger(status.Ledger, cur.Ledger) + var settled bool + status.Ledger, settled = mergeLedger(status.Ledger, cur.Ledger) // The failing check's detail is the worker's, and a verify job lands in this // same window: it is enqueued before this reservation is written, so a worker // that claims and finishes it first would otherwise have it erased here, and the // next build step would be asked to fix an item without being told what its own - // check reported. The exception is a pass that just proved something, which is - // the pass that cleared the feedback on purpose. - if !proved { + // check reported. The exception is a pass that just proved or abandoned an item, + // which is the pass that cleared the feedback on purpose. + if !settled { status.ItemFeedback = cur.ItemFeedback status.MissingCommand, status.MissingRepeat = cur.MissingCommand, cur.MissingRepeat } @@ -72,19 +72,22 @@ func (g *Reconciler) recordDispatch(ctx context.Context, r resource.Resource, st } // mergeLedger reconciles this reconcile's per-item state with the copy on the freshest -// record, and reports whether the merge carried a proof the record did not already have. +// record, and reports whether the merge carried a proof or an abandon mark the record did +// not already have. // // The shape of the ledger comes from theirs: it is the copy written against the newest // version of the goal, so a planning step that appended items in this window is respected // and this write does not shorten the plan. The proven marks come from whichever side has // them, because a proof only ever goes from unset to set and never back. An item that is // proven on either copy is proven, and the earlier proof's evidence and timestamp are kept -// intact rather than restamped. +// intact rather than restamped. An abandon mark travels the same way, and reporting it as +// carried keeps the caller from restoring the abandoned item's check feedback, which would +// read as the next item's first failure. func mergeLedger(mine, theirs []LedgerState) ([]LedgerState, bool) { - proofs := make(map[string]LedgerState, len(mine)) + settled := make(map[string]LedgerState, len(mine)) for _, st := range mine { - if st.Proven { - proofs[st.ID] = st + if st.Proven || st.Abandoned { + settled[st.ID] = st } } added := false @@ -94,9 +97,15 @@ func mergeLedger(mine, theirs []LedgerState) ([]LedgerState, bool) { if st.Proven { continue } - if p, ok := proofs[st.ID]; ok { + p, ok := settled[st.ID] + switch { + case !ok: + case p.Proven: out[i] = p added = true + case !st.Abandoned: + out[i].Abandoned, out[i].AbandonedReason = true, p.AbandonedReason + added = true } } return out, added diff --git a/goal/reconciler.go b/goal/reconciler.go index 9c45e954..c1e73c30 100644 --- a/goal/reconciler.go +++ b/goal/reconciler.go @@ -357,7 +357,9 @@ func (g *Reconciler) judgeOrStep(ctx context.Context, r resource.Resource, spec // just spoken, and under the ledger gate the item's check has just been settled, so // the feedback describes the cycle that ended rather than the one before it. if !met && g.countsAsCycle(obs.completed, obs.kind, *status) { - status.ObserveVerdict(reason, status.ExecutedFeedback(spec.Ledger, recorded)) + feedback := status.ExecutedFeedback(spec.Ledger, recorded) + status.ObserveVerdict(reason, feedback) + g.moveOffUnprovableItem(spec, status, feedback) } if met { return g.converge(ctx, r, spec, status, specHash, reason) diff --git a/goal/step.go b/goal/step.go index 9435dced..ce3e6fae 100644 --- a/goal/step.go +++ b/goal/step.go @@ -73,9 +73,15 @@ func (g *Reconciler) observeInFlight(ctx context.Context, r resource.Resource, s // planGate holds a planning goal at its planning phase and reports whether it handled // the reconcile. The first dispatch is a planning step, and the stop condition is not -// evaluated until there is a ledger to evaluate it against. A planner that produced -// nothing leaves a goal with no definition of done, which stalls: building anyway is how -// a run claims success against a record that never said what success was. +// evaluated until there is a ledger to evaluate it against. +// +// A planner that produced nothing stalls a run that requires proof: with no items there +// is nothing to prove, and building anyway is how a run claims success against a record +// that never said what success was. A run that does not require proof builds instead. +// Its completion was always the model's call, so an empty ledger takes nothing away from +// it, and some objectives have no checkable result (run a command and report what it +// printed); planning a check for one is how a run ends up inventing a file for the check +// to find and then working to create it. func (g *Reconciler) planGate(ctx context.Context, r resource.Resource, spec Spec, status Status, specHash string) (reconcile.Result, bool, error) { switch { case !g.planning: @@ -83,7 +89,7 @@ func (g *Reconciler) planGate(ctx context.Context, r resource.Resource, spec Spe case !status.Planned: res, err := g.dispatch(ctx, r, status, specHash, PlanJobKind, PhasePlanning, "PlanDispatched") return res, true, err - case len(spec.Ledger) == 0: + case len(spec.Ledger) == 0 && g.ledgerConverge: status.stall("EmptyLedger", "planning produced an empty ledger", g.clk.Now()) res, err := g.terminal(ctx, r, status, specHash) return res, true, err diff --git a/mission/itembrief_test.go b/mission/itembrief_test.go index 1b47fa47..3b796ac5 100644 --- a/mission/itembrief_test.go +++ b/mission/itembrief_test.go @@ -140,3 +140,67 @@ func ledgerGoal(t *testing.T, spec goal.Spec, st goal.Status) resource.Resource } return resource.Resource{APIVersion: goal.GroupVersion, Kind: goal.Kind, Name: "brief-run", Spec: raw, Status: enc} } + +// TestAFailingCheckIsReportedNotWorkedAround: the check is the model's own guess at what +// done looks like, so the turn that carries its failure also tells the run that a wrong +// check is to be said in the answer. Without it a run told "the check failed" reshapes +// the work until a mistaken check passes, which is the field failure this guards. +func TestAFailingCheckIsReportedNotWorkedAround(t *testing.T) { + spec, st := briefFixture(t) + st.ItemFeedback = "exit 1: findstr found nothing" + e := NewExecutor(llmtest.NewScripted(llmtest.SayText("fixing"))) + + raw, err := e.Execute(context.Background(), ledgerGoal(t, spec, st)) + if err != nil { + t.Fatalf("step: %v", err) + } + cp, err := decodeCheckpoint(raw) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(transcript(cp), "do not change the work to fit it") { + t.Fatalf("the failing check's turn did not say a wrong check is to be reported:\n%s", transcript(cp)) + } +} + +// TestTheRunHearsOnceThatItsPlanClosedWithItemsUnproven: when the last item settles by +// being abandoned there is no next item to brief, so the run is told once that its plan is +// over, which items were left unproven and why, and that its answer must say so. +func TestTheRunHearsOnceThatItsPlanClosedWithItemsUnproven(t *testing.T) { + spec, st := briefFixture(t) + // The conversation was last briefed on the second item, which has now been abandoned. + cp := checkpoint{Item: spec.Ledger[1].ID} + st.Ledger[1].Abandoned = true + st.Ledger[1].AbandonedReason = "its check failed the same way on 2 consecutive runs: exit 1" + + briefTurn(&cp, spec, st) + text := transcript(cp) + for _, want := range []string{ + "Every planned item is settled", + "[x] 1. add the endpoint", + "[-] 2. cover it with a test", + "cover it with a test: its check failed the same way on 2 consecutive runs: exit 1", + "Say in your final answer", + } { + if !strings.Contains(text, want) { + t.Fatalf("closing brief missing %q:\n%s", want, text) + } + } + + briefTurn(&cp, spec, st) + if n := strings.Count(transcript(cp), "Every planned item is settled"); n != 1 { + t.Fatalf("the closing brief appeared %d times, want once", n) + } +} + +// TestAFullyProvenPlanClosesWithoutANote: nothing was left unproven, so there is nothing +// to report and the run is not told anything extra. +func TestAFullyProvenPlanClosesWithoutANote(t *testing.T) { + spec, st := briefFixture(t) + if err := st.MarkProven(spec.Ledger[1].ID, "2", time.Unix(0, 0).UTC()); err != nil { + t.Fatal(err) + } + if brief := ledgerClosed(spec, st); brief != "" { + t.Fatalf("a fully proven plan produced a closing brief: %q", brief) + } +} diff --git a/mission/mission.go b/mission/mission.go index 57b6ecb6..d247d3ff 100644 --- a/mission/mission.go +++ b/mission/mission.go @@ -451,14 +451,26 @@ func (e *Executor) bindRunContext(ctx context.Context, r resource.Resource, spec func briefTurn(cp *checkpoint, spec goal.Spec, status goal.Status) { // Only at an item boundary: re-stating the same item each step would copy it into the // transcript once per turn for no new information. - if item, ok := status.CurrentItem(spec.Ledger); ok && item.ID != cp.Item { + item, ok := status.CurrentItem(spec.Ledger) + switch { + case ok && item.ID != cp.Item: cp.Messages = appendNudge(cp.Messages, itemBrief(spec, status, item)) cp.Item = item.ID + case !ok && cp.Item != "": + // The last item settled with no new one to brief, so this is the one turn the run + // hears that its plan is over, and what was left unproven in it. + if brief := ledgerClosed(spec, status); brief != "" { + cp.Messages = appendNudge(cp.Messages, brief) + } + cp.Item = "" } // Every turn the failure survives: the item has not moved, and the failing output is - // the most useful thing the run can be told. + // the most useful thing the run can be told. The check is the model's own guess at + // what done looks like, so a wrong one is to be reported, not worked around: reshaping + // the work until a mistaken check passes hides exactly what the ledger exists to show. if status.ItemFeedback != "" { - cp.Messages = appendNudge(cp.Messages, "The last run of this item's check did not pass: "+status.ItemFeedback) + cp.Messages = appendNudge(cp.Messages, "The last run of this item's check did not pass: "+status.ItemFeedback+ + "\nIf the work is right and the check itself is wrong, do not change the work to fit it: say so in your final answer.") } // Empty on a turn nobody has redirected. if brief := goal.SteerBrief(spec, status); brief != "" { @@ -557,27 +569,78 @@ func (e *Executor) settleTurn(ctx context.Context, r resource.Resource, turn int // and because seeing the proven items above and the unproven ones below is what makes the // position mean something. func itemBrief(spec goal.Spec, status goal.Status, current goal.LedgerItem) string { - proven := make(map[string]bool, len(status.Ledger)) - for _, st := range status.Ledger { - proven[st.ID] = st.Proven - } var b strings.Builder b.WriteString("Current item: ") b.WriteString(current.Item) b.WriteString("\nIt is proven when: ") b.WriteString(current.Verify) b.WriteString("\n\nWork this item, not the whole objective. The plan:\n") + writePlan(&b, spec, status, current.ID) + if len(status.Abandoned()) > 0 { + b.WriteString(abandonedNote) + } + return strings.TrimRight(b.String(), "\n") +} + +// abandonedNote is what the run is told about an item marked [-]: its check stopped +// telling the run anything new, so the run moved past it unproven, and the final answer +// is where an operator learns that. +const abandonedNote = "Items marked [-] were left unproven because their checks kept failing the same way. " + + "Say in your final answer which they are and what their checks reported.\n" + +// ledgerClosed is the brief for the turn after the last item settles: the plan with its +// final marks, and the instruction to report what was left unproven. It is "" when every +// item was proven, since a fully proven plan has nothing to report beyond the answer. +func ledgerClosed(spec goal.Spec, status goal.Status) string { + abandoned := status.Abandoned() + if len(abandoned) == 0 { + return "" + } + var b strings.Builder + b.WriteString("Every planned item is settled. The plan:\n") + writePlan(&b, spec, status, "") + b.WriteString(abandonedNote) + for _, st := range abandoned { + fmt.Fprintf(&b, "- %s: %s\n", itemText(spec, st.ID), st.AbandonedReason) + } + b.WriteString("Finish the objective and give your final answer.") + return b.String() +} + +// writePlan renders the ledger one line per item with its mark: [x] proven, [-] left +// unproven, [>] the item being worked, [ ] not reached. +func writePlan(b *strings.Builder, spec goal.Spec, status goal.Status, current string) { + marks := make(map[string]string, len(status.Ledger)) + for _, st := range status.Ledger { + switch { + case st.Proven: + marks[st.ID] = "[x]" + case st.Abandoned: + marks[st.ID] = "[-]" + } + } for i, it := range spec.Ledger { - mark := "[ ]" + mark, ok := marks[it.ID] switch { - case proven[it.ID]: - mark = "[x]" - case it.ID == current.ID: + case ok: + case it.ID == current: mark = "[>]" + default: + mark = "[ ]" } - fmt.Fprintf(&b, "%s %d. %s\n", mark, i+1, it.Item) + fmt.Fprintf(b, "%s %d. %s\n", mark, i+1, it.Item) } - return strings.TrimRight(b.String(), "\n") +} + +// itemText is the planner's wording of the item with id, or the id when the ledger no +// longer carries it. +func itemText(spec goal.Spec, id string) string { + for _, it := range spec.Ledger { + if it.ID == id { + return it.Item + } + } + return id } // appendNudge folds a stalling warning into the transcript as user-visible text without diff --git a/mission/planner.go b/mission/planner.go index b46acfc5..247745e2 100644 --- a/mission/planner.go +++ b/mission/planner.go @@ -36,7 +36,9 @@ Expand the objective below into the concrete units of work it actually implies: The check is run, not read. It must be a single shell command line, and it is treated as proof only when that command exits 0. Write the command, not a description of it: "go test ./..." is a check; "go test ./... passes" and "it looks done" are not. Reach for a command whose exit code already means the right thing (a test run, a build, a linter, grep for the thing you added, curl --fail against the endpoint), and compose with && when one item needs two conditions. -An item you cannot express as a command is usually an item phrased too vaguely to check. Sharpen it, or split it into parts that can be. A check that cannot be run does not prove its item: it is recorded as unproven, and the goal will not complete on it. +An item you cannot express as a command is usually an item phrased too vaguely to check. Sharpen it, or split it into parts that can be. A check that cannot be run does not prove its item: it is recorded as unproven. + +A check reads what the work leaves behind: a file, a build, a passing test, a running service. An objective whose whole result is what you report back (run a command and say what it printed, answer a question, look something up) leaves nothing for a check to read, so it has no items. Never plan an item, or a file, that exists only so a check has something to find. Respond with ONLY a JSON array, no prose before or after, no code fence. Each element is an object with exactly two string fields: - "item": what the work is, in one clear sentence. @@ -45,7 +47,7 @@ Respond with ONLY a JSON array, no prose before or after, no code fence. Each el Example: [{"item":"Add a /health endpoint","verify":"curl --fail --silent http://localhost:8080/health"}] -If the objective genuinely needs no work — it is already satisfied, or it is not a work request — respond with an empty array []. Do not invent filler items to look busy. Every item you return will be held to its verify clause.` +If the objective needs no checkable work (it is already satisfied, it is not a work request, or its result is only what you report), respond with an empty array []. Do not invent filler items to look busy. Every item you return will be held to its verify clause.` // plannerCorrections is how many times the planner sends a plan back to the model for a // check that cannot run before it gives the sample up as a transient failure. One round