Harden provider diagnostics and tool failure handling
This commit is contained in:
Ryan Ward
@@ -469,6 +469,13 @@ impl BackgroundJobsModel {
|
||||
id: BackgroundJobId,
|
||||
ctx: &mut ModelContext<Self>,
|
||||
) -> Result<(), String> {
|
||||
let job = self.jobs.get(&id).ok_or("Background job not found")?;
|
||||
// Completion can win the race with an already-authorized cancellation.
|
||||
// Its recorded outcome is final; acknowledge the request without
|
||||
// signalling a process or publishing another completion.
|
||||
if job.status.is_terminal() {
|
||||
return Ok(());
|
||||
}
|
||||
if self.unreconciled_remote_jobs.contains(&id)
|
||||
&& !self.remote_cancellations.contains_key(&id)
|
||||
{
|
||||
|
||||
@@ -144,3 +144,88 @@ fn corrupt_history_is_reported_instead_of_replaced() {
|
||||
b"broken"
|
||||
);
|
||||
}
|
||||
|
||||
fn cancellation_fixture(status: BackgroundJobStatus) -> BackgroundJob {
|
||||
BackgroundJob {
|
||||
id: uuid::Uuid::new_v4(),
|
||||
conversation_id: crate::ai::agent::conversation::AIConversationId::new(),
|
||||
action_id: "cancellation-test-action".to_owned().into(),
|
||||
command: "already completed command".to_owned(),
|
||||
working_directory: std::path::PathBuf::new(),
|
||||
project_root: None,
|
||||
host_id: None,
|
||||
started_at: Utc::now(),
|
||||
finished_at: Some(Utc::now()),
|
||||
status,
|
||||
exit_code: Some(17),
|
||||
error: Some("original outcome".to_owned()),
|
||||
completion_delivered: true,
|
||||
log_path: std::path::PathBuf::new(),
|
||||
output: "original output".to_owned(),
|
||||
output_truncated: false,
|
||||
log_bytes: 15,
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn cancelling_a_completed_job_preserves_its_outcome_without_signalling_or_redelivery() {
|
||||
galaxyui::App::test((), |mut app| async move {
|
||||
let jobs = app.add_model(super::BackgroundJobsModel::new);
|
||||
for status in [
|
||||
BackgroundJobStatus::Succeeded,
|
||||
BackgroundJobStatus::Failed,
|
||||
BackgroundJobStatus::Cancelled,
|
||||
] {
|
||||
jobs.update(&mut app, |jobs, ctx| {
|
||||
let job = cancellation_fixture(status);
|
||||
let id = job.id;
|
||||
let owner = job.conversation_id;
|
||||
let finished_at = job.finished_at;
|
||||
let (cancel, cancellations) = async_channel::bounded(1);
|
||||
jobs.jobs.insert(id, job);
|
||||
// Even a stale remote cancellation handle must not be used.
|
||||
jobs.remote_cancellations.insert(id, cancel);
|
||||
assert_eq!(jobs.cancel(id, ctx), Ok(()));
|
||||
assert_eq!(jobs.cancel(id, ctx), Ok(()));
|
||||
assert_eq!(
|
||||
cancellations.try_recv(),
|
||||
Err(async_channel::TryRecvError::Empty)
|
||||
);
|
||||
let job = jobs.job(id).unwrap();
|
||||
assert_eq!(job.status, status);
|
||||
assert_eq!(job.exit_code, Some(17));
|
||||
assert_eq!(job.finished_at, finished_at);
|
||||
assert_eq!(job.error.as_deref(), Some("original outcome"));
|
||||
assert_eq!(job.output, "original output");
|
||||
assert_eq!(job.completion_delivered, true);
|
||||
assert_eq!(jobs.pending_completions(owner).len(), 0);
|
||||
assert_eq!(jobs.unpublished_completions.contains(&id), false);
|
||||
});
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn cancellation_still_rejects_unknown_and_unowned_running_processes() {
|
||||
galaxyui::App::test((), |mut app| async move {
|
||||
let jobs = app.add_model(super::BackgroundJobsModel::new);
|
||||
jobs.update(&mut app, |jobs, ctx| {
|
||||
let unknown = uuid::Uuid::new_v4();
|
||||
let (cancel, cancellations) = async_channel::bounded(1);
|
||||
jobs.remote_cancellations.insert(unknown, cancel);
|
||||
assert_eq!(jobs.cancel(unknown, ctx), Err("Background job not found".to_owned()));
|
||||
assert_eq!(cancellations.try_recv(), Err(async_channel::TryRecvError::Empty));
|
||||
|
||||
let mut job = cancellation_fixture(BackgroundJobStatus::Running);
|
||||
job.finished_at = None;
|
||||
let id = job.id;
|
||||
jobs.jobs.insert(id, job);
|
||||
// A known Running record alone never authorizes signalling a PID.
|
||||
assert_eq!(jobs.cancel(id, ctx), Err("This background job is no longer running".to_owned()));
|
||||
|
||||
jobs.unreconciled_remote_jobs.insert(id);
|
||||
assert_eq!(jobs.cancel(id, ctx), Err("Waiting to reconnect and confirm this job's state on its host before cancellation is available".to_owned()));
|
||||
assert_eq!(jobs.job(id).unwrap().status, BackgroundJobStatus::Running);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
@@ -93,7 +93,7 @@ impl GrepError {
|
||||
error: GrepErrorType::NonZeroExitCode,
|
||||
output: Some(output),
|
||||
..
|
||||
} => format!("{NON_ZERO_EXIT_CODE_ERROR}, output:\n{output}"),
|
||||
} => actionable_grep_error(format!("{NON_ZERO_EXIT_CODE_ERROR}, output:\n{output}")),
|
||||
GrepError {
|
||||
error: GrepErrorType::NonZeroExitCode,
|
||||
output: None,
|
||||
@@ -102,11 +102,43 @@ impl GrepError {
|
||||
GrepError {
|
||||
error: GrepErrorType::Other(error),
|
||||
..
|
||||
} => error.clone(),
|
||||
} => actionable_grep_error(error.clone()),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Preserve backend regex semantics and report recovery guidance only after
|
||||
/// that backend has diagnosed invalid syntax. Never retry with different
|
||||
/// semantics or reject valid backend-specific expressions in a local parser.
|
||||
fn actionable_grep_error(message: String) -> String {
|
||||
let lower = message.to_ascii_lowercase();
|
||||
let invalid_regex = [
|
||||
"invalid regular expression",
|
||||
"invalid pattern",
|
||||
"regex parse error",
|
||||
"unmatched [",
|
||||
"unmatched (",
|
||||
"unmatched )",
|
||||
"unbalanced parenth",
|
||||
"parentheses not balanced",
|
||||
"brackets ([ ]) not balanced",
|
||||
"unclosed group",
|
||||
"unclosed character class",
|
||||
"unterminated character class",
|
||||
"unterminated [",
|
||||
"missing closing parenthesis",
|
||||
"not enough )",
|
||||
"not enough ]",
|
||||
]
|
||||
.iter()
|
||||
.any(|marker| lower.contains(marker));
|
||||
if invalid_regex {
|
||||
format!("{message}\nGrep uses regex by default. For literal source text, retry the same queries with fixed_strings: true. For an intended regex, correct or escape the invalid syntax; do not repeat the same malformed pattern.")
|
||||
} else {
|
||||
message
|
||||
}
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
fn create_redacted_grep_error_event(
|
||||
should_collect_ugc: bool,
|
||||
|
||||
@@ -125,3 +125,45 @@ fn parse_grep_output_reports_when_results_reach_the_per_file_limit() {
|
||||
);
|
||||
assert!(truncated);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn backend_regex_syntax_errors_explain_literal_and_regex_recovery() {
|
||||
for output in [
|
||||
"fatal: -e option, 'variantResolutions: []': brackets ([ ]) not balanced",
|
||||
"fatal: -e option, 'indexAsset(context': parentheses not balanced",
|
||||
"fatal: command line, 'variantResolutions: []': Unmatched [, [^, [:, [., or [=",
|
||||
r"fatal: command line, 'indexAsset(context': Unmatched ( or \(",
|
||||
"regex parse error: unclosed group",
|
||||
"Select-String: Invalid pattern 'indexAsset(context': Not enough )'s.",
|
||||
] {
|
||||
let error = GrepError::new_for_non_zero_exit_code().with_output(output.to_owned());
|
||||
let message = error.error_for_conversation();
|
||||
assert!(message.contains(output));
|
||||
assert!(message.contains("fixed_strings: true"));
|
||||
assert!(message.contains("correct or escape"));
|
||||
assert_eq!(error.error_message(), NON_ZERO_EXIT_CODE_ERROR);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ripgrep_regex_errors_also_include_literal_mode_guidance() {
|
||||
let error =
|
||||
GrepError::new("Ripgrep search failed: regex parse error: unclosed group".to_owned());
|
||||
assert!(error
|
||||
.error_for_conversation()
|
||||
.contains("fixed_strings: true"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unrelated_grep_failures_keep_the_original_diagnostic() {
|
||||
let output = "fatal: cannot access '/project': Permission denied";
|
||||
let error = GrepError::new_for_non_zero_exit_code().with_output(output.to_owned());
|
||||
assert_eq!(
|
||||
error.error_for_conversation(),
|
||||
format!("{NON_ZERO_EXIT_CODE_ERROR}, output:\n{output}")
|
||||
);
|
||||
assert_eq!(
|
||||
GrepError::new("Grep operation timed out".to_owned()).error_for_conversation(),
|
||||
"Grep operation timed out"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -173,57 +173,7 @@ impl ReadFilesExecutor {
|
||||
.await
|
||||
.map_err(|e| anyhow::anyhow!("Remote read failed: {e}"))?;
|
||||
|
||||
if !response.failed_files.is_empty() && response.file_contexts.is_empty() {
|
||||
let failed = response
|
||||
.failed_files
|
||||
.iter()
|
||||
.map(|f| {
|
||||
let reason = f
|
||||
.error
|
||||
.as_ref()
|
||||
.map(|e| e.message.as_str())
|
||||
.unwrap_or("unknown error");
|
||||
format!("{}: {reason}", f.path)
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ");
|
||||
return Ok(ReadFilesResult::Error(format!(
|
||||
"Failed to read files: {failed}"
|
||||
)));
|
||||
}
|
||||
|
||||
let file_contexts = response
|
||||
.file_contexts
|
||||
.into_iter()
|
||||
.filter_map(|fc| {
|
||||
let content = match fc.content? {
|
||||
remote_server::proto::file_context_proto::Content::TextContent(
|
||||
text,
|
||||
) => crate::ai::agent::AnyFileContent::StringContent(text),
|
||||
remote_server::proto::file_context_proto::Content::BinaryContent(
|
||||
bytes,
|
||||
) => crate::ai::agent::AnyFileContent::BinaryContent(bytes),
|
||||
};
|
||||
let line_range = match (fc.line_range_start, fc.line_range_end) {
|
||||
(Some(start), Some(end)) => Some(start as usize..end as usize),
|
||||
_ => None,
|
||||
};
|
||||
let last_modified = fc.last_modified_epoch_millis.map(|ms| {
|
||||
std::time::UNIX_EPOCH + std::time::Duration::from_millis(ms)
|
||||
});
|
||||
Some(crate::ai::agent::FileContext {
|
||||
file_name: fc.file_name,
|
||||
content,
|
||||
line_range,
|
||||
last_modified,
|
||||
line_count: fc.line_count as usize,
|
||||
})
|
||||
})
|
||||
.collect();
|
||||
|
||||
Ok(ReadFilesResult::Success {
|
||||
files: file_contexts,
|
||||
})
|
||||
Ok(remote_read_files_result(response))
|
||||
}),
|
||||
on_complete: Box::new(|res: Result<ReadFilesResult, anyhow::Error>, _ctx| {
|
||||
let action_result =
|
||||
@@ -254,16 +204,7 @@ impl ReadFilesExecutor {
|
||||
result.file_contexts.extend(file_result.file_contexts);
|
||||
result.missing_files.extend(file_result.missing_files);
|
||||
}
|
||||
if result.missing_files.is_empty() {
|
||||
Ok(ReadFilesResult::Success {
|
||||
files: result.file_contexts,
|
||||
})
|
||||
} else {
|
||||
let missing_files = result.missing_files.join(", ");
|
||||
Ok(ReadFilesResult::Error(format!(
|
||||
"These files do not exist: {missing_files}"
|
||||
)))
|
||||
}
|
||||
Ok(local_read_files_result(result))
|
||||
}),
|
||||
on_complete: Box::new(|res: Result<ReadFilesResult, anyhow::Error>, _ctx| {
|
||||
let action_result = res.unwrap_or_else(|e| ReadFilesResult::Error(e.to_string()));
|
||||
@@ -284,3 +225,125 @@ impl ReadFilesExecutor {
|
||||
impl Entity for ReadFilesExecutor {
|
||||
type Event = ();
|
||||
}
|
||||
|
||||
/// The shared action/protobuf contract has no partial-success representation.
|
||||
/// Do not claim a remote batch succeeded while silently losing failed paths.
|
||||
fn remote_read_files_result(
|
||||
response: remote_server::proto::ReadFileContextResponse,
|
||||
) -> ReadFilesResult {
|
||||
if !response.failed_files.is_empty()
|
||||
|| response
|
||||
.file_contexts
|
||||
.iter()
|
||||
.any(|file| file.content.is_none())
|
||||
{
|
||||
let failed = response.failed_files.iter().map(|failure| {
|
||||
(
|
||||
failure.path.as_str(),
|
||||
failure.error.as_ref().map(|error| error.message.as_str()),
|
||||
)
|
||||
});
|
||||
let missing_content = response
|
||||
.file_contexts
|
||||
.iter()
|
||||
.filter(|file| file.content.is_none())
|
||||
.map(|file| {
|
||||
let name = if file.file_name.is_empty() {
|
||||
"<unnamed remote file>"
|
||||
} else {
|
||||
file.file_name.as_str()
|
||||
};
|
||||
(name, Some("remote host returned no file content"))
|
||||
});
|
||||
return read_files_failure(failed.chain(missing_content));
|
||||
}
|
||||
let file_contexts = response
|
||||
.file_contexts
|
||||
.into_iter()
|
||||
.filter_map(|fc| {
|
||||
let content = match fc.content? {
|
||||
remote_server::proto::file_context_proto::Content::TextContent(text) => {
|
||||
crate::ai::agent::AnyFileContent::StringContent(text)
|
||||
}
|
||||
remote_server::proto::file_context_proto::Content::BinaryContent(bytes) => {
|
||||
crate::ai::agent::AnyFileContent::BinaryContent(bytes)
|
||||
}
|
||||
};
|
||||
let line_range = match (fc.line_range_start, fc.line_range_end) {
|
||||
(Some(start), Some(end)) => Some(start as usize..end as usize),
|
||||
_ => None,
|
||||
};
|
||||
let last_modified = fc
|
||||
.last_modified_epoch_millis
|
||||
.map(|ms| std::time::UNIX_EPOCH + std::time::Duration::from_millis(ms));
|
||||
Some(crate::ai::agent::FileContext {
|
||||
file_name: fc.file_name,
|
||||
content,
|
||||
line_range,
|
||||
last_modified,
|
||||
line_count: fc.line_count as usize,
|
||||
})
|
||||
})
|
||||
.collect();
|
||||
ReadFilesResult::Success {
|
||||
files: file_contexts,
|
||||
}
|
||||
}
|
||||
|
||||
fn local_read_files_result(result: super::ReadFileContextResult) -> ReadFilesResult {
|
||||
if result.missing_files.is_empty() {
|
||||
ReadFilesResult::Success {
|
||||
files: result.file_contexts,
|
||||
}
|
||||
} else {
|
||||
read_files_failure(
|
||||
result
|
||||
.missing_files
|
||||
.iter()
|
||||
.map(|path| (path.as_str(), None)),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
const MAX_REPORTED_READ_FAILURES: usize = 8;
|
||||
const MAX_READ_FAILURE_FIELD_CHARS: usize = 256;
|
||||
|
||||
fn read_files_failure<'a>(
|
||||
failures: impl IntoIterator<Item = (&'a str, Option<&'a str>)>,
|
||||
) -> ReadFilesResult {
|
||||
let mut message = "Could not read all requested files. No file contents were returned for this batch.\nFailed paths:".to_owned();
|
||||
let mut total = 0;
|
||||
for (path, reason) in failures {
|
||||
total += 1;
|
||||
if total > MAX_REPORTED_READ_FAILURES {
|
||||
continue;
|
||||
}
|
||||
message.push_str("\n- ");
|
||||
append_failure_field(&mut message, path);
|
||||
message.push_str(": ");
|
||||
append_failure_field(
|
||||
&mut message,
|
||||
reason.unwrap_or("missing, unreadable, or exceeds the read limit"),
|
||||
);
|
||||
}
|
||||
if total > MAX_REPORTED_READ_FAILURES {
|
||||
message.push_str(&format!(
|
||||
"\n{} additional failed paths omitted.",
|
||||
total - MAX_REPORTED_READ_FAILURES
|
||||
));
|
||||
}
|
||||
message.push_str("\nUse file discovery to confirm exact paths before retrying. For large files, request a smaller line range or fewer files. A file may exist but be unreadable or exceed the read limit.");
|
||||
ReadFilesResult::Error(message)
|
||||
}
|
||||
|
||||
fn append_failure_field(message: &mut String, field: &str) {
|
||||
let mut characters = field.chars();
|
||||
message.extend(characters.by_ref().take(MAX_READ_FAILURE_FIELD_CHARS));
|
||||
if characters.next().is_some() {
|
||||
message.push('…');
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "read_files_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -0,0 +1,143 @@
|
||||
use super::*;
|
||||
use crate::ai::agent::{AnyFileContent, FileContext};
|
||||
|
||||
fn remote_file() -> remote_server::proto::FileContextProto {
|
||||
remote_server::proto::FileContextProto {
|
||||
file_name: "/project/present.rs".to_owned(),
|
||||
content: Some(
|
||||
remote_server::proto::file_context_proto::Content::TextContent(
|
||||
"returned file content".to_owned(),
|
||||
),
|
||||
),
|
||||
line_range_start: Some(3),
|
||||
line_range_end: Some(5),
|
||||
last_modified_epoch_millis: Some(42),
|
||||
line_count: 20,
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn partial_remote_reads_report_failed_paths_instead_of_claiming_success() {
|
||||
let result = remote_read_files_result(remote_server::proto::ReadFileContextResponse {
|
||||
file_contexts: vec![remote_file()],
|
||||
failed_files: vec![remote_server::proto::FailedFileRead {
|
||||
path: "/project/restricted.rs".to_owned(),
|
||||
error: Some(remote_server::proto::FileOperationError {
|
||||
message: "Permission denied".to_owned(),
|
||||
}),
|
||||
}],
|
||||
});
|
||||
let ReadFilesResult::Error(message) = result else {
|
||||
panic!("A partial remote read must not claim full success");
|
||||
};
|
||||
assert!(message.contains("/project/restricted.rs: Permission denied"));
|
||||
assert!(message.contains("No file contents were returned for this batch"));
|
||||
assert!(!message.contains("returned file content"));
|
||||
assert!(message.contains("confirm exact paths"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn remote_file_context_without_content_is_an_explicit_failure() {
|
||||
let mut missing = remote_file();
|
||||
missing.file_name = "/project/no-content.rs".to_owned();
|
||||
missing.content = None;
|
||||
let result = remote_read_files_result(remote_server::proto::ReadFileContextResponse {
|
||||
file_contexts: vec![remote_file(), missing.clone()],
|
||||
failed_files: vec![],
|
||||
});
|
||||
let ReadFilesResult::Error(message) = result else {
|
||||
panic!("A remote context without content must not disappear into Success");
|
||||
};
|
||||
assert!(message.contains("/project/no-content.rs: remote host returned no file content"));
|
||||
assert!(!message.contains("returned file content"));
|
||||
|
||||
let result = remote_read_files_result(remote_server::proto::ReadFileContextResponse {
|
||||
file_contexts: vec![missing],
|
||||
failed_files: vec![remote_server::proto::FailedFileRead {
|
||||
path: "/project/restricted.rs".to_owned(),
|
||||
error: Some(remote_server::proto::FileOperationError {
|
||||
message: "Permission denied".to_owned(),
|
||||
}),
|
||||
}],
|
||||
});
|
||||
let ReadFilesResult::Error(message) = result else {
|
||||
panic!("Both remote failure categories must remain visible");
|
||||
};
|
||||
assert!(message.contains("/project/no-content.rs: remote host returned no file content"));
|
||||
assert!(message.contains("/project/restricted.rs: Permission denied"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn successful_remote_reads_preserve_content_ranges_and_file_metadata() {
|
||||
let result = remote_read_files_result(remote_server::proto::ReadFileContextResponse {
|
||||
file_contexts: vec![remote_file()],
|
||||
failed_files: vec![],
|
||||
});
|
||||
let ReadFilesResult::Success { files } = result else {
|
||||
panic!("A complete remote read must succeed");
|
||||
};
|
||||
assert_eq!(files.len(), 1);
|
||||
assert_eq!(files[0].file_name, "/project/present.rs");
|
||||
assert_eq!(
|
||||
files[0].content,
|
||||
AnyFileContent::StringContent("returned file content".to_owned())
|
||||
);
|
||||
assert_eq!(files[0].line_range, Some(3..5));
|
||||
assert_eq!(files[0].line_count, 20);
|
||||
assert_eq!(
|
||||
files[0].last_modified,
|
||||
Some(std::time::UNIX_EPOCH + std::time::Duration::from_millis(42))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn local_failures_do_not_assert_that_oversized_or_unreadable_files_are_missing() {
|
||||
let result = local_read_files_result(super::super::ReadFileContextResult {
|
||||
file_contexts: vec![],
|
||||
missing_files: vec!["/project/large.bin".to_owned()],
|
||||
});
|
||||
let ReadFilesResult::Error(message) = result else {
|
||||
panic!("An unread file must remain an error");
|
||||
};
|
||||
assert!(message.contains("/project/large.bin"));
|
||||
assert!(message.contains("exceeds the read limit"));
|
||||
assert!(message.contains("smaller line range or fewer files"));
|
||||
assert!(!message.contains("do not exist"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn successful_local_reads_preserve_their_original_content() {
|
||||
let file = FileContext {
|
||||
file_name: "/project/present.rs".to_owned(),
|
||||
content: AnyFileContent::StringContent("unchanged".to_owned()),
|
||||
line_range: None,
|
||||
last_modified: None,
|
||||
line_count: 1,
|
||||
};
|
||||
assert_eq!(
|
||||
local_read_files_result(super::super::ReadFileContextResult {
|
||||
file_contexts: vec![file.clone()],
|
||||
missing_files: vec![],
|
||||
}),
|
||||
ReadFilesResult::Success { files: vec![file] },
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failure_details_bound_unicode_paths_reasons_and_the_number_of_reported_files() {
|
||||
let long_field = "界".repeat(10_000);
|
||||
let result = read_files_failure(
|
||||
(0..MAX_REPORTED_READ_FAILURES + 3)
|
||||
.map(|_| (long_field.as_str(), Some(long_field.as_str()))),
|
||||
);
|
||||
let ReadFilesResult::Error(message) = result else {
|
||||
panic!("Failed paths must produce an error");
|
||||
};
|
||||
assert_eq!(message.matches("\n- ").count(), MAX_REPORTED_READ_FAILURES);
|
||||
assert!(message.contains("3 additional failed paths omitted"));
|
||||
assert!(message.contains('…'));
|
||||
assert!(
|
||||
message.chars().count()
|
||||
< MAX_REPORTED_READ_FAILURES * MAX_READ_FAILURE_FIELD_CHARS * 2 + 1_000
|
||||
);
|
||||
}
|
||||
@@ -2258,7 +2258,12 @@ fn provider_run_terminal_remote_log_record(
|
||||
stream_id: &ResponseStreamId,
|
||||
run: &ProviderRun,
|
||||
outcome: &ProviderRunOutcome,
|
||||
model_failure_diagnostics: serde_json::Value,
|
||||
) -> RemoteLogRecord {
|
||||
let source_error = match outcome {
|
||||
ProviderRunOutcome::Failed(failure) => failure.source.as_ref(),
|
||||
ProviderRunOutcome::Completed(_) | ProviderRunOutcome::Cancelled { .. } => None,
|
||||
};
|
||||
let (level, outcome_name, llm_finished, stop_reason, failure_kind, error) = match outcome {
|
||||
ProviderRunOutcome::Completed(completion) => (
|
||||
RemoteLogLevel::Info,
|
||||
@@ -2297,10 +2302,13 @@ fn provider_run_terminal_remote_log_record(
|
||||
"profile": run.profile().as_str(),
|
||||
"model_turn_count": run.model_turns(),
|
||||
"model_retry_count": run.model_retries(),
|
||||
"model_failure_diagnostics": model_failure_diagnostics,
|
||||
"outcome": outcome_name,
|
||||
"stop_reason": stop_reason,
|
||||
"failure_kind": failure_kind,
|
||||
"error": error,
|
||||
"error_kind": source_error.map(|error| format!("{:?}", error.kind)),
|
||||
"error_recoverable": source_error.map(|error| error.recoverable),
|
||||
"llm_finished": llm_finished,
|
||||
"provider_run_finished": true,
|
||||
"response_stream_terminal": true,
|
||||
@@ -9519,6 +9527,7 @@ impl BlocklistAIController {
|
||||
&stream_id,
|
||||
run.coordinator.run(),
|
||||
&outcome,
|
||||
run.coordinator.model_failure_diagnostics(),
|
||||
),
|
||||
);
|
||||
let completed_cleanly = matches!(
|
||||
|
||||
@@ -156,6 +156,7 @@ fn provider_terminal_logs_distinguish_clean_completion_from_failure() {
|
||||
&ProviderRunOutcome::Completed(galaxy_agent_core::ProviderRunCompletion {
|
||||
stop_reason: StopReason::Completed,
|
||||
}),
|
||||
serde_json::json!({"observed_count": 0, "recent": []}),
|
||||
);
|
||||
let failed = super::provider_run_terminal_remote_log_record(
|
||||
conversation_id,
|
||||
@@ -164,8 +165,14 @@ fn provider_terminal_logs_distinguish_clean_completion_from_failure() {
|
||||
&ProviderRunOutcome::Failed(ProviderRunFailure {
|
||||
kind: ProviderRunFailureKind::RetryLimitExceeded,
|
||||
message: "sk-secret timeout".to_owned(),
|
||||
source: None,
|
||||
source: Some(galaxy_agent_core::AgentError {
|
||||
kind: galaxy_agent_core::AgentErrorKind::Transport,
|
||||
message: "connection failed".to_owned(),
|
||||
user_message: None,
|
||||
recoverable: true,
|
||||
}),
|
||||
}),
|
||||
serde_json::json!({"observed_count": 1, "recent": [{"error": "connection failed"}]}),
|
||||
);
|
||||
|
||||
assert_eq!(completed.context["llm_finished"], true);
|
||||
@@ -176,6 +183,9 @@ fn provider_terminal_logs_distinguish_clean_completion_from_failure() {
|
||||
assert_eq!(failed.context["llm_finished"], false);
|
||||
assert_eq!(failed.context["outcome"], "failed");
|
||||
assert_eq!(failed.context["failure_kind"], "RetryLimitExceeded");
|
||||
assert_eq!(failed.context["error_kind"], "Transport");
|
||||
assert_eq!(failed.context["error_recoverable"], true);
|
||||
assert_eq!(failed.context["model_failure_diagnostics"]["observed_count"], 1);
|
||||
assert_eq!(failed.context["error"], "[redacted] timeout");
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
//! Match-mode preparation shared by native and compatibility grep tool calls.
|
||||
|
||||
/// Keep regex mode unchanged; literal mode escapes only metacharacters shared
|
||||
/// by git/grep extended regex, ripgrep, and PowerShell Select-String. This keeps
|
||||
/// the persisted action and backend execution paths identical in both modes.
|
||||
pub(crate) fn prepare_grep_queries(queries: Vec<String>, fixed_strings: bool) -> Vec<String> {
|
||||
if !fixed_strings {
|
||||
return queries;
|
||||
}
|
||||
queries
|
||||
.into_iter()
|
||||
.map(|query| {
|
||||
let mut escaped = String::with_capacity(query.len());
|
||||
for character in query.chars() {
|
||||
if matches!(
|
||||
character,
|
||||
'\\' | '.'
|
||||
| '^'
|
||||
| '$'
|
||||
| '|'
|
||||
| '?'
|
||||
| '*'
|
||||
| '+'
|
||||
| '('
|
||||
| ')'
|
||||
| '['
|
||||
| ']'
|
||||
| '{'
|
||||
| '}'
|
||||
) {
|
||||
escaped.push('\\');
|
||||
}
|
||||
escaped.push(character);
|
||||
}
|
||||
escaped
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "grep_query_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,39 @@
|
||||
use super::prepare_grep_queries;
|
||||
|
||||
#[test]
|
||||
fn literal_queries_match_source_punctuation_backslashes_and_unicode_exactly() {
|
||||
let cases = [
|
||||
("variantResolutions: []", r"variantResolutions: \[\]"),
|
||||
("indexAsset(context", r"indexAsset\(context"),
|
||||
(
|
||||
r#"describe("duplicate"|managed output|duplicateContentAsset"#,
|
||||
r#"describe\("duplicate"\|managed output\|duplicateContentAsset"#,
|
||||
),
|
||||
(r"C:\repo\file.rs", r"C:\\repo\\file\.rs"),
|
||||
("🚀 café [数据] (名)", r"🚀 café \[数据\] \(名\)"),
|
||||
(
|
||||
r#"owner's $HOME `id` #&-~ name=foo: bar"#,
|
||||
r#"owner's \$HOME `id` #&-~ name=foo: bar"#,
|
||||
),
|
||||
(
|
||||
"^foo.*(bar|baz)?[0-9]+{2}$",
|
||||
r"\^foo\.\*\(bar\|baz\)\?\[0-9\]\+\{2\}\$",
|
||||
),
|
||||
];
|
||||
for (literal, escaped) in cases {
|
||||
let queries = prepare_grep_queries(vec![literal.to_string()], true);
|
||||
assert_eq!(queries, vec![escaped], "literal: {literal}");
|
||||
let regex = regex::Regex::new(&queries[0]).unwrap();
|
||||
assert_eq!(regex.find(literal).unwrap().as_str(), literal);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn regex_queries_stay_byte_for_byte_unchanged_including_backend_specific_syntax() {
|
||||
let queries = vec![
|
||||
r"^(foo|bar)\s+[0-9]+$".to_string(),
|
||||
r"(?<=prefix)lookbehind".to_string(),
|
||||
"[invalid".to_string(),
|
||||
];
|
||||
assert_eq!(prepare_grep_queries(queries.clone(), false), queries);
|
||||
}
|
||||
@@ -81,6 +81,7 @@ pub mod connected_self_hosted_workers;
|
||||
pub mod execution_profiles;
|
||||
pub mod facts;
|
||||
pub(crate) mod generate_block_title;
|
||||
pub(crate) mod grep_query;
|
||||
pub(crate) mod generate_code_review_content;
|
||||
pub(crate) mod loading;
|
||||
pub mod mcp;
|
||||
|
||||
@@ -136,11 +136,12 @@ fn apply_file_diffs() -> ToolDefinition {
|
||||
fn grep() -> ToolDefinition {
|
||||
ToolDefinition {
|
||||
name: "grep".to_string(),
|
||||
description: "Search for up to 3 focused regex patterns per call. Use for exact symbols, strings, imports, and call sites in source code or other text files. Scope to known files or directories when possible; use search_codebase for semantic discovery. Uses git grep in git repos (respects .gitignore) or ripgrep otherwise. Returns up to 1,000 matching line locations and reports when additional matches were omitted. Use read_files afterward to see context around matches.".to_string(),
|
||||
description: "Search for up to 3 focused patterns per call. Regex is the default; set fixed_strings=true for literal source text containing brackets, parentheses, or other regex metacharacters. Use for exact symbols, strings, imports, and call sites in source code or other text files. Scope to known files or directories when possible; use search_codebase for semantic discovery. Uses git grep in git repos (respects .gitignore) or ripgrep otherwise. Returns up to 1,000 matching line locations and reports when additional matches were omitted. Use read_files afterward to see context around matches.".to_string(),
|
||||
input_schema: serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"queries": { "type": "array", "maxItems": 3, "items": { "type": "string" }, "description": "Regex patterns to search for (maximum 3 per call)" },
|
||||
"queries": { "type": "array", "maxItems": 3, "items": { "type": "string" }, "description": "Patterns (literal when fixed_strings=true) to search for (maximum 3 per call)" },
|
||||
"fixed_strings": { "type": "boolean", "default": false, "description": "Treat every query as literal text instead of regex" },
|
||||
"path": { "type": "string", "description": "Directory to scope the search to" }
|
||||
},
|
||||
"required": ["queries"]
|
||||
|
||||
@@ -1868,11 +1868,12 @@ pub fn default_tool_definitions() -> Vec<ToolDefinition> {
|
||||
},
|
||||
ToolDefinition {
|
||||
name: "grep".to_string(),
|
||||
description: "Search for regex patterns in files. Use for exact symbols, strings, imports, and call sites in source code or other text files. Scope to known files or directories when possible; use search_codebase for semantic discovery. Uses git grep in git repos (respects .gitignore) or ripgrep otherwise. Returns up to 1,000 matching line locations and reports when additional matches were omitted. Use read_files afterward to see context around matches. Pass ALL patterns you need in one call.".to_string(),
|
||||
description: "Search for patterns in files. Regex is the default; set fixed_strings=true for literal source text containing brackets, parentheses, or other regex metacharacters. Use for exact symbols, strings, imports, and call sites in source code or other text files. Scope to known files or directories when possible; use search_codebase for semantic discovery. Uses git grep in git repos (respects .gitignore) or ripgrep otherwise. Returns up to 1,000 matching line locations and reports when additional matches were omitted. Use read_files afterward to see context around matches. Pass ALL patterns you need in one call.".to_string(),
|
||||
input_schema: serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"queries": { "type": "array", "items": { "type": "string" }, "description": "Regex patterns to search for" },
|
||||
"queries": { "type": "array", "items": { "type": "string" }, "description": "Patterns (literal when fixed_strings=true) to search for" },
|
||||
"fixed_strings": { "type": "boolean", "default": false, "description": "Treat every query as literal text instead of regex" },
|
||||
"path": { "type": "string", "description": "Directory to scope the search to" }
|
||||
},
|
||||
"required": ["queries"]
|
||||
|
||||
@@ -16,6 +16,7 @@ use warp_multi_agent_api::{self as api, ClientAction, ResponseEvent};
|
||||
use super::convert::{ContentPart, ConversationMessage, MessageContent, MessageRole};
|
||||
use super::diagnostic::BedrockDiagnosticLogger;
|
||||
use crate::ai::agent::api::LegacyEvent;
|
||||
use crate::ai::grep_query::prepare_grep_queries;
|
||||
use crate::server::server_api::AIApiError;
|
||||
|
||||
fn json_to_prost_struct(value: &serde_json::Value) -> prost_types::Struct {
|
||||
@@ -1018,6 +1019,13 @@ pub fn build_tool_call_message(
|
||||
.collect()
|
||||
})
|
||||
.unwrap_or_default();
|
||||
let queries = prepare_grep_queries(
|
||||
queries,
|
||||
input
|
||||
.get("fixed_strings")
|
||||
.and_then(serde_json::Value::as_bool)
|
||||
.unwrap_or(false),
|
||||
);
|
||||
let path = input
|
||||
.get("path")
|
||||
.and_then(|v| v.as_str())
|
||||
|
||||
@@ -522,3 +522,36 @@ fn direct_provider_known_tools_exclude_hosted_only_tools() {
|
||||
assert!(is_known_tool("recall_tool_history"));
|
||||
assert!(is_known_tool("interrupt_shell_command"));
|
||||
}
|
||||
|
||||
|
||||
#[test]
|
||||
fn compatibility_grep_literal_mode_uses_the_same_escaped_patterns() {
|
||||
let tool = tool_from_event(build_tool_call_message(
|
||||
"task-1",
|
||||
"grep-1",
|
||||
"grep",
|
||||
r#"{"queries":["variantResolutions: []","indexAsset(context"],"fixed_strings":true,"path":"/project/src"}"#,
|
||||
));
|
||||
let api::message::tool_call::Tool::Grep(grep) = tool else {
|
||||
panic!("expected grep tool");
|
||||
};
|
||||
assert_eq!(
|
||||
grep.queries,
|
||||
vec![r"variantResolutions: \[\]", r"indexAsset\(context"]
|
||||
);
|
||||
assert_eq!(grep.path, "/project/src");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn compatibility_grep_keeps_default_regex_semantics() {
|
||||
let tool = tool_from_event(build_tool_call_message(
|
||||
"task-1",
|
||||
"grep-1",
|
||||
"grep",
|
||||
r#"{"queries":["foo|bar","[unclosed"]}"#,
|
||||
));
|
||||
let api::message::tool_call::Tool::Grep(grep) = tool else {
|
||||
panic!("expected grep tool");
|
||||
};
|
||||
assert_eq!(grep.queries, vec!["foo|bar", "[unclosed"]);
|
||||
}
|
||||
|
||||
@@ -17,6 +17,7 @@ use std::{
|
||||
use chrono::Utc;
|
||||
use galaxy_core::channel::ChannelState;
|
||||
use galaxyui::{Entity, ModelContext, SingletonEntity, View, ViewContext};
|
||||
use regex::Regex;
|
||||
use serde::Serialize;
|
||||
use serde_json::{json, Value};
|
||||
use settings::Setting;
|
||||
@@ -34,6 +35,15 @@ static REMOTE_LOG_CLIENT: LazyLock<Result<reqwest::Client, String>> = LazyLock::
|
||||
.map_err(|error| format!("could not create HTTP client: {error}"))
|
||||
});
|
||||
const MAX_ERROR_CHARS: usize = 500;
|
||||
const MAX_ERROR_INPUT_CHARS: usize = 16 * 1024;
|
||||
static ERROR_URL: LazyLock<Regex> =
|
||||
LazyLock::new(|| Regex::new(r#"(?i)https?://[^\s\"'<>]+"#).expect("valid error URL pattern"));
|
||||
static ERROR_CREDENTIAL: LazyLock<Regex> = LazyLock::new(|| {
|
||||
Regex::new(r#"(?i)[\"'\\]*\b(?:authorization|proxy-authorization|x-api-key|api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|password|secret)\b[\"'\\\s]*[:=]\s*"#)
|
||||
.expect("valid error credential pattern")
|
||||
});
|
||||
static ERROR_BEARER: LazyLock<Regex> =
|
||||
LazyLock::new(|| Regex::new(r#"(?i)\bbearer\s+[^\s,\"';}\]]+"#).expect("valid bearer pattern"));
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
||||
pub(crate) enum RemoteLogLevel {
|
||||
@@ -210,8 +220,49 @@ fn write_local_log(payload: &RemoteLogPayload) {
|
||||
}
|
||||
|
||||
pub(crate) fn sanitize_error(error: impl std::fmt::Display) -> String {
|
||||
let compact = error
|
||||
.to_string()
|
||||
// Stop formatting before scanning/redacting an arbitrarily large upstream
|
||||
// error body. A truncated diagnostic must never traverse the entire body.
|
||||
struct ErrorPrefix {
|
||||
text: String,
|
||||
remaining: usize,
|
||||
}
|
||||
impl std::fmt::Write for ErrorPrefix {
|
||||
fn write_str(&mut self, value: &str) -> std::fmt::Result {
|
||||
if let Some((boundary, _)) = value.char_indices().nth(self.remaining) {
|
||||
self.text.push_str(&value[..boundary]);
|
||||
self.remaining = 0;
|
||||
Err(std::fmt::Error)
|
||||
} else {
|
||||
self.text.push_str(value);
|
||||
self.remaining -= value.chars().count();
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
}
|
||||
let mut prefix = ErrorPrefix {
|
||||
text: String::new(),
|
||||
remaining: MAX_ERROR_INPUT_CHARS,
|
||||
};
|
||||
let _ = std::fmt::write(&mut prefix, format_args!("{error}"));
|
||||
let text = prefix.text;
|
||||
let safe_urls = ERROR_URL.replace_all(&text, |captures: ®ex::Captures<'_>| {
|
||||
let matched = &captures[0];
|
||||
let url_text = matched.trim_end_matches([')', ']', '}', ',', ';']);
|
||||
let suffix = &matched[url_text.len()..];
|
||||
match reqwest::Url::parse(url_text) {
|
||||
Ok(mut url) => {
|
||||
let _ = url.set_username("");
|
||||
let _ = url.set_password(None);
|
||||
url.set_query(None);
|
||||
url.set_fragment(None);
|
||||
format!("{url}{suffix}")
|
||||
}
|
||||
Err(_) => format!("[redacted URL]{suffix}"),
|
||||
}
|
||||
});
|
||||
let safe_credentials = redact_error_credentials(&safe_urls);
|
||||
let safe_bearers = ERROR_BEARER.replace_all(&safe_credentials, "Bearer [redacted]");
|
||||
let compact = safe_bearers
|
||||
.split_whitespace()
|
||||
.map(redact_sensitive_token)
|
||||
.collect::<Vec<_>>()
|
||||
@@ -219,6 +270,63 @@ pub(crate) fn sanitize_error(error: impl std::fmt::Display) -> String {
|
||||
truncate_chars(&compact, MAX_ERROR_CHARS)
|
||||
}
|
||||
|
||||
fn redact_error_credentials(text: &str) -> String {
|
||||
let mut redacted = String::with_capacity(text.len());
|
||||
let mut cursor = 0;
|
||||
for assignment in ERROR_CREDENTIAL.find_iter(text) {
|
||||
// A match inside an already-redacted credential is credential content,
|
||||
// not a new assignment. Do not copy any of that content back out.
|
||||
if assignment.start() < cursor {
|
||||
continue;
|
||||
}
|
||||
redacted.push_str(&text[cursor..assignment.end()]);
|
||||
redacted.push_str("[redacted]");
|
||||
cursor = assignment.end() + error_credential_value_end(&text[assignment.end()..]);
|
||||
}
|
||||
redacted.push_str(&text[cursor..]);
|
||||
redacted
|
||||
}
|
||||
|
||||
fn error_credential_value_end(value: &str) -> usize {
|
||||
let escapes = value.bytes().take_while(|byte| *byte == b'\\').count();
|
||||
if let Some(quote @ (b'"' | b'\'')) = value.as_bytes().get(escapes).copied() {
|
||||
// JSON strings can themselves contain escaped JSON. A delimiter at
|
||||
// nesting depth N has 2^N-1 preceding slashes; an escaped quote inside
|
||||
// its value has a different residue. Preserve the original escaping,
|
||||
// so a quoted password cannot be broken open by global unescaping.
|
||||
let span = escapes + 1;
|
||||
if !span.is_power_of_two() {
|
||||
return value.len();
|
||||
}
|
||||
let mut slashes = 0;
|
||||
let start = escapes + 1;
|
||||
for (index, character) in value[start..].char_indices() {
|
||||
if character == '\\' {
|
||||
slashes += 1;
|
||||
continue;
|
||||
}
|
||||
if character as u32 == u32::from(quote) && slashes % (2 * span) == escapes {
|
||||
return start + index + 1;
|
||||
}
|
||||
slashes = 0;
|
||||
}
|
||||
// An incomplete quoted value may be a truncated provider error body.
|
||||
return value.len();
|
||||
}
|
||||
if value.starts_with('{') || value.starts_with('[') {
|
||||
let mut values = serde_json::Deserializer::from_str(value).into_iter::<Value>();
|
||||
return if values.next().is_some_and(|result| result.is_ok()) {
|
||||
values.byte_offset()
|
||||
} else {
|
||||
value.len()
|
||||
};
|
||||
}
|
||||
// An unquoted credential/header has no reliable value terminator. It may
|
||||
// contain spaces, an authentication scheme, or multiple signed fields.
|
||||
// Redact through the line boundary instead of guessing and exposing a suffix.
|
||||
value.find(['\r', '\n']).unwrap_or(value.len())
|
||||
}
|
||||
|
||||
pub(crate) fn raw_model_payload_context<M>(
|
||||
ctx: &ModelContext<M>,
|
||||
build_payload: impl FnOnce(usize) -> Value,
|
||||
@@ -287,7 +395,7 @@ async fn send_remote_log(config: RemoteLogConfig, payload: RemoteLogPayload) ->
|
||||
.json(&payload)
|
||||
.send()
|
||||
.await
|
||||
.map_err(|error| format!("request failed: {error}"))?;
|
||||
.map_err(|error| format!("request failed: {}", sanitize_error(error)))?;
|
||||
|
||||
if !response.status().is_success() {
|
||||
return Err(format!("server returned HTTP {}", response.status()));
|
||||
|
||||
@@ -24,6 +24,109 @@ fn errors_are_compacted_truncated_and_lightly_redacted() {
|
||||
assert!(sanitized.chars().count() <= 501);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn error_redaction_removes_url_credentials_query_tokens_and_header_values() {
|
||||
let input = "request failed for https://alice:private-password@example.com/v1/chat?api_key=query-value&code=oauth-code#fragment\nAuthorization: Bearer bearer-value\nx-api-key: header-value\n{\"refresh_token\":\"refresh-value\",\"api_key\":\"json-value\"}\nTLS certificate unknown issuer";
|
||||
let actual = sanitize_error(input);
|
||||
for secret in [
|
||||
"alice",
|
||||
"private-password",
|
||||
"query-value",
|
||||
"oauth-code",
|
||||
"fragment",
|
||||
"bearer-value",
|
||||
"header-value",
|
||||
"refresh-value",
|
||||
"json-value",
|
||||
] {
|
||||
assert!(
|
||||
!actual.contains(secret),
|
||||
"credential escaped redaction: {secret}"
|
||||
);
|
||||
}
|
||||
assert!(actual.contains("https://example.com/v1/chat"));
|
||||
assert!(actual.contains("TLS certificate unknown issuer"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn error_redaction_handles_nested_json_and_preserves_nonsecret_token_counts() {
|
||||
let actual = sanitize_error(
|
||||
r#"{\"access_token\":\"private-access\",\"password\":\"private-password\",\"input_tokens\":42} network timed out"#,
|
||||
);
|
||||
assert!(!actual.contains("private-access"));
|
||||
assert!(!actual.contains("private-password"));
|
||||
assert!(actual.contains("input_tokens"));
|
||||
assert!(actual.contains("42"));
|
||||
assert!(actual.contains("network timed out"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn escaped_quotes_and_backslashes_do_not_expose_credential_suffixes() {
|
||||
for input in [
|
||||
r#"{"password":"private-\"quoted\"-suffix","input_tokens":42} connection reset"#,
|
||||
r#"{\"password\":\"private-\\\"quoted\\\"-suffix\",\"input_tokens\":42} connection reset"#,
|
||||
r#"api_key="private\\path\"quoted-suffix" connection reset"#,
|
||||
r#"password="private unterminated credential suffix"#,
|
||||
] {
|
||||
let actual = sanitize_error(input);
|
||||
for secret in ["private", "quoted", "suffix", "path"] {
|
||||
assert!(
|
||||
!actual.contains(secret),
|
||||
"credential fragment escaped redaction: {actual}"
|
||||
);
|
||||
}
|
||||
if input.contains("connection reset") {
|
||||
assert!(actual.contains("connection reset"));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn basic_and_multifield_authorization_values_are_redacted() {
|
||||
let basic = sanitize_error("Authorization: Basic dXNlcjpwYXNz\nTLS certificate rejected");
|
||||
assert!(!basic.contains("dXNlcjpwYXNz"));
|
||||
assert!(basic.contains("TLS certificate rejected"));
|
||||
let digest = sanitize_error("Proxy-Authorization: Digest username=private-user, response=private-response\nconnection reset");
|
||||
assert!(!digest.contains("private-user"));
|
||||
assert!(!digest.contains("private-response"));
|
||||
assert!(digest.contains("connection reset"));
|
||||
let password = sanitize_error("password: private password with spaces\nnetwork timeout");
|
||||
assert!(!password.contains("private password"));
|
||||
assert!(password.contains("network timeout"));
|
||||
for prefix in ["Basic ", "Bearer "] {
|
||||
let password = sanitize_error(format!(
|
||||
"password: {prefix}private password with spaces\nnetwork timeout"
|
||||
));
|
||||
assert!(!password.contains("private password"));
|
||||
assert!(!password.contains("with spaces"));
|
||||
assert!(password.contains("network timeout"));
|
||||
}
|
||||
let spaced = sanitize_error("Authorization: Bearer\t private-token\nx-api-key: private key with spaces\nnetwork timeout");
|
||||
assert!(!spaced.contains("private-token"));
|
||||
assert!(!spaced.contains("private key"));
|
||||
assert!(spaced.contains("network timeout"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn error_formatting_stops_before_walking_an_unbounded_display() {
|
||||
use std::cell::Cell;
|
||||
struct LongError<'a>(&'a Cell<usize>);
|
||||
impl std::fmt::Display for LongError<'_> {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
for _ in 0..1_000_000 {
|
||||
self.0.set(self.0.get() + 1);
|
||||
f.write_str("é日本🙂 network timed out ")?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
let written = Cell::new(0);
|
||||
let output = sanitize_error(LongError(&written));
|
||||
assert!(written.get() < 2_000);
|
||||
assert!(output.chars().count() <= 501);
|
||||
assert!(output.contains("network timed out"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn raw_payload_cap_keeps_tail() {
|
||||
assert_eq!(
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
mod child_context;
|
||||
mod event_translator;
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
mod model_failure_diagnostics;
|
||||
mod provider_run_coordinator;
|
||||
mod rig;
|
||||
mod rig_request;
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
//! Bounded operational evidence retained until a provider run finishes.
|
||||
//!
|
||||
//! A failed telemetry POST must not be the only copy of a model failure. The
|
||||
//! terminal event includes these summaries even when the run later recovers.
|
||||
//! This is process-local diagnostic state, never provider history/checkpoints.
|
||||
|
||||
use std::collections::VecDeque;
|
||||
|
||||
use chrono::Utc;
|
||||
use galaxy_agent_core::AgentError;
|
||||
use serde_json::{json, Value};
|
||||
|
||||
use crate::ai::remote_logging::sanitize_error;
|
||||
|
||||
const MAX_FAILURES: usize = 8;
|
||||
const MAX_ID_CHARS: usize = 256;
|
||||
|
||||
#[derive(Default)]
|
||||
pub(crate) struct ModelFailureDiagnostics {
|
||||
observed_count: u64,
|
||||
recent: VecDeque<Value>,
|
||||
}
|
||||
|
||||
impl ModelFailureDiagnostics {
|
||||
pub(crate) fn record(&mut self, identity: Value, error: &AgentError) {
|
||||
self.observed_count = self.observed_count.saturating_add(1);
|
||||
// Select operational identifiers explicitly; never retain a request,
|
||||
// prompt, provider configuration, or arbitrary caller-supplied context.
|
||||
let mut record = json!({
|
||||
"timestamp": Utc::now().to_rfc3339(),
|
||||
"error_kind": format!("{:?}", error.kind),
|
||||
"error_recoverable": error.recoverable,
|
||||
"error": sanitize_error(&error.message),
|
||||
});
|
||||
for key in ["provider_run_id", "profile", "runtime_id", "model_id"] {
|
||||
if let Some(value) = identity.get(key).and_then(Value::as_str) {
|
||||
record[key] = json!(value.chars().take(MAX_ID_CHARS).collect::<String>());
|
||||
}
|
||||
}
|
||||
for key in ["provider_epoch", "retry_attempt", "elapsed_ms"] {
|
||||
if let Some(value) = identity.get(key).and_then(Value::as_u64) {
|
||||
record[key] = json!(value);
|
||||
}
|
||||
}
|
||||
if self.recent.len() == MAX_FAILURES {
|
||||
self.recent.pop_front();
|
||||
}
|
||||
self.recent.push_back(record);
|
||||
}
|
||||
|
||||
pub(crate) fn snapshot(&self) -> Value {
|
||||
json!({
|
||||
"scope": "current_process",
|
||||
"observed_count": self.observed_count,
|
||||
"truncated": self.observed_count > self.recent.len() as u64,
|
||||
"recent": self.recent,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "model_failure_diagnostics_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,58 @@
|
||||
use galaxy_agent_core::AgentErrorKind;
|
||||
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn retains_sanitized_causes_after_recovery_without_request_content() {
|
||||
let mut diagnostics = ModelFailureDiagnostics::default();
|
||||
let mut error = AgentError::new(AgentErrorKind::Transport, "sk-private connection failed");
|
||||
error.recoverable = true;
|
||||
diagnostics.record(
|
||||
json!({
|
||||
"provider_run_id": "run-1", "provider_epoch": 6,
|
||||
"retry_attempt": 0, "elapsed_ms": 100,
|
||||
"model_id": "test-model", "runtime_id": "rig:openai", "profile": "base",
|
||||
"prompt": "private prompt", "api_key": "private credential",
|
||||
}),
|
||||
&error,
|
||||
);
|
||||
let snapshot = diagnostics.snapshot();
|
||||
let serialized = snapshot.to_string();
|
||||
assert_eq!(snapshot["observed_count"], 1);
|
||||
assert_eq!(
|
||||
snapshot["recent"][0]["error"],
|
||||
"[redacted] connection failed"
|
||||
);
|
||||
assert_eq!(snapshot["recent"][0]["error_kind"], "Transport");
|
||||
assert_eq!(snapshot["recent"][0]["provider_epoch"], 6);
|
||||
assert_eq!(snapshot["recent"][0]["error_recoverable"], true);
|
||||
assert!(!serialized.contains("private prompt"));
|
||||
assert!(!serialized.contains("private credential"));
|
||||
assert!(!serialized.contains("sk-private"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn retention_keeps_newest_failures_with_explicit_omission_accounting() {
|
||||
let mut diagnostics = ModelFailureDiagnostics::default();
|
||||
for epoch in 0..MAX_FAILURES + 2 {
|
||||
diagnostics.record(
|
||||
json!({"provider_epoch": epoch, "model_id": "é".repeat(10_000)}),
|
||||
&AgentError::new(AgentErrorKind::Provider, "overloaded"),
|
||||
);
|
||||
}
|
||||
let snapshot = diagnostics.snapshot();
|
||||
assert_eq!(snapshot["scope"], "current_process");
|
||||
assert_eq!(snapshot["observed_count"], MAX_FAILURES as u64 + 2);
|
||||
assert_eq!(snapshot["truncated"], true);
|
||||
let recent = snapshot["recent"].as_array().unwrap();
|
||||
assert_eq!(recent.len(), MAX_FAILURES);
|
||||
assert_eq!(recent[0]["provider_epoch"], 2);
|
||||
assert_eq!(
|
||||
recent[0]["model_id"].as_str().unwrap().chars().count(),
|
||||
MAX_ID_CHARS
|
||||
);
|
||||
assert_eq!(
|
||||
ModelFailureDiagnostics::default().snapshot()["observed_count"],
|
||||
0
|
||||
);
|
||||
}
|
||||
@@ -18,6 +18,8 @@ use galaxy_agent_core::{
|
||||
use instant::Instant;
|
||||
use warpui::r#async::Timer;
|
||||
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
use super::model_failure_diagnostics::ModelFailureDiagnostics;
|
||||
use super::task_progress::{add_task_list_context, initial_task_list, task_list_from_plan_batch};
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
use crate::ai::activity;
|
||||
@@ -290,6 +292,8 @@ pub(crate) struct ProviderRunCoordinator {
|
||||
max_context_tokens: Option<u32>,
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
activity_session_id: Option<String>,
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
model_failure_diagnostics: ModelFailureDiagnostics,
|
||||
}
|
||||
|
||||
impl ProviderRunCoordinator {
|
||||
@@ -334,6 +338,8 @@ impl ProviderRunCoordinator {
|
||||
max_context_tokens: None,
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
activity_session_id: None,
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
model_failure_diagnostics: ModelFailureDiagnostics::default(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -349,6 +355,11 @@ impl ProviderRunCoordinator {
|
||||
self.activity_session_id = Some(conversation_id.to_string());
|
||||
}
|
||||
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
pub(crate) fn model_failure_diagnostics(&self) -> serde_json::Value {
|
||||
self.model_failure_diagnostics.snapshot()
|
||||
}
|
||||
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
fn activity_context(
|
||||
&self,
|
||||
@@ -1206,6 +1217,12 @@ impl ProviderRunCoordinator {
|
||||
where
|
||||
F: FnMut(ProviderRunProjection) -> BoxFuture<'static, Result<(), String>>,
|
||||
{
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
{
|
||||
let mut identity = self.activity_context(call, profile);
|
||||
identity["elapsed_ms"] = serde_json::json!(elapsed_millis(started_at));
|
||||
self.model_failure_diagnostics.record(identity, &error);
|
||||
}
|
||||
#[cfg(not(target_family = "wasm"))]
|
||||
model_output_logging::log(serde_json::json!({
|
||||
"event": "model_error",
|
||||
|
||||
@@ -16,6 +16,7 @@ use crate::ai::agent::{
|
||||
RunAgentsRequest, SearchCodebaseRequest, ShellCommandDelay,
|
||||
};
|
||||
use crate::ai::document::ai_document_model::AIDocumentId;
|
||||
use crate::ai::grep_query::prepare_grep_queries;
|
||||
|
||||
const MAX_BATCH_READ_ITEMS: usize = 6;
|
||||
const MAX_BATCH_SEARCH_ITEMS: usize = 3;
|
||||
@@ -83,7 +84,10 @@ pub(crate) fn action_from_tool_call(
|
||||
title: Some(required_string(input, "summary")?),
|
||||
},
|
||||
"grep" => AIAgentActionType::Grep {
|
||||
queries: limited_required_strings(input, "queries", MAX_BATCH_SEARCH_ITEMS)?,
|
||||
queries: prepare_grep_queries(
|
||||
limited_required_strings(input, "queries", MAX_BATCH_SEARCH_ITEMS)?,
|
||||
optional_boolean(input, "fixed_strings")?.unwrap_or(false),
|
||||
),
|
||||
path: optional_string(input, "path")?.unwrap_or_default(),
|
||||
},
|
||||
"file_glob" => AIAgentActionType::FileGlob {
|
||||
|
||||
@@ -651,3 +651,74 @@ fn mcp_title_metadata_does_not_change_tool_arguments() {
|
||||
if input == serde_json::json!({"message": "hello", "_galaxy_tool_title": "a real server argument"}))
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
#[test]
|
||||
fn grep_literal_mode_escapes_queries_without_changing_scope() {
|
||||
let action = action_from_tool_call(
|
||||
"task",
|
||||
&call(
|
||||
"grep",
|
||||
serde_json::json!({
|
||||
"queries": ["variantResolutions: []", "indexAsset(context"],
|
||||
"fixed_strings": true,
|
||||
"path": "/project/src",
|
||||
}),
|
||||
),
|
||||
&SkillPathOrigin::Local,
|
||||
&HashMap::new(),
|
||||
)
|
||||
.unwrap();
|
||||
let AIAgentActionType::Grep { queries, path } = action.action else {
|
||||
panic!("expected grep action");
|
||||
};
|
||||
assert_eq!(
|
||||
queries,
|
||||
vec![r"variantResolutions: \[\]", r"indexAsset\(context"]
|
||||
);
|
||||
assert_eq!(path, "/project/src");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn grep_default_and_explicit_regex_modes_preserve_patterns() {
|
||||
for fixed_strings in [None, Some(false)] {
|
||||
let mut input = serde_json::json!({"queries": ["[unclosed", "foo|bar"]});
|
||||
if let Some(value) = fixed_strings {
|
||||
input["fixed_strings"] = value.into();
|
||||
}
|
||||
let action = action_from_tool_call(
|
||||
"task",
|
||||
&call("grep", input),
|
||||
&SkillPathOrigin::Local,
|
||||
&HashMap::new(),
|
||||
)
|
||||
.unwrap();
|
||||
let AIAgentActionType::Grep { queries, .. } = action.action else {
|
||||
panic!("expected grep action");
|
||||
};
|
||||
assert_eq!(queries, vec!["[unclosed", "foo|bar"]);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn grep_literal_mode_still_rejects_invalid_booleans_and_excessive_batches() {
|
||||
for (input, expected) in [
|
||||
(
|
||||
serde_json::json!({"queries": ["needle"], "fixed_strings": "true"}),
|
||||
"fixed_strings",
|
||||
),
|
||||
(
|
||||
serde_json::json!({"queries": ["a", "b", "c", "d"], "fixed_strings": true}),
|
||||
"queries",
|
||||
),
|
||||
] {
|
||||
let error = action_from_tool_call(
|
||||
"task",
|
||||
&call("grep", input),
|
||||
&SkillPathOrigin::Local,
|
||||
&HashMap::new(),
|
||||
)
|
||||
.unwrap_err();
|
||||
assert!(error.contains(expected), "unexpected error: {error}");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user