Skip to content

Stabilize autonomous tool loops and coach feedback handoff #73

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
sinieogni wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Stabilize autonomous tool loops and coach feedback handoff #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4e5fad
fix(g3-core): suppress player stutter text and duplicate tool exec pe…
sinieogni Mar 15, 2026
8eb6cf7
fix(g3): preserve meaningful coach feedback across autonomous turns
sinieogni Mar 16, 2026
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
116 changes: 16 additions & 100 deletions crates/g3-cli/src/coach_feedback.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@
//! Extracts feedback from the coach agent's session logs for the coach-player loop.

use anyhow::Result;
use std::path::Path;

use g3_core::Agent;
use g3_core::{Agent, FeedbackExtractionConfig};

use crate::simple_output::SimpleOutput;
use crate::ui_writer_impl::ConsoleUiWriter;
Expand All @@ -18,107 +16,25 @@ pub fn extract_from_logs(
coach_agent: &Agent<ConsoleUiWriter>,
output: &SimpleOutput,
) -> Result<String> {
let session_id = coach_agent
.get_session_id()
.ok_or_else(|| anyhow::anyhow!("Coach agent has no session ID"))?;

let log_file_path = resolve_log_path(&session_id);

// Try to extract from session log
if let Some(feedback) = try_extract_from_log(&log_file_path) {
output.print(&format!("✅ Extracted coach feedback from session: {}", session_id));
return Ok(feedback);
let extracted = g3_core::extract_coach_feedback(
coach_result,
coach_agent,
&FeedbackExtractionConfig::default(),
);

if extracted.is_fallback() {
return Err(anyhow::anyhow!(
"Could not extract coach feedback. Coach result response length: {} chars",
coach_result.response.len()
));
}

// Fallback: use the TaskResult's extract_summary method
let fallback = coach_result.extract_summary();
if !fallback.is_empty() {
if let Some(session_id) = coach_agent.get_session_id() {
output.print(&format!(
"✅ Extracted coach feedback from response: {} chars",
fallback.len()
"✅ Extracted coach feedback from session: {} ({:?})",
session_id, extracted.source
));
return Ok(fallback);
}

Err(anyhow::anyhow!(
"Could not extract coach feedback from session: {}\n\
Log file path: {:?}\n\
Log file exists: {}\n\
Coach result response length: {} chars",
session_id,
log_file_path,
log_file_path.exists(),
coach_result.response.len()
))
}

/// Resolve the log file path, trying new path first then falling back to old.
fn resolve_log_path(session_id: &str) -> std::path::PathBuf {
g3_core::get_session_file(session_id)
}

/// Extract feedback from a session log file.
///
/// Searches backwards for the last assistant message with substantial text content.
fn try_extract_from_log(log_file_path: &Path) -> Option<String> {
if !log_file_path.exists() {
return None;
}

let log_content = std::fs::read_to_string(log_file_path).ok()?;
let log_json: serde_json::Value = serde_json::from_str(&log_content).ok()?;

let messages = log_json
.get("context_window")?
.get("conversation_history")?
.as_array()?;

// Search backwards for the last assistant message with text content
for msg in messages.iter().rev() {
if let Some(feedback) = extract_assistant_text(msg) {
return Some(feedback);
}
}

None
}

/// Extract text content from an assistant message.
fn extract_assistant_text(msg: &serde_json::Value) -> Option<String> {
let role = msg.get("role").and_then(|v| v.as_str())?;
if !role.eq_ignore_ascii_case("assistant") {
return None;
}

let content = msg.get("content")?;

// Handle string content
if let Some(content_str) = content.as_str() {
return filter_substantial_text(content_str);
}

// Handle array content (native tool calling format)
if let Some(content_array) = content.as_array() {
for block in content_array {
if block.get("type").and_then(|v| v.as_str()) == Some("text") {
if let Some(text) = block.get("text").and_then(|v| v.as_str()) {
if let Some(result) = filter_substantial_text(text) {
return Some(result);
}
}
}
}
}

None
}

/// Filter out empty or very short responses (likely just tool calls).
fn filter_substantial_text(text: &str) -> Option<String> {
let trimmed = text.trim();
if !trimmed.is_empty() && trimmed.len() > 10 {
Some(trimmed.to_string())
} else {
None
}
Ok(extracted.content)
}
156 changes: 123 additions & 33 deletions crates/g3-core/src/feedback_extraction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,9 @@ pub fn extract_coach_feedback<W>(
config: &FeedbackExtractionConfig,
) -> ExtractedFeedback
where
W: UiWriter + Clone + Send + Sync + 'static,
W: UiWriter + Send + Sync + 'static,
{
// Try session log first - now looks for last assistant message (primary method)
if let Some(session_id) = agent.get_session_id() {
if let Some(feedback) = try_extract_last_assistant_message(&session_id, config) {
debug!("Extracted coach feedback from last assistant message: {} chars", feedback.len());
return ExtractedFeedback::new(feedback, FeedbackSource::ConversationHistory);
}
}

// Fallback: Try session log with final_output pattern (backwards compatibility)
// Try session log with verified final_output pattern first.
if let Some(session_id) = agent.get_session_id() {
if let Some(feedback) = try_extract_from_session_log(&session_id, config) {
debug!("Extracted coach feedback from session log (final_output): {} chars", feedback.len());
Expand All @@ -128,11 +120,22 @@ where

// Fallback: Try TaskResult parsing (extracts last text block)
let extracted = coach_result.extract_final_output();
if !extracted.is_empty() {
if !extracted.is_empty() && !is_meta_coach_text(&extracted) {
debug!("Extracted coach feedback from task result: {} chars", extracted.len());
return ExtractedFeedback::new(extracted, FeedbackSource::TaskResultResponse);
}

// Last resort: use the last assistant message, but skip obvious meta-review chatter.
if let Some(session_id) = agent.get_session_id() {
if let Some(feedback) = try_extract_last_assistant_message(&session_id, config) {
debug!(
"Extracted coach feedback from last assistant message: {} chars",
feedback.len()
);
return ExtractedFeedback::new(feedback, FeedbackSource::ConversationHistory);
}
}

// Fallback to default
warn!("Could not extract coach feedback, using default");
ExtractedFeedback::new(config.default_feedback.clone(), FeedbackSource::DefaultFallback)
Expand Down Expand Up @@ -163,41 +166,76 @@ fn try_extract_last_assistant_message(
.get("conversation_history")?
.as_array()?;

// Search backwards for the last assistant message with text content
extract_last_meaningful_assistant_message(messages)
}

fn extract_last_meaningful_assistant_message(messages: &[Value]) -> Option<String> {
for msg in messages.iter().rev() {
let role = msg.get("role").and_then(|v| v.as_str())?;

if role.eq_ignore_ascii_case("assistant") {
if let Some(content) = msg.get("content") {
// Handle string content
if let Some(content_str) = content.as_str() {
let trimmed = content_str.trim();
// Skip empty or very short responses (likely just tool calls)
if let Some(text) = extract_assistant_text(msg) {
if !is_meta_coach_text(&text) {
return Some(text);
}
}
}
}

None
}

fn extract_assistant_text(msg: &Value) -> Option<String> {
let content = msg.get("content")?;

if let Some(content_str) = content.as_str() {
let trimmed = content_str.trim();
if !trimmed.is_empty() && trimmed.len() > 10 {
return Some(trimmed.to_string());
}
}

if let Some(content_array) = content.as_array() {
for block in content_array {
if block.get("type").and_then(|v| v.as_str()) == Some("text") {
if let Some(text) = block.get("text").and_then(|v| v.as_str()) {
let trimmed = text.trim();
if !trimmed.is_empty() && trimmed.len() > 10 {
return Some(trimmed.to_string());
}
}
// Handle array content (native tool calling format)
// Look for text blocks in the array
if let Some(content_array) = content.as_array() {
for block in content_array {
if block.get("type").and_then(|v| v.as_str()) == Some("text") {
if let Some(text) = block.get("text").and_then(|v| v.as_str()) {
let trimmed = text.trim();
if !trimmed.is_empty() && trimmed.len() > 10 {
return Some(trimmed.to_string());
}
}
}
}
}
}
}
}

None
}

fn is_meta_coach_text(text: &str) -> bool {
let normalized = text.trim().to_ascii_lowercase();
let prefixes = [
"let me ",
"now let me ",
"i'll ",
"i will ",
"first, let me ",
];
let meta_markers = [
"run the acceptance commands",
"check the remaining",
"review the requirements more carefully",
"read the spec more carefully",
"check several important details",
"check the current working directory",
"find the actual workspace directory",
"deeper review of the implementation quality",
"more carefully",
];

prefixes.iter().any(|p| normalized.starts_with(p))
&& meta_markers.iter().any(|m| normalized.contains(m))
}

/// Try to extract feedback from session log file
fn try_extract_from_session_log(
session_id: &str,
Expand Down Expand Up @@ -629,4 +667,56 @@ mod tests {
assert!(!config.verbose);
assert!(config.default_feedback.contains("review"));
}

#[test]
fn test_is_meta_coach_text_detects_review_chatter() {
assert!(is_meta_coach_text(
"Now let me check the attach_to_order method more carefully — specifically whether attaching to an order moves the state to linked."
));
assert!(!is_meta_coach_text(
"Fix reopened history persistence and make attach_to_order persist linked state."
));
}

#[test]
fn test_extract_last_meaningful_assistant_message_skips_meta_review_chatter() {
let messages = vec![
serde_json::json!({
"role": "assistant",
"content": "Let me run the acceptance commands from the correct directory."
}),
serde_json::json!({
"role": "assistant",
"content": "Now let me check the attach_to_order method more carefully — specifically whether attaching to an order moves the state to linked."
}),
serde_json::json!({
"role": "assistant",
"content": "The implementation is missing reopened history preservation and attach_to_order does not persist linked state."
}),
];

let extracted = extract_last_meaningful_assistant_message(&messages);
assert_eq!(
extracted.as_deref(),
Some(
"The implementation is missing reopened history preservation and attach_to_order does not persist linked state."
)
);
}

#[test]
fn test_extract_last_meaningful_assistant_message_returns_none_for_only_meta_chatter() {
let messages = vec![
serde_json::json!({
"role": "assistant",
"content": "Let me check the current working directory and run the tests."
}),
serde_json::json!({
"role": "assistant",
"content": "Now let me check the remaining acceptance commands individually and review the requirements more carefully."
}),
];

assert_eq!(extract_last_meaningful_assistant_message(&messages), None);
}
}
Loading