diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 7a29447..b06edeb 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -36,6 +36,7 @@ uv run yada --task-file issue.md --workspace /path/to/repository | `--base-url URL` | DeepSeek-compatible API base URL. | `DEEPSEEK_BASE_URL` or `https://api.deepseek.com` | | `--reasoning-effort high\|max` | Thinking effort. | `max` | | `--thinking` / `--no-thinking` | Enable or disable thinking. | Enabled | +| `--editing-strategy patch-only\|replace-first` | Freeze the run-level editing policy and model-facing edit tools. | `replace-first` | | `--max-steps N` | Maximum model turns. | `30` | | `--max-output-tokens N` | Maximum tokens requested per completion. | `16384` | | `--api-timeout SECONDS` | Timeout for one model request. | `300` | @@ -289,12 +290,28 @@ container; Yada's automatic Agent command container applies only to the native | `--max-steps N` | Model-turn budget. | `30` | | `--wall-time SECONDS` | Comparable wall-time budget. | `1800` | | `--max-output-tokens N` | Per-completion token limit. | `16384` | +| `--editing-strategy patch-only\|replace-first` | Native Yada editing policy. | `replace-first` | The native agent also accepts the model, thinking, timeout, command-policy, and trace-level options documented for `yada`. A deployment-level supervisor should enforce a hard wall-time limit for an in-process native agent. Use the same task, base commit, model budget, network policy, and grader when comparing agents. +For a controlled editing-policy comparison, run the same case once with each +strategy while holding the remaining options constant: + +```bash +uv run yada eval --case CASE --agent yada --yes \ + --editing-strategy patch-only --output results/patch-only.json + +uv run yada eval --case CASE --agent yada --yes \ + --editing-strategy replace-first --output results/replace-first.json +``` + +The result's `agent_run.details` records `editing_strategy` and +`editing_metrics`; the benchmark grader remains authoritative for resolved-task +status. + Evaluation exits with `0` for `resolved`, `1` for `unresolved`, and `2` for errors or non-verdict outcomes such as skipped grading. diff --git a/docs/configuration.md b/docs/configuration.md index ad7855b..ee3d51d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -70,6 +70,30 @@ When thinking is enabled, Yada retains `reasoning_content` across tool-calling turns as required by DeepSeek. In non-thinking mode, Yada uses ordinary automatic tool selection. +## Editing strategy + +Editing strategy is frozen for the complete run: + +| Strategy | Editing tools shown to the model | Policy | +| --- | --- | --- | +| `patch-only` | `apply_patch` | Express every edit as a checked unified diff. | +| `replace-first` | `replace_text`, `apply_patch` | Prefer exact replacement for localized edits and use patch for unsuitable operations. | + +`replace-first` is the default. Select `patch-only` explicitly when every edit must +use a checked unified diff: + +```bash +uv run yada "Fix the localized parser bug" \ + --workspace /path/to/repository \ + --editing-strategy patch-only +``` + +Both strategies allow at most one editing tool call per Assistant turn. This +prevents a replacement and a precomputed patch from acting as an opaque +same-turn fallback. See the +[editing strategy design](dev/editing-strategy.md) for routing and recovery +rules. + ## Command execution policy Repository commands are independently validated and then handled by one of diff --git a/docs/dev/architecture.md b/docs/dev/architecture.md index f656d6c..78ba3c1 100644 --- a/docs/dev/architecture.md +++ b/docs/dev/architecture.md @@ -70,7 +70,7 @@ stable system prompt + tool schemas ↓ append assistant + tool observations ↓ - repeat or verified finish + repeat or verified finish_task ``` `Agent` in `agents/default.py` owns the append-only message list, step limit, @@ -85,8 +85,8 @@ requests satisfy DeepSeek's thinking/tool-call contract. The current `Planner` is deterministic; it is not another model call. It builds the initial prompt, interprets assistant output, recovers from text-only turns, -and rejects unsafe batches such as `finish` mixed with another tool. It has no -workspace access. +and rejects unsafe batches such as `finish_task` mixed with another tool or more than +one editing operation in one turn. It has no workspace access. The `Executor` parses tool arguments, preserves model-provided call order, invokes `ToolRunner`, and records correlated `tool_call` and `tool_result` @@ -104,10 +104,12 @@ Yada exposes six tools: | `replace_text` | Apply exact unique replacements to existing UTF-8 files. | | `apply_patch` | Validate and apply a Git-style unified diff. | | `run_command` | Run an approved argv array and return bounded structured output. | -| `finish` | End only after verification of the latest revision. | +| `finish_task` | End only after verification of the latest revision. | `ToolRunner` composes shared workspace, approval, output-limit, and verification -state. Handlers remain otherwise stateless. File paths are resolved through the +state. At run start it freezes either the `patch-only` interface or the +`replace-first` interface; the implementation still contains both editing tools. +Handlers remain otherwise stateless. File paths are resolved through the workspace boundary, which rejects absolute paths, `..` escapes, symlink escapes, and access to `.git` or `.yada` internals. @@ -147,7 +149,7 @@ secret-looking environment variables and return stdout, stderr, exit code, timeout, and duration through the same tool result. The model labels a command as `inspect`, `test`, or `build`. Only a successful -`test` or `build` verifies the current workspace revision. `finish` rejects the +`test` or `build` verifies the current workspace revision. `finish_task` rejects the run when: - no verification succeeded; @@ -223,16 +225,17 @@ load, workspace, grading, cache, and artifact sequence. ## Core invariants -1. System prompt and tool schemas stay stable during a run. -2. Conversation messages are append-only. -3. DeepSeek reasoning is preserved across tool-call turns. -4. File mutation occurs only through a checked unified diff. -5. Existing patch targets must match their last-read SHA-256. -6. Every patch invalidates previous verification. -7. `finish` requires verification of the latest revision. -8. Trace events are append-only and self-correlating. -9. Benchmark grading happens outside the agent's tool boundary. -10. Hidden grading inputs never enter the official Agent command container. +1. Editing strategy, system prompt, and tool schemas stay stable during a run. +2. At most one editing operation executes from one Assistant turn. +3. Conversation messages are append-only. +4. DeepSeek reasoning is preserved across tool-call turns. +5. File mutation occurs only through a checked unified diff. +6. Existing patch targets must match their last-read SHA-256. +7. Every patch invalidates previous verification. +8. `finish_task` requires verification of the latest revision. +9. Trace events are append-only and self-correlating. +10. Benchmark grading happens outside the agent's tool boundary. +11. Hidden grading inputs never enter the official Agent command container. ## Security boundary diff --git a/docs/dev/debugging.md b/docs/dev/debugging.md index 7542243..fe085d5 100644 --- a/docs/dev/debugging.md +++ b/docs/dev/debugging.md @@ -39,7 +39,7 @@ uv run --frozen pytest tests/ -v ``` The suite includes a fully offline fake-model path through read → patch → test → -finish. Prefer fake clients and temporary Git repositories for agent-loop tests; +finish_task. Prefer fake clients and temporary Git repositories for agent-loop tests; unit tests must not require a DeepSeek key or network access. ## Capture a useful trace @@ -156,7 +156,7 @@ Every schema v2 record contains `schema_version`, `run_id`, `sequence`, UTC | Event | Meaning | Main correlation | | --- | --- | --- | -| `run_start` | Task, workspace, model config, trace level, and provenance. | `run_id` | +| `run_start` | Task, workspace, model config, editing strategy, frozen tool names, trace level, and provenance. | `run_id` | | `model_request` | Attempted model turn; debug adds `payload`. | `step`, `request_id` | | `assistant` | Model message, usage, metadata, finish reason, and latency. | `step`, `request_id` | | `model_error` | Model request exception instead of an assistant response. | `step`, `request_id` | @@ -245,7 +245,7 @@ SWE-bench score. Use the official Docker grader for published results; see the - **`DEEPSEEK_API_KEY is not set`**: export the key in the shell launching Yada. - **No request payload in a trace**: rerun with `--trace-level debug`. -- **`finish` rejected**: run a successful `test` or `build` after the latest patch. +- **`finish_task` rejected**: run a successful `test` or `build` after the latest patch. - **Tool reports `ok` but tests failed**: inspect the command `exit_code`. - **No `run_end`**: the process was interrupted or raised outside a graceful path. - **Step limit reached**: inspect repeated reminders, failed tools, context growth, diff --git a/docs/dev/editing-strategy.md b/docs/dev/editing-strategy.md new file mode 100644 index 0000000..7e0baf7 --- /dev/null +++ b/docs/dev/editing-strategy.md @@ -0,0 +1,355 @@ +# Editing Strategies + +## Status + +This document defines the minimal implementation for +[Issue #10: replace-first routing with apply_patch fallback](https://github.com/GenTang/Yada/issues/10). + +Issues #8 and #9 make apply_patch and replace_text independently safe. Issue +#10 adds the thin coordination layer between them: + +- a stable run-level strategy; +- explicit routing and recovery instructions; +- one deterministic host rule: one editing operation per Agent turn; +- trace and evaluation data that make the strategies comparable. + +It does not add a recovery state machine, automatic recovery reads, per-error +retry budgets, or hidden tool conversion. + +## 1. Responsibilities + +### 1.1 Editing tools + +The tools remain responsible for their existing contracts: + +- exact SHA validation; +- path and target validation; +- fail-closed matching or patch application; +- transactional multi-file mutation; +- structured, bounded errors; +- revision and verification bookkeeping. + +Neither tool owns routing policy. A failed replace_text call never constructs or +executes an apply_patch fallback. + +### 1.2 Model + +The model is responsible for: + +- deciding whether the intended edit is localized or structural; +- selecting an editing tool according to the strategy prompt; +- observing structured errors; +- re-reading when the recovery policy requires current content; +- proposing a corrected replacement or patch in a later turn; +- verifying the final revision before calling finish_task. + +### 1.3 Yada host + +Yada is responsible for: + +- selecting and freezing the run strategy; +- freezing the strategy-specific prompt and tool schemas; +- rejecting an Assistant response containing more than one editing operation; +- returning every tool failure to the model through the existing message loop; +- recording strategy, tool selection, results, and errors; +- stopping the run at the existing max_steps boundary. + +## 2. Run-level strategies + +Yada supports: + +| Strategy | Exposed editing tools | Prompt policy | +| --- | --- | --- | +| patch-only | apply_patch | Express every workspace edit as a checked unified diff. | +| replace-first | replace_text and apply_patch | Prefer exact replacement for suitable localized edits and patch otherwise. | + +replace-first is the default. patch-only remains available as an explicit baseline +and for runs that require unified-diff-only editing. + +The strategy is selected once through: + +~~~text +--editing-strategy patch-only +--editing-strategy replace-first +~~~ + +The ToolRunner builds its handlers and schemas once. The Planner builds its +system prompt once from the same strategy. Neither changes during the run. + +The run_start trace records: + +~~~json +{ + "editing_strategy": "replace-first", + "tool_names": [ + "search_code", + "read_file", + "apply_patch", + "replace_text", + "run_command", + "finish_task" + ] +} +~~~ + +## 3. Replace-first routing + +Use replace_text when all of these are true: + +- the target is an existing regular UTF-8 text file; +- the intended change is localized; +- current source text supplies an exact, unique anchor; +- the anchor is reasonably bounded. + +Use apply_patch directly when: + +- creating or deleting a file; +- applying a large structural rewrite; +- the exact anchor would reproduce an impractically large source block; +- replace_text does not support the target operation. + +Issue #10 also names renames as a patch case. The current Issue #8 patch +contract rejects rename metadata, so routing may select apply_patch but the +operation remains unsupported until rename support is added separately. + +The semantic predicates such as localized are model judgments. Yada does not +add an AST router or second model. Tool validation remains the deterministic +safety boundary. + +## 4. Recovery policy + +Fallback means a deliberate apply_patch call in a later Agent turn after the +model has observed a replace_text failure. It does not mean: + +- replace_text internally calling apply_patch as a strategy decision; +- Yada converting failed replacement arguments into a patch; +- the model submitting replace_text and apply_patch in the same response. + +The detailed recovery matrix is the design and test reference: + +| Error code | Required model response | +| --- | --- | +| stale_hash | Re-read the affected file before retrying. Do not fall back automatically. | +| no_match | Re-read relevant content, then use current exact text or deliberately generate a patch. | +| ambiguous_match | Read a narrower range or enlarge the exact anchor until it is unique. | +| invalid_edit | Correct the arguments in a later turn. | +| unsupported_target | Use apply_patch only when its contract supports the requested operation. | +| invalid_patch | Correct or regenerate the patch. | +| patch_context_mismatch | Re-read affected files and regenerate the patch. | +| apply_failed | Preserve and act on the diagnostic evidence; Yada performs no fallback. | + +invalid_patch comes from Issue #8, on which Issue #10 depends. + +The system prompt tells the model to follow the structured recovery instruction +returned with the actual error, refresh stale content when required, and retry or +switch tools only in a later turn. Keeping the full table here avoids paying for and +repeatedly presenting the same verbose matrix on every model turn. + +The host does not enforce a multi-stage recovery protocol. If the model ignores +the prompt, the resulting call is handled by the ordinary tool contract and +remains visible in the trace. Repeated non-progress ends at max_steps. + +## 5. One editing operation per turn + +Define: + +~~~python +EDITING_TOOLS = {"replace_text", "apply_patch"} +~~~ + +Before execution, the Planner counts editing calls in the Assistant response. +If the count is greater than one, Yada rejects the complete batch: + +~~~json +{ + "ok": false, + "error_code": "multiple_edit_operations", + "error": "Only one editing operation is allowed per Agent turn." +} +~~~ + +Every proposed call receives a rejection result so the provider conversation +contains no unmatched tool_call. + +This rule matters because all calls in one Assistant response are generated +before any result is visible to the model. A response containing: + +~~~text +replace_text +apply_patch +~~~ + +has no conditional if-replace-fails semantics. The patch is an unconditional, +precomputed second edit, not an evidence-based fallback. + +Both editing tools already support transactional multi-file requests. One +editing operation per turn does not mean one file or one changed location per +turn. + +## 6. End-to-end algorithm + +~~~text +initialize run + select editing strategy + build strategy prompt and tool interface once + record strategy and tool names + +for each model turn up to max_steps + request completion with the frozen prompt and schemas + parse tool calls + + if more than one editing call is present + reject the complete batch without side effects + append one structured result per call + continue to the next model turn + + execute the accepted calls in their existing order + append every tool result + + if an edit failed + the next model turn observes its structured error + the model follows the prompt recovery matrix + + if verified finish_task succeeds + end successfully + +end unfinished when max_steps is exhausted +~~~ + +There is no same-call fallback and no host-generated mutation. + +## 7. Flowcharts + +### 7.1 Overall control flow + +~~~mermaid +flowchart TD + A["Start run"] --> B{"Editing strategy"} + B -->|"patch-only"| C["Freeze patch-only prompt and schemas"] + B -->|"replace-first"| D["Freeze replace-first prompt and schemas"] + C --> E["Request model turn"] + D --> E + + E --> F["Planner parses tool calls"] + F --> G{"More than one editing call?"} + G -->|"Yes"| H["Reject complete batch without side effects"] + H --> I["Return structured results to next model turn"] + G -->|"No"| J["Execute accepted calls in order"] + + J --> K{"Edit result"} + K -->|"Success"| L["Invalidate old verification and continue"] + K -->|"Failure"| M["Return structured error to next model turn"] + K -->|"No edit"| N["Continue normal loop"] + + I --> O{"max_steps exhausted?"} + L --> O + M --> O + N --> O + O -->|"No"| E + O -->|"Yes"| P["End unfinished"] +~~~ + +### 7.2 Replace-first fallback + +~~~mermaid +flowchart TD + A["Model evaluates edit"] --> B{"Localized existing text with exact unique anchor?"} + B -->|"Yes"| C["replace_text"] + B -->|"No"| D["apply_patch"] + + C --> E{"Result"} + E -->|"Success"| F["Verify latest revision"] + E -->|"Failure"| G["Next model turn observes structured error"] + G --> H{"Recovery guidance"} + H -->|"Fresh source required"| I["read_file in a later turn"] + H -->|"Arguments invalid"| J["Correct arguments"] + H -->|"Patch is now appropriate"| D + I --> K["Following turn: corrected replace or deliberate patch"] + J --> C + K --> C + K --> D + + D --> L{"Patch result"} + L -->|"Success"| F + L -->|"Failure"| M["Next turn: diagnose, re-read if required, regenerate"] + M --> D +~~~ + +## 8. Trace and evaluation + +Existing tool_call and tool_result events already record: + +- chosen editing tool; +- arguments and correlation ID; +- success or failure; +- structured error code and bounded details; +- the later calls that form the recovery path. + +Issue #10 adds editing_strategy and frozen tool_names to run_start. Batch +rejection is recorded as a protocol_violation with +multiple_edit_operations. + +Native evaluation results also record the strategy and editing metrics: + +- first edit-attempt success; +- eventual mutation success; +- edit attempts, additional attempts, and failed attempts; +- replace and patch attempt counts; +- rejected editing calls; +- error-code counts; +- verification success after mutation; +- Agent steps and token usage. + +Resolved-task rate comes from the benchmark grader. Unrelated changed lines and +wrong-target or partial-target mutations require benchmark-specific ground +truth and cannot be inferred reliably by the generic Agent loop. + +Fair comparison requires the same: + +- tasks and base commits; +- model and model parameters; +- step, token, command, and wall-time budgets; +- grading logic; +- number of repeated trials. + +Run each task with both: + +~~~text +yada eval ... --editing-strategy patch-only +yada eval ... --editing-strategy replace-first +~~~ + +No benchmark winner is claimed until those controlled runs exist. + +## 9. Deterministic tests + +Tests cover: + +- patch-only exposes apply_patch but not replace_text; +- replace-first exposes both editing tools; +- prompts document the matching routing policy; +- strategy and tool names appear in run_start; +- schemas stay unchanged across model requests; +- two editing calls in one response execute neither call; +- every rejected call receives a structured result; +- a failed replacement is visible before a later patch; +- no tool or host code performs automatic strategy fallback; +- CLI and evaluation adapters propagate the selected strategy; +- existing SHA, transaction, and verification tests pass unchanged. + +## 10. Deliberate limits + +The first implementation relies on max_steps as its only loop boundary. It does +not add: + +- per-error retry limits; +- a per-revision edit-failure budget; +- protocol or no-progress budgets; +- unchanged-attempt fingerprints; +- automatic recovery reads; +- mandatory read state; +- PATCH_REQUIRED or other recovery phases; +- a formal termination proof beyond the existing finite model-turn loop. + +If trace-backed benchmarks reveal a concrete repeated-failure pattern, address +that pattern in a separate, measured change. diff --git a/docs/evaluation.md b/docs/evaluation.md index f452c96..aef9377 100644 --- a/docs/evaluation.md +++ b/docs/evaluation.md @@ -55,7 +55,7 @@ recorded in the result JSON instead of losing the whole run. Argument errors and a missing API key happen before the runner starts and therefore do not create a result. -The grader owns the final verdict. The agent's `finish` call and summary do not +The grader owns the final verdict. The agent's `finish_task` call and summary do not make an evaluation `resolved`. ## Why Yada keeps both selectors @@ -171,14 +171,22 @@ model-requested commands; it does not skip environment preparation or grading. During the run: - all model-facing file operations stay inside the candidate workspace; +- the selected editing strategy and model-facing tool schemas stay fixed; - edits are SHA-bound and applied through checked patch transactions; - `run_command` uses the configured approval policy; -- `finish` requires a successful test or build after the latest edit; and +- `finish_task` requires a successful test or build after the latest edit; and - events are appended to `yada-trace.jsonl`. After the agent stops, Yada collects all tracked and untracked Git changes into one patch relative to `HEAD`. +For the native adapter, `agent_run.details` also records the selected +`editing_strategy` and trace-derived `editing_metrics`, including first-edit +success, eventual mutation success, additional and failed edit attempts, per-tool +attempts, structured error counts, rejected editing calls, and post-edit +verification status. Use the same task, model, parameters, and budgets when +comparing `patch-only` with `replace-first`. + ### 4. Run the case grader Yada writes the collected patch to `agent.patch`, substitutes manifest diff --git a/src/yada/agents/default.py b/src/yada/agents/default.py index f5bb196..6a4ed57 100644 --- a/src/yada/agents/default.py +++ b/src/yada/agents/default.py @@ -25,7 +25,7 @@ class AgentResult: """Final outcome returned by :meth:`Agent.run`. Attributes: - finished: Whether the verification-gated ``finish`` tool succeeded. + finished: Whether the verification-gated ``finish_task`` tool succeeded. steps: Number of model turns consumed. summary: Model-provided summary or the step-limit explanation. usage: Flattened token and provider usage counters. @@ -72,7 +72,14 @@ def __init__( self.trace = trace self.max_steps = max_steps self.emit = emit - self.planner = planner or Planner() + if ( + planner is not None + and planner.editing_strategy is not tools.editing_strategy + ): + raise ValueError( + "planner and tool runner must use the same editing strategy" + ) + self.planner = planner or Planner(tools.editing_strategy) self.executor = executor or Executor(tools=tools, trace=trace, emit=emit) self.trace_metadata = dict(trace_metadata or {}) @@ -99,6 +106,8 @@ def run(self, task: str) -> AgentResult: "task": task, "workspace": str(self.tools.workspace.root), "max_steps": self.max_steps, + "editing_strategy": self.tools.editing_strategy.value, + "tool_names": list(self.tools.tool_names), "trace_level": self.trace.level, "model_config": client_trace_config(self.client), "provenance": collect_provenance( @@ -183,6 +192,7 @@ def run(self, task: str) -> AgentResult: "action": "execute_tools" if plan.tool_calls else "remind", "tools": [tool_name(call) for call in plan.tool_calls], "rejection_error": plan.rejection_error, + "rejection_error_code": plan.rejection_error_code, }, ) consecutive_text_turns = plan.consecutive_text_turns @@ -201,6 +211,7 @@ def run(self, task: str) -> AgentResult: step, plan.tool_calls, rejection_error=plan.rejection_error, + rejection_error_code=plan.rejection_error_code, ) for executed in executed_calls: messages.append( @@ -223,10 +234,20 @@ def run(self, task: str) -> AgentResult: self.trace.write("run_end", _result_record(result)) return result + state = self.tools.context.state + if state.patch_count > 0 and state.verified_revision == state.revision: + step_limit_summary = ( + "Step limit reached after verification succeeded but before " + "finish_task was called." + ) + else: + step_limit_summary = ( + "Step limit reached before the verification gate was satisfied." + ) result = AgentResult( finished=False, steps=self.max_steps, - summary="Step limit reached before the verification gate was satisfied.", + summary=step_limit_summary, usage=total_usage, final_state=self.tools.final_state(), ) diff --git a/src/yada/agents/executor.py b/src/yada/agents/executor.py index 9128206..ee4f686 100644 --- a/src/yada/agents/executor.py +++ b/src/yada/agents/executor.py @@ -48,6 +48,7 @@ def execute_batch( tool_calls: tuple[dict[str, Any], ...], *, rejection_error: str | None = None, + rejection_error_code: str | None = None, ) -> list[ExecutedToolCall]: """Execute a validated batch while preserving model call order. @@ -55,6 +56,7 @@ def execute_batch( step: One-based agent-loop step used for trace correlation. tool_calls: Calls from the current assistant response. rejection_error: If set, reject every call without side effects. + rejection_error_code: Stable code attached to every rejection result. Returns: One result per input call in the same order. @@ -63,19 +65,38 @@ def execute_batch( if rejection_error is not None: self.trace.write( "protocol_violation", - {"step": step, "error": rejection_error, "call_count": len(tool_calls)}, + { + "step": step, + "error_code": rejection_error_code, + "error": rejection_error, + "call_count": len(tool_calls), + }, ) return [ - self._rejected_call(step, call, rejection_error) for call in tool_calls + self._rejected_call( + step, + call, + rejection_error, + error_code=rejection_error_code, + ) + for call in tool_calls ] return [self._execute_tool_call(step, call) for call in tool_calls] def _rejected_call( - self, step: int, call: dict[str, Any], error: str + self, + step: int, + call: dict[str, Any], + error: str, + *, + error_code: str | None = None, ) -> ExecutedToolCall: call_id = _tool_call_id(call, step) name = tool_name(call) - execution = ToolExecution({"ok": False, "error": error}) + data = {"ok": False, "error": error} + if error_code is not None: + data["error_code"] = error_code + execution = ToolExecution(data) self.trace.write( "tool_call", { @@ -83,6 +104,7 @@ def _rejected_call( "tool_call_id": call_id, "tool": name, "rejected": True, + "error_code": error_code, }, ) self._trace_result(step, call_id, name, execution, duration_ms=0) diff --git a/src/yada/agents/planning.py b/src/yada/agents/planning.py index c2e4654..2a755f5 100644 --- a/src/yada/agents/planning.py +++ b/src/yada/agents/planning.py @@ -11,7 +11,14 @@ from dataclasses import dataclass from typing import Any -from yada.agents.prompts import SYSTEM_PROMPT, task_prompt +from yada.agents.prompts import system_prompt, task_prompt +from yada.editing import ( + DEFAULT_EDITING_STRATEGY, + EditingStrategy, + parse_editing_strategy, +) + +EDITING_TOOLS = frozenset({"replace_text", "apply_patch"}) @dataclass(frozen=True) @@ -24,6 +31,7 @@ class StepPlan: display_text: Assistant text that should be shown to the user. reminder: Protocol reminder to append to the conversation, if needed. rejection_error: Batch-level protocol error that rejects every tool call. + rejection_error_code: Stable code for a rejected batch. """ tool_calls: tuple[dict[str, Any], ...] @@ -31,6 +39,7 @@ class StepPlan: display_text: str = "" reminder: str | None = None rejection_error: str | None = None + rejection_error_code: str | None = None class Planner: @@ -41,6 +50,13 @@ class Planner: not know how any tool is implemented and cannot modify the workspace. """ + def __init__( + self, + editing_strategy: EditingStrategy | str = DEFAULT_EDITING_STRATEGY, + ) -> None: + self.editing_strategy = parse_editing_strategy(editing_strategy) + self._system_prompt = system_prompt(self.editing_strategy) + def initial_messages(self, task: str) -> list[dict[str, Any]]: """Build the stable message prefix for a user task. @@ -57,7 +73,7 @@ def initial_messages(self, task: str) -> list[dict[str, Any]]: if not task.strip(): raise ValueError("task must not be empty") return [ - {"role": "system", "content": SYSTEM_PROMPT}, + {"role": "system", "content": self._system_prompt}, {"role": "user", "content": task_prompt(task)}, ] @@ -75,7 +91,7 @@ def plan( Returns: A :class:`StepPlan` containing either executable calls or a recovery - reminder. A mixed ``finish`` batch is preserved for traceability but + reminder. A mixed ``finish_task`` batch is preserved for traceability but marked with ``rejection_error`` so the executor cannot run it. """ @@ -83,9 +99,9 @@ def plan( if not tool_calls: text_turns = consecutive_text_turns + 1 reminder = ( - "Continue working with tools. You must call finish after a patch and a " - "successful test/build; a text-only response does not complete the " - "task." + "Continue working with tools. You must call the finish_task tool after " + "a patch and a successful test/build; a text-only response does not " + "complete the task." ) if text_turns >= 3: reminder += ( @@ -99,16 +115,27 @@ def plan( ) rejection_error = None - if len(tool_calls) > 1 and any( - _tool_name(call) == "finish" for call in tool_calls + rejection_error_code = None + editing_call_count = sum( + _tool_name(call) in EDITING_TOOLS for call in tool_calls + ) + if editing_call_count > 1: + rejection_error = "only one editing operation is allowed per assistant turn" + rejection_error_code = "multiple_edit_operations" + elif len(tool_calls) > 1 and any( + _tool_name(call) == "finish_task" for call in tool_calls ): - # A concurrent finish could report success while sibling calls are still + # Concurrent completion could report success while sibling calls are still # mutating or verifying the repository, so reject the entire batch. - rejection_error = "finish must be the only tool call in its assistant turn" + rejection_error = ( + "finish_task must be the only tool call in its assistant turn" + ) + rejection_error_code = "finish_task_must_be_alone" return StepPlan( tool_calls=tool_calls, consecutive_text_turns=0, rejection_error=rejection_error, + rejection_error_code=rejection_error_code, ) diff --git a/src/yada/agents/prompts.py b/src/yada/agents/prompts.py index 55802a8..815a222 100644 --- a/src/yada/agents/prompts.py +++ b/src/yada/agents/prompts.py @@ -1,34 +1,77 @@ """Stable prompts for the default Yada agent.""" -SYSTEM_PROMPT = """You are Yada, a small autonomous coding agent optimized for DeepSeek. +from __future__ import annotations + +from yada.editing import ( + DEFAULT_EDITING_STRATEGY, + EditingStrategy, + parse_editing_strategy, +) + +_BASE_SYSTEM_PROMPT = """You are Yada, a small autonomous coding agent optimized for DeepSeek. Your job is to solve the user's task inside the provided workspace and leave a minimal, correct patch. Work directly with tools. Be concise and evidence-driven. Rules: -1. Search before reading, and read a file before editing it. -2. read_file returns a SHA-256. replace_text and apply_patch require the current - SHA-256 for every existing file they touch; apply_patch uses NEW for a new file. -3. Prefer small unified diffs. Do not rewrite unrelated code. -4. Run the most relevant available tests after the last patch. A successful inspection - command is not a test. -5. When a command fails, use its exit code and structured output to form a new hypothesis. -6. Never claim success without verification. Call finish only after a relevant test or - build succeeds after the latest patch. -7. Stay inside the workspace. Do not access secrets, hidden grader tests, the network, +1. Use search when the target location is unclear. Read the exact target before editing. +2. read_file returns a SHA-256. Editing tools require the current SHA-256 for + every existing file they touch; apply_patch uses NEW for a new file. +3. Once the target and intended edit are clear, edit promptly. Before changing shared + helpers, lifecycle behavior, or public APIs, inspect the directly relevant callers + and invariants. Do not repeat searches that only confirm established facts. +4. Prefer small, targeted edits. Do not rewrite unrelated code. +5. Use editing tools for all workspace file changes. Submit at most one editing + operation per assistant turn. Never modify workspace files through run_command. +6. Run the smallest relevant test or build after the latest edit; broaden verification + only when concrete risk or evidence warrants it. Prefer direct test commands. A + wrapper must propagate its child process exit code. Inspection is not verification. +7. After a focused reproducer or relevant suite passes for the current revision, call + finish_task next. Do not perform final re-reads or equivalent checks unless the + output shows a specific unresolved problem. +8. When a tool or command fails, use its structured error, recovery instruction, exit + code, and output to form the next action. +9. Stay inside the workspace. Do not access secrets, hidden grader tests, the network, .git internals, or .yada traces. -8. Do not ask the user to perform work that the available tools can do. - -Tool strategy: -- search_code: locate symbols and references. -- read_file: inspect bounded line ranges and obtain a file hash. -- replace_text: make exact, unique, version-checked replacements in existing text. -- apply_patch: make a version-checked unified-diff edit. -- run_command: inspect or verify with an argv array; no shell syntax. -- finish: submit only after the verification gate is satisfied. +10. Do not ask the user to perform work that the available tools can do. +""" + +_PATCH_ONLY_POLICY = """ +Editing strategy: patch-only. +- Use apply_patch for every workspace edit. +- After a failure, follow the structured recovery instruction and retry only after + correcting the patch or refreshing stale content. +""" + +_REPLACE_FIRST_POLICY = """ +Editing strategy: replace-first. +- For a localized change to an existing text file, prefer replace_text with an exact, + unique, reasonably bounded old_text. +- Use apply_patch for new or deleted files, broad structural changes, or edits that + cannot be expressed with a reasonably sized exact anchor. +- After a failure, follow the structured recovery instruction. Retry or switch tools + only in a later turn after observing the result. """ +def system_prompt( + editing_strategy: EditingStrategy | str = DEFAULT_EDITING_STRATEGY, +) -> str: + """Return the frozen system prompt for one editing strategy.""" + + strategy = parse_editing_strategy(editing_strategy) + policy = ( + _PATCH_ONLY_POLICY + if strategy is EditingStrategy.PATCH_ONLY + else _REPLACE_FIRST_POLICY + ) + return _BASE_SYSTEM_PROMPT.rstrip() + "\n" + policy.strip() + "\n" + + +# Compatibility constant for callers that use Yada's default strategy. +SYSTEM_PROMPT = system_prompt() + + def task_prompt(task: str) -> str: """Wrap a user task with workspace and behavior constraints. diff --git a/src/yada/editing.py b/src/yada/editing.py new file mode 100644 index 0000000..f713ec4 --- /dev/null +++ b/src/yada/editing.py @@ -0,0 +1,38 @@ +"""Run-level editing strategy shared by prompts, tools, and evaluations.""" + +from __future__ import annotations + +from enum import Enum + + +class EditingStrategy(str, Enum): + """Stable editing policy selected once for an Agent run.""" + + PATCH_ONLY = "patch-only" + REPLACE_FIRST = "replace-first" + + +DEFAULT_EDITING_STRATEGY = EditingStrategy.REPLACE_FIRST +EDITING_STRATEGY_CHOICES = tuple(strategy.value for strategy in EditingStrategy) + + +def parse_editing_strategy( + value: EditingStrategy | str, +) -> EditingStrategy: + """Normalize a public strategy value or raise a concise validation error.""" + + if isinstance(value, EditingStrategy): + return value + try: + return EditingStrategy(value) + except ValueError as exc: + choices = ", ".join(EDITING_STRATEGY_CHOICES) + raise ValueError(f"editing_strategy must be one of: {choices}") from exc + + +__all__ = [ + "DEFAULT_EDITING_STRATEGY", + "EDITING_STRATEGY_CHOICES", + "EditingStrategy", + "parse_editing_strategy", +] diff --git a/src/yada/evals/agents/yada.py b/src/yada/evals/agents/yada.py index 3bf3f1f..40f34e8 100644 --- a/src/yada/evals/agents/yada.py +++ b/src/yada/evals/agents/yada.py @@ -7,12 +7,19 @@ from typing import Callable from yada.agents import Agent +from yada.editing import ( + DEFAULT_EDITING_STRATEGY, + EditingStrategy, + parse_editing_strategy, +) from yada.environments import CommandApprover, CommandExecutor, DockerCommandExecutor from yada.evals.base import AgentRunResult, PreparedTask, RunBudget from yada.evals.patches import collect_git_patch from yada.models import CompletionClient, DeepSeekClient from yada.tools import ToolRunner -from yada.traces import TraceWriter +from yada.traces import TraceWriter, read_trace + +_EDITING_TOOLS = frozenset({"apply_patch", "replace_text"}) class YadaAgentAdapter: @@ -32,6 +39,7 @@ def __init__( command_timeout_seconds: int = 120, command_policy: str = "ask", trace_level: str = "summary", + editing_strategy: EditingStrategy | str = DEFAULT_EDITING_STRATEGY, client_factory: Callable[[RunBudget], CompletionClient] | None = None, emit: Callable[[str], None] = print, ) -> None: @@ -46,6 +54,7 @@ def __init__( self.command_timeout_seconds = command_timeout_seconds self.command_policy = command_policy self.trace_level = trace_level + self.editing_strategy = parse_editing_strategy(editing_strategy) self.client_factory = client_factory self.emit = emit @@ -75,6 +84,7 @@ def run( command_timeout_seconds=self.command_timeout_seconds, command_environment=_task_environment(prepared), command_executor=_task_command_executor(prepared), + editing_strategy=self.editing_strategy, ) command_provenance = _command_provenance(prepared, tools) agent = Agent( @@ -102,6 +112,8 @@ def run( steps = native_result.steps details = { "finished": native_result.finished, + "editing_strategy": self.editing_strategy.value, + "editing_metrics": _editing_metrics(trace_path, tools), **command_provenance, } except Exception as exc: @@ -112,6 +124,7 @@ def run( details = { "error_type": type(exc).__name__, "error": str(exc), + "editing_strategy": self.editing_strategy.value, **command_provenance, } finally: @@ -142,6 +155,58 @@ def _task_environment(prepared: PreparedTask) -> dict[str, str]: } +def _editing_metrics(trace_path: Path, tools: ToolRunner) -> dict[str, object]: + """Derive strategy-comparison metrics from one native Yada trace.""" + + events = read_trace(trace_path) + calls = { + str(event["data"].get("tool_call_id")): event["data"] + for event in events + if event["event"] == "tool_call" + and event["data"].get("tool_call_id") is not None + } + attempts: list[dict[str, object]] = [] + rejected = 0 + error_codes: dict[str, int] = {} + tool_attempts = {"apply_patch": 0, "replace_text": 0} + for event in events: + if event["event"] != "tool_result": + continue + data = event["data"] + tool = data.get("tool") + if tool not in _EDITING_TOOLS: + continue + call = calls.get(str(data.get("tool_call_id")), {}) + if call.get("rejected") is True: + rejected += 1 + continue + result = data.get("result") + if not isinstance(result, dict): + continue + attempts.append(result) + tool_attempts[str(tool)] += 1 + error_code = result.get("error_code") + if isinstance(error_code, str): + error_codes[error_code] = error_codes.get(error_code, 0) + 1 + + state = tools.context.state + first_success = None if not attempts else attempts[0].get("ok") is True + failed_attempts = sum(result.get("ok") is False for result in attempts) + return { + "first_edit_attempt_success": first_success, + "eventual_mutation_success": state.patch_count > 0, + "edit_attempts": len(attempts), + "additional_edit_attempts": max(0, len(attempts) - 1), + "failed_edit_attempts": failed_attempts, + "tool_attempts": tool_attempts, + "rejected_editing_calls": rejected, + "error_codes": error_codes, + "verification_success_after_mutation": ( + state.patch_count > 0 and state.verified_revision == state.revision + ), + } + + def _task_command_executor(prepared: PreparedTask) -> CommandExecutor | None: value = prepared.metadata.get("command_backend") if value is None: diff --git a/src/yada/evals/cli.py b/src/yada/evals/cli.py index 5f835aa..597fd9b 100644 --- a/src/yada/evals/cli.py +++ b/src/yada/evals/cli.py @@ -9,6 +9,7 @@ import sys from pathlib import Path +from yada.editing import DEFAULT_EDITING_STRATEGY, EDITING_STRATEGY_CHOICES from yada.evals.agents import CommandAgentAdapter, YadaAgentAdapter from yada.evals.base import RunBudget from yada.evals.benchmarks import LocalBenchmark, SWEbenchBenchmark @@ -80,6 +81,15 @@ def build_parser() -> argparse.ArgumentParser: action=argparse.BooleanOptionalAction, default=True, ) + model.add_argument( + "--editing-strategy", + choices=EDITING_STRATEGY_CHOICES, + default=DEFAULT_EDITING_STRATEGY.value, + help=( + "Run-level editing policy for the native Yada agent " + f"(default: {DEFAULT_EDITING_STRATEGY.value})." + ), + ) model.add_argument("--api-timeout", type=int, default=300) model.add_argument("--command-timeout", type=int, default=120) model.add_argument( @@ -144,6 +154,7 @@ def run_cli(argv: list[str] | None = None) -> int: command_timeout_seconds=args.command_timeout, command_policy="allow" if args.yes else args.command_policy, trace_level=args.trace_level, + editing_strategy=args.editing_strategy, ) else: if not args.agent_command: diff --git a/src/yada/run/cli.py b/src/yada/run/cli.py index 90b79ac..f2978a2 100644 --- a/src/yada/run/cli.py +++ b/src/yada/run/cli.py @@ -9,6 +9,7 @@ from yada import __version__ from yada.agents import Agent +from yada.editing import DEFAULT_EDITING_STRATEGY, EDITING_STRATEGY_CHOICES from yada.models import DeepSeekAPIError, DeepSeekClient from yada.tools import ToolRunner from yada.traces import TraceWriter @@ -47,6 +48,12 @@ def build_parser() -> argparse.ArgumentParser: parser.add_argument( "--thinking", action=argparse.BooleanOptionalAction, default=True ) + parser.add_argument( + "--editing-strategy", + choices=EDITING_STRATEGY_CHOICES, + default=DEFAULT_EDITING_STRATEGY.value, + help=(f"Run-level editing policy (default: {DEFAULT_EDITING_STRATEGY.value})."), + ) parser.add_argument("--max-steps", type=int, default=30) parser.add_argument("--max-output-tokens", type=int, default=16_384) parser.add_argument("--api-timeout", type=int, default=300) @@ -137,6 +144,7 @@ def run_cli(argv: list[str] | None = None) -> int: workspace, command_policy=command_policy, command_timeout_seconds=args.command_timeout, + editing_strategy=args.editing_strategy, ), trace=TraceWriter( trace_path, @@ -151,6 +159,7 @@ def run_cli(argv: list[str] | None = None) -> int: f"Model: {args.model} " f"(thinking={args.thinking}, effort={args.reasoning_effort})" ) + print(f"Editing strategy: {args.editing_strategy}") print(f"Trace: {trace_path}") print(f"Trace level: {args.trace_level}") if command_policy == "allow": diff --git a/src/yada/tools/command.py b/src/yada/tools/command.py index 3579ee7..3144d96 100644 --- a/src/yada/tools/command.py +++ b/src/yada/tools/command.py @@ -113,7 +113,7 @@ def run_command( stderr_text, stderr_truncated = truncate_text(stderr, context.max_output_chars) # Verification is tied to the current revision. A later patch increments the - # revision and invalidates this success, so finish cannot use stale test output. + # revision and invalidates this success, so finish_task cannot use stale output. if purpose in {"test", "build"} and exit_code == 0 and not timed_out: context.state.verified_revision = context.state.revision context.state.successful_verifications.append( diff --git a/src/yada/tools/finish.py b/src/yada/tools/finish.py index f9ba9b7..26cb78f 100644 --- a/src/yada/tools/finish.py +++ b/src/yada/tools/finish.py @@ -10,7 +10,7 @@ from yada.utils.text import truncate_text -def finish(context: ToolContext, summary: str) -> ToolExecution: +def finish_task(context: ToolContext, summary: str) -> ToolExecution: """Complete a run only after the latest revision passes verification. Args: @@ -27,14 +27,14 @@ def finish(context: ToolContext, summary: str) -> ToolExecution: if not isinstance(summary, str) or not summary.strip(): raise ToolError("summary must be a non-empty string") if context.state.patch_count == 0: - raise ToolError("finish rejected: no patch has been applied") + raise ToolError("finish_task rejected: no patch has been applied") if context.state.verified_revision != context.state.revision: raise ToolError( - "finish rejected: run a successful test or build after the latest patch" + "finish_task rejected: run a successful test or build after the latest patch" ) diff_check = _git_diff_check(context) if diff_check: - raise ToolError(f"finish rejected by git diff --check: {diff_check}") + raise ToolError(f"finish_task rejected by git diff --check: {diff_check}") return ToolExecution( { "ok": True, diff --git a/src/yada/tools/patch.py b/src/yada/tools/patch.py index 31515e5..124d141 100644 --- a/src/yada/tools/patch.py +++ b/src/yada/tools/patch.py @@ -126,7 +126,7 @@ def apply_patch( return { "revision": context.state.revision, "changed_files": changed, - "message": "patch applied; run a relevant test before finish", + "message": "patch applied; run a relevant test before finish_task", } diff --git a/src/yada/tools/replace.py b/src/yada/tools/replace.py index 4d9d996..ef05c25 100644 --- a/src/yada/tools/replace.py +++ b/src/yada/tools/replace.py @@ -165,7 +165,7 @@ def replace_text( recovery="Read the current files and retry from the latest workspace state.", ) from exc - result["message"] = "text replaced; run a relevant test before finish" + result["message"] = "text replaced; run a relevant test before finish_task" return result diff --git a/src/yada/tools/runner.py b/src/yada/tools/runner.py index 2233182..68231fc 100644 --- a/src/yada/tools/runner.py +++ b/src/yada/tools/runner.py @@ -5,6 +5,11 @@ from pathlib import Path from typing import Any, Callable +from yada.editing import ( + DEFAULT_EDITING_STRATEGY, + EditingStrategy, + parse_editing_strategy, +) from yada.environments import ( CommandApprover, CommandExecutor, @@ -14,7 +19,7 @@ from yada.exceptions import ToolError from yada.tools.base import ToolContext, ToolExecution from yada.tools.command import run_command -from yada.tools.finish import final_state, finish +from yada.tools.finish import final_state, finish_task from yada.tools.patch import apply_patch from yada.tools.read import read_file from yada.tools.replace import replace_text @@ -37,7 +42,9 @@ def __init__( command_environment: dict[str, str] | None = None, command_executor: CommandExecutor | None = None, approver: CommandApprover | None = None, + editing_strategy: EditingStrategy | str = DEFAULT_EDITING_STRATEGY, ) -> None: + self.editing_strategy = parse_editing_strategy(editing_strategy) self.context = ToolContext( workspace=Workspace(workspace), approver=approver or CommandApprover(command_policy), @@ -50,9 +57,18 @@ def __init__( "search_code": search_code, "read_file": read_file, "apply_patch": apply_patch, - "replace_text": replace_text, "run_command": run_command, } + if self.editing_strategy is EditingStrategy.REPLACE_FIRST: + self._handlers["replace_text"] = replace_text + self._schemas = [ + schema + for schema in TOOL_SCHEMAS + if ( + self.editing_strategy is EditingStrategy.REPLACE_FIRST + or schema["function"]["name"] != "replace_text" + ) + ] @property def workspace(self) -> Workspace: @@ -64,7 +80,13 @@ def workspace(self) -> Workspace: def schemas(self) -> list[dict[str, Any]]: """Return the stable tool schemas sent with every model request.""" - return TOOL_SCHEMAS + return self._schemas + + @property + def tool_names(self) -> tuple[str, ...]: + """Return the frozen model-facing tool names for trace metadata.""" + + return tuple(schema["function"]["name"] for schema in self._schemas) def execute(self, name: str, arguments: dict[str, Any]) -> ToolExecution: """Dispatch one tool call and normalize expected validation failures. @@ -79,8 +101,8 @@ def execute(self, name: str, arguments: dict[str, Any]) -> ToolExecution: """ try: - if name == "finish": - return finish(self.context, **arguments) + if name == "finish_task": + return finish_task(self.context, **arguments) handler = self._handlers.get(name) if handler is None: raise ToolError(f"unknown tool: {name}") diff --git a/src/yada/tools/schemas.py b/src/yada/tools/schemas.py index 4d522c1..3cd11df 100644 --- a/src/yada/tools/schemas.py +++ b/src/yada/tools/schemas.py @@ -9,7 +9,7 @@ "type": "function", "function": { "name": "search_code", - "description": "Search text or regex in workspace files. Use this to locate symbols before reading.", + "description": "Search text or regex in workspace files when the target location is unclear.", "parameters": { "type": "object", "properties": { @@ -118,7 +118,7 @@ "type": "function", "function": { "name": "run_command", - "description": "Run an argv array in the workspace without a shell. Label it inspect, test, or build. Commands require policy approval unless Yada runs with --yes.", + "description": "Run an argv array without a shell for inspection, testing, or builds; do not modify workspace files. Prefer direct test/build commands; wrappers must propagate child exit status. Commands require policy approval unless Yada runs with --yes.", "parameters": { "type": "object", "properties": { @@ -138,7 +138,7 @@ { "type": "function", "function": { - "name": "finish", + "name": "finish_task", "description": "Submit the completed task. Rejected unless a patch exists and a relevant test/build passed after the latest patch.", "parameters": { "type": "object", diff --git a/src/yada/traces/html.py b/src/yada/traces/html.py index 5d2babb..7ba27cf 100644 --- a/src/yada/traces/html.py +++ b/src/yada/traces/html.py @@ -106,6 +106,18 @@ def _render_header(path: Path, run: TraceRun) -> str: def _render_run_details(run: TraceRun) -> str: start = run.run_start.data if run.run_start else {} end = run.run_end.data if run.run_end else {} + raw_model_config = start.get("model_config") + model_config = ( + dict(raw_model_config) + if isinstance(raw_model_config, dict) + else {"configuration": raw_model_config or "Unavailable"} + ) + model_config.update( + { + "editing_strategy": start.get("editing_strategy", "legacy"), + "tool_names": start.get("tool_names", "Unavailable"), + } + ) details = '
' details += _details( "Task", @@ -115,7 +127,7 @@ def _render_run_details(run: TraceRun) -> str: ) details += _details( "Model configuration", - start.get("model_config", "Unavailable"), + model_config, open_by_default=False, ) details += _details( diff --git a/src/yada/traces/report.py b/src/yada/traces/report.py index e503f58..6c81233 100644 --- a/src/yada/traces/report.py +++ b/src/yada/traces/report.py @@ -346,6 +346,7 @@ def _report_header(path: Path, run: TraceRun) -> list[str]: f"Path: {path}", f"Run: {run_id}", f"Model: {start_data.get('model', 'unknown')}", + f"Editing strategy: {start_data.get('editing_strategy', 'legacy')}", f"Trace level: {start_data.get('trace_level', 'legacy')}", f"Task: {_one_line(start_data.get('task', 'unknown'), 160)}", f"Outcome: {outcome}", diff --git a/tests/agents/test_default.py b/tests/agents/test_default.py index 2a9d479..3320de0 100644 --- a/tests/agents/test_default.py +++ b/tests/agents/test_default.py @@ -5,6 +5,8 @@ from pathlib import Path from typing import Any +import pytest + from yada.agents import Agent, Planner from yada.environments import CommandApprover from yada.models import Completion @@ -97,7 +99,7 @@ def add(a, b): "purpose": "test", }, ), - tool_call("call-finish", "finish", {"summary": "fixed add"}), + tool_call("call-finish-task", "finish_task", {"summary": "fixed add"}), ] ) trace_path = tmp_path / ".yada" / "test.jsonl" @@ -122,6 +124,62 @@ def add(a, b): assert client.seen_messages[1][-2]["reasoning_content"] == "reasoning for read_file" +def test_step_limit_reports_verified_revision_without_finish(tmp_path: Path) -> None: + path = tmp_path / "value.py" + path.write_text("VALUE = 1\n", encoding="utf-8") + subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) + subprocess.run(["git", "add", "value.py"], cwd=tmp_path, check=True) + runner = ToolRunner(tmp_path, approver=CommandApprover("allow")) + digest = runner.workspace.sha256(path) + client = FakeClient( + [ + tool_call( + "replace", + "replace_text", + { + "edits": [ + { + "path": "value.py", + "sha256": digest, + "old_text": "VALUE = 1", + "new_text": "VALUE = 2", + } + ] + }, + ), + tool_call( + "test", + "run_command", + { + "argv": [ + "python3", + "-c", + "import value; assert value.VALUE == 2", + ], + "purpose": "test", + }, + ), + ] + ) + trace_path = tmp_path / ".yada" / "verified-step-limit.jsonl" + + result = Agent( + client=client, + tools=runner, + trace=TraceWriter(trace_path), + max_steps=2, + emit=lambda _: None, + ).run("Change VALUE") + + assert not result.finished + assert result.summary == ( + "Step limit reached after verification succeeded but before finish_task was called." + ) + run_end = read_trace(trace_path)[-1] + assert run_end["event"] == "run_end" + assert run_end["data"]["summary"] == result.summary + + def test_debug_trace_reconstructs_exact_client_payload(tmp_path: Path) -> None: path = tmp_path / "value.py" path.write_text("VALUE = 1\n", encoding="utf-8") @@ -154,6 +212,9 @@ def test_debug_trace_reconstructs_exact_client_payload(tmp_path: Path) -> None: result = agent.run("Inspect VALUE") assert not result.finished + assert result.summary == ( + "Step limit reached before the verification gate was satisfied." + ) events = read_trace(trace_path) assert reconstruct_model_request(events, 1) == client.seen_payloads[0] assert reconstruct_model_request(events, 2) == client.seen_payloads[1] @@ -161,18 +222,22 @@ def test_debug_trace_reconstructs_exact_client_payload(tmp_path: Path) -> None: assert second_messages[-2]["reasoning_content"] == "reasoning for read_file" run_start = events[0]["data"] assert run_start["trace_level"] == "debug" + assert run_start["editing_strategy"] == "replace-first" + assert "apply_patch" in run_start["tool_names"] + assert "replace_text" in run_start["tool_names"] assert run_start["provenance"]["case_id"] == "fixture-1" assert "yada_version" in run_start["provenance"] assert "workspace_base_commit" in run_start["provenance"] + assert client.seen_payloads[0]["tools"] == client.seen_payloads[1]["tools"] -def test_planner_rejects_finish_mixed_with_other_calls() -> None: +def test_planner_rejects_finish_task_mixed_with_other_calls() -> None: planner = Planner() assistant_message = { "role": "assistant", "tool_calls": [ {"function": {"name": "run_command", "arguments": "{}"}}, - {"function": {"name": "finish", "arguments": "{}"}}, + {"function": {"name": "finish_task", "arguments": "{}"}}, ], } @@ -181,8 +246,234 @@ def test_planner_rejects_finish_mixed_with_other_calls() -> None: assert len(plan.tool_calls) == 2 assert plan.consecutive_text_turns == 0 assert plan.rejection_error == ( - "finish must be the only tool call in its assistant turn" + "finish_task must be the only tool call in its assistant turn" + ) + assert plan.rejection_error_code == "finish_task_must_be_alone" + + +def test_strategy_prompts_are_explicit_and_stable() -> None: + patch_planner = Planner("patch-only") + replace_planner = Planner("replace-first") + + patch_prompt = patch_planner.initial_messages("Fix it")[0]["content"] + replace_prompt = replace_planner.initial_messages("Fix it")[0]["content"] + + assert "Use search when the target location is unclear" in replace_prompt + assert "Never modify workspace files" in replace_prompt + assert "Submit at most one editing" in patch_prompt + assert "Submit at most one editing" in replace_prompt + assert "inspect the directly relevant callers" in replace_prompt + assert "wrapper must propagate its child process exit code" in replace_prompt + assert "finish_task next." in replace_prompt + assert "Do not perform final re-reads" in replace_prompt + assert "Tool strategy:" not in patch_prompt + assert "Tool strategy:" not in replace_prompt + assert "Editing strategy: patch-only" in patch_prompt + assert "Use apply_patch for every workspace edit" in patch_prompt + assert "follow the structured recovery instruction" in patch_prompt + assert "Editing strategy: replace-first" in replace_prompt + assert "prefer replace_text with an exact" in replace_prompt + assert "Once the target and intended edit are clear" in replace_prompt + assert "Do not repeat" in replace_prompt + assert "Retry or switch tools" in replace_prompt + assert replace_prompt == replace_planner.initial_messages("Fix it")[0]["content"] + + +def test_agent_rejects_mismatched_strategy_components(tmp_path: Path) -> None: + runner = ToolRunner(tmp_path, approver=CommandApprover("allow")) + + with pytest.raises(ValueError, match="same editing strategy"): + Agent( + client=FakeClient([]), + tools=runner, + trace=TraceWriter(None), + planner=Planner("patch-only"), + ) + + +def test_planner_rejects_multiple_editing_operations() -> None: + planner = Planner("replace-first") + assistant_message = { + "role": "assistant", + "tool_calls": [ + {"function": {"name": "replace_text", "arguments": "{}"}}, + {"function": {"name": "apply_patch", "arguments": "{}"}}, + ], + } + + plan = planner.plan(assistant_message, consecutive_text_turns=0) + + assert len(plan.tool_calls) == 2 + assert plan.rejection_error_code == "multiple_edit_operations" + assert plan.rejection_error == ( + "only one editing operation is allowed per assistant turn" + ) + + +def test_multiple_editing_calls_are_rejected_without_mutation( + tmp_path: Path, +) -> None: + path = tmp_path / "value.py" + path.write_text("VALUE = 1\n", encoding="utf-8") + subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) + subprocess.run(["git", "add", "value.py"], cwd=tmp_path, check=True) + runner = ToolRunner( + tmp_path, + approver=CommandApprover("allow"), + editing_strategy="replace-first", + ) + digest = runner.workspace.sha256(path) + patch = """diff --git a/value.py b/value.py +--- a/value.py ++++ b/value.py +@@ -1 +1 @@ +-VALUE = 1 ++VALUE = 2 +""" + first = Completion( + message={ + "role": "assistant", + "content": "", + "reasoning_content": "try replace and patch", + "tool_calls": [ + { + "id": "replace", + "type": "function", + "function": { + "name": "replace_text", + "arguments": json.dumps( + { + "edits": [ + { + "path": "value.py", + "sha256": digest, + "old_text": "VALUE = 1", + "new_text": "VALUE = 2", + } + ] + } + ), + }, + }, + { + "id": "patch", + "type": "function", + "function": { + "name": "apply_patch", + "arguments": json.dumps( + { + "patch": patch, + "expected_files": [ + {"path": "value.py", "sha256": digest} + ], + } + ), + }, + }, + ], + }, + usage={}, + model="fake-deepseek-v4-pro", + finish_reason="tool_calls", + ) + client = FakeClient( + [ + first, + Completion( + message={"role": "assistant", "content": "stop"}, + usage={}, + model="fake-deepseek-v4-pro", + finish_reason="stop", + ), + ] ) + trace_path = tmp_path / ".yada" / "multiple-edits.jsonl" + result = Agent( + client=client, + tools=runner, + trace=TraceWriter(trace_path), + max_steps=2, + emit=lambda _: None, + ).run("Change VALUE") + + assert not result.finished + assert path.read_text(encoding="utf-8") == "VALUE = 1\n" + tool_results = [ + message for message in client.seen_messages[1] if message.get("role") == "tool" + ] + assert len(tool_results) == 2 + assert all( + json.loads(message["content"])["error_code"] == "multiple_edit_operations" + for message in tool_results + ) + events = read_trace(trace_path) + violation = next( + event for event in events if event["event"] == "protocol_violation" + ) + assert violation["data"]["error_code"] == "multiple_edit_operations" + assert runner.context.state.patch_count == 0 + + +def test_failed_replace_is_observed_before_later_patch(tmp_path: Path) -> None: + path = tmp_path / "value.py" + path.write_text("VALUE = 1\n", encoding="utf-8") + subprocess.run(["git", "init", "-q"], cwd=tmp_path, check=True) + subprocess.run(["git", "add", "value.py"], cwd=tmp_path, check=True) + runner = ToolRunner( + tmp_path, + approver=CommandApprover("allow"), + editing_strategy="replace-first", + ) + digest = runner.workspace.sha256(path) + patch = """diff --git a/value.py b/value.py +--- a/value.py ++++ b/value.py +@@ -1 +1 @@ +-VALUE = 1 ++VALUE = 2 +""" + client = FakeClient( + [ + tool_call( + "replace", + "replace_text", + { + "edits": [ + { + "path": "value.py", + "sha256": digest, + "old_text": "VALUE = 9", + "new_text": "VALUE = 2", + } + ] + }, + ), + tool_call( + "patch", + "apply_patch", + { + "patch": patch, + "expected_files": [{"path": "value.py", "sha256": digest}], + }, + ), + ] + ) + trace_path = tmp_path / ".yada" / "fallback.jsonl" + result = Agent( + client=client, + tools=runner, + trace=TraceWriter(trace_path, level="debug"), + max_steps=2, + emit=lambda _: None, + ).run("Change VALUE") + + assert not result.finished + assert path.read_text(encoding="utf-8") == "VALUE = 2\n" + observed = json.loads(client.seen_messages[1][-1]["content"]) + assert observed["error_code"] == "no_match" + start = read_trace(trace_path)[0]["data"] + assert start["editing_strategy"] == "replace-first" + assert "replace_text" in start["tool_names"] def test_planner_escalates_repeated_text_only_turns() -> None: diff --git a/tests/conftest.py b/tests/conftest.py index 97032ab..b9a2296 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -21,4 +21,8 @@ def git_workspace(tmp_path: Path) -> Path: @pytest.fixture def tool_runner(git_workspace: Path) -> ToolRunner: - return ToolRunner(git_workspace, approver=CommandApprover("allow")) + return ToolRunner( + git_workspace, + approver=CommandApprover("allow"), + editing_strategy="replace-first", + ) diff --git a/tests/evals/test_eval_cli.py b/tests/evals/test_eval_cli.py index d052814..2105df5 100644 --- a/tests/evals/test_eval_cli.py +++ b/tests/evals/test_eval_cli.py @@ -15,6 +15,15 @@ def test_eval_cli_has_two_task_selectors() -> None: assert case.swebench is None assert swebench.case is None assert swebench.swebench == "owner__repo-1" + assert case.editing_strategy == "replace-first" + + +def test_eval_cli_exposes_editing_strategy() -> None: + args = build_parser().parse_args( + ["--case", "case-dir", "--editing-strategy", "replace-first"] + ) + + assert args.editing_strategy == "replace-first" def test_eval_cli_rejects_multiple_task_selectors() -> None: diff --git a/tests/evals/test_yada_agent.py b/tests/evals/test_yada_agent.py index eea4e00..abae63e 100644 --- a/tests/evals/test_yada_agent.py +++ b/tests/evals/test_yada_agent.py @@ -1,21 +1,30 @@ from __future__ import annotations +import json import subprocess from pathlib import Path +from yada.agents.executor import Executor +from yada.environments import CommandApprover from yada.evals import EvalTask, PreparedTask, RunBudget from yada.evals.agents import YadaAgentAdapter +from yada.evals.agents.yada import _editing_metrics from yada.models import Completion -from yada.traces import read_trace, reconstruct_model_request +from yada.tools import ToolRunner +from yada.traces import TraceWriter, read_trace, reconstruct_model_request class OneTurnClient: model = "fake-deepseek" + def __init__(self) -> None: + self.seen_tools: list[list[dict[str, object]]] = [] + def request_payload(self, *, messages, tools): return {"model": self.model, "messages": messages, "tools": tools} def complete(self, *, messages, tools): + self.seen_tools.append(tools) return Completion( message={ "role": "assistant", @@ -56,6 +65,7 @@ def test_yada_eval_trace_includes_case_and_workspace_provenance( adapter = YadaAgentAdapter( client_factory=lambda _: client, trace_level="debug", + editing_strategy="replace-first", emit=lambda _: None, ) @@ -68,8 +78,73 @@ def test_yada_eval_trace_includes_case_and_workspace_provenance( assert result.status == "unfinished" events = read_trace(run_dir / "yada-trace.jsonl") start = events[0]["data"] + assert start["editing_strategy"] == "replace-first" + assert "replace_text" in start["tool_names"] assert start["provenance"]["case_id"] == "case-123" assert start["provenance"]["workspace_base_commit"] == head assert reconstruct_model_request(events, 1)["model"] == "fake-deepseek" assistant = next(event for event in events if event["event"] == "assistant") assert assistant["data"]["message"]["reasoning_content"] == "debug reasoning" + assert any( + schema["function"]["name"] == "replace_text" for schema in client.seen_tools[0] + ) + assert result.details["editing_strategy"] == "replace-first" + metrics = result.details["editing_metrics"] + assert metrics["first_edit_attempt_success"] is None + assert metrics["edit_attempts"] == 0 + + +def test_editing_metrics_capture_retry_and_success(tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + workspace.mkdir() + path = workspace / "value.py" + path.write_text("VALUE = 1\n", encoding="utf-8") + subprocess.run(["git", "init", "-q"], cwd=workspace, check=True) + subprocess.run(["git", "add", "value.py"], cwd=workspace, check=True) + tools = ToolRunner( + workspace, + approver=CommandApprover("allow"), + editing_strategy="replace-first", + ) + digest = tools.workspace.sha256(path) + trace_path = tmp_path / "metrics.jsonl" + executor = Executor( + tools=tools, + trace=TraceWriter(trace_path), + emit=lambda _: None, + ) + + def call(call_id: str, old_text: str) -> dict[str, object]: + return { + "id": call_id, + "type": "function", + "function": { + "name": "replace_text", + "arguments": json.dumps( + { + "edits": [ + { + "path": "value.py", + "sha256": digest, + "old_text": old_text, + "new_text": "VALUE = 2", + } + ] + } + ), + }, + } + + executor.execute_batch(1, (call("miss", "VALUE = 9"),)) + executor.execute_batch(2, (call("success", "VALUE = 1"),)) + + metrics = _editing_metrics(trace_path, tools) + + assert metrics["first_edit_attempt_success"] is False + assert metrics["eventual_mutation_success"] is True + assert metrics["edit_attempts"] == 2 + assert metrics["additional_edit_attempts"] == 1 + assert metrics["failed_edit_attempts"] == 1 + assert metrics["tool_attempts"] == {"apply_patch": 0, "replace_text": 2} + assert metrics["error_codes"] == {"no_match": 1} + assert metrics["verification_success_after_mutation"] is False diff --git a/tests/test_cli.py b/tests/test_cli.py new file mode 100644 index 0000000..4999612 --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,13 @@ +from __future__ import annotations + +from yada.run.cli import build_parser + + +def test_direct_cli_exposes_editing_strategy() -> None: + parser = build_parser() + + default = parser.parse_args(["Fix it"]) + replace_first = parser.parse_args(["Fix it", "--editing-strategy", "replace-first"]) + + assert default.editing_strategy == "replace-first" + assert replace_first.editing_strategy == "replace-first" diff --git a/tests/tools/test_replace.py b/tests/tools/test_replace.py index 64ea8b3..a6e047a 100644 --- a/tests/tools/test_replace.py +++ b/tests/tools/test_replace.py @@ -50,7 +50,7 @@ def test_replace_text_is_public_and_updates_edit_state( assert tool_runner.context.state.patch_count == 1 assert tool_runner.context.state.touched_files == {"app.py"} assert tool_runner.context.state.verified_revision == -1 - assert not tool_runner.execute("finish", {"summary": "done"}).data["ok"] + assert not tool_runner.execute("finish_task", {"summary": "done"}).data["ok"] def test_no_match_is_structured_and_does_not_change_state( diff --git a/tests/tools/test_runner.py b/tests/tools/test_runner.py index 962d55a..4b5cce4 100644 --- a/tests/tools/test_runner.py +++ b/tests/tools/test_runner.py @@ -49,6 +49,40 @@ def answer(): """ +def test_editing_strategy_freezes_public_tool_interface( + git_workspace: Path, +) -> None: + default_runner = ToolRunner( + git_workspace, + approver=CommandApprover("allow"), + ) + patch_only = ToolRunner( + git_workspace, + approver=CommandApprover("allow"), + editing_strategy="patch-only", + ) + replace_first = ToolRunner( + git_workspace, + approver=CommandApprover("allow"), + editing_strategy="replace-first", + ) + + assert default_runner.editing_strategy.value == "replace-first" + assert "replace_text" in default_runner.tool_names + assert "finish_task" in default_runner.tool_names + assert "finish" not in default_runner.tool_names + assert not default_runner.execute("finish", {"summary": "done"}).data["ok"] + assert patch_only.editing_strategy.value == "patch-only" + assert "apply_patch" in patch_only.tool_names + assert "replace_text" not in patch_only.tool_names + assert replace_first.editing_strategy.value == "replace-first" + assert "apply_patch" in replace_first.tool_names + assert "replace_text" in replace_first.tool_names + assert patch_only.schemas is patch_only.schemas + assert replace_first.schemas is replace_first.schemas + assert not patch_only.execute("replace_text", {"edits": []}).data["ok"] + + def test_read_and_hash_checked_patch( git_workspace: Path, tool_runner: ToolRunner ) -> None: @@ -321,7 +355,7 @@ def test_finish_requires_verification_after_latest_patch( }, ) - premature = tool_runner.execute("finish", {"summary": "done"}) + premature = tool_runner.execute("finish_task", {"summary": "done"}) assert not premature.data["ok"] checked = tool_runner.execute( @@ -334,7 +368,7 @@ def test_finish_requires_verification_after_latest_patch( assert checked.data["ok"] assert checked.data["exit_code"] == 0 - finished = tool_runner.execute("finish", {"summary": "fixed answer"}) + finished = tool_runner.execute("finish_task", {"summary": "fixed answer"}) assert finished.finished assert finished.data["ok"] diff --git a/tests/traces/test_trace_html.py b/tests/traces/test_trace_html.py index c456d46..fad0567 100644 --- a/tests/traces/test_trace_html.py +++ b/tests/traces/test_trace_html.py @@ -23,6 +23,8 @@ def test_completed_trace_renders_offline_semantic_view( { "model": "deepseek-v4-pro", "task": "Fix parser\n\nA long issue description that stays collapsed.", + "editing_strategy": "replace-first", + "tool_names": ["read_file", "replace_text", "apply_patch"], "trace_level": "debug", "model_config": {"thinking": True}, "provenance": {"yada_version": "0.1.0", "case_id": "parser-1"}, @@ -102,6 +104,13 @@ def test_completed_trace_renders_offline_semantic_view( '
Task' in document ) assert "A long issue description that stays collapsed." in document + assert "replace-first" in document + assert "replace_text" in document + assert "Model configuration" in document + assert "Editing strategy" not in document + assert ""editing_strategy": "replace-first"" in document + assert ""thinking": true" in document + assert ""tool_names"" in document assert "Resolved · 1 step" in document assert "The boundary is off by one." in document assert "Reasoning" not in document diff --git a/tests/traces/test_trace_report.py b/tests/traces/test_trace_report.py index 19e7a4a..75f274b 100644 --- a/tests/traces/test_trace_report.py +++ b/tests/traces/test_trace_report.py @@ -29,6 +29,8 @@ def test_trace_events_have_correlation_metadata_and_redaction(tmp_path: Path) -> "model": "fake", "task": "fix", "workspace": ".", + "editing_strategy": "patch-only", + "tool_names": ["read_file", "apply_patch", "finish_task"], "trace_level": "summary", }, ) @@ -55,6 +57,7 @@ def test_trace_events_have_correlation_metadata_and_redaction(tmp_path: Path) -> report = render_trace_report(path) assert "Run: run-test" in report + assert "Editing strategy: patch-only" in report assert "Trace level: summary" in report assert "Outcome: unfinished" in report assert "Step 1/1 — fake 7ms 12 tokens [L2]" in report @@ -415,18 +418,18 @@ def test_protocol_violation_and_failed_tool_keep_line_references( { "step": 2, "action": "execute_tools", - "rejection_error": "duplicate finish calls", + "rejection_error": "duplicate finish_task calls", }, ), _record( 4, "protocol_violation", - {"step": 2, "error": "duplicate finish calls"}, + {"step": 2, "error": "duplicate finish_task calls"}, ), _record( 5, "tool_call", - {"step": 2, "tool_call_id": "bad", "tool": "finish"}, + {"step": 2, "tool_call_id": "bad", "tool": "finish_task"}, ), _record( 6, @@ -434,9 +437,9 @@ def test_protocol_violation_and_failed_tool_keep_line_references( { "step": 2, "tool_call_id": "bad", - "tool": "finish", + "tool": "finish_task", "duration_ms": 0, - "result": {"ok": False, "error": "duplicate finish calls"}, + "result": {"ok": False, "error": "duplicate finish_task calls"}, }, ), _record( @@ -453,9 +456,9 @@ def test_protocol_violation_and_failed_tool_keep_line_references( report = render_trace_report(path) assert "Plan: execute_tools [L3]" in report - assert "Protocol violation: duplicate finish calls [L4]" in report + assert "Protocol violation: duplicate finish_task calls [L4]" in report assert "Protocol reminder: use the required tool protocol [L7]" in report - assert "[error] finish 0ms [call L5 → result L6]" in report + assert "[error] finish_task 0ms [call L5 → result L6]" in report def test_step_verbose_events_and_flat_event_mode_use_physical_lines(