mirror of
https://github.com/Darkatse/TauriTavern.git
synced 2026-10-19 15:45:12 +00:00
fix(agent): type workspace directory errors
This commit is contained in:
parent
567be35106
commit
07715203c3
15 changed files with 103 additions and 95 deletions
|
|
@ -39,7 +39,9 @@ impl From<DomainError> for ApplicationError {
|
|||
DomainError::InternalError(msg) => ApplicationError::InternalError(msg),
|
||||
DomainError::RateLimited { message } => ApplicationError::RateLimited(message),
|
||||
DomainError::Transient(msg) => ApplicationError::Transient(msg),
|
||||
DomainError::Conflict(msg) => ApplicationError::ValidationError(msg),
|
||||
DomainError::WorkspacePathIsDirectory { path } => {
|
||||
ApplicationError::ValidationError(format!("Workspace path is a directory: {path}"))
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,14 +2,15 @@ use serde_json::{Value, json};
|
|||
use tokio::sync::oneshot;
|
||||
use uuid::Uuid;
|
||||
|
||||
use super::commit_ledger::RunCommitLedger;
|
||||
use super::{
|
||||
AgentCancelReceiver, AgentRuntimeService, HostChatCommitResult, PendingHostChatCommit,
|
||||
};
|
||||
use super::commit_ledger::RunCommitLedger;
|
||||
use crate::application::dto::agent_dto::AgentResolveChatCommitDto;
|
||||
use crate::application::errors::ApplicationError;
|
||||
use crate::application::services::agent_tools::{
|
||||
AgentToolDispatchOutcome, AgentToolEffect, parse_workspace_conflict_message,
|
||||
AgentToolDispatchOutcome, AgentToolEffect, WORKSPACE_PATH_IS_DIRECTORY_CODE,
|
||||
workspace_path_is_directory_message,
|
||||
};
|
||||
use crate::domain::errors::DomainError;
|
||||
use crate::domain::models::agent::{
|
||||
|
|
@ -82,11 +83,13 @@ impl AgentRuntimeService {
|
|||
elapsed_ms,
|
||||
));
|
||||
}
|
||||
Err(DomainError::Conflict(message)) => {
|
||||
if let Some((code, detail)) = parse_workspace_conflict_message(&message) {
|
||||
return Ok(recoverable_tool_error(call, code, detail, elapsed_ms));
|
||||
}
|
||||
return Err(DomainError::Conflict(message).into());
|
||||
Err(DomainError::WorkspacePathIsDirectory { path }) => {
|
||||
return Ok(recoverable_tool_error(
|
||||
call,
|
||||
WORKSPACE_PATH_IS_DIRECTORY_CODE,
|
||||
&workspace_path_is_directory_message(&path),
|
||||
elapsed_ms,
|
||||
));
|
||||
}
|
||||
Err(error) => return Err(error.into()),
|
||||
};
|
||||
|
|
|
|||
|
|
@ -163,12 +163,12 @@ impl AgentRuntimeService {
|
|||
round,
|
||||
&call,
|
||||
&mut tool_session,
|
||||
profile,
|
||||
commit_count,
|
||||
commit_ledger,
|
||||
cancel,
|
||||
)
|
||||
.await?;
|
||||
profile,
|
||||
commit_count,
|
||||
commit_ledger,
|
||||
cancel,
|
||||
)
|
||||
.await?;
|
||||
match &outcome.effect {
|
||||
AgentToolEffect::WorkspaceFileWritten { file } => {
|
||||
self.checkpoint_workspace_file(
|
||||
|
|
|
|||
|
|
@ -3,8 +3,8 @@ use std::time::Instant;
|
|||
use serde_json::json;
|
||||
use sha2::{Digest, Sha256};
|
||||
|
||||
use super::commit_ledger::RunCommitLedger;
|
||||
use super::AgentRuntimeService;
|
||||
use super::commit_ledger::RunCommitLedger;
|
||||
use crate::application::errors::ApplicationError;
|
||||
use crate::application::services::agent_tools::{
|
||||
AgentToolDispatchOutcome, AgentToolEffect, AgentToolSession,
|
||||
|
|
|
|||
|
|
@ -11,4 +11,4 @@ pub use dispatcher::{AgentToolDispatchOutcome, AgentToolDispatcher, AgentToolEff
|
|||
pub use registry::BuiltinAgentToolRegistry;
|
||||
pub use session::AgentToolSession;
|
||||
|
||||
pub(crate) use common::parse_workspace_conflict_message;
|
||||
pub(crate) use common::{WORKSPACE_PATH_IS_DIRECTORY_CODE, workspace_path_is_directory_message};
|
||||
|
|
|
|||
|
|
@ -51,22 +51,15 @@ pub(super) fn optional_bool_arg(
|
|||
.ok_or_else(|| format!("{key} must be a boolean"))
|
||||
}
|
||||
|
||||
/// Decode a known `DomainError::Conflict` payload raised by the workspace
|
||||
/// repository into a `(code, detail)` pair suitable for a model-facing
|
||||
/// `AgentToolResult`. Unknown conflicts are not model-recoverable by default:
|
||||
/// they must bubble up so new repository states do not get silently flattened
|
||||
/// into generic tool errors.
|
||||
///
|
||||
/// Visibility is `pub(crate)` so that both the workspace tool layer
|
||||
/// (`workspace/args.rs`) and the runtime-service commit hook
|
||||
/// (`agent_runtime_service/commit.rs`) share a single parsing source of
|
||||
/// truth.
|
||||
pub(crate) fn parse_workspace_conflict_message(message: &str) -> Option<(&'static str, &str)> {
|
||||
let trimmed = message.trim();
|
||||
if let Some(rest) = trimmed.strip_prefix("workspace.path_is_directory:") {
|
||||
return Some(("workspace.path_is_directory", rest.trim_start()));
|
||||
}
|
||||
None
|
||||
pub(crate) const WORKSPACE_PATH_IS_DIRECTORY_CODE: &str = "workspace.path_is_directory";
|
||||
|
||||
/// Shared model-facing wording for the typed workspace-directory domain
|
||||
/// error. The repository only reports the fact; this layer knows which Agent
|
||||
/// tool can help the model recover.
|
||||
pub(crate) fn workspace_path_is_directory_message(path: &str) -> String {
|
||||
format!(
|
||||
"workspace path `{path}` is a directory; call workspace_list_files to list its contents and re-target a specific file."
|
||||
)
|
||||
}
|
||||
|
||||
pub(super) fn tool_error(call: &AgentToolCall, error_code: &str, message: &str) -> AgentToolResult {
|
||||
|
|
|
|||
|
|
@ -158,7 +158,10 @@ pub(in crate::application::services::agent_tools) async fn apply_patch(
|
|||
file.text.replacen(old_string, new_string, 1)
|
||||
};
|
||||
let old_sha256 = file.sha256.clone();
|
||||
let file = match workspace_repository.write_text(run_id, &path, &updated).await {
|
||||
let file = match workspace_repository
|
||||
.write_text(run_id, &path, &updated)
|
||||
.await
|
||||
{
|
||||
Ok(file) => file,
|
||||
Err(error) => match classify_workspace_io_error(call, error) {
|
||||
Ok(result) => return Ok((result, AgentToolEffect::None)),
|
||||
|
|
|
|||
|
|
@ -5,8 +5,9 @@ use crate::domain::errors::DomainError;
|
|||
use crate::domain::models::agent::{AgentToolCall, AgentToolResult, WorkspacePath};
|
||||
|
||||
pub(super) use crate::application::services::agent_tools::common::{
|
||||
object_args, optional_bool_arg, optional_usize_arg, parse_workspace_conflict_message,
|
||||
WORKSPACE_PATH_IS_DIRECTORY_CODE, object_args, optional_bool_arg, optional_usize_arg,
|
||||
required_raw_string_arg, required_trimmed_string_arg, tool_error,
|
||||
workspace_path_is_directory_message,
|
||||
};
|
||||
|
||||
/// Pattern-match helper used by every read/write entry point so they all
|
||||
|
|
@ -18,13 +19,14 @@ pub(super) fn classify_workspace_io_error(
|
|||
error: DomainError,
|
||||
) -> Result<AgentToolResult, DomainError> {
|
||||
match error {
|
||||
DomainError::NotFound(message) => Ok(tool_error(call, "workspace.file_not_found", &message)),
|
||||
DomainError::Conflict(message) => {
|
||||
if let Some((code, detail)) = parse_workspace_conflict_message(&message) {
|
||||
return Ok(tool_error(call, code, detail));
|
||||
}
|
||||
Err(DomainError::Conflict(message))
|
||||
DomainError::NotFound(message) => {
|
||||
Ok(tool_error(call, "workspace.file_not_found", &message))
|
||||
}
|
||||
DomainError::WorkspacePathIsDirectory { path } => Ok(tool_error(
|
||||
call,
|
||||
WORKSPACE_PATH_IS_DIRECTORY_CODE,
|
||||
&workspace_path_is_directory_message(&path),
|
||||
)),
|
||||
other => Err(other),
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -67,19 +67,17 @@ fn make_test_tool_call(name: &str) -> AgentToolCall {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn classify_conflict_error_maps_to_path_is_directory() {
|
||||
fn classify_workspace_path_is_directory_error_maps_to_tool_error() {
|
||||
// Issue #54: a directory hit on workspace_read_file used to surface as
|
||||
// `agent.internal_error`. The tool layer now classifies the
|
||||
// repository's structured Conflict into the recoverable
|
||||
// repository's typed domain error into the recoverable
|
||||
// `workspace.path_is_directory` business error so the model can
|
||||
// self-correct by calling workspace_list_files.
|
||||
let call = make_test_tool_call("workspace.read_file");
|
||||
let error = DomainError::Conflict(
|
||||
"workspace.path_is_directory: workspace path `persist` is a directory".to_string(),
|
||||
);
|
||||
let error = DomainError::workspace_path_is_directory("persist");
|
||||
|
||||
let result = classify_workspace_io_error(&call, error)
|
||||
.expect("conflict must classify into a tool result, not a hard error");
|
||||
.expect("directory error must classify into a tool result, not a hard error");
|
||||
|
||||
assert!(result.is_error);
|
||||
assert_eq!(
|
||||
|
|
@ -102,7 +100,10 @@ fn classify_not_found_error_maps_to_file_not_found() {
|
|||
.expect("not found must classify into a tool result");
|
||||
|
||||
assert!(result.is_error);
|
||||
assert_eq!(result.error_code.as_deref(), Some("workspace.file_not_found"));
|
||||
assert_eq!(
|
||||
result.error_code.as_deref(),
|
||||
Some("workspace.file_not_found")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
@ -116,15 +117,3 @@ fn classify_unknown_error_bubbles_up_for_host_failure() {
|
|||
"infrastructural errors must remain host-level failures",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn classify_unknown_conflict_bubbles_up_for_host_failure() {
|
||||
let call = make_test_tool_call("workspace.read_file");
|
||||
let error = DomainError::Conflict("workspace.journal_conflict: checkpoint diverged".to_string());
|
||||
|
||||
let result = classify_workspace_io_error(&call, error);
|
||||
assert!(
|
||||
result.is_err(),
|
||||
"only explicitly known workspace conflicts are model-recoverable",
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -49,7 +49,10 @@ pub(in crate::application::services::agent_tools) async fn write_file(
|
|||
if let Err(result) = ensure_writable_workspace_path(call, &policy, &path) {
|
||||
return Ok((result, AgentToolEffect::None));
|
||||
}
|
||||
let file = match workspace_repository.write_text(run_id, &path, content).await {
|
||||
let file = match workspace_repository
|
||||
.write_text(run_id, &path, content)
|
||||
.await
|
||||
{
|
||||
Ok(file) => file,
|
||||
Err(error) => match classify_workspace_io_error(call, error) {
|
||||
Ok(result) => return Ok((result, AgentToolEffect::None)),
|
||||
|
|
|
|||
|
|
@ -25,8 +25,8 @@ pub enum DomainError {
|
|||
#[error("{0}")]
|
||||
Transient(String),
|
||||
|
||||
#[error("{0}")]
|
||||
Conflict(String),
|
||||
#[error("Workspace path is a directory: {path}")]
|
||||
WorkspacePathIsDirectory { path: String },
|
||||
}
|
||||
|
||||
impl DomainError {
|
||||
|
|
@ -48,9 +48,8 @@ impl DomainError {
|
|||
Self::Transient(message.into())
|
||||
}
|
||||
|
||||
#[allow(dead_code)]
|
||||
pub fn conflict(message: impl Into<String>) -> Self {
|
||||
Self::Conflict(message.into())
|
||||
pub fn workspace_path_is_directory(path: impl Into<String>) -> Self {
|
||||
Self::WorkspacePathIsDirectory { path: path.into() }
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -77,4 +76,14 @@ mod tests {
|
|||
DomainError::Cancelled(message) if message == "Job cancelled"
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn workspace_path_is_directory_constructor_keeps_path() {
|
||||
let error = DomainError::workspace_path_is_directory("persist");
|
||||
|
||||
assert!(matches!(
|
||||
&error,
|
||||
DomainError::WorkspacePathIsDirectory { path } if path == "persist"
|
||||
));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -417,7 +417,10 @@ fn map_domain_error(error: DomainError) -> (StatusCode, String) {
|
|||
DomainError::InternalError(message) => (StatusCode::INTERNAL_SERVER_ERROR, message),
|
||||
DomainError::RateLimited { message } => (StatusCode::TOO_MANY_REQUESTS, message),
|
||||
DomainError::Transient(message) => (StatusCode::SERVICE_UNAVAILABLE, message),
|
||||
DomainError::Conflict(message) => (StatusCode::CONFLICT, message),
|
||||
DomainError::WorkspacePathIsDirectory { path } => (
|
||||
StatusCode::CONFLICT,
|
||||
format!("Workspace path is a directory: {path}"),
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -227,12 +227,12 @@ async fn repository_round_trips_run_workspace_event_and_checkpoint() {
|
|||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_text_on_directory_returns_conflict_error() {
|
||||
async fn read_text_on_directory_returns_typed_workspace_error() {
|
||||
// Issue #54: workspace_read_file used to bubble up the raw EISDIR
|
||||
// ("Is a directory") OS error as `agent.internal_error` (retryable=false)
|
||||
// and tear down the whole run. We now translate it into a structured
|
||||
// `DomainError::Conflict` so the tool layer can surface it as a
|
||||
// recoverable `workspace.path_is_directory` business error.
|
||||
// domain error so the tool layer can surface it as a recoverable
|
||||
// `workspace.path_is_directory` business error.
|
||||
let root = temp_root();
|
||||
let repository = FileAgentRepository::new(root.clone());
|
||||
let run = sample_run_with_id("run_dir_read");
|
||||
|
|
@ -241,7 +241,12 @@ async fn read_text_on_directory_returns_conflict_error() {
|
|||
|
||||
repository.create_run(&run).await.expect("create run");
|
||||
repository
|
||||
.initialize_run(&run, &manifest, &serde_json::json!({"messages": []}), &profile)
|
||||
.initialize_run(
|
||||
&run,
|
||||
&manifest,
|
||||
&serde_json::json!({"messages": []}),
|
||||
&profile,
|
||||
)
|
||||
.await
|
||||
.expect("initialize workspace");
|
||||
|
||||
|
|
@ -252,24 +257,17 @@ async fn read_text_on_directory_returns_conflict_error() {
|
|||
.expect_err("reading a directory must fail");
|
||||
|
||||
match error {
|
||||
DomainError::Conflict(message) => {
|
||||
assert!(
|
||||
message.starts_with("workspace.path_is_directory:"),
|
||||
"expected workspace.path_is_directory conflict, got `{message}`"
|
||||
);
|
||||
assert!(
|
||||
message.contains("persist"),
|
||||
"conflict message should mention offending path, got `{message}`"
|
||||
);
|
||||
DomainError::WorkspacePathIsDirectory { path } => {
|
||||
assert_eq!(path, "persist");
|
||||
}
|
||||
other => panic!("expected DomainError::Conflict, got {other:?}"),
|
||||
other => panic!("expected DomainError::WorkspacePathIsDirectory, got {other:?}"),
|
||||
}
|
||||
|
||||
fs::remove_dir_all(root).await.expect("cleanup");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_text_on_directory_returns_conflict_error() {
|
||||
async fn write_text_on_directory_returns_typed_workspace_error() {
|
||||
// Same guard for write_text so workspace_write_file cannot wipe out a
|
||||
// directory through the temp-file swap path.
|
||||
let root = temp_root();
|
||||
|
|
@ -280,7 +278,12 @@ async fn write_text_on_directory_returns_conflict_error() {
|
|||
|
||||
repository.create_run(&run).await.expect("create run");
|
||||
repository
|
||||
.initialize_run(&run, &manifest, &serde_json::json!({"messages": []}), &profile)
|
||||
.initialize_run(
|
||||
&run,
|
||||
&manifest,
|
||||
&serde_json::json!({"messages": []}),
|
||||
&profile,
|
||||
)
|
||||
.await
|
||||
.expect("initialize workspace");
|
||||
|
||||
|
|
@ -291,11 +294,10 @@ async fn write_text_on_directory_returns_conflict_error() {
|
|||
.expect_err("writing to a directory must fail");
|
||||
|
||||
match error {
|
||||
DomainError::Conflict(message) => {
|
||||
assert!(message.starts_with("workspace.path_is_directory:"));
|
||||
assert!(message.contains("output"));
|
||||
DomainError::WorkspacePathIsDirectory { path } => {
|
||||
assert_eq!(path, "output");
|
||||
}
|
||||
other => panic!("expected DomainError::Conflict, got {other:?}"),
|
||||
other => panic!("expected DomainError::WorkspacePathIsDirectory, got {other:?}"),
|
||||
}
|
||||
|
||||
fs::remove_dir_all(root).await.expect("cleanup");
|
||||
|
|
|
|||
|
|
@ -306,19 +306,16 @@ impl WorkspaceRepository for FileAgentRepository {
|
|||
/// Reject calls that target an existing directory at `target`. The OS error
|
||||
/// for `read_to_string` on a directory (EISDIR / "Is a directory") used to
|
||||
/// bubble up as `DomainError::InternalError`, surfaced to the model as a
|
||||
/// non-retryable `agent.internal_error`. We translate it into a structured
|
||||
/// `DomainError::Conflict` so the tool layer can return a recoverable
|
||||
/// `workspace.path_is_directory` business error and prompt the model to use
|
||||
/// `workspace_list_files` instead.
|
||||
/// non-retryable `agent.internal_error`. The repository now returns a typed
|
||||
/// domain error and leaves model-facing recovery advice to the tool layer.
|
||||
async fn ensure_target_is_not_directory(
|
||||
target: &std::path::Path,
|
||||
workspace_path: &WorkspacePath,
|
||||
) -> Result<(), DomainError> {
|
||||
match fs::symlink_metadata(target).await {
|
||||
Ok(metadata) if metadata.file_type().is_dir() => Err(DomainError::Conflict(format!(
|
||||
"workspace.path_is_directory: workspace path `{}` is a directory; use workspace_list_files to list its contents and re-target a specific file.",
|
||||
workspace_path.as_str()
|
||||
))),
|
||||
Ok(metadata) if metadata.file_type().is_dir() => Err(
|
||||
DomainError::workspace_path_is_directory(workspace_path.as_str()),
|
||||
),
|
||||
Ok(_) => Ok(()),
|
||||
Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()),
|
||||
Err(error) => Err(DomainError::InternalError(format!(
|
||||
|
|
|
|||
|
|
@ -49,7 +49,9 @@ impl From<DomainError> for CommandError {
|
|||
DomainError::InternalError(msg) => CommandError::InternalServerError(msg),
|
||||
DomainError::RateLimited { message } => CommandError::TooManyRequests(message),
|
||||
DomainError::Transient(msg) => CommandError::InternalServerError(msg),
|
||||
DomainError::Conflict(msg) => CommandError::BadRequest(msg),
|
||||
DomainError::WorkspacePathIsDirectory { path } => {
|
||||
CommandError::BadRequest(format!("Workspace path is a directory: {path}"))
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue