From 8373c208b549f0e689bbaad7926d14b3fd909224 Mon Sep 17 00:00:00 2001 From: Gen TANG Date: Tue, 4 Aug 2026 16:41:17 +0800 Subject: [PATCH 1/6] add algo for editing files --- docs/dev/editing-strategy.md | 1264 ++++++++++++++++++++++++++++++++++ 1 file changed, 1264 insertions(+) create mode 100644 docs/dev/editing-strategy.md diff --git a/docs/dev/editing-strategy.md b/docs/dev/editing-strategy.md new file mode 100644 index 0000000..e9b9020 --- /dev/null +++ b/docs/dev/editing-strategy.md @@ -0,0 +1,1264 @@ +# Editing Strategy and Recovery Algorithm + +## Status + +This document specifies the implementation algorithm for +[Issue #10: replace-first routing with apply_patch fallback](https://github.com/GenTang/Yada/issues/10). +It consolidates the routing, recovery, safety, bounded-retry, tracing, testing, +and evaluation rules needed to implement the issue. + +The key architectural decision is: + +> The model decides how to express an edit. Yada deterministically controls +> which editing operations are currently allowed, supplies safe recovery +> context, bounds retries, and terminates explicitly when progress cannot be +> made. + +This is a hybrid design: + +- the Agent retains flexibility for semantic decisions; +- Yada enforces safety and control-flow invariants in ordinary code; +- a rejected `replace_text` call is never silently converted into an + `apply_patch` call; +- every run finishes or fails in a finite number of steps. + +## 1. Scope + +### 1.1 Goals + +The implementation must: + +1. Support the run-level strategies `patch-only` and `replace-first`. +2. Keep the selected strategy, prompt, and tool schemas stable for the complete + run. +3. Prefer `replace_text` for localized changes to existing text when an exact, + unique anchor is available. +4. Use `apply_patch` for file creation, deletion, unsuitable structural edits, + and other operations that `replace_text` does not support. +5. Return a failed edit to the Agent before another edit is attempted. +6. Prevent stale or ambiguous replacement failures from causing blind fallback. +7. Preserve SHA binding, transactionality, touched-file accounting, revision + tracking, and post-edit verification. +8. Prevent infinite retry loops in both strategies. +9. Make routing and recovery paths reconstructable from traces. +10. Support deterministic tests with mocked model/tool interactions. +11. Add no runtime dependency. + +### 1.2 Non-goals + +This design does not add: + +- same-call automatic fallback; +- fuzzy or whitespace-normalized matching; +- automatic selection between ambiguous locations; +- AST-based edit routing; +- an external fast-apply model; +- a new public orchestration tool; +- a guarantee that the model can always generate a correct patch; +- removal of the `patch-only` compatibility baseline. + +## 2. Terminology + +### Agent turn + +One model response and the tool calls proposed by that response. + +### Editing operation + +A tool call that changes workspace files. In this issue, the editing operations +are: + +- `replace_text`; +- `apply_patch`. + +Earlier discussions sometimes call these operations *mutations*. This document +uses *editing operation* unless referring to the metric name from Issue #10. + +### Read-only recovery + +A bounded `read_file` operation performed to refresh the source content and +SHA after an edit failure. Read-only recovery does not change workspace files +and is not a fallback edit. + +### Fallback + +A later Agent turn deliberately choosing `apply_patch` after observing a +failed `replace_text` result and any required recovery context. + +### Recovery episode + +The interval beginning with one failed editing operation and ending when: + +- a later edit succeeds; +- the controller reaches a terminal failure; +- the run exhausts a recovery or protocol budget. + +### Progress + +An event that materially advances the run. The exact definition is given in +[Section 10](#10-bounded-retries-and-loop-prevention). + +## 3. Required invariants + +The implementation must preserve the following invariants. + +1. **Stable strategy:** `editing_strategy` cannot change after run start. +2. **Stable interface:** the prompt and tool schemas cannot change during the + run. +3. **Single edit per turn:** at most one editing operation may execute from one + Agent turn. +4. **Observed failure:** a failed edit must be present in the next model request + before a later edit may execute. +5. **Fresh context:** errors that require re-reading must have a post-failure + read snapshot visible to the model before the next accepted edit. +6. **No hidden edit:** automatic recovery may read files but may never generate + or execute `replace_text` or `apply_patch`. +7. **No blind fallback:** `stale_hash` and `ambiguous_match` may not directly + trigger an automatic patch. +8. **No side effects on failure:** a failed edit must not advance the workspace + revision, touched-file set, patch count, or verified revision. +9. **Verification invalidation:** a successful edit invalidates previous + verification. +10. **Transactional fallback:** a successful fallback uses the same SHA, + validation, transaction, and verification boundary as any other patch. +11. **Finite execution:** every retry path consumes a finite budget or reaches a + terminal state. +12. **Trace completeness:** the strategy, tool choice, result, error code, + recovery reads, state transitions, rejections, retries, and terminal reason + must be traceable. + +## 4. Responsibilities + +### 4.1 Model responsibilities + +The model is responsible for: + +- understanding the requested code change; +- deciding whether a localized exact replacement is suitable; +- constructing `old_text` and `new_text`; +- constructing a unified diff when using `apply_patch`; +- revising its plan after observing structured errors and refreshed content; +- running relevant verification before `finish`. + +### 4.2 Yada responsibilities + +Yada is responsible for: + +- selecting and freezing the run strategy; +- exposing the appropriate stable tool interface; +- enforcing one editing operation per Agent turn; +- authorizing or rejecting proposed editing operations according to recovery + state; +- performing deterministic read-only recovery when possible; +- rejecting unchanged retries; +- bounding patch, replace, protocol, and no-progress loops; +- preserving SHA and transactional guarantees; +- ending the run explicitly when recovery is exhausted; +- recording the complete control path in traces. + +### 4.3 Guarantee boundary + +Yada can guarantee: + +- that a disallowed edit does not execute; +- that a required re-read occurs before a later accepted edit; +- that `PATCH_REQUIRED` accepts no further `replace_text` edit; +- that retries are finite; +- that failure is explicit and auditable. + +Yada cannot guarantee: + +- that the model will call `apply_patch` when requested; +- that generated patch arguments are syntactically valid; +- that a valid patch solves the user task; +- that every run completes successfully. + +When the model cannot produce an allowed, valid edit within the budgets, the run +must end as `unfinished`; Yada must not manufacture a permissive edit behind the +Agent's back. + +## 5. State model + +The editing controller should be implemented as a small state machine whose +transition logic can be tested as a pure function. + +```python +from dataclasses import dataclass, field +from enum import Enum + + +class EditingStrategy(str, Enum): + PATCH_ONLY = "patch-only" + REPLACE_FIRST = "replace-first" + + +class RecoveryPhase(str, Enum): + NORMAL = "normal" + NEED_READ = "need-read" + NEED_MODEL_READ = "need-model-read" + READY_TO_REPLAN = "ready-to-replan" + REPLACE_RETRY_ALLOWED = "replace-retry-allowed" + PATCH_RETRY_ALLOWED = "patch-retry-allowed" + PATCH_REQUIRED = "patch-required" + TERMINAL_FAILURE = "terminal-failure" + + +@dataclass(frozen=True) +class ReadSnapshot: + path: str + sha256: str + start_line: int + end_line: int + available_to_model_at_step: int + + +@dataclass +class PendingRecovery: + failed_step: int + failed_tool_call_id: str + failed_tool: str + error_code: str + paths: tuple[str, ...] + arguments_fingerprint: str + phase: RecoveryPhase + corrected_retry_count: int = 0 + + +@dataclass +class EditingRunState: + strategy: EditingStrategy + frozen_tool_names: tuple[str, ...] + tool_schema_fingerprint: str + prompt_fingerprint: str + pending_recovery: PendingRecovery | None = None + last_reads: dict[str, ReadSnapshot] = field(default_factory=dict) + failure_counts: dict[tuple[int, str, str], int] = field(default_factory=dict) + protocol_violation_count: int = 0 + no_progress_turns: int = 0 + edit_failures_this_revision: int = 0 + workspace_revision: int = 0 +``` + +The recommended interface is: + +```python +transition(state, event) -> tuple[new_state, actions] +``` + +Representative events include: + +- `RunStarted`; +- `ModelTurnStarted`; +- `ToolCallProposed`; +- `ReadCompleted`; +- `EditSucceeded`; +- `EditFailed`; +- `ToolCallRejected`; +- `VerificationSucceeded`; +- `BudgetExhausted`. + +Representative actions include: + +- `ExecuteTool`; +- `PerformRecoveryRead`; +- `RejectToolCall`; +- `AppendObservation`; +- `WriteTraceEvent`; +- `EndRun`. + +## 6. Run initialization + +### 6.1 Strategy selection + +The CLI and evaluation adapter must accept: + +```text +--editing-strategy patch-only +--editing-strategy replace-first +``` + +`patch-only` remains the default until benchmark evidence justifies changing +it. + +### 6.2 Stable tool exposure + +For `patch-only`, expose: + +```text +search_code +read_file +apply_patch +run_command +finish +``` + +For `replace-first`, expose: + +```text +search_code +read_file +replace_text +apply_patch +run_command +finish +``` + +The tool collection must be created once during initialization. Recovery state +must not dynamically remove, reorder, or redefine tools. For example, +`PATCH_REQUIRED` leaves the schemas unchanged but deterministically rejects a +later `replace_text` call. + +### 6.3 Stable prompt + +The strategy-specific system prompt is also created once. + +`patch-only` instructions state that all workspace edits use `apply_patch`. + +`replace-first` instructions state: + +- use `replace_text` for an existing regular text file when the change is local + and `old_text` is exact and unique; +- use `apply_patch` directly for creation, deletion, rename, large structural + rewrite, impractically large anchors, or unsupported targets; +- follow the structured recovery matrix; +- never treat a failed replacement as permission for same-turn fallback. + +### 6.4 Run-start trace + +The `run_start` event must contain at least: + +```json +{ + "editing_strategy": "replace-first", + "tool_names": [ + "search_code", + "read_file", + "replace_text", + "apply_patch", + "run_command", + "finish" + ], + "tool_schema_fingerprint": "...", + "prompt_fingerprint": "..." +} +``` + +The fingerprints make strategy comparisons auditable and detect accidental +mid-run drift. + +## 7. Routing algorithm + +### 7.1 `patch-only` + +`replace_text` is not exposed. If an edit occurs, the model must express it as +`apply_patch`. + +This guarantees which public editing tool is available, but it does not +guarantee that the model can generate a valid patch. Patch recovery and bounded +failure are therefore required; see [Section 11](#11-patch-only-control-flow). + +### 7.2 `replace-first` + +The model applies the following decision rule: + +```python +def preferred_editing_tool(intent): + if intent.creates_file: + return "apply_patch" + if intent.deletes_or_renames_file: + return "apply_patch" + if intent.is_large_structural_rewrite: + return "apply_patch" + if intent.requires_impractically_large_anchor: + return "apply_patch" + if intent.target_is_unsupported_by_replace: + return "apply_patch" + + if ( + intent.targets_existing_regular_text + and intent.is_localized + and intent.has_exact_unique_anchor + ): + return "replace_text" + + return "apply_patch" +``` + +The semantic predicates such as `is_localized` are model judgments guided by +the prompt. The tools remain the deterministic backstop: + +- `replace_text` validates file type, UTF-8, SHA, exact match count, limits, and + transactionality; +- `apply_patch` validates declared targets, SHA, paths, patch syntax, context, + and transactionality. + +The framework must not add a second model or AST router merely to classify the +edit; those approaches are outside Issue #10. + +## 8. Agent-turn execution algorithm + +### 8.1 Batch validation + +Define: + +```python +EDITING_TOOLS = {"replace_text", "apply_patch"} +``` + +Before executing a model response: + +```python +editing_calls = [ + call for call in tool_calls + if call.name in EDITING_TOOLS +] + +if len(editing_calls) > 1: + reject_editing_batch( + error_code="multiple_edit_operations", + error="Only one editing operation is allowed per Agent turn.", + ) +``` + +The rejected editing calls produce no side effects. This prevents the sequence +below from executing in one turn: + +```text +replace_text -> failure -> apply_patch +``` + +because the model could not have observed the replacement failure before it +generated the patch call. + +### 8.2 Recovery authorization + +Before an editing call executes, the controller checks: + +1. Is this editing tool allowed by the current recovery phase? +2. Has every required post-failure read become visible to the model? +3. Are the arguments different from an already rejected attempt? +4. Is the relevant retry budget still available? +5. Is the run already in a terminal state? + +If any condition fails, the call is rejected with a structured observation and +no edit is executed. + +### 8.3 Read visibility + +A model-generated read and edit in the same turn cannot satisfy a recovery +precondition: the model generated the edit before seeing the read result. + +For that reason, a read snapshot records: + +```text +available_to_model_at_step +``` + +An edit in step `N` may use a recovery snapshot only if: + +```python +snapshot.available_to_model_at_step <= N +``` + +An automatic recovery read performed between steps `N - 1` and `N` is visible +in step `N`. A `read_file` proposed by the model in step `N` becomes visible no +earlier than step `N + 1`. + +## 9. Edit results and recovery + +### 9.1 Success + +On success: + +```python +state.workspace_revision += 1 +state.pending_recovery = None +state.edit_failures_this_revision = 0 +state.no_progress_turns = 0 +context.state.verified_revision = -1 +context.state.touched_files.update(changed_paths) +``` + +The result includes the new revision, changed paths, and post-edit hashes. A +relevant test or build must succeed at the new revision before `finish`. + +### 9.2 General failure procedure + +For an editing failure: + +1. Preserve the original structured error and bounded details. +2. Do not advance workspace or verification state. +3. Increment the per-revision edit failure counter. +4. Create a `PendingRecovery` object. +5. Determine whether read-only recovery is required. +6. Perform bounded automatic reads when a reliable range is available. +7. Attach the recovery context to the observation for the next model turn. +8. Transition to the error-specific recovery phase. +9. Never execute another edit from the same Agent turn. + +Conceptually: + +```text +edit failure + -> structured result + -> PendingRecovery + -> optional automatic read-only recovery + -> failure + recovery context in next model request + -> later model decision +``` + +### 9.3 Automatic read-only recovery + +Automatic reads strengthen the prompt-only recovery policy without violating +Issue #10: they do not edit the workspace, and fallback still happens only in a +later Agent turn after the model observes the failure. + +The controller should retain the range of every successful `read_file` call. + +Recommended recovery ranges: + +- `stale_hash`: re-read the last ranges used for each affected path and obtain + the current SHA; +- `no_match`: re-read the last ranges from which the proposed anchor was + derived; +- `ambiguous_match`: read bounded windows around the returned match line + numbers; +- `patch_context_mismatch`: read bounded windows derived from the failed patch + hunks. + +If no reliable bounded range is available, set `NEED_MODEL_READ`. In this phase, +editing calls are rejected until a model-requested `read_file` result has become +visible in a later turn. Reading an arbitrary part of a large file must not be +treated as sufficient merely to satisfy the state machine. + +An observation may contain: + +```json +{ + "ok": false, + "error_code": "no_match", + "error": "old_text was not found in src/app.py", + "details": { + "paths": ["src/app.py"] + }, + "recovery_context": { + "action": "reread", + "reads": [ + { + "path": "src/app.py", + "sha256": "current-sha256", + "start_line": 70, + "end_line": 130, + "content": "..." + } + ] + } +} +``` + +### 9.4 Error transition matrix + +The matrix covers errors from Issue #10 and `invalid_patch` from its dependency, +Issue #8. + +| Error code | Deterministic Yada action | Next accepted editing behavior | Exhaustion behavior | +|---|---|---|---| +| `stale_hash` | Re-read affected ranges and return current SHA | Re-plan from fresh content; do not automatically patch | Repeated staleness becomes `concurrent_modification` | +| `no_match` | Re-read the source range used for the anchor | Retry with a new exact anchor or deliberately generate a patch | A second corrected `no_match` enters `PATCH_REQUIRED` | +| `ambiguous_match` | Read windows around all reported matches | Retry `replace_text` with a larger unique anchor | Persistent ambiguity becomes `unresolved_ambiguity`; no blind patch | +| `invalid_edit` | Return validation details; no automatic read | Correct arguments and retry | Repeated unchanged/invalid arguments exhaust the protocol budget | +| `invalid_patch` | Return patch syntax diagnostics | Regenerate the patch once | Persistent invalidity becomes `patch_retry_exhausted` | +| `unsupported_target` | Determine whether `apply_patch` supports the requested operation | Enter `PATCH_REQUIRED` only when patching is valid | Otherwise terminate as `unsupported_operation` | +| `patch_context_mismatch` | Re-read affected hunk ranges | Regenerate `apply_patch` once | Persistent mismatch becomes `patch_retry_exhausted` | +| `apply_failed` | Preserve complete bounded diagnostics | No automatic recovery | Immediate `terminal_edit_failure` | + +### 9.5 Unchanged retry detection + +Canonicalize and hash editing arguments. A failed attempt key should include: + +```python +attempt_key = ( + state.workspace_revision, + tool_name, + canonical_arguments_fingerprint, + tuple(sorted(affected_paths)), +) +``` + +If the same attempt is proposed again at the same revision, reject it without +re-running the editing tool: + +```json +{ + "ok": false, + "error_code": "unchanged_retry", + "details": { + "recovery": "Use the refreshed content to construct different arguments." + } +} +``` + +## 10. Bounded retries and loop prevention + +Neither strategy inherently prevents loops. A model may repeatedly generate an +invalid patch, repeatedly choose an unsuitable replacement, alternate between +tools, alternate between error codes, or avoid editing entirely. Loop prevention +must therefore be strategy-independent. + +### 10.1 Recommended initial budgets + +The exact values may be tuned by benchmark evidence, but they must be finite, +recorded in `run_start`, and covered by tests. + +```python +MAX_CORRECTED_REPLACE_RETRIES = 1 +MAX_REGENERATED_PATCH_RETRIES = 1 +MAX_STALE_REFRESHES = 2 +MAX_EDIT_FAILURES_PER_REVISION = 4 +MAX_RECOVERY_PROTOCOL_VIOLATIONS = 2 +MAX_NO_PROGRESS_TURNS = 3 +``` + +The existing `max_steps` remains the global final bound. + +### 10.2 Why several budgets are required + +Per-error budgets alone are insufficient. A model could alternate: + +```text +invalid_patch +-> patch_context_mismatch +-> stale_hash +-> invalid_patch +-> ... +``` + +The per-revision failure budget closes this loophole because every failed edit +at the same workspace revision consumes the same global edit-failure budget, +regardless of tool or error code. + +The protocol budget covers calls that are rejected before execution, such as: + +- proposing more than one editing operation in one turn; +- retrying identical failed arguments; +- proposing `replace_text` in `PATCH_REQUIRED`; +- attempting to bypass ambiguity recovery; +- proposing an edit before the required read is visible. + +The no-progress budget covers turns in which the model: + +- emits text without an actionable tool call; +- repeatedly reads the same range at the same SHA; +- performs unrelated searches; +- proposes only rejected editing operations; +- changes error types without advancing the recovery phase. + +### 10.3 What counts as progress + +The no-progress counter resets only for a meaningful event: + +1. a successful edit increments the workspace revision; +2. a required read returns a new SHA; +3. a read completes a pending recovery requirement; +4. the recovery phase advances monotonically, for example + `NEED_READ -> READY_TO_REPLAN -> PATCH_REQUIRED`; +5. a relevant test or build succeeds for the latest revision. + +The following do not count as progress: + +- submitting a different malformed patch; +- changing from one edit error code to another at the same revision; +- repeating the same read range and SHA; +- a rejected call; +- an irrelevant inspection command; +- a text-only claim of completion. + +### 10.4 Termination function + +```python +def terminal_reason(state, *, max_steps): + if state.pending_recovery is not None: + if state.pending_recovery.phase == RecoveryPhase.TERMINAL_FAILURE: + return state.pending_recovery.error_code + + if state.edit_failures_this_revision >= MAX_EDIT_FAILURES_PER_REVISION: + return "edit_failure_budget_exhausted" + + if state.protocol_violation_count >= MAX_RECOVERY_PROTOCOL_VIOLATIONS: + return "recovery_protocol_exhausted" + + if state.no_progress_turns >= MAX_NO_PROGRESS_TURNS: + return "no_progress" + + if current_step >= max_steps: + return "max_steps_exhausted" + + return None +``` + +### 10.5 Finite-termination argument + +Consider the finite budget vector: + +```text +( + remaining steps, + remaining edit failures for the current revision, + remaining recovery retries, + remaining protocol violations, + remaining no-progress turns +) +``` + +Every Agent turn either: + +- makes genuine progress; +- decreases at least one finite budget; +- reaches a terminal state. + +The global remaining-step count decreases on every model turn. Therefore a run +cannot execute indefinitely: it eventually finishes successfully or ends with +an explicit `unfinished` reason. + +## 11. `patch-only` control flow + +`patch-only` does not have an alternate public editing tool, so repeated patch +failure must be bounded directly. + +### 11.1 Correctable patch failures + +- `invalid_patch`: return bounded parser diagnostics and allow one regenerated + patch; +- `stale_hash`: refresh the relevant source and allow a patch using the current + SHA; +- `patch_context_mismatch`: refresh hunk ranges and allow one regenerated patch. + +### 11.2 Non-correctable or exhausted failures + +- unsupported paths or target types terminate explicitly; +- `apply_failed` terminates immediately; +- a second corrected syntax/context failure terminates with + `patch_retry_exhausted`; +- identical patch arguments are rejected without executing `git apply`; +- cross-error alternation is stopped by `MAX_EDIT_FAILURES_PER_REVISION`; +- refusal to generate a patch is stopped by protocol, no-progress, or global + step budgets. + +The guarantee is not that patching always succeeds. The guarantee is: + +```text +a patch succeeds, or patch-only ends explicitly within finite budgets +``` + +## 12. `replace-first` control flow + +### 12.1 Successful replacement + +A successful `replace_text` edit completes through the existing transactional +patch boundary. The public `apply_patch` tool does not need to appear in every +run; retaining both tools in the architecture does not require using both in +each task. + +### 12.2 `no_match` + +```text +replace_text -> no_match +-> automatic bounded re-read +-> next model turn sees failure and current source +-> one corrected replace or a deliberate patch +``` + +If the corrected replacement again returns `no_match`, transition to +`PATCH_REQUIRED`. From that point, `replace_text` remains visible in the stable +schema but is rejected by the controller. + +### 12.3 `ambiguous_match` + +```text +replace_text -> ambiguous_match +-> read windows around reported matches +-> require a larger exact unique anchor +-> allow one corrected replacement +``` + +If the corrected replacement remains ambiguous, terminate as +`unresolved_ambiguity`. Do not automatically require or execute a patch merely +to escape the loop, because that could bypass the evidence that the edit target +is not unique. + +### 12.4 `unsupported_target` + +If the requested operation is valid for `apply_patch`, enter `PATCH_REQUIRED`. +If the target is prohibited for both tools, such as a protected or escaping +path, terminate as `unsupported_operation`. + +### 12.5 Patch fallback failures + +After transitioning to `PATCH_REQUIRED`, all patch attempts are governed by the +same recovery and retry budgets as `patch-only`. This prevents a replacement +loop from merely turning into a patch loop. + +The guarantee is: + +```text +replace succeeds, +or a deliberate bounded patch succeeds, +or replace-first ends explicitly within finite budgets +``` + +## 13. End-to-end pseudocode + +```python +def run(task, strategy): + state = initialize_frozen_editing_state(strategy) + messages = build_initial_messages(task, strategy) + trace_run_start(state) + + for step in range(1, max_steps + 1): + response = model.complete( + messages=messages, + tools=state.frozen_tool_schemas, + ) + plan = planner.plan(response) + editing_calls = [ + call + for call in plan.tool_calls + if call.name in {"replace_text", "apply_patch"} + ] + + if len(editing_calls) > 1: + results = reject_multiple_edit_operations(plan.tool_calls) + state.protocol_violation_count += 1 + state.no_progress_turns += 1 + messages.extend(tool_results_to_messages(results)) + if reason := terminal_reason(state, max_steps=max_steps): + return unfinished(reason) + continue + + results = [] + turn_made_progress = False + + for call in plan.tool_calls: + if call.name not in {"replace_text", "apply_patch"}: + result = execute_non_editing_tool(call) + progress = record_observation_if_relevant( + state, + call, + result, + step, + ) + turn_made_progress = turn_made_progress or progress + results.append(result) + continue + + authorization = authorize_editing_call(state, call, step) + if not authorization.allowed: + state.protocol_violation_count += 1 + results.append(authorization.rejection) + continue + + if is_unchanged_retry(state, call): + state.protocol_violation_count += 1 + results.append(unchanged_retry_observation(call)) + continue + + result = execute_editing_tool(call) + trace_tool_result(call, result) + + if result.ok: + on_edit_success(state, call, result) + turn_made_progress = True + results.append(result) + continue + + state.edit_failures_this_revision += 1 + recovery = create_pending_recovery(step, call, result) + state.pending_recovery = recovery + + recovery_reads = perform_safe_recovery_reads( + state=state, + recovery=recovery, + visible_to_model_at_step=step + 1, + ) + phase_advanced = transition_after_failure( + state, + recovery, + recovery_reads, + ) + turn_made_progress = turn_made_progress or phase_advanced + results.append(attach_recovery_context(result, recovery_reads)) + + state.no_progress_turns = ( + 0 if turn_made_progress else state.no_progress_turns + 1 + ) + + messages.append(response.message) + messages.extend(tool_results_to_messages(results)) + trace_recovery_state(state) + + if reason := terminal_reason(state, max_steps=max_steps): + return unfinished(reason) + + if a_finish_result_succeeded(results): + return finished(results) + + return unfinished("max_steps_exhausted") +``` + +## 14. Overall flowchart + +```mermaid +flowchart TD + A["Start run"] --> B{"Select editing strategy"} + B -- "patch-only" --> C["Freeze tools: apply_patch only"] + B -- "replace-first" --> D["Freeze tools: replace_text and apply_patch"] + C --> E["Freeze prompt, schemas, and fingerprints"] + D --> E + E --> F["Write run_start trace"] + F --> G["Request next model turn"] + + G --> H["Planner parses tool calls"] + H --> I{"More than one editing operation?"} + I -- "Yes" --> J["Reject editing batch; consume protocol budget"] + J --> K{"Budget exhausted?"} + K -- "Yes" --> L["End unfinished with explicit reason"] + K -- "No" --> G + + I -- "No" --> M{"Contains an editing operation?"} + M -- "No" --> N["Execute read, search, test, or finish"] + N --> O["Record observations and progress"] + O --> P{"Finished or budget exhausted?"} + P -- "Finished" --> Q["End finished"] + P -- "Exhausted" --> L + P -- "Continue" --> G + + M -- "Yes" --> R{"Controller authorizes this edit?"} + R -- "No" --> S["Reject call; consume protocol/no-progress budget"] + S --> K + R -- "Yes" --> T["Execute replace_text or apply_patch"] + T --> U{"Edit succeeded?"} + + U -- "Yes" --> V["Advance revision and invalidate verification"] + V --> W["Require relevant test or build"] + W --> G + + U -- "No" --> X["Preserve structured error and arguments fingerprint"] + X --> Y["Create PendingRecovery"] + Y --> Z{"Read-only recovery required?"} + Z -- "Yes" --> AA["Perform bounded recovery reads"] + Z -- "No" --> AB["Skip automatic read"] + AA --> AC["Attach failure and recovery context"] + AB --> AC + AC --> AD["Transition recovery phase and consume retry budget"] + AD --> AE{"Terminal recovery state?"} + AE -- "Yes" --> L + AE -- "No" --> G +``` + +## 15. `patch-only` flowchart + +```mermaid +flowchart TD + A["Model proposes apply_patch"] --> B{"Controller authorizes call?"} + B -- "No" --> C["Reject and consume protocol budget"] + C --> D{"Protocol/no-progress budget exhausted?"} + D -- "Yes" --> E["End unfinished"] + D -- "No" --> A + + B -- "Yes" --> F["Execute apply_patch"] + F --> G{"Result"} + G -- "Success" --> H["Advance revision; require verification"] + G -- "invalid_patch" --> I{"Regenerated patch retry available?"} + I -- "Yes" --> J["Return syntax diagnostics to next model turn"] + J --> A + I -- "No" --> K["End: patch_retry_exhausted"] + + G -- "stale_hash" --> L["Refresh source ranges and SHA"] + L --> M{"Stale refresh budget available?"} + M -- "Yes" --> A + M -- "No" --> N["End: concurrent_modification"] + + G -- "patch_context_mismatch" --> O["Refresh affected hunk ranges"] + O --> P{"Regenerated patch retry available?"} + P -- "Yes" --> A + P -- "No" --> K + + G -- "unsupported_target" --> Q["End: unsupported_operation"] + G -- "apply_failed" --> R["End: terminal_edit_failure"] +``` + +## 16. `replace-first` flowchart + +```mermaid +flowchart TD + A["Model evaluates requested edit"] --> B{"Existing text, localized, exact unique anchor?"} + B -- "No" --> C["Propose apply_patch"] + B -- "Yes" --> D["Propose replace_text"] + + D --> E{"replace_text result"} + E -- "Success" --> F["Advance revision; require verification"] + E -- "stale_hash" --> G["Refresh content and SHA"] + G --> H{"Stale refresh budget available?"} + H -- "Yes" --> A + H -- "No" --> I["End: concurrent_modification"] + + E -- "no_match" --> J["Refresh source range"] + J --> K{"Corrected replace already attempted?"} + K -- "No" --> L["Next turn: corrected replace or deliberate patch"] + L --> A + K -- "Yes" --> M["Enter PATCH_REQUIRED"] + + E -- "ambiguous_match" --> N["Read windows around all matches"] + N --> O{"Expanded-anchor retry already attempted?"} + O -- "No" --> P["Require one larger exact unique anchor"] + P --> D + O -- "Yes" --> Q["End: unresolved_ambiguity"] + + E -- "invalid_edit" --> R["Return validation details"] + R --> S{"Arguments changed and retry available?"} + S -- "Yes" --> D + S -- "No" --> T["Reject or end by protocol budget"] + + E -- "unsupported_target" --> U{"Operation valid for apply_patch?"} + U -- "No" --> V["End: unsupported_operation"] + U -- "Yes" --> M + + M --> W{"Next proposed editing operation"} + W -- "replace_text" --> X["Reject: patch_required"] + X --> Y{"Protocol budget exhausted?"} + Y -- "Yes" --> Z["End: recovery_protocol_exhausted"] + Y -- "No" --> W + W -- "apply_patch" --> C + + C --> AA["Use patch-only recovery and retry rules"] + AA --> AB{"Patch succeeds?"} + AB -- "Yes" --> F + AB -- "No and budget exhausted" --> AC["End: patch_retry_exhausted"] +``` + +## 17. Trace requirements + +In addition to existing model, tool-call, and tool-result events, the controller +should record: + +### `recovery_started` + +```json +{ + "step": 4, + "failed_tool_call_id": "call-replace-4", + "failed_tool": "replace_text", + "error_code": "no_match", + "paths": ["src/app.py"], + "phase": "need-read" +} +``` + +### `recovery_read` + +```json +{ + "step": 4, + "triggered_by_tool_call_id": "call-replace-4", + "paths": ["src/app.py"], + "ranges": [{"start_line": 70, "end_line": 130}], + "visible_to_model_at_step": 5 +} +``` + +### `recovery_transition` + +```json +{ + "step": 4, + "from": "need-read", + "to": "ready-to-replan", + "error_code": "no_match", + "corrected_retry_count": 0 +} +``` + +### `editing_call_rejected` + +```json +{ + "step": 6, + "tool": "replace_text", + "error_code": "patch_required", + "protocol_violation_count": 1 +} +``` + +### `run_end` + +An unfinished run must include a stable reason and the final controller state: + +```json +{ + "finished": false, + "status": "unfinished", + "reason": "patch_retry_exhausted", + "editing_strategy": "replace-first", + "steps": 7, + "workspace_revision": 0, + "edit_attempts": 3, + "last_error_code": "patch_context_mismatch" +} +``` + +Trace content must remain bounded and follow the existing summary/debug +redaction rules. Full source content should not be added to summary traces merely +because it was read for recovery. + +## 18. Evaluation algorithm + +Compare `patch-only` and `replace-first` as two policies over the same underlying +editing implementations. + +For every paired comparison, hold constant: + +- task set and base commits; +- model and model parameters; +- thinking/reasoning settings; +- tool schemas and prompt fingerprints within each strategy; +- step, token, command, and wall-time budgets; +- grading logic; +- retry and no-progress budgets. + +Repeat trials when model nondeterminism makes a single run unreliable. + +Derive at least these metrics from traces and grader results: + +- first edit-attempt success rate; +- eventual editing/mutation success rate; +- completed/resolved task rate; +- replace and patch retry counts; +- recovery-protocol rejection counts; +- Agent turns and total steps; +- input and output tokens; +- successful verification after the latest edit; +- unrelated changed lines, using a benchmark reference or explicit allowed + ranges; +- wrong-target and partial-target edits, which must remain zero; +- terminal-reason distribution, including patch retry exhaustion, unresolved + ambiguity, concurrent modification, protocol exhaustion, no progress, and + max-step exhaustion. + +The comparison validates routing quality and end-to-end efficiency. It must not +replace either editing implementation or remove the `patch-only` baseline. + +## 19. Deterministic test matrix + +At minimum, mocked Agent/tool tests must cover: + +### Strategy and interface + +- `patch-only` exposes no `replace_text` schema; +- `replace-first` exposes both edit schemas; +- schemas and prompt fingerprints remain identical across all model requests in + a run; +- `run_start` records strategy and fingerprints. + +### Turn isolation + +- a turn containing `replace_text` and `apply_patch` executes neither edit; +- a failed edit is present in the next model request; +- a model-generated read and edit in the same turn cannot satisfy a pending + recovery read. + +### Replacement recovery + +- `stale_hash` performs a bounded re-read before retry authorization; +- `no_match` permits one corrected replace or a deliberate patch after a fresh + read; +- a second corrected `no_match` enters `PATCH_REQUIRED`; +- `ambiguous_match` requires a larger anchor and never triggers blind patch; +- persistent ambiguity terminates explicitly; +- `invalid_edit` with unchanged arguments is rejected without execution; +- `unsupported_target` enters `PATCH_REQUIRED` only for patch-supported + operations. + +### Patch recovery + +- `invalid_patch` permits one regenerated patch; +- `patch_context_mismatch` re-reads hunk ranges before retry; +- persistent invalid/context-mismatched patch terminates; +- repeated identical patch arguments are not executed twice; +- `apply_failed` terminates and preserves diagnostics. + +### Loop prevention + +- alternating replace and patch failures consume the per-revision failure + budget; +- alternating error codes cannot evade the global failure budget; +- repeated disallowed tools consume the protocol budget; +- repeated identical reads do not reset the no-progress budget; +- a successful edit resets per-revision recovery counters; +- every mocked loop reaches a stable finished or unfinished result in finite + steps. + +### Safety and verification + +- failed edits do not change revision, files, touched paths, or verification; +- successful fallback remains SHA-bound and transactional; +- successful fallback invalidates previous verification; +- `finish` remains unavailable until a relevant post-edit test/build succeeds. + +## 20. Suggested implementation boundaries + +The state machine should remain small and independent of tool implementation +details. + +Suggested integration points: + +- `src/yada/editing.py`: enums, state records, budgets, transition and + authorization logic; +- `src/yada/tools/schemas.py`: strategy-specific stable schema construction; +- `src/yada/tools/runner.py`: frozen strategy, schemas, handlers, and direct + execution boundary; +- `src/yada/agents/prompts.py`: strategy-specific routing and recovery + instructions; +- `src/yada/agents/planning.py`: one-edit-per-turn batch validation; +- `src/yada/agents/executor.py`: authorization, recovery reads, result wrapping, + and trace events; +- `src/yada/agents/default.py`: run-start metadata, controller lifetime, message + loop, progress and terminal checks; +- `src/yada/evals/`: strategy plumbing, trace metric extraction, comparison, + and reporting. + +`replace_text` and `apply_patch` remain the data-plane editing implementations. +They should not contain model-routing logic or silently call each other as a +fallback. `replace_text` may continue to reuse the validated patch application +boundary internally for transactional file application. + +## 21. Industry positioning + +There is no single industry-standard algorithm for deciding when a coding Agent +should replace text or generate a patch. The broadly established pattern is to +combine: + +- exact, unique replacement for targeted edits; +- optimistic concurrency checks for writes; +- transactional and fail-closed editing; +- structured tool errors; +- a deterministic host policy around model-proposed actions; +- bounded retries, timeouts, and explicit terminal states; +- trace-based evaluation over repeated trials. + +Relevant examples and references: + +- [Gemini CLI file tools](https://github.com/google-gemini/gemini-cli/blob/main/docs/tools/file-system.md) + use exact targeted replacement and default to one occurrence; +- [Claude text editor](https://platform.claude.com/docs/en/agents-and-tools/tool-use/text-editor-tool) + uses exact `str_replace` semantics; +- [RFC 9110, If-Match](https://www.rfc-editor.org/rfc/rfc9110.html#section-13.1.1) + describes strong preconditions used to prevent lost updates; +- [Gemini CLI policy engine](https://github.com/google-gemini/gemini-cli/blob/main/docs/reference/policy-engine.md) + evaluates model-proposed tool calls using deterministic host rules; +- [LangGraph workflows and agents](https://docs.langchain.com/oss/python/langgraph/workflows-agents) + distinguishes predetermined workflow control from dynamic Agent decisions; +- [Anthropic Agent Evals](https://www.anthropic.com/engineering/demystifying-evals-for-ai-agents) + recommends repeated trials, trace inspection, and multiple evaluation layers. + +The algorithm in this document follows that hybrid pattern while preserving the +specific safety and evaluation constraints of Issue #10. From df87ab671ba135e6a6668b57c675debdb0a77137 Mon Sep 17 00:00:00 2001 From: Gen TANG Date: Tue, 4 Aug 2026 20:23:47 +0800 Subject: [PATCH 2/6] make editing-strategy simpler --- docs/dev/editing-strategy.md | 1562 ++++++++++------------------------ 1 file changed, 439 insertions(+), 1123 deletions(-) diff --git a/docs/dev/editing-strategy.md b/docs/dev/editing-strategy.md index e9b9020..eb558a4 100644 --- a/docs/dev/editing-strategy.md +++ b/docs/dev/editing-strategy.md @@ -1,333 +1,131 @@ -# Editing Strategy and Recovery Algorithm +# Editing Strategies and Recovery Policy ## Status -This document specifies the implementation algorithm for +This document is the implementation contract for [Issue #10: replace-first routing with apply_patch fallback](https://github.com/GenTang/Yada/issues/10). -It consolidates the routing, recovery, safety, bounded-retry, tracing, testing, -and evaluation rules needed to implement the issue. +It describes the behavior that must remain stable across prompts, tools, traces, +tests, and evaluations. It intentionally does not prescribe Python classes or +duplicate the Agent loop as near-executable pseudocode. -The key architectural decision is: +The central rule is: -> The model decides how to express an edit. Yada deterministically controls -> which editing operations are currently allowed, supplies safe recovery -> context, bounds retries, and terminates explicitly when progress cannot be -> made. - -This is a hybrid design: - -- the Agent retains flexibility for semantic decisions; -- Yada enforces safety and control-flow invariants in ordinary code; -- a rejected `replace_text` call is never silently converted into an - `apply_patch` call; -- every run finishes or fails in a finite number of steps. +> The model chooses how to express an edit. Yada exposes a stable strategy, +> executes edits through existing fail-closed tools, and ensures that a failed +> edit is observed by the model before another edit can run. ## 1. Scope ### 1.1 Goals -The implementation must: - -1. Support the run-level strategies `patch-only` and `replace-first`. -2. Keep the selected strategy, prompt, and tool schemas stable for the complete - run. -3. Prefer `replace_text` for localized changes to existing text when an exact, - unique anchor is available. -4. Use `apply_patch` for file creation, deletion, unsuitable structural edits, - and other operations that `replace_text` does not support. -5. Return a failed edit to the Agent before another edit is attempted. -6. Prevent stale or ambiguous replacement failures from causing blind fallback. -7. Preserve SHA binding, transactionality, touched-file accounting, revision - tracking, and post-edit verification. -8. Prevent infinite retry loops in both strategies. -9. Make routing and recovery paths reconstructable from traces. -10. Support deterministic tests with mocked model/tool interactions. -11. Add no runtime dependency. +Issue #10 adds: + +1. Explicit run-level patch-only and replace-first strategies. +2. A stable tool interface and strategy prompt for the complete run. +3. A documented rule for choosing replace_text or apply_patch. +4. A later-turn recovery policy for structured edit failures. +5. A small execution barrier that prevents same-turn fallback. +6. Trace fields and evaluation metrics for comparing both strategies. +7. Deterministic tests using mocked model and tool interactions. + +The existing SHA checks, transactional application, revision accounting, and +verification gate remain unchanged. ### 1.2 Non-goals -This design does not add: +Issue #10 does not add: -- same-call automatic fallback; - fuzzy or whitespace-normalized matching; -- automatic selection between ambiguous locations; -- AST-based edit routing; -- an external fast-apply model; -- a new public orchestration tool; -- a guarantee that the model can always generate a correct patch; -- removal of the `patch-only` compatibility baseline. +- automatic selection between ambiguous matches; +- AST-based routing; +- an external edit model; +- a public orchestration tool; +- framework-generated patches; +- automatic conversion of a failed replacement into a patch; +- a guarantee that the model will eventually produce a valid edit. -## 2. Terminology +## 2. Terms ### Agent turn -One model response and the tool calls proposed by that response. +One assistant response and the tool calls proposed by that response. ### Editing operation -A tool call that changes workspace files. In this issue, the editing operations -are: - -- `replace_text`; -- `apply_patch`. - -Earlier discussions sometimes call these operations *mutations*. This document -uses *editing operation* unless referring to the metric name from Issue #10. - -### Read-only recovery - -A bounded `read_file` operation performed to refresh the source content and -SHA after an edit failure. Read-only recovery does not change workspace files -and is not a fallback edit. +A replace_text or apply_patch call. Earlier discussions use mutation for the +same concept: an operation that changes workspace files. ### Fallback -A later Agent turn deliberately choosing `apply_patch` after observing a -failed `replace_text` result and any required recovery context. - -### Recovery episode - -The interval beginning with one failed editing operation and ending when: +An apply_patch call deliberately proposed by the model in a later Agent turn +after it has observed a failed replace_text result and any required fresh read. -- a later edit succeeds; -- the controller reaches a terminal failure; -- the run exhausts a recovery or protocol budget. +Fallback is not an internal call from replace_text to apply_patch. The +replace_text implementation may continue to reuse the validated patch boundary +to commit an already validated replacement transaction; that is an +implementation detail, not strategy fallback. -### Progress +### Recovery -An event that materially advances the run. The exact definition is given in -[Section 10](#10-bounded-retries-and-loop-prevention). +The model observes a structured edit error, gathers required current context, +and proposes a corrected edit in a later turn. ## 3. Required invariants -The implementation must preserve the following invariants. - -1. **Stable strategy:** `editing_strategy` cannot change after run start. -2. **Stable interface:** the prompt and tool schemas cannot change during the - run. -3. **Single edit per turn:** at most one editing operation may execute from one - Agent turn. -4. **Observed failure:** a failed edit must be present in the next model request - before a later edit may execute. -5. **Fresh context:** errors that require re-reading must have a post-failure - read snapshot visible to the model before the next accepted edit. -6. **No hidden edit:** automatic recovery may read files but may never generate - or execute `replace_text` or `apply_patch`. -7. **No blind fallback:** `stale_hash` and `ambiguous_match` may not directly - trigger an automatic patch. -8. **No side effects on failure:** a failed edit must not advance the workspace - revision, touched-file set, patch count, or verified revision. -9. **Verification invalidation:** a successful edit invalidates previous +The implementation must preserve these invariants: + +1. **Stable strategy:** editing_strategy is selected at run start and cannot + change during the run. +2. **Stable interface:** the prompt and exposed tool schemas do not change + during the run. +3. **Model-owned routing:** Yada does not use a second model, AST router, or + hidden conversion to choose an editing representation. +4. **One edit per turn:** at most one editing operation may execute from one + assistant response. +5. **Observed failure:** a failed edit result is included in a later model + request before another edit may execute. +6. **Read visibility:** a read and an edit generated in the same assistant + response cannot use that read as recovery evidence. The model had not seen + the read result when it generated the edit. +7. **No hidden fallback:** a failed replace_text never causes Yada to generate + or execute apply_patch. +8. **Fail closed:** a failed edit does not change files, revision, touched-file + accounting, or verification state. +9. **Verification after success:** every successful edit invalidates previous verification. -10. **Transactional fallback:** a successful fallback uses the same SHA, - validation, transaction, and verification boundary as any other patch. -11. **Finite execution:** every retry path consumes a finite budget or reaches a - terminal state. -12. **Trace completeness:** the strategy, tool choice, result, error code, - recovery reads, state transitions, rejections, retries, and terminal reason - must be traceable. - -## 4. Responsibilities - -### 4.1 Model responsibilities - -The model is responsible for: - -- understanding the requested code change; -- deciding whether a localized exact replacement is suitable; -- constructing `old_text` and `new_text`; -- constructing a unified diff when using `apply_patch`; -- revising its plan after observing structured errors and refreshed content; -- running relevant verification before `finish`. - -### 4.2 Yada responsibilities - -Yada is responsible for: - -- selecting and freezing the run strategy; -- exposing the appropriate stable tool interface; -- enforcing one editing operation per Agent turn; -- authorizing or rejecting proposed editing operations according to recovery - state; -- performing deterministic read-only recovery when possible; -- rejecting unchanged retries; -- bounding patch, replace, protocol, and no-progress loops; -- preserving SHA and transactional guarantees; -- ending the run explicitly when recovery is exhausted; -- recording the complete control path in traces. - -### 4.3 Guarantee boundary - -Yada can guarantee: - -- that a disallowed edit does not execute; -- that a required re-read occurs before a later accepted edit; -- that `PATCH_REQUIRED` accepts no further `replace_text` edit; -- that retries are finite; -- that failure is explicit and auditable. - -Yada cannot guarantee: - -- that the model will call `apply_patch` when requested; -- that generated patch arguments are syntactically valid; -- that a valid patch solves the user task; -- that every run completes successfully. - -When the model cannot produce an allowed, valid edit within the budgets, the run -must end as `unfinished`; Yada must not manufacture a permissive edit behind the -Agent's back. - -## 5. State model - -The editing controller should be implemented as a small state machine whose -transition logic can be tested as a pure function. - -```python -from dataclasses import dataclass, field -from enum import Enum - - -class EditingStrategy(str, Enum): - PATCH_ONLY = "patch-only" - REPLACE_FIRST = "replace-first" - - -class RecoveryPhase(str, Enum): - NORMAL = "normal" - NEED_READ = "need-read" - NEED_MODEL_READ = "need-model-read" - READY_TO_REPLAN = "ready-to-replan" - REPLACE_RETRY_ALLOWED = "replace-retry-allowed" - PATCH_RETRY_ALLOWED = "patch-retry-allowed" - PATCH_REQUIRED = "patch-required" - TERMINAL_FAILURE = "terminal-failure" - - -@dataclass(frozen=True) -class ReadSnapshot: - path: str - sha256: str - start_line: int - end_line: int - available_to_model_at_step: int - - -@dataclass -class PendingRecovery: - failed_step: int - failed_tool_call_id: str - failed_tool: str - error_code: str - paths: tuple[str, ...] - arguments_fingerprint: str - phase: RecoveryPhase - corrected_retry_count: int = 0 - - -@dataclass -class EditingRunState: - strategy: EditingStrategy - frozen_tool_names: tuple[str, ...] - tool_schema_fingerprint: str - prompt_fingerprint: str - pending_recovery: PendingRecovery | None = None - last_reads: dict[str, ReadSnapshot] = field(default_factory=dict) - failure_counts: dict[tuple[int, str, str], int] = field(default_factory=dict) - protocol_violation_count: int = 0 - no_progress_turns: int = 0 - edit_failures_this_revision: int = 0 - workspace_revision: int = 0 -``` - -The recommended interface is: - -```python -transition(state, event) -> tuple[new_state, actions] -``` - -Representative events include: - -- `RunStarted`; -- `ModelTurnStarted`; -- `ToolCallProposed`; -- `ReadCompleted`; -- `EditSucceeded`; -- `EditFailed`; -- `ToolCallRejected`; -- `VerificationSucceeded`; -- `BudgetExhausted`. - -Representative actions include: - -- `ExecuteTool`; -- `PerformRecoveryRead`; -- `RejectToolCall`; -- `AppendObservation`; -- `WriteTraceEvent`; -- `EndRun`. - -## 6. Run initialization - -### 6.1 Strategy selection - -The CLI and evaluation adapter must accept: - -```text ---editing-strategy patch-only ---editing-strategy replace-first -``` - -`patch-only` remains the default until benchmark evidence justifies changing -it. - -### 6.2 Stable tool exposure - -For `patch-only`, expose: +10. **Bounded run:** max_steps is the final termination boundary. A small + per-revision edit-failure limit may end an unproductive recovery earlier. -```text -search_code -read_file -apply_patch -run_command -finish -``` +These are control-flow and safety properties. They do not guarantee that the +model follows the preferred routing policy or completes the task. -For `replace-first`, expose: +## 4. Run-level strategies -```text -search_code -read_file -replace_text -apply_patch -run_command -finish -``` +### 4.1 Initialization -The tool collection must be created once during initialization. Recovery state -must not dynamically remove, reorder, or redefine tools. For example, -`PATCH_REQUIRED` leaves the schemas unchanged but deterministically rejects a -later `replace_text` call. +The CLI and evaluation adapter accept: -### 6.3 Stable prompt - -The strategy-specific system prompt is also created once. +~~~text +--editing-strategy patch-only +--editing-strategy replace-first +~~~ -`patch-only` instructions state that all workspace edits use `apply_patch`. +patch-only remains the default until benchmark evidence supports changing it. -`replace-first` instructions state: +The strategy-specific tool collection is built once at run initialization: -- use `replace_text` for an existing regular text file when the change is local - and `old_text` is exact and unique; -- use `apply_patch` directly for creation, deletion, rename, large structural - rewrite, impractically large anchors, or unsupported targets; -- follow the structured recovery matrix; -- never treat a failed replacement as permission for same-turn fallback. +| Strategy | Exposed editing tools | +| --- | --- | +| patch-only | apply_patch | +| replace-first | replace_text and apply_patch | -### 6.4 Run-start trace +Both strategies also expose search_code, read_file, run_command, and finish. +Recovery state must not add, remove, reorder, or redefine tools later in the +run. -The `run_start` event must contain at least: +The run-start trace records at least: -```json +~~~json { "editing_strategy": "replace-first", "tool_names": [ @@ -338,927 +136,445 @@ The `run_start` event must contain at least: "run_command", "finish" ], - "tool_schema_fingerprint": "...", - "prompt_fingerprint": "..." + "max_edit_failures_per_revision": 4 } -``` - -The fingerprints make strategy comparisons auditable and detect accidental -mid-run drift. - -## 7. Routing algorithm - -### 7.1 `patch-only` - -`replace_text` is not exposed. If an edit occurs, the model must express it as -`apply_patch`. - -This guarantees which public editing tool is available, but it does not -guarantee that the model can generate a valid patch. Patch recovery and bounded -failure are therefore required; see [Section 11](#11-patch-only-control-flow). - -### 7.2 `replace-first` - -The model applies the following decision rule: - -```python -def preferred_editing_tool(intent): - if intent.creates_file: - return "apply_patch" - if intent.deletes_or_renames_file: - return "apply_patch" - if intent.is_large_structural_rewrite: - return "apply_patch" - if intent.requires_impractically_large_anchor: - return "apply_patch" - if intent.target_is_unsupported_by_replace: - return "apply_patch" - - if ( - intent.targets_existing_regular_text - and intent.is_localized - and intent.has_exact_unique_anchor - ): - return "replace_text" - - return "apply_patch" -``` - -The semantic predicates such as `is_localized` are model judgments guided by -the prompt. The tools remain the deterministic backstop: - -- `replace_text` validates file type, UTF-8, SHA, exact match count, limits, and - transactionality; -- `apply_patch` validates declared targets, SHA, paths, patch syntax, context, - and transactionality. - -The framework must not add a second model or AST router merely to classify the -edit; those approaches are outside Issue #10. - -## 8. Agent-turn execution algorithm - -### 8.1 Batch validation - -Define: - -```python -EDITING_TOOLS = {"replace_text", "apply_patch"} -``` - -Before executing a model response: - -```python -editing_calls = [ - call for call in tool_calls - if call.name in EDITING_TOOLS -] +~~~ -if len(editing_calls) > 1: - reject_editing_batch( - error_code="multiple_edit_operations", - error="Only one editing operation is allowed per Agent turn.", - ) -``` +Prompt and schema fingerprints are not required for the first implementation. +The frozen strategy and tool names, combined with the existing model request +trace, are sufficient to audit the comparison. -The rejected editing calls produce no side effects. This prevents the sequence -below from executing in one turn: +### 4.2 Routing policy -```text -replace_text -> failure -> apply_patch -``` +For patch-only, every workspace edit is expressed as apply_patch. -because the model could not have observed the replacement failure before it -generated the patch call. +For replace-first, use this selection matrix: -### 8.2 Recovery authorization +| Edit intent | Preferred tool | Reason | +| --- | --- | --- | +| Localized change to an existing regular UTF-8 file with an exact unique anchor | replace_text | Avoid model-generated diff hunk metadata | +| Create a file | apply_patch | replace_text supports existing files only | +| Delete a file | apply_patch | replace_text cannot delete files | +| Rename a file | apply_patch when the patch tool supports the operation; otherwise report unsupported | Do not simulate a rename with text replacement | +| Large structural rewrite | apply_patch | An exact replacement would reproduce too much source | +| Exact anchor would be impractically large | apply_patch | Keep replacement requests bounded | +| Target is unsupported by replace_text but valid for apply_patch | apply_patch | Use the tool whose contract covers the operation | +| Exact uniqueness is unknown | read_file first | Do not guess between locations | -Before an editing call executes, the controller checks: +Localized, large, and impractically large are model judgments guided by the +prompt. The tools remain the deterministic safety boundary: they validate +paths, hashes, exact matches, patch syntax, context, and transactionality. -1. Is this editing tool allowed by the current recovery phase? -2. Has every required post-failure read become visible to the model? -3. Are the arguments different from an already rejected attempt? -4. Is the relevant retry budget still available? -5. Is the run already in a terminal state? +Issue #10 lists renames as an apply_patch case, while the current patch contract +from Issue #8 rejects rename metadata. Strategy routing may select the patch +path, but Issue #10 must not silently expand the patch tool contract; until +rename support is added separately, the operation fails as unsupported. -If any condition fails, the call is rejected with a structured observation and -no edit is executed. +### 4.3 Prompt contract -### 8.3 Read visibility +The patch-only prompt instructs the model to: -A model-generated read and edit in the same turn cannot satisfy a recovery -precondition: the model generated the edit before seeing the read result. +- read current files before editing; +- use apply_patch for all workspace edits; +- regenerate a patch from fresh content after a structured failure. -For that reason, a read snapshot records: +The replace-first prompt instructs the model to: -```text -available_to_model_at_step -``` +- prefer replace_text only for localized exact unique replacement; +- use apply_patch directly for creation, deletion, structural edits, or + unsupported replacement targets; +- observe the recovery matrix in Section 6; +- never propose a same-turn fallback after replace_text; +- re-read when an error says current source context is required. -An edit in step `N` may use a recovery snapshot only if: +Prompt guidance influences routing but is not treated as a safety guarantee. -```python -snapshot.available_to_model_at_step <= N -``` +## 5. Agent-turn control algorithm -An automatic recovery read performed between steps `N - 1` and `N` is visible -in step `N`. A `read_file` proposed by the model in step `N` becomes visible no -earlier than step `N + 1`. +The existing model → planner → executor → observation loop remains in place. +Issue #10 adds only the strategy selection and edit isolation described below. -## 9. Edit results and recovery +### 5.1 Batch preflight -### 9.1 Success +Before tool execution, count replace_text and apply_patch calls in the assistant +response. -On success: +If more than one editing operation is present: -```python -state.workspace_revision += 1 -state.pending_recovery = None -state.edit_failures_this_revision = 0 -state.no_progress_turns = 0 -context.state.verified_revision = -1 -context.state.touched_files.update(changed_paths) -``` +- reject the complete tool-call batch using the existing Planner/Executor + rejection path; +- return a structured multiple_edit_operations result for every call; +- leave workspace and verification state unchanged; +- let the next model turn choose one operation after observing the rejection. -The result includes the new revision, changed paths, and post-edit hashes. A -relevant test or build must succeed at the new revision before `finish`. +This prevents a response such as: -### 9.2 General failure procedure - -For an editing failure: - -1. Preserve the original structured error and bounded details. -2. Do not advance workspace or verification state. -3. Increment the per-revision edit failure counter. -4. Create a `PendingRecovery` object. -5. Determine whether read-only recovery is required. -6. Perform bounded automatic reads when a reliable range is available. -7. Attach the recovery context to the observation for the next model turn. -8. Transition to the error-specific recovery phase. -9. Never execute another edit from the same Agent turn. - -Conceptually: - -```text -edit failure - -> structured result - -> PendingRecovery - -> optional automatic read-only recovery - -> failure + recovery context in next model request - -> later model decision -``` - -### 9.3 Automatic read-only recovery - -Automatic reads strengthen the prompt-only recovery policy without violating -Issue #10: they do not edit the workspace, and fallback still happens only in a -later Agent turn after the model observes the failure. - -The controller should retain the range of every successful `read_file` call. - -Recommended recovery ranges: - -- `stale_hash`: re-read the last ranges used for each affected path and obtain - the current SHA; -- `no_match`: re-read the last ranges from which the proposed anchor was - derived; -- `ambiguous_match`: read bounded windows around the returned match line - numbers; -- `patch_context_mismatch`: read bounded windows derived from the failed patch - hunks. +~~~text +replace_text +apply_patch +~~~ -If no reliable bounded range is available, set `NEED_MODEL_READ`. In this phase, -editing calls are rejected until a model-requested `read_file` result has become -visible in a later turn. Reading an arbitrary part of a large file must not be -treated as sufficient merely to satisfy the state machine. +from acting as a precomputed fallback. The patch was generated before the model +knew whether the replacement had failed. -An observation may contain: +### 5.2 Failure observation barrier -```json -{ - "ok": false, - "error_code": "no_match", - "error": "old_text was not found in src/app.py", - "details": { - "paths": ["src/app.py"] - }, - "recovery_context": { - "action": "reread", - "reads": [ - { - "path": "src/app.py", - "sha256": "current-sha256", - "start_line": 70, - "end_line": 130, - "content": "..." - } - ] - } -} -``` +When the single editing operation fails: -### 9.4 Error transition matrix +1. Preserve its error_code, human-readable error, and bounded details. +2. Rely on the one-edit-per-turn preflight to ensure that no later editing + operation exists in that assistant response. +3. Append the assistant message and one result for every tool call. +4. Send those observations in the next model request. +5. Permit a later edit only after the error-specific read requirement has been + satisfied. -The matrix covers errors from Issue #10 and `invalid_patch` from its dependency, -Issue #8. +Every model tool call must still receive a result. A rejected batch must not +leave the provider conversation with an unmatched tool_call. -| Error code | Deterministic Yada action | Next accepted editing behavior | Exhaustion behavior | -|---|---|---|---| -| `stale_hash` | Re-read affected ranges and return current SHA | Re-plan from fresh content; do not automatically patch | Repeated staleness becomes `concurrent_modification` | -| `no_match` | Re-read the source range used for the anchor | Retry with a new exact anchor or deliberately generate a patch | A second corrected `no_match` enters `PATCH_REQUIRED` | -| `ambiguous_match` | Read windows around all reported matches | Retry `replace_text` with a larger unique anchor | Persistent ambiguity becomes `unresolved_ambiguity`; no blind patch | -| `invalid_edit` | Return validation details; no automatic read | Correct arguments and retry | Repeated unchanged/invalid arguments exhaust the protocol budget | -| `invalid_patch` | Return patch syntax diagnostics | Regenerate the patch once | Persistent invalidity becomes `patch_retry_exhausted` | -| `unsupported_target` | Determine whether `apply_patch` supports the requested operation | Enter `PATCH_REQUIRED` only when patching is valid | Otherwise terminate as `unsupported_operation` | -| `patch_context_mismatch` | Re-read affected hunk ranges | Regenerate `apply_patch` once | Persistent mismatch becomes `patch_retry_exhausted` | -| `apply_failed` | Preserve complete bounded diagnostics | No automatic recovery | Immediate `terminal_edit_failure` | +### 5.3 Recovery read visibility -### 9.5 Unchanged retry detection +Some errors require a fresh read before the next edit. The minimal controller +only needs to remember: -Canonicalize and hash editing arguments. A failed attempt key should include: +- the failed step; +- the error code; +- affected paths; +- whether a required read result has become visible to the model. -```python -attempt_key = ( - state.workspace_revision, - tool_name, - canonical_arguments_fingerprint, - tuple(sorted(affected_paths)), -) -``` +No automatic read is required. The model requests read_file in the next turn, +and Yada returns the normal bounded content and current SHA. -If the same attempt is proposed again at the same revision, reject it without -re-running the editing tool: +If an error occurs in step N: -```json -{ - "ok": false, - "error_code": "unchanged_retry", - "details": { - "recovery": "Use the refreshed content to construct different arguments." - } -} -``` +- a read already visible before step N is not post-failure evidence; +- a read proposed in step N + 1 becomes visible to the model in step N + 2; +- an edit also proposed in step N + 1 was generated without seeing that read + and causes that complete batch to be rejected when fresh context is required; +- an edit proposed in step N + 2 may use the read result. -## 10. Bounded retries and loop prevention +Consequently, recovery that requires fresh content uses a read-only Agent turn +followed by an editing turn. This fits the existing whole-batch rejection path +and avoids introducing per-call scheduling. -Neither strategy inherently prevents loops. A model may repeatedly generate an -invalid patch, repeatedly choose an unsuitable replacement, alternate between -tools, alternate between error codes, or avoid editing entirely. Loop prevention -must therefore be strategy-independent. +This is the semantic purpose previously represented by +available_to_model_at_step. It should be implemented with the smallest state +that fits the existing Planner and Executor boundaries. -### 10.1 Recommended initial budgets +### 5.4 Successful edit -The exact values may be tuned by benchmark evidence, but they must be finite, -recorded in `run_start`, and covered by tests. +Successful replace_text and apply_patch calls continue to use the existing tool +state: -```python -MAX_CORRECTED_REPLACE_RETRIES = 1 -MAX_REGENERATED_PATCH_RETRIES = 1 -MAX_STALE_REFRESHES = 2 -MAX_EDIT_FAILURES_PER_REVISION = 4 -MAX_RECOVERY_PROTOCOL_VIOLATIONS = 2 -MAX_NO_PROGRESS_TURNS = 3 -``` +- increment workspace revision; +- update touched files; +- invalidate the verified revision; +- require a relevant successful test or build before finish. -The existing `max_steps` remains the global final bound. +A successful edit clears the pending recovery requirement and resets the +per-revision edit-failure counter. -### 10.2 Why several budgets are required +## 6. Recovery matrix -Per-error budgets alone are insufficient. A model could alternate: +Recovery is model-driven and occurs in later Agent turns. Yada may enforce a +required post-failure read, but it does not generate a replacement or patch. -```text -invalid_patch --> patch_context_mismatch --> stale_hash --> invalid_patch --> ... -``` +| Error code | Fresh read required before another edit? | Next model action | Exhaustion behavior | +| --- | --- | --- | --- | +| stale_hash | Yes | Read affected files and reconstruct the edit with current SHA and content. Do not fall back automatically. | Count the failed edit; the per-revision or max_steps boundary eventually ends repeated races. | +| no_match | Yes | Read relevant content, then use current exact text or deliberately generate a new patch. | No forced second-attempt transition; further failures consume the shared edit-failure limit. | +| ambiguous_match | Yes | Read a narrower range or enlarge the exact anchor until the target is unique. | No fixed one-retry limit. Continue only while shared budgets remain; ambiguity alone never authorizes a blind patch. | +| invalid_edit | No | Correct the arguments in a later turn. | Repeated failures consume the shared edit-failure limit. | +| unsupported_target | No, unless current source is needed to build a patch | Use apply_patch only if that operation is valid under its contract; otherwise report the unsupported operation. | No hidden conversion or mutation. | +| invalid_patch | No for syntax-only errors; read if source context may be stale | Correct or regenerate the unified diff. | Repeated failures consume the shared edit-failure limit. | +| patch_context_mismatch | Yes | Read affected files and regenerate the patch from current content. | Repeated failures consume the shared edit-failure limit. | +| apply_failed | No automatic recovery | Preserve diagnostics and fail loudly; the model may inspect the cause, but Yada performs no fallback edit. | The shared boundaries end repeated attempts. | -The per-revision failure budget closes this loophole because every failed edit -at the same workspace revision consumes the same global edit-failure budget, -regardless of tool or error code. +The invalid_patch row comes from Issue #8, on which Issue #10 depends. -The protocol budget covers calls that are rejected before execution, such as: +### 6.1 Clarification for ambiguous_match -- proposing more than one editing operation in one turn; -- retrying identical failed arguments; -- proposing `replace_text` in `PATCH_REQUIRED`; -- attempting to bypass ambiguity recovery; -- proposing an edit before the required read is visible. +Issue #10 says to read a narrower range or enlarge the exact anchor until it is +unique. It does not specify that only one corrected replacement is allowed. -The no-progress budget covers turns in which the model: +Therefore the first implementation must not: -- emits text without an actionable tool call; -- repeatedly reads the same range at the same SHA; -- performs unrelated searches; -- proposes only rejected editing operations; -- changes error types without advancing the recovery phase. +- force PATCH_REQUIRED after one ambiguous retry; +- terminate as unresolved_ambiguity after one retry; +- treat ambiguity itself as permission to patch an uncertain target. -### 10.3 What counts as progress +The model may make multiple evidence-based attempts, bounded by the shared +per-revision failure limit and max_steps. A deliberate patch is acceptable only +after fresh context makes the intended target unambiguous; it is never an +automatic reaction to ambiguous_match. -The no-progress counter resets only for a meaningful event: +## 7. Loop prevention -1. a successful edit increments the workspace revision; -2. a required read returns a new SHA; -3. a read completes a pending recovery requirement; -4. the recovery phase advances monotonically, for example - `NEED_READ -> READY_TO_REPLAN -> PATCH_REQUIRED`; -5. a relevant test or build succeeds for the latest revision. +### 7.1 Two boundaries -The following do not count as progress: +The first implementation uses only two loop boundaries: -- submitting a different malformed patch; -- changing from one edit error code to another at the same revision; -- repeating the same read range and SHA; -- a rejected call; -- an irrelevant inspection command; -- a text-only claim of completion. +1. max_steps, which already decreases on every model turn and guarantees that + the run is finite; +2. MAX_EDIT_FAILURES_PER_REVISION, initially 4, which ends repeated failed + editing attempts earlier at one unchanged workspace revision. -### 10.4 Termination function +The per-revision counter: -```python -def terminal_reason(state, *, max_steps): - if state.pending_recovery is not None: - if state.pending_recovery.phase == RecoveryPhase.TERMINAL_FAILURE: - return state.pending_recovery.error_code +- increments when replace_text or apply_patch actually executes and fails; +- does not increment for read-only calls; +- resets after a successful edit advances the revision; +- ends the run as unfinished with edit_failure_budget_exhausted when it reaches + the configured limit. - if state.edit_failures_this_revision >= MAX_EDIT_FAILURES_PER_REVISION: - return "edit_failure_budget_exhausted" +Rejected protocol calls can rely on max_steps in the first implementation. +There is no separate protocol-violation budget, no no-progress budget, and no +per-error retry budget. - if state.protocol_violation_count >= MAX_RECOVERY_PROTOCOL_VIOLATIONS: - return "recovery_protocol_exhausted" +### 7.2 What these boundaries guarantee - if state.no_progress_turns >= MAX_NO_PROGRESS_TURNS: - return "no_progress" +They guarantee finite execution, not successful completion: - if current_step >= max_steps: - return "max_steps_exhausted" +- patch-only may repeatedly generate invalid patches; +- replace-first may repeatedly produce no_match or ambiguous_match; +- the model may ignore a requested read; +- the model may avoid editing entirely. - return None -``` +Actual failed edits are stopped early by the per-revision limit. Other +non-progress behavior is stopped by max_steps. -### 10.5 Finite-termination argument +Additional counters should be introduced only after traces or the Issue #10 +benchmark demonstrate a loop pattern that these two boundaries cannot diagnose +or control adequately. -Consider the finite budget vector: +No formal budget-vector proof is necessary: max_steps alone is already a +strictly decreasing global bound. -```text -( - remaining steps, - remaining edit failures for the current revision, - remaining recovery retries, - remaining protocol violations, - remaining no-progress turns -) -``` +## 8. Control flows -Every Agent turn either: +### 8.1 Overall flow -- makes genuine progress; -- decreases at least one finite budget; -- reaches a terminal state. +~~~mermaid +flowchart TD + A["Start run"] --> B{"Editing strategy"} + B -->|"patch-only"| C["Freeze prompt and tools: apply_patch"] + B -->|"replace-first"| D["Freeze prompt and tools: replace_text + apply_patch"] + C --> E["Request model turn"] + D --> E -The global remaining-step count decreases on every model turn. Therefore a run -cannot execute indefinitely: it eventually finishes successfully or ends with -an explicit `unfinished` reason. + E --> F["Planner inspects proposed tool calls"] + F --> G{"More than one editing operation?"} + G -->|"Yes"| H["Reject complete batch with no side effects"] + H --> I["Return structured results in next model request"] + I --> N{"max_steps exhausted?"} -## 11. `patch-only` control flow + G -->|"No"| J{"Required read pending and edit proposed?"} + J -->|"Yes"| K["Reject complete batch and request read-only turn"] + K --> I + J -->|"No"| L["Execute tools in existing order"] -`patch-only` does not have an alternate public editing tool, so repeated patch -failure must be bounded directly. + L --> M{"Editing result"} + M -->|"Success"| O["Advance revision and require verification"] + O --> P["Continue normal Agent loop"] + P --> N -### 11.1 Correctable patch failures + M -->|"Failure"| Q["Return structured error; increment revision failure count"] + Q --> R{"Failure limit reached?"} + R -->|"Yes"| S["End unfinished: edit_failure_budget_exhausted"] + R -->|"No"| T["Record any required post-failure read"] + T --> I -- `invalid_patch`: return bounded parser diagnostics and allow one regenerated - patch; -- `stale_hash`: refresh the relevant source and allow a patch using the current - SHA; -- `patch_context_mismatch`: refresh hunk ranges and allow one regenerated patch. + M -->|"No edit"| P + N -->|"No"| E + N -->|"Yes"| U["End unfinished: max_steps_exhausted"] +~~~ -### 11.2 Non-correctable or exhausted failures +### 8.2 Replace-first routing and recovery -- unsupported paths or target types terminate explicitly; -- `apply_failed` terminates immediately; -- a second corrected syntax/context failure terminates with - `patch_retry_exhausted`; -- identical patch arguments are rejected without executing `git apply`; -- cross-error alternation is stopped by `MAX_EDIT_FAILURES_PER_REVISION`; -- refusal to generate a patch is stopped by protocol, no-progress, or global - step budgets. - -The guarantee is not that patching always succeeds. The guarantee is: - -```text -a patch succeeds, or patch-only ends explicitly within finite budgets -``` - -## 12. `replace-first` control flow - -### 12.1 Successful replacement - -A successful `replace_text` edit completes through the existing transactional -patch boundary. The public `apply_patch` tool does not need to appear in every -run; retaining both tools in the architecture does not require using both in -each task. - -### 12.2 `no_match` - -```text -replace_text -> no_match --> automatic bounded re-read --> next model turn sees failure and current source --> one corrected replace or a deliberate patch -``` - -If the corrected replacement again returns `no_match`, transition to -`PATCH_REQUIRED`. From that point, `replace_text` remains visible in the stable -schema but is rejected by the controller. - -### 12.3 `ambiguous_match` - -```text -replace_text -> ambiguous_match --> read windows around reported matches --> require a larger exact unique anchor --> allow one corrected replacement -``` - -If the corrected replacement remains ambiguous, terminate as -`unresolved_ambiguity`. Do not automatically require or execute a patch merely -to escape the loop, because that could bypass the evidence that the edit target -is not unique. - -### 12.4 `unsupported_target` - -If the requested operation is valid for `apply_patch`, enter `PATCH_REQUIRED`. -If the target is prohibited for both tools, such as a protected or escaping -path, terminate as `unsupported_operation`. - -### 12.5 Patch fallback failures - -After transitioning to `PATCH_REQUIRED`, all patch attempts are governed by the -same recovery and retry budgets as `patch-only`. This prevents a replacement -loop from merely turning into a patch loop. - -The guarantee is: - -```text -replace succeeds, -or a deliberate bounded patch succeeds, -or replace-first ends explicitly within finite budgets -``` - -## 13. End-to-end pseudocode - -```python -def run(task, strategy): - state = initialize_frozen_editing_state(strategy) - messages = build_initial_messages(task, strategy) - trace_run_start(state) - - for step in range(1, max_steps + 1): - response = model.complete( - messages=messages, - tools=state.frozen_tool_schemas, - ) - plan = planner.plan(response) - editing_calls = [ - call - for call in plan.tool_calls - if call.name in {"replace_text", "apply_patch"} - ] - - if len(editing_calls) > 1: - results = reject_multiple_edit_operations(plan.tool_calls) - state.protocol_violation_count += 1 - state.no_progress_turns += 1 - messages.extend(tool_results_to_messages(results)) - if reason := terminal_reason(state, max_steps=max_steps): - return unfinished(reason) - continue - - results = [] - turn_made_progress = False - - for call in plan.tool_calls: - if call.name not in {"replace_text", "apply_patch"}: - result = execute_non_editing_tool(call) - progress = record_observation_if_relevant( - state, - call, - result, - step, - ) - turn_made_progress = turn_made_progress or progress - results.append(result) - continue - - authorization = authorize_editing_call(state, call, step) - if not authorization.allowed: - state.protocol_violation_count += 1 - results.append(authorization.rejection) - continue - - if is_unchanged_retry(state, call): - state.protocol_violation_count += 1 - results.append(unchanged_retry_observation(call)) - continue - - result = execute_editing_tool(call) - trace_tool_result(call, result) - - if result.ok: - on_edit_success(state, call, result) - turn_made_progress = True - results.append(result) - continue - - state.edit_failures_this_revision += 1 - recovery = create_pending_recovery(step, call, result) - state.pending_recovery = recovery - - recovery_reads = perform_safe_recovery_reads( - state=state, - recovery=recovery, - visible_to_model_at_step=step + 1, - ) - phase_advanced = transition_after_failure( - state, - recovery, - recovery_reads, - ) - turn_made_progress = turn_made_progress or phase_advanced - results.append(attach_recovery_context(result, recovery_reads)) - - state.no_progress_turns = ( - 0 if turn_made_progress else state.no_progress_turns + 1 - ) - - messages.append(response.message) - messages.extend(tool_results_to_messages(results)) - trace_recovery_state(state) - - if reason := terminal_reason(state, max_steps=max_steps): - return unfinished(reason) - - if a_finish_result_succeeded(results): - return finished(results) - - return unfinished("max_steps_exhausted") -``` - -## 14. Overall flowchart - -```mermaid -flowchart TD - A["Start run"] --> B{"Select editing strategy"} - B -- "patch-only" --> C["Freeze tools: apply_patch only"] - B -- "replace-first" --> D["Freeze tools: replace_text and apply_patch"] - C --> E["Freeze prompt, schemas, and fingerprints"] - D --> E - E --> F["Write run_start trace"] - F --> G["Request next model turn"] - - G --> H["Planner parses tool calls"] - H --> I{"More than one editing operation?"} - I -- "Yes" --> J["Reject editing batch; consume protocol budget"] - J --> K{"Budget exhausted?"} - K -- "Yes" --> L["End unfinished with explicit reason"] - K -- "No" --> G - - I -- "No" --> M{"Contains an editing operation?"} - M -- "No" --> N["Execute read, search, test, or finish"] - N --> O["Record observations and progress"] - O --> P{"Finished or budget exhausted?"} - P -- "Finished" --> Q["End finished"] - P -- "Exhausted" --> L - P -- "Continue" --> G - - M -- "Yes" --> R{"Controller authorizes this edit?"} - R -- "No" --> S["Reject call; consume protocol/no-progress budget"] - S --> K - R -- "Yes" --> T["Execute replace_text or apply_patch"] - T --> U{"Edit succeeded?"} - - U -- "Yes" --> V["Advance revision and invalidate verification"] - V --> W["Require relevant test or build"] - W --> G - - U -- "No" --> X["Preserve structured error and arguments fingerprint"] - X --> Y["Create PendingRecovery"] - Y --> Z{"Read-only recovery required?"} - Z -- "Yes" --> AA["Perform bounded recovery reads"] - Z -- "No" --> AB["Skip automatic read"] - AA --> AC["Attach failure and recovery context"] - AB --> AC - AC --> AD["Transition recovery phase and consume retry budget"] - AD --> AE{"Terminal recovery state?"} - AE -- "Yes" --> L - AE -- "No" --> G -``` - -## 15. `patch-only` flowchart - -```mermaid -flowchart TD - A["Model proposes apply_patch"] --> B{"Controller authorizes call?"} - B -- "No" --> C["Reject and consume protocol budget"] - C --> D{"Protocol/no-progress budget exhausted?"} - D -- "Yes" --> E["End unfinished"] - D -- "No" --> A - - B -- "Yes" --> F["Execute apply_patch"] - F --> G{"Result"} - G -- "Success" --> H["Advance revision; require verification"] - G -- "invalid_patch" --> I{"Regenerated patch retry available?"} - I -- "Yes" --> J["Return syntax diagnostics to next model turn"] - J --> A - I -- "No" --> K["End: patch_retry_exhausted"] - - G -- "stale_hash" --> L["Refresh source ranges and SHA"] - L --> M{"Stale refresh budget available?"} - M -- "Yes" --> A - M -- "No" --> N["End: concurrent_modification"] - - G -- "patch_context_mismatch" --> O["Refresh affected hunk ranges"] - O --> P{"Regenerated patch retry available?"} - P -- "Yes" --> A - P -- "No" --> K - - G -- "unsupported_target" --> Q["End: unsupported_operation"] - G -- "apply_failed" --> R["End: terminal_edit_failure"] -``` - -## 16. `replace-first` flowchart - -```mermaid +~~~mermaid flowchart TD - A["Model evaluates requested edit"] --> B{"Existing text, localized, exact unique anchor?"} - B -- "No" --> C["Propose apply_patch"] - B -- "Yes" --> D["Propose replace_text"] - - D --> E{"replace_text result"} - E -- "Success" --> F["Advance revision; require verification"] - E -- "stale_hash" --> G["Refresh content and SHA"] - G --> H{"Stale refresh budget available?"} - H -- "Yes" --> A - H -- "No" --> I["End: concurrent_modification"] - - E -- "no_match" --> J["Refresh source range"] - J --> K{"Corrected replace already attempted?"} - K -- "No" --> L["Next turn: corrected replace or deliberate patch"] - L --> A - K -- "Yes" --> M["Enter PATCH_REQUIRED"] - - E -- "ambiguous_match" --> N["Read windows around all matches"] - N --> O{"Expanded-anchor retry already attempted?"} - O -- "No" --> P["Require one larger exact unique anchor"] - P --> D - O -- "Yes" --> Q["End: unresolved_ambiguity"] - - E -- "invalid_edit" --> R["Return validation details"] - R --> S{"Arguments changed and retry available?"} - S -- "Yes" --> D - S -- "No" --> T["Reject or end by protocol budget"] - - E -- "unsupported_target" --> U{"Operation valid for apply_patch?"} - U -- "No" --> V["End: unsupported_operation"] - U -- "Yes" --> M - - M --> W{"Next proposed editing operation"} - W -- "replace_text" --> X["Reject: patch_required"] - X --> Y{"Protocol budget exhausted?"} - Y -- "Yes" --> Z["End: recovery_protocol_exhausted"] - Y -- "No" --> W - W -- "apply_patch" --> C - - C --> AA["Use patch-only recovery and retry rules"] - AA --> AB{"Patch succeeds?"} - AB -- "Yes" --> F - AB -- "No and budget exhausted" --> AC["End: patch_retry_exhausted"] -``` - -## 17. Trace requirements - -In addition to existing model, tool-call, and tool-result events, the controller -should record: - -### `recovery_started` - -```json -{ - "step": 4, - "failed_tool_call_id": "call-replace-4", - "failed_tool": "replace_text", - "error_code": "no_match", - "paths": ["src/app.py"], - "phase": "need-read" -} -``` - -### `recovery_read` - -```json -{ - "step": 4, - "triggered_by_tool_call_id": "call-replace-4", - "paths": ["src/app.py"], - "ranges": [{"start_line": 70, "end_line": 130}], - "visible_to_model_at_step": 5 -} -``` + A["Model evaluates edit intent"] --> B{"Localized existing text with exact unique anchor?"} + B -->|"Yes"| C["Propose replace_text"] + B -->|"No"| D["Propose apply_patch"] + + C --> E{"replace_text result"} + E -->|"Success"| F["Verify latest revision"] + E -->|"stale_hash or no_match"| G["Next turn: read current relevant content"] + E -->|"ambiguous_match"| H["Next turn: read narrower range or match windows"] + E -->|"invalid_edit"| I["Next turn: correct arguments"] + E -->|"unsupported_target"| J["Next turn: choose apply_patch only if valid"] + E -->|"apply_failed"| K["Preserve diagnostics; no automatic fallback"] + + G --> L["Following turn: corrected replace or deliberate patch"] + H --> M["Following turn: use a larger unique anchor"] + M --> E + I --> C + J --> D + L --> N{"Chosen tool"} + N -->|"replace_text"| C + N -->|"apply_patch"| D + + D --> O{"apply_patch result"} + O -->|"Success"| F + O -->|"stale_hash or patch_context_mismatch"| P["Next turn: read affected files"] + O -->|"invalid_patch"| Q["Next turn: correct or regenerate patch"] + O -->|"apply_failed"| K + P --> R["Following turn: regenerate patch"] + Q --> R + R --> D + + E -->|"Any repeated failure"| S["Consume shared per-revision failure limit"] + O -->|"Any repeated failure"| S + S --> T{"Limit or max_steps reached?"} + T -->|"No"| A + T -->|"Yes"| U["End unfinished with explicit reason"] +~~~ + +### 8.3 Patch-only behavior + +patch-only follows the same recovery matrix but never exposes replace_text: + +~~~text +read current source + → propose apply_patch + → success: verify + → structured failure: observe it in the next model turn + → perform any required fresh read + → regenerate the patch in a later turn + → stop at the per-revision failure limit or max_steps +~~~ + +There is no patch-specific retry counter. A valid patch may succeed on any +attempt before the shared boundaries are reached. + +## 9. Trace requirements + +The existing tool_call and tool_result events already record: + +- selected tool; +- arguments; +- success or failure; +- structured error_code; +- Agent step and tool-call correlation. + +Issue #10 adds editing_strategy and the frozen tool names to run_start. It also +needs a bounded trace record when Yada rejects an edit because: + +- multiple editing operations were proposed in one turn; +- a required read was not yet visible to the model; +- the per-revision failure limit was exhausted. + +The recovery path can then be reconstructed from existing ordered model, call, +and result events. A large family of recovery_started, recovery_read, and +recovery_transition events is not required unless implementation experience +shows that existing traces are insufficient. + +An unfinished run records a stable terminal reason such as: + +- edit_failure_budget_exhausted; +- max_steps_exhausted; +- an existing fatal tool or runtime error. + +## 10. Deterministic tests + +Mocked model and tool interactions should cover: + +### Strategy stability + +- patch-only exposes apply_patch but not replace_text; +- replace-first exposes both editing tools; +- strategy and schemas remain stable across all model requests; +- run_start records strategy, tool names, and the edit-failure limit. + +### Routing + +- a localized existing-file change is prompted toward replace_text; +- creation, deletion, and structural edits are prompted toward apply_patch; +- patch-only never exposes replace_text. + +### Turn isolation and visibility + +- a turn with replace_text and apply_patch executes neither edit; +- a failed edit appears in the next model request; +- every call in a rejected batch receives a tool result; +- a read and edit generated in the same recovery turn cause the batch to be + rejected and cannot satisfy the fresh-read requirement; +- an edit in the following turn can use the now-visible read. + +### Recovery + +- stale_hash requires a fresh read before retry; +- no_match permits a corrected replacement or deliberate patch after a read; +- repeated ambiguous_match can continue with larger anchors while shared + budgets remain; +- ambiguity never causes automatic patch fallback; +- invalid_edit permits corrected arguments in a later turn; +- unsupported_target permits patch only when the patch contract supports it; +- invalid_patch permits a regenerated patch; +- patch_context_mismatch requires a fresh read; +- apply_failed preserves diagnostics and performs no hidden edit. -### `recovery_transition` +### Loop and safety boundaries -```json -{ - "step": 4, - "from": "need-read", - "to": "ready-to-replan", - "error_code": "no_match", - "corrected_retry_count": 0 -} -``` +- four failed edits at one revision end with + edit_failure_budget_exhausted; +- a successful edit resets the per-revision failure counter; +- text-only, repeated-read, or rejected-call loops still end at max_steps; +- failed and rejected edits leave files, revision, touched paths, and + verification unchanged; +- successful fallback remains SHA-bound, transactional, and subject to + post-edit verification. -### `editing_call_rejected` +## 11. Evaluation -```json -{ - "step": 6, - "tool": "replace_text", - "error_code": "patch_required", - "protocol_violation_count": 1 -} -``` +Compare patch-only and replace-first using: -### `run_end` +- the same task set and base commits; +- the same model and model parameters; +- the same step, token, command, and wall-time budgets; +- stable prompts and tool schemas within each strategy; +- repeated runs when model nondeterminism requires them. -An unfinished run must include a stable reason and the final controller state: +Record at least: -```json -{ - "finished": false, - "status": "unfinished", - "reason": "patch_retry_exhausted", - "editing_strategy": "replace-first", - "steps": 7, - "workspace_revision": 0, - "edit_attempts": 3, - "last_error_code": "patch_context_mismatch" -} -``` +- first edit-attempt success rate; +- eventual mutation success rate; +- completed or resolved task rate; +- edit retries; +- Agent turns and total steps; +- input and output tokens; +- verification success after mutation; +- unrelated changed lines; +- wrong-target or partial-target mutations, which must remain zero; +- terminal reason distribution. -Trace content must remain bounded and follow the existing summary/debug -redaction rules. Full source content should not be added to summary traces merely -because it was read for recovery. +The first benchmark should run with max_steps and +MAX_EDIT_FAILURES_PER_REVISION only. Add more specialized retry controls only +when a measured failure mode justifies them. -## 18. Evaluation algorithm +## 12. Implementation guidance -Compare `patch-only` and `replace-first` as two policies over the same underlying -editing implementations. +Keep the implementation within the existing Planner, Executor, ToolRunner, +prompt, trace, CLI, and evaluation boundaries described in +[Yada architecture](architecture.md). -For every paired comparison, hold constant: +The design deliberately does not define a new editing.py state machine or copy +the complete Agent loop into this document. Source code and tests are the +authoritative description of implementation mechanics. After Issue #10 lands, +this section should link to the small policy functions and their tests rather +than duplicate them. -- task set and base commits; -- model and model parameters; -- thinking/reasoning settings; -- tool schemas and prompt fingerprints within each strategy; -- step, token, command, and wall-time budgets; -- grading logic; -- retry and no-progress budgets. +replace_text and apply_patch remain data-plane tools. Neither contains routing +policy or invokes the other as a strategy fallback. -Repeat trials when model nondeterminism makes a single run unreliable. +## 13. Industry context -Derive at least these metrics from traces and grader results: +Exact unique replacement, optimistic concurrency checks, fail-closed +transactions, structured errors, and trace-based evaluation are established +patterns rather than one universal editing algorithm. Comparable examples +include [Gemini CLI file tools](https://github.com/google-gemini/gemini-cli/blob/main/docs/tools/file-system.md), +[Claude text editor](https://platform.claude.com/docs/en/agents-and-tools/tool-use/text-editor-tool), +and HTTP [If-Match](https://www.rfc-editor.org/rfc/rfc9110.html#section-13.1.1). -- first edit-attempt success rate; -- eventual editing/mutation success rate; -- completed/resolved task rate; -- replace and patch retry counts; -- recovery-protocol rejection counts; -- Agent turns and total steps; -- input and output tokens; -- successful verification after the latest edit; -- unrelated changed lines, using a benchmark reference or explicit allowed - ranges; -- wrong-target and partial-target edits, which must remain zero; -- terminal-reason distribution, including patch retry exhaustion, unresolved - ambiguity, concurrent modification, protocol exhaustion, no progress, and - max-step exhaustion. - -The comparison validates routing quality and end-to-end efficiency. It must not -replace either editing implementation or remove the `patch-only` baseline. - -## 19. Deterministic test matrix - -At minimum, mocked Agent/tool tests must cover: - -### Strategy and interface - -- `patch-only` exposes no `replace_text` schema; -- `replace-first` exposes both edit schemas; -- schemas and prompt fingerprints remain identical across all model requests in - a run; -- `run_start` records strategy and fingerprints. - -### Turn isolation - -- a turn containing `replace_text` and `apply_patch` executes neither edit; -- a failed edit is present in the next model request; -- a model-generated read and edit in the same turn cannot satisfy a pending - recovery read. - -### Replacement recovery - -- `stale_hash` performs a bounded re-read before retry authorization; -- `no_match` permits one corrected replace or a deliberate patch after a fresh - read; -- a second corrected `no_match` enters `PATCH_REQUIRED`; -- `ambiguous_match` requires a larger anchor and never triggers blind patch; -- persistent ambiguity terminates explicitly; -- `invalid_edit` with unchanged arguments is rejected without execution; -- `unsupported_target` enters `PATCH_REQUIRED` only for patch-supported - operations. - -### Patch recovery - -- `invalid_patch` permits one regenerated patch; -- `patch_context_mismatch` re-reads hunk ranges before retry; -- persistent invalid/context-mismatched patch terminates; -- repeated identical patch arguments are not executed twice; -- `apply_failed` terminates and preserves diagnostics. - -### Loop prevention - -- alternating replace and patch failures consume the per-revision failure - budget; -- alternating error codes cannot evade the global failure budget; -- repeated disallowed tools consume the protocol budget; -- repeated identical reads do not reset the no-progress budget; -- a successful edit resets per-revision recovery counters; -- every mocked loop reaches a stable finished or unfinished result in finite - steps. - -### Safety and verification - -- failed edits do not change revision, files, touched paths, or verification; -- successful fallback remains SHA-bound and transactional; -- successful fallback invalidates previous verification; -- `finish` remains unavailable until a relevant post-edit test/build succeeds. - -## 20. Suggested implementation boundaries - -The state machine should remain small and independent of tool implementation -details. - -Suggested integration points: - -- `src/yada/editing.py`: enums, state records, budgets, transition and - authorization logic; -- `src/yada/tools/schemas.py`: strategy-specific stable schema construction; -- `src/yada/tools/runner.py`: frozen strategy, schemas, handlers, and direct - execution boundary; -- `src/yada/agents/prompts.py`: strategy-specific routing and recovery - instructions; -- `src/yada/agents/planning.py`: one-edit-per-turn batch validation; -- `src/yada/agents/executor.py`: authorization, recovery reads, result wrapping, - and trace events; -- `src/yada/agents/default.py`: run-start metadata, controller lifetime, message - loop, progress and terminal checks; -- `src/yada/evals/`: strategy plumbing, trace metric extraction, comparison, - and reporting. - -`replace_text` and `apply_patch` remain the data-plane editing implementations. -They should not contain model-routing logic or silently call each other as a -fallback. `replace_text` may continue to reuse the validated patch application -boundary internally for transactional file application. - -## 21. Industry positioning - -There is no single industry-standard algorithm for deciding when a coding Agent -should replace text or generate a patch. The broadly established pattern is to -combine: - -- exact, unique replacement for targeted edits; -- optimistic concurrency checks for writes; -- transactional and fail-closed editing; -- structured tool errors; -- a deterministic host policy around model-proposed actions; -- bounded retries, timeouts, and explicit terminal states; -- trace-based evaluation over repeated trials. - -Relevant examples and references: - -- [Gemini CLI file tools](https://github.com/google-gemini/gemini-cli/blob/main/docs/tools/file-system.md) - use exact targeted replacement and default to one occurrence; -- [Claude text editor](https://platform.claude.com/docs/en/agents-and-tools/tool-use/text-editor-tool) - uses exact `str_replace` semantics; -- [RFC 9110, If-Match](https://www.rfc-editor.org/rfc/rfc9110.html#section-13.1.1) - describes strong preconditions used to prevent lost updates; -- [Gemini CLI policy engine](https://github.com/google-gemini/gemini-cli/blob/main/docs/reference/policy-engine.md) - evaluates model-proposed tool calls using deterministic host rules; -- [LangGraph workflows and agents](https://docs.langchain.com/oss/python/langgraph/workflows-agents) - distinguishes predetermined workflow control from dynamic Agent decisions; -- [Anthropic Agent Evals](https://www.anthropic.com/engineering/demystifying-evals-for-ai-agents) - recommends repeated trials, trace inspection, and multiple evaluation layers. - -The algorithm in this document follows that hybrid pattern while preserving the -specific safety and evaluation constraints of Issue #10. +Yada keeps only the parts needed to test Issue #10 without turning the harness +into a general workflow engine. From 11b8302979131906b16aaa2f1d351651a3700b79 Mon Sep 17 00:00:00 2001 From: Gen TANG Date: Tue, 4 Aug 2026 22:47:47 +0800 Subject: [PATCH 3/6] finish issue 10 --- docs/cli-reference.md | 17 + docs/configuration.md | 23 + docs/dev/architecture.md | 29 +- docs/dev/debugging.md | 2 +- docs/dev/editing-strategy.md | 705 ++++++++++-------------------- docs/evaluation.md | 8 + src/yada/agents/default.py | 13 +- src/yada/agents/executor.py | 30 +- src/yada/agents/planning.py | 33 +- src/yada/agents/prompts.py | 72 ++- src/yada/editing.py | 38 ++ src/yada/evals/agents/yada.py | 65 ++- src/yada/evals/cli.py | 8 + src/yada/run/cli.py | 9 + src/yada/tools/runner.py | 26 +- src/yada/traces/html.py | 8 + src/yada/traces/report.py | 1 + tests/agents/test_default.py | 219 ++++++++++ tests/conftest.py | 6 +- tests/evals/test_eval_cli.py | 9 + tests/evals/test_yada_agent.py | 77 +++- tests/test_cli.py | 15 + tests/tools/test_runner.py | 24 + tests/traces/test_trace_html.py | 4 + tests/traces/test_trace_report.py | 3 + 25 files changed, 941 insertions(+), 503 deletions(-) create mode 100644 src/yada/editing.py create mode 100644 tests/test_cli.py diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 7a29447..5dcbb88 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. | `patch-only` | | `--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. | `patch-only` | 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..a24f291 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -70,6 +70,29 @@ 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. | + +`patch-only` is the default until controlled evaluation supports changing it: + +```bash +uv run yada "Fix the localized parser bug" \ + --workspace /path/to/repository \ + --editing-strategy replace-first +``` + +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..64bc8eb 100644 --- a/docs/dev/architecture.md +++ b/docs/dev/architecture.md @@ -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` 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` @@ -107,7 +107,9 @@ Yada exposes six tools: | `finish` | 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. @@ -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` 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..78b144b 100644 --- a/docs/dev/debugging.md +++ b/docs/dev/debugging.md @@ -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` | diff --git a/docs/dev/editing-strategy.md b/docs/dev/editing-strategy.md index eb558a4..dd751a0 100644 --- a/docs/dev/editing-strategy.md +++ b/docs/dev/editing-strategy.md @@ -1,129 +1,81 @@ -# Editing Strategies and Recovery Policy +# Editing Strategies ## Status -This document is the implementation contract for +This document defines the minimal implementation for [Issue #10: replace-first routing with apply_patch fallback](https://github.com/GenTang/Yada/issues/10). -It describes the behavior that must remain stable across prompts, tools, traces, -tests, and evaluations. It intentionally does not prescribe Python classes or -duplicate the Agent loop as near-executable pseudocode. -The central rule is: +Issues #8 and #9 make apply_patch and replace_text independently safe. Issue +#10 adds the thin coordination layer between them: -> The model chooses how to express an edit. Yada exposes a stable strategy, -> executes edits through existing fail-closed tools, and ensures that a failed -> edit is observed by the model before another edit can run. +- 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. -## 1. Scope +It does not add a recovery state machine, automatic recovery reads, per-error +retry budgets, or hidden tool conversion. -### 1.1 Goals +## 1. Responsibilities -Issue #10 adds: +### 1.1 Editing tools -1. Explicit run-level patch-only and replace-first strategies. -2. A stable tool interface and strategy prompt for the complete run. -3. A documented rule for choosing replace_text or apply_patch. -4. A later-turn recovery policy for structured edit failures. -5. A small execution barrier that prevents same-turn fallback. -6. Trace fields and evaluation metrics for comparing both strategies. -7. Deterministic tests using mocked model and tool interactions. +The tools remain responsible for their existing contracts: -The existing SHA checks, transactional application, revision accounting, and -verification gate remain unchanged. +- exact SHA validation; +- path and target validation; +- fail-closed matching or patch application; +- transactional multi-file mutation; +- structured, bounded errors; +- revision and verification bookkeeping. -### 1.2 Non-goals +Neither tool owns routing policy. A failed replace_text call never constructs or +executes an apply_patch fallback. -Issue #10 does not add: +### 1.2 Model -- fuzzy or whitespace-normalized matching; -- automatic selection between ambiguous matches; -- AST-based routing; -- an external edit model; -- a public orchestration tool; -- framework-generated patches; -- automatic conversion of a failed replacement into a patch; -- a guarantee that the model will eventually produce a valid edit. +The model is responsible for: -## 2. Terms +- 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 finish. -### Agent turn +### 1.3 Yada host -One assistant response and the tool calls proposed by that response. +Yada is responsible for: -### Editing operation +- 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. -A replace_text or apply_patch call. Earlier discussions use mutation for the -same concept: an operation that changes workspace files. +## 2. Run-level strategies -### Fallback +Yada supports: -An apply_patch call deliberately proposed by the model in a later Agent turn -after it has observed a failed replace_text result and any required fresh read. - -Fallback is not an internal call from replace_text to apply_patch. The -replace_text implementation may continue to reuse the validated patch boundary -to commit an already validated replacement transaction; that is an -implementation detail, not strategy fallback. - -### Recovery - -The model observes a structured edit error, gathers required current context, -and proposes a corrected edit in a later turn. - -## 3. Required invariants - -The implementation must preserve these invariants: - -1. **Stable strategy:** editing_strategy is selected at run start and cannot - change during the run. -2. **Stable interface:** the prompt and exposed tool schemas do not change - during the run. -3. **Model-owned routing:** Yada does not use a second model, AST router, or - hidden conversion to choose an editing representation. -4. **One edit per turn:** at most one editing operation may execute from one - assistant response. -5. **Observed failure:** a failed edit result is included in a later model - request before another edit may execute. -6. **Read visibility:** a read and an edit generated in the same assistant - response cannot use that read as recovery evidence. The model had not seen - the read result when it generated the edit. -7. **No hidden fallback:** a failed replace_text never causes Yada to generate - or execute apply_patch. -8. **Fail closed:** a failed edit does not change files, revision, touched-file - accounting, or verification state. -9. **Verification after success:** every successful edit invalidates previous - verification. -10. **Bounded run:** max_steps is the final termination boundary. A small - per-revision edit-failure limit may end an unproductive recovery earlier. - -These are control-flow and safety properties. They do not guarantee that the -model follows the preferred routing policy or completes the task. - -## 4. Run-level strategies +| 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. | -### 4.1 Initialization +patch-only remains the default until benchmark evidence supports changing it. -The CLI and evaluation adapter accept: +The strategy is selected once through: ~~~text --editing-strategy patch-only --editing-strategy replace-first ~~~ -patch-only remains the default until benchmark evidence supports changing it. - -The strategy-specific tool collection is built once at run initialization: - -| Strategy | Exposed editing tools | -| --- | --- | -| patch-only | apply_patch | -| replace-first | replace_text and apply_patch | +The ToolRunner builds its handlers and schemas once. The Planner builds its +system prompt once from the same strategy. Neither changes during the run. -Both strategies also expose search_code, read_file, run_command, and finish. -Recovery state must not add, remove, reorder, or redefine tools later in the -run. - -The run-start trace records at least: +The run_start trace records: ~~~json { @@ -131,450 +83,267 @@ The run-start trace records at least: "tool_names": [ "search_code", "read_file", - "replace_text", "apply_patch", + "replace_text", "run_command", "finish" - ], - "max_edit_failures_per_revision": 4 + ] } ~~~ -Prompt and schema fingerprints are not required for the first implementation. -The frozen strategy and tool names, combined with the existing model request -trace, are sufficient to audit the comparison. +## 3. Replace-first routing -### 4.2 Routing policy +Use replace_text when all of these are true: -For patch-only, every workspace edit is expressed as apply_patch. +- 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. -For replace-first, use this selection matrix: +Use apply_patch directly when: -| Edit intent | Preferred tool | Reason | -| --- | --- | --- | -| Localized change to an existing regular UTF-8 file with an exact unique anchor | replace_text | Avoid model-generated diff hunk metadata | -| Create a file | apply_patch | replace_text supports existing files only | -| Delete a file | apply_patch | replace_text cannot delete files | -| Rename a file | apply_patch when the patch tool supports the operation; otherwise report unsupported | Do not simulate a rename with text replacement | -| Large structural rewrite | apply_patch | An exact replacement would reproduce too much source | -| Exact anchor would be impractically large | apply_patch | Keep replacement requests bounded | -| Target is unsupported by replace_text but valid for apply_patch | apply_patch | Use the tool whose contract covers the operation | -| Exact uniqueness is unknown | read_file first | Do not guess between locations | +- 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. -Localized, large, and impractically large are model judgments guided by the -prompt. The tools remain the deterministic safety boundary: they validate -paths, hashes, exact matches, patch syntax, context, and transactionality. +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. -Issue #10 lists renames as an apply_patch case, while the current patch contract -from Issue #8 rejects rename metadata. Strategy routing may select the patch -path, but Issue #10 must not silently expand the patch tool contract; until -rename support is added separately, the operation fails as unsupported. +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.3 Prompt contract +## 4. Recovery policy -The patch-only prompt instructs the model to: +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: -- read current files before editing; -- use apply_patch for all workspace edits; -- regenerate a patch from fresh content after a structured failure. +- 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 replace-first prompt instructs the model to: +The strategy prompt contains this recovery matrix: -- prefer replace_text only for localized exact unique replacement; -- use apply_patch directly for creation, deletion, structural edits, or - unsupported replacement targets; -- observe the recovery matrix in Section 6; -- never propose a same-turn fallback after replace_text; -- re-read when an error says current source context is required. +| 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. | -Prompt guidance influences routing but is not treated as a safety guarantee. +invalid_patch comes from Issue #8, on which Issue #10 depends. -## 5. Agent-turn control algorithm +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. -The existing model → planner → executor → observation loop remains in place. -Issue #10 adds only the strategy selection and edit isolation described below. +## 5. One editing operation per turn -### 5.1 Batch preflight +Define: -Before tool execution, count replace_text and apply_patch calls in the assistant -response. +~~~python +EDITING_TOOLS = {"replace_text", "apply_patch"} +~~~ -If more than one editing operation is present: +Before execution, the Planner counts editing calls in the Assistant response. +If the count is greater than one, Yada rejects the complete batch: -- reject the complete tool-call batch using the existing Planner/Executor - rejection path; -- return a structured multiple_edit_operations result for every call; -- leave workspace and verification state unchanged; -- let the next model turn choose one operation after observing the rejection. +~~~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 prevents a response such as: +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 ~~~ -from acting as a precomputed fallback. The patch was generated before the model -knew whether the replacement had failed. - -### 5.2 Failure observation barrier - -When the single editing operation fails: - -1. Preserve its error_code, human-readable error, and bounded details. -2. Rely on the one-edit-per-turn preflight to ensure that no later editing - operation exists in that assistant response. -3. Append the assistant message and one result for every tool call. -4. Send those observations in the next model request. -5. Permit a later edit only after the error-specific read requirement has been - satisfied. - -Every model tool call must still receive a result. A rejected batch must not -leave the provider conversation with an unmatched tool_call. - -### 5.3 Recovery read visibility - -Some errors require a fresh read before the next edit. The minimal controller -only needs to remember: - -- the failed step; -- the error code; -- affected paths; -- whether a required read result has become visible to the model. - -No automatic read is required. The model requests read_file in the next turn, -and Yada returns the normal bounded content and current SHA. - -If an error occurs in step N: - -- a read already visible before step N is not post-failure evidence; -- a read proposed in step N + 1 becomes visible to the model in step N + 2; -- an edit also proposed in step N + 1 was generated without seeing that read - and causes that complete batch to be rejected when fresh context is required; -- an edit proposed in step N + 2 may use the read result. - -Consequently, recovery that requires fresh content uses a read-only Agent turn -followed by an editing turn. This fits the existing whole-batch rejection path -and avoids introducing per-call scheduling. +has no conditional if-replace-fails semantics. The patch is an unconditional, +precomputed second edit, not an evidence-based fallback. -This is the semantic purpose previously represented by -available_to_model_at_step. It should be implemented with the smallest state -that fits the existing Planner and Executor boundaries. +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. -### 5.4 Successful edit +## 6. End-to-end algorithm -Successful replace_text and apply_patch calls continue to use the existing tool -state: - -- increment workspace revision; -- update touched files; -- invalidate the verified revision; -- require a relevant successful test or build before finish. - -A successful edit clears the pending recovery requirement and resets the -per-revision edit-failure counter. - -## 6. Recovery matrix - -Recovery is model-driven and occurs in later Agent turns. Yada may enforce a -required post-failure read, but it does not generate a replacement or patch. - -| Error code | Fresh read required before another edit? | Next model action | Exhaustion behavior | -| --- | --- | --- | --- | -| stale_hash | Yes | Read affected files and reconstruct the edit with current SHA and content. Do not fall back automatically. | Count the failed edit; the per-revision or max_steps boundary eventually ends repeated races. | -| no_match | Yes | Read relevant content, then use current exact text or deliberately generate a new patch. | No forced second-attempt transition; further failures consume the shared edit-failure limit. | -| ambiguous_match | Yes | Read a narrower range or enlarge the exact anchor until the target is unique. | No fixed one-retry limit. Continue only while shared budgets remain; ambiguity alone never authorizes a blind patch. | -| invalid_edit | No | Correct the arguments in a later turn. | Repeated failures consume the shared edit-failure limit. | -| unsupported_target | No, unless current source is needed to build a patch | Use apply_patch only if that operation is valid under its contract; otherwise report the unsupported operation. | No hidden conversion or mutation. | -| invalid_patch | No for syntax-only errors; read if source context may be stale | Correct or regenerate the unified diff. | Repeated failures consume the shared edit-failure limit. | -| patch_context_mismatch | Yes | Read affected files and regenerate the patch from current content. | Repeated failures consume the shared edit-failure limit. | -| apply_failed | No automatic recovery | Preserve diagnostics and fail loudly; the model may inspect the cause, but Yada performs no fallback edit. | The shared boundaries end repeated attempts. | - -The invalid_patch row comes from Issue #8, on which Issue #10 depends. - -### 6.1 Clarification for ambiguous_match - -Issue #10 says to read a narrower range or enlarge the exact anchor until it is -unique. It does not specify that only one corrected replacement is allowed. - -Therefore the first implementation must not: - -- force PATCH_REQUIRED after one ambiguous retry; -- terminate as unresolved_ambiguity after one retry; -- treat ambiguity itself as permission to patch an uncertain target. - -The model may make multiple evidence-based attempts, bounded by the shared -per-revision failure limit and max_steps. A deliberate patch is acceptable only -after fresh context makes the intended target unambiguous; it is never an -automatic reaction to ambiguous_match. - -## 7. Loop prevention - -### 7.1 Two boundaries - -The first implementation uses only two loop boundaries: - -1. max_steps, which already decreases on every model turn and guarantees that - the run is finite; -2. MAX_EDIT_FAILURES_PER_REVISION, initially 4, which ends repeated failed - editing attempts earlier at one unchanged workspace revision. - -The per-revision counter: - -- increments when replace_text or apply_patch actually executes and fails; -- does not increment for read-only calls; -- resets after a successful edit advances the revision; -- ends the run as unfinished with edit_failure_budget_exhausted when it reaches - the configured limit. +~~~text +initialize run + select editing strategy + build strategy prompt and tool interface once + record strategy and tool names -Rejected protocol calls can rely on max_steps in the first implementation. -There is no separate protocol-violation budget, no no-progress budget, and no -per-error retry budget. +for each model turn up to max_steps + request completion with the frozen prompt and schemas + parse tool calls -### 7.2 What these boundaries guarantee + 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 -They guarantee finite execution, not successful completion: + execute the accepted calls in their existing order + append every tool result -- patch-only may repeatedly generate invalid patches; -- replace-first may repeatedly produce no_match or ambiguous_match; -- the model may ignore a requested read; -- the model may avoid editing entirely. + if an edit failed + the next model turn observes its structured error + the model follows the prompt recovery matrix -Actual failed edits are stopped early by the per-revision limit. Other -non-progress behavior is stopped by max_steps. + if verified finish succeeds + end successfully -Additional counters should be introduced only after traces or the Issue #10 -benchmark demonstrate a loop pattern that these two boundaries cannot diagnose -or control adequately. +end unfinished when max_steps is exhausted +~~~ -No formal budget-vector proof is necessary: max_steps alone is already a -strictly decreasing global bound. +There is no same-call fallback and no host-generated mutation. -## 8. Control flows +## 7. Flowcharts -### 8.1 Overall flow +### 7.1 Overall control flow ~~~mermaid flowchart TD A["Start run"] --> B{"Editing strategy"} - B -->|"patch-only"| C["Freeze prompt and tools: apply_patch"] - B -->|"replace-first"| D["Freeze prompt and tools: replace_text + apply_patch"] + 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 inspects proposed tool calls"] - F --> G{"More than one editing operation?"} - G -->|"Yes"| H["Reject complete batch with no side effects"] - H --> I["Return structured results in next model request"] - I --> N{"max_steps exhausted?"} - - G -->|"No"| J{"Required read pending and edit proposed?"} - J -->|"Yes"| K["Reject complete batch and request read-only turn"] - K --> I - J -->|"No"| L["Execute tools in existing order"] - - L --> M{"Editing result"} - M -->|"Success"| O["Advance revision and require verification"] - O --> P["Continue normal Agent loop"] - P --> N - - M -->|"Failure"| Q["Return structured error; increment revision failure count"] - Q --> R{"Failure limit reached?"} - R -->|"Yes"| S["End unfinished: edit_failure_budget_exhausted"] - R -->|"No"| T["Record any required post-failure read"] - T --> I - - M -->|"No edit"| P - N -->|"No"| E - N -->|"Yes"| U["End unfinished: max_steps_exhausted"] + 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"] ~~~ -### 8.2 Replace-first routing and recovery +### 7.2 Replace-first fallback ~~~mermaid flowchart TD - A["Model evaluates edit intent"] --> B{"Localized existing text with exact unique anchor?"} - B -->|"Yes"| C["Propose replace_text"] - B -->|"No"| D["Propose apply_patch"] + A["Model evaluates edit"] --> B{"Localized existing text with exact unique anchor?"} + B -->|"Yes"| C["replace_text"] + B -->|"No"| D["apply_patch"] - C --> E{"replace_text result"} + C --> E{"Result"} E -->|"Success"| F["Verify latest revision"] - E -->|"stale_hash or no_match"| G["Next turn: read current relevant content"] - E -->|"ambiguous_match"| H["Next turn: read narrower range or match windows"] - E -->|"invalid_edit"| I["Next turn: correct arguments"] - E -->|"unsupported_target"| J["Next turn: choose apply_patch only if valid"] - E -->|"apply_failed"| K["Preserve diagnostics; no automatic fallback"] - - G --> L["Following turn: corrected replace or deliberate patch"] - H --> M["Following turn: use a larger unique anchor"] - M --> E - I --> C - J --> D - L --> N{"Chosen tool"} - N -->|"replace_text"| C - N -->|"apply_patch"| D - - D --> O{"apply_patch result"} - O -->|"Success"| F - O -->|"stale_hash or patch_context_mismatch"| P["Next turn: read affected files"] - O -->|"invalid_patch"| Q["Next turn: correct or regenerate patch"] - O -->|"apply_failed"| K - P --> R["Following turn: regenerate patch"] - Q --> R - R --> D - - E -->|"Any repeated failure"| S["Consume shared per-revision failure limit"] - O -->|"Any repeated failure"| S - S --> T{"Limit or max_steps reached?"} - T -->|"No"| A - T -->|"Yes"| U["End unfinished with explicit reason"] + 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.3 Patch-only behavior +## 8. Trace and evaluation -patch-only follows the same recovery matrix but never exposes replace_text: +Existing tool_call and tool_result events already record: -~~~text -read current source - → propose apply_patch - → success: verify - → structured failure: observe it in the next model turn - → perform any required fresh read - → regenerate the patch in a later turn - → stop at the per-revision failure limit or max_steps -~~~ - -There is no patch-specific retry counter. A valid patch may succeed on any -attempt before the shared boundaries are reached. +- chosen editing tool; +- arguments and correlation ID; +- success or failure; +- structured error code and bounded details; +- the later calls that form the recovery path. -## 9. Trace requirements +Issue #10 adds editing_strategy and frozen tool_names to run_start. Batch +rejection is recorded as a protocol_violation with +multiple_edit_operations. -The existing tool_call and tool_result events already record: +Native evaluation results also record the strategy and editing metrics: -- selected tool; -- arguments; -- success or failure; -- structured error_code; -- Agent step and tool-call correlation. +- first edit-attempt success; +- eventual mutation success; +- edit attempts and retries; +- replace and patch attempt counts; +- rejected editing calls; +- error-code counts; +- verification success after mutation; +- Agent steps and token usage. -Issue #10 adds editing_strategy and the frozen tool names to run_start. It also -needs a bounded trace record when Yada rejects an edit because: +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. -- multiple editing operations were proposed in one turn; -- a required read was not yet visible to the model; -- the per-revision failure limit was exhausted. +Fair comparison requires the same: -The recovery path can then be reconstructed from existing ordered model, call, -and result events. A large family of recovery_started, recovery_read, and -recovery_transition events is not required unless implementation experience -shows that existing traces are insufficient. +- tasks and base commits; +- model and model parameters; +- step, token, command, and wall-time budgets; +- grading logic; +- number of repeated trials. -An unfinished run records a stable terminal reason such as: +Run each task with both: -- edit_failure_budget_exhausted; -- max_steps_exhausted; -- an existing fatal tool or runtime error. +~~~text +yada eval ... --editing-strategy patch-only +yada eval ... --editing-strategy replace-first +~~~ -## 10. Deterministic tests +No benchmark winner is claimed until those controlled runs exist. -Mocked model and tool interactions should cover: +## 9. Deterministic tests -### Strategy stability +Tests cover: - patch-only exposes apply_patch but not replace_text; - replace-first exposes both editing tools; -- strategy and schemas remain stable across all model requests; -- run_start records strategy, tool names, and the edit-failure limit. - -### Routing - -- a localized existing-file change is prompted toward replace_text; -- creation, deletion, and structural edits are prompted toward apply_patch; -- patch-only never exposes replace_text. - -### Turn isolation and visibility - -- a turn with replace_text and apply_patch executes neither edit; -- a failed edit appears in the next model request; -- every call in a rejected batch receives a tool result; -- a read and edit generated in the same recovery turn cause the batch to be - rejected and cannot satisfy the fresh-read requirement; -- an edit in the following turn can use the now-visible read. - -### Recovery - -- stale_hash requires a fresh read before retry; -- no_match permits a corrected replacement or deliberate patch after a read; -- repeated ambiguous_match can continue with larger anchors while shared - budgets remain; -- ambiguity never causes automatic patch fallback; -- invalid_edit permits corrected arguments in a later turn; -- unsupported_target permits patch only when the patch contract supports it; -- invalid_patch permits a regenerated patch; -- patch_context_mismatch requires a fresh read; -- apply_failed preserves diagnostics and performs no hidden edit. - -### Loop and safety boundaries - -- four failed edits at one revision end with - edit_failure_budget_exhausted; -- a successful edit resets the per-revision failure counter; -- text-only, repeated-read, or rejected-call loops still end at max_steps; -- failed and rejected edits leave files, revision, touched paths, and - verification unchanged; -- successful fallback remains SHA-bound, transactional, and subject to - post-edit verification. - -## 11. Evaluation - -Compare patch-only and replace-first using: - -- the same task set and base commits; -- the same model and model parameters; -- the same step, token, command, and wall-time budgets; -- stable prompts and tool schemas within each strategy; -- repeated runs when model nondeterminism requires them. - -Record at least: - -- first edit-attempt success rate; -- eventual mutation success rate; -- completed or resolved task rate; -- edit retries; -- Agent turns and total steps; -- input and output tokens; -- verification success after mutation; -- unrelated changed lines; -- wrong-target or partial-target mutations, which must remain zero; -- terminal reason distribution. - -The first benchmark should run with max_steps and -MAX_EDIT_FAILURES_PER_REVISION only. Add more specialized retry controls only -when a measured failure mode justifies them. - -## 12. Implementation guidance - -Keep the implementation within the existing Planner, Executor, ToolRunner, -prompt, trace, CLI, and evaluation boundaries described in -[Yada architecture](architecture.md). - -The design deliberately does not define a new editing.py state machine or copy -the complete Agent loop into this document. Source code and tests are the -authoritative description of implementation mechanics. After Issue #10 lands, -this section should link to the small policy functions and their tests rather -than duplicate them. - -replace_text and apply_patch remain data-plane tools. Neither contains routing -policy or invokes the other as a strategy fallback. - -## 13. Industry context - -Exact unique replacement, optimistic concurrency checks, fail-closed -transactions, structured errors, and trace-based evaluation are established -patterns rather than one universal editing algorithm. Comparable examples -include [Gemini CLI file tools](https://github.com/google-gemini/gemini-cli/blob/main/docs/tools/file-system.md), -[Claude text editor](https://platform.claude.com/docs/en/agents-and-tools/tool-use/text-editor-tool), -and HTTP [If-Match](https://www.rfc-editor.org/rfc/rfc9110.html#section-13.1.1). - -Yada keeps only the parts needed to test Issue #10 without turning the harness -into a general workflow engine. +- 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..c4bffdb 100644 --- a/docs/evaluation.md +++ b/docs/evaluation.md @@ -171,6 +171,7 @@ 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 @@ -179,6 +180,13 @@ During the run: 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, edit retries, 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..b9d1536 100644 --- a/src/yada/agents/default.py +++ b/src/yada/agents/default.py @@ -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( 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..6671767 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)}, ] @@ -99,16 +115,27 @@ def plan( ) rejection_error = None - if len(tool_calls) > 1 and any( + 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" for call in tool_calls ): # A concurrent finish 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_code = "finish_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..ba92ca9 100644 --- a/src/yada/agents/prompts.py +++ b/src/yada/agents/prompts.py @@ -1,20 +1,28 @@ """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 +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. Prefer small, targeted edits. Do not rewrite unrelated code. +4. Run the most relevant available tests after the last edit. 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. + build succeeds after the latest edit. 7. 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. @@ -22,12 +30,60 @@ 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. """ +_PATCH_ONLY_POLICY = """ +Editing strategy: patch-only. +- Use apply_patch for every workspace edit. +- stale_hash: re-read affected files before retrying. +- invalid_patch: correct or regenerate the patch. +- patch_context_mismatch: re-read affected files and regenerate the patch. +- apply_failed: preserve the diagnostic evidence; do not attempt hidden recovery. +- apply_patch: make a version-checked unified-diff edit. +""" + +_REPLACE_FIRST_POLICY = """ +Editing strategy: replace-first. +- Prefer replace_text for a localized edit to an existing regular text file when + old_text is an exact, unique, reasonably bounded anchor. +- Use apply_patch directly for file creation or deletion, large structural rewrites, + impractically large anchors, and operations unsupported by replace_text. +- Submit at most one editing operation per assistant turn. A patch generated in the + same turn as a replacement is not a fallback. +- Fallback means choosing apply_patch in a later turn after observing the failed + replacement and re-reading when required. +- stale_hash: re-read before retrying; never fall back automatically. +- no_match: re-read relevant content, then use current exact text or a deliberate patch. +- ambiguous_match: read a narrower range or enlarge the anchor until it is unique. +- invalid_edit: correct the arguments in a later turn. +- unsupported_target: use apply_patch only if its contract supports the operation. +- invalid_patch: correct or regenerate the patch. +- patch_context_mismatch: re-read affected files and regenerate the patch. +- apply_failed: preserve the diagnostic evidence; do not attempt hidden fallback. +- replace_text: make exact, unique, version-checked replacements in existing text. +- apply_patch: make a version-checked unified-diff edit. +""" + + +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..85aba2a --- /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.PATCH_ONLY +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..bbac04b 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,56 @@ 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 + return { + "first_edit_attempt_success": first_success, + "eventual_mutation_success": state.patch_count > 0, + "edit_attempts": len(attempts), + "edit_retries": max(0, len(attempts) - 1), + "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..488d762 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,12 @@ 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.", + ) model.add_argument("--api-timeout", type=int, default=300) model.add_argument("--command-timeout", type=int, default=120) model.add_argument( @@ -144,6 +151,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..33b3d76 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="Run-level editing policy (default: patch-only).", + ) 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/runner.py b/src/yada/tools/runner.py index 2233182..c5b8c4b 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, @@ -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. diff --git a/src/yada/traces/html.py b/src/yada/traces/html.py index 5d2babb..fd1c2c3 100644 --- a/src/yada/traces/html.py +++ b/src/yada/traces/html.py @@ -118,6 +118,14 @@ def _render_run_details(run: TraceRun) -> str: start.get("model_config", "Unavailable"), open_by_default=False, ) + details += _details( + "Editing strategy", + { + "editing_strategy": start.get("editing_strategy", "legacy"), + "tool_names": start.get("tool_names", "Unavailable"), + }, + open_by_default=False, + ) details += _details( "Provenance", start.get("provenance", "Unavailable"), 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..a3ef1ca 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 @@ -161,9 +163,13 @@ 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"] == "patch-only" + assert "apply_patch" in run_start["tool_names"] + assert "replace_text" not 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: @@ -183,6 +189,219 @@ def test_planner_rejects_finish_mixed_with_other_calls() -> None: assert plan.rejection_error == ( "finish must be the only tool call in its assistant turn" ) + assert plan.rejection_error_code == "finish_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 "Editing strategy: patch-only" in patch_prompt + assert "Use apply_patch for every workspace edit" in patch_prompt + assert "Editing strategy: replace-first" in replace_prompt + assert "Prefer replace_text for a localized edit" in replace_prompt + assert "stale_hash: re-read before retrying" 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("replace-first"), + ) + + +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..99b3a94 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 == "patch-only" + + +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..291d672 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["edit_retries"] == 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..44387ab --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,15 @@ +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 == "patch-only" + assert replace_first.editing_strategy == "replace-first" diff --git a/tests/tools/test_runner.py b/tests/tools/test_runner.py index 962d55a..f9b0272 100644 --- a/tests/tools/test_runner.py +++ b/tests/tools/test_runner.py @@ -49,6 +49,30 @@ def answer(): """ +def test_editing_strategy_freezes_public_tool_interface( + git_workspace: Path, +) -> None: + patch_only = ToolRunner( + git_workspace, + approver=CommandApprover("allow"), + ) + replace_first = ToolRunner( + git_workspace, + approver=CommandApprover("allow"), + editing_strategy="replace-first", + ) + + 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: diff --git a/tests/traces/test_trace_html.py b/tests/traces/test_trace_html.py index c456d46..acc0bcf 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,8 @@ 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 "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..f207189 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"], "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 From c7d1c7f8ecf6ced0cc7a471aab8adfac3d5818a5 Mon Sep 17 00:00:00 2001 From: Gen TANG Date: Tue, 4 Aug 2026 23:39:15 +0800 Subject: [PATCH 4/6] upgrade prompt --- docs/cli-reference.md | 4 ++-- docs/configuration.md | 5 +++-- docs/dev/editing-strategy.md | 11 +++++++++-- src/yada/agents/prompts.py | 20 ++++++++------------ src/yada/editing.py | 2 +- src/yada/evals/cli.py | 5 ++++- src/yada/run/cli.py | 5 ++++- src/yada/traces/html.py | 22 +++++++++++++--------- tests/agents/test_default.py | 11 +++++++---- tests/evals/test_eval_cli.py | 2 +- tests/test_cli.py | 2 +- tests/tools/test_runner.py | 7 +++++++ tests/traces/test_trace_html.py | 5 +++++ 13 files changed, 65 insertions(+), 36 deletions(-) diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 5dcbb88..b06edeb 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -36,7 +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. | `patch-only` | +| `--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` | @@ -290,7 +290,7 @@ 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. | `patch-only` | +| `--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 diff --git a/docs/configuration.md b/docs/configuration.md index a24f291..ee3d51d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -79,12 +79,13 @@ Editing strategy is frozen for the complete run: | `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. | -`patch-only` is the default until controlled evaluation supports changing it: +`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 replace-first + --editing-strategy patch-only ``` Both strategies allow at most one editing tool call per Assistant turn. This diff --git a/docs/dev/editing-strategy.md b/docs/dev/editing-strategy.md index dd751a0..a7dc2b7 100644 --- a/docs/dev/editing-strategy.md +++ b/docs/dev/editing-strategy.md @@ -63,7 +63,8 @@ Yada supports: | 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. | -patch-only remains the default until benchmark evidence supports changing it. +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: @@ -124,7 +125,7 @@ model has observed a replace_text failure. It does not mean: - Yada converting failed replacement arguments into a patch; - the model submitting replace_text and apply_patch in the same response. -The strategy prompt contains this recovery matrix: +The detailed recovery matrix is the design and test reference: | Error code | Required model response | | --- | --- | @@ -139,6 +140,12 @@ The strategy prompt contains this recovery matrix: invalid_patch comes from Issue #8, on which Issue #10 depends. +The system prompt summarizes this table as a small number of principles: use the +structured error, re-read when the target may be stale or unclear, retry or fall +back only in a later turn, correct invalid arguments, and preserve apply_failed +diagnostics. 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. diff --git a/src/yada/agents/prompts.py b/src/yada/agents/prompts.py index ba92ca9..9727f92 100644 --- a/src/yada/agents/prompts.py +++ b/src/yada/agents/prompts.py @@ -37,10 +37,8 @@ _PATCH_ONLY_POLICY = """ Editing strategy: patch-only. - Use apply_patch for every workspace edit. -- stale_hash: re-read affected files before retrying. -- invalid_patch: correct or regenerate the patch. -- patch_context_mismatch: re-read affected files and regenerate the patch. -- apply_failed: preserve the diagnostic evidence; do not attempt hidden recovery. +- After a patch failure, follow its structured error: re-read stale or mismatched + targets, correct invalid patches, and preserve apply_failed diagnostics. - apply_patch: make a version-checked unified-diff edit. """ @@ -50,18 +48,16 @@ old_text is an exact, unique, reasonably bounded anchor. - Use apply_patch directly for file creation or deletion, large structural rewrites, impractically large anchors, and operations unsupported by replace_text. +- Once the target and exact replacement are clear, edit promptly. Do not repeat + searches that only confirm already established facts. - Submit at most one editing operation per assistant turn. A patch generated in the same turn as a replacement is not a fallback. - Fallback means choosing apply_patch in a later turn after observing the failed replacement and re-reading when required. -- stale_hash: re-read before retrying; never fall back automatically. -- no_match: re-read relevant content, then use current exact text or a deliberate patch. -- ambiguous_match: read a narrower range or enlarge the anchor until it is unique. -- invalid_edit: correct the arguments in a later turn. -- unsupported_target: use apply_patch only if its contract supports the operation. -- invalid_patch: correct or regenerate the patch. -- patch_context_mismatch: re-read affected files and regenerate the patch. -- apply_failed: preserve the diagnostic evidence; do not attempt hidden fallback. +- After an edit failure, use its structured error and current file contents to decide + a later-turn retry or fallback; re-read whenever the target may be stale or unclear. +- Correct invalid arguments, use patches only for supported operations, and preserve + apply_failed diagnostics instead of attempting hidden recovery. - replace_text: make exact, unique, version-checked replacements in existing text. - apply_patch: make a version-checked unified-diff edit. """ diff --git a/src/yada/editing.py b/src/yada/editing.py index 85aba2a..f713ec4 100644 --- a/src/yada/editing.py +++ b/src/yada/editing.py @@ -12,7 +12,7 @@ class EditingStrategy(str, Enum): REPLACE_FIRST = "replace-first" -DEFAULT_EDITING_STRATEGY = EditingStrategy.PATCH_ONLY +DEFAULT_EDITING_STRATEGY = EditingStrategy.REPLACE_FIRST EDITING_STRATEGY_CHOICES = tuple(strategy.value for strategy in EditingStrategy) diff --git a/src/yada/evals/cli.py b/src/yada/evals/cli.py index 488d762..597fd9b 100644 --- a/src/yada/evals/cli.py +++ b/src/yada/evals/cli.py @@ -85,7 +85,10 @@ def build_parser() -> argparse.ArgumentParser: "--editing-strategy", choices=EDITING_STRATEGY_CHOICES, default=DEFAULT_EDITING_STRATEGY.value, - help="Run-level editing policy for the native Yada agent.", + 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) diff --git a/src/yada/run/cli.py b/src/yada/run/cli.py index 33b3d76..8ac7c35 100644 --- a/src/yada/run/cli.py +++ b/src/yada/run/cli.py @@ -52,7 +52,10 @@ def build_parser() -> argparse.ArgumentParser: "--editing-strategy", choices=EDITING_STRATEGY_CHOICES, default=DEFAULT_EDITING_STRATEGY.value, - help="Run-level editing policy (default: patch-only).", + help=( + "Run-level editing policy " + f"(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) diff --git a/src/yada/traces/html.py b/src/yada/traces/html.py index fd1c2c3..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,15 +127,7 @@ def _render_run_details(run: TraceRun) -> str: ) details += _details( "Model configuration", - start.get("model_config", "Unavailable"), - open_by_default=False, - ) - details += _details( - "Editing strategy", - { - "editing_strategy": start.get("editing_strategy", "legacy"), - "tool_names": start.get("tool_names", "Unavailable"), - }, + model_config, open_by_default=False, ) details += _details( diff --git a/tests/agents/test_default.py b/tests/agents/test_default.py index a3ef1ca..b76a7c4 100644 --- a/tests/agents/test_default.py +++ b/tests/agents/test_default.py @@ -163,9 +163,9 @@ 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"] == "patch-only" + assert run_start["editing_strategy"] == "replace-first" assert "apply_patch" in run_start["tool_names"] - assert "replace_text" not 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"] @@ -201,9 +201,12 @@ def test_strategy_prompts_are_explicit_and_stable() -> None: assert "Editing strategy: patch-only" in patch_prompt assert "Use apply_patch for every workspace edit" in patch_prompt + assert "After a patch failure, follow its structured error" in patch_prompt assert "Editing strategy: replace-first" in replace_prompt assert "Prefer replace_text for a localized edit" in replace_prompt - assert "stale_hash: re-read before retrying" in replace_prompt + assert "Once the target and exact replacement are clear" in replace_prompt + assert "Do not repeat" in replace_prompt + assert "After an edit failure, use its structured error" in replace_prompt assert replace_prompt == replace_planner.initial_messages("Fix it")[0]["content"] @@ -215,7 +218,7 @@ def test_agent_rejects_mismatched_strategy_components(tmp_path: Path) -> None: client=FakeClient([]), tools=runner, trace=TraceWriter(None), - planner=Planner("replace-first"), + planner=Planner("patch-only"), ) diff --git a/tests/evals/test_eval_cli.py b/tests/evals/test_eval_cli.py index 99b3a94..2105df5 100644 --- a/tests/evals/test_eval_cli.py +++ b/tests/evals/test_eval_cli.py @@ -15,7 +15,7 @@ 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 == "patch-only" + assert case.editing_strategy == "replace-first" def test_eval_cli_exposes_editing_strategy() -> None: diff --git a/tests/test_cli.py b/tests/test_cli.py index 44387ab..db609f3 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -11,5 +11,5 @@ def test_direct_cli_exposes_editing_strategy() -> None: ["Fix it", "--editing-strategy", "replace-first"] ) - assert default.editing_strategy == "patch-only" + assert default.editing_strategy == "replace-first" assert replace_first.editing_strategy == "replace-first" diff --git a/tests/tools/test_runner.py b/tests/tools/test_runner.py index f9b0272..dab1af7 100644 --- a/tests/tools/test_runner.py +++ b/tests/tools/test_runner.py @@ -52,9 +52,14 @@ 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, @@ -62,6 +67,8 @@ def test_editing_strategy_freezes_public_tool_interface( editing_strategy="replace-first", ) + assert default_runner.editing_strategy.value == "replace-first" + assert "replace_text" in default_runner.tool_names 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 diff --git a/tests/traces/test_trace_html.py b/tests/traces/test_trace_html.py index acc0bcf..fad0567 100644 --- a/tests/traces/test_trace_html.py +++ b/tests/traces/test_trace_html.py @@ -106,6 +106,11 @@ def test_completed_trace_renders_offline_semantic_view( 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 From a1f417243d903cd5e4eff9eb71dafa8b6fdf3f9d Mon Sep 17 00:00:00 2001 From: Gen TANG Date: Wed, 5 Aug 2026 00:33:06 +0800 Subject: [PATCH 5/6] upgrade prompt --- docs/dev/architecture.md | 10 ++-- docs/dev/debugging.md | 4 +- docs/dev/editing-strategy.md | 17 +++--- docs/evaluation.md | 12 ++--- src/yada/agents/default.py | 14 ++++- src/yada/agents/planning.py | 18 ++++--- src/yada/agents/prompts.py | 59 +++++++++------------ src/yada/evals/agents/yada.py | 4 +- src/yada/tools/command.py | 2 +- src/yada/tools/finish.py | 8 +-- src/yada/tools/patch.py | 2 +- src/yada/tools/replace.py | 2 +- src/yada/tools/runner.py | 6 +-- src/yada/tools/schemas.py | 6 +-- tests/agents/test_default.py | 87 +++++++++++++++++++++++++++---- tests/evals/test_yada_agent.py | 3 +- tests/tools/test_replace.py | 2 +- tests/tools/test_runner.py | 7 ++- tests/traces/test_trace_report.py | 16 +++--- 19 files changed, 178 insertions(+), 101 deletions(-) diff --git a/docs/dev/architecture.md b/docs/dev/architecture.md index 64bc8eb..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,7 +85,7 @@ 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 or more than +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, @@ -104,7 +104,7 @@ 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. At run start it freezes either the `patch-only` interface or the @@ -149,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; @@ -232,7 +232,7 @@ load, workspace, grading, cache, and artifact sequence. 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` requires verification of the latest revision. +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. diff --git a/docs/dev/debugging.md b/docs/dev/debugging.md index 78b144b..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 @@ -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 index a7dc2b7..7e0baf7 100644 --- a/docs/dev/editing-strategy.md +++ b/docs/dev/editing-strategy.md @@ -41,7 +41,7 @@ The model is responsible for: - 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 finish. +- verifying the final revision before calling finish_task. ### 1.3 Yada host @@ -87,7 +87,7 @@ The run_start trace records: "apply_patch", "replace_text", "run_command", - "finish" + "finish_task" ] } ~~~ @@ -140,11 +140,10 @@ The detailed recovery matrix is the design and test reference: invalid_patch comes from Issue #8, on which Issue #10 depends. -The system prompt summarizes this table as a small number of principles: use the -structured error, re-read when the target may be stale or unclear, retry or fall -back only in a later turn, correct invalid arguments, and preserve apply_failed -diagnostics. Keeping the full table here avoids paying for and repeatedly presenting -the same verbose matrix on every model turn. +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 @@ -211,7 +210,7 @@ for each model turn up to max_steps the next model turn observes its structured error the model follows the prompt recovery matrix - if verified finish succeeds + if verified finish_task succeeds end successfully end unfinished when max_steps is exhausted @@ -294,7 +293,7 @@ Native evaluation results also record the strategy and editing metrics: - first edit-attempt success; - eventual mutation success; -- edit attempts and retries; +- edit attempts, additional attempts, and failed attempts; - replace and patch attempt counts; - rejected editing calls; - error-code counts; diff --git a/docs/evaluation.md b/docs/evaluation.md index c4bffdb..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 @@ -174,7 +174,7 @@ During the run: - 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 @@ -182,10 +182,10 @@ 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, edit retries, 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`. +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 diff --git a/src/yada/agents/default.py b/src/yada/agents/default.py index b9d1536..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. @@ -234,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/planning.py b/src/yada/agents/planning.py index 6671767..f93726b 100644 --- a/src/yada/agents/planning.py +++ b/src/yada/agents/planning.py @@ -91,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. """ @@ -99,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 += ( @@ -125,12 +125,14 @@ def plan( ) rejection_error_code = "multiple_edit_operations" elif len(tool_calls) > 1 and any( - _tool_name(call) == "finish" for call in tool_calls + _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_code = "finish_must_be_alone" + 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, diff --git a/src/yada/agents/prompts.py b/src/yada/agents/prompts.py index 9727f92..815a222 100644 --- a/src/yada/agents/prompts.py +++ b/src/yada/agents/prompts.py @@ -14,52 +14,43 @@ correct patch. Work directly with tools. Be concise and evidence-driven. Rules: -1. Search before reading, and read a file before editing it. +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. Prefer small, targeted edits. Do not rewrite unrelated code. -4. Run the most relevant available tests after the last edit. 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 edit. -7. Stay inside the workspace. Do not access secrets, hidden grader tests, the network, +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. -- 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 patch failure, follow its structured error: re-read stale or mismatched - targets, correct invalid patches, and preserve apply_failed diagnostics. -- apply_patch: make a version-checked unified-diff 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. -- Prefer replace_text for a localized edit to an existing regular text file when - old_text is an exact, unique, reasonably bounded anchor. -- Use apply_patch directly for file creation or deletion, large structural rewrites, - impractically large anchors, and operations unsupported by replace_text. -- Once the target and exact replacement are clear, edit promptly. Do not repeat - searches that only confirm already established facts. -- Submit at most one editing operation per assistant turn. A patch generated in the - same turn as a replacement is not a fallback. -- Fallback means choosing apply_patch in a later turn after observing the failed - replacement and re-reading when required. -- After an edit failure, use its structured error and current file contents to decide - a later-turn retry or fallback; re-read whenever the target may be stale or unclear. -- Correct invalid arguments, use patches only for supported operations, and preserve - apply_failed diagnostics instead of attempting hidden recovery. -- replace_text: make exact, unique, version-checked replacements in existing text. -- apply_patch: make a version-checked unified-diff edit. +- 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. """ diff --git a/src/yada/evals/agents/yada.py b/src/yada/evals/agents/yada.py index bbac04b..40f34e8 100644 --- a/src/yada/evals/agents/yada.py +++ b/src/yada/evals/agents/yada.py @@ -191,11 +191,13 @@ def _editing_metrics(trace_path: Path, tools: ToolRunner) -> dict[str, object]: 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), - "edit_retries": max(0, len(attempts) - 1), + "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, 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 c5b8c4b..68231fc 100644 --- a/src/yada/tools/runner.py +++ b/src/yada/tools/runner.py @@ -19,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 @@ -101,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/tests/agents/test_default.py b/tests/agents/test_default.py index b76a7c4..3320de0 100644 --- a/tests/agents/test_default.py +++ b/tests/agents/test_default.py @@ -99,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" @@ -124,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") @@ -156,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] @@ -172,13 +231,13 @@ def test_debug_trace_reconstructs_exact_client_payload(tmp_path: Path) -> None: 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": "{}"}}, ], } @@ -187,9 +246,9 @@ 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_must_be_alone" + assert plan.rejection_error_code == "finish_task_must_be_alone" def test_strategy_prompts_are_explicit_and_stable() -> None: @@ -199,14 +258,24 @@ def test_strategy_prompts_are_explicit_and_stable() -> None: 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 "After a patch failure, follow its structured error" in patch_prompt + assert "follow the structured recovery instruction" in patch_prompt assert "Editing strategy: replace-first" in replace_prompt - assert "Prefer replace_text for a localized edit" in replace_prompt - assert "Once the target and exact replacement are clear" 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 "After an edit failure, use its structured error" in replace_prompt + assert "Retry or switch tools" in replace_prompt assert replace_prompt == replace_planner.initial_messages("Fix it")[0]["content"] diff --git a/tests/evals/test_yada_agent.py b/tests/evals/test_yada_agent.py index 291d672..1a6b707 100644 --- a/tests/evals/test_yada_agent.py +++ b/tests/evals/test_yada_agent.py @@ -144,7 +144,8 @@ def call(call_id: str, old_text: str) -> dict[str, object]: assert metrics["first_edit_attempt_success"] is False assert metrics["eventual_mutation_success"] is True assert metrics["edit_attempts"] == 2 - assert metrics["edit_retries"] == 1 + 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/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 dab1af7..4b5cce4 100644 --- a/tests/tools/test_runner.py +++ b/tests/tools/test_runner.py @@ -69,6 +69,9 @@ def test_editing_strategy_freezes_public_tool_interface( 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 @@ -352,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( @@ -365,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_report.py b/tests/traces/test_trace_report.py index f207189..75f274b 100644 --- a/tests/traces/test_trace_report.py +++ b/tests/traces/test_trace_report.py @@ -30,7 +30,7 @@ def test_trace_events_have_correlation_metadata_and_redaction(tmp_path: Path) -> "task": "fix", "workspace": ".", "editing_strategy": "patch-only", - "tool_names": ["read_file", "apply_patch", "finish"], + "tool_names": ["read_file", "apply_patch", "finish_task"], "trace_level": "summary", }, ) @@ -418,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, @@ -437,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( @@ -456,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( From 5cbb631bb3faa0bf25bf2c20ac85eb5d253133b4 Mon Sep 17 00:00:00 2001 From: Gen TANG Date: Wed, 5 Aug 2026 00:38:06 +0800 Subject: [PATCH 6/6] fix format --- src/yada/agents/planning.py | 4 +--- src/yada/run/cli.py | 5 +---- tests/evals/test_yada_agent.py | 3 +-- tests/test_cli.py | 4 +--- 4 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/yada/agents/planning.py b/src/yada/agents/planning.py index f93726b..2a755f5 100644 --- a/src/yada/agents/planning.py +++ b/src/yada/agents/planning.py @@ -120,9 +120,7 @@ def plan( _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 = "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 diff --git a/src/yada/run/cli.py b/src/yada/run/cli.py index 8ac7c35..f2978a2 100644 --- a/src/yada/run/cli.py +++ b/src/yada/run/cli.py @@ -52,10 +52,7 @@ def build_parser() -> argparse.ArgumentParser: "--editing-strategy", choices=EDITING_STRATEGY_CHOICES, default=DEFAULT_EDITING_STRATEGY.value, - help=( - "Run-level editing policy " - f"(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) diff --git a/tests/evals/test_yada_agent.py b/tests/evals/test_yada_agent.py index 1a6b707..abae63e 100644 --- a/tests/evals/test_yada_agent.py +++ b/tests/evals/test_yada_agent.py @@ -86,8 +86,7 @@ def test_yada_eval_trace_includes_case_and_workspace_provenance( 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] + schema["function"]["name"] == "replace_text" for schema in client.seen_tools[0] ) assert result.details["editing_strategy"] == "replace-first" metrics = result.details["editing_metrics"] diff --git a/tests/test_cli.py b/tests/test_cli.py index db609f3..4999612 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -7,9 +7,7 @@ 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"] - ) + replace_first = parser.parse_args(["Fix it", "--editing-strategy", "replace-first"]) assert default.editing_strategy == "replace-first" assert replace_first.editing_strategy == "replace-first"