mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-27 08:00:17 +00:00
feat: structured fallback deliverables for failed/stuck jobs (#236)
* feat: structured fallback deliverables for failed/stuck jobs (#221) When a job fails or gets stuck, build a FallbackDeliverable that captures partial results, action statistics, cost, timing, and repair attempts. This replaces opaque error strings with structured data users can act on. - Add FallbackDeliverable, LastAction, ActionStats types in context/fallback.rs - Store fallback in JobContext.metadata["fallback_deliverable"] on failure - Surface fallback in job_status tool output and SSE job_result events - Update mark_failed() and mark_stuck() in worker to build fallback - 8 unit tests covering zero/mixed actions, truncation, timing, serialization Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address review comments on fallback deliverables - Fix doc comment: "200 chars" -> "200 bytes (UTF-8 safe)" since truncate_str operates on byte length, not character count. - Add code comment documenting that SSE fallback_deliverable is currently always None (forward-compatible infrastructure). Co-Authored-By: Claude Opus 4.6 <[email protected]> * refactor: take Option<&FallbackDeliverable> instead of &Option<…> Addresses Gemini review feedback: idiomatic Rust prefers Option<&T> over &Option<T> for borrowed optional values. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: guard against non-object metadata and add fallback test - store_fallback_in_metadata now resets metadata to {} when it's any non-object type (string, array, number), not just null. Prevents panic on index assignment. - Add test_job_status_includes_fallback_deliverable to verify the fallback field is surfaced in job_status tool output. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: use sanitized output in fallback preview + add integration tests Security fix: FallbackDeliverable::build() now uses output_sanitized instead of output_raw, preventing secrets/PII from leaking through the job_status tool and SSE job_result events. Also adds: - test_fallback_uses_sanitized_output: proves raw secrets don't leak - test_store_fallback_in_metadata_roundtrip: full serialize/deserialize - test_store_fallback_handles_non_object_metadata: edge case coverage - test_store_fallback_none_is_noop: None input is safe Addresses serrrfirat review feedback on PR #236. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: harden fallback deliverables against review findings - Truncate failure_reason to 1000 bytes to prevent metadata bloat - Add tracing::warn on fallback serialization failure (was silently discarded) - Fix module/struct docs to cover stuck jobs, remove stale SSE claim - Fix job.rs test to use real FallbackDeliverable field names - Add tests for failure_reason truncation and completed_at=None elapsed time - Fix pre-existing clippy warning in settings.rs (field_reassign_with_default) Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address Copilot review findings on fallback deliverables - Fix output_raw/output_sanitized field swap in ActionRecord::succeed() so sanitized data actually goes into the sanitized field (security) - Return None instead of empty Memory when get_memory fails in build_fallback, with tracing::warn for observability - Replace manual elapsed calculation with ctx.elapsed() which already clamps negative durations Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: resolve rebase conflicts and update tests for parameter swap - Add fallback field to SseEvent::JobResult in job_monitor - Fix type annotation in fallback deliverable test - Update test_action_record_succeed_sets_fields for new parameter order - Use create_job_for_user in test (API changed on main) Co-Authored-By: Claude Opus 4.6 <[email protected]> * chore: trigger CI re-check after rebase * fix: fall back to error message for failed action output_preview When the last action is a failed tool call, output_sanitized is None, leaving output_preview empty. Now falls back to the action's error message so users see what went wrong. [skip-regression-check] * ci: add safety comments to test code for no-panics check The CI no-panics grep check cannot distinguish test code inside src/ files from production code. Add // safety: test annotations to .unwrap(), .expect(), and assert!() calls in #[cfg(test)] modules. * fix: clarify succeed() doc and avoid clone in output_preview - Fix doc comment: output_raw is stored as pretty-printed JSON string, not a raw JSON value - Borrow string slice directly in fallback preview to avoid cloning potentially large sanitized outputs before truncation * refactor: reuse floor_char_boundary in truncate_str Replace hand-rolled UTF-8 boundary logic with existing crate::util::floor_char_boundary to reduce duplication. * fix: rename SSE fallback field to fallback_deliverable for consistency The SSE JobResult field was named `fallback` while everywhere else (metadata key, job_status tool) uses `fallback_deliverable`. Align the SSE wire format to avoid forcing clients to handle two names. --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
alexthebuildr
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
86ae12747b
commit
65062f3cc0
+149
-28
@@ -196,6 +196,7 @@ impl Worker {
|
||||
.get("session_id")
|
||||
.and_then(|v| v.as_str())
|
||||
.map(|s| s.to_string()),
|
||||
fallback_deliverable: data.get("fallback_deliverable").cloned(),
|
||||
}),
|
||||
_ => None,
|
||||
};
|
||||
@@ -960,9 +961,14 @@ Report when the job is complete or if you encounter issues you cannot resolve."#
|
||||
}
|
||||
|
||||
async fn mark_failed(&self, reason: &str) -> Result<(), Error> {
|
||||
// Build fallback deliverable from memory before transitioning.
|
||||
let fallback = self.build_fallback(reason).await;
|
||||
|
||||
self.context_manager()
|
||||
.update_context(self.job_id, |ctx| {
|
||||
ctx.transition_to(JobState::Failed, Some(reason.to_string()))
|
||||
ctx.transition_to(JobState::Failed, Some(reason.to_string()))?;
|
||||
store_fallback_in_metadata(ctx, fallback.as_ref());
|
||||
Ok(())
|
||||
})
|
||||
.await?
|
||||
.map_err(|s| crate::error::JobError::ContextError {
|
||||
@@ -983,8 +989,15 @@ Report when the job is complete or if you encounter issues you cannot resolve."#
|
||||
}
|
||||
|
||||
async fn mark_stuck(&self, reason: &str) -> Result<(), Error> {
|
||||
// Build fallback deliverable from memory before transitioning.
|
||||
let fallback = self.build_fallback(reason).await;
|
||||
|
||||
self.context_manager()
|
||||
.update_context(self.job_id, |ctx| ctx.mark_stuck(reason))
|
||||
.update_context(self.job_id, |ctx| {
|
||||
ctx.mark_stuck(reason)?;
|
||||
store_fallback_in_metadata(ctx, fallback.as_ref());
|
||||
Ok(())
|
||||
})
|
||||
.await?
|
||||
.map_err(|s| crate::error::JobError::ContextError {
|
||||
id: self.job_id,
|
||||
@@ -1002,6 +1015,57 @@ Report when the job is complete or if you encounter issues you cannot resolve."#
|
||||
self.persist_status(JobState::Stuck, Some(reason.to_string()));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Build a [`FallbackDeliverable`] from the current job context and memory.
|
||||
async fn build_fallback(&self, reason: &str) -> Option<crate::context::FallbackDeliverable> {
|
||||
let memory = match self.context_manager().get_memory(self.job_id).await {
|
||||
Ok(memory) => memory,
|
||||
Err(e) => {
|
||||
tracing::warn!(
|
||||
job_id = %self.job_id,
|
||||
"Failed to load memory while building fallback deliverable: {e}"
|
||||
);
|
||||
return None;
|
||||
}
|
||||
};
|
||||
let ctx = match self.context_manager().get_context(self.job_id).await {
|
||||
Ok(ctx) => ctx,
|
||||
Err(e) => {
|
||||
tracing::warn!(
|
||||
job_id = %self.job_id,
|
||||
"Failed to load context while building fallback deliverable: {e}"
|
||||
);
|
||||
return None;
|
||||
}
|
||||
};
|
||||
Some(crate::context::FallbackDeliverable::build(
|
||||
&ctx, &memory, reason,
|
||||
))
|
||||
}
|
||||
}
|
||||
|
||||
/// Store a fallback deliverable in the job context's metadata.
|
||||
fn store_fallback_in_metadata(
|
||||
ctx: &mut crate::context::JobContext,
|
||||
fallback: Option<&crate::context::FallbackDeliverable>,
|
||||
) {
|
||||
let Some(fb) = fallback else {
|
||||
return;
|
||||
};
|
||||
match serde_json::to_value(fb) {
|
||||
Ok(val) => {
|
||||
if !ctx.metadata.is_object() {
|
||||
ctx.metadata = serde_json::json!({});
|
||||
}
|
||||
ctx.metadata["fallback_deliverable"] = val;
|
||||
}
|
||||
Err(e) => {
|
||||
tracing::warn!(
|
||||
"Failed to serialize fallback deliverable for job {}: {e}",
|
||||
ctx.job_id
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Job delegate: implements `LoopDelegate` for the background job context.
|
||||
@@ -1440,7 +1504,7 @@ mod tests {
|
||||
}
|
||||
|
||||
let cm = Arc::new(crate::context::ContextManager::new(5));
|
||||
let job_id = cm.create_job("test", "test job").await.unwrap();
|
||||
let job_id = cm.create_job("test", "test job").await.unwrap(); // safety: test
|
||||
|
||||
let deps = WorkerDeps {
|
||||
context_manager: cm,
|
||||
@@ -1472,8 +1536,9 @@ mod tests {
|
||||
tool_call_id: "call_abc123".to_string(),
|
||||
};
|
||||
|
||||
assert_eq!(selection.tool_call_id, "call_abc123");
|
||||
assert_eq!(selection.tool_call_id, "call_abc123"); // safety: test
|
||||
assert_ne!(
|
||||
/* safety: test */
|
||||
selection.tool_call_id, "tool_call_id",
|
||||
"tool_call_id must not be the hardcoded placeholder string"
|
||||
);
|
||||
@@ -1509,11 +1574,12 @@ mod tests {
|
||||
let results = worker.execute_tools_parallel(&selections).await;
|
||||
let elapsed = start.elapsed();
|
||||
|
||||
assert_eq!(results.len(), 3);
|
||||
assert_eq!(results.len(), 3); // safety: test
|
||||
for r in &results {
|
||||
assert!(r.result.is_ok(), "Tool should succeed");
|
||||
assert!(r.result.is_ok(), "Tool should succeed"); // safety: test
|
||||
}
|
||||
assert!(
|
||||
/* safety: test */
|
||||
elapsed < Duration::from_millis(800),
|
||||
"Parallel execution took {:?}, expected < 800ms (sequential would be ~600ms)",
|
||||
elapsed
|
||||
@@ -1565,9 +1631,9 @@ mod tests {
|
||||
|
||||
let results = worker.execute_tools_parallel(&selections).await;
|
||||
|
||||
assert!(results[0].result.as_ref().unwrap().contains("done_tool_a"));
|
||||
assert!(results[1].result.as_ref().unwrap().contains("done_tool_b"));
|
||||
assert!(results[2].result.as_ref().unwrap().contains("done_tool_c"));
|
||||
assert!(results[0].result.as_ref().unwrap().contains("done_tool_a")); // safety: test
|
||||
assert!(results[1].result.as_ref().unwrap().contains("done_tool_b")); // safety: test
|
||||
assert!(results[2].result.as_ref().unwrap().contains("done_tool_c")); // safety: test
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -1583,8 +1649,9 @@ mod tests {
|
||||
}];
|
||||
|
||||
let results = worker.execute_tools_parallel(&selections).await;
|
||||
assert_eq!(results.len(), 1);
|
||||
assert_eq!(results.len(), 1); // safety: test
|
||||
assert!(
|
||||
/* safety: test */
|
||||
results[0].result.is_err(),
|
||||
"Missing tool should produce an error, not a panic"
|
||||
);
|
||||
@@ -1600,23 +1667,24 @@ mod tests {
|
||||
ctx.transition_to(JobState::InProgress, None)
|
||||
})
|
||||
.await
|
||||
.unwrap()
|
||||
.unwrap();
|
||||
.unwrap() // safety: test
|
||||
.unwrap(); // safety: test
|
||||
|
||||
worker.mark_completed().await.unwrap();
|
||||
worker.mark_completed().await.unwrap(); // safety: test
|
||||
|
||||
let ctx = worker
|
||||
.context_manager()
|
||||
.get_context(worker.job_id)
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(ctx.state, JobState::Completed);
|
||||
.unwrap(); // safety: test
|
||||
assert_eq!(ctx.state, JobState::Completed); // safety: test
|
||||
|
||||
// Second mark_completed should succeed (idempotent) rather than
|
||||
// erroring, matching the fix for the execution_loop / worker wrapper
|
||||
// race condition.
|
||||
let result = worker.mark_completed().await;
|
||||
assert!(
|
||||
/* safety: test */
|
||||
result.is_ok(),
|
||||
"Completed -> Completed transition should be idempotent"
|
||||
);
|
||||
@@ -1641,7 +1709,7 @@ mod tests {
|
||||
}
|
||||
|
||||
let cm = Arc::new(crate::context::ContextManager::new(5));
|
||||
let job_id = cm.create_job("test", "test job").await.unwrap();
|
||||
let job_id = cm.create_job("test", "test job").await.unwrap(); // safety: test
|
||||
|
||||
let deps = WorkerDeps {
|
||||
context_manager: cm,
|
||||
@@ -1740,6 +1808,7 @@ mod tests {
|
||||
.execute_tool("needs_approval", &serde_json::json!({}))
|
||||
.await;
|
||||
assert!(
|
||||
/* safety: test */
|
||||
result.is_err(),
|
||||
"Should be blocked without approval context"
|
||||
);
|
||||
@@ -1752,7 +1821,7 @@ mod tests {
|
||||
let result = worker_allowed
|
||||
.execute_tool("needs_approval", &serde_json::json!({}))
|
||||
.await;
|
||||
assert!(result.is_ok(), "Should be allowed with autonomous context");
|
||||
assert!(result.is_ok(), "Should be allowed with autonomous context"); // safety: test
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -1766,6 +1835,7 @@ mod tests {
|
||||
.execute_tool("always_approval", &serde_json::json!({}))
|
||||
.await;
|
||||
assert!(
|
||||
/* safety: test */
|
||||
result.is_err(),
|
||||
"Always tool should be blocked without permission"
|
||||
);
|
||||
@@ -1781,6 +1851,7 @@ mod tests {
|
||||
.execute_tool("always_approval", &serde_json::json!({}))
|
||||
.await;
|
||||
assert!(
|
||||
/* safety: test */
|
||||
result.is_ok(),
|
||||
"Always tool should be allowed with permission"
|
||||
);
|
||||
@@ -1797,8 +1868,8 @@ mod tests {
|
||||
ctx.transition_to(JobState::InProgress, None)
|
||||
})
|
||||
.await
|
||||
.unwrap()
|
||||
.unwrap();
|
||||
.unwrap() // safety: test
|
||||
.unwrap(); // safety: test
|
||||
|
||||
// Set a token budget
|
||||
worker
|
||||
@@ -1807,16 +1878,17 @@ mod tests {
|
||||
ctx.max_tokens = 100;
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
.unwrap(); // safety: test
|
||||
|
||||
// Simulate adding tokens that exceed the budget
|
||||
let budget_result = worker
|
||||
.context_manager()
|
||||
.update_context(worker.job_id, |ctx| ctx.add_tokens(200))
|
||||
.await
|
||||
.unwrap();
|
||||
.unwrap(); // safety: test
|
||||
|
||||
assert!(
|
||||
/* safety: test */
|
||||
budget_result.is_err(),
|
||||
"Should return error when token budget exceeded"
|
||||
);
|
||||
@@ -1825,13 +1897,13 @@ mod tests {
|
||||
worker
|
||||
.mark_failed(&budget_result.unwrap_err().to_string())
|
||||
.await
|
||||
.unwrap();
|
||||
.unwrap(); // safety: test
|
||||
let ctx = worker
|
||||
.context_manager()
|
||||
.get_context(worker.job_id)
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(ctx.state, JobState::Failed);
|
||||
.unwrap(); // safety: test
|
||||
assert_eq!(ctx.state, JobState::Failed); // safety: test
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -1845,21 +1917,22 @@ mod tests {
|
||||
ctx.transition_to(JobState::InProgress, None)
|
||||
})
|
||||
.await
|
||||
.unwrap()
|
||||
.unwrap();
|
||||
.unwrap() // safety: test
|
||||
.unwrap(); // safety: test
|
||||
|
||||
// Simulate what the execution loop does when max_iterations is exceeded
|
||||
worker
|
||||
.mark_failed("Maximum iterations exceeded: job hit the iteration cap")
|
||||
.await
|
||||
.unwrap();
|
||||
.unwrap(); // safety: test
|
||||
|
||||
let ctx = worker
|
||||
.context_manager()
|
||||
.get_context(worker.job_id)
|
||||
.await
|
||||
.unwrap();
|
||||
.unwrap(); // safety: test
|
||||
assert_eq!(
|
||||
/* safety: test */
|
||||
ctx.state,
|
||||
JobState::Failed,
|
||||
"Iteration cap should transition to Failed, not Stuck"
|
||||
@@ -1989,4 +2062,52 @@ mod tests {
|
||||
"Should skip empty first reasoning and return the first non-empty one"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_store_fallback_in_metadata_roundtrip() {
|
||||
use crate::context::FallbackDeliverable;
|
||||
|
||||
let mut ctx = JobContext::new("Test", "fallback roundtrip");
|
||||
let memory = crate::context::Memory::new(ctx.job_id);
|
||||
let fb = FallbackDeliverable::build(&ctx, &memory, "test failure");
|
||||
|
||||
// Store into metadata
|
||||
store_fallback_in_metadata(&mut ctx, Some(&fb));
|
||||
|
||||
// Verify it's stored and can be deserialized back
|
||||
let stored = ctx.metadata.get("fallback_deliverable");
|
||||
assert!(stored.is_some(), "fallback missing from metadata"); // safety: test
|
||||
|
||||
let recovered: FallbackDeliverable =
|
||||
serde_json::from_value(stored.unwrap().clone()).expect("deserialize fallback"); // safety: test
|
||||
assert_eq!(recovered.failure_reason, "test failure"); // safety: test
|
||||
assert!(!recovered.partial); // safety: test
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_store_fallback_handles_non_object_metadata() {
|
||||
use crate::context::FallbackDeliverable;
|
||||
|
||||
let mut ctx = JobContext::new("Test", "non-object metadata");
|
||||
ctx.metadata = serde_json::json!("not an object");
|
||||
|
||||
let memory = crate::context::Memory::new(ctx.job_id);
|
||||
let fb = FallbackDeliverable::build(&ctx, &memory, "failed");
|
||||
|
||||
store_fallback_in_metadata(&mut ctx, Some(&fb));
|
||||
|
||||
// Must normalize to object and store
|
||||
assert!(ctx.metadata.is_object()); // safety: test
|
||||
assert!(ctx.metadata.get("fallback_deliverable").is_some()); // safety: test
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_store_fallback_none_is_noop() {
|
||||
let mut ctx = JobContext::new("Test", "noop");
|
||||
let original = ctx.metadata.clone();
|
||||
|
||||
store_fallback_in_metadata(&mut ctx, None);
|
||||
|
||||
assert_eq!(ctx.metadata, original); // safety: test
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user