diff --git a/Makefile b/Makefile index add9a69..15ddd19 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/README.md b/README.md index fc222fb..42cf85d 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) | 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 | | [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/cmd/odek-eval/main.go b/cmd/odek-eval/main.go new file mode 100644 index 0000000..cac6e44 --- /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/CONFIG.md b/docs/CONFIG.md index a34b21e..6934bb8 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 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 +`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 2e08f12..3d2af95 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 +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. +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 new file mode 100644 index 0000000..3c09632 --- /dev/null +++ b/docs/EVALS.md @@ -0,0 +1,56 @@ +# 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, 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. + +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. + +## 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 eleven-case baseline includes a deliberate +unguarded false-success control: all scenarios pass while +`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 +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 f918a6d..8e4f6a4 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,75 @@ 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 + +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 +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 @@ -126,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 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: @@ -134,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 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 unrevised unchecked plans, when the render exceeds `max_render_chars`, the oldest `done` steps are dropped first behind an explicit marker: ``` @@ -182,63 +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 are steering aids, not contracts.", "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": { + "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"] + "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" } } ``` @@ -259,6 +572,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: @@ -301,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; overflow drops oldest done steps first behind `[+N done steps omitted]` | +| `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. @@ -387,6 +702,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, 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. - GET-only by contract: a non-GET request to `…/plan` cannot fall through to the base-session mutators (POST would otherwise rename the session @@ -436,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`, @@ -545,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 @@ -562,8 +888,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- @@ -583,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. @@ -600,7 +927,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. `revise` changes the approach incrementally while keeping acceptance checks. +The prompt distinguishes advisory steps from checked completion. diff --git a/docs/WEBUI.md b/docs/WEBUI.md index b87031f..39700bc 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 diff --git a/internal/eval/eval.go b/internal/eval/eval.go new file mode 100644 index 0000000..0a9caa7 --- /dev/null +++ b/internal/eval/eval.go @@ -0,0 +1,424 @@ +// 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 + Plan *loop.PlanState + 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) + 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() + 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 { + 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 { + 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{}} } + +// 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() + 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 { + 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} + }}, + {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} + }}, + } +} + +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 0000000..6154488 --- /dev/null +++ b/internal/eval/eval_test.go @@ -0,0 +1,84 @@ +package eval + +import ( + "context" + "encoding/json" + "testing" + + "github.com/BackendStack21/odek/internal/llmclient" +) + +func TestDefaultScenariosAreIndependentAndBounded(t *testing.T) { + r := Run(context.Background(), Scenarios()) + 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) + } + 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 != 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" { + 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) + } +} + +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/completion_checks.go b/internal/loop/completion_checks.go new file mode 100644 index 0000000..7fc3cb2 --- /dev/null +++ b/internal/loop/completion_checks.go @@ -0,0 +1,65 @@ +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.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() + } + 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 { + // 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 0000000..9b2ef52 --- /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 724a842..b3ccf1b 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{ @@ -2034,8 +2036,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 +2575,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 +2917,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 +2969,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 +3273,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 +3306,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 +3467,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. @@ -3732,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 { @@ -3749,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) { @@ -3814,8 +3833,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 +4282,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 +4299,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 d12f10d..5777e6c 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,17 +50,41 @@ 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 // 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 @@ -77,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 @@ -110,6 +135,8 @@ 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 + revisionNotify bool } // NewPlanStore creates a store with the given resolved caps. Degenerate @@ -149,14 +176,40 @@ 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 + 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) + } return st } @@ -165,6 +218,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 +244,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 { @@ -226,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) @@ -233,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 @@ -245,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 } @@ -259,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 { @@ -273,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: @@ -315,12 +383,26 @@ 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 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) + } + 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 +414,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 +428,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 +451,12 @@ func (s *PlanStore) update(updates []planUpdateArg) (string, error) { if s.plan != nil { old = s.plan.Steps } + 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) + } s.noteStatusTransitionsLocked(old, working) - s.plan = &PlanState{Version: s.nextVersion(), Steps: working} + s.plan = &candidate return s.renderLocked(), nil } @@ -383,10 +472,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, 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) + } s.noteStatusTransitionsLocked(s.plan.Steps, working) - s.plan = &PlanState{Version: s.nextVersion(), Steps: working} + s.plan = &candidate return s.renderLocked(), nil } @@ -557,6 +653,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 { @@ -578,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)) @@ -586,8 +683,12 @@ 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)+"]") + } + } if omitted > 0 { parts = append(parts, fmt.Sprintf(planOverflowMarker, omitted)) } @@ -637,11 +738,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) && p.Revision == nil { 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 +755,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 +785,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 { @@ -687,6 +794,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 @@ -704,6 +812,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") @@ -719,7 +836,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 +858,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, Revision: revision} + 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 +892,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 +987,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 +1032,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 +1111,28 @@ 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. 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 { @@ -983,21 +1140,12 @@ func (t *PlanTool) Schema() 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"}, - }, - "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", @@ -1016,6 +1164,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 new file mode 100644 index 0000000..d47741b --- /dev/null +++ b/internal/loop/plan_checks.go @@ -0,0 +1,378 @@ +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 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 { + 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) && p.Revision == nil { + 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)) + } + if p.Revision != nil { + if b, err := json.Marshal(p.Revision); err == nil { + size += len(b) + len("[Plan revision: ]") + 1 + } + } + 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, Revision: cloneRevision(s.plan.Revision)} + 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 0000000..3d11aac --- /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(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) + } + 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 06f8153..4e72a9a 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_revisions.go b/internal/loop/plan_revisions.go new file mode 100644 index 0000000..cc61855 --- /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 0000000..8a6428b --- /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 3d50827..67b1013 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) } } @@ -157,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 { @@ -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]) } } diff --git a/internal/loop/revision_integration_test.go b/internal/loop/revision_integration_test.go new file mode 100644 index 0000000..cd79d5f --- /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 0000000..68cab5f --- /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") + } +}