From 778e5ab638e6c5a6e3c2a7a966c316b93be3b288 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:15:24 +0200 Subject: [PATCH 1/4] feat: require tool evidence for declared plan checks --- Makefile | 4 + cmd/odek-eval/main.go | 24 ++ docs/EVALS.md | 31 ++ docs/PLANNING.md | 92 +++++- internal/eval/eval.go | 377 ++++++++++++++++++++++++ internal/eval/eval_test.go | 51 ++++ internal/loop/completion_checks.go | 63 ++++ internal/loop/completion_checks_test.go | 186 ++++++++++++ internal/loop/loop.go | 34 ++- internal/loop/plan.go | 156 ++++++++-- internal/loop/plan_checks.go | 366 +++++++++++++++++++++++ internal/loop/plan_checks_test.go | 194 ++++++++++++ internal/loop/plan_coverage_test.go | 3 +- internal/loop/plan_test.go | 7 +- 14 files changed, 1542 insertions(+), 46 deletions(-) create mode 100644 cmd/odek-eval/main.go create mode 100644 docs/EVALS.md create mode 100644 internal/eval/eval.go create mode 100644 internal/eval/eval_test.go create mode 100644 internal/loop/completion_checks.go create mode 100644 internal/loop/completion_checks_test.go create mode 100644 internal/loop/plan_checks.go create mode 100644 internal/loop/plan_checks_test.go diff --git a/Makefile b/Makefile index add9a69e..15ddd195 100644 --- a/Makefile +++ b/Makefile @@ -42,6 +42,10 @@ test-internal: ## Run only internal package tests (excludes cmd/odek) test-cmd: ## Run cmd/odek unit tests (env-gated E2E/sandbox suites skipped) $(GO) test -short -count=1 -timeout 600s ./cmd/odek -skip 'TestE2E_|TestMCPE2E|TestSandbox' +.PHONY: eval +eval: ## Run deterministic local runtime evaluations (no credentials or external network) + $(GO) run ./cmd/odek-eval + .PHONY: test-cli-serve test-cli-serve: ## Serve/WS/REST headless-run surface only (~1 min) $(GO) test -short -count=1 -timeout 300s -run 'TestServe|TestWS|TestRestRun|TestPrompt|TestHandlePrompt' ./cmd/odek diff --git a/cmd/odek-eval/main.go b/cmd/odek-eval/main.go new file mode 100644 index 00000000..cac6e44a --- /dev/null +++ b/cmd/odek-eval/main.go @@ -0,0 +1,24 @@ +// odek-eval runs deterministic, localhost-only runtime evaluations. +package main + +import ( + "context" + "encoding/json" + "fmt" + "os" + + "github.com/BackendStack21/odek/internal/eval" +) + +func main() { + report := eval.Run(context.Background(), eval.Scenarios()) + enc := json.NewEncoder(os.Stdout) + enc.SetIndent("", " ") + if err := enc.Encode(report); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + if report.Failed != 0 { + os.Exit(1) + } +} diff --git a/docs/EVALS.md b/docs/EVALS.md new file mode 100644 index 00000000..b15c1c24 --- /dev/null +++ b/docs/EVALS.md @@ -0,0 +1,31 @@ +# Runtime evaluation harness + +`make eval` runs `cmd/odek-eval`, a deterministic harness around the +production `internal/loop.Engine`. It starts an OpenAI-compatible provider on +localhost, feeds scripted assistant replies, and exposes small stateful +fixture tools. No credentials, external network, or live model is used. + +Each scenario keeps its model messages separate from its oracle. The oracle +checks fixture state and observed tool calls, so an assistant saying “done” +cannot by itself make a task pass. The JSON report contains per-case scenario +status, independently determined `task_success`, tool calls, synthetic +scripted-provider token counts, and elapsed milliseconds. `false_completion_rate` is the fraction of +cases whose oracle explicitly marked a success claim while required fixture +state was absent; it is a regression signal, not a model-quality score. +`cost_known` is always false for the shipped harness; it does not configure +prices or estimate cost. + +The initial suite covers verified artifact work, a failed read followed by a +false success claim, unrelated reads after a write, transient failure and +recovery, cancellation, and plan acceptance checks for success, failed +evidence, and missing evidence. Negative cases can still be scenario passes +when the oracle correctly records that the task did not succeed. The plan +cases use the production `plan` tool and `PlanStore`, including the runtime +incomplete marker for failed or missing evidence. + +The reusable `internal/eval.RunWithOptions` API accepts a per-case client +factory. An application may use that adapter to compare a separately +authorized real provider, but it must provide its own credentials, network +policy, and model-message adapter. The shipped CLI intentionally does not +evaluate live-model intelligence. Cost reporting stays unknown because the +harness does not configure token prices. diff --git a/docs/PLANNING.md b/docs/PLANNING.md index f918a6d5..ab79c567 100644 --- a/docs/PLANNING.md +++ b/docs/PLANNING.md @@ -9,9 +9,9 @@ protected system message that is visible on every iteration — immune to context trimming, survival trim, and process restarts. Planning fits the ReAct loop without altering it: observe → think → act is -unchanged, plan calls ride ordinary parallel tool batches, and nothing in the -loop ever gates on plan existence or step order — a model that ignores the -tool behaves exactly as if the feature did not exist. Planning is **on by +unchanged and plan calls ride ordinary parallel tool batches. The runtime gates +completion of steps with declared checks; plan existence and step order remain +advisory. Planning is **on by default**; kill switches, in priority order: CLI flag (`--no-planning`), environment (`ODEK_PLANNING=false`), global config (`planning.enabled: false`), project config (opt-out only). @@ -37,6 +37,16 @@ type PlanStep struct { Title string // ≤200 chars, flattened to one render line Status StepStatus Note string // optional, flattened like Title + Checks []PlanCheck // optional, at most 4 evidence checks +} + +type PlanCheck struct { + ID string + Description string + Tool string + Arguments map[string]any // canonical JSON arguments + Status string // "pending" | "passed" | "failed" + CallID string // matching tool-call evidence, when observed } type PlanState struct { @@ -48,8 +58,64 @@ type PlanState struct { A `PlanStore` holds the state behind a dedicated mutex — plan calls can arrive inside a parallel tool batch (`max_tool_parallel` defaults to 4), so every mutation serializes. Caps come from *resolved* config values, never raw project -config. Any status transition is allowed (pending→done included); only -structural validity is enforced. The plan is advisory steering, not a contract. +config. Unchecked steps allow any status transition (pending→done included). +Plans without checks are advisory steering; +steps with checks also enforce their declared evidence before completion. + +### Acceptance checks + +A step may declare up to four optional acceptance checks. Each check contains +an `id`, human-readable `description`, exact `tool` name, and an `arguments` +object. Checks are evidence requirements attached to the step; they are not +commands or an execution queue. The model must invoke the named tool normally, +through the ordinary approval and budget path. The runtime never auto-executes +a check. + +When a tool call completes, the scheduler records evidence only when the tool +name matches exactly and its canonical JSON arguments match exactly. The +record includes the originating call ID and the actual tool outcome. A +successful matching call marks the check `passed`; a matching failed call +marks it `failed`. The latest matching outcome wins. Calls to mutating or +unknown tools that do not match a declared check invalidate the step's check +evidence conservatively across the whole plan, returning checks to `pending`. +A failed matching check also invalidates prior evidence before recording its +failure. Checks act as ordering barriers within a tool batch, and must be +declared in an earlier batch to collect evidence. +A step cannot be completed while any check is pending or failed. The model +must use `plan complete` only after all checks pass; the runtime reports +pending or failed checks in its completion notice and gives the existing +single bounded completion nudge when the run is otherwise ready to finish. + +For example, the model can create a step with a real test command, then run +that command and complete the step only after the matching successful result: + +```json +{"verb":"create","steps":[{"id":"tests","title":"Run the auth regression suite","checks":[{"id":"go-test","description":"Auth package tests pass","tool":"shell","arguments":{"command":"go test ./internal/auth"}}]}]} +``` + +```json +{"command":"go test ./internal/auth"} +``` + +```json +{"verb":"complete","step_id":"tests"} +``` + +Use an execution tool such as `shell` for a test check; a `read_file` call +that merely reads a script is not evidence that the test passed. Check status +records the declared tool outcome, not semantic proof that the description is +true, and there is no independent verifier model yet. + +On resume, persisted check evidence is downgraded to `pending`, and steps +marked done with checks return to `in_progress` until the checks are rerun. +Plans without checks remain compatible and advisory: their existing status +behavior is unchanged. + +Checked declarations reserve space in the protected render. The runtime +rejects a checked plan when its complete render cannot fit +`max_render_chars`, and titles or notes containing the reserved ` || checks:` +delimiter are rejected so the persisted representation remains unambiguous +on resume. ### One store, two holders @@ -197,7 +263,8 @@ multi-step work; update statuses as you go (in_progress when you start a step, done only after verifying it); mark blocked with a note explaining why. The plan is shown to you on every iteration and survives context trimming — trust it over your memory of earlier turns. Replan freely with create when the -approach changes; plans are steering aids, not contracts.", +approach changes; plans without checks are steering aids, while checked steps +also require their declared evidence before completion.", "parameters": { "type": "object", "properties": { @@ -562,8 +629,9 @@ for dashboards. No titles, notes, or loop behavior. ### Non-goals -- **Not waterfall.** No gating anywhere: the loop never blocks on plan - existence, coverage, or step order. +- **Not waterfall.** Unchecked plans never gate on plan existence, coverage, or + step order. Checked steps gate their own completion on declared evidence; + they do not impose a global plan order. - **No DAG/dependency graph.** An ordered flat list suffices; ordering is advisory. - **No Telegram markdown migration.** `/plan` continues to manage operator- @@ -600,7 +668,7 @@ injection mechanism are deliberately droppable by trimming — exactly wrong for state that must survive the whole run. The plan uses the compaction-digest pattern instead: recognized-by-prefix, `headLen`-protected, upsert-in-place. -**Bias to action.** The plan is a steering instrument, never a gate: no -enforcement path exists anywhere in the loop, `create` replaces wholesale so -replanning is one call, and the prompt frames plans as "steering aids, not -contracts." +**Bias to action.** The plan remains a steering instrument for ordinary +steps; acceptance checks add a narrow completion gate without auto-executing +anything. `create` replaces wholesale so replanning is one call, and the +prompt distinguishes advisory steps from checked completion. diff --git a/internal/eval/eval.go b/internal/eval/eval.go new file mode 100644 index 00000000..75ab914d --- /dev/null +++ b/internal/eval/eval.go @@ -0,0 +1,377 @@ +// Package eval provides a deterministic, local-only harness for exercising the +// production loop.Engine. It evaluates runtime behavior and fixture state; it +// does not measure live-model intelligence. +package eval + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "time" + + "github.com/BackendStack21/odek/internal/llmclient" + "github.com/BackendStack21/odek/internal/loop" + "github.com/BackendStack21/odek/internal/tool" +) + +type ToolCall struct { + Name string + Args string + Error bool +} +type Fixture struct { + Values map[string]string + Calls []ToolCall + GoodCalls []ToolCall + mu sync.Mutex +} + +func (f *Fixture) get(k string) string { f.mu.Lock(); defer f.mu.Unlock(); return f.Values[k] } + +// Scenario describes model messages and an independent oracle. Responses are +// wire-level assistant replies, while Oracle is the only source of pass/fail. +type OracleResult struct { + TaskSuccess bool + FalseCompletion bool + Errors []string +} + +type Scenario struct { + Name string + Task string + Responses []string + Tools []tool.Tool + Fixture *Fixture + Oracle func(*Fixture, string, error, []ToolCall) OracleResult + Cancel bool + Plan bool +} +type ToolCallReport struct { + Name string `json:"name"` + Error bool `json:"error"` +} +type CaseReport struct { + Name string `json:"name"` + Success bool `json:"scenario_passed"` + TaskSuccess bool `json:"task_success"` + FalseCompletion bool `json:"false_completion"` + AssertionErrors []string `json:"assertion_errors,omitempty"` + Error string `json:"error,omitempty"` + ToolCalls []ToolCallReport `json:"tool_calls,omitempty"` + InputTokens int64 `json:"input_tokens"` + OutputTokens int64 `json:"output_tokens"` + ElapsedMS int64 `json:"elapsed_ms"` +} +type Report struct { + Cases []CaseReport `json:"cases"` + Total int `json:"total"` + Passed int `json:"passed"` + Failed int `json:"failed"` + FalseCompletionRate float64 `json:"false_completion_rate"` + TokensKnown bool `json:"tokens_known"` + CostKnown bool `json:"cost_known"` +} + +// RunOptions can replace the deterministic client constructor. The scripted +// localhost provider remains the default; a caller may supply a factory for +// an already-authorized provider when comparing adapters. The harness never +// supplies live credentials. The selected client determines the endpoint; +// callers are responsible for authorizing any external provider access. +type RunOptions struct { + ClientFactory func(baseURL string) (*llmclient.Client, error) +} + +// Run executes scenarios against a fresh localhost scripted provider. +func Run(ctx context.Context, scenarios []Scenario) Report { + return RunWithOptions(ctx, scenarios, RunOptions{}) +} + +// RunWithOptions is Run with an optional per-case client factory. +func RunWithOptions(ctx context.Context, scenarios []Scenario, opts RunOptions) Report { + out := Report{Total: len(scenarios)} + for _, s := range scenarios { + out.Cases = append(out.Cases, runCase(ctx, s, opts)) + } + for _, c := range out.Cases { + if c.Success { + out.Passed++ + } else { + out.Failed++ + } + } + // This rate is deliberately narrow: cases whose oracle reports a false + // completion, divided by cases. It is not a model quality score. + falseCount := 0 + for _, c := range out.Cases { + if c.FalseCompletion { + falseCount++ + } + } + if out.Total > 0 { + out.FalseCompletionRate = float64(falseCount) / float64(out.Total) + } + for _, c := range out.Cases { + if c.InputTokens > 0 || c.OutputTokens > 0 { + out.TokensKnown = true + } + // Cost is deliberately unavailable: this harness does not configure prices. + } + return out +} + +func runCase(parent context.Context, s Scenario, opts RunOptions) CaseReport { + started := time.Now() + cr := CaseReport{Name: s.Name} + if s.Oracle == nil { + cr.Error = "scenario requires an independent oracle" + return cr + } + parent, cancel := context.WithTimeout(parent, 10*time.Second) + defer cancel() + var mu sync.Mutex + calls := []ToolCall{} + n := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + i := n + n++ + mu.Unlock() + body := `{"choices":[{"message":{"content":"done"},"finish_reason":"stop"}],"usage":{"prompt_tokens":7,"completion_tokens":3}}` + if i < len(s.Responses) { + body = s.Responses[i] + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + defer srv.Close() + var client *llmclient.Client + var err error + if opts.ClientFactory != nil { + client, err = opts.ClientFactory(srv.URL) + } else { + client, err = scriptedClient(srv.URL) + } + if err == nil && client == nil { + err = fmt.Errorf("client factory returned no client") + } + if err != nil { + cr.Error = err.Error() + cr.ElapsedMS = time.Since(started).Milliseconds() + return cr + } + registered := append([]tool.Tool(nil), s.Tools...) + var planStore *loop.PlanStore + if s.Plan { + planStore = loop.NewPlanStore(12, 4000) + registered = append(registered, loop.NewPlanTool(planStore)) + } + e := loop.New(client, tool.NewRegistry(registered), 12, "", nil, 0) + if planStore != nil { + e.SetPlanStore(planStore) + } + e.SetInteractionMode("off") + callIndex := map[string]int{} + e.SetToolDetailHandler(func(ev loop.ToolDetailEvent) { + mu.Lock() + defer mu.Unlock() + switch ev.Type { + case "tool_call": + callIndex[ev.CallID] = len(calls) + calls = append(calls, ToolCall{Name: ev.Name, Args: ev.Data}) + case "tool_result": + if i, ok := callIndex[ev.CallID]; ok { + calls[i].Error = ev.Outcome == "failed" + } + } + }) + if s.Cancel { + cancelled, cancel := context.WithCancel(parent) + cancel() + parent = cancelled + } + result, runErr := e.Run(parent, s.Task) + mu.Lock() + snapshot := append([]ToolCall(nil), calls...) + mu.Unlock() + for _, c := range snapshot { + cr.ToolCalls = append(cr.ToolCalls, ToolCallReport{Name: c.Name, Error: c.Error}) + } + var outcome OracleResult + if s.Oracle != nil { + outcome = s.Oracle(s.Fixture, result, runErr, snapshot) + } + cr.AssertionErrors = outcome.Errors + cr.TaskSuccess = outcome.TaskSuccess + cr.FalseCompletion = outcome.FalseCompletion + if runErr != nil && len(cr.AssertionErrors) == 0 && !s.Cancel { + cr.Error = runErr.Error() + } + cr.InputTokens = e.BudgetUsage().InputTokens + cr.OutputTokens = e.BudgetUsage().OutputTokens + cr.Success = len(cr.AssertionErrors) == 0 && (runErr == nil || s.Cancel) + cr.ElapsedMS = time.Since(started).Milliseconds() + return cr +} + +func scriptedClient(baseURL string) (*llmclient.Client, error) { + s, err := llmclient.NewSDK(llmclient.Options{Provider: "eval", Model: "eval-model", APIKey: "eval-key", BaseURL: baseURL, Providers: map[string]llmclient.ProviderOverride{"eval": {APIKey: "eval-key", BaseURL: baseURL, Format: "openai"}}}) + if err != nil { + return nil, err + } + return llmclient.New(s, "eval", "eval-model") +} + +// Tool returns a stateful fixture tool. kind is write, read, flaky, or check. +func Tool(f *Fixture, name, kind string) tool.Tool { return fixtureTool{f: f, name: name, kind: kind} } + +type fixtureTool struct { + f *Fixture + name, kind string +} + +func (t fixtureTool) Name() string { return t.name } +func (t fixtureTool) Description() string { return "deterministic evaluation fixture" } +func (t fixtureTool) Schema() any { + return map[string]any{"type": "object", "properties": map[string]any{"key": map[string]string{"type": "string"}, "value": map[string]string{"type": "string"}}} +} +func (t fixtureTool) Call(raw string) (string, error) { + var a struct{ Key, Value string } + _ = json.Unmarshal([]byte(raw), &a) + if a.Key == "" { + a.Key = "artifact" + } + t.f.mu.Lock() + defer t.f.mu.Unlock() + t.f.Calls = append(t.f.Calls, ToolCall{Name: t.name, Args: raw}) + switch t.kind { + case "write": + t.f.Values[a.Key] = a.Value + t.f.GoodCalls = append(t.f.GoodCalls, ToolCall{Name: t.name, Args: raw}) + return "artifact written", nil + case "read": + if v, ok := t.f.Values[a.Key]; ok { + t.f.GoodCalls = append(t.f.GoodCalls, ToolCall{Name: t.name, Args: raw}) + return v, nil + } + return "", fmt.Errorf("missing artifact %q", a.Key) + case "flaky": + if t.f.Values["attempts"] != "1" { + t.f.Values["attempts"] = "1" + return "", fmt.Errorf("transient failure") + } + t.f.Values["attempts"] = "2" + t.f.GoodCalls = append(t.f.GoodCalls, ToolCall{Name: t.name, Args: raw}) + return "recovered", nil + case "check": + if v := t.f.Values[a.Key]; v != "" { + return v, nil + } + return "missing evidence", nil + default: + return "", fmt.Errorf("unknown fixture") + } +} + +func toolCall(name, id, args string) string { + b, _ := json.Marshal(args) + return fmt.Sprintf(`{"choices":[{"message":{"content":"","tool_calls":[{"id":%q,"type":"function","function":{"name":%q,"arguments":%s}}]},"finish_reason":"tool_calls"}],"usage":{"prompt_tokens":7,"completion_tokens":3}}`, id, name, b) +} +func final(text string) string { + b, _ := json.Marshal(text) + return fmt.Sprintf(`{"choices":[{"message":{"content":%s},"finish_reason":"stop"}],"usage":{"prompt_tokens":7,"completion_tokens":3}}`, b) +} + +func baseFixture() *Fixture { return &Fixture{Values: map[string]string{}} } + +// Scenarios is the standard deterministic suite shipped with the CLI. +func Scenarios() []Scenario { + f1 := baseFixture() + f2 := baseFixture() + f3 := baseFixture() + f4 := baseFixture() + f5 := baseFixture() + f6 := baseFixture() + f6.Values["evidence"] = "evidence" + f7 := baseFixture() + f8 := baseFixture() + return []Scenario{ + {Name: "successful_fix_verified", Task: "write and verify artifact", Fixture: f1, Tools: []tool.Tool{Tool(f1, "write_file", "write"), Tool(f1, "read_file", "read")}, Responses: []string{toolCall("write_file", "w1", `{"key":"artifact","value":"fixed"}`), toolCall("read_file", "r1", `{"key":"artifact"}`), final("verified")}, Oracle: func(f *Fixture, r string, e error, _ []ToolCall) OracleResult { + if e != nil { + return OracleResult{Errors: []string{e.Error()}} + } + if f.get("artifact") != "fixed" || !hasGoodCall(f, "read_file") { + return OracleResult{Errors: []string{"artifact was not verified"}} + } + return OracleResult{TaskSuccess: true} + }}, + {Name: "failed_tool_false_success", Task: "read missing artifact", Fixture: f2, Tools: []tool.Tool{Tool(f2, "read_file", "read")}, Responses: []string{toolCall("read_file", "r1", `{"key":"missing"}`), final("success")}, Oracle: func(f *Fixture, r string, e error, c []ToolCall) OracleResult { + if len(c) == 1 && c[0].Name == "read_file" && c[0].Error && e == nil && strings.Contains(r, "success") { + return OracleResult{FalseCompletion: true} + } + return OracleResult{Errors: []string{"expected failed read followed by success claim was not observed"}} + }}, + {Name: "unrelated_read_after_write", Task: "write then inspect unrelated key", Fixture: f3, Tools: []tool.Tool{Tool(f3, "write_file", "write"), Tool(f3, "read_file", "read")}, Responses: []string{toolCall("write_file", "w1", `{"key":"artifact","value":"ok"}`), toolCall("read_file", "r1", `{"key":"other"}`), final("stopped")}, Oracle: func(f *Fixture, r string, e error, c []ToolCall) OracleResult { + if f.get("artifact") != "ok" || !hasGoodCall(f, "write_file") { + return OracleResult{Errors: []string{"write did not persist"}} + } + if len(c) != 2 || c[1].Name != "read_file" || !c[1].Error { + return OracleResult{Errors: []string{"expected unrelated failing read was not observed"}} + } + return OracleResult{TaskSuccess: false} + }}, + {Name: "repeated_failure_recovery", Task: "retry transient operation", Fixture: f4, Tools: []tool.Tool{Tool(f4, "flaky", "flaky")}, Responses: []string{toolCall("flaky", "f1", `{}`), toolCall("flaky", "f2", `{}`), final("recovered")}, Oracle: func(f *Fixture, r string, e error, _ []ToolCall) OracleResult { + if f.get("attempts") != "2" || !hasGoodCall(f, "flaky") { + return OracleResult{Errors: []string{"recovery was not observed"}} + } + return OracleResult{TaskSuccess: true} + }}, + {Name: "explicit_cancellation", Task: "must cancel", Fixture: f5, Tools: nil, Cancel: true, Responses: []string{final("cancelled")}, Oracle: func(_ *Fixture, _ string, e error, _ []ToolCall) OracleResult { + if e == nil { + return OracleResult{Errors: []string{"expected cancellation error"}} + } + return OracleResult{TaskSuccess: false} + }}, + {Name: "plan_check_success", Task: "plan and verify evidence", Fixture: f6, Plan: true, Tools: []tool.Tool{Tool(f6, "read_file", "read")}, Responses: []string{toolCall("plan", "p1", `{"verb":"create","steps":[{"id":"verify","title":"Verify","checks":[{"id":"e1","description":"Read evidence","tool":"read_file","arguments":{"key":"evidence"}}]}]}`), toolCall("read_file", "r1", `{"key":"evidence"}`), toolCall("plan", "p2", `{"verb":"complete","step_id":"verify"}`), final("complete")}, Oracle: func(f *Fixture, r string, e error, c []ToolCall) OracleResult { + if e != nil { + return OracleResult{Errors: []string{e.Error()}} + } + if f.get("evidence") == "" || len(c) != 3 || c[1].Name != "read_file" || c[1].Error || c[2].Name != "plan" || c[2].Error || strings.Contains(r, "[odek verification incomplete:") { + return OracleResult{Errors: []string{"plan success lacked verified evidence, successful read, or successful completion"}} + } + return OracleResult{TaskSuccess: true} + }}, + {Name: "plan_check_failed", Task: "plan and handle failed evidence", Fixture: f7, Plan: true, Tools: []tool.Tool{Tool(f7, "read_file", "read")}, Responses: []string{toolCall("plan", "p1", `{"verb":"create","steps":[{"id":"verify","title":"Verify","checks":[{"id":"e1","description":"Read evidence","tool":"read_file","arguments":{"key":"evidence"}}]}]}`), toolCall("read_file", "r1", `{"key":"evidence"}`), toolCall("plan", "p2", `{"verb":"complete","step_id":"verify"}`), final("complete")}, Oracle: func(_ *Fixture, r string, e error, _ []ToolCall) OracleResult { + if e != nil { + return OracleResult{Errors: []string{e.Error()}} + } + if !strings.Contains(r, "[odek verification incomplete:") { + return OracleResult{Errors: []string{"failed check lacked incomplete marker"}} + } + return OracleResult{TaskSuccess: false} + }}, + {Name: "plan_check_missing", Task: "plan but omit evidence", Fixture: f8, Plan: true, Tools: []tool.Tool{Tool(f8, "read_file", "read")}, Responses: []string{toolCall("plan", "p1", `{"verb":"create","steps":[{"id":"verify","title":"Verify","checks":[{"id":"e1","description":"Read evidence","tool":"read_file","arguments":{"key":"evidence"}}]}]}`), toolCall("plan", "p2", `{"verb":"complete","step_id":"verify"}`), final("complete")}, Oracle: func(_ *Fixture, r string, e error, _ []ToolCall) OracleResult { + if e != nil { + return OracleResult{Errors: []string{e.Error()}} + } + if !strings.Contains(r, "[odek verification incomplete:") { + return OracleResult{Errors: []string{"missing check lacked incomplete marker"}} + } + return OracleResult{TaskSuccess: false} + }}, + } +} + +func hasGoodCall(f *Fixture, name string) bool { + f.mu.Lock() + defer f.mu.Unlock() + for _, c := range f.GoodCalls { + if c.Name == name { + return true + } + } + return false +} diff --git a/internal/eval/eval_test.go b/internal/eval/eval_test.go new file mode 100644 index 00000000..1f75f03a --- /dev/null +++ b/internal/eval/eval_test.go @@ -0,0 +1,51 @@ +package eval + +import ( + "context" + "testing" + + "github.com/BackendStack21/odek/internal/llmclient" +) + +func TestDefaultScenariosAreIndependentAndBounded(t *testing.T) { + r := Run(context.Background(), Scenarios()) + if r.Total != 8 { + t.Fatalf("total=%d want 8", r.Total) + } + if r.Failed != 0 { + t.Fatalf("scenario failures=%d report=%+v", r.Failed, r) + } + if !r.TokensKnown || r.CostKnown { + t.Fatalf("token/cost availability = %v/%v", r.TokensKnown, r.CostKnown) + } + if r.FalseCompletionRate <= 0 { + t.Fatalf("false completion rate=%v", r.FalseCompletionRate) + } + var taskSuccess int + for _, c := range r.Cases { + if c.TaskSuccess { + taskSuccess++ + } + } + if taskSuccess != 3 { + t.Fatalf("task successes=%d want 3", taskSuccess) + } + for _, c := range r.Cases { + if c.Name == "plan_check_failed" || c.Name == "plan_check_missing" { + if c.FalseCompletion { + t.Errorf("%s treated guarded incomplete result as false completion", c.Name) + } + } + } +} + +func TestHarnessRejectsMissingOracleOrClient(t *testing.T) { + missing := Run(context.Background(), []Scenario{{Name: "no oracle"}}) + if missing.Failed != 1 || missing.Cases[0].Error == "" { + t.Fatalf("accepted no oracle: %+v", missing) + } + emptyClient := RunWithOptions(context.Background(), []Scenario{{Name: "no client", Oracle: func(*Fixture, string, error, []ToolCall) OracleResult { return OracleResult{} }}}, RunOptions{ClientFactory: func(string) (*llmclient.Client, error) { return nil, nil }}) + if emptyClient.Failed != 1 || emptyClient.Cases[0].Error == "" { + t.Fatalf("accepted no client: %+v", emptyClient) + } +} diff --git a/internal/loop/completion_checks.go b/internal/loop/completion_checks.go new file mode 100644 index 00000000..814a9fd9 --- /dev/null +++ b/internal/loop/completion_checks.go @@ -0,0 +1,63 @@ +package loop + +import ( + "fmt" + "strconv" + "strings" + + "github.com/BackendStack21/odek/internal/session" +) + +// recordPlanCheckResult consumes engine outcomes, never claims inside tool +// output. Conflicting tool effects execute in transcript order, so a later +// mutation invalidates an earlier check in the same batch. +func (e *Engine) recordPlanCheckResult(epoch uint64, tc session.ToolCall, callID string, failed bool) { + if e.planStore == nil || tc.Function.Name == "plan" { + return + } + if e.planStore.CheckEpoch() == epoch && e.planStore.MatchesCheck(tc.Function.Name, tc.Function.Arguments) { + if failed { + // Failed verification may itself leave partial effects. Earlier + // checks cannot establish the state after that failure. + e.planStore.InvalidateChecks() + } + e.planStore.RecordCheckOutcome(epoch, tc.Function.Name, tc.Function.Arguments, callID, failed) + return + } + fx := e.executionEffects(tc) + if fx.unknown || len(fx.writes) > 0 { + // A failure may leave partial effects. Invalidate conservatively even + // when a command failed or its effects cannot be described precisely. + e.planStore.InvalidateChecks() + } +} + +func (e *Engine) pendingPlanChecks() []string { + if e == nil || e.planStore == nil { + return nil + } + return e.planStore.PendingChecks() +} + +// appendCheckNotice keeps the bounded completion nudge from becoming an +// endless retry loop while making missing evidence explicit in the final +// persisted answer. Only bounded identifiers are included, never commands. +func (e *Engine) appendCheckNotice(answer string) string { + pending := e.pendingPlanChecks() + if len(pending) == 0 { + return answer + } + shown := pending + if len(shown) > 8 { + shown = shown[:8] + } + labels := make([]string, len(shown)) + for i, id := range shown { + labels[i] = strconv.QuoteToASCII(id) + } + notice := fmt.Sprintf("\n\n[odek verification incomplete: %d declared acceptance check(s) have no current passing evidence: %s", len(pending), strings.Join(labels, ", ")) + if len(pending) > len(shown) { + notice += ", …" + } + return answer + notice + ". Task completion is not verified.]" +} diff --git a/internal/loop/completion_checks_test.go b/internal/loop/completion_checks_test.go new file mode 100644 index 00000000..9b2ef52f --- /dev/null +++ b/internal/loop/completion_checks_test.go @@ -0,0 +1,186 @@ +package loop + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/BackendStack21/odek/internal/session" + "github.com/BackendStack21/odek/internal/tool" +) + +const acceptancePlanArgs = `{"verb":"create","steps":[{"id":"fix","title":"Fix parser","checks":[{"id":"test","description":"Parser regression passes","tool":"shell","arguments":{"command":"go test ./parser"}}]}]}` + +func acceptanceCall(id, name, args string) session.ToolCall { + var c session.ToolCall + c.ID, c.Type = id, "function" + c.Function.Name, c.Function.Arguments = name, args + return c +} + +func acceptanceEngine(t *testing.T, batches [][]session.ToolCall, shellFailure bool) (*Engine, *atomic.Int32) { + t.Helper() + var requests atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + i := int(requests.Add(1)) - 1 + msg := map[string]any{"content": "All done."} + finish := "stop" + if i < len(batches) { + msg = map[string]any{"tool_calls": batches[i]} + finish = "tool_calls" + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"choices": []any{map[string]any{"message": msg, "finish_reason": finish}}}) + })) + t.Cleanup(srv.Close) + store := NewPlanStore(12, 4000) + shell := &contractTool{name: "shell", run: func(string) (string, error) { + if shellFailure { + return "tests passed (misleading stdout)", errors.New("exit status 1") + } + return "ok parser", nil + }} + write := &contractTool{name: "write_file", run: func(string) (string, error) { return `{"success":true}`, nil }} + read := &contractTool{name: "read_file", run: func(string) (string, error) { return "contents", nil }} + e := New(testChatClient(t, srv.URL), tool.NewRegistry([]tool.Tool{NewPlanTool(store), shell, write, read}), 12, "sys", nil, 0) + e.SetPlanStore(store) + return e, &requests +} + +func TestAcceptanceChecksBoundFinalClaims(t *testing.T) { + create := acceptanceCall("plan", "plan", acceptancePlanArgs) + check := acceptanceCall("check", "shell", `{"command":"go test ./parser"}`) + complete := acceptanceCall("complete", "plan", `{"verb":"complete","step_id":"fix"}`) + write := acceptanceCall("write", "write_file", `{"path":"parser.go","content":"changed"}`) + read := acceptanceCall("read", "read_file", `{"path":"parser.go"}`) + for _, tc := range []struct { + name string + batches [][]session.ToolCall + failed, unverified bool + }{ + {"success", [][]session.ToolCall{{create}, {check}, {complete}}, false, false}, + {"missing", [][]session.ToolCall{{create}, {complete}}, false, true}, + {"failed", [][]session.ToolCall{{create}, {check}, {complete}}, true, true}, + {"read_is_not_declared_check", [][]session.ToolCall{{create}, {write}, {read}, {complete}}, false, true}, + {"same_batch_declaration_not_evidence", [][]session.ToolCall{{check, create}, {complete}}, false, true}, + {"write_after_check_invalidates", [][]session.ToolCall{{create}, {check, write}, {complete}}, false, true}, + {"check_after_write", [][]session.ToolCall{{create}, {write, check}, {complete}}, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + e, requests := acceptanceEngine(t, tc.batches, tc.failed) + answer, messages, err := e.RunWithMessages(context.Background(), []session.Message{{Role: "user", Content: "Fix parser and verify."}}) + if err != nil { + t.Fatal(err) + } + got := strings.Contains(answer, "[odek verification incomplete:") + if got != tc.unverified { + t.Fatalf("unverified=%v want %v; answer=%s", got, tc.unverified, answer) + } + if requests.Load() > int32(len(tc.batches)+2) { + t.Fatalf("completion retry was not bounded: %d calls", requests.Load()) + } + if len(messages) == 0 || messages[len(messages)-1].Content != answer { + t.Fatal("final verification notice not persisted") + } + st, _ := e.planStore.Snapshot() + if tc.unverified && st.Steps[0].Status == StepDone { + t.Fatal("unverified step marked done") + } + }) + } +} + +func TestAcceptanceCheckResumeRequiresFreshEvidence(t *testing.T) { + e, _ := acceptanceEngine(t, [][]session.ToolCall{ + {acceptanceCall("plan", "plan", acceptancePlanArgs)}, + {acceptanceCall("check", "shell", `{"command":"go test ./parser"}`)}, + {acceptanceCall("complete", "plan", `{"verb":"complete","step_id":"fix"}`)}, + }, false) + _, history, err := e.RunWithMessages(context.Background(), []session.Message{{Role: "user", Content: "Fix parser."}}) + if err != nil { + t.Fatal(err) + } + history = append(history, session.Message{Role: "user", Content: "Recheck current state."}) + resumed, _ := acceptanceEngine(t, nil, false) + answer, _, err := resumed.RunWithMessages(context.Background(), history) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(answer, "[odek verification incomplete:") { + t.Fatalf("resumed run reused stale evidence: %s", answer) + } + state, _ := resumed.planStore.Snapshot() + if state.Steps[0].Status != StepInProgress || state.Steps[0].Checks[0].Status != PlanCheckPending { + t.Fatalf("stale restored status: %+v", state) + } +} + +func TestAcceptanceReadCheckWaitsForPriorMutation(t *testing.T) { + create := acceptanceCall("plan", "plan", `{"verb":"create","steps":[{"id":"fix","title":"Verify generated output","checks":[{"id":"inspect","description":"Inspect current output","tool":"read_file","arguments":{"path":"output.go"}}]}]}`) + e, _ := acceptanceEngine(t, [][]session.ToolCall{{create}, { + acceptanceCall("write", "write_file", `{"path":"settings.json"}`), + acceptanceCall("read", "read_file", `{"path":"output.go"}`), + }, {acceptanceCall("complete", "plan", `{"verb":"complete","step_id":"fix"}`)}}, false) + started, release, read := make(chan struct{}), make(chan struct{}), make(chan struct{}, 1) + var written atomic.Bool + e.registry = tool.NewRegistry([]tool.Tool{NewPlanTool(e.planStore), + &contractTool{name: "write_file", run: func(string) (string, error) { + close(started) + <-release + written.Store(true) + return `{"success":true}`, nil + }}, + &contractTool{name: "read_file", run: func(string) (string, error) { + read <- struct{}{} + if !written.Load() { + return "", errors.New("stale output") + } + return "current output", nil + }}, + }) + done := make(chan error, 1) + go func() { + _, err := e.Run(context.Background(), "Update settings and inspect generated output.") + done <- err + }() + <-started + select { + case <-read: + close(release) + <-done + t.Fatal("acceptance check ran before prior mutation completed") + case <-time.After(30 * time.Millisecond): + } + close(release) + if err := <-done; err != nil { + t.Fatal(err) + } + if pending := e.pendingPlanChecks(); len(pending) != 0 { + t.Fatalf("successful ordered check left pending evidence: %v", pending) + } +} + +func TestAcceptanceFailedCheckInvalidatesEarlierEvidence(t *testing.T) { + store := NewPlanStore(12, 4000) + _, err := store.Execute(`{"verb":"create","steps":[{"id":"s","title":"Verify changes","checks":[{"id":"a","description":"First check","tool":"shell","arguments":{"command":"test-a"}},{"id":"b","description":"Second check","tool":"shell","arguments":{"command":"test-b"}}]}]}`) + if err != nil { + t.Fatal(err) + } + e := &Engine{planStore: store} + epoch := store.CheckEpoch() + e.recordPlanCheckResult(epoch, acceptanceCall("a", "shell", `{"command":"test-a"}`), "a", false) + e.recordPlanCheckResult(epoch, acceptanceCall("b", "shell", `{"command":"test-b"}`), "b", true) + e.recordPlanCheckResult(epoch, acceptanceCall("b2", "shell", `{"command":"test-b"}`), "b2", false) + if pending := store.PendingChecks(); len(pending) != 1 || pending[0] != "s/a" { + t.Fatalf("failure retained stale prior evidence: %v", pending) + } + if _, err := store.Execute(`{"verb":"complete","step_id":"s"}`); err == nil { + t.Fatal("completed with stale first check") + } +} diff --git a/internal/loop/loop.go b/internal/loop/loop.go index 724a842f..c24301e9 100644 --- a/internal/loop/loop.go +++ b/internal/loop/loop.go @@ -2034,8 +2034,10 @@ func (e *Engine) syncPlanFromMessages(messages []session.Message) []session.Mess e.planStore.Restore(newest) // The persisted render already reflects this version; seed the cache // so the next refresh no-ops until the state changes. - e.planRenderedVersion = newest.Version - e.planRenderedContent = newestContent + if restored, ok := e.planStore.Snapshot(); ok && restored.Version == newest.Version { + e.planRenderedVersion = newest.Version + e.planRenderedContent = newestContent + } } return out } @@ -2571,6 +2573,7 @@ func (e *Engine) runLoop(ctx context.Context, in []session.Message) (answer stri // (docs/PLANNING.md — Restart Resume). Unparseable plan messages are // removed from the history. No-op when planning is disabled. messages = e.syncPlanFromMessages(messages) + messages = e.refreshPlanMessage(ctx, messages) // Trim statistics and rolling-digest state are per-conversation, not // per-engine: reset the counters and re-derive the digest from THIS @@ -2912,7 +2915,7 @@ func (e *Engine) runLoop(ctx context.Context, in []session.Message) (answer stri // (the tool-call path's dangling-call protection) throws // away the only useful artifact. Persist the answer, still // return the typed budget error. - result.Content = e.reconcileFinalReply(result.Content) + result.Content = e.appendCheckNotice(e.reconcileFinalReply(result.Content)) messages = append(messages, session.Message{ Role: "assistant", Content: result.Content, @@ -2964,7 +2967,7 @@ func (e *Engine) runLoop(ctx context.Context, in []session.Message) (answer stri // reconcile the reply against the action ledger before it // goes out. A reply that misreports side effects ("blocked", // "no changes made") after they happened is worse than silence. - result.Content = e.reconcileFinalReply(result.Content) + result.Content = e.appendCheckNotice(e.reconcileFinalReply(result.Content)) if e.renderer != nil && e.interactionMode != "off" { // Show the model's reasoning for the final answer before the @@ -3268,6 +3271,13 @@ func (e *Engine) runLoop(ctx context.Context, in []session.Message) (answer stri e.externalChargeMu.Unlock() } + // Checks must be declared before this batch. A plan created or replaced + // within the batch cannot claim earlier actions as verification. + var checkEpoch uint64 + if e.planStore != nil { + checkEpoch = e.planStore.CheckEpoch() + } + // Phase 2: execute tools in parallel (bounded by semaphore) type execResult struct { output string @@ -3294,6 +3304,12 @@ func (e *Engine) runLoop(ctx context.Context, in []session.Message) (answer stri for i, tc := range result.ToolCalls { done[i] = make(chan struct{}) effects[i] = e.executionEffects(tc) + if e.planStore != nil && e.planStore.MatchesCheck(tc.Function.Name, tc.Function.Arguments) { + // Acceptance checks are ordering barriers: even a read-only + // check may validate a condition affected by another resource. + // Its evidence must reflect prior mutations in this batch. + effects[i].unknown = true + } } var workers sync.WaitGroup @@ -3449,6 +3465,7 @@ func (e *Engine) runLoop(ctx context.Context, in []session.Message) (answer stri for i, tc := range result.ToolCalls { output := results[i].output fullOutput := output + e.recordPlanCheckResult(checkEpoch, tc, callIDs[i], results[i].errored) // ledger the mutating calls that completed this run so the // final reply can be reconciled against what actually happened. @@ -3814,8 +3831,8 @@ func (e *Engine) runLoop(ctx context.Context, in []session.Message) (answer stri } e.lastPartialReason = reason if summary := progressSummary; summary != "" { - final := marker + "\n\n" + summary - persistedFinal := marker + "\n\n" + e.protectDerivedContext(ctx, "progress_summary", summary) + final := e.appendCheckNotice(marker + "\n\n" + summary) + persistedFinal := e.appendCheckNotice(marker + "\n\n" + e.protectDerivedContext(ctx, "progress_summary", summary)) if e.renderer != nil && e.interactionMode != "off" { // This summary comes from a buffered side call — nothing was @@ -4263,7 +4280,7 @@ func (e *Engine) needsCompletionNudge() bool { } open := e.openPlanStepCount() uncaught := len(e.runMutations) > 0 && !e.sawReadAfterMutation - return open > 0 || uncaught + return open > 0 || uncaught || len(e.pendingPlanChecks()) > 0 } func (e *Engine) completionNudgeText() string { @@ -4280,5 +4297,8 @@ func (e *Engine) completionNudgeText() string { b.WriteString("Uncaught mutations remain. ") } b.WriteString("Either call the check, update the plan, or tell the principal what remains. Do not claim done.") + if pending := e.pendingPlanChecks(); len(pending) > 0 { + b.WriteString(" Declared acceptance checks remain unverified. Run their declared tools through the normal approval path, then complete the step; if blocked, report the missing verification. A plan update cannot self-certify a check.") + } return b.String() } diff --git a/internal/loop/plan.go b/internal/loop/plan.go index d12f10d5..3e53a9d7 100644 --- a/internal/loop/plan.go +++ b/internal/loop/plan.go @@ -12,7 +12,7 @@ package loop // // Validation is fail-closed: any malformed input rejects the whole call with // a typed error and leaves the state untouched. Any status transition is -// allowed (the plan is advisory); only structural validity is enforced. +// allowed for unchecked steps; checked steps require successful tool outcomes. import ( "encoding/json" @@ -50,10 +50,28 @@ func validStepStatus(s StepStatus) bool { // (e.g. "s1"); they exist so updates can target steps without positional // ambiguity when the list is reordered. type PlanStep struct { - ID string `json:"id"` - Title string `json:"title"` - Status StepStatus `json:"status"` - Note string `json:"note,omitempty"` + ID string `json:"id"` + Title string `json:"title"` + Status StepStatus `json:"status"` + Note string `json:"note,omitempty"` + Checks []PlanCheck `json:"checks,omitempty"` +} + +type PlanCheckStatus string + +const ( + PlanCheckPending PlanCheckStatus = "pending" + PlanCheckPassed PlanCheckStatus = "passed" + PlanCheckFailed PlanCheckStatus = "failed" +) + +type PlanCheck struct { + ID string `json:"id"` + Description string `json:"description"` + Tool string `json:"tool"` + Arguments map[string]any `json:"arguments"` + Status PlanCheckStatus `json:"status"` + CallID string `json:"call_id,omitempty"` } // PlanState is the authoritative plan. Version bumps on every mutation and @@ -110,6 +128,7 @@ type PlanStore struct { blockedStreak int // consecutive blocked status transitions lastBlocked bool // last status transition was to blocked blockedFired bool // this mutation tripped the streak (consumed by notify) + epoch uint64 } // NewPlanStore creates a store with the given resolved caps. Degenerate @@ -149,7 +168,26 @@ func (s *PlanStore) Restore(st PlanState) { s.mu.Lock() defer s.mu.Unlock() cp := clonePlanState(st) + restoredChecks := false + for i := range cp.Steps { + checked := len(cp.Steps[i].Checks) > 0 + for j := range cp.Steps[i].Checks { + if cp.Steps[i].Checks[j].Status != PlanCheckPending || cp.Steps[i].Status == StepDone { + restoredChecks = true + } + cp.Steps[i].Checks[j].Status = PlanCheckPending + cp.Steps[i].Checks[j].CallID = "" + } + if checked && cp.Steps[i].Status == StepDone { + cp.Steps[i].Status = StepInProgress + restoredChecks = true + } + } + if restoredChecks { + cp.Version++ + } s.plan = &cp + s.epoch++ s.blockedStreak = 0 s.lastBlocked = false s.blockedFired = false @@ -157,6 +195,9 @@ func (s *PlanStore) Restore(st PlanState) { func clonePlanState(st PlanState) PlanState { st.Steps = append([]PlanStep(nil), st.Steps...) + for i := range st.Steps { + st.Steps[i].Checks = clonePlanChecks(st.Steps[i].Checks) + } return st } @@ -165,6 +206,7 @@ func (s *PlanStore) Reset() { s.mu.Lock() defer s.mu.Unlock() s.plan = nil + s.epoch++ s.blockedStreak = 0 s.lastBlocked = false s.blockedFired = false @@ -190,9 +232,17 @@ func (s *PlanStore) SetOnChange(fn func(PlanChange)) { // ── Tool-call envelope ──────────────────────────────────────────────── type planStepArg struct { - ID string `json:"id"` - Title string `json:"title"` - Note string `json:"note"` + ID string `json:"id"` + Title string `json:"title"` + Note string `json:"note"` + Checks []planCheckArg `json:"checks"` +} + +type planCheckArg struct { + ID string `json:"id"` + Description string `json:"description"` + Tool string `json:"tool"` + Arguments json.RawMessage `json:"arguments"` } type planUpdateArg struct { @@ -315,12 +365,21 @@ func (s *PlanStore) create(steps []planStepArg) (string, error) { if len(title) > maxPlanTitleChars { return "", fmt.Errorf("plan: step[%d]: title is too long (%d > %d chars)", i, len(title), maxPlanTitleChars) } - out = append(out, PlanStep{ID: id, Title: title, Status: StepPending, Note: normalizePlanText(in.Note)}) + checks, err := validatePlanChecks(in.Checks) + if err != nil { + return "", fmt.Errorf("plan: step[%d]: %w", i, err) + } + out = append(out, PlanStep{ID: id, Title: title, Status: StepPending, Note: normalizePlanText(in.Note), Checks: checks}) } + candidate := PlanState{Version: s.nextVersion(), Steps: out} + if !checkedPlanFits(candidate, s.maxRenderChars) { + return "", fmt.Errorf("plan: checked plan exceeds max_render_chars (%d) or uses reserved checks delimiter in title/note", s.maxRenderChars) + } + s.epoch++ s.blockedStreak = 0 s.lastBlocked = false s.blockedFired = false - s.plan = &PlanState{Version: s.nextVersion(), Steps: out} + s.plan = &candidate return s.renderLocked(), nil } @@ -332,7 +391,7 @@ func (s *PlanStore) update(updates []planUpdateArg) (string, error) { // call and leaves the stored plan untouched (atomic batch). var working []PlanStep if s.plan != nil { - working = append(working, s.plan.Steps...) + working = clonePlanSteps(s.plan.Steps) } changed := false for i, u := range updates { @@ -346,6 +405,9 @@ func (s *PlanStore) update(updates []planUpdateArg) (string, error) { return "", fmt.Errorf("plan: update: step[%d]: unknown status %q", i, u.Status) } if working[idx].Status != st { + if st == StepDone && !allPlanChecksPassed(working[idx]) { + return "", fmt.Errorf("plan: update: step %q has checks that have not passed", working[idx].ID) + } working[idx].Status = st changed = true } @@ -366,8 +428,12 @@ func (s *PlanStore) update(updates []planUpdateArg) (string, error) { if s.plan != nil { old = s.plan.Steps } + candidate := PlanState{Version: s.nextVersion(), Steps: working} + if !checkedPlanFits(candidate, s.maxRenderChars) { + return "", fmt.Errorf("plan: checked plan exceeds max_render_chars (%d) or uses reserved checks delimiter in title/note", s.maxRenderChars) + } s.noteStatusTransitionsLocked(old, working) - s.plan = &PlanState{Version: s.nextVersion(), Steps: working} + s.plan = &candidate return s.renderLocked(), nil } @@ -383,10 +449,17 @@ func (s *PlanStore) complete(stepID string) (string, error) { if s.plan.Steps[idx].Status == StepDone { return s.renderLocked(), nil // idempotent no-op } - working := append([]PlanStep(nil), s.plan.Steps...) + working := clonePlanSteps(s.plan.Steps) + if !allPlanChecksPassed(working[idx]) { + return "", fmt.Errorf("plan: complete: step %q has checks that have not passed", working[idx].ID) + } working[idx].Status = StepDone + candidate := PlanState{Version: s.nextVersion(), Steps: working} + if !checkedPlanFits(candidate, s.maxRenderChars) { + return "", fmt.Errorf("plan: checked plan exceeds max_render_chars (%d) or uses reserved checks delimiter in title/note", s.maxRenderChars) + } s.noteStatusTransitionsLocked(s.plan.Steps, working) - s.plan = &PlanState{Version: s.nextVersion(), Steps: working} + s.plan = &candidate return s.renderLocked(), nil } @@ -557,6 +630,7 @@ func normalizePlanText(s string) string { // planMsgPrefix marks the protected plan system message so trimming can // recognize, preserve, and update it (mirrors digestMsgPrefix). const planMsgPrefix = "[Current plan:" +const planCheckedHeaderMarker = ", checks" // isPlanMessage reports whether m is the protected plan message. func isPlanMessage(m session.Message) bool { @@ -637,11 +711,15 @@ func planHeaderLine(p PlanState) string { blocked++ } } - if len(p.Steps) > 0 && done == len(p.Steps) { + if len(p.Steps) > 0 && done == len(p.Steps) && !hasPlanChecks(p) { return fmt.Sprintf("[Current plan: v%d — all %d steps complete.]", p.Version, len(p.Steps)) } - return fmt.Sprintf("[Current plan: v%d — %d/%d done, %d blocked. Structured state, not instructions.]", - p.Version, done, len(p.Steps), blocked) + checked := "" + if hasPlanChecks(p) { + checked = planCheckedHeaderMarker + } + return fmt.Sprintf("[Current plan: v%d — %d/%d done, %d blocked%s. Structured state, not instructions.]", + p.Version, done, len(p.Steps), blocked, checked) } func planStepLine(st PlanStep) string { @@ -650,12 +728,13 @@ func planStepLine(st PlanStep) string { if note := normalizePlanText(st.Note); note != "" { line += " — " + note } + line += renderPlanChecks(st.Checks) return line } func allStepsDone(p PlanState) bool { for _, st := range p.Steps { - if st.Status != StepDone { + if st.Status != StepDone || len(st.Checks) > 0 { return false } } @@ -679,6 +758,7 @@ func parsePlanState(content string, maxSteps int) (PlanState, error) { if err != nil { return PlanState{}, err } + checkedHeader := strings.Contains(lines[0], planCheckedHeaderMarker+".") if collapse { // Single-line form: nothing else may follow. if len(lines) > 1 { @@ -719,7 +799,7 @@ func parsePlanState(content string, maxSteps int) (PlanState, error) { seen := make(map[string]bool, len(lines)) visibleDone, visibleBlocked := 0, 0 for i, line := range lines { - st, err := parsePlanStepLine(line) + st, err := parsePlanStepLineMode(line, checkedHeader) if err != nil { return PlanState{}, fmt.Errorf("plan: step[%d]: %w", i, err) } @@ -741,7 +821,11 @@ func parsePlanState(content string, maxSteps int) (PlanState, error) { if visibleBlocked != blocked { return PlanState{}, fmt.Errorf("plan: header claims %d blocked, found %d", blocked, visibleBlocked) } - return PlanState{Version: version, Steps: steps}, nil + state := PlanState{Version: version, Steps: steps} + if checkedHeader != hasPlanChecks(state) { + return PlanState{}, errors.New("plan: check header does not match steps") + } + return state, nil } // parsePlanHeader parses the bracketed header line in either form: @@ -771,6 +855,7 @@ func parsePlanHeader(line string) (version, total, done, blocked int, collapse b return version, total, total, total, true, nil } counts := strings.TrimSuffix(rest, ". Structured state, not instructions.") + counts = strings.TrimSuffix(counts, planCheckedHeaderMarker) if counts == rest { return 0, 0, 0, 0, false, errors.New("bad plan header") } @@ -865,6 +950,21 @@ func unwrapPlanBody(lines []string) ([]string, error) { // parsePlanStepLine parses one `id [status] title — note` line. func parsePlanStepLine(line string) (PlanStep, error) { + return parsePlanStepLineMode(line, true) +} + +func parsePlanStepLineMode(line string, allowChecks bool) (PlanStep, error) { + checks := []PlanCheck(nil) + if allowChecks { + if idx := strings.Index(line, planCheckRenderMarker); idx >= 0 { + var err error + checks, err = parsePlanChecks(line[idx+len(planCheckRenderMarker):]) + if err != nil { + return PlanStep{}, fmt.Errorf("invalid checks: %w", err) + } + line = line[:idx] + } + } sep := strings.Index(line, " [") if sep <= 0 { return PlanStep{}, errors.New("malformed step line") @@ -895,7 +995,7 @@ func parsePlanStepLine(line string) (PlanStep, error) { if title == "" { return PlanStep{}, errors.New("missing title") } - return PlanStep{ID: id, Title: title, Status: status, Note: note}, nil + return PlanStep{ID: id, Title: title, Status: status, Note: note, Checks: checks}, nil } // parsePlanNumber parses a non-negative integer of digits only — signs, @@ -974,8 +1074,10 @@ func (t *PlanTool) Description() string { "update statuses as you go (in_progress when you start a step, done only after " + "verifying it); mark blocked with a note explaining why. The plan is shown to you " + "on every iteration and survives context trimming — trust it over your memory of " + - "earlier turns. Replan freely with create when the approach changes; plans are " + - "steering aids, not contracts." + "earlier turns. Replan freely with create when the approach changes. For verifiable " + + "work, declare optional checks with exact tool arguments before running them in a later " + + "batch. Complete checked steps only after their tools succeed; do not self-certify. " + + "Plans without checks remain advisory." } func (t *PlanTool) Schema() any { @@ -994,6 +1096,14 @@ func (t *PlanTool) Schema() any { "id": map[string]any{"type": "string"}, "title": map[string]any{"type": "string"}, "note": map[string]any{"type": "string"}, + "checks": map[string]any{ + "type": "array", "maxItems": 4, + "description": "Optional declarative verification checks. Run the named tool separately; only its actual result can pass a check.", + "items": map[string]any{"type": "object", "properties": map[string]any{ + "id": map[string]any{"type": "string"}, "description": map[string]any{"type": "string"}, + "tool": map[string]any{"type": "string"}, "arguments": map[string]any{"type": "object"}, + }, "required": []string{"id", "description", "tool", "arguments"}}, + }, }, "required": []string{"id", "title"}, }, diff --git a/internal/loop/plan_checks.go b/internal/loop/plan_checks.go new file mode 100644 index 00000000..e9a1abd8 --- /dev/null +++ b/internal/loop/plan_checks.go @@ -0,0 +1,366 @@ +package loop + +import ( + "bytes" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "io" + "sort" + "strings" + "unicode" +) + +const ( + maxPlanChecks = 4 + maxPlanCheckIDChars = 32 + maxPlanCheckDescChars = 200 + maxPlanCheckToolChars = 128 + maxPlanCheckArgsBytes = 4096 + planCheckRenderMarker = " || checks:" + maxPlanCheckCallIDChars = 128 +) + +func clonePlanChecks(in []PlanCheck) []PlanCheck { + if in == nil { + return nil + } + out := make([]PlanCheck, len(in)) + for i, check := range in { + out[i] = check + out[i].Arguments = clonePlanArguments(check.Arguments) + } + return out +} + +func clonePlanSteps(in []PlanStep) []PlanStep { + out := append([]PlanStep(nil), in...) + for i := range out { + out[i].Checks = clonePlanChecks(out[i].Checks) + } + return out +} + +func clonePlanArguments(in map[string]any) map[string]any { + if in == nil { + return nil + } + raw, err := json.Marshal(in) + if err != nil { + return nil + } + out, err := decodeJSONObject(raw) + if err != nil { + return nil + } + return out +} + +func validatePlanChecks(in []planCheckArg) ([]PlanCheck, error) { + if len(in) == 0 { + return nil, nil + } + if len(in) > maxPlanChecks { + return nil, fmt.Errorf("too many checks (%d > %d)", len(in), maxPlanChecks) + } + out := make([]PlanCheck, 0, len(in)) + seen := make(map[string]bool, len(in)) + for i, raw := range in { + id := strings.TrimSpace(raw.ID) + if id == "" || len([]rune(id)) > maxPlanCheckIDChars || strings.ContainsAny(id, "[]/") || strings.ContainsFunc(id, func(r rune) bool { return unicode.IsSpace(r) || unicode.IsControl(r) }) { + return nil, fmt.Errorf("check[%d]: invalid id", i) + } + if seen[id] { + return nil, fmt.Errorf("check[%d]: duplicate id %q", i, id) + } + seen[id] = true + description := normalizePlanText(raw.Description) + if description == "" || len([]rune(description)) > maxPlanCheckDescChars { + return nil, fmt.Errorf("check[%d]: invalid description", i) + } + tool := strings.TrimSpace(raw.Tool) + if tool == "" || tool == "plan" || len([]rune(tool)) > maxPlanCheckToolChars { + return nil, fmt.Errorf("check[%d]: invalid tool", i) + } + for _, r := range tool { + if unicode.IsControl(r) { + return nil, fmt.Errorf("check[%d]: invalid tool", i) + } + } + args, err := decodeJSONObject(raw.Arguments) + if err != nil { + return nil, fmt.Errorf("check[%d]: arguments must be a JSON object: %w", i, err) + } + canonical, err := json.Marshal(args) + if err != nil || len(canonical) > maxPlanCheckArgsBytes { + return nil, fmt.Errorf("check[%d]: arguments too large", i) + } + out = append(out, PlanCheck{ID: id, Description: description, Tool: tool, Arguments: args, Status: PlanCheckPending}) + } + return out, nil +} + +func allPlanChecksPassed(step PlanStep) bool { + for _, check := range step.Checks { + if check.Status != PlanCheckPassed { + return false + } + } + return true +} + +func hasPlanChecks(p PlanState) bool { + for _, step := range p.Steps { + if len(step.Checks) > 0 { + return true + } + } + return false +} + +func checkedPlanFits(p PlanState, maxChars int) bool { + if !hasPlanChecks(p) { + return true + } + reserve := clonePlanState(p) + // Reserve the longest statuses and bounded evidence references without + // invoking the renderer's lossy overflow fallback for legacy plans. + reserve.Version = 999999999 + for i := range reserve.Steps { + step := &reserve.Steps[i] + if strings.Contains(step.Title, planCheckRenderMarker) || strings.Contains(step.Note, planCheckRenderMarker) { + return false + } + step.Status = StepInProgress + for j := range step.Checks { + step.Checks[j].Status = PlanCheckPending + step.Checks[j].CallID = strings.Repeat("x", maxPlanCheckCallIDChars) + } + } + // Both header counts may grow to the width of the total step count. + size := len(planHeaderLine(reserve)) + 2*len(fmt.Sprint(len(reserve.Steps))) + for _, step := range reserve.Steps { + size += 1 + len(planStepLine(step)) + } + return size <= maxChars +} + +func canonicalPlanArguments(raw []byte) ([]byte, error) { + obj, err := decodeJSONObject(raw) + if err != nil { + return nil, err + } + return json.Marshal(obj) +} + +func decodeJSONObject(raw []byte) (map[string]any, error) { + if len(bytes.TrimSpace(raw)) == 0 { + return nil, errors.New("empty arguments") + } + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.UseNumber() + var value any + if err := dec.Decode(&value); err != nil { + return nil, err + } + var extra any + if err := dec.Decode(&extra); err != io.EOF { + if err == nil { + return nil, errors.New("trailing JSON") + } + return nil, err + } + obj, ok := value.(map[string]any) + if !ok || obj == nil { + return nil, errors.New("must be an object") + } + return obj, nil +} + +func renderPlanChecks(checks []PlanCheck) string { + if len(checks) == 0 { + return "" + } + b, _ := json.Marshal(checks) + return planCheckRenderMarker + string(b) +} + +func parsePlanChecks(raw string) ([]PlanCheck, error) { + var in []struct { + ID string `json:"id"` + Description string `json:"description"` + Tool string `json:"tool"` + Arguments json.RawMessage `json:"arguments"` + Status PlanCheckStatus `json:"status"` + CallID string `json:"call_id"` + } + if err := json.Unmarshal([]byte(raw), &in); err != nil { + return nil, err + } + args := make([]planCheckArg, len(in)) + for i, check := range in { + if check.Status != "" && check.Status != PlanCheckPending && check.Status != PlanCheckPassed && check.Status != PlanCheckFailed { + return nil, fmt.Errorf("check[%d]: unknown status", i) + } + if len([]rune(check.CallID)) > maxPlanCheckCallIDChars { + return nil, fmt.Errorf("check[%d]: call id too long", i) + } + for _, r := range check.CallID { + if unicode.IsControl(r) { + return nil, fmt.Errorf("check[%d]: invalid call id", i) + } + } + args[i] = planCheckArg{ID: check.ID, Description: check.Description, Tool: check.Tool, Arguments: check.Arguments} + } + out, err := validatePlanChecks(args) + if err != nil { + return nil, err + } + for i := range out { + out[i].Status = in[i].Status + if out[i].Status == "" { + out[i].Status = PlanCheckPending + } + out[i].CallID = in[i].CallID + } + return out, nil +} + +func (s *PlanStore) CheckEpoch() uint64 { + s.mu.Lock() + defer s.mu.Unlock() + return s.epoch +} + +func (s *PlanStore) MatchesCheck(tool, args string) bool { + canonical, err := canonicalPlanArguments([]byte(args)) + if err != nil { + return false + } + s.mu.Lock() + defer s.mu.Unlock() + for _, step := range s.planStepsLocked() { + for _, check := range step.Checks { + if check.Tool == tool && samePlanArguments(check.Arguments, canonical) { + return true + } + } + } + return false +} + +func samePlanArguments(args map[string]any, canonical []byte) bool { + b, err := json.Marshal(args) + return err == nil && bytes.Equal(b, canonical) +} + +func (s *PlanStore) RecordCheckOutcome(epoch uint64, tool, args, callID string, failed bool) { + canonical, err := canonicalPlanArguments([]byte(args)) + if err != nil { + return + } + s.mu.Lock() + defer s.mu.Unlock() + if epoch != s.epoch || s.plan == nil { + return + } + callID = truncatePlanCallID(callID) + status := PlanCheckPassed + if failed { + status = PlanCheckFailed + } + changed := false + for i := range s.plan.Steps { + for j := range s.plan.Steps[i].Checks { + check := &s.plan.Steps[i].Checks[j] + if check.Tool == tool && samePlanArguments(check.Arguments, canonical) { + if check.Status != status || check.CallID != callID { + check.Status, check.CallID = status, callID + changed = true + } + if failed && s.plan.Steps[i].Status == StepDone { + s.plan.Steps[i].Status = StepInProgress + changed = true + } + } + } + } + if changed { + s.plan.Version++ + s.notifyLocked(false, false) + } +} + +func truncatePlanCallID(callID string) string { + if callID != "" { + valid := len(callID) <= maxPlanCheckCallIDChars + for _, r := range callID { + switch { + case r == '-', r == '_', r == '.', r == ':', r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + default: + valid = false + } + } + if valid { + return callID + } + } + digest := sha256.Sum256([]byte(callID)) + return "sha256:" + hex.EncodeToString(digest[:]) +} + +func (s *PlanStore) InvalidateChecks() { + s.mu.Lock() + defer s.mu.Unlock() + if s.plan == nil { + return + } + changed := false + working := clonePlanSteps(s.plan.Steps) + for i := range working { + if len(working[i].Checks) == 0 { + continue + } + if working[i].Status == StepDone { + working[i].Status = StepInProgress + changed = true + } + for j := range working[i].Checks { + if working[i].Checks[j].Status != PlanCheckPending || working[i].Checks[j].CallID != "" { + changed = true + } + working[i].Checks[j].Status = PlanCheckPending + working[i].Checks[j].CallID = "" + } + } + if !changed { + return + } + s.noteStatusTransitionsLocked(s.plan.Steps, working) + s.plan = &PlanState{Version: s.nextVersion(), Steps: working} + s.notifyLocked(false, false) +} + +func (s *PlanStore) PendingChecks() []string { + s.mu.Lock() + defer s.mu.Unlock() + var out []string + for _, step := range s.planStepsLocked() { + for _, check := range step.Checks { + if check.Status != PlanCheckPassed { + out = append(out, step.ID+"/"+check.ID) + } + } + } + sort.Strings(out) + return out +} + +func (s *PlanStore) planStepsLocked() []PlanStep { + if s.plan == nil { + return nil + } + return s.plan.Steps +} diff --git a/internal/loop/plan_checks_test.go b/internal/loop/plan_checks_test.go new file mode 100644 index 00000000..1236cf6e --- /dev/null +++ b/internal/loop/plan_checks_test.go @@ -0,0 +1,194 @@ +package loop + +import ( + "encoding/json" + "reflect" + "strings" + "testing" +) + +func checkedCreateArgs() string { + return `{"verb":"create","steps":[{"id":"s1","title":"verify","checks":[{"id":"c1","description":"Read the output","tool":"read_file","arguments":{"path":"out","line":1}}]}]}` +} + +func TestPlanChecks_RenderParseAndRequireOutcomes(t *testing.T) { + s := NewPlanStore(4, 4000) + if _, err := s.Execute(checkedCreateArgs()); err != nil { + t.Fatal(err) + } + state, ok := s.Snapshot() + if !ok || len(state.Steps[0].Checks) != 1 { + t.Fatalf("missing check: %+v", state) + } + if _, err := s.Execute(`{"verb":"complete","step_id":"s1"}`); err == nil { + t.Fatal("completed a step before its check passed") + } + rendered := renderPlan(state, 4000) + parsed, err := parsePlanState(rendered, 4) + if err != nil { + t.Fatalf("rendered checked plan did not parse: %v\n%s", err, rendered) + } + if len(parsed.Steps[0].Checks) != 1 || parsed.Steps[0].Checks[0].Status != PlanCheckPending { + t.Fatalf("parsed check state: %+v", parsed.Steps[0].Checks) + } + epoch := s.CheckEpoch() + if !s.MatchesCheck("read_file", `{"line":1,"path":"out"}`) { + t.Fatal("argument key order did not match") + } + s.RecordCheckOutcome(epoch, "read_file", `{"path":"out","line":1}`, "call-1", false) + if _, err := s.Execute(`{"verb":"complete","step_id":"s1"}`); err != nil { + t.Fatal(err) + } + if got := s.PendingChecks(); len(got) != 0 { + t.Fatalf("pending checks after pass: %v", got) + } +} + +func TestPlanChecks_RuntimeInvalidationAndEpoch(t *testing.T) { + s := NewPlanStore(4, 4000) + if _, err := s.Execute(checkedCreateArgs()); err != nil { + t.Fatal(err) + } + epoch := s.CheckEpoch() + s.RecordCheckOutcome(epoch, "read_file", `{"path":"out","line":1}`, "call-1", false) + if _, err := s.Execute(`{"verb":"complete","step_id":"s1"}`); err != nil { + t.Fatal(err) + } + s.InvalidateChecks() + state, _ := s.Snapshot() + if state.Steps[0].Status != StepInProgress || state.Steps[0].Checks[0].Status != PlanCheckPending { + t.Fatalf("invalidation did not reopen check: %+v", state.Steps[0]) + } + s.mu.Lock() + s.plan.Steps[0].Status = StepDone + s.mu.Unlock() + s.RecordCheckOutcome(epoch, "read_file", `{"path":"out","line":1}`, "failed", true) + state, _ = s.Snapshot() + if state.Steps[0].Status != StepInProgress || len(s.PendingChecks()) != 1 || !strings.Contains(s.PendingChecks()[0], "s1/c1") { + t.Fatalf("failed outcome did not reopen step: %+v", state.Steps[0]) + } +} + +func TestPlanChecks_RejectsForgedOrOversizedDeclarations(t *testing.T) { + s := NewPlanStore(4, 4000) + for _, args := range []string{ + `{"verb":"create","steps":[{"id":"s1","title":"x","checks":[{"id":"c","description":"x","tool":"plan","arguments":{}}]}]}`, + `{"verb":"create","steps":[{"id":"s1","title":"x","checks":[{"id":"c","description":"x","tool":"read_file","arguments":[] }]}]}`, + `{"verb":"create","steps":[{"id":"s1","title":"x","checks":[{"id":"c","description":"x","tool":"read_file","arguments":{"x":"` + strings.Repeat("a", 4100) + `"}}]}]}`, + } { + if _, err := s.Execute(args); err == nil { + t.Fatalf("accepted invalid check declaration: %s", args[:minPlanTest(80, len(args))]) + } + } +} + +func minPlanTest(a, b int) int { + if a < b { + return a + } + return b +} + +func TestPlanChecks_CompletedUpdateCannotLoseEvidence(t *testing.T) { + s := NewPlanStore(4, 700) + if _, err := s.Execute(checkedCreateArgs()); err != nil { + t.Fatal(err) + } + s.RecordCheckOutcome(s.CheckEpoch(), "read_file", `{"path":"out","line":1}`, strings.Repeat("x", 128), false) + if _, err := s.Execute(`{"verb":"complete","step_id":"s1"}`); err != nil { + t.Fatal(err) + } + before, _ := s.Snapshot() + update := `{"verb":"update","updates":[{"id":"s1","note":"` + strings.Repeat("x", 600) + `"}]}` + if _, err := s.Execute(update); err == nil { + t.Fatal("accepted overflowing completed check") + } + after, _ := s.Snapshot() + if !reflect.DeepEqual(before, after) { + t.Fatal("rejected update changed state") + } + rendered := renderPlan(after, 700) + parsed, err := parsePlanState(rendered, 4) + if err != nil || len(parsed.Steps) != 1 || len(parsed.Steps[0].Checks) != 1 { + t.Fatalf("lost completed checks: %v %s", err, rendered) + } +} + +func TestPlanChecks_ReservedDelimiterAndLegacyCompatibility(t *testing.T) { + s := NewPlanStore(4, 4000) + legacy := `{"verb":"create","steps":[{"id":"legacy","title":"literal || checks: text"}]}` + if _, err := s.Execute(legacy); err != nil { + t.Fatal(err) + } + state, _ := s.Snapshot() + parsed, err := parsePlanState(renderPlan(state, 4000), 4) + if err != nil || parsed.Steps[0].Title != state.Steps[0].Title { + t.Fatalf("legacy delimiter: %v", err) + } + mixed := strings.Replace(checkedCreateArgs(), `"title":"verify"`, `"title":"literal || checks: text"`, 1) + if _, err := s.Execute(mixed); err == nil { + t.Fatal("accepted ambiguous checked title") + } + if _, err := s.Execute(checkedCreateArgs()); err != nil { + t.Fatal(err) + } + if _, err := s.Execute(`{"verb":"update","updates":[{"id":"s1","note":"literal || checks: text"}]}`); err == nil { + t.Fatal("accepted ambiguous checked note") + } +} + +func TestPlanChecks_NormalizedEvidenceAndStaleEpoch(t *testing.T) { + s := NewPlanStore(4, 4000) + if _, err := s.Execute(checkedCreateArgs()); err != nil { + t.Fatal(err) + } + oldEpoch := s.CheckEpoch() + if _, err := s.Execute(checkedCreateArgs()); err != nil { + t.Fatal(err) + } + s.RecordCheckOutcome(oldEpoch, "read_file", `{"line":1,"path":"out"}`, "old", false) + if len(s.PendingChecks()) != 1 { + t.Fatal("stale epoch supplied evidence") + } + id := strings.Repeat("\n😀", 200) + s.RecordCheckOutcome(s.CheckEpoch(), "read_file", `{"line":1,"path":"out"}`, id, false) + first, _ := s.Snapshot() + s.RecordCheckOutcome(s.CheckEpoch(), "read_file", `{"line":1,"path":"out"}`, id, false) + second, _ := s.Snapshot() + if first.Version != second.Version { + t.Fatal("normalized identical call ID bumped version") + } + if _, err := parsePlanState(renderPlan(second, 4000), 4); err != nil { + t.Fatal(err) + } + s.Restore(second) + restored, _ := s.Snapshot() + if len(s.PendingChecks()) != 1 || restored.Steps[0].Checks[0].CallID != "" { + t.Fatal("restored evidence trusted") + } +} + +func TestPlanChecks_ArgumentsAreIsolatedAndStrict(t *testing.T) { + s := NewPlanStore(4, 4000) + args := strings.Replace(checkedCreateArgs(), `"line":1`, `"line":1,"nested":{"values":[1,2]}`, 1) + if _, err := s.Execute(args); err != nil { + t.Fatal(err) + } + snap, _ := s.Snapshot() + snap.Steps[0].Checks[0].Arguments["nested"].(map[string]any)["values"].([]any)[0] = json.Number("99") + if !s.MatchesCheck("read_file", `{"path":"out","line":1,"nested":{"values":[1,2]}}`) { + t.Fatal("snapshot aliases stored arguments") + } + for _, raw := range []string{`{} {}`, `{} junk`, `[]`, `null`} { + if _, err := canonicalPlanArguments([]byte(raw)); err == nil { + t.Fatalf("accepted %s", raw) + } + } + forged := strings.Replace(checkedCreateArgs(), `"description":"Read the output"`, `"description":"Read the output","status":"passed","call_id":"forged"`, 1) + if _, err := s.Execute(forged); err != nil { + t.Fatal(err) + } + if len(s.PendingChecks()) != 1 { + t.Fatal("model self-certified check") + } +} diff --git a/internal/loop/plan_coverage_test.go b/internal/loop/plan_coverage_test.go index 06f8153f..4e72a9a8 100644 --- a/internal/loop/plan_coverage_test.go +++ b/internal/loop/plan_coverage_test.go @@ -6,6 +6,7 @@ package loop // ignore". Table-driven per existing plan_test.go conventions. import ( + "reflect" "strings" "testing" "unicode/utf8" @@ -307,7 +308,7 @@ func TestPlan_ParseStepLine_Table(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if got != tc.want { + if !reflect.DeepEqual(got, tc.want) { t.Errorf("step = %+v, want %+v", got, tc.want) } }) diff --git a/internal/loop/plan_test.go b/internal/loop/plan_test.go index 3d50827a..5549064b 100644 --- a/internal/loop/plan_test.go +++ b/internal/loop/plan_test.go @@ -1,6 +1,7 @@ package loop import ( + "reflect" "strings" "testing" @@ -85,7 +86,7 @@ func TestPlan_Validate_UpdateAtomicity(t *testing.T) { t.Errorf("version bumped after rejected update: %d -> %d", before.Version, after.Version) } for i, st := range after.Steps { - if st != before.Steps[i] { + if !reflect.DeepEqual(st, before.Steps[i]) { t.Errorf("step %d changed after rejection: %+v -> %+v", i, before.Steps[i], st) } } @@ -204,7 +205,7 @@ func TestPlan_RenderParseRoundTrip(t *testing.T) { t.Fatalf("steps = %d, want %d\nrendered:\n%s", len(got.Steps), len(in.Steps), rendered) } for i := range in.Steps { - if got.Steps[i] != in.Steps[i] { + if !reflect.DeepEqual(got.Steps[i], in.Steps[i]) { t.Errorf("step[%d] = %+v, want %+v", i, got.Steps[i], in.Steps[i]) } } @@ -237,7 +238,7 @@ func TestPlan_RoundTripThroughValidation(t *testing.T) { t.Fatalf("steps = %d, want %d", len(got.Steps), len(stored.Steps)) } for i := range stored.Steps { - if got.Steps[i] != stored.Steps[i] { + if !reflect.DeepEqual(got.Steps[i], stored.Steps[i]) { t.Errorf("step[%d] = %+v, want %+v", i, got.Steps[i], stored.Steps[i]) } } From c475d1e1f27a44d5ebada0a5707f0e920095de50 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:17:18 +0200 Subject: [PATCH 2/4] docs: explain acceptance checks and runtime evaluations --- README.md | 4 +++- docs/CONFIG.md | 6 +++++- docs/DEVELOPMENT.md | 9 +++++++++ docs/EVALS.md | 25 +++++++++++++++++++++++++ docs/PLANNING.md | 42 ++++++++++++++++++++++++++++++++++++++---- docs/WEBUI.md | 13 +++++++++++++ 6 files changed, 93 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index fc222fb0..c14f49c9 100644 --- a/README.md +++ b/README.md @@ -201,7 +201,8 @@ odek run "@README.md what does this project do?" | [Extensions](docs/EXTENSIONS.md) | `odek-extension/v1` contract: MCP limits, artifact refs, event stream, external refs, budgets | | [Maintenance](docs/MAINTENANCE.md) | Storage janitor: retention, log rotation, `odek cleanup` | | [Extended Memory](docs/EXTENDED_MEMORY.md) | Atomic long-term memory layer (opt-in) | -| [Planning](docs/PLANNING.md) | Plan tool, protected plan message, security model | +| [Planning](docs/PLANNING.md) | Plan tool, acceptance checks, completion evidence, resume behavior | +| [Runtime Evals](docs/EVALS.md) | Deterministic task scenarios, independent checks, JSON reports | | [Tool Selection](docs/TOOL_SELECTION.md) | Tool whitelist/blacklist guide and names reference | | [Daily Worker](docs/DAILY-WORKER.md) | Headless scheduled-worker patterns | | [Providers](docs/PROVIDERS.md) | go-llm-sdk registry, `--provider`, v2 knobs | @@ -238,6 +239,7 @@ The full `Config` struct supports: `Provider`, `Providers`, `BaseURL` (selected- go test ./... # full suite, no setup required go test -race ./... # also clean under the race detector go test -cover ./... # per-package coverage report +make eval # scripted runtime evaluations, no model credentials ODEK_E2E=1 go test ./cmd/odek/ # opt-in Docker / subprocess E2E suite ``` diff --git a/docs/CONFIG.md b/docs/CONFIG.md index a34b21e3..5ec5e222 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -408,7 +408,11 @@ Gives the agent a protected plan tool and a plan message that survives context t |-------|---------|---------|----------|-------------| | `planning.enabled` | `true` | `ODEK_PLANNING` | `--planning` / `--no-planning` | Enable the plan tool and protected plan message | | `planning.max_steps` | `12` | — | — | Plan steps allowed (clamped 1–50) | -| `planning.max_render_chars` | `2000` | — | — | Cap on the rendered plan shown in the UI (clamped 200–8000) | +| `planning.max_render_chars` | `2000` | — | — | Cap on the protected plan render (clamped 200–8000); checked plans must fit in full, including reserved evidence space | + +Acceptance checks need no additional flag: declare them through `plan create` +while planning is enabled. Large declarations may need a higher operator-set +`max_render_chars`; project config cannot raise this cap. Feature behavior, verbs, and the security model are documented in [PLANNING.md](PLANNING.md). diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 2e08f121..f496ad45 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -172,6 +172,15 @@ go test -v -count=1 ./cmd/odek/ -run "TestSubagent|TestDelegateTasks" Zero external test dependencies — tests use `httptest`, `testing`, and the standard library only. +### Runtime evaluations + +Run `make eval` (or `go run ./cmd/odek-eval`) from the repository root. The +eight scripted scenarios use the production loop with localhost fixture tools +and independent outcome checks. The command writes a JSON report and exits +nonzero if a scenario assertion fails; expected task failures can still pass +the scenario. No model credentials or external provider calls are needed. +See [EVALS.md](EVALS.md) for report fields, limitations, and adding scenarios. + ### Test layers | Layer | Runner | What's tested | diff --git a/docs/EVALS.md b/docs/EVALS.md index b15c1c24..8027dd8c 100644 --- a/docs/EVALS.md +++ b/docs/EVALS.md @@ -29,3 +29,28 @@ authorized real provider, but it must provide its own credentials, network policy, and model-message adapter. The shipped CLI intentionally does not evaluate live-model intelligence. Cost reporting stays unknown because the harness does not configure token prices. + +## Running and extending the suite + +```bash +make eval +# Save only the JSON report (without make's command echo): +go run ./cmd/odek-eval > eval-report.json +``` + +Exit status is 0 when every scenario passes, 1 for scenario failures, and 2 +if the report cannot be encoded. The eight-case baseline includes a deliberate +unguarded false-success control: all scenarios pass while +`false_completion_rate` is 0.125. That expected control is not a failure of +the checked-plan guard or a live-model benchmark. + +Add a case to `internal/eval.Scenarios` with fresh fixture state, scripted +responses, tools, and an independent oracle. Assert the required state and +observed tool outcomes, including expected failures; do not accept a success +claim as proof. `task_success` answers whether the fixture task was completed; +`scenario_passed` answers whether the runtime behaved as the test expected. +Add regression assertions in `internal/eval/eval_test.go`, then run: + +```bash +go test -count=1 -timeout=120s ./internal/eval ./internal/loop +``` diff --git a/docs/PLANNING.md b/docs/PLANNING.md index ab79c567..b91d0011 100644 --- a/docs/PLANNING.md +++ b/docs/PLANNING.md @@ -64,6 +64,17 @@ steps with checks also enforce their declared evidence before completion. ### Acceptance checks +Example user prompt: + +> Fix the auth bug. Create a plan with an acceptance check that runs +> `go test ./internal/auth`. Run that exact check after your changes and +> only mark the step complete when it passes. If it fails or cannot run, +> report the task as unverified. + +Replace the package path with one that exists in the target project. The +agent turns this instruction into the check declaration below; the prompt +itself does not bypass tool approval or execute a command. + A step may declare up to four optional acceptance checks. Each check contains an `id`, human-readable `description`, exact `tool` name, and an `arguments` object. Checks are evidence requirements attached to the step; they are not @@ -192,7 +203,7 @@ no-op. ### Collapse and overflow -When every step is `done`, the render collapses to a single line (~15 tokens), +For plans without checks, when every step is `done`, the render collapses to a single line (~15 tokens), so an idle or completed plan doesn't tempt the model to keep reporting on finished work: @@ -200,7 +211,11 @@ finished work: [Current plan: v7 — all 5 steps complete.] ``` -When the render would exceed `max_render_chars`, the oldest `done` steps are +Checked plans retain every step and check even when all steps are done. They +reserve evidence space at validation time, so accepted updates never rely on +lossy overflow rendering. + +For unchecked plans, when the render exceeds `max_render_chars`, the oldest `done` steps are dropped first behind an explicit marker: ``` @@ -281,7 +296,20 @@ current plan." "properties": { "id": { "type": "string" }, "title": { "type": "string" }, - "note": { "type": "string" } + "note": { "type": "string" }, + "checks": { + "type": "array", "maxItems": 4, + "items": { + "type": "object", + "properties": { + "id": { "type": "string" }, + "description": { "type": "string" }, + "tool": { "type": "string" }, + "arguments": { "type": "object" } + }, + "required": ["id", "description", "tool", "arguments"] + } + } }, "required": ["id", "title"] }, @@ -326,6 +354,8 @@ committing). | `update` referencing unknown id | `plan: update: unknown step id %q` | | `update` with unrecognized status token | `plan: update: step[%d]: unknown status %q` | | `complete` with unknown/missing step_id | `plan: complete: unknown step id %q` | +| checked step marked done before every check passes | `plan: complete: step %q has checks that have not passed` (or `plan: update: …`) | +| checked plan exceeds render space or uses reserved delimiter | `plan: checked plan exceeds max_render_chars (%d) or uses reserved checks delimiter in title/note` | | status already terminal-equal (no-op) | allowed — returns current plan, **no version bump** | Notes: @@ -368,7 +398,7 @@ Resolved onto the config layer as `Planning PlanningConfig` |-----|---------|-------|---------| | `enabled` | `true` | global-off wins | Master switch; false removes the tool from the registry and skips all plan logic | | `max_steps` | `12` | 1..50 | `create` size cap; enforced fail-closed | -| `max_render_chars` | `2000` | 200..8000 | Rendered message cap; overflow drops oldest done steps first behind `[+N done steps omitted]` | +| `max_render_chars` | `2000` | 200..8000 | Rendered message cap; checked plans must fit in full, unchecked overflow drops oldest done steps first | Disable precedence (highest wins): `--no-planning` flag → `ODEK_PLANNING=false` env → global config → project opt-out. @@ -454,6 +484,10 @@ over-cap plan may still display on surfaces even when resume would drop it. - `found:false` (still HTTP 200) when the transcript carries no parseable plan message; `version`/`steps` are then zero/empty. A collapsed all-done plan parses to a version with no rows — `steps` is `[]`, not null. +- Checked plans retain completed step rows. This endpoint exposes step + status, not individual check arguments, status, or call IDs; those remain + in the protected plan transcript. Reading the endpoint does not invalidate + evidence; resuming a run does. - **404** for an unknown session id; `note` is omitted when empty. - GET-only by contract: a non-GET request to `…/plan` cannot fall through to the base-session mutators (POST would otherwise rename the session diff --git a/docs/WEBUI.md b/docs/WEBUI.md index b87031fa..39700bc1 100644 --- a/docs/WEBUI.md +++ b/docs/WEBUI.md @@ -310,6 +310,19 @@ The core session CRUD (session-token gated): at least one field required (**400** otherwise). - **DELETE** — removes the session and its index entry; **204**. +### `GET /api/sessions/{id}/plan` + +Returns the persisted structured plan as `session_id`, `version`, `found`, +and `steps` (each with `id`, `title`, `status`, and optional `note`). This +read-only endpoint uses the session-token checks of the session API. A +missing plan returns HTTP 200 with `found: false`; an unknown session is 404. + +Steps with acceptance checks cannot be marked done until their declared +checks succeed. The endpoint and Now panel expose step statuses only; +individual check evidence remains in the protected plan transcript. See +[Planning](PLANNING.md#acceptance-checks) for declarations, invalidation, +and resume behavior. + ### `POST /api/cancel?session_id=` Cancels the prompt currently executing on a session (the REST twin of the From a4b4b7a091fef504f1b14b0fddee588d24032826 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:35:20 +0200 Subject: [PATCH 3/4] feat: revise plans incrementally without dropping acceptance checks --- README.md | 2 +- docs/CONFIG.md | 2 +- docs/DEVELOPMENT.md | 2 +- docs/EVALS.md | 6 +- docs/PLANNING.md | 327 +++++++++++++++++---- internal/eval/eval.go | 27 ++ internal/eval/eval_test.go | 8 +- internal/loop/completion_checks.go | 8 +- internal/loop/loop.go | 8 +- internal/loop/plan.go | 115 ++++++-- internal/loop/plan_checks.go | 16 +- internal/loop/plan_checks_test.go | 2 +- internal/loop/plan_revisions.go | 303 +++++++++++++++++++ internal/loop/plan_revisions_test.go | 46 +++ internal/loop/plan_test.go | 2 +- internal/loop/revision_integration_test.go | 99 +++++++ internal/loop/revision_safety_test.go | 139 +++++++++ 17 files changed, 1011 insertions(+), 101 deletions(-) create mode 100644 internal/loop/plan_revisions.go create mode 100644 internal/loop/plan_revisions_test.go create mode 100644 internal/loop/revision_integration_test.go create mode 100644 internal/loop/revision_safety_test.go diff --git a/README.md b/README.md index c14f49c9..42cf85d3 100644 --- a/README.md +++ b/README.md @@ -201,7 +201,7 @@ odek run "@README.md what does this project do?" | [Extensions](docs/EXTENSIONS.md) | `odek-extension/v1` contract: MCP limits, artifact refs, event stream, external refs, budgets | | [Maintenance](docs/MAINTENANCE.md) | Storage janitor: retention, log rotation, `odek cleanup` | | [Extended Memory](docs/EXTENDED_MEMORY.md) | Atomic long-term memory layer (opt-in) | -| [Planning](docs/PLANNING.md) | Plan tool, acceptance checks, completion evidence, resume behavior | +| [Planning](docs/PLANNING.md) | Incremental plan revisions, acceptance checks, completion evidence | | [Runtime Evals](docs/EVALS.md) | Deterministic task scenarios, independent checks, JSON reports | | [Tool Selection](docs/TOOL_SELECTION.md) | Tool whitelist/blacklist guide and names reference | | [Daily Worker](docs/DAILY-WORKER.md) | Headless scheduled-worker patterns | diff --git a/docs/CONFIG.md b/docs/CONFIG.md index 5ec5e222..6934bb82 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -408,7 +408,7 @@ Gives the agent a protected plan tool and a plan message that survives context t |-------|---------|---------|----------|-------------| | `planning.enabled` | `true` | `ODEK_PLANNING` | `--planning` / `--no-planning` | Enable the plan tool and protected plan message | | `planning.max_steps` | `12` | — | — | Plan steps allowed (clamped 1–50) | -| `planning.max_render_chars` | `2000` | — | — | Cap on the protected plan render (clamped 200–8000); checked plans must fit in full, including reserved evidence space | +| `planning.max_render_chars` | `2000` | — | — | Cap on the protected plan render (clamped 200–8000); checked or revised plans must fit in full, including reserved evidence space | Acceptance checks need no additional flag: declare them through `plan create` while planning is enabled. Large declarations may need a higher operator-set diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index f496ad45..3d2af955 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -175,7 +175,7 @@ Zero external test dependencies — tests use `httptest`, `testing`, and the sta ### Runtime evaluations Run `make eval` (or `go run ./cmd/odek-eval`) from the repository root. The -eight scripted scenarios use the production loop with localhost fixture tools +eleven scripted scenarios use the production loop with localhost fixture tools and independent outcome checks. The command writes a JSON report and exits nonzero if a scenario assertion fails; expected task failures can still pass the scenario. No model credentials or external provider calls are needed. diff --git a/docs/EVALS.md b/docs/EVALS.md index 8027dd8c..3c09632a 100644 --- a/docs/EVALS.md +++ b/docs/EVALS.md @@ -18,7 +18,7 @@ prices or estimate cost. The initial suite covers verified artifact work, a failed read followed by a false success claim, unrelated reads after a write, transient failure and recovery, cancellation, and plan acceptance checks for success, failed -evidence, and missing evidence. Negative cases can still be scenario passes +evidence, missing evidence, and incremental revision behavior. Negative cases can still be scenario passes when the oracle correctly records that the task did not succeed. The plan cases use the production `plan` tool and `PlanStore`, including the runtime incomplete marker for failed or missing evidence. @@ -39,9 +39,9 @@ go run ./cmd/odek-eval > eval-report.json ``` Exit status is 0 when every scenario passes, 1 for scenario failures, and 2 -if the report cannot be encoded. The eight-case baseline includes a deliberate +if the report cannot be encoded. The eleven-case baseline includes a deliberate unguarded false-success control: all scenarios pass while -`false_completion_rate` is 0.125. That expected control is not a failure of +`false_completion_rate` is 1/11 (about 0.091). That expected control is not a failure of the checked-plan guard or a live-model benchmark. Add a case to `internal/eval.Scenarios` with fresh fixture state, scripted diff --git a/docs/PLANNING.md b/docs/PLANNING.md index b91d0011..8e4f6a4c 100644 --- a/docs/PLANNING.md +++ b/docs/PLANNING.md @@ -203,7 +203,7 @@ no-op. ### Collapse and overflow -For plans without checks, when every step is `done`, the render collapses to a single line (~15 tokens), +For plans without checks or revision metadata, when every step is `done`, the render collapses to a single line (~15 tokens), so an idle or completed plan doesn't tempt the model to keep reporting on finished work: @@ -211,11 +211,11 @@ finished work: [Current plan: v7 — all 5 steps complete.] ``` -Checked plans retain every step and check even when all steps are done. They +Checked and revised plans retain every step and check even when all steps are done. They reserve evidence space at validation time, so accepted updates never rely on lossy overflow rendering. -For unchecked plans, when the render exceeds `max_render_chars`, the oldest `done` steps are +For unrevised unchecked plans, when the render exceeds `max_render_chars`, the oldest `done` steps are dropped first behind an explicit marker: ``` @@ -263,77 +263,295 @@ standard built-in interface (`Name`/`Description`/`Schema`/`Call`). | Verb | Arguments | Effect | |------|-----------|--------| -| `create` | `steps`: full ordered list (1..max_steps) | Replaces the whole plan wholesale — this *is* replanning. All steps start `pending`. | +| `create` | `steps`: full ordered list (1..max_steps) | Replaces the ordered plan. Existing acceptance-check identity cannot be dropped or changed. New steps start `pending`; unchanged checked steps retain progress. Prefer `revise` for incremental changes. | +| `revise` | `reason`, `operations` (≤8) | Applies bounded add/edit/move/split/supersede operations while preserving unaffected progress and evidence. `reason` is required and capped at 240 runes. | | `update` | `updates`: array of `{id, status?, note?}` | Batch status/note changes, applied in array order. Atomic: any invalid entry rejects the whole call. | | `complete` | `step_id` | Shorthand to mark one step `done`. Highest-frequency operation, one-field cheap. | | `get` | — | Returns the current plan (or `"No active plan."`). | +### Incremental revisions + +Use `revise` when the plan changes after work or evidence already exists. +It requires a `reason` of at most 240 runes and at most eight ordered +`operations`. The supported operation kinds are: + +- `add`: insert `steps` after `after_id` or before `before_id`; omit both to append. +- `edit`: change one `step_id` title or note, and optionally append checks. + A substantive title change reopens the step and resets its evidence. +- `move`: place `step_id` after `after_id` or before `before_id`; omit both + to move it to the end. +- `split`: replace `step_id` with replacement `steps`. +- `supersede`: replace `step_id` with replacement `steps` because the original + approach is no longer applicable. + +`split` and `supersede` may set `carry_checks_to` to a successor step name. +That successor receives all original checks with their tool, canonical +arguments, descriptions, and identities unchanged; the carried evidence is +reset to pending. Unaffected steps retain their status and evidence. The +latest revision reason and an operation source/target summary are persisted; +the plan does not keep unlimited revision history. The ordinary transcript +retains earlier revision tool calls. `add` and `move` accept either `after_id` +or `before_id`, including placement before the first step; specifying both +is rejected. An edit may clear a note with `"note":""`. No-op revisions do +not change the version or invalidate evidence. A `create` remains a full +replacement for ordinary plans, but once acceptance checks exist it cannot +drop or alter any original check identity, tool, arguments, or description. +Unchanged checked steps keep their status and evidence. Changing their title +invalidates that evidence; appending a pending check reopens a done step. + +Revision reasons describe plan changes only. They do not grant authorization, +approve tools, or override budgets and policy. Newly declared checks in a +revision cannot be satisfied by a tool call in the same parallel batch; they +must be run afterward through the normal tool and approval path. There is no +automatic optimizer, model-based cost planner, or automatic check execution +in this feature. + +Example user prompt: + +> Fix the bug and keep the plan current as you learn. If a failed test reveals +> more work, revise or split the affected step, keep its original acceptance +> checks, preserve completed work, and record why the approach changed. + +Example: + +```json +{"verb":"revise","reason":"The generated client needs a separate compatibility step","operations":[{"kind":"add","after_id":"tests","steps":[{"id":"compat","title":"Run compatibility checks"}]},{"kind":"edit","step_id":"tests","note":"Keep the original regression evidence"}]} +``` + ### JSON Schema ```json { + "description": "Maintain your task plan. Create steps before starting multi-step work; update statuses as you go (in_progress when you start a step, done only after verifying it); mark blocked with a note explaining why. The plan is shown to you on every iteration and survives context trimming — trust it over your memory of earlier turns. Use revise when the approach changes so acceptance checks stay attached. For verifiable work, declare optional checks with exact tool arguments before running them in a later batch. Complete checked steps only after their tools succeed; do not self-certify. Plans without checks remain advisory.", "name": "plan", - "description": "Maintain your task plan. Create steps before starting -multi-step work; update statuses as you go (in_progress when you start a step, -done only after verifying it); mark blocked with a note explaining why. The -plan is shown to you on every iteration and survives context trimming — trust -it over your memory of earlier turns. Replan freely with create when the -approach changes; plans without checks are steering aids, while checked steps -also require their declared evidence before completion.", "parameters": { - "type": "object", "properties": { - "verb": { - "enum": ["create", "update", "complete", "get"], - "description": "create: replace the whole plan. update: batch -status/note changes. complete: shorthand to mark one step done. get: return -current plan." + "operations": { + "description": "revise only: ordered atomic operations. Existing checks cannot be removed or changed. New checks require a later tool batch.", + "items": { + "properties": { + "after_id": { + "description": "For add/move: place after this existing step. Use only one anchor; omit both to append.", + "type": "string" + }, + "before_id": { + "description": "For add/move: place before this existing step. Mutually exclusive with after_id.", + "type": "string" + }, + "carry_checks_to": { + "description": "Required when split/supersede replaces a checked step: replacement ID receiving all original checks, pending fresh verification.", + "type": "string" + }, + "checks": { + "description": "Optional checks with exact tool arguments. In revise edit, append only; IDs must not duplicate existing checks. Invoke tools separately through normal approval; only actual outcomes can pass checks.", + "items": { + "properties": { + "arguments": { + "type": "object" + }, + "description": { + "type": "string" + }, + "id": { + "type": "string" + }, + "tool": { + "type": "string" + } + }, + "required": [ + "id", + "description", + "tool", + "arguments" + ], + "type": "object" + }, + "maxItems": 4, + "type": "array" + }, + "kind": { + "enum": [ + "add", + "edit", + "move", + "split", + "supersede" + ] + }, + "note": { + "description": "edit only: replace note, including an empty string to clear it.", + "type": "string" + }, + "step_id": { + "description": "Existing step for edit/move/split/supersede.", + "type": "string" + }, + "steps": { + "description": "New steps for add/split/supersede. Split requires at least two. Replacement IDs must be unique; carried checks are added to the target's declared checks.", + "items": { + "properties": { + "checks": { + "description": "Optional checks with exact tool arguments. In revise edit, append only; IDs must not duplicate existing checks. Invoke tools separately through normal approval; only actual outcomes can pass checks.", + "items": { + "properties": { + "arguments": { + "type": "object" + }, + "description": { + "type": "string" + }, + "id": { + "type": "string" + }, + "tool": { + "type": "string" + } + }, + "required": [ + "id", + "description", + "tool", + "arguments" + ], + "type": "object" + }, + "maxItems": 4, + "type": "array" + }, + "id": { + "type": "string" + }, + "note": { + "type": "string" + }, + "title": { + "type": "string" + } + }, + "required": [ + "id", + "title" + ], + "type": "object" + }, + "minItems": 1, + "type": "array" + }, + "title": { + "description": "edit only: replace title; changed work loses its prior check evidence and reopens if completed.", + "type": "string" + } + }, + "required": [ + "kind" + ], + "type": "object" + }, + "maxItems": 8, + "minItems": 1, + "type": "array" + }, + "reason": { + "description": "revise only: bounded reason for the change.", + "maxLength": 240, + "type": "string" + }, + "step_id": { + "description": "complete only", + "type": "string" }, "steps": { - "type": "array", + "description": "create only: full ordered step list (1..max_steps). New steps start pending. Existing checked requirements must remain identical; unchanged checked steps retain progress. Prefer revise for incremental changes.", "items": { - "type": "object", "properties": { - "id": { "type": "string" }, - "title": { "type": "string" }, - "note": { "type": "string" }, "checks": { - "type": "array", "maxItems": 4, + "description": "Optional checks with exact tool arguments. In revise edit, append only; IDs must not duplicate existing checks. Invoke tools separately through normal approval; only actual outcomes can pass checks.", "items": { - "type": "object", "properties": { - "id": { "type": "string" }, - "description": { "type": "string" }, - "tool": { "type": "string" }, - "arguments": { "type": "object" } + "arguments": { + "type": "object" + }, + "description": { + "type": "string" + }, + "id": { + "type": "string" + }, + "tool": { + "type": "string" + } }, - "required": ["id", "description", "tool", "arguments"] - } + "required": [ + "id", + "description", + "tool", + "arguments" + ], + "type": "object" + }, + "maxItems": 4, + "type": "array" + }, + "id": { + "type": "string" + }, + "note": { + "type": "string" + }, + "title": { + "type": "string" } }, - "required": ["id", "title"] + "required": [ + "id", + "title" + ], + "type": "object" }, - "description": "create only: full ordered step list (1..max_steps). -All steps start pending; mark the first one in_progress with a follow-up -update (can ride the same parallel batch)." + "type": "array" }, "updates": { - "type": "array", + "description": "update only: applied in array order; unknown id or unknown status fails the whole call (atomic).", "items": { - "type": "object", "properties": { - "id": { "type": "string" }, - "status": { "enum": ["pending", "in_progress", "done", "blocked"] }, - "note": { "type": "string" } + "id": { + "type": "string" + }, + "note": { + "type": "string" + }, + "status": { + "enum": [ + "pending", + "in_progress", + "done", + "blocked" + ] + } }, - "required": ["id"] + "required": [ + "id" + ], + "type": "object" }, - "description": "update only: applied in array order; unknown id or -unknown status fails the whole call (atomic)." + "type": "array" }, - "step_id": { "type": "string", "description": "complete only" } + "verb": { + "description": "create: replace the whole plan. update: batch status/note changes. complete: shorthand to mark one step done. revise: atomically add/edit/move/split/supersede steps while preserving checked requirements. get: return current plan.", + "enum": [ + "create", + "update", + "complete", + "revise", + "get" + ] + } }, - "required": ["verb"] + "required": [ + "verb" + ], + "type": "object" } } ``` @@ -398,7 +616,7 @@ Resolved onto the config layer as `Planning PlanningConfig` |-----|---------|-------|---------| | `enabled` | `true` | global-off wins | Master switch; false removes the tool from the registry and skips all plan logic | | `max_steps` | `12` | 1..50 | `create` size cap; enforced fail-closed | -| `max_render_chars` | `2000` | 200..8000 | Rendered message cap; checked plans must fit in full, unchecked overflow drops oldest done steps first | +| `max_render_chars` | `2000` | 200..8000 | Rendered message cap; checked/revised plans must fit in full, unrevised unchecked overflow drops oldest done steps first | Disable precedence (highest wins): `--no-planning` flag → `ODEK_PLANNING=false` env → global config → project opt-out. @@ -485,7 +703,7 @@ over-cap plan may still display on surfaces even when resume would drop it. plan message; `version`/`steps` are then zero/empty. A collapsed all-done plan parses to a version with no rows — `steps` is `[]`, not null. - Checked plans retain completed step rows. This endpoint exposes step - status, not individual check arguments, status, or call IDs; those remain + status, not individual check arguments, status, call IDs, or revision metadata; those remain in the protected plan transcript. Reading the endpoint does not invalidate evidence; resuming a run does. - **404** for an unknown session id; `note` is omitted when empty. @@ -537,6 +755,13 @@ such a relay lands; responses are tiny, so the cadence is cheap. ## Tests +Revision regressions cover atomic rejection, check-preserving splits and +replacement, safe evidence retention, same-batch failures, no-op edits, +reordering, render limits, wrapped save/resume, and skill rematching in +`plan_revisions_test.go`, `revision_safety_test.go`, and +`revision_integration_test.go`. [Runtime evals](EVALS.md) exercise complete +replanning scenarios with independent fixture-state checks. + `internal/loop/plan_test.go`: - Validation matrix (`TestPlan_Validate_CreateCaps`, `_UpdateAtomicity`, @@ -646,7 +871,7 @@ semantics, payload minimality, `ExtractPlan`), `cmd/odek/serve_plan_test.go` these payloads may be the only surviving context after a trim, so ids alone tell the summarizer nothing. Stall hints keep the title-free id format. - **Blocked-step streak.** Three consecutive transitions to `blocked` inject - a decompose-or-`create` hint and emit `plan_blocked` (`steps`, `blocked`, + a decompose-with-`revise` hint and emit `plan_blocked` (`steps`, `blocked`, `version` only). The streak resets on `create` or a `done` / `in_progress` transition, and after firing (once then reset). - **Remaining-steps on exhaustion.** Pending / in_progress / blocked IDs and @@ -685,9 +910,9 @@ for dashboards. No titles, notes, or loop behavior. ## Design Notes -**One tool, four verbs — not many tools.** Tool schemas count against the +**One tool, five verbs.** Tool schemas count against the context budget on every request; splitting into `plan_create`/`plan_update`/… -would cost 4× schema tokens and reserve 4× names for marginal clarity. +would repeat schema fields and reserve more names for marginal clarity. `complete` exists as a verb because it is the highest-frequency, lowest- argument operation — making it cheap encourages actually closing steps. @@ -704,5 +929,5 @@ pattern instead: recognized-by-prefix, `headLen`-protected, upsert-in-place. **Bias to action.** The plan remains a steering instrument for ordinary steps; acceptance checks add a narrow completion gate without auto-executing -anything. `create` replaces wholesale so replanning is one call, and the -prompt distinguishes advisory steps from checked completion. +anything. `revise` changes the approach incrementally while keeping acceptance checks. +The prompt distinguishes advisory steps from checked completion. diff --git a/internal/eval/eval.go b/internal/eval/eval.go index 75ab914d..13944557 100644 --- a/internal/eval/eval.go +++ b/internal/eval/eval.go @@ -27,6 +27,7 @@ type Fixture struct { Values map[string]string Calls []ToolCall GoodCalls []ToolCall + Plan *loop.PlanState mu sync.Mutex } @@ -194,6 +195,11 @@ func runCase(parent context.Context, s Scenario, opts RunOptions) CaseReport { parent = cancelled } result, runErr := e.Run(parent, s.Task) + if planStore != nil && s.Fixture != nil { + if state, ok := planStore.Snapshot(); ok { + s.Fixture.Plan = &state + } + } mu.Lock() snapshot := append([]ToolCall(nil), calls...) mu.Unlock() @@ -298,6 +304,9 @@ func Scenarios() []Scenario { f6.Values["evidence"] = "evidence" f7 := baseFixture() f8 := baseFixture() + f9 := baseFixture() + f10 := baseFixture() + f11 := baseFixture() return []Scenario{ {Name: "successful_fix_verified", Task: "write and verify artifact", Fixture: f1, Tools: []tool.Tool{Tool(f1, "write_file", "write"), Tool(f1, "read_file", "read")}, Responses: []string{toolCall("write_file", "w1", `{"key":"artifact","value":"fixed"}`), toolCall("read_file", "r1", `{"key":"artifact"}`), final("verified")}, Oracle: func(f *Fixture, r string, e error, _ []ToolCall) OracleResult { if e != nil { @@ -362,6 +371,24 @@ func Scenarios() []Scenario { } return OracleResult{TaskSuccess: false} }}, + {Name: "incremental_replan_preserves_done", Task: "revise plan without losing completed work", Fixture: f9, Plan: true, Responses: []string{toolCall("plan", "p1", `{"verb":"create","steps":[{"id":"fix","title":"Fix"},{"id":"test","title":"Test"}]}`), toolCall("plan", "p2", `{"verb":"update","updates":[{"id":"fix","status":"done"}]}`), toolCall("plan", "p3", `{"verb":"revise","reason":"split test and reorder","operations":[{"kind":"split","step_id":"test","steps":[{"id":"test-a","title":"Test A"},{"id":"test-b","title":"Test B"}]},{"kind":"move","step_id":"fix","after_id":"test-b"}]}`), final("working")}, Oracle: func(f *Fixture, _ string, e error, c []ToolCall) OracleResult { + if e != nil || f.Plan == nil || len(f.Plan.Steps) != 3 || f.Plan.Steps[0].ID != "test-a" || f.Plan.Steps[1].ID != "test-b" || f.Plan.Steps[2].ID != "fix" || f.Plan.Steps[2].Status != loop.StepDone || len(c) != 3 || c[1].Error || c[2].Error { + return OracleResult{Errors: []string{"incremental update lost completed work or failed atomically"}} + } + return OracleResult{TaskSuccess: false} + }}, + {Name: "acceptance_check_cannot_be_dropped", Task: "failed check must block completion", Fixture: f10, Plan: true, Tools: []tool.Tool{Tool(f10, "read_file", "read")}, Responses: []string{toolCall("plan", "p1", `{"verb":"create","steps":[{"id":"fix","title":"Fix","checks":[{"id":"e1","description":"Read evidence","tool":"read_file","arguments":{"key":"evidence"}}]}]}`), toolCall("read_file", "r1", `{"key":"evidence"}`), toolCall("plan", "p2", `{"verb":"revise","reason":"bad supersede","operations":[{"kind":"supersede","step_id":"fix","steps":[{"id":"replacement","title":"Replacement"}]}]}`), toolCall("plan", "p3", `{"verb":"create","steps":[{"id":"fix","title":"Fix"}]}`), final("blocked")}, Oracle: func(f *Fixture, r string, e error, c []ToolCall) OracleResult { + if e != nil || f.Plan == nil || len(f.Plan.Steps) != 1 || f.Plan.Steps[0].Status == loop.StepDone || len(f.Plan.Steps[0].Checks) != 1 || f.Plan.Steps[0].Checks[0].ID != "e1" || f.Plan.Steps[0].Checks[0].Status != loop.PlanCheckFailed || !strings.Contains(r, "[odek verification incomplete:") || len(c) != 4 || !c[1].Error || !c[2].Error || !c[3].Error { + return OracleResult{Errors: []string{"failed acceptance check was dropped or completion was accepted"}} + } + return OracleResult{TaskSuccess: false} + }}, + {Name: "replan_rerun_can_finish", Task: "rerun failed check after fix", Fixture: f11, Plan: true, Tools: []tool.Tool{Tool(f11, "read_file", "read"), Tool(f11, "write_file", "write")}, Responses: []string{toolCall("plan", "p1", `{"verb":"create","steps":[{"id":"fix","title":"Fix","checks":[{"id":"e1","description":"Read evidence","tool":"read_file","arguments":{"key":"evidence"}}]}]}`), toolCall("read_file", "r1", `{"key":"evidence"}`), toolCall("plan", "p2", `{"verb":"revise","reason":"split fix after failure","operations":[{"kind":"split","step_id":"fix","carry_checks_to":"fix-a","steps":[{"id":"fix-a","title":"Fix A"},{"id":"fix-b","title":"Fix B"}]}]}`), toolCall("write_file", "w1", `{"key":"evidence","value":"fixed"}`), toolCall("read_file", "r2", `{"key":"evidence"}`), toolCall("plan", "p3", `{"verb":"update","updates":[{"id":"fix-a","status":"done"},{"id":"fix-b","status":"done"}]}`), final("complete")}, Oracle: func(f *Fixture, r string, e error, c []ToolCall) OracleResult { + if e != nil || f.Plan == nil || len(f.Plan.Steps) != 2 || f.Plan.Steps[0].Status != loop.StepDone || f.Plan.Steps[1].Status != loop.StepDone || len(f.Plan.Steps[0].Checks) != 1 || f.Plan.Steps[0].Checks[0].Status != loop.PlanCheckPassed || strings.Contains(r, "[odek verification incomplete:") || f.get("evidence") != "fixed" || len(c) != 6 || !c[1].Error || c[2].Error || c[3].Error || c[4].Error || c[5].Error { + return OracleResult{Errors: []string{"successful rerun did not complete the fixed acceptance check"}} + } + return OracleResult{TaskSuccess: true} + }}, } } diff --git a/internal/eval/eval_test.go b/internal/eval/eval_test.go index 1f75f03a..796c9128 100644 --- a/internal/eval/eval_test.go +++ b/internal/eval/eval_test.go @@ -9,8 +9,8 @@ import ( func TestDefaultScenariosAreIndependentAndBounded(t *testing.T) { r := Run(context.Background(), Scenarios()) - if r.Total != 8 { - t.Fatalf("total=%d want 8", r.Total) + if r.Total != 11 { + t.Fatalf("total=%d want 11", r.Total) } if r.Failed != 0 { t.Fatalf("scenario failures=%d report=%+v", r.Failed, r) @@ -27,8 +27,8 @@ func TestDefaultScenariosAreIndependentAndBounded(t *testing.T) { taskSuccess++ } } - if taskSuccess != 3 { - t.Fatalf("task successes=%d want 3", taskSuccess) + if taskSuccess != 4 { + t.Fatalf("task successes=%d want 4", taskSuccess) } for _, c := range r.Cases { if c.Name == "plan_check_failed" || c.Name == "plan_check_missing" { diff --git a/internal/loop/completion_checks.go b/internal/loop/completion_checks.go index 814a9fd9..7fc3cb26 100644 --- a/internal/loop/completion_checks.go +++ b/internal/loop/completion_checks.go @@ -15,14 +15,16 @@ func (e *Engine) recordPlanCheckResult(epoch uint64, tc session.ToolCall, callID if e.planStore == nil || tc.Function.Name == "plan" { return } - if e.planStore.CheckEpoch() == epoch && e.planStore.MatchesCheck(tc.Function.Name, tc.Function.Arguments) { + if e.planStore.MatchesCheck(tc.Function.Name, tc.Function.Arguments) { if failed { // Failed verification may itself leave partial effects. Earlier // checks cannot establish the state after that failure. e.planStore.InvalidateChecks() } - e.planStore.RecordCheckOutcome(epoch, tc.Function.Name, tc.Function.Arguments, callID, failed) - return + if e.planStore.CheckEpoch() == epoch { + e.planStore.RecordCheckOutcome(epoch, tc.Function.Name, tc.Function.Arguments, callID, failed) + return + } } fx := e.executionEffects(tc) if fx.unknown || len(fx.writes) > 0 { diff --git a/internal/loop/loop.go b/internal/loop/loop.go index c24301e9..b3ccf1b4 100644 --- a/internal/loop/loop.go +++ b/internal/loop/loop.go @@ -869,8 +869,10 @@ func (e *Engine) SetPlanStore(s *PlanStore) { // emit plan_blocked. Payloads carry counts and version ONLY — never // step titles or notes. func (e *Engine) emitPlanChangeEvent(ch PlanChange) { - if ch.Created { + if ch.Created || ch.Revised { e.skillRematchPending.Store(true) + } + if ch.Created { e.emitEvent(events.Event{ Type: events.TypePlanCreated, Data: map[string]any{ @@ -3749,7 +3751,7 @@ func (e *Engine) runLoop(ctx context.Context, in []session.Message) (answer stri corrections = append(corrections, e.budgetWarnings(i+1, startTime, ctx, &hints)...) } if e.blockedHintPending.Swap(false) { - corrections = append(corrections, "⚠️ Plan has 3 consecutive blocked steps. Decompose the blocked work or `create` a new plan — do not keep marking steps blocked.") + corrections = append(corrections, "⚠️ Plan has 3 consecutive blocked steps. Use plan revise to decompose blocked work or change the approach while preserving acceptance checks — do not keep marking steps blocked.") } // Inject all corrections as a single system message if len(corrections) > 0 { @@ -3766,7 +3768,7 @@ func (e *Engine) runLoop(ctx context.Context, in []session.Message) (answer stri // the persisted snapshot always carries the current plan. messages = e.refreshPlanMessage(ctx, messages) - // After plan(create), rematch lazy skills on step titles only so a + // After plan(create/revise), rematch lazy skills on step titles only so a // plan that names work the original user line missed can still load // a promoted skill. Titles are query text, not instructions. if e.skillRematchPending.Swap(false) { diff --git a/internal/loop/plan.go b/internal/loop/plan.go index 3e53a9d7..d44defa2 100644 --- a/internal/loop/plan.go +++ b/internal/loop/plan.go @@ -77,8 +77,14 @@ type PlanCheck struct { // PlanState is the authoritative plan. Version bumps on every mutation and // is echoed in the rendered message so drift is correlatable. type PlanState struct { - Version int `json:"version"` - Steps []PlanStep `json:"steps"` + Version int `json:"version"` + Steps []PlanStep `json:"steps"` + Revision *PlanRevision `json:"revision,omitempty"` +} + +type PlanRevision struct { + Reason string `json:"reason"` + Summary []string `json:"summary"` } // PlanChange describes one effective plan mutation for the change @@ -95,6 +101,7 @@ type PlanChange struct { Pending int Version int // store version after the mutation BlockedStreak bool // true when this mutation tripped the 3-blocked streak + Revised bool } // Structural caps enforced by validation (docs/PLANNING.md — Fail-Closed @@ -129,6 +136,7 @@ type PlanStore struct { lastBlocked bool // last status transition was to blocked blockedFired bool // this mutation tripped the streak (consumed by notify) epoch uint64 + revisionNotify bool } // NewPlanStore creates a store with the given resolved caps. Degenerate @@ -191,10 +199,14 @@ func (s *PlanStore) Restore(st PlanState) { s.blockedStreak = 0 s.lastBlocked = false s.blockedFired = false + s.revisionNotify = false } func clonePlanState(st PlanState) PlanState { st.Steps = append([]PlanStep(nil), st.Steps...) + if st.Revision != nil { + st.Revision = &PlanRevision{Reason: st.Revision.Reason, Summary: append([]string(nil), st.Revision.Summary...)} + } for i := range st.Steps { st.Steps[i].Checks = clonePlanChecks(st.Steps[i].Checks) } @@ -276,6 +288,7 @@ func (s *PlanStore) Execute(argsJSON string) (string, error) { var res string var err error s.blockedFired = false + s.revisionNotify = false switch args.Verb { case "create": res, err = s.create(args.Steps) @@ -283,10 +296,12 @@ func (s *PlanStore) Execute(argsJSON string) (string, error) { res, err = s.update(args.Updates) case "complete": res, err = s.complete(args.StepID) + case "revise": + res, err = s.revise(argsJSON) case "get": return s.get() default: - return "", fmt.Errorf("plan: unknown verb %q (want create/update/complete/get)", args.Verb) + return "", fmt.Errorf("plan: unknown verb %q (want create/update/complete/revise/get)", args.Verb) } // A version bump is exactly the "effective mutation" contract: no-op // update/complete calls return early without reassigning s.plan, so they @@ -295,6 +310,7 @@ func (s *PlanStore) Execute(argsJSON string) (string, error) { if err == nil && s.plan != nil && s.plan.Version != prevVersion { s.notifyLocked(args.Verb == "create", s.blockedFired) } + s.revisionNotify = false return res, err } @@ -309,6 +325,7 @@ func (s *PlanStore) notifyLocked(created, blockedStreak bool) { Steps: len(s.plan.Steps), Version: s.plan.Version, BlockedStreak: blockedStreak, + Revised: s.revisionNotify, } for _, st := range s.plan.Steps { switch st.Status { @@ -323,6 +340,7 @@ func (s *PlanStore) notifyLocked(created, blockedStreak bool) { } } s.onChange(ch) + s.revisionNotify = false } // nextVersion returns the version the next successful mutation gets: @@ -372,6 +390,11 @@ func (s *PlanStore) create(steps []planStepArg) (string, error) { out = append(out, PlanStep{ID: id, Title: title, Status: StepPending, Note: normalizePlanText(in.Note), Checks: checks}) } candidate := PlanState{Version: s.nextVersion(), Steps: out} + if s.plan != nil && hasPlanChecks(*s.plan) { + if err := preserveCheckedPlan(*s.plan, &candidate); err != nil { + return "", err + } + } if !checkedPlanFits(candidate, s.maxRenderChars) { return "", fmt.Errorf("plan: checked plan exceeds max_render_chars (%d) or uses reserved checks delimiter in title/note", s.maxRenderChars) } @@ -428,7 +451,7 @@ func (s *PlanStore) update(updates []planUpdateArg) (string, error) { if s.plan != nil { old = s.plan.Steps } - candidate := PlanState{Version: s.nextVersion(), Steps: working} + candidate := PlanState{Version: s.nextVersion(), Steps: working, Revision: cloneRevision(s.plan.Revision)} if !checkedPlanFits(candidate, s.maxRenderChars) { return "", fmt.Errorf("plan: checked plan exceeds max_render_chars (%d) or uses reserved checks delimiter in title/note", s.maxRenderChars) } @@ -454,7 +477,7 @@ func (s *PlanStore) complete(stepID string) (string, error) { return "", fmt.Errorf("plan: complete: step %q has checks that have not passed", working[idx].ID) } working[idx].Status = StepDone - candidate := PlanState{Version: s.nextVersion(), Steps: working} + candidate := PlanState{Version: s.nextVersion(), Steps: working, Revision: cloneRevision(s.plan.Revision)} if !checkedPlanFits(candidate, s.maxRenderChars) { return "", fmt.Errorf("plan: checked plan exceeds max_render_chars (%d) or uses reserved checks delimiter in title/note", s.maxRenderChars) } @@ -652,7 +675,7 @@ const planTruncatedMarker = "[plan truncated: exceeded max_render_chars]" // remainder still does not fit, the tail is hard-truncated. func renderPlan(p PlanState, maxChars int) string { header := planHeaderLine(p) - if allStepsDone(p) { + if allStepsDone(p) && p.Revision == nil { return header } lines := make([]string, 0, len(p.Steps)) @@ -662,6 +685,11 @@ func renderPlan(p PlanState, maxChars int) string { build := func(omit map[int]bool, omitted int) string { parts := make([]string, 0, len(lines)+2) parts = append(parts, header) + if p.Revision != nil { + if b, err := json.Marshal(p.Revision); err == nil { + parts = append(parts, "[Plan revision: "+string(b)+"]") + } + } if omitted > 0 { parts = append(parts, fmt.Sprintf(planOverflowMarker, omitted)) } @@ -711,7 +739,7 @@ func planHeaderLine(p PlanState) string { blocked++ } } - if len(p.Steps) > 0 && done == len(p.Steps) && !hasPlanChecks(p) { + if len(p.Steps) > 0 && done == len(p.Steps) && !hasPlanChecks(p) && p.Revision == nil { return fmt.Sprintf("[Current plan: v%d — all %d steps complete.]", p.Version, len(p.Steps)) } checked := "" @@ -767,6 +795,7 @@ func parsePlanState(content string, maxSteps int) (PlanState, error) { return PlanState{Version: version}, nil } lines = lines[1:] + var revision *PlanRevision // An omission marker means the live render overflowed and dropped done // steps. Resuming such a plan would be lossy — the omitted steps are @@ -784,6 +813,15 @@ func parsePlanState(content string, maxSteps int) (PlanState, error) { if err != nil { return PlanState{}, err } + if len(lines) > 0 && strings.HasPrefix(lines[0], "[Plan revision: ") && strings.HasSuffix(lines[0], "]") { + var rev PlanRevision + raw := strings.TrimSuffix(strings.TrimPrefix(lines[0], "[Plan revision: "), "]") + if err := json.Unmarshal([]byte(raw), &rev); err != nil || !validPlanRevision(&rev) { + return PlanState{}, errors.New("plan: invalid revision metadata") + } + revision = &rev + lines = lines[1:] + } if len(lines) == 0 { return PlanState{}, errors.New("plan: no step lines") @@ -821,7 +859,7 @@ func parsePlanState(content string, maxSteps int) (PlanState, error) { if visibleBlocked != blocked { return PlanState{}, fmt.Errorf("plan: header claims %d blocked, found %d", blocked, visibleBlocked) } - state := PlanState{Version: version, Steps: steps} + state := PlanState{Version: version, Steps: steps, Revision: revision} if checkedHeader != hasPlanChecks(state) { return PlanState{}, errors.New("plan: check header does not match steps") } @@ -1074,40 +1112,41 @@ func (t *PlanTool) Description() string { "update statuses as you go (in_progress when you start a step, done only after " + "verifying it); mark blocked with a note explaining why. The plan is shown to you " + "on every iteration and survives context trimming — trust it over your memory of " + - "earlier turns. Replan freely with create when the approach changes. For verifiable " + + "earlier turns. Use revise when the approach changes so acceptance checks stay attached. For verifiable " + "work, declare optional checks with exact tool arguments before running them in a later " + "batch. Complete checked steps only after their tools succeed; do not self-certify. " + "Plans without checks remain advisory." } +func planChecksSchema() map[string]any { + return map[string]any{ + "type": "array", "maxItems": maxPlanChecks, + "description": "Optional checks with exact tool arguments. In revise edit, append only; IDs must not duplicate existing checks. Invoke tools separately through normal approval; only actual outcomes can pass checks.", + "items": map[string]any{"type": "object", "properties": map[string]any{ + "id": map[string]any{"type": "string"}, "description": map[string]any{"type": "string"}, + "tool": map[string]any{"type": "string"}, "arguments": map[string]any{"type": "object"}, + }, "required": []string{"id", "description", "tool", "arguments"}}, + } +} + +func planStepSchema() map[string]any { + return map[string]any{"type": "object", "properties": map[string]any{ + "id": map[string]any{"type": "string"}, "title": map[string]any{"type": "string"}, + "note": map[string]any{"type": "string"}, "checks": planChecksSchema(), + }, "required": []string{"id", "title"}} +} + func (t *PlanTool) Schema() any { return map[string]any{ "type": "object", "properties": map[string]any{ "verb": map[string]any{ - "enum": []string{"create", "update", "complete", "get"}, - "description": "create: replace the whole plan. update: batch status/note changes. complete: shorthand to mark one step done. get: return current plan.", + "enum": []string{"create", "update", "complete", "revise", "get"}, + "description": "create: replace the whole plan. update: batch status/note changes. complete: shorthand to mark one step done. revise: atomically add/edit/move/split/supersede steps while preserving checked requirements. get: return current plan.", }, "steps": map[string]any{ - "type": "array", - "items": map[string]any{ - "type": "object", - "properties": map[string]any{ - "id": map[string]any{"type": "string"}, - "title": map[string]any{"type": "string"}, - "note": map[string]any{"type": "string"}, - "checks": map[string]any{ - "type": "array", "maxItems": 4, - "description": "Optional declarative verification checks. Run the named tool separately; only its actual result can pass a check.", - "items": map[string]any{"type": "object", "properties": map[string]any{ - "id": map[string]any{"type": "string"}, "description": map[string]any{"type": "string"}, - "tool": map[string]any{"type": "string"}, "arguments": map[string]any{"type": "object"}, - }, "required": []string{"id", "description", "tool", "arguments"}}, - }, - }, - "required": []string{"id", "title"}, - }, - "description": "create only: full ordered step list (1..max_steps). All steps start pending; mark the first one in_progress with a follow-up update (can ride the same parallel batch).", + "type": "array", "items": planStepSchema(), + "description": "create only: full ordered step list (1..max_steps). New steps start pending. Existing checked requirements must remain identical; unchanged checked steps retain progress. Prefer revise for incremental changes.", }, "updates": map[string]any{ "type": "array", @@ -1126,6 +1165,22 @@ func (t *PlanTool) Schema() any { "type": "string", "description": "complete only", }, + "reason": map[string]any{"type": "string", "maxLength": maxRevisionReason, "description": "revise only: bounded reason for the change."}, + "operations": map[string]any{ + "type": "array", "minItems": 1, "maxItems": maxRevisionOps, + "description": "revise only: ordered atomic operations. Existing checks cannot be removed or changed. New checks require a later tool batch.", + "items": map[string]any{"type": "object", "properties": map[string]any{ + "kind": map[string]any{"enum": []string{"add", "edit", "move", "split", "supersede"}}, + "step_id": map[string]any{"type": "string", "description": "Existing step for edit/move/split/supersede."}, + "after_id": map[string]any{"type": "string", "description": "For add/move: place after this existing step. Use only one anchor; omit both to append."}, + "before_id": map[string]any{"type": "string", "description": "For add/move: place before this existing step. Mutually exclusive with after_id."}, + "title": map[string]any{"type": "string", "description": "edit only: replace title; changed work loses its prior check evidence and reopens if completed."}, + "note": map[string]any{"type": "string", "description": "edit only: replace note, including an empty string to clear it."}, + "checks": planChecksSchema(), + "carry_checks_to": map[string]any{"type": "string", "description": "Required when split/supersede replaces a checked step: replacement ID receiving all original checks, pending fresh verification."}, + "steps": map[string]any{"type": "array", "minItems": 1, "items": planStepSchema(), "description": "New steps for add/split/supersede. Split requires at least two. Replacement IDs must be unique; carried checks are added to the target's declared checks."}, + }, "required": []string{"kind"}}, + }, }, "required": []string{"verb"}, } diff --git a/internal/loop/plan_checks.go b/internal/loop/plan_checks.go index e9a1abd8..d47741b5 100644 --- a/internal/loop/plan_checks.go +++ b/internal/loop/plan_checks.go @@ -35,6 +35,13 @@ func clonePlanChecks(in []PlanCheck) []PlanCheck { return out } +func cloneRevision(in *PlanRevision) *PlanRevision { + if in == nil { + return nil + } + return &PlanRevision{Reason: in.Reason, Summary: append([]string(nil), in.Summary...)} +} + func clonePlanSteps(in []PlanStep) []PlanStep { out := append([]PlanStep(nil), in...) for i := range out { @@ -121,7 +128,7 @@ func hasPlanChecks(p PlanState) bool { } func checkedPlanFits(p PlanState, maxChars int) bool { - if !hasPlanChecks(p) { + if !hasPlanChecks(p) && p.Revision == nil { return true } reserve := clonePlanState(p) @@ -144,6 +151,11 @@ func checkedPlanFits(p PlanState, maxChars int) bool { for _, step := range reserve.Steps { size += 1 + len(planStepLine(step)) } + if p.Revision != nil { + if b, err := json.Marshal(p.Revision); err == nil { + size += len(b) + len("[Plan revision: ]") + 1 + } + } return size <= maxChars } @@ -339,7 +351,7 @@ func (s *PlanStore) InvalidateChecks() { return } s.noteStatusTransitionsLocked(s.plan.Steps, working) - s.plan = &PlanState{Version: s.nextVersion(), Steps: working} + s.plan = &PlanState{Version: s.nextVersion(), Steps: working, Revision: cloneRevision(s.plan.Revision)} s.notifyLocked(false, false) } diff --git a/internal/loop/plan_checks_test.go b/internal/loop/plan_checks_test.go index 1236cf6e..3d11aac3 100644 --- a/internal/loop/plan_checks_test.go +++ b/internal/loop/plan_checks_test.go @@ -184,7 +184,7 @@ func TestPlanChecks_ArgumentsAreIsolatedAndStrict(t *testing.T) { t.Fatalf("accepted %s", raw) } } - forged := strings.Replace(checkedCreateArgs(), `"description":"Read the output"`, `"description":"Read the output","status":"passed","call_id":"forged"`, 1) + forged := strings.Replace(args, `"description":"Read the output"`, `"description":"Read the output","status":"passed","call_id":"forged"`, 1) if _, err := s.Execute(forged); err != nil { t.Fatal(err) } diff --git a/internal/loop/plan_revisions.go b/internal/loop/plan_revisions.go new file mode 100644 index 00000000..cc618551 --- /dev/null +++ b/internal/loop/plan_revisions.go @@ -0,0 +1,303 @@ +package loop + +import ( + "encoding/json" + "fmt" + "reflect" + "strings" + "unicode" +) + +const ( + maxRevisionReason = 240 + maxRevisionOps = 8 + maxRevisionSummary = 8 + maxRevisionSummaryItem = 2048 + planRevisionPrefix = "[Plan revision:" +) + +type planRevisionArgs struct { + Verb string `json:"verb"` + Reason string `json:"reason"` + Operations []planRevisionOp `json:"operations"` +} +type planRevisionOp struct { + Kind string `json:"kind"` + StepID string `json:"step_id"` + AfterID string `json:"after_id"` + BeforeID string `json:"before_id"` + Title *string `json:"title"` + Note *string `json:"note"` + Steps []planStepArg `json:"steps"` + CarryChecksTo string `json:"carry_checks_to"` + Checks []planCheckArg `json:"checks"` +} + +func preserveCheckedPlan(old PlanState, next *PlanState) error { + for _, previous := range old.Steps { + if len(previous.Checks) == 0 { + continue + } + idx := indexOfStep(next.Steps, previous.ID) + if idx < 0 { + return fmt.Errorf("plan: create cannot remove or change checked step %q; use revise", previous.ID) + } + step := &next.Steps[idx] + for _, check := range previous.Checks { + found := false + for j, c := range step.Checks { + if c.ID != check.ID { + continue + } + if c.Tool != check.Tool || c.Description != check.Description || !samePlanArguments(c.Arguments, mustCanonical(check.Arguments)) { + return fmt.Errorf("plan: create cannot remove or change checked requirement %s/%s; use revise", previous.ID, check.ID) + } + step.Checks[j].Status, step.Checks[j].CallID = check.Status, check.CallID + found = true + } + if !found { + return fmt.Errorf("plan: create cannot remove or change checked requirement %s/%s; use revise", previous.ID, check.ID) + } + } + step.Status = previous.Status + if step.Title != previous.Title { + reopenRevisedStep(step) + } + if step.Status == StepDone && !allPlanChecksPassed(*step) { + step.Status = StepInProgress + } + } + next.Revision = cloneRevision(old.Revision) + return nil +} + +func reopenRevisedStep(step *PlanStep) { + if step.Status == StepDone || step.Status == StepBlocked { + step.Status = StepInProgress + } + for i := range step.Checks { + step.Checks[i].Status = PlanCheckPending + step.Checks[i].CallID = "" + } +} + +func mustCanonical(args map[string]any) []byte { b, _ := json.Marshal(args); return b } +func (s *PlanStore) revise(raw string) (string, error) { + var args planRevisionArgs + if err := json.Unmarshal([]byte(raw), &args); err != nil { + return "", fmt.Errorf("plan: revise: %w", err) + } + if strings.TrimSpace(args.Reason) == "" || len([]rune(args.Reason)) > maxRevisionReason { + return "", fmt.Errorf("plan: revise requires a bounded reason") + } + if len(args.Operations) == 0 || len(args.Operations) > maxRevisionOps { + return "", fmt.Errorf("plan: revise requires 1..%d operations", maxRevisionOps) + } + if s.plan == nil { + return "", fmt.Errorf("plan: revise: no active plan") + } + working := clonePlanSteps(s.plan.Steps) + var summary []string + for _, op := range args.Operations { + switch op.Kind { + case "add": + steps, err := validateRevisionSteps(op.Steps) + if err != nil { + return "", err + } + pos, err := revisionPosition(working, op.AfterID, op.BeforeID) + if err != nil { + return "", err + } + working = append(append(append([]PlanStep{}, working[:pos]...), steps...), working[pos:]...) + names := make([]string, 0, len(steps)) + for _, step := range steps { + names = append(names, step.ID) + } + summary = append(summary, fmt.Sprintf("add %q after %q before %q", names, op.AfterID, op.BeforeID)) + case "edit": + idx := indexOfStep(working, op.StepID) + if idx < 0 { + return "", fmt.Errorf("plan: revise: unknown step %q", op.StepID) + } + if op.Title != nil { + title := normalizePlanText(*op.Title) + if title == "" { + return "", fmt.Errorf("plan: revise: title is required") + } + if title != working[idx].Title { + working[idx].Title = title + reopenRevisedStep(&working[idx]) + } + } + if op.Note != nil { + working[idx].Note = normalizePlanText(*op.Note) + } + if len(op.Checks) > 0 { + checks, err := validatePlanChecks(op.Checks) + if err != nil { + return "", err + } + working[idx].Checks = append(clonePlanChecks(working[idx].Checks), checks...) + if working[idx].Status == StepDone { + working[idx].Status = StepInProgress + } + summary = append(summary, fmt.Sprintf("edit checks %q", op.StepID)) + } else { + summary = append(summary, fmt.Sprintf("edit %q", op.StepID)) + } + case "move": + idx := indexOfStep(working, op.StepID) + if idx < 0 { + return "", fmt.Errorf("plan: revise: unknown step %q", op.StepID) + } + step := working[idx] + working = append(working[:idx], working[idx+1:]...) + pos, err := revisionPosition(working, op.AfterID, op.BeforeID) + if err != nil { + return "", err + } + working = append(working, PlanStep{}) + copy(working[pos+1:], working[pos:]) + working[pos] = step + summary = append(summary, fmt.Sprintf("move %q after %q before %q", op.StepID, op.AfterID, op.BeforeID)) + case "split", "supersede": + idx := indexOfStep(working, op.StepID) + if idx < 0 { + return "", fmt.Errorf("plan: revise: unknown step %q", op.StepID) + } + steps, err := validateRevisionSteps(op.Steps) + if err != nil { + return "", err + } + if op.Kind == "split" && len(steps) < 2 { + return "", fmt.Errorf("plan: split requires at least two replacement steps") + } + if len(working[idx].Checks) > 0 { + if op.CarryChecksTo == "" { + return "", fmt.Errorf("plan: revise: carry_checks_to required") + } + target := -1 + for i := range steps { + if steps[i].ID == op.CarryChecksTo { + target = i + } + } + if target < 0 { + return "", fmt.Errorf("plan: revise: carry_checks_to must name a replacement") + } + for _, existing := range steps[target].Checks { + for _, carried := range working[idx].Checks { + if existing.ID == carried.ID { + return "", fmt.Errorf("plan: duplicate carried check %q", existing.ID) + } + } + } + steps[target].Checks = append(steps[target].Checks, clonePlanChecks(working[idx].Checks)...) + steps[target].Status = StepInProgress + for j := range steps[target].Checks { + steps[target].Checks[j].Status = PlanCheckPending + steps[target].Checks[j].CallID = "" + } + } + working = append(append(append([]PlanStep{}, working[:idx]...), steps...), working[idx+1:]...) + names := make([]string, 0, len(steps)) + for _, step := range steps { + names = append(names, step.ID) + } + summary = append(summary, fmt.Sprintf("%s %q -> %q; checks to %q", op.Kind, op.StepID, names, op.CarryChecksTo)) + default: + return "", fmt.Errorf("plan: revise: unknown operation %q", op.Kind) + } + } + if err := validateRevisionFinalSteps(working, s.maxSteps); err != nil { + return "", err + } + if reflect.DeepEqual(working, s.plan.Steps) { + return s.renderLocked(), nil + } + candidate := PlanState{Version: s.nextVersion(), Steps: working, Revision: &PlanRevision{Reason: normalizePlanText(args.Reason), Summary: summary}} + if !validPlanRevision(candidate.Revision) || !checkedPlanFits(candidate, s.maxRenderChars) { + return "", fmt.Errorf("plan: revise exceeds configured limits") + } + s.noteStatusTransitionsLocked(s.plan.Steps, working) + s.plan = &candidate + s.epoch++ + s.revisionNotify = true + return s.renderLocked(), nil +} + +func validateRevisionFinalSteps(steps []PlanStep, max int) error { + if len(steps) == 0 || len(steps) > max { + return fmt.Errorf("plan: revise exceeds step cap") + } + seen := map[string]bool{} + for _, step := range steps { + if step.ID == "" || len(step.ID) > maxPlanIDChars || seen[step.ID] || strings.ContainsAny(step.ID, "[]") || strings.ContainsFunc(step.ID, func(r rune) bool { return unicode.IsSpace(r) || unicode.IsControl(r) }) { + return fmt.Errorf("plan: revise: invalid or duplicate step id %q", step.ID) + } + seen[step.ID] = true + if step.Title == "" || len(step.Title) > maxPlanTitleChars { + return fmt.Errorf("plan: revise: invalid title for %q", step.ID) + } + args := make([]planCheckArg, len(step.Checks)) + for i, c := range step.Checks { + args[i] = planCheckArg{ID: c.ID, Description: c.Description, Tool: c.Tool, Arguments: mustCanonical(c.Arguments)} + } + if _, err := validatePlanChecks(args); err != nil { + return fmt.Errorf("plan: revise: step %q: %w", step.ID, err) + } + } + return nil +} + +func validateRevisionSteps(in []planStepArg) ([]PlanStep, error) { + if len(in) == 0 { + return nil, fmt.Errorf("plan: revise: steps must not be empty") + } + out := make([]PlanStep, 0, len(in)) + for _, step := range in { + checks, err := validatePlanChecks(step.Checks) + if err != nil { + return nil, err + } + out = append(out, PlanStep{ID: strings.TrimSpace(step.ID), Title: normalizePlanText(step.Title), Note: normalizePlanText(step.Note), Status: StepPending, Checks: checks}) + } + if err := validateRevisionFinalSteps(out, extractPlanStepCap); err != nil { + return nil, err + } + return out, nil +} + +func validPlanRevision(rev *PlanRevision) bool { + if rev == nil || strings.TrimSpace(rev.Reason) == "" || len([]rune(rev.Reason)) > maxRevisionReason || len(rev.Summary) == 0 || len(rev.Summary) > maxRevisionSummary { + return false + } + for _, item := range rev.Summary { + if item == "" || len([]rune(item)) > maxRevisionSummaryItem || strings.ContainsAny(item, "\r\n") { + return false + } + } + return true +} + +// revisionPosition permits either relative anchor, including insertion before +// the first step. An omitted anchor appends at the end. +func revisionPosition(steps []PlanStep, after, before string) (int, error) { + if after != "" && before != "" { + return 0, fmt.Errorf("plan: revise: choose either after_id or before_id") + } + if after != "" { + if i := indexOfStep(steps, after); i >= 0 { + return i + 1, nil + } + return 0, fmt.Errorf("plan: revise: unknown after_id %q", after) + } + if before != "" { + if i := indexOfStep(steps, before); i >= 0 { + return i, nil + } + return 0, fmt.Errorf("plan: revise: unknown before_id %q", before) + } + return len(steps), nil +} diff --git a/internal/loop/plan_revisions_test.go b/internal/loop/plan_revisions_test.go new file mode 100644 index 00000000..8a6428b5 --- /dev/null +++ b/internal/loop/plan_revisions_test.go @@ -0,0 +1,46 @@ +package loop + +import ( + "strings" + "testing" +) + +func TestPlanRevisionPreservesChecksAndEvidence(t *testing.T) { + s := NewPlanStore(8, 4000) + if _, err := s.Execute(checkedCreateArgs()); err != nil { + t.Fatal(err) + } + epoch := s.CheckEpoch() + s.RecordCheckOutcome(epoch, "read_file", `{"path":"out","line":1}`, "call", false) + args := `{"verb":"revise","reason":"split the implementation","operations":[{"kind":"split","step_id":"s1","carry_checks_to":"s1b","steps":[{"id":"s1a","title":"prepare"},{"id":"s1b","title":"verify"}]}]}` + if _, err := s.Execute(args); err != nil { + t.Fatal(err) + } + state, _ := s.Snapshot() + if len(state.Steps) != 2 || len(state.Steps[1].Checks) != 1 || state.Steps[1].Checks[0].Status != PlanCheckPending { + t.Fatalf("bad split state: %+v", state) + } + if state.Revision == nil || state.Revision.Reason == "" { + t.Fatalf("missing revision metadata: %+v", state.Revision) + } + parsed, err := parsePlanState(renderPlan(state, 4000), 8) + if err != nil || parsed.Revision == nil { + t.Fatalf("revision did not round-trip: %v", err) + } +} + +func TestPlanRevisionRejectsCheckedCreateEscapeAtomically(t *testing.T) { + s := NewPlanStore(4, 4000) + if _, err := s.Execute(checkedCreateArgs()); err != nil { + t.Fatal(err) + } + before, _ := s.Snapshot() + _, err := s.Execute(`{"verb":"create","steps":[{"id":"s1","title":"changed","checks":[{"id":"c1","description":"different","tool":"read_file","arguments":{"path":"out"}}]}]}`) + if err == nil || !strings.Contains(err.Error(), "cannot remove or change") { + t.Fatalf("escape accepted: %v", err) + } + after, _ := s.Snapshot() + if after.Version != before.Version || after.Steps[0].Title != before.Steps[0].Title { + t.Fatalf("rejected create changed state: %+v", after) + } +} diff --git a/internal/loop/plan_test.go b/internal/loop/plan_test.go index 5549064b..67b10130 100644 --- a/internal/loop/plan_test.go +++ b/internal/loop/plan_test.go @@ -158,7 +158,7 @@ func TestPlan_Validate_BadArgsAndVerb(t *testing.T) { t.Errorf("bad JSON error = %v, want plan: parse args:", err) } if _, err := s.Execute(`{"verb":"replan"}`); err == nil || - !strings.Contains(err.Error(), `plan: unknown verb "replan" (want create/update/complete/get)`) { + !strings.Contains(err.Error(), `plan: unknown verb "replan" (want create/update/complete/revise/get)`) { t.Errorf("unknown verb error = %v", err) } if _, ok := s.Snapshot(); ok { diff --git a/internal/loop/revision_integration_test.go b/internal/loop/revision_integration_test.go new file mode 100644 index 00000000..cd79d5f2 --- /dev/null +++ b/internal/loop/revision_integration_test.go @@ -0,0 +1,99 @@ +package loop + +import ( + "context" + "errors" + "strings" + "sync/atomic" + "testing" + + "github.com/BackendStack21/odek/internal/session" + "github.com/BackendStack21/odek/internal/tool" +) + +func TestRevisionFailureCannotReuseEarlierPassingEvidence(t *testing.T) { + create := acceptanceCall("p", "plan", `{"verb":"create","steps":[{"id":"verify","title":"Inspect output","checks":[{"id":"read","description":"Read current output","tool":"read_file","arguments":{"path":"output"}}]},{"id":"other","title":"Other work"}]}`) + read := acceptanceCall("read", "read_file", `{"path":"output"}`) + revision := acceptanceCall("rev", "plan", `{"verb":"revise","reason":"Prioritize other work","operations":[{"kind":"move","step_id":"verify","after_id":"other"}]}`) + e, _ := acceptanceEngine(t, [][]session.ToolCall{{create}, {read}, {read, revision}, {acceptanceCall("done", "plan", `{"verb":"complete","step_id":"verify"}`)}}, false) + var reads atomic.Int32 + e.registry = tool.NewRegistry([]tool.Tool{NewPlanTool(e.planStore), &contractTool{name: "read_file", run: func(string) (string, error) { + if reads.Add(1) == 1 { + return "old output", nil + } + return "", errors.New("output missing") + }}}) + answer, history, err := e.RunWithMessages(context.Background(), []session.Message{{Role: "user", Content: "Inspect output and adjust plan if needed."}}) + if err != nil { + t.Fatal(err) + } + state, _ := e.planStore.Snapshot() + if state.Steps[0].ID != "other" { + t.Fatal("revision did not execute") + } + if state.Steps[1].Status == StepDone || len(e.pendingPlanChecks()) != 1 { + t.Fatalf("failed check retained earlier evidence across revision: %+v", state) + } + if !strings.Contains(answer, "[odek verification incomplete:") || history[len(history)-1].Content != answer { + t.Fatal("missing persisted verification warning") + } +} + +func TestRevisionCannotClaimChecksFromItsOwnBatch(t *testing.T) { + create := acceptanceCall("p", "plan", `{"verb":"create","steps":[{"id":"s","title":"Inspect output"}]}`) + revise := acceptanceCall("r", "plan", `{"verb":"revise","reason":"Require verification","operations":[{"kind":"edit","step_id":"s","checks":[{"id":"inspect","description":"Read output","tool":"read_file","arguments":{"path":"output"}}]}]}`) + read := acceptanceCall("read", "read_file", `{"path":"output"}`) + for _, batch := range [][]session.ToolCall{{read, revise}, {revise, read}} { + e, _ := acceptanceEngine(t, [][]session.ToolCall{{create}, batch, {acceptanceCall("done", "plan", `{"verb":"complete","step_id":"s"}`)}}, false) + answer, err := e.Run(context.Background(), "Verify output") + if err != nil { + t.Fatal(err) + } + if len(e.pendingPlanChecks()) != 1 || !strings.Contains(answer, "[odek verification incomplete:") { + t.Fatalf("same-batch call certified newly revised check: %s", answer) + } + } +} + +func TestRevisionTriggersSkillRematchWithoutNewPlan(t *testing.T) { + e := &Engine{} + s := NewPlanStore(12, 4000) + e.SetPlanStore(s) + mustExecute(t, s, `{"verb":"create","steps":[{"id":"s","title":"Inspect"}]}`) + e.skillRematchPending.Store(false) + mustExecute(t, s, `{"verb":"revise","reason":"Found database dependency","operations":[{"kind":"add","steps":[{"id":"db","title":"Review database schema"}]}]}`) + if !e.skillRematchPending.Load() { + t.Fatal("revision did not request skill rematch") + } + if !strings.Contains(e.planTitleQuery(), "database") { + t.Fatal("new title absent from skill query") + } +} + +func TestRevisionSurvivesWrappedTranscriptAndCompletion(t *testing.T) { + e, _ := acceptanceEngine(t, [][]session.ToolCall{ + {acceptanceCall("p", "plan", `{"verb":"create","steps":[{"id":"s","title":"Inspect"}]}`)}, + {acceptanceCall("r", "plan", `{"verb":"revise","reason":"New evidence changes the approach","operations":[{"kind":"edit","step_id":"s","title":"Inspect configuration"}]}`)}, + {acceptanceCall("d", "plan", `{"verb":"complete","step_id":"s"}`)}, + }, false) + e.SetUntrustedWrapper(func(source, content string) string { + return "\n" + content + "\n" + }) + _, history, err := e.RunWithMessages(context.Background(), []session.Message{{Role: "user", Content: "Inspect and adjust the plan."}}) + if err != nil { + t.Fatal(err) + } + parsed, ok := ExtractPlan(history) + if !ok || parsed.Revision == nil || parsed.Revision.Reason != "New evidence changes the approach" || len(parsed.Steps) != 1 || parsed.Steps[0].Status != StepDone { + t.Fatalf("lost completed revised plan in wrapped transcript: %+v", parsed) + } + resumed, _ := acceptanceEngine(t, nil, false) + _, _, err = resumed.RunWithMessages(context.Background(), append(history, session.Message{Role: "user", Content: "Report the plan."})) + if err != nil { + t.Fatal(err) + } + state, ok := resumed.planStore.Snapshot() + if !ok || state.Revision == nil || len(state.Steps) != 1 { + t.Fatalf("lost revision on resume: %+v", state) + } +} diff --git a/internal/loop/revision_safety_test.go b/internal/loop/revision_safety_test.go new file mode 100644 index 00000000..68cab5f7 --- /dev/null +++ b/internal/loop/revision_safety_test.go @@ -0,0 +1,139 @@ +package loop + +import ( + "encoding/json" + "reflect" + "strings" + "testing" +) + +func TestRevisionMalformedChangesAreAtomic(t *testing.T) { + for name, op := range map[string]string{ + "duplicate check identity": `{"kind":"edit","step_id":"s1","checks":[{"id":"c1","description":"Different requirement","tool":"read_file","arguments":{"path":"elsewhere"}}]}`, + "empty addition": `{"kind":"add","steps":[]}`, + "empty replacement": `{"kind":"supersede","step_id":"s1","steps":[]}`, + "long title": `{"kind":"edit","step_id":"s1","title":"` + strings.Repeat("x", 201) + `"}`, + "invalid id": `{"kind":"add","steps":[{"id":"bad\nidentifier","title":"New work"}]}`, + "empty normalized title": `{"kind":"edit","step_id":"s1","title":" "}`, + } { + t.Run(name, func(t *testing.T) { + s := NewPlanStore(12, 4000) + mustExecute(t, s, checkedCreateArgs()) + before, _ := s.Snapshot() + epoch := s.CheckEpoch() + if _, err := s.Execute(`{"verb":"revise","reason":"New finding","operations":[{"kind":"add","steps":[{"id":"new","title":"Valid addition"}]},` + op + `]}`); err == nil { + t.Fatal("accepted malformed revision") + } + after, _ := s.Snapshot() + if !reflect.DeepEqual(before, after) || epoch != s.CheckEpoch() { + t.Fatal("rejected revision changed state or epoch") + } + }) + } +} + +func TestRevisionTransferKeepsNewAndExistingRequirements(t *testing.T) { + s := NewPlanStore(12, 4000) + mustExecute(t, s, checkedCreateArgs()) + mustExecute(t, s, `{"verb":"revise","reason":"Add output verification","operations":[{"kind":"supersede","step_id":"s1","carry_checks_to":"replacement","steps":[{"id":"replacement","title":"Verify both files","checks":[{"id":"extra","description":"Inspect another file","tool":"read_file","arguments":{"path":"other"}}]}]}]}`) + state, _ := s.Snapshot() + if len(state.Steps) != 1 || len(state.Steps[0].Checks) != 2 || len(s.PendingChecks()) != 2 { + t.Fatalf("transfer lost newly declared or existing criteria: %+v", state) + } +} + +func TestCreateCannotReuseEvidenceForChangedWork(t *testing.T) { + s := NewPlanStore(12, 4000) + mustExecute(t, s, checkedCreateArgs()) + s.RecordCheckOutcome(s.CheckEpoch(), "read_file", `{"path":"out","line":1}`, "passed", false) + mustExecute(t, s, `{"verb":"complete","step_id":"s1"}`) + mustExecute(t, s, checkedCreateArgs()) + state, _ := s.Snapshot() + if state.Steps[0].Status != StepDone || len(s.PendingChecks()) != 0 { + t.Fatal("identical create lost fulfilled work") + } + mustExecute(t, s, strings.Replace(checkedCreateArgs(), `"title":"verify"`, `"title":"Verify a different approach"`, 1)) + state, _ = s.Snapshot() + if state.Steps[0].Status == StepDone || len(s.PendingChecks()) != 1 { + t.Fatal("changed work reused old evidence") + } +} + +func TestRevisionPersistedMetadataIsDetached(t *testing.T) { + s := NewPlanStore(12, 4000) + mustExecute(t, s, checkedCreateArgs()) + mustExecute(t, s, `{"verb":"revise","reason":"Inspect configuration first","operations":[{"kind":"add","steps":[{"id":"config","title":"Inspect configuration"}]}]}`) + before, _ := s.Snapshot() + b, _ := json.Marshal(before) + before.Revision.Reason = "mutated" + if len(before.Revision.Summary) > 0 { + before.Revision.Summary[0] = "mutated" + } + after, _ := s.Snapshot() + a, _ := json.Marshal(after) + if string(a) != string(b) { + t.Fatal("snapshot aliases revision metadata") + } + mustExecute(t, s, `{"verb":"update","updates":[{"id":"config","status":"in_progress"}]}`) + updated, _ := s.Snapshot() + if updated.Revision == nil { + t.Fatal("status update erased revision metadata") + } +} + +func TestRevisionNotificationDoesNotLeakIntoCheckOutcomes(t *testing.T) { + s := NewPlanStore(12, 4000) + var changes []PlanChange + s.SetOnChange(func(ch PlanChange) { changes = append(changes, ch) }) + mustExecute(t, s, checkedCreateArgs()) + mustExecute(t, s, `{"verb":"revise","reason":"Add preparation","operations":[{"kind":"add","steps":[{"id":"prep","title":"Prepare"}]}]}`) + if !changes[len(changes)-1].Revised { + t.Fatal("missing revision notification") + } + s.RecordCheckOutcome(s.CheckEpoch(), "read_file", `{"path":"out","line":1}`, "read", false) + if changes[len(changes)-1].Revised { + t.Fatal("evidence outcome incorrectly announced revision") + } +} + +func TestRevisionNoOpPreservesEvidenceAndVersion(t *testing.T) { + s := NewPlanStore(12, 4000) + mustExecute(t, s, checkedCreateArgs()) + s.RecordCheckOutcome(s.CheckEpoch(), "read_file", `{"path":"out","line":1}`, "pass", false) + mustExecute(t, s, `{"verb":"complete","step_id":"s1"}`) + before, _ := s.Snapshot() + epoch := s.CheckEpoch() + mustExecute(t, s, `{"verb":"revise","reason":"Already arranged","operations":[{"kind":"move","step_id":"s1"},{"kind":"edit","step_id":"s1","title":"verify"}]}`) + after, _ := s.Snapshot() + if !reflect.DeepEqual(before, after) || s.CheckEpoch() != epoch { + t.Fatal("no-op revision changed evidence or version") + } +} + +func TestRevisionOverflowRejectedWithoutLosingCompletedSteps(t *testing.T) { + s := NewPlanStore(12, 900) + mustExecute(t, s, `{"verb":"create","steps":[{"id":"s","title":"Work"}]}`) + mustExecute(t, s, `{"verb":"complete","step_id":"s"}`) + before, _ := s.Snapshot() + raw := `{"verb":"revise","reason":"Expand details","operations":[{"kind":"edit","step_id":"s","note":"` + strings.Repeat("x", 1000) + `"}]}` + if _, err := s.Execute(raw); err == nil { + t.Fatal("accepted revision that loses completed work on rendering") + } + after, _ := s.Snapshot() + if !reflect.DeepEqual(before, after) { + t.Fatal("overflow rejection mutated plan") + } +} + +func TestRevisionCanMoveToFrontAndInsertBefore(t *testing.T) { + s := NewPlanStore(12, 4000) + mustExecute(t, s, `{"verb":"create","steps":[{"id":"a","title":"A"},{"id":"b","title":"B"}]}`) + mustExecute(t, s, `{"verb":"revise","reason":"Prioritize new prerequisite","operations":[{"kind":"move","step_id":"b","before_id":"a"},{"kind":"add","before_id":"b","steps":[{"id":"prep","title":"Prepare"}]}]}`) + state, _ := s.Snapshot() + if state.Steps[0].ID != "prep" || state.Steps[1].ID != "b" || state.Steps[2].ID != "a" { + t.Fatalf("wrong order: %+v", state.Steps) + } + if _, err := s.Execute(`{"verb":"revise","reason":"Invalid anchors","operations":[{"kind":"move","step_id":"prep","before_id":"b","after_id":"a"}]}`); err == nil { + t.Fatal("accepted ambiguous anchors") + } +} From b549d29d1e44d80b8771b9f342818e734117c266 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso <4096860+jkyberneees@users.noreply.github.com> Date: Fri, 18 Sep 2026 09:40:50 +0200 Subject: [PATCH 4/4] fix: resolve release CodeQL findings in planning and evals --- internal/eval/eval.go | 28 ++++++++++++++++++++++++---- internal/eval/eval_test.go | 33 +++++++++++++++++++++++++++++++++ internal/loop/plan.go | 3 +-- 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/internal/eval/eval.go b/internal/eval/eval.go index 13944557..0a9caa76 100644 --- a/internal/eval/eval.go +++ b/internal/eval/eval.go @@ -283,12 +283,32 @@ func (t fixtureTool) Call(raw string) (string, error) { } func toolCall(name, id, args string) string { - b, _ := json.Marshal(args) - return fmt.Sprintf(`{"choices":[{"message":{"content":"","tool_calls":[{"id":%q,"type":"function","function":{"name":%q,"arguments":%s}}]},"finish_reason":"tool_calls"}],"usage":{"prompt_tokens":7,"completion_tokens":3}}`, id, name, b) + body := map[string]any{ + "choices": []any{map[string]any{ + "message": map[string]any{ + "content": "", + "tool_calls": []any{map[string]any{ + "id": id, "type": "function", + "function": map[string]any{"name": name, "arguments": args}, + }}, + }, + "finish_reason": "tool_calls", + }}, + "usage": map[string]int{"prompt_tokens": 7, "completion_tokens": 3}, + } + b, _ := json.Marshal(body) + return string(b) } func final(text string) string { - b, _ := json.Marshal(text) - return fmt.Sprintf(`{"choices":[{"message":{"content":%s},"finish_reason":"stop"}],"usage":{"prompt_tokens":7,"completion_tokens":3}}`, b) + body := map[string]any{ + "choices": []any{map[string]any{ + "message": map[string]any{"content": text}, + "finish_reason": "stop", + }}, + "usage": map[string]int{"prompt_tokens": 7, "completion_tokens": 3}, + } + b, _ := json.Marshal(body) + return string(b) } func baseFixture() *Fixture { return &Fixture{Values: map[string]string{}} } diff --git a/internal/eval/eval_test.go b/internal/eval/eval_test.go index 796c9128..6154488f 100644 --- a/internal/eval/eval_test.go +++ b/internal/eval/eval_test.go @@ -2,6 +2,7 @@ package eval import ( "context" + "encoding/json" "testing" "github.com/BackendStack21/odek/internal/llmclient" @@ -49,3 +50,35 @@ func TestHarnessRejectsMissingOracleOrClient(t *testing.T) { t.Fatalf("accepted no client: %+v", emptyClient) } } + +func TestScriptedResponsesEscapeJSONStrings(t *testing.T) { + value := "quoted\"\\\n\x01" + args := `{"value":"quoted\""}` + var response struct { + Choices []struct { + Message struct { + Content string + ToolCalls []struct { + ID string + Function struct { + Name string + Arguments string + } + } `json:"tool_calls"` + } + } + } + if err := json.Unmarshal([]byte(toolCall(value, value, args)), &response); err != nil { + t.Fatal(err) + } + call := response.Choices[0].Message.ToolCalls[0] + if call.ID != value || call.Function.Name != value || call.Function.Arguments != args { + t.Fatal("tool fixture strings changed during encoding") + } + if err := json.Unmarshal([]byte(final(value)), &response); err != nil { + t.Fatal(err) + } + if response.Choices[0].Message.Content != value { + t.Fatal("final fixture text changed during encoding") + } +} diff --git a/internal/loop/plan.go b/internal/loop/plan.go index d44defa2..5777e6c4 100644 --- a/internal/loop/plan.go +++ b/internal/loop/plan.go @@ -683,8 +683,7 @@ func renderPlan(p PlanState, maxChars int) string { lines = append(lines, planStepLine(st)) } build := func(omit map[int]bool, omitted int) string { - parts := make([]string, 0, len(lines)+2) - parts = append(parts, header) + parts := []string{header} if p.Revision != nil { if b, err := json.Marshal(p.Revision); err == nil { parts = append(parts, "[Plan revision: "+string(b)+"]")