Skip to content

review: expose failed review diagnostics in raw logs #213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
smcdef wants to merge 1 commit into
base: main
Choose a base branch
from
Open

review: expose failed review diagnostics in raw logs #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,7 @@ impl Database {
) -> Result<()> {
self.conn
.execute(
"UPDATE reviews SET status = ?, result_description = ?, summary = ?, interaction_id = ?, inline_review = ?, logs = ? WHERE id = ?",
"UPDATE reviews SET status = ?, result_description = ?, summary = ?, interaction_id = ?, inline_review = ?, logs = COALESCE(?, logs) WHERE id = ?",
libsql::params![status, result, summary, interaction_id, inline_review, logs, review_id],
)
.await?;
Expand Down
123 changes: 122 additions & 1 deletion src/reviewer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ use tokio::process::Command;
use tokio::sync::Semaphore;
use tracing::{error, info, warn};

const REVIEW_LOG_MAX_ENTRIES: usize = 2000;
const REVIEW_LOG_STRING_LIMIT: usize = 12_000;

#[derive(Clone)]
struct ReviewContext {
semaphore: Arc<Semaphore>,
Expand Down Expand Up @@ -1473,6 +1476,65 @@ impl Reviewer {
}
}

fn truncate_review_log_string(s: &str) -> String {
let redacted = redact_secret(s);
if redacted.chars().count() <= REVIEW_LOG_STRING_LIMIT {
return redacted;
}

let mut truncated: String = redacted.chars().take(REVIEW_LOG_STRING_LIMIT).collect();
truncated.push_str("\n...[truncated]");
truncated
}

fn truncate_review_log_value(value: &mut Value) {
match value {
Value::String(s) => {
*s = truncate_review_log_string(s);
}
Value::Array(values) => {
for value in values {
truncate_review_log_value(value);
}
}
Value::Object(map) => {
for value in map.values_mut() {
truncate_review_log_value(value);
}
}
_ => {}
}
}

fn push_review_log_entry(log: &mut Vec<Value>, mut entry: Value) {
truncate_review_log_value(&mut entry);

if log.len() < REVIEW_LOG_MAX_ENTRIES {
log.push(entry);
} else if log.len() == REVIEW_LOG_MAX_ENTRIES {
log.push(json!({
"role": "system",
"content": "Additional review log entries were omitted."
}));
}
}

fn review_logs_json(log: &[Value]) -> Option<String> {
if log.is_empty() {
None
} else {
serde_json::to_string_pretty(log).ok()
}
}

fn review_history_needs_fallback(json: &Value) -> bool {
match json.get("history") {
None | Some(Value::Null) => true,
Some(Value::Array(entries)) => entries.is_empty(),
Some(_) => true,
}
}

#[allow(clippy::too_many_arguments)]
async fn run_review_tool(
patchset_id: i64,
Expand Down Expand Up @@ -1575,6 +1637,7 @@ async fn run_review_tool(
cmd.kill_on_drop(true);

let mut child = cmd.spawn()?;
let mut captured_log = Vec::new();

if let Some(stderr) = child.stderr.take() {
tokio::spawn(async move {
Expand Down Expand Up @@ -1665,6 +1728,26 @@ async fn run_review_tool(
serde_json::from_value::<AiRequest>(payload_val.clone())
{
turn_count += 1;
let mut request_entry = req
.messages
.last()
.and_then(|m| serde_json::to_value(m).ok())
.unwrap_or_else(|| {
json!({
"role": "user",
"content": "AI request contained no messages"
})
});
if let Some(obj) = request_entry.as_object_mut() {
obj.insert(
"context_tag".to_string(),
Value::String(
req.context_tag.clone().unwrap_or_default(),
),
);
obj.insert("turn".to_string(), json!(turn_count));
}
push_review_log_entry(&mut captured_log, request_entry);
if settings.ai.log_turns {
let n_msgs = req.messages.len();
let last = req.messages.last();
Expand Down Expand Up @@ -1726,6 +1809,20 @@ async fn run_review_tool(

let reply = match resp_payload {
Ok(p) => {
let mut response_entry = serde_json::to_value(&p)
.unwrap_or_else(|_| {
json!({
"content": "Failed to serialize AI response"
})
});
if let Some(obj) = response_entry.as_object_mut() {
obj.insert(
"role".to_string(),
Value::String("assistant".to_string()),
);
obj.insert("turn".to_string(), json!(turn_count));
}
push_review_log_entry(&mut captured_log, response_entry);
if let Some(usage) = &p.usage {
let cached = usage.cached_tokens.unwrap_or(0);
let uncached_input = usage.prompt_tokens.saturating_sub(cached);
Expand Down Expand Up @@ -1791,6 +1888,13 @@ async fn run_review_tool(
Err(e) => {
let message = e.to_string();
let class = classify_ai_error(&e);
push_review_log_entry(
&mut captured_log,
json!({
"role": "system",
"content": format!("AI provider error: {message}")
}),
);
let payload = RemoteAiErrorPayload::new(message, class);
json!({ "type": "error", "payload": payload })
}
Expand Down Expand Up @@ -1840,7 +1944,14 @@ async fn run_review_tool(
let _ = child.wait().await; // Reap zombie

match interaction_result {
Ok(json) => {
Ok(mut json) => {
if review_history_needs_fallback(&json)
&& let Some(logs) = review_logs_json(&captured_log)
&& let Ok(history) = serde_json::from_str::<Value>(&logs)
&& let Some(obj) = json.as_object_mut()
{
obj.insert("history".to_string(), history);
}
// Update DB with patch statuses if final_result available
if let Some(patches) = json["patches"].as_array() {
for p in patches {
Expand Down Expand Up @@ -1887,6 +1998,16 @@ async fn run_review_tool(
Ok(json)
}
Err(e) => {
if let Some(logs) = review_logs_json(&captured_log)
&& let Err(log_err) = db
.update_review_status(review_id, ReviewStatus::InReview.as_str(), Some(&logs))
.await
{
warn!(
"Failed to persist partial review log for review {}: {}",
review_id, log_err
);
}
// Check if it's the specific active time exceeded error we throw in the loop
if e.to_string()
.contains("Review tool timed out (active time exceeded)")
Expand Down
17 changes: 15 additions & 2 deletions static/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2382,9 +2382,21 @@ <h1>${escapeHtml(data.subject) || '(no subject)'}</h1>
if (!res.ok) throw new Error('Review not found');
const data = await res.json();

const failedLogView = String(data.status || '').toLowerCase().includes('failed') ||
String(data.status || '').toLowerCase().includes('error');
const failureReasonHtml = failedLogView ? `
<div class="section" style="background:var(--error-bg); border:1px solid var(--error-border); padding:12px; border-radius:4px; margin-bottom:16px;">
<h2 style="margin-top:0; color:var(--error-title);">Final Failure Reason</h2>
<div style="color:var(--error-text); white-space:pre-wrap;">${escapeHtml(data.result || 'No failure reason recorded.')}</div>
</div>
` : '';

const totalTokens = (data.tokens_in || 0) + (data.tokens_out || 0) + (data.tokens_cached || 0);
let logs = [];
try { logs = JSON.parse(data.logs || '[]'); } catch (e) { }
try {
const parsedLogs = JSON.parse(data.logs || '[]');
logs = Array.isArray(parsedLogs) ? parsedLogs : [];
} catch (e) { }

let logsHtml = '';
if (!logs.length) {
Expand All @@ -2408,7 +2420,7 @@ <h1>${escapeHtml(data.subject) || '(no subject)'}</h1>
if (entry.thought && typeof entry.thought === 'string') {
content += `<div style="color: var(--thought-color); font-style: italic; border-left: 2px solid var(--thought-border); padding-left: 8px; margin: 4px 0; white-space: pre-wrap;">${escapeHtml(entry.thought)}</div>`;
}

if (entry.content && typeof entry.content === 'string') {
if (role === 'tool') {
content += formatJsonExpandable(entry.content, jsonCounter++);
Expand Down Expand Up @@ -2456,6 +2468,7 @@ <h1>
<span class="status-badge status-${(data.status || '').replace(/ /g, '')}">${data.status}</span>
</span>
</h1>
${failureReasonHtml}
<div>${logsHtml}</div>
`;
}
Expand Down