Background
----------
Issue surfaced in production: model uses tools correctly through the
full pipeline (write_file → commit), then on the round expected to call
workspace_finish, replies with the full content as plain assistant text
instead. The committed message survives but the run fails with
`model.tool_call_required`, leaving an orphaned chat message and the UI
locked. PR #62 (issue #55) added rollback signalling so the host can
unwind cleanly, but the model's work is still discarded.
Two complementary fixes:
1. Strengthen `workspace.commit`'s tool result string with an explicit
"REQUIRED NEXT STEP: call workspace_finish" reminder + a
`nextRequiredTool` field in structured payload. This is the highest
attention real-estate in the prompt (last tool result the model sees
before the next turn) and costs essentially nothing.
2. Soft drift recovery: when the model returns 0 tool calls, give it
ONE corrective nudge instead of failing immediately. We push the
model's drifted text into history (so it owns what it said) and
append a synthetic `user` reminder that distinguishes the post-commit
case (commit will be rolled back) from the no-commit case (every
turn must use a tool). If the model recovers (any tool call) the
run continues normally; if it drifts again, we fall through to the
existing #55 path (rollback + `userRetryable=true`).
Hard cap of 1 attempt per run via `DRIFT_RECOVERY_MAX_ATTEMPTS`
prevents token-bombing on stubborn drifters. Multi-turn pattern
verified against Anthropic + OpenAI docs as API-compatible (no
role-alternation constraint after no-tool-use turn).
Backward compatibility
----------------------
- Failed recovery falls through to existing #55 code path: same
`model.tool_call_required` error, same `run_rollback_targets` event,
same `userRetryable=true`. No frontend changes required for the
failure case.
- New `drift_recovery_attempted` event is additive; old hosts ignore.
- Existing #55 drift test extended to include the recovery attempt
before falling through to rollback.
Tests
-----
- `foreground_run_emits_rollback_targets_on_tool_call_required_drift`:
updated to assert exactly one `drift_recovery_attempted` precedes the
rollback when the model drifts twice in a row.
- `foreground_run_recovers_from_post_commit_drift_with_nudge`: new —
model drifts after commit, gets nudged, complies with finish; run
completes, no rollback, commit preserved. Also asserts the nudge text
reaches the model verbatim.
- `foreground_run_recovers_from_no_commit_drift_with_nudge`: new —
model drifts on turn 1, gets nudged, completes the full pipeline.
601 lib tests passing.
Docs
----
- `RunEventJournal.md`: documents `drift_recovery_attempted` event +
recovery flow.
- `ToolSystem.md`: spells out the soft-recovery contract and which
drift codes do/don't get the second chance.
Co-authored-by: Cursor <cursoragent@cursor.com>
PR review surfaced that the rollback path for `run_rollback_targets`
was always calling `deleteMessage(index)`, which wipes the entire chat
entry — including any swipes the user had authored before the agent
run started. For regenerate / swipe generation types the run only
appends a new swipe in-place via `saveReply({ type: 'swipe' })`, so
nuking the whole message is too aggressive.
Captures the rollback strategy at commit time on
`extra.tauritavern.agent.rollback`:
- If the first `saveReply` call grew `chat.length` (the run created a
new chat entry), strategy is `deleteMessage`.
- Otherwise the run appended a swipe to a pre-existing assistant
message, strategy is `deleteSwipe` with the new swipe's id.
The rollback helper now branches on that strategy: it calls vendor's
exported `deleteSwipe(swipeId, messageId)` for swipe-mode targets
(only when the swipes array still has more than one entry so vendor's
"can't delete the last swipe" toastr never fires), and falls back to
`deleteMessage` otherwise. Messages persisted before this field
existed default to the old `deleteMessage` behavior, so no migration
is needed.
No vendor file is touched: `deleteSwipe` is already exported from
`src/script.js`, the bridge change lives in
`src/tauri/main/api/agent-chat-commit-bridge.js`, and the rollback
helper is project-owned.
Tests: two new cases on the rollback helper — one drives the
deleteSwipe path against a 3-swipe message and asserts that the
prior swipes (`'user-authored'`, `'user-authored alt 1'`) survive
while the agent's appended swipe is popped; the other asserts the
fallback to `deleteMessage` when the swipe array is a singleton.
Also rebased onto current next/2.0.0 so the duplicate
cherry-picked backend commit drops cleanly and the PR diff stays
frontend-only as promised in the PR description.
Refs PR review feedback on Darkatse/TauriTavern#63.
Co-authored-by: Cursor <cursoragent@cursor.com>
The backend started emitting run_rollback_targets and a userRetryable
flag on run_failed when an Agent run drifts off-tool (#55), but the
host shell was ignoring both. As a result drift would leave orphan chat
messages and the user had no in-UI Retry affordance. This wires three
host-side loops without touching vendor:
1. Retry entry. `presentAgentRunFailure` now also returns
`userRetryable`, and the run-failure detail section adds an explicit
Retry button (calling `$('#option_regenerate').trigger('click')`, the
existing regenerate path) plus a User-retryable field. The timeline
panel auto-expands and opens the failure details when a user-
retryable failure arrives so the button is immediately visible.
2. Drift rollback. `agent-run-controller` now subscribes to
`run_rollback_targets` and delegates to the new
`agent-run-message-rollback` helper, which uses vendor-exported
`deleteMessage` against the host's own messageId (the chat array
index returned via `resolve_agent_chat_commit`). Matches are gated
on `extra.tauritavern.agent.runId === runId` so a re-saved message
from another run is never wiped, missing messages are silently
skipped, and ids are deleted back-to-front so splice indices stay
valid.
3. Unlocking send. The controller now defers resolving / rejecting the
run promise until the rollback Promise settles. Vendor's existing
`finally { unblockGeneration(type); }` in Generate() therefore fires
after the chat is clean, so the send button (and other
`is_send_press` gates) are released against the post-rollback state
rather than mid-rollback. No vendor file is touched.
Friendly drift-error copy is also added for `agent.tool_after_finish`
and `agent.max_tool_rounds_exceeded`, and the new i18n keys
(`timelineActionRetry`, `timelineDetailFieldUserRetryable`,
`timelineActionRetryHint`) are mirrored in en, zh-cn and zh-tw to keep
the i18n contract test green.
Test plan: pnpm run check:frontend, pnpm run check:types,
pnpm run test:contracts (246 tests, all green; 4 new tests cover
presenter, formatter, controller-await-rollback, and the rollback
helper itself).
Co-authored-by: Cursor <cursoragent@cursor.com>
Chat completion JSON-parse / body-read failures used to surface as
`DomainError::InternalError`, which `error_payload::is_retryable` treats as
non-retryable `agent.internal_error`. In practice these failures are caused by
upstream-side hiccups (CDN HTML challenges, proxy timeouts mid-body, plain-text
200 errors, SSE on a non-stream endpoint) and tearing down the run is the wrong
default — the user just sees an opaque permanent failure.
Reclassify the helper to return `DomainError::Transient` carrying the
`model.upstream_invalid_response` structured prefix so:
* `From<DomainError> for ApplicationError` maps it to `ApplicationError::Transient`,
* `error_payload::structured_code_and_message` extracts the
`model.upstream_invalid_response` code so the frontend can present a coherent
error,
* `error_payload::is_retryable` reports `retryable=true`.
The OpenAI Responses WebSocket message parser uses the same prefix and helper
so WS-frame JSON failures behave identically to HTTP-body JSON failures.
No new `DomainError` variant: this reuses the existing `Transient` variant, so
the three exhaustive `From<DomainError>` / `map_domain_error` match arms are
untouched.
Adds a regression test in `error_payload::tests` asserting that an
`ApplicationError::Transient` whose message starts with
`model.upstream_invalid_response:` produces `code=model.upstream_invalid_response`
and `retryable=true`.
Co-authored-by: Cursor <cursoragent@cursor.com>
When an upstream chat-completion provider returns 2xx with a body the client
cannot decode as JSON (HTML challenge pages, plain-text proxy errors, empty
bodies, SSE on a non-stream endpoint, etc.), the failure used to silently bubble
up as `Failed to parse generation JSON` with no log of what was actually
received, leaving operators without any signal to diagnose CDN/proxy issues.
Introduce a shared body-preview helper in
`infrastructure/apis/http_chat_completion_repository/body_preview.rs`:
* `log_upstream_body_parse_failure(provider, operation, status, content_type,
body, error)` emits a structured `tracing::error!` event with a UTF-8 lossy
preview of the first 512 bytes plus the full body length, so logs stay bounded
even when upstream pours out megabytes of HTML.
* `read_upstream_json_body(provider, operation, response)` is the async wrapper
every HTTP provider call site uses: it reads the body once, attempts JSON
decode, calls the logger on failure, and surfaces a more diagnostic error
string (`Failed to parse {operation} JSON from {provider} (status {n})`).
All twelve `response.json::<Value>().await.map_err(...)` call sites across the
seven provider modules (`openai`, `claude`, `cohere`, `makersuite`,
`gemini_interactions`, `vertexai`, `openai_responses`) now route through the
helper. The OpenAI Responses WebSocket message parser
(`parse_ws_event`) also calls the logger before returning so non-JSON frames
emit the same diagnostic.
Error classification is intentionally unchanged in this commit (still
`DomainError::InternalError`); the follow-up will reclassify it as transient.
Co-authored-by: Cursor <cursoragent@cursor.com>
When the model deviated from the tool contract after committing chat
artifacts (e.g. answered with plain text instead of calling
`workspace_finish`), the run failed with `model.tool_call_required` /
`agent.tool_after_finish` / `agent.max_tool_rounds_exceeded`, but the
already-committed messages were left orphaned in chat and the UI had no
signal to offer a Retry path.
Loop runner now tracks each `AgentToolEffect::ChatCommitted` during a
run. When the run later enters a drift state, it emits a
`run_rollback_targets` warn event BEFORE `run_failed`, listing the
`{ path, mode, messageId, round }` of every commit the host UI needs to
roll back.
`run_failed` payload gains a `userRetryable: bool` flag:
- `retryable=true` (transient / rate-limited) implies `userRetryable=true`.
- Otherwise drift codes (`model.tool_call_required`,
`agent.tool_after_finish`, `agent.max_tool_rounds_exceeded`) are
`userRetryable=true` while remaining auto-non-retryable, so the UI can
surface a manual Retry button after performing the rollback. Policy /
validation errors stay `userRetryable=false`.
`workspace_commit` description tightened to spell out the
"commit -> finish in the same or next turn, never plain text" contract.
Docs:
- `docs/Agent/RunEventJournal.md`: new `run_rollback_targets` event,
expanded `run_failed` payload schema with `userRetryable` semantics.
- `docs/Agent/ToolSystem.md`: documents the
`workspace.path_is_directory` business error and the commit/finish
drift contract so the tool-system docs stay in sync.
Tests: integration test
`foreground_run_emits_rollback_targets_on_tool_call_required_drift`
drives a mock gateway to commit then drift, asserting that the
rollback event lists the committed message and that the run fails with
`userRetryable=true`. Extended `error_payload` unit tests cover all
three drift codes.
Co-authored-by: Cursor <cursoragent@cursor.com>
`workspace_read_file` and the other workspace IO tools used to crash the
run with a non-retryable `agent.internal_error` when the model targeted
a directory (e.g. `persist`, `output`) instead of a regular file: the
underlying `fs::read_to_string` raised `EISDIR`, which the repository
surfaced as `DomainError::InternalError`.
We now model "the target is a directory" as a domain-level conflict and
let the tool layer translate it into a model-facing recoverable error:
- `DomainError::Conflict(String)` added at the domain layer; mapped to
`ApplicationError::ValidationError`, `CommandError::BadRequest`, and
HTTP 409 in the LAN sync server (all 3 exhaustive matches updated).
- `file_agent_repository::workspace_store::ensure_target_is_not_directory`
probes `fs::symlink_metadata` in `read_text` / `write_text` and raises
`DomainError::Conflict("workspace.path_is_directory: ...")` before
hitting `EISDIR`.
- A shared `parse_workspace_conflict_message` lives in
`agent_tools/common.rs` (re-exported `pub(crate)`) so both the
workspace tool layer (`classify_workspace_io_error` in `args.rs`) and
the runtime-service commit hook (`perform_host_chat_commit`) decode
the conflict into a `workspace.path_is_directory` recoverable tool
result through a single source of truth.
- `workspace_read_file` description tightened to explicitly forbid
directory targets and point at `workspace_list_files` instead.
Tests:
- Repository-level: `read_text` / `write_text` on a directory return
`DomainError::Conflict` with the documented prefix.
- Tool layer: `classify_workspace_io_error` maps Conflict ->
`workspace.path_is_directory`, NotFound -> `workspace.file_not_found`,
and bubbles unknown errors for the dispatcher to fail-fast on.
Co-authored-by: Cursor <cursoragent@cursor.com>
`.cursor/` holds personal IDE / Cursor agent skill files (e.g. the
TauriTavern contribution conventions skill) that are local to each
developer and should not flow through code review.
Co-authored-by: Cursor <cursoragent@cursor.com>
