diff --git a/app/src/lib/i18n/ar.ts b/app/src/lib/i18n/ar.ts index a75efcd4f9..c15190b6e4 100644 --- a/app/src/lib/i18n/ar.ts +++ b/app/src/lib/i18n/ar.ts @@ -60,6 +60,12 @@ const messages: TranslationMap = { 'تحقّق من اتصالك أو إعدادات النموذج؛ سيعيد OpenHuman المحاولة.', 'conversations.toolFailure.timeout.cause': 'استغرق الإجراء وقتًا طويلاً وتم إيقافه.', 'conversations.toolFailure.timeout.next': 'سيعيد OpenHuman المحاولة، أو يمكنك إعادتها يدويًا.', + 'conversations.toolFailure.denied.cause': 'لقد رفضت هذا الإجراء.', + 'conversations.toolFailure.denied.next': + 'لا حاجة لأي شيء — لم يُنفَّذ. اطلبه مجددًا إذا غيّرت رأيك.', + 'conversations.toolFailure.approvalExpired.cause': 'انتهت صلاحية طلب الموافقة قبل أن يردّ أحد.', + 'conversations.toolFailure.approvalExpired.next': + 'اطلبه مجددًا لتشغيله — لن يعيد OpenHuman المحاولة من تلقاء نفسه.', 'conversations.toolFailure.unknown.cause': 'حدث خطأ ما في هذا الإجراء.', 'conversations.toolFailure.unknown.next': 'حاول مرة أخرى؛ وإذا استمر الفشل، شغّل التشخيص من الإعدادات.', diff --git a/app/src/lib/i18n/bn.ts b/app/src/lib/i18n/bn.ts index cf2f10fba4..a672a9ad98 100644 --- a/app/src/lib/i18n/bn.ts +++ b/app/src/lib/i18n/bn.ts @@ -64,6 +64,12 @@ const messages: TranslationMap = { 'conversations.toolFailure.timeout.cause': 'কাজটি অনেক বেশি সময় নেওয়ায় থামিয়ে দেওয়া হয়েছে।', 'conversations.toolFailure.timeout.next': 'OpenHuman আবার চেষ্টা করবে, অথবা আপনি নিজে আবার চেষ্টা করতে পারেন।', + 'conversations.toolFailure.denied.cause': 'আপনি এই কাজটি প্রত্যাখ্যান করেছেন।', + 'conversations.toolFailure.denied.next': 'কিছু করার নেই — এটি চালানো হয়নি। মত বদলালে আবার বলুন।', + 'conversations.toolFailure.approvalExpired.cause': + 'কেউ সাড়া দেওয়ার আগেই অনুমোদনের অনুরোধের মেয়াদ শেষ হয়ে গেছে।', + 'conversations.toolFailure.approvalExpired.next': + 'এটি চালাতে আবার বলুন — OpenHuman নিজে থেকে পুনরায় চেষ্টা করবে না।', 'conversations.toolFailure.unknown.cause': 'এই কাজটিতে কিছু ভুল হয়েছে।', 'conversations.toolFailure.unknown.next': 'আবার চেষ্টা করুন; বারবার ব্যর্থ হলে সেটিংস থেকে ডায়াগনস্টিকস চালান।', diff --git a/app/src/lib/i18n/de.ts b/app/src/lib/i18n/de.ts index c42eaaaa8b..5bc8f26fb4 100644 --- a/app/src/lib/i18n/de.ts +++ b/app/src/lib/i18n/de.ts @@ -68,6 +68,13 @@ const messages: TranslationMap = { 'conversations.toolFailure.timeout.cause': 'Die Aktion hat zu lange gedauert und wurde gestoppt.', 'conversations.toolFailure.timeout.next': 'OpenHuman versucht es erneut, oder du kannst es manuell wiederholen.', + 'conversations.toolFailure.denied.cause': 'Du hast diese Aktion abgelehnt.', + 'conversations.toolFailure.denied.next': + 'Nichts zu tun — sie wurde nicht ausgeführt. Frag erneut, wenn du es dir anders überlegst.', + 'conversations.toolFailure.approvalExpired.cause': + 'Die Genehmigungsanfrage ist abgelaufen, bevor jemand geantwortet hat.', + 'conversations.toolFailure.approvalExpired.next': + 'Frag erneut, um sie auszuführen — OpenHuman versucht es nicht von selbst erneut.', 'conversations.toolFailure.unknown.cause': 'Bei dieser Aktion ist etwas schiefgelaufen.', 'conversations.toolFailure.unknown.next': 'Versuche es erneut; wenn es weiterhin fehlschlägt, führe die Diagnose in den Einstellungen aus.', diff --git a/app/src/lib/i18n/en.ts b/app/src/lib/i18n/en.ts index 326630c0ad..0af06d66ea 100644 --- a/app/src/lib/i18n/en.ts +++ b/app/src/lib/i18n/en.ts @@ -3852,6 +3852,13 @@ const en: TranslationMap = { 'conversations.toolFailure.timeout.cause': 'The action took too long and was stopped.', 'conversations.toolFailure.timeout.next': 'OpenHuman will try again, or you can retry it manually.', + 'conversations.toolFailure.denied.cause': 'You declined this action.', + 'conversations.toolFailure.denied.next': + 'Nothing to do — it was not run. Ask again if you change your mind.', + 'conversations.toolFailure.approvalExpired.cause': + 'The approval request expired before anyone responded.', + 'conversations.toolFailure.approvalExpired.next': + "Ask again to run it — OpenHuman won't retry it on its own.", 'conversations.toolFailure.unknown.cause': 'Something went wrong with this action.', 'conversations.toolFailure.unknown.next': 'Try again; if it keeps failing, run diagnostics from Settings.', diff --git a/app/src/lib/i18n/es.ts b/app/src/lib/i18n/es.ts index 5d917e7568..6456390a30 100644 --- a/app/src/lib/i18n/es.ts +++ b/app/src/lib/i18n/es.ts @@ -66,6 +66,13 @@ const messages: TranslationMap = { 'conversations.toolFailure.timeout.cause': 'La acción tardó demasiado y se detuvo.', 'conversations.toolFailure.timeout.next': 'OpenHuman lo intentará de nuevo, o puedes reintentarlo manualmente.', + 'conversations.toolFailure.denied.cause': 'Rechazaste esta acción.', + 'conversations.toolFailure.denied.next': + 'No hay nada que hacer: no se ejecutó. Vuelve a pedirlo si cambias de opinión.', + 'conversations.toolFailure.approvalExpired.cause': + 'La solicitud de aprobación caducó antes de que alguien respondiera.', + 'conversations.toolFailure.approvalExpired.next': + 'Vuelve a pedirlo para ejecutarlo: OpenHuman no lo reintentará por su cuenta.', 'conversations.toolFailure.unknown.cause': 'Algo salió mal con esta acción.', 'conversations.toolFailure.unknown.next': 'Inténtalo de nuevo; si sigue fallando, ejecuta el diagnóstico desde Configuración.', diff --git a/app/src/lib/i18n/fr.ts b/app/src/lib/i18n/fr.ts index 9a645489e4..0569a5edfd 100644 --- a/app/src/lib/i18n/fr.ts +++ b/app/src/lib/i18n/fr.ts @@ -66,6 +66,13 @@ const messages: TranslationMap = { 'conversations.toolFailure.timeout.cause': "L'action a pris trop de temps et a été arrêtée.", 'conversations.toolFailure.timeout.next': "OpenHuman réessaiera, ou vous pouvez relancer l'action manuellement.", + 'conversations.toolFailure.denied.cause': 'Vous avez refusé cette action.', + 'conversations.toolFailure.denied.next': + "Rien à faire — elle n'a pas été exécutée. Redemandez si vous changez d'avis.", + 'conversations.toolFailure.approvalExpired.cause': + "La demande d'approbation a expiré avant que quiconque réponde.", + 'conversations.toolFailure.approvalExpired.next': + "Redemandez pour l'exécuter — OpenHuman ne réessaiera pas tout seul.", 'conversations.toolFailure.unknown.cause': 'Un problème est survenu avec cette action.', 'conversations.toolFailure.unknown.next': "Réessayez ; si l'échec persiste, lancez le diagnostic depuis les Paramètres.", diff --git a/app/src/lib/i18n/hi.ts b/app/src/lib/i18n/hi.ts index 7a29722b84..18aea7dd51 100644 --- a/app/src/lib/i18n/hi.ts +++ b/app/src/lib/i18n/hi.ts @@ -63,6 +63,13 @@ const messages: TranslationMap = { 'conversations.toolFailure.timeout.cause': 'कार्य में बहुत अधिक समय लगा और इसे रोक दिया गया।', 'conversations.toolFailure.timeout.next': 'OpenHuman फिर से प्रयास करेगा, या आप इसे मैन्युअल रूप से दोबारा कर सकते हैं।', + 'conversations.toolFailure.denied.cause': 'आपने इस क्रिया को अस्वीकार कर दिया।', + 'conversations.toolFailure.denied.next': + 'कुछ नहीं करना है — यह चलाई नहीं गई। मन बदलें तो फिर से कहें।', + 'conversations.toolFailure.approvalExpired.cause': + 'किसी के जवाब देने से पहले ही अनुमोदन अनुरोध की समय-सीमा समाप्त हो गई।', + 'conversations.toolFailure.approvalExpired.next': + 'इसे चलाने के लिए फिर से कहें — OpenHuman इसे स्वयं दोबारा नहीं आज़माएगा।', 'conversations.toolFailure.unknown.cause': 'इस कार्य में कुछ गड़बड़ हो गई।', 'conversations.toolFailure.unknown.next': 'दोबारा प्रयास करें; यदि यह बार-बार विफल हो, तो सेटिंग्स से डायग्नोस्टिक्स चलाएँ।', diff --git a/app/src/lib/i18n/id.ts b/app/src/lib/i18n/id.ts index c506aeab87..e776a282d1 100644 --- a/app/src/lib/i18n/id.ts +++ b/app/src/lib/i18n/id.ts @@ -65,6 +65,13 @@ const messages: TranslationMap = { 'conversations.toolFailure.timeout.cause': 'Tindakan memakan waktu terlalu lama dan dihentikan.', 'conversations.toolFailure.timeout.next': 'OpenHuman akan mencoba lagi, atau Anda dapat mengulanginya secara manual.', + 'conversations.toolFailure.denied.cause': 'Anda menolak tindakan ini.', + 'conversations.toolFailure.denied.next': + 'Tidak ada yang perlu dilakukan — tindakan ini tidak dijalankan. Minta lagi jika Anda berubah pikiran.', + 'conversations.toolFailure.approvalExpired.cause': + 'Permintaan persetujuan kedaluwarsa sebelum ada yang merespons.', + 'conversations.toolFailure.approvalExpired.next': + 'Minta lagi untuk menjalankannya — OpenHuman tidak akan mencobanya sendiri.', 'conversations.toolFailure.unknown.cause': 'Terjadi kesalahan pada tindakan ini.', 'conversations.toolFailure.unknown.next': 'Coba lagi; jika terus gagal, jalankan diagnostik dari Pengaturan.', diff --git a/app/src/lib/i18n/it.ts b/app/src/lib/i18n/it.ts index 0f02fd2ab8..79bbaa5db9 100644 --- a/app/src/lib/i18n/it.ts +++ b/app/src/lib/i18n/it.ts @@ -66,6 +66,13 @@ const messages: TranslationMap = { "L'azione ha richiesto troppo tempo ed è stata interrotta.", 'conversations.toolFailure.timeout.next': 'OpenHuman riproverà, oppure puoi riprovare manualmente.', + 'conversations.toolFailure.denied.cause': 'Hai rifiutato questa azione.', + 'conversations.toolFailure.denied.next': + 'Niente da fare — non è stata eseguita. Richiedila di nuovo se cambi idea.', + 'conversations.toolFailure.approvalExpired.cause': + 'La richiesta di approvazione è scaduta prima che qualcuno rispondesse.', + 'conversations.toolFailure.approvalExpired.next': + 'Richiedila di nuovo per eseguirla — OpenHuman non riproverà da solo.', 'conversations.toolFailure.unknown.cause': 'Qualcosa è andato storto con questa azione.', 'conversations.toolFailure.unknown.next': 'Riprova; se continua a fallire, esegui la diagnostica dalle Impostazioni.', diff --git a/app/src/lib/i18n/ko.ts b/app/src/lib/i18n/ko.ts index bc5157f2a4..22fcf7d697 100644 --- a/app/src/lib/i18n/ko.ts +++ b/app/src/lib/i18n/ko.ts @@ -60,6 +60,13 @@ const messages: TranslationMap = { 'conversations.toolFailure.timeout.cause': '작업이 너무 오래 걸려 중지되었습니다.', 'conversations.toolFailure.timeout.next': 'OpenHuman이 다시 시도하거나 수동으로 다시 실행할 수 있습니다.', + 'conversations.toolFailure.denied.cause': '이 작업을 거부했습니다.', + 'conversations.toolFailure.denied.next': + '할 일이 없습니다 — 실행되지 않았습니다. 마음이 바뀌면 다시 요청하세요.', + 'conversations.toolFailure.approvalExpired.cause': + '아무도 응답하기 전에 승인 요청이 만료되었습니다.', + 'conversations.toolFailure.approvalExpired.next': + '실행하려면 다시 요청하세요 — OpenHuman이 스스로 재시도하지 않습니다.', 'conversations.toolFailure.unknown.cause': '이 작업에서 문제가 발생했습니다.', 'conversations.toolFailure.unknown.next': '다시 시도하세요. 계속 실패하면 설정에서 진단을 실행하세요.', diff --git a/app/src/lib/i18n/pl.ts b/app/src/lib/i18n/pl.ts index 0ad901757e..e7a1715246 100644 --- a/app/src/lib/i18n/pl.ts +++ b/app/src/lib/i18n/pl.ts @@ -68,6 +68,13 @@ const messages: TranslationMap = { 'conversations.toolFailure.timeout.cause': 'Czynność trwała zbyt długo i została zatrzymana.', 'conversations.toolFailure.timeout.next': 'OpenHuman spróbuje ponownie lub możesz powtórzyć ją ręcznie.', + 'conversations.toolFailure.denied.cause': 'Odrzuciłeś tę czynność.', + 'conversations.toolFailure.denied.next': + 'Nic do zrobienia — nie została wykonana. Poproś ponownie, jeśli zmienisz zdanie.', + 'conversations.toolFailure.approvalExpired.cause': + 'Prośba o zatwierdzenie wygasła, zanim ktokolwiek odpowiedział.', + 'conversations.toolFailure.approvalExpired.next': + 'Poproś ponownie, aby ją uruchomić — OpenHuman nie ponowi próby samodzielnie.', 'conversations.toolFailure.unknown.cause': 'Coś poszło nie tak z tą czynnością.', 'conversations.toolFailure.unknown.next': 'Spróbuj ponownie; jeśli nadal się nie udaje, uruchom diagnostykę w Ustawieniach.', diff --git a/app/src/lib/i18n/pt.ts b/app/src/lib/i18n/pt.ts index 9896ddac74..448501dfd3 100644 --- a/app/src/lib/i18n/pt.ts +++ b/app/src/lib/i18n/pt.ts @@ -65,6 +65,13 @@ const messages: TranslationMap = { 'conversations.toolFailure.timeout.cause': 'A ação demorou demais e foi interrompida.', 'conversations.toolFailure.timeout.next': 'O OpenHuman tentará novamente, ou você pode tentar manualmente.', + 'conversations.toolFailure.denied.cause': 'Você recusou esta ação.', + 'conversations.toolFailure.denied.next': + 'Nada a fazer — ela não foi executada. Peça novamente se mudar de ideia.', + 'conversations.toolFailure.approvalExpired.cause': + 'A solicitação de aprovação expirou antes que alguém respondesse.', + 'conversations.toolFailure.approvalExpired.next': + 'Peça novamente para executá-la — o OpenHuman não tentará de novo sozinho.', 'conversations.toolFailure.unknown.cause': 'Algo deu errado com esta ação.', 'conversations.toolFailure.unknown.next': 'Tente novamente; se continuar falhando, execute o diagnóstico nas Configurações.', diff --git a/app/src/lib/i18n/ru.ts b/app/src/lib/i18n/ru.ts index facf3881ce..5bd10891fc 100644 --- a/app/src/lib/i18n/ru.ts +++ b/app/src/lib/i18n/ru.ts @@ -68,6 +68,13 @@ const messages: TranslationMap = { 'Действие заняло слишком много времени и было остановлено.', 'conversations.toolFailure.timeout.next': 'OpenHuman повторит попытку, или вы можете повторить её вручную.', + 'conversations.toolFailure.denied.cause': 'Вы отклонили это действие.', + 'conversations.toolFailure.denied.next': + 'Ничего делать не нужно — оно не было выполнено. Попросите снова, если передумаете.', + 'conversations.toolFailure.approvalExpired.cause': + 'Срок запроса на подтверждение истёк, прежде чем кто-либо ответил.', + 'conversations.toolFailure.approvalExpired.next': + 'Попросите снова, чтобы выполнить его — OpenHuman не повторит попытку сам.', 'conversations.toolFailure.unknown.cause': 'С этим действием что-то пошло не так.', 'conversations.toolFailure.unknown.next': 'Повторите попытку; если ошибка повторяется, запустите диагностику в Настройках.', diff --git a/app/src/lib/i18n/zh-CN.ts b/app/src/lib/i18n/zh-CN.ts index 76f99baa4f..e7090310ff 100644 --- a/app/src/lib/i18n/zh-CN.ts +++ b/app/src/lib/i18n/zh-CN.ts @@ -50,6 +50,11 @@ const messages: TranslationMap = { '请检查你的网络连接或模型设置；OpenHuman 将会重试。', 'conversations.toolFailure.timeout.cause': '操作耗时过长，已被停止。', 'conversations.toolFailure.timeout.next': 'OpenHuman 会重试，你也可以手动重试。', + 'conversations.toolFailure.denied.cause': '你拒绝了此操作。', + 'conversations.toolFailure.denied.next': + '无需任何操作——它没有被执行。如果你改变主意，请再次提出。', + 'conversations.toolFailure.approvalExpired.cause': '在有人响应之前，审批请求已过期。', + 'conversations.toolFailure.approvalExpired.next': '再次提出以执行它——OpenHuman 不会自行重试。', 'conversations.toolFailure.unknown.cause': '此操作出现了问题。', 'conversations.toolFailure.unknown.next': '请重试；如果持续失败，请在设置中运行诊断。', 'conversations.backgroundTasks.title': 'Background tasks', diff --git a/app/src/pages/conversations/components/ProcessingTranscriptView.tsx b/app/src/pages/conversations/components/ProcessingTranscriptView.tsx index d82ba2f1f7..7e303f7e63 100644 --- a/app/src/pages/conversations/components/ProcessingTranscriptView.tsx +++ b/app/src/pages/conversations/components/ProcessingTranscriptView.tsx @@ -114,7 +114,7 @@ function ToolGroupBlock({ summary, entries }: { summary: string; entries: ToolTi } /** - * The 8 failure classes the UI has localized copy for (#4254), keyed by the + * The failure classes the UI has localized copy for (#4254 / #4459), keyed by the * camelCase form of the wire's PascalCase `class`. Any class not in this set * falls back to the English `causePlain` / `nextAction` carried on the payload. */ @@ -126,6 +126,8 @@ const LOCALIZED_FAILURE_CLASSES: ReadonlySet = new Set([ 'blockedByPolicy', 'modelConnection', 'timeout', + 'denied', + 'approvalExpired', 'unknown', ]); diff --git a/app/src/store/chatRuntimeSlice.ts b/app/src/store/chatRuntimeSlice.ts index 26b01d77b8..e7ffdf2f03 100644 --- a/app/src/store/chatRuntimeSlice.ts +++ b/app/src/store/chatRuntimeSlice.ts @@ -174,6 +174,9 @@ export interface SubagentToolCallEntry { displayName?: string; /** Server-computed contextual detail (path / recipient / query). */ detail?: string; + /** Plain-language explanation for a FAILED child call (#4459). Mirrors the + * parent {@link ToolTimelineEntry.failure}; absent on successful rows. */ + failure?: ToolFailureExplanation; } /** @@ -677,6 +680,8 @@ function subagentToolCallFromPersisted(call: PersistedSubagentToolCall): Subagen outputChars: call.outputChars, displayName: call.displayName, detail: call.detail, + // Carry the persisted failure explanation across the round-trip (#4459). + failure: parseToolFailure(call.failure), }; } diff --git a/app/src/types/turnState.ts b/app/src/types/turnState.ts index 8cb02c7e8b..8be9848146 100644 --- a/app/src/types/turnState.ts +++ b/app/src/types/turnState.ts @@ -63,6 +63,10 @@ export interface PersistedSubagentToolCall { displayName?: string; /** Server-computed contextual detail (path / recipient / query). */ detail?: string; + /** Plain-language failure explanation for a FAILED child call (#4459). + * Mirrors the parent {@link PersistedToolTimelineEntry.failure}; absent on + * successful rows and on snapshots written before this field. */ + failure?: PersistedToolFailure; } /** diff --git a/src/bin/harness_subagent_audit.rs b/src/bin/harness_subagent_audit.rs index 39dc0d9c2c..53a6320e2d 100644 --- a/src/bin/harness_subagent_audit.rs +++ b/src/bin/harness_subagent_audit.rs @@ -586,6 +586,7 @@ async fn drain_progress( output: _, elapsed_ms, iteration, + failure: _, } => { eprintln!( "[harness_subagent_audit] progress turn={} subagent_tool_completed agent_id={} task_id={} tool={} call_id={} success={} output_chars={} elapsed_ms={} iteration={}", diff --git a/src/core/event_bus/events.rs b/src/core/event_bus/events.rs index fd88f8565b..92246a4453 100644 --- a/src/core/event_bus/events.rs +++ b/src/core/event_bus/events.rs @@ -180,6 +180,20 @@ pub enum DomainEvent { thread_id: String, cancelled_request_id: String, }, + /// One or more queued steer/collect messages were delivered into a running + /// turn's steering handle (the harness applies them at the next iteration + /// checkpoint). Restores the delivery visibility the legacy + /// `RunQueueMessageDelivered` event provided before the TinyAgents migration + /// (issue #4456). `mode` is `"steer"` or `"collect"`. + RunQueueMessageDelivered { + thread_id: String, + mode: String, + delivered: usize, + }, + /// Residual steer messages that the turn ended or was cancelled before + /// applying were drained back into the session run queue so they become the + /// next turn's input instead of silently vanishing (issue #4456). + RunQueueSteerRequeued { thread_id: String, requeued: usize }, // ── Monitor ─────────────────────────────────────────────────────── /// A background monitor changed lifecycle state. @@ -1263,7 +1277,9 @@ impl DomainEvent { | Self::OrchestrationSessionMessage { .. } | Self::RunQueueMessageQueued { .. } | Self::RunQueueFollowupDispatched { .. } - | Self::RunQueueInterrupted { .. } => "agent", + | Self::RunQueueInterrupted { .. } + | Self::RunQueueMessageDelivered { .. } + | Self::RunQueueSteerRequeued { .. } => "agent", Self::MonitorStatusChanged { .. } | Self::MonitorLine { .. } => "monitor", @@ -1427,6 +1443,8 @@ impl DomainEvent { Self::RunQueueMessageQueued { .. } => "RunQueueMessageQueued", Self::RunQueueFollowupDispatched { .. } => "RunQueueFollowupDispatched", Self::RunQueueInterrupted { .. } => "RunQueueInterrupted", + Self::RunQueueMessageDelivered { .. } => "RunQueueMessageDelivered", + Self::RunQueueSteerRequeued { .. } => "RunQueueSteerRequeued", Self::MonitorStatusChanged { .. } => "MonitorStatusChanged", Self::MonitorLine { .. } => "MonitorLine", Self::MemoryStored { .. } => "MemoryStored", @@ -1573,7 +1591,9 @@ impl DomainEvent { Self::WorkspaceViolation { path } => Some(path.as_str()), Self::RunQueueMessageQueued { thread_id, .. } | Self::RunQueueFollowupDispatched { thread_id, .. } - | Self::RunQueueInterrupted { thread_id, .. } => Some(thread_id.as_str()), + | Self::RunQueueInterrupted { thread_id, .. } + | Self::RunQueueMessageDelivered { thread_id, .. } + | Self::RunQueueSteerRequeued { thread_id, .. } => Some(thread_id.as_str()), Self::MonitorStatusChanged { thread_id, .. } | Self::MonitorLine { thread_id, .. } => { thread_id.as_deref() } diff --git a/src/openhuman/agent/README.md b/src/openhuman/agent/README.md index ea386a4b50..eb36cba9ad 100644 --- a/src/openhuman/agent/README.md +++ b/src/openhuman/agent/README.md @@ -9,7 +9,6 @@ Multi-agent orchestration domain. Owns the LLM tool-calling loop, sub-agent disp - `pub fn run_subagent` / `pub struct SubagentRunOptions` / `pub enum SubagentRunError` — `harness/subagent_runner/` — execute a hierarchical sub-agent from a parent tool loop. - `pub struct AgentDefinition` / `pub struct AgentDefinitionRegistry` / `pub enum SandboxMode` / `pub enum ToolScope` — `harness/definition.rs` — sub-agent archetypes loaded from built-ins + workspace TOML. - `pub mod harness::fork_context` — `harness/fork_context.rs` — task-local parent context for KV-cache reuse. -- `pub mod harness::interrupt` (`check_interrupt`, `InterruptFence`, `InterruptedError`) — `harness/interrupt.rs` — graceful cancellation primitives. - `pub trait ToolDispatcher` / `pub struct ParsedToolCall` / `pub struct ToolExecutionResult` — `dispatcher.rs:14-50` — pluggable tool-call format (XML / JSON / P-Format). - `pub mod triage` (`run_triage`, `apply_decision`, `TriggerEnvelope`, `TriageDecision`, `TriageAction`) — `triage/mod.rs:34-45` — classify external triggers, escalate to sub-agents. - `pub mod prompts::SystemPromptBuilder` — `prompts/` — system-prompt section composer. diff --git a/src/openhuman/agent/harness/agent_graph.rs b/src/openhuman/agent/harness/agent_graph.rs index 98147643dd..8951e34561 100644 --- a/src/openhuman/agent/harness/agent_graph.rs +++ b/src/openhuman/agent/harness/agent_graph.rs @@ -62,6 +62,11 @@ pub struct AgentTurnRequest { pub provider_label: String, pub(crate) handoff_cache: Option>, + /// Agent-level TokenJuice compaction profile + /// (`definition.effective_tokenjuice_compression()`), threaded into the + /// sub-agent `TurnContextMiddleware` so tool outputs compact like the chat + /// path instead of taking a blunt byte-cap truncation (#4466). + pub tokenjuice_compression: crate::openhuman::tokenjuice::AgentTokenjuiceCompression, } /// Token/cost totals a custom runner reports back. Mirrors the runner's internal @@ -85,6 +90,9 @@ pub struct AgentTurnResult { pub early_exit_tool: Option, /// `true` when the run stopped at the model-call cap with work still pending. pub hit_cap: bool, + /// Set (with the halt reason) when the repeated-failure / repeat-progress + /// circuit breaker halted the run; the runner reports `Incomplete` (#4466). + pub breaker_halt: Option, } /// A per-agent custom turn-graph runner: given the assembled [`AgentTurnRequest`], diff --git a/src/openhuman/agent/harness/credentials.rs b/src/openhuman/agent/harness/credentials.rs index ab32eb1765..847a114770 100644 --- a/src/openhuman/agent/harness/credentials.rs +++ b/src/openhuman/agent/harness/credentials.rs @@ -1,50 +1,91 @@ use regex::Regex; use std::sync::LazyLock; +/// Key/value credential shapes: `token: "…"`, `api_key=…`, `bearer: …`, etc. static SENSITIVE_KV_REGEX: LazyLock = LazyLock::new(|| { Regex::new(r#"(?i)(token|api[_-]?key|password|secret|user[_-]?key|bearer|credential)["']?\s*[:=]\s*(?:"([^"]{8,})"|'([^']{8,})'|([a-zA-Z0-9_\-\.]{8,}))"#).unwrap() }); +/// Bare AWS access-key IDs — `AKIA…`/`ASIA…` followed by 16 base32 chars — which +/// appear naked in env dumps and config reads with no surrounding key name. +static AWS_ACCESS_KEY_REGEX: LazyLock = + LazyLock::new(|| Regex::new(r"\b((?:AKIA|ASIA)[0-9A-Z]{16})\b").unwrap()); + +/// Bare OpenAI-style secret keys — `sk-…` (incl. `sk-proj-…`) with a long token +/// body. Not necessarily attached to a `key:` label in raw API responses. +static OPENAI_KEY_REGEX: LazyLock = + LazyLock::new(|| Regex::new(r"\b(sk-[A-Za-z0-9_\-]{16,})\b").unwrap()); + +/// Space-separated bearer tokens as they appear in HTTP auth headers +/// (`Authorization: Bearer `) — the KV regex only catches `bearer:`/`=`. +static BEARER_SPACE_REGEX: LazyLock = + LazyLock::new(|| Regex::new(r"(?i)\b(Bearer)\s+([A-Za-z0-9_\-\.=+/]{16,})").unwrap()); + +/// Preserve the first 4 chars of `val` for context, returning the redacted +/// prefix (empty when the value is too short to safely reveal any of it). +fn redact_prefix(val: &str) -> &str { + if val.chars().count() > 4 { + match val.char_indices().nth(4) { + Some((idx, _)) => &val[..idx], + None => val, + } + } else { + "" + } +} + /// Scrub credentials from tool output to prevent accidental exfiltration. /// Replaces known credential patterns with a redacted placeholder while preserving /// a small prefix for context. +/// +/// Covers labelled key/value pairs plus bare secrets that show up unlabelled in +/// env dumps, config reads and API responses: AWS access-key IDs (`AKIA…`/ +/// `ASIA…`), OpenAI-style `sk-…` keys, and space-separated `Bearer ` +/// auth headers. pub(crate) fn scrub_credentials(input: &str) -> String { - SENSITIVE_KV_REGEX - .replace_all(input, |caps: ®ex::Captures| { - let full_match = &caps[0]; - let key = &caps[1]; - let val = caps - .get(2) - .or(caps.get(3)) - .or(caps.get(4)) - .map(|m| m.as_str()) - .unwrap_or(""); + let stage_kv = SENSITIVE_KV_REGEX.replace_all(input, |caps: ®ex::Captures| { + let full_match = &caps[0]; + let key = &caps[1]; + let val = caps + .get(2) + .or(caps.get(3)) + .or(caps.get(4)) + .map(|m| m.as_str()) + .unwrap_or(""); - // Preserve first 4 chars for context, then redact - let prefix = if val.chars().count() > 4 { - match val.char_indices().nth(4) { - Some((idx, _)) => &val[..idx], - None => val, - } - } else { - "" - }; + let prefix = redact_prefix(val); - if full_match.contains(':') { - if full_match.contains('"') { - format!("\"{}\": \"{}*[REDACTED]\"", key, prefix) - } else { - format!("{}: {}*[REDACTED]", key, prefix) - } - } else if full_match.contains('=') { - if full_match.contains('"') { - format!("{}=\"{}*[REDACTED]\"", key, prefix) - } else { - format!("{}={}*[REDACTED]", key, prefix) - } + if full_match.contains(':') { + if full_match.contains('"') { + format!("\"{}\": \"{}*[REDACTED]\"", key, prefix) } else { format!("{}: {}*[REDACTED]", key, prefix) } + } else if full_match.contains('=') { + if full_match.contains('"') { + format!("{}=\"{}*[REDACTED]\"", key, prefix) + } else { + format!("{}={}*[REDACTED]", key, prefix) + } + } else { + format!("{}: {}*[REDACTED]", key, prefix) + } + }); + + // Bare AWS access-key IDs: keep the 4-char `AKIA`/`ASIA` prefix for context. + let stage_aws = AWS_ACCESS_KEY_REGEX.replace_all(&stage_kv, |caps: ®ex::Captures| { + format!("{}*[REDACTED]", redact_prefix(&caps[1])) + }); + + // Bare `sk-…` keys: keep the `sk-` scheme, redact the secret body. + let stage_openai = OPENAI_KEY_REGEX.replace_all(&stage_aws, |_caps: ®ex::Captures| { + "sk-*[REDACTED]".to_string() + }); + + // Space-separated `Bearer `: keep the scheme word, redact the token. + BEARER_SPACE_REGEX + .replace_all(&stage_openai, |caps: ®ex::Captures| { + format!("{} *[REDACTED]", &caps[1]) }) .to_string() } diff --git a/src/openhuman/agent/harness/graph.rs b/src/openhuman/agent/harness/graph.rs index 48727e09ed..ace8c0d506 100644 --- a/src/openhuman/agent/harness/graph.rs +++ b/src/openhuman/agent/harness/graph.rs @@ -55,10 +55,18 @@ pub(crate) async fn run_channel_turn_via_graph( ) -> Result { let extra_arc = Arc::new(extra_tools); - // The callable set is the visibility whitelist (empty = every tool visible - // across the registry + per-turn extras). The runner advertises each via its - // own `spec()`, deduped by name (extras shadow the registry). - let allowed = visible_tool_names.cloned().unwrap_or_default(); + // The callable set is the visibility whitelist. The runner advertises each via + // its own `spec()`, deduped by name (extras shadow the registry). + // Fail-closed allowlist plumbing (issue #4452): the shared seam takes an + // `Option>` where `None` = no filter (all visible tools) and + // `Some(set)` = exactly those tools. The channel/CLI path's historical + // convention is "no filter / empty set = every visible tool", so map both a + // missing filter and an empty set to `None`; only a populated set is treated + // as an explicit whitelist. + let allowed: Option> = match visible_tool_names { + Some(set) if !set.is_empty() => Some(set.clone()), + _ => None, + }; // Capture native-tool support before `provider` is moved into the runner: the // durable history append below serializes this turn's typed suffix with the @@ -132,6 +140,10 @@ pub(crate) async fn run_channel_turn_via_graph( None, // Interactive channel/CLI turn — never serve a cached model response. false, + // #4457 (defect C): the channel/CLI path has no post-run wrap-up and does + // NOT emit `TurnCompleted` itself, so let the seam emit the single + // terminal event (legacy-engine parity). + false, ) .await?; // Append only this turn's typed suffix (assistant tool-calls + tool results + diff --git a/src/openhuman/agent/harness/memory_protocol.rs b/src/openhuman/agent/harness/memory_protocol.rs index 90965afb99..4894cb551b 100644 --- a/src/openhuman/agent/harness/memory_protocol.rs +++ b/src/openhuman/agent/harness/memory_protocol.rs @@ -77,6 +77,17 @@ pub fn classify_memory_op(tool_name: &str, arguments: &serde_json::Value) -> Mem // Durable mutations: create an entry, delete an entry, or ingest a // document into the memory tree (the split-out ingest tool). "memory_store" | "memory_forget" | "memory_tree_ingest_document" => MemoryOp::Write, + // `remember_preference` / `save_preference` (#4458): these DO persist via + // `Memory::store`, but into dedicated preference namespaces + // (`pinned_preferences` / `user_pref_{general,situational}`) that are + // surfaced by direct system-prompt injection or per-query recall — they + // bypass the inference/stability pipeline and are NOT part of the + // `MEMORY.md` curated wiki the archivist reconciles. So they are neither + // an index write that needs a dedupe read nor one that must be closed by + // `update_memory_md`; treating them as `Write` would resurrect the very + // unsatisfiable "call update_memory_md" nag loop this issue removes. + // Classified `Other` deliberately (explicit arm, not fall-through). + "remember_preference" | "save_preference" => MemoryOp::Other, // Consolidated memory_tree tool: `ingest_document` writes; every other // mode is a read-only retrieval. "memory_tree" => match arg_str("mode") { diff --git a/src/openhuman/agent/harness/mod.rs b/src/openhuman/agent/harness/mod.rs index bbed3437cb..da98fdd5ea 100644 --- a/src/openhuman/agent/harness/mod.rs +++ b/src/openhuman/agent/harness/mod.rs @@ -24,7 +24,7 @@ pub mod agent_graph; pub mod archivist; pub(crate) mod builtin_definitions; -mod credentials; +pub(crate) mod credentials; pub mod definition; pub(crate) mod definition_loader; pub mod fork_context; @@ -62,7 +62,7 @@ pub use task_recency_context::{current_task_recency_window, with_task_recency_wi pub(crate) use graph::run_channel_turn_via_graph; pub(crate) use instructions::build_tool_instructions_filtered; -pub(crate) use parse::parse_tool_calls; +pub(crate) use parse::{parse_tool_calls, parse_tool_calls_with_pformat}; #[cfg(test)] mod harness_gap_tests; diff --git a/src/openhuman/agent/harness/parse.rs b/src/openhuman/agent/harness/parse.rs index 73d9232bc1..7f7718a680 100644 --- a/src/openhuman/agent/harness/parse.rs +++ b/src/openhuman/agent/harness/parse.rs @@ -5,7 +5,7 @@ use crate::openhuman::tools::Tool; use regex::Regex; use std::sync::LazyLock; -#[derive(Debug)] +#[derive(Debug, Clone)] pub(crate) struct ParsedToolCall { pub name: String, pub arguments: serde_json::Value, @@ -708,6 +708,105 @@ pub(crate) fn parse_tool_calls(response: &str) -> (String, Vec) (text_parts.join("\n"), calls) } +/// P-Format-aware wrapper over [`parse_tool_calls`] (issue #4465). +/// +/// The migrated tinyagents parse path +/// (`crate::openhuman::tinyagents::model`) kept the XML/JSON/markdown/GLM +/// grammars but dropped the legacy **P-Format** positional grammar +/// (`name[arg1|arg2]`) — even though `PFormat` is the +/// default [`ToolCallFormat`](crate::openhuman::context::prompt::ToolCallFormat) +/// and ~10 builtin agent prompts still *teach* the `name[a|b]` form. A model +/// that followed its own instructions therefore emitted calls that +/// [`parse_tool_calls`] logged as "malformed `` JSON" and silently +/// dropped, so the turn continued as if no tool was called. +/// +/// This restores parity by walking the ``-family tags and, for each +/// tag body, **preferring** the registry-driven P-Format parse +/// ([`pformat::parse_call`](crate::openhuman::agent::pformat::parse_call)) and +/// **falling back** to the JSON entry the canonical parser produced at the same +/// ordinal position — the exact per-tag selection the legacy +/// `PFormatToolDispatcher` performed. This makes it a strict superset of +/// [`parse_tool_calls`]: +/// +/// - An **empty** `registry` (native/JSON agents advertise no positional +/// layout, or no tools at all) short-circuits to [`parse_tool_calls`], so +/// nothing changes for non-PFormat callers. +/// - A tag body that is not a valid `name[...]` positional call (e.g. a JSON +/// `{"name":..}` body, or an unregistered tool name) leaves +/// [`pformat::parse_call`](crate::openhuman::agent::pformat::parse_call) +/// returning `None`, so the canonical JSON entry is used unchanged. +pub(crate) fn parse_tool_calls_with_pformat( + response: &str, + registry: &crate::openhuman::agent::pformat::PFormatRegistry, +) -> (String, Vec) { + // Canonical parse first: narrative text + JSON/XML/markdown/GLM calls. + let (narrative, json_calls) = parse_tool_calls(response); + + // Without a registry there is no positional layout to reconstruct — keep + // the canonical result verbatim (behaviour-neutral for non-PFormat paths). + if registry.is_empty() { + return (narrative, json_calls); + } + + // Walk the tags ourselves, preferring a P-Format body per tag and falling + // back to the JSON entry the canonical parser produced at the same ordinal + // position (both walk the same ordered set of ``-family tags). + let mut combined: Vec = Vec::new(); + let mut json_idx = 0usize; + let mut remaining = response; + + while !remaining.is_empty() { + let Some((open_idx, open_tag)) = find_first_tag(remaining, &TOOL_CALL_OPEN_TAGS) else { + break; + }; + let Some(close_tag) = matching_tool_call_close_tag(open_tag) else { + break; + }; + let after_open = &remaining[open_idx + open_tag.len()..]; + let Some(close_idx) = after_open.find(close_tag) else { + break; + }; + let body = &after_open[..close_idx]; + + if let Some((name, arguments)) = + crate::openhuman::agent::pformat::parse_call(body, registry) + { + // Do NOT log the arguments — a p-format body carries tool arguments + // that may contain user data (bug-report-2026-05-26 A3 parity). + tracing::debug!( + tool = name.as_str(), + "[agent_parse] recovered P-Format tool call (name[arg|arg]) the JSON pass dropped" + ); + combined.push(ParsedToolCall { + name, + arguments, + id: None, + }); + // Do NOT advance `json_idx` here: a P-Format tag is one the JSON pass + // could not parse, so `parse_tool_calls` produced no `json_calls` + // entry for it. Advancing would shift every later JSON tag onto the + // wrong `json_calls` index and silently drop a real JSON call. + } else if let Some(json_call) = json_calls.get(json_idx) { + combined.push(json_call.clone()); + json_idx += 1; + } + + remaining = &after_open[close_idx + close_tag.len()..]; + } + + if combined.is_empty() { + // No `` tag recovered a positional call — the canonical + // result already covers JSON/XML/markdown/GLM grammars. + return (narrative, json_calls); + } + + tracing::debug!( + parsed_tool_calls = combined.len(), + "[agent_parse] P-Format-aware parse produced combined tool-call set" + ); + (narrative, combined) +} + #[cfg(test)] pub(crate) fn parse_structured_tool_calls(tool_calls: &[ToolCall]) -> Vec { tool_calls diff --git a/src/openhuman/agent/harness/session/turn/core.rs b/src/openhuman/agent/harness/session/turn/core.rs index a8dedb3aea..e645c85c14 100644 --- a/src/openhuman/agent/harness/session/turn/core.rs +++ b/src/openhuman/agent/harness/session/turn/core.rs @@ -75,7 +75,13 @@ fn tool_records_from_conversation( if let ConversationMessage::AssistantToolCalls { tool_calls, .. } = msg { for call in tool_calls { let outcome = tool_outcomes.iter().find(|o| o.call_id == call.id); - let success = outcome.map(|o| o.success).unwrap_or(true); + // Default a MISSING outcome to `false` (#4467, item 7): a call + // with no captured outcome is a hallucinated/unknown tool the + // crate recovered via `ReturnToolError` without running + // `after_tool` (so the capture sink never saw it). Recording it as + // succeeded misreports the timeline; real executed tools always + // have an outcome, so this only flips the genuinely-unknown case. + let success = outcome.map(|o| o.success).unwrap_or(false); let output_summary = outcome .map(|o| hooks::sanitize_tool_output(&o.content, &call.name, success)) .unwrap_or_default(); @@ -689,10 +695,13 @@ impl Agent { // read the parent's provider, tools, model, and workspace via // the PARENT_CONTEXT task-local. // Arm the thread-goal budget stop hook for this turn when an active, - // budgeted goal exists — it hard-stops the loop the moment running usage - // would exceed the cap (so an autonomous run can't blow past it between - // accounting points). Merge with any ambient stop hooks rather than - // clobbering them. No budgeted active goal → no extra hook, no wrap. + // budgeted goal exists — it votes to stop the loop as soon as running + // usage would exceed the cap. #4469 item 1: the stop is a graceful pause + // drained at the next iteration boundary, not an instantaneous abort, so + // the current tool round + one wrap-up summary call can still run past the + // cap (a small, bounded overshoot) before the partial transcript returns. + // Merge with any ambient stop hooks rather than clobbering them. No + // budgeted active goal → no extra hook, no wrap. let mut turn_stop_hooks = crate::openhuman::agent::stop_hooks::current_stop_hooks(); if let Some(ref goal) = active_goal { if let Some(hook) = @@ -900,6 +909,9 @@ impl Agent { // Progressive-disclosure handoff is a sub-agent (integrations_agent) // concern; the top-level chat turn never sets it. handoff: None, + // Live transcript snapshotting is a sub-agent error-recovery concern + // (#4466); the chat path persists its transcript post-run. + transcript_snapshot: None, }; // Gather any sub-agent spend delegated during this turn (synchronous @@ -989,6 +1001,28 @@ impl Agent { // A completion with no text and no tool calls is never a valid final // answer — surface it as an error instead of wedging the thread on a // blank reply (bug-report-2026-05-26 A1, defect B). + // + // #4457 (defect A): the empty terminal assistant response was already + // folded into `self.history` via `outcome.conversation` at the + // `history.extend` above (an empty `Chat(assistant(""))`). The #4093 + // branch below pops that dangling blank row before re-prompting, but + // this `tool_calls == 0` path returned the error with the empty row + // still in history — so the *next* request carried an empty-content + // assistant message and strict providers (Anthropic: "text content + // blocks must be non-empty") 400 the whole thread, not just this turn. + // Pop the trailing empty assistant row before returning so a retry + // sends a clean transcript. + if matches!( + self.history.last(), + Some(ConversationMessage::Chat(msg)) + if msg.role == "assistant" && msg.content.trim().is_empty() + ) { + log::debug!( + "[agent_loop] EmptyProviderResponse at iteration {}: popping dangling empty assistant row before returning — #4457 defect A", + outcome.model_calls + ); + self.history.pop(); + } return Err(anyhow::Error::new( crate::openhuman::agent::error::AgentError::EmptyProviderResponse { iteration: outcome.model_calls, @@ -1122,14 +1156,32 @@ impl Agent { .await; // Content (prompt + reply) rides its own event so a tracing consumer can - // attach it to the turn span. Transmission off-device stays gated by the - // exporter's opt-in `capture_content` flag; here it is only surfaced onto - // the in-memory span. - self.emit_progress(AgentProgress::TurnContent { - input: Some(user_message.to_string()), - output: Some(reply.clone()), - }) - .await; + // attach it to the turn span. Gated on the opt-in + // `observability.agent_tracing.capture_content` flag (#4454): with the + // default off, we don't even emit the content event, so prompt/reply text + // never reaches the span store or any exporter. The collector applies the + // same storage-level gate as defense in depth. + let capture_content = self + .integration_runtime_config + .as_ref() + .map(|c| c.observability.agent_tracing.capture_content) + .unwrap_or(false); + if capture_content { + log::debug!( + target: "agent-tracing", + "[agent-tracing] emitting TurnContent (capture_content=true)" + ); + self.emit_progress(AgentProgress::TurnContent { + input: Some(user_message.to_string()), + output: Some(reply.clone()), + }) + .await; + } else { + log::debug!( + target: "agent-tracing", + "[agent-tracing] skipping TurnContent emit (capture_content=false)" + ); + } self.emit_progress(AgentProgress::TurnCompleted { iterations: outcome.model_calls as u32, diff --git a/src/openhuman/agent/harness/session/turn/graph.rs b/src/openhuman/agent/harness/session/turn/graph.rs index fd2e5fde68..11a044d907 100644 --- a/src/openhuman/agent/harness/session/turn/graph.rs +++ b/src/openhuman/agent/harness/session/turn/graph.rs @@ -79,13 +79,23 @@ pub(crate) struct ChatTurnGraph { /// ([`core`](super::core) folds usage, persists the conversation, and handles a /// cap-hit checkpoint). pub(crate) async fn run_chat_turn_graph(graph: ChatTurnGraph) -> Result { + // Fail-closed allowlist plumbing (issue #4452): the shared seam now takes an + // `Option>` where `None` = no filter (all visible tools) and + // `Some(set)` = exactly those tools. The chat path's historical convention is + // "empty `visible_tool_names` = every visible tool", so map an empty set to + // `None` to preserve that behavior; a populated set stays an explicit filter. + let visible_tool_names = if graph.visible_tool_names.is_empty() { + None + } else { + Some(graph.visible_tool_names) + }; run_turn_via_tinyagents_shared( graph.provider, &graph.model, graph.temperature, graph.messages, vec![graph.tools], - graph.visible_tool_names, + visible_tool_names, graph.max_iterations, // Mirror the harness event stream onto this session's progress sink. graph.on_progress, @@ -112,6 +122,12 @@ pub(crate) async fn run_chat_turn_graph(graph: ChatTurnGraph) -> Result Option { pub async fn with_autonomous_iter_cap(cap: usize, fut: F) -> F::Output { AUTONOMOUS_ITER_CAP.scope(cap, fut).await } + +/// Lift a sub-agent's per-agent iteration `base` to the active autonomous cap +/// when one is in scope (issue #4463). +/// +/// Autonomous task/skill runs (`task_dispatcher` / `skill_runtime`) scope an +/// [`with_autonomous_iter_cap`] of `TASK_RUN_MAX_ITERATIONS` / +/// `WORKFLOW_RUN_MAX_ITERATIONS` around the whole tree so an unattended run +/// continues until it's done or a circuit breaker trips, rather than stopping at +/// a specialist sub-agent's normal cap (e.g. 10). The migration to the tinyagents +/// harness dropped every reader of [`autonomous_iter_cap`], so those setters +/// became dead knobs and sub-agents silently reverted to the normal cap. This is +/// the restored reader: sub-agent iteration computations run their +/// `effective_max_iterations()` through it so the lift takes effect again. The +/// cost budget + repeated-failure breakers remain the primary runaway guards. +pub fn subagent_iter_cap_with_autonomous_lift(base: usize) -> usize { + match autonomous_iter_cap() { + Some(cap) if cap > base => { + tracing::debug!( + base, + autonomous_cap = cap, + "[subagent_runner:autonomous] lifting sub-agent iteration cap for autonomous run" + ); + cap + } + _ => base, + } +} diff --git a/src/openhuman/agent/harness/subagent_runner/mod.rs b/src/openhuman/agent/harness/subagent_runner/mod.rs index 79007b2301..50be61cfb8 100644 --- a/src/openhuman/agent/harness/subagent_runner/mod.rs +++ b/src/openhuman/agent/harness/subagent_runner/mod.rs @@ -46,7 +46,9 @@ mod tool_prep; mod types; // Public API — the entry point and the shapes it returns. -pub use autonomous::{autonomous_iter_cap, with_autonomous_iter_cap}; +pub use autonomous::{ + autonomous_iter_cap, subagent_iter_cap_with_autonomous_lift, with_autonomous_iter_cap, +}; pub use ops::run_subagent; pub use types::{ SubagentCheckpointData, SubagentMode, SubagentRunError, SubagentRunOptions, SubagentRunOutcome, diff --git a/src/openhuman/agent/harness/subagent_runner/ops/graph.rs b/src/openhuman/agent/harness/subagent_runner/ops/graph.rs index 1a1dcf6a4d..44e5ca9842 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops/graph.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops/graph.rs @@ -37,6 +37,7 @@ use crate::openhuman::agent::harness::subagent_runner::types::SubagentRunError; use crate::openhuman::agent::progress::AgentProgress; use crate::openhuman::inference::provider::{ChatMessage, ConversationMessage, Provider}; use crate::openhuman::tinyagents::{run_turn_via_tinyagents_shared, SubagentScope}; +use crate::openhuman::tokenjuice::AgentTokenjuiceCompression; use crate::openhuman::tools::{Tool, ToolSpec}; use tinyagents::harness::workspace::WorkspaceDescriptor; @@ -80,33 +81,36 @@ pub(crate) async fn run_agent_turn_request_via_default_graph( transcript_stem, provider_label, handoff_cache, + tokenjuice_compression, } = req; - let (output, iterations, usage, early_exit_tool, hit_cap) = run_subagent_via_graph( - provider, - &model, - temperature, - &mut history, - parent_tools, - dynamic_tools, - specs, - allowed_names, - max_iterations, - run_queue, - on_progress, - &agent_id, - &task_id, - extended_policy, - worker_thread_id, - workspace_dir, - workspace_descriptor, - max_output_tokens, - model_vision, - &transcript_stem, - &provider_label, - handoff_cache, - ) - .await?; + let (output, iterations, usage, early_exit_tool, hit_cap, breaker_halt) = + run_subagent_via_graph( + provider, + &model, + temperature, + &mut history, + parent_tools, + dynamic_tools, + specs, + allowed_names, + max_iterations, + run_queue, + on_progress, + &agent_id, + &task_id, + extended_policy, + worker_thread_id, + workspace_dir, + workspace_descriptor, + max_output_tokens, + model_vision, + &transcript_stem, + &provider_label, + handoff_cache, + tokenjuice_compression, + ) + .await?; Ok(AgentTurnResult { history, @@ -120,6 +124,7 @@ pub(crate) async fn run_agent_turn_request_via_default_graph( }, early_exit_tool, hit_cap, + breaker_halt, }) } @@ -161,7 +166,25 @@ pub(super) async fn run_subagent_via_graph( handoff_cache: Option< std::sync::Arc, >, -) -> Result<(String, usize, AggregatedUsage, Option, bool), SubagentRunError> { + // Agent-level TokenJuice profile (`definition.effective_tokenjuice_compression()`, + // #4466). Threaded into the sub-agent `TurnContextMiddleware` so sub-agent + // tool outputs get the same content-aware compaction the chat path applies + // instead of a blunt byte-cap truncation. + tokenjuice_compression: AgentTokenjuiceCompression, +) -> Result< + ( + String, + usize, + AggregatedUsage, + Option, + bool, + // Breaker-halt reason (#4466): `Some` when the repeated-failure / + // repeat-progress circuit breaker stopped the run; the caller reports + // `Incomplete` instead of `Completed`. + Option, + ), + SubagentRunError, +> { tracing::info!( model, max_iterations, @@ -212,6 +235,25 @@ pub(super) async fn run_subagent_via_graph( // call rather than relying solely on the parent's one-time trim. let context_window = provider.effective_context_window(model).await; + // Build the sub-agent's context middleware from the live `[context]` config + + // the agent's TokenJuice profile (#4466), matching how the chat path wires + // `TurnContextMiddleware` (session/turn/core.rs). The migrated sub-agent path + // had regressed to `TurnContextMiddleware::defaults()` — compression Off — so + // sub-agent tool outputs took a blunt 16 KiB truncation instead of the + // content-aware TokenJuice compaction the definition asked for. Honor the + // `[context]` enabled / autocompact opt-outs, microcompact keep-recent, and + // per-result byte budget too, so a sub-agent turn compacts like a chat turn. + let context_mw = build_subagent_context_mw(tokenjuice_compression).await; + + // Live transcript snapshot sink (#4466): the harness owns the working message + // vector and drops it on a mid-run `Err`, so a failed sub-agent run used to + // persist NOTHING (breaking `learning/transcript_ingest`) and leave an empty + // worker thread. Attach a snapshot middleware that mirrors each `before_model` + // request's transcript here, so the error path below can still persist the + // rounds that completed before the failure. + let transcript_snapshot: crate::openhuman::tinyagents::TranscriptSnapshotSink = + std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + // A sub-agent turn runs *nested inside* the parent agent's turn (parent // harness → spawn_subagent tool → here), so the child's full // `run_turn_via_tinyagents_shared` future would otherwise sit on the parent's @@ -220,7 +262,7 @@ pub(super) async fn run_subagent_via_graph( // Capture native-tool support before `provider` is moved: the durable-history // append below serializes this turn's typed suffix with the matching dispatcher. let native_tools = provider.supports_native_tools(); - let mut outcome = Box::pin(run_turn_via_tinyagents_shared( + let run_result = Box::pin(run_turn_via_tinyagents_shared( provider, model, temperature, @@ -232,7 +274,13 @@ pub(super) async fn run_subagent_via_graph( // adapter resolves a name by scanning the sets in order, so a // parent-first order would run the parent impl for a shadowed name. vec![Arc::new(dynamic_tools), parent_tools], - allowed_names, + // Fail-closed (issue #4452): a sub-agent ALWAYS carries a concrete, + // resolved allowlist (`allowed_names`), so pass it as `Some(..)`. An empty + // set is therefore a genuine deny-all — a tool-less agent + // (`ToolScope::Named([])`), a zero-match `skill_filter`, or a `named` list + // that resolved to nothing registers ZERO tools instead of implicitly + // inheriting the parent's full surface (shell/file-write/spawn). + Some(allowed_names), max_iterations, // Parent's progress sink — child events ride it, scoped below. on_progress, @@ -248,11 +296,12 @@ pub(super) async fn run_subagent_via_graph( true, // Bound the sub-agent's per-call output at its configured budget. Some(max_output_tokens), - // Context middlewares: cache-align + default tool-result byte cap so a - // sub-agent's (often large) tool outputs stay bounded in its transcript, - // plus the progressive-disclosure handoff when a cache is attached. + // Context middlewares (#4466): config-sourced TokenJuice compaction + + // tool-result byte cap + microcompact + summarization opt-outs (built + // above), plus the progressive-disclosure handoff when a cache is + // attached, plus the live transcript-snapshot sink for error recovery. { - let mut mw = crate::openhuman::tinyagents::TurnContextMiddleware::defaults(); + let mut mw = context_mw; if let Some(cache) = handoff_cache { mw.handoff = Some(crate::openhuman::tinyagents::HandoffConfig { cache, @@ -260,6 +309,7 @@ pub(super) async fn run_subagent_via_graph( task_id: task_id.to_string(), }); } + mw.transcript_snapshot = Some(transcript_snapshot.clone()); mw }, // Sub-agents gate via their own SubagentToolSource policy path, not the @@ -270,9 +320,50 @@ pub(super) async fn run_subagent_via_graph( // Sub-agent turns run tools with external effects; not a deterministic // internal run, so response caching stays off (safe default). false, + // #4457 (defect C): irrelevant for sub-agents — they carry a + // `subagent_scope`, so the seam never emits a top-level `TurnCompleted` + // (they report via `Subagent*` events). Pass `false` for clarity. + false, )) - .await - .map_err(map_tinyagents_subagent_error)?; + .await; + + let mut outcome = match run_result { + Ok(outcome) => outcome, + Err(err) => { + // #4466: the harness dropped its partial transcript, but the snapshot + // middleware mirrored every completed round. Persist those rounds to + // `session_raw` (so `learning/transcript_ingest` can still read a + // failed run) and mirror them onto the worker thread, THEN surface the + // error. Previously the `?`-return skipped both persistence steps, so + // a failed run left no transcript and an empty worker thread. + let mapped = map_tinyagents_subagent_error(err); + let recovered = transcript_snapshot + .lock() + .map(|g| g.clone()) + .unwrap_or_default(); + tracing::warn!( + agent_id, + task_id, + error = %mapped, + recovered_rounds = recovered.len(), + "[subagent_runner:graph] sub-agent run errored; persisting recovered transcript before returning (#4466)" + ); + persist_failed_run( + &workspace_dir, + transcript_stem, + agent_id, + task_id, + provider_label, + model, + &recovered, + context_window.unwrap_or(0), + if native_tools { "native" } else { "xml" }, + worker_thread_id.as_deref(), + &mapped, + ); + return Err(mapped); + } + }; // Write the final conversation back so the caller can checkpoint / persist. // Keep the original (un-expanded) prior turns and append only this turn's typed @@ -307,21 +398,65 @@ pub(super) async fn run_subagent_via_graph( // checkpoint (the delegating agent continues from partial progress) rather // than surfacing an empty/partial answer — the legacy `SubagentCheckpoint`. if outcome.hit_cap { - let digest = build_cap_digest(&outcome.conversation); + let digest = build_cap_digest(&outcome.conversation, &outcome.tool_outcomes); let strategy = super::checkpoint::SubagentCheckpoint { provider: summary_provider.as_ref(), model: model.to_string(), temperature, agent_id: agent_id.to_string(), - // The checkpoint summary call's output cap — the standard per-turn - // budget (the value this field replaced when it was hardcoded). - max_output_tokens: crate::openhuman::inference::provider::AGENT_TURN_MAX_OUTPUT_TOKENS, + // The checkpoint summary call's output cap. #4469 item 5: honour this + // sub-agent definition's own per-call output budget (the same + // `max_output_tokens` bounding every task model call above) instead of + // the process-global `AGENT_TURN_MAX_OUTPUT_TOKENS` floor, so a + // definition that raised or lowered its output cap is respected by the + // cap-summary call too. + max_output_tokens, }; match strategy.summarize_cap_hit(&digest, max_iterations).await { Ok(co) => { if let Some(u) = co.usage { + // Fold ALL four token fields (the legacy cap-summary folded + // cached tokens too, not just input/output), then price the + // call and feed the global cost tracker directly (#4467, + // item 2). The checkpoint summary call bypasses the harness so + // the observability bridge never sees it — without this record + // its cached tokens are lost and it costs $0 in the footer / + // transcript meta / cost dashboard. usage.input_tokens += u.input_tokens; usage.output_tokens += u.output_tokens; + usage.cached_input_tokens += u.cached_input_tokens; + let call_cost = + if u.charged_amount_usd.is_finite() && u.charged_amount_usd > 0.0 { + u.charged_amount_usd + } else { + crate::openhuman::cost::catalog::estimate_cost_usd( + model, + u.input_tokens, + u.output_tokens, + u.cached_input_tokens, + ) + }; + usage.charged_amount_usd += call_cost; + crate::openhuman::cost::record_provider_usage( + model, + &crate::openhuman::inference::provider::UsageInfo { + input_tokens: u.input_tokens, + output_tokens: u.output_tokens, + context_window: u.context_window, + cached_input_tokens: u.cached_input_tokens, + cache_creation_tokens: u.cache_creation_tokens, + reasoning_tokens: u.reasoning_tokens, + charged_amount_usd: call_cost, + }, + ); + tracing::debug!( + agent_id, + input_tokens = u.input_tokens, + output_tokens = u.output_tokens, + cached_input_tokens = u.cached_input_tokens, + call_cost, + "[subagent] cap-hit summary call folded + priced + recorded into cost tracker (#4467, item 2)" + ); } outcome.text = co.text; } @@ -398,9 +533,60 @@ pub(super) async fn run_subagent_via_graph( usage, outcome.early_exit_tool, outcome.hit_cap, + // #4466: propagate a circuit-breaker halt so the runner reports Incomplete. + outcome.breaker_halt, )) } +/// Build the sub-agent turn's [`TurnContextMiddleware`] from the live +/// `[context]` config and the agent's TokenJuice profile (#4466), mirroring the +/// chat path (`session/turn/core.rs`). Falls back to +/// [`TurnContextMiddleware::defaults`] when the config can't be loaded so a +/// config glitch degrades to the safe (byte-cap-only) behavior rather than +/// erroring the run. +async fn build_subagent_context_mw( + tokenjuice_compression: AgentTokenjuiceCompression, +) -> crate::openhuman::tinyagents::TurnContextMiddleware { + let mut mw = crate::openhuman::tinyagents::TurnContextMiddleware::defaults(); + // Always thread the agent's compression profile — even on the config-default + // path — so the definition's TokenJuice choice is honored. + mw.tokenjuice_compression = tokenjuice_compression; + match crate::openhuman::config::Config::load_or_init().await { + Ok(config) => { + let ctx = &config.context; + // TokenJuice content-aware compaction gates on the same master + // `[context].compaction_enabled` the chat path reads + // (`ContextManager::compaction_enabled`). + mw.tokenjuice_compaction_enabled = ctx.compaction_enabled; + mw.tool_result_budget_bytes = ctx.tool_result_budget_bytes; + // Microcompact keep-recent is `0` (disabled) unless microcompact is on. + mw.microcompact_keep_recent = if ctx.microcompact_enabled { + ctx.microcompact_keep_recent + } else { + 0 + }; + // Summarization step honors the `[context].enabled` + autocompact + // opt-outs, same as `ContextManager::autocompact_enabled`. + mw.autocompact_enabled = ctx.enabled && ctx.autocompact_enabled; + tracing::debug!( + tokenjuice_compaction_enabled = mw.tokenjuice_compaction_enabled, + compression = ?mw.tokenjuice_compression, + tool_result_budget_bytes = mw.tool_result_budget_bytes, + microcompact_keep_recent = mw.microcompact_keep_recent, + autocompact_enabled = mw.autocompact_enabled, + "[subagent_runner:graph] built sub-agent context middleware from config (#4466)" + ); + } + Err(err) => { + tracing::debug!( + error = %err, + "[subagent_runner:graph] config load failed building sub-agent context mw; using defaults + compression profile" + ); + } + } + mw +} + fn map_tinyagents_subagent_error(err: anyhow::Error) -> SubagentRunError { match err.downcast::() { Ok(run_err) => run_err, @@ -481,11 +667,117 @@ fn persist_subagent_transcript( } } +/// Persist a **failed** sub-agent run (#4466): write whatever rounds the live +/// transcript-snapshot middleware captured before the harness error to +/// `session_raw` (so `learning/transcript_ingest` can still ingest a failed run, +/// not skip an absent file), mirror those rounds onto the worker thread, and +/// append a trailing failure marker so the record is self-describing. Usage is +/// zeroed — the harness reported no totals on the error path — and the iteration +/// count is the number of completed rounds recovered. +#[allow(clippy::too_many_arguments)] +fn persist_failed_run( + workspace_dir: &std::path::Path, + transcript_stem: &str, + agent_id: &str, + task_id: &str, + provider_label: &str, + model: &str, + recovered: &[ChatMessage], + context_window: u64, + dispatcher: &str, + worker_thread_id: Option<&str>, + error: &SubagentRunError, +) { + let marker = format!("[subagent run failed before completion: {error}]"); + let mut history = recovered.to_vec(); + history.push(ChatMessage::assistant(marker.clone())); + + // A failed run has no usage totals; record zeros so the transcript is still a + // valid, ingestable `session_raw` record with the failure surfaced. + let usage = AggregatedUsage::default(); + persist_subagent_transcript( + workspace_dir, + transcript_stem, + agent_id, + task_id, + provider_label, + model, + &history, + &usage, + context_window, + dispatcher, + recovered.len() as u32, + ); + + if let Some(thread_id) = worker_thread_id { + mirror_worker_thread_from_history( + workspace_dir, + thread_id, + agent_id, + task_id, + recovered, + Some(marker.as_str()), + ); + } +} + +/// Append a worker-thread [`StoredMessage`](crate::openhuman::memory_conversations::ConversationMessage) +/// with the restored legacy [`SubagentObserver`] metadata (#4466): `scope`, +/// `agent_id`, `task_id`, plus the per-message `iteration`, `final`, `mode`, and +/// (for assistant tool rounds / tool results) `tool_calls` / `tool_call_id` / +/// `tool_name`. The migrated path had reduced this to `{scope, agent_id, +/// task_id}` only, dropping the fields worker-thread consumers key on. +#[allow(clippy::too_many_arguments)] +fn append_worker_message( + workspace_dir: &std::path::Path, + thread_id: &str, + agent_id: &str, + task_id: &str, + content: String, + sender: &str, + metadata: serde_json::Value, +) { + use crate::openhuman::memory_conversations::{ + append_message, ConversationMessage as StoredMessage, + }; + let mut extra = serde_json::json!({ + "scope": "worker_thread", + "agent_id": agent_id, + "task_id": task_id, + "mode": "typed", + }); + if let (Some(base), Some(extra_fields)) = (extra.as_object_mut(), metadata.as_object()) { + for (k, v) in extra_fields { + base.insert(k.clone(), v.clone()); + } + } + let message = StoredMessage { + id: format!("{sender}:{}", uuid::Uuid::new_v4()), + content, + message_type: "text".to_string(), + extra_metadata: extra, + sender: sender.to_string(), + created_at: chrono::Utc::now().to_rfc3339(), + }; + if let Err(err) = append_message(workspace_dir.to_path_buf(), thread_id, message) { + tracing::debug!( + agent_id, + thread_id, + error = %err, + "[subagent_runner:graph] failed to append worker-thread message" + ); + } +} + /// Mirror a sub-agent turn's structured conversation to its worker thread, /// matching the legacy [`SubagentObserver`]: assistant turns (intents + final) /// become `agent` messages, tool results become `user` messages. `extra_final`, /// when set, is appended as a trailing `agent` message (the cap checkpoint or /// clarifying question, which isn't a plain assistant turn in the transcript). +/// +/// Each message carries the restored legacy metadata (#4466): a per-round +/// `iteration` counter, `final` on the trailing message, `tool_calls` on an +/// assistant round, and `tool_call_id` / `tool_name` on each tool result. fn mirror_worker_thread( workspace_dir: &std::path::Path, thread_id: &str, @@ -494,48 +786,80 @@ fn mirror_worker_thread( conversation: &[ConversationMessage], extra_final: Option<&str>, ) { - use crate::openhuman::memory_conversations::{ - append_message, ConversationMessage as StoredMessage, - }; + use std::collections::HashMap; - let append = |content: String, sender: &str| { - let message = StoredMessage { - id: format!("{sender}:{}", uuid::Uuid::new_v4()), - content, - message_type: "text".to_string(), - extra_metadata: serde_json::json!({ - "scope": "worker_thread", - "agent_id": agent_id, - "task_id": task_id, - }), - sender: sender.to_string(), - created_at: chrono::Utc::now().to_rfc3339(), - }; - if let Err(err) = append_message(workspace_dir.to_path_buf(), thread_id, message) { - tracing::debug!( - agent_id, - thread_id, - error = %err, - "[subagent_runner:graph] failed to append worker-thread message" - ); + // call_id -> tool name, so each tool result records the tool it came from. + let mut names: HashMap<&str, &str> = HashMap::new(); + for msg in conversation { + if let ConversationMessage::AssistantToolCalls { tool_calls, .. } = msg { + for call in tool_calls { + names.insert(call.id.as_str(), call.name.as_str()); + } } - }; + } + let mut iteration: u64 = 0; for msg in conversation { match msg { - ConversationMessage::AssistantToolCalls { text, .. } => { + ConversationMessage::AssistantToolCalls { + text, tool_calls, .. + } => { + iteration += 1; if let Some(t) = text.as_deref().filter(|t| !t.trim().is_empty()) { - append(t.to_string(), "agent"); + let call_names: Vec<&str> = + tool_calls.iter().map(|c| c.name.as_str()).collect(); + append_worker_message( + workspace_dir, + thread_id, + agent_id, + task_id, + t.to_string(), + "agent", + serde_json::json!({ + "iteration": iteration, + "final": false, + "tool_calls": call_names, + }), + ); } } ConversationMessage::ToolResults(results) => { for r in results { - append(r.content.clone(), "user"); + let tool_name = names + .get(r.tool_call_id.as_str()) + .copied() + .unwrap_or("tool"); + append_worker_message( + workspace_dir, + thread_id, + agent_id, + task_id, + r.content.clone(), + "user", + serde_json::json!({ + "iteration": iteration, + "final": false, + "tool_call_id": r.tool_call_id, + "tool_name": tool_name, + }), + ); } } ConversationMessage::Chat(c) if c.role == "assistant" => { if !c.content.trim().is_empty() { - append(c.content.clone(), "agent"); + iteration += 1; + append_worker_message( + workspace_dir, + thread_id, + agent_id, + task_id, + c.content.clone(), + "agent", + serde_json::json!({ + "iteration": iteration, + "final": extra_final.is_none(), + }), + ); } } _ => {} @@ -543,15 +867,85 @@ fn mirror_worker_thread( } if let Some(text) = extra_final.filter(|t| !t.trim().is_empty()) { - append(text.to_string(), "agent"); + append_worker_message( + workspace_dir, + thread_id, + agent_id, + task_id, + text.to_string(), + "agent", + serde_json::json!({ "iteration": iteration + 1, "final": true }), + ); + } +} + +/// Worker-thread mirror from a flat [`ChatMessage`] history (the error-recovery +/// path, #4466): assistant messages become `agent` rows, tool messages become +/// `user` rows. Used when only the recovered snapshot (not the typed +/// `conversation`) is available. `failure_final`, when set, is appended as a +/// trailing `agent` failure marker. +fn mirror_worker_thread_from_history( + workspace_dir: &std::path::Path, + thread_id: &str, + agent_id: &str, + task_id: &str, + history: &[ChatMessage], + failure_final: Option<&str>, +) { + let mut iteration: u64 = 0; + for m in history { + match m.role.as_str() { + "assistant" if !m.content.trim().is_empty() => { + iteration += 1; + append_worker_message( + workspace_dir, + thread_id, + agent_id, + task_id, + m.content.clone(), + "agent", + serde_json::json!({ "iteration": iteration, "final": false }), + ); + } + "tool" if !m.content.trim().is_empty() => { + append_worker_message( + workspace_dir, + thread_id, + agent_id, + task_id, + m.content.clone(), + "user", + serde_json::json!({ "iteration": iteration, "final": false }), + ); + } + _ => {} + } + } + if let Some(text) = failure_final.filter(|t| !t.trim().is_empty()) { + append_worker_message( + workspace_dir, + thread_id, + agent_id, + task_id, + text.to_string(), + "agent", + serde_json::json!({ "iteration": iteration + 1, "final": true }), + ); } } /// Build the `tool → outcome` digest the cap-hit summary call summarizes, in the /// legacy `- {name} [{ok|failed}]: {output}` format (engine `run_tool_digest`), -/// pairing each tool result back to its call by id. Tool success isn't carried -/// on the converted transcript, so results are reported optimistically as `ok`. -fn build_cap_digest(conversation: &[ConversationMessage]) -> String { +/// pairing each tool result back to its call by id. Per-tool success is derived +/// from the turn's captured [`ToolCallOutcome`]s (#4467, item 7) rather than +/// reported optimistically as `ok`: a result whose call has no captured outcome +/// — e.g. a hallucinated/unknown tool the crate recovered without running +/// `after_tool` — is marked `failed`, so the summary no longer tells the model +/// every call succeeded. +fn build_cap_digest( + conversation: &[ConversationMessage], + tool_outcomes: &[crate::openhuman::tinyagents::ToolCallOutcome], +) -> String { use std::collections::HashMap; use std::fmt::Write as _; @@ -565,6 +959,12 @@ fn build_cap_digest(conversation: &[ConversationMessage]) -> String { } } + // call_id -> success, from the captured per-call outcomes. + let success_by_id: HashMap<&str, bool> = tool_outcomes + .iter() + .map(|o| (o.call_id.as_str(), o.success)) + .collect(); + let mut out = String::new(); for msg in conversation { if let ConversationMessage::ToolResults(results) = msg { @@ -573,8 +973,15 @@ fn build_cap_digest(conversation: &[ConversationMessage]) -> String { .get(r.tool_call_id.as_str()) .copied() .unwrap_or("tool"); + // Missing outcome → `false` (unknown/hallucinated tool): honest + // failed status rather than an optimistic `[ok]`. + let ok = success_by_id + .get(r.tool_call_id.as_str()) + .copied() + .unwrap_or(false); + let tag = if ok { "ok" } else { "failed" }; let body = crate::openhuman::util::truncate_with_ellipsis(&r.content, 800); - let _ = writeln!(out, "- {name} [ok]: {body}"); + let _ = writeln!(out, "- {name} [{tag}]: {body}"); } } } @@ -660,7 +1067,7 @@ mod tests { allowed.insert("echo".to_string()); let mut history = vec![ChatMessage::user("please echo hi")]; - let (output, iterations, usage, early_exit, hit_cap) = run_subagent_via_graph( + let (output, iterations, usage, early_exit, hit_cap, _breaker) = run_subagent_via_graph( provider, "mock-model", 0.0, @@ -683,6 +1090,7 @@ mod tests { "root-session__real_tools", "mock-channel", None, + AgentTokenjuiceCompression::Off, ) .await .expect("graph subagent runs"); @@ -748,7 +1156,7 @@ mod tests { let parent_tools: Arc>> = Arc::new(vec![]); let mut history = vec![ChatMessage::user("hi")]; - let (output, _iters, _usage, _early, _hit_cap) = run_subagent_via_graph( + let (output, _iters, _usage, _early, _hit_cap, _breaker) = run_subagent_via_graph( Arc::new(ThinkingStreamProvider), "mock-model", 0.0, @@ -771,6 +1179,7 @@ mod tests { "root-session__scoped_deltas", "mock-channel", None, + AgentTokenjuiceCompression::Off, ) .await .expect("child-delta subagent runs"); @@ -894,7 +1303,7 @@ mod tests { allowed.insert("ask_user_clarification".to_string()); let mut history = vec![ChatMessage::user("help me")]; - let (output, iterations, _usage, early_exit, _hit_cap) = run_subagent_via_graph( + let (output, iterations, _usage, early_exit, _hit_cap, _breaker) = run_subagent_via_graph( provider.clone(), "mock-model", 0.0, @@ -917,6 +1326,7 @@ mod tests { "root-session__clarification", "mock-channel", None, + AgentTokenjuiceCompression::Off, ) .await .expect("ask-clarification subagent runs"); @@ -1000,7 +1410,7 @@ mod tests { allowed.insert("noop".to_string()); let mut history = vec![ChatMessage::user("do a big task")]; - let (output, iterations, _usage, early_exit, hit_cap) = run_subagent_via_graph( + let (output, iterations, _usage, early_exit, hit_cap, _breaker) = run_subagent_via_graph( Arc::new(LoopForeverProvider), "mock-model", 0.0, @@ -1023,6 +1433,7 @@ mod tests { "root-session__cap_hit", "mock-channel", None, + AgentTokenjuiceCompression::Off, ) .await .expect("cap-hit subagent runs"); diff --git a/src/openhuman/agent/harness/subagent_runner/ops/runner.rs b/src/openhuman/agent/harness/subagent_runner/ops/runner.rs index 90ba65e31c..e74202c756 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops/runner.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops/runner.rs @@ -20,6 +20,7 @@ use crate::openhuman::agent::harness::fork_context::{ }; use crate::openhuman::agent::harness::subagent_runner::extract_tool::ExtractFromResultTool; use crate::openhuman::agent::harness::subagent_runner::handoff::ResultHandoffCache; +use crate::openhuman::agent::harness::subagent_runner::subagent_iter_cap_with_autonomous_lift; use crate::openhuman::agent::harness::subagent_runner::tool_prep::{ build_text_mode_tool_instructions, filter_tool_indices, is_subagent_spawn_tool, load_prompt_source, top_k_for_toolkit, @@ -704,7 +705,7 @@ async fn run_typed_mode( agent_id = %definition.id, model = %model, tool_count = allowed_names.len(), - max_iterations = definition.effective_max_iterations(), + max_iterations = subagent_iter_cap_with_autonomous_lift(definition.effective_max_iterations()), iteration_policy = ?definition.iteration_policy, "[subagent_runner:typed] resolved configuration" ); @@ -958,87 +959,95 @@ async fn run_typed_mode( ); } - let (output, iterations, agg_usage, early_exit_tool, hit_cap) = match &definition.graph { - AgentGraph::Default => { - super::graph::run_subagent_via_graph( - subagent_provider.clone(), - &model, - temperature, - &mut history, - parent.all_tools.clone(), - dynamic_tools, - filtered_specs.clone(), - allowed_names, - definition.effective_max_iterations(), - options.run_queue.clone(), - parent.on_progress.clone(), - &definition.id, - task_id, - definition.iteration_policy == IterationPolicy::Extended, - options.worker_thread_id.clone(), - parent.workspace_dir.clone(), - workspace_descriptor.clone(), - max_output_tokens, - model_vision, - &transcript_stem, - // Sub-agent turns record their provider label as the literal - // "subagent" (parity with the legacy observer's TurnObserver - // provenance), distinguishing delegated spend from the parent's - // own channel in per-thread usage reads. - "subagent", - // Progressive-disclosure handoff cache (shared with the - // extract_from_result tool registered above). - handoff_cache.clone(), - ) - .await? - } - AgentGraph::Custom(run) => { - let req = AgentTurnRequest { - provider: subagent_provider.clone(), - model: model.clone(), - temperature, - history: std::mem::take(&mut history), - parent_tools: parent.all_tools.clone(), - dynamic_tools, - specs: filtered_specs.clone(), - allowed_names, - max_iterations: definition.effective_max_iterations(), - run_queue: options.run_queue.clone(), - on_progress: parent.on_progress.clone(), - agent_id: definition.id.clone(), - task_id: task_id.to_string(), - extended_policy: definition.iteration_policy == IterationPolicy::Extended, - worker_thread_id: options.worker_thread_id.clone(), - workspace_dir: parent.workspace_dir.clone(), - workspace_descriptor: workspace_descriptor.clone(), - max_output_tokens, - model_vision, - transcript_stem: transcript_stem.clone(), - provider_label: "subagent".to_string(), - handoff_cache: handoff_cache.clone(), - }; - let res = run(req).await?; - history = res.history; - let AgentTurnUsage { - input_tokens, - output_tokens, - cached_input_tokens, - charged_amount_usd, - } = res.usage; - ( - res.output, - res.iterations, - AggregatedUsage { + let (output, iterations, agg_usage, early_exit_tool, hit_cap, breaker_halt) = + match &definition.graph { + AgentGraph::Default => { + super::graph::run_subagent_via_graph( + subagent_provider.clone(), + &model, + temperature, + &mut history, + parent.all_tools.clone(), + dynamic_tools, + filtered_specs.clone(), + allowed_names, + subagent_iter_cap_with_autonomous_lift(definition.effective_max_iterations()), + options.run_queue.clone(), + parent.on_progress.clone(), + &definition.id, + task_id, + definition.iteration_policy == IterationPolicy::Extended, + options.worker_thread_id.clone(), + parent.workspace_dir.clone(), + workspace_descriptor.clone(), + max_output_tokens, + model_vision, + &transcript_stem, + // Sub-agent turns record their provider label as the literal + // "subagent" (parity with the legacy observer's TurnObserver + // provenance), distinguishing delegated spend from the parent's + // own channel in per-thread usage reads. + "subagent", + // Progressive-disclosure handoff cache (shared with the + // extract_from_result tool registered above). + handoff_cache.clone(), + // Agent-level TokenJuice profile → sub-agent context middleware + // (#4466), so sub-agent tool outputs compact like the chat path. + definition.effective_tokenjuice_compression(), + ) + .await? + } + AgentGraph::Custom(run) => { + let req = AgentTurnRequest { + provider: subagent_provider.clone(), + model: model.clone(), + temperature, + history: std::mem::take(&mut history), + parent_tools: parent.all_tools.clone(), + dynamic_tools, + specs: filtered_specs.clone(), + allowed_names, + max_iterations: subagent_iter_cap_with_autonomous_lift( + definition.effective_max_iterations(), + ), + run_queue: options.run_queue.clone(), + on_progress: parent.on_progress.clone(), + agent_id: definition.id.clone(), + task_id: task_id.to_string(), + extended_policy: definition.iteration_policy == IterationPolicy::Extended, + worker_thread_id: options.worker_thread_id.clone(), + workspace_dir: parent.workspace_dir.clone(), + workspace_descriptor: workspace_descriptor.clone(), + max_output_tokens, + model_vision, + transcript_stem: transcript_stem.clone(), + provider_label: "subagent".to_string(), + handoff_cache: handoff_cache.clone(), + tokenjuice_compression: definition.effective_tokenjuice_compression(), + }; + let res = run(req).await?; + history = res.history; + let AgentTurnUsage { input_tokens, output_tokens, cached_input_tokens, charged_amount_usd, - }, - res.early_exit_tool, - res.hit_cap, - ) - } - }; + } = res.usage; + ( + res.output, + res.iterations, + AggregatedUsage { + input_tokens, + output_tokens, + cached_input_tokens, + charged_amount_usd, + }, + res.early_exit_tool, + res.hit_cap, + res.breaker_halt, + ) + } + }; // Determine status: if the turn engine exited early because of // ask_user_clarification, checkpoint the history and return @@ -1104,6 +1113,22 @@ async fn run_typed_mode( question, options: options_vec, } + } else if let Some(reason) = breaker_halt { + // The repeated-failure / repeat-progress circuit breaker halted the run + // (#4466). It is NOT a clean finish: `output` carries the breaker's + // root-cause summary, not a completed answer. Surface `Incomplete` with + // the halt reason so a delegating parent relays the blocker instead of + // treating the halted child as finished (the migrated path reported + // `hit_cap=false` → `Completed`, hiding the halt). + tracing::warn!( + task_id = %task_id, + agent_id = %definition.id, + reason = %reason, + "[subagent_runner] child halted by circuit breaker; reporting Incomplete (#4466)" + ); + crate::openhuman::agent::harness::subagent_runner::types::SubagentRunStatus::Incomplete { + reason, + } } else if hit_cap { // The tinyagents run stopped at the model-call cap with work still // pending (graph summarized a resumable checkpoint into `output`). @@ -1206,6 +1231,31 @@ impl crate::openhuman::inference::provider::Provider for TextModeProvider { self.inner.effective_context_window(model).await } + // #4469 item 2: forward the local-provider identity + cache passthroughs. This + // decorator only masks native tool calling (above); everything about *where* + // and *how* the inner provider runs must pass through unchanged. Without these + // the default trait impls report the inner as a remote, non-caching provider, + // so a local runtime behind text mode loses its `n_keep >= n_ctx` un-evictable + // prefix guard (`is_local_provider*` / `loaded_context_window`, #3550) and its + // KV-cache pricing/strategy (`prompt_cache_capabilities`, #3939). + fn is_local_provider(&self) -> bool { + self.inner.is_local_provider() + } + + fn is_local_provider_for_model(&self, model: &str) -> bool { + self.inner.is_local_provider_for_model(model) + } + + async fn loaded_context_window(&self, model: &str) -> Option { + self.inner.loaded_context_window(model).await + } + + fn prompt_cache_capabilities( + &self, + ) -> crate::openhuman::inference::provider::traits::PromptCacheCapabilities { + self.inner.prompt_cache_capabilities() + } + async fn warmup(&self) -> anyhow::Result<()> { self.inner.warmup().await } diff --git a/src/openhuman/agent/harness/tool_result_artifacts/mod.rs b/src/openhuman/agent/harness/tool_result_artifacts/mod.rs index 9c163f493f..502794ef9a 100644 --- a/src/openhuman/agent/harness/tool_result_artifacts/mod.rs +++ b/src/openhuman/agent/harness/tool_result_artifacts/mod.rs @@ -17,6 +17,17 @@ use tinyagents::harness::store::Store; const ARTIFACT_ROOT: &str = "artifacts/tool-results"; const AGGREGATE_PREVIEW_BUDGET_BYTES: usize = 512; +/// #4469 item 6: floor for how tightly a persisted `[tool_result_preview]` +/// envelope may be bounded during aggregate spill. `allowed_len` can saturate to +/// `0` (or a handful of bytes) once earlier-spilled results have already consumed +/// the aggregate budget; bounding the envelope to that would return `""` — or a +/// header cut mid-line — discarding the `artifact_path` pointer the model needs +/// to `file_read` the full output. This floor keeps the envelope header (through +/// the `artifact_path` / `read_with` lines) intact even when the raw budget math +/// says zero; `apply_tool_result_budget` retains the head, so the pointer always +/// survives. Slightly overshooting the aggregate budget here is the correct +/// trade — a valid pointer is worth a few hundred bytes. +const MIN_ENVELOPE_ALLOWANCE_BYTES: usize = 512; pub(crate) const TINYAGENTS_TOOL_RESULT_ARTIFACT_STORE: &str = "openhuman_tool_result_artifacts"; const TRAILER_RESERVED: usize = 256; @@ -264,8 +275,13 @@ pub(crate) async fn apply_per_result_persistence( Ok(persisted) => { let (output, final_bytes) = bound_text_to_budget(persisted.output, budget_bytes); if final_bytes >= original_bytes { + // #4469 item 9: this branch does NOT fall back to inline + // truncation — the envelope is returned regardless, because it + // carries the `artifact_path` pointer to the full stored output + // (worth keeping even when the preview text nets no byte saving + // vs. the raw result). Log it as an observation only. log::debug!( - "[agent][tool-result-artifacts] persisted envelope too large tool={} original_bytes={} final_bytes={} budget_bytes={} -- falling back to inline truncation", + "[agent][tool-result-artifacts] persisted envelope not smaller than raw result tool={} original_bytes={} final_bytes={} budget_bytes={} -- keeping envelope for its artifact_path pointer", tool_name, original_bytes, final_bytes, @@ -362,7 +378,14 @@ pub(crate) async fn spill_aggregate_tool_results( }; match persisted_output { Ok(persisted) => { - let (output, final_bytes) = bound_text_to_budget(persisted.output, allowed_len); + // #4469 item 6: never bound the preview envelope below the minimum + // that preserves its `[tool_result_preview]` header + artifact + // pointer — `allowed_len` can be 0 here, which would blank the + // result and strip the `artifact_path` the model reads to recover + // the full output. + let envelope_allowance = allowed_len.max(MIN_ENVELOPE_ALLOWANCE_BYTES); + let (output, final_bytes) = + bound_text_to_budget(persisted.output, envelope_allowance); total = total .saturating_sub(original_len) .saturating_add(final_bytes); @@ -579,7 +602,21 @@ mod tests { spill_aggregate_tool_results(&mut results, Some(&store), 500).await; let total: usize = results.iter().map(|result| result.output.len()).sum(); - assert!(total <= 500, "total={total}"); + // #4469 item 6: the aggregate spill now floors each persisted envelope at + // MIN_ENVELOPE_ALLOWANCE_BYTES so the `[tool_result_preview]` header + + // `artifact_path` pointer always survives (previously an exhausted budget + // could blank a result to ""). That is a documented trade — the total may + // slightly overshoot the raw aggregate budget — so the invariant is now: + // (a) no envelope is blanked, and (b) the total stays bounded by the + // per-result floor rather than the raw budget. + assert!( + results.iter().all(|result| !result.output.is_empty()), + "no persisted envelope may be blanked — the artifact pointer must survive" + ); + assert!( + total <= results.len() * MIN_ENVELOPE_ALLOWANCE_BYTES, + "total={total} exceeds the per-result envelope floor bound" + ); assert!(tmp .path() .join("artifacts/tool-results/session/one/one.txt") diff --git a/src/openhuman/agent/harness/turn_attachments_context.rs b/src/openhuman/agent/harness/turn_attachments_context.rs index 5b984bb819..7939a9bc65 100644 --- a/src/openhuman/agent/harness/turn_attachments_context.rs +++ b/src/openhuman/agent/harness/turn_attachments_context.rs @@ -12,7 +12,7 @@ //! them to the sub-agent prompt so the (vision-capable) sub-agent's turn //! rehydrates the image from the on-disk sidecar. //! -//! Mirrors [`super::model_vision_context`]. Scoped around the orchestrator's +//! A task-local carrier scoped around the orchestrator's //! turn future (`run_turn_via_tinyagents_shared`); //! [`current_turn_image_placeholders`] returns an empty vec when no scope is //! active (CLI / direct invocation / tests) — strictly additive. diff --git a/src/openhuman/agent/progress.rs b/src/openhuman/agent/progress.rs index a019e0d2a7..a45cb9a596 100644 --- a/src/openhuman/agent/progress.rs +++ b/src/openhuman/agent/progress.rs @@ -190,6 +190,11 @@ pub enum AgentProgress { elapsed_ms: u64, /// 1-based child iteration index. iteration: u32, + /// Present when `success` is false: a user-facing classification of the + /// child tool failure, mirroring [`Self::ToolCallCompleted::failure`] so + /// a failed sub-agent row carries the same "why + what to do next" copy + /// instead of discarding the already-computed classification (#4459). + failure: Option, }, /// A chunk of a sub-agent's visible assistant text arrived from the diff --git a/src/openhuman/agent/progress_tracing.rs b/src/openhuman/agent/progress_tracing.rs index eb99953a90..4681418274 100644 --- a/src/openhuman/agent/progress_tracing.rs +++ b/src/openhuman/agent/progress_tracing.rs @@ -25,14 +25,22 @@ //! //! Spans always carry *metadata* — span names, counts, timings, and //! token/cost figures. While `observability.agent_tracing.capture_content` is -//! on (its default), the turn's prompt/reply and **truncated** tool -//! arguments/results are additionally recorded as span `input`/`output`; -//! with the flag off, none of that content ever reaches the in-memory span. +//! on, the turn's prompt/reply and **truncated** tool arguments/results are +//! additionally recorded as span `input`/`output`; with the flag off (the +//! default — #4454), none of that content ever reaches the in-memory span, so +//! no exporter (NDJSON file, app log, or Langfuse) can leak it. //! Streamed text/thinking deltas (`TextDelta`, `ThinkingDelta`, //! `ToolCallArgsDelta`), raw error strings, and filesystem paths are **never** //! recorded regardless of the flag, honoring the project's "never log secrets //! or full PII" rule for logs. //! +//! The one exception is the turn's prompt/reply, delivered via +//! `AgentProgress::TurnContent`. It is attached to the turn span **only** when +//! the operator opts in via `observability.agent_tracing.capture_content` +//! (default `false`). That gate is enforced at storage time in +//! [`SpanCollector`] — the single choke point — so with the default off, no +//! exporter (NDJSON file, app log, or Langfuse push) can ever serialize it. +//! //! ## Wiring //! //! [`SpanCollector`] is a pure state machine: feed it the progress events plus @@ -226,7 +234,16 @@ pub enum SpanStatus { } /// A single finished (or in-flight) span. Field names follow OpenTelemetry -/// conventions so the NDJSON drops cleanly into an OTel/Langfuse importer. +/// conventions (snake_case `trace_id`/`span_id`/`start_unix_ms`/…) so the raw +/// NDJSON file/log export is a self-describing OTel-style span dump for local +/// inspection. +/// +/// #4469 item 13: this raw record is **not** directly Langfuse-ingestible — the +/// Langfuse `/api/public/ingestion` API needs each span wrapped in a +/// `{ type, id, timestamp, body }` event envelope. That envelope is produced +/// only by [`langfuse::spans_to_langfuse_batch`] on the remote-push path; the +/// local NDJSON exporter intentionally emits the raw spans, not the batch +/// format. #[derive(Debug, Clone, Serialize)] pub struct TraceSpan { /// Trace id (the session id) — shared by every span in the run. @@ -249,12 +266,13 @@ pub struct TraceSpan { pub status: SpanStatus, /// Metadata-only attributes (no secrets/PII). pub attributes: BTreeMap, - /// Optional prompt/input content. Populated only when content capture is on - /// (via `AgentProgress::TurnContent`); the exporter still gates transmission - /// behind `observability.agent_tracing.capture_content`. + /// Optional prompt/input content. Populated (via `AgentProgress::TurnContent`) + /// **only** when `observability.agent_tracing.capture_content` is opted in — + /// the [`SpanCollector`] drops content at storage time otherwise, so with the + /// default gate off this is always `None` and no exporter can serialize it. #[serde(skip_serializing_if = "Option::is_none")] pub input: Option, - /// Optional model-reply/output content. Same capture/gating rules as + /// Optional model-reply/output content. Same storage-level gating as /// [`Self::input`]. #[serde(skip_serializing_if = "Option::is_none")] pub output: Option, @@ -297,6 +315,15 @@ pub struct SpanCollector { /// fresh nonce makes every span id globally unique. id_prefix: String, + /// Storage-level privacy gate (mirrors + /// `observability.agent_tracing.capture_content`). When `false` (the + /// default), prompt/reply content from [`AgentProgress::TurnContent`] is + /// **never attached to a span** — so no exporter (NDJSON file, app log, or + /// Langfuse push) can ever serialize it. This is the single choke point + /// referenced in the module docs: gating at storage protects every present + /// and future exporter, not just the transmission path. + capture_content: bool, + turn_span_id: Option, turn_span_index: Option, current_iteration_span_id: Option, @@ -315,6 +342,8 @@ impl SpanCollector { spans: Vec::new(), next_span_seq: 0, id_prefix: uuid::Uuid::new_v4().simple().to_string(), + // Metadata-only by default; opt in via `with_content_capture`. + capture_content: false, turn_span_id: None, turn_span_index: None, current_iteration_span_id: None, @@ -324,6 +353,15 @@ impl SpanCollector { } } + /// Opt into attaching prompt/reply content to spans (from + /// [`AgentProgress::TurnContent`]). Wire this to + /// `observability.agent_tracing.capture_content`. Left off, content is + /// dropped at storage time so it can never reach any exporter. + pub fn with_content_capture(mut self, capture_content: bool) -> Self { + self.capture_content = capture_content; + self + } + /// All spans recorded so far (finished and in-flight). pub fn spans(&self) -> &[TraceSpan] { &self.spans @@ -990,8 +1028,19 @@ impl SpanCollector { } AgentProgress::TurnContent { input, output } => { - // Attach prompt/reply to the root turn span. Held in-memory only; - // the exporter decides whether to transmit it (opt-in gate). + // Storage-level privacy gate (#4454): prompt/reply text is + // attached to the span ONLY when content capture is opted in. + // With the gate off (default), the content is dropped here so no + // exporter — NDJSON file, app log, or Langfuse push — can ever + // serialize it. This is the single choke point; the exporters + // deliberately do not re-check the flag. + if !self.capture_content { + log::debug!( + target: "agent-tracing", + "[agent-tracing] TurnContent dropped at storage (capture_content=false)" + ); + return; + } let index = match self.turn_span_index { Some(idx) => idx, None => { @@ -1006,6 +1055,10 @@ impl SpanCollector { if let Some(text) = output { span.output = Some(serde_json::Value::String(text.clone())); } + log::debug!( + target: "agent-tracing", + "[agent-tracing] TurnContent attached to turn span (capture_content=true)" + ); } } @@ -1161,12 +1214,22 @@ pub(crate) fn export_spans(config: &AgentTracingConfig, spans: &[TraceSpan]) { } } None => { - // No path configured — surface to the log so the export still works - // on read-only / sandboxed deployments. + // No path configured. Surface only metadata (count + trace id) at + // `info` so the export is visible on read-only / sandboxed + // deployments WITHOUT ever printing span content at `info` (#4454). + // The NDJSON body — which may carry prompt/reply text when + // `capture_content` is opted in — goes to `debug` only. With the + // default gate off, the storage layer already strips content, so + // `payload` is metadata-only regardless. log::info!( - "[agent-tracing] {} spans (trace_id={}):\n{}", + "[agent-tracing] {} spans (trace_id={}) — set observability.agent_tracing.export_path to persist", spans.len(), spans.first().map(|s| s.trace_id.as_str()).unwrap_or(""), + ); + log::debug!( + target: "agent-tracing", + "[agent-tracing] span NDJSON ({} spans):\n{}", + spans.len(), payload.trim_end() ); } diff --git a/src/openhuman/agent/progress_tracing/langfuse.rs b/src/openhuman/agent/progress_tracing/langfuse.rs index 9df931260b..e669cab7e6 100644 --- a/src/openhuman/agent/progress_tracing/langfuse.rs +++ b/src/openhuman/agent/progress_tracing/langfuse.rs @@ -259,16 +259,22 @@ fn apply_usage_fields(body: &mut Value, span: &TraceSpan) -> bool { } let input = input.unwrap_or(0); let output = output.unwrap_or(0); + let cached = attrs + .get("gen_ai.usage.cached_input_tokens") + .and_then(Value::as_u64) + .unwrap_or(0); + // #4454: `input_tokens` is INCLUSIVE of cached prompt tokens (cost.rs treats + // cached as a subset of input). Langfuse sums `usageDetails` components as + // disjoint buckets, so emit the NON-cached input (input - cached) — the + // components (non_cached_input + cache_read + output) are then disjoint and + // reconcile to `total` = input_tokens + output_tokens. + let non_cached_input = input.saturating_sub(cached); let mut usage = Map::new(); - usage.insert("input".to_string(), json!(input)); + usage.insert("input".to_string(), json!(non_cached_input)); usage.insert("output".to_string(), json!(output)); usage.insert("total".to_string(), json!(input.saturating_add(output))); // Cache reads always flow into usageDetails (0 included) so the figure is // explicit rather than absent when no cache was hit. - let cached = attrs - .get("gen_ai.usage.cached_input_tokens") - .and_then(Value::as_u64) - .unwrap_or(0); usage.insert("cache_read_input_tokens".to_string(), json!(cached)); // Reasoning + cache-write tokens ride along whenever the span carries them // (the collector stamps them when > 0). Langfuse accepts arbitrary diff --git a/src/openhuman/agent/progress_tracing/tests.rs b/src/openhuman/agent/progress_tracing/tests.rs index 32911d1935..fbb8c4ab88 100644 --- a/src/openhuman/agent/progress_tracing/tests.rs +++ b/src/openhuman/agent/progress_tracing/tests.rs @@ -288,6 +288,7 @@ fn subagent_lifecycle_nests_under_the_turn() { output: "file contents".to_string(), elapsed_ms: 40, iteration: 1, + failure: None, }, 30, ), @@ -663,22 +664,22 @@ async fn export_run_trace_otel_backend_uses_local_sink() { // ── Route A: content + grouping + span-id uniqueness ──────────────────────── #[test] -fn turn_content_attaches_input_output_to_turn_span() { - let c = collect(&[ - (AgentProgress::TurnStarted, 1_000), - ( - AgentProgress::TurnContent { - input: Some("what is your favorite color?".to_string()), - output: Some("i'm partial to teal".to_string()), - }, - 1_100, - ), - ]); +fn turn_content_attaches_input_output_to_turn_span_when_capture_enabled() { + // Content lands on the span ONLY when capture is opted in (#4454). + let mut c = SpanCollector::new(ctx()).with_content_capture(true); + c.record(&AgentProgress::TurnStarted, 1_000); + c.record( + &AgentProgress::TurnContent { + input: Some("what is your favorite color?".to_string()), + output: Some("i'm partial to teal".to_string()), + }, + 1_100, + ); let turn = find(c.spans(), "agent.turn"); assert_eq!( turn.input.as_ref().and_then(|v| v.as_str()), Some("what is your favorite color?"), - "TurnContent input must land on the turn span" + "TurnContent input must land on the turn span when capture_content=true" ); assert_eq!( turn.output.as_ref().and_then(|v| v.as_str()), @@ -791,6 +792,7 @@ fn tool_io_is_captured_when_capture_content_is_on() { output: "file contents".to_string(), elapsed_ms: 4, iteration: 1, + failure: None, }, 4, ); @@ -838,6 +840,7 @@ fn tool_io_is_never_recorded_when_capture_content_is_off() { output: "sekrit".to_string(), elapsed_ms: 4, iteration: 1, + failure: None, }, 4, ), diff --git a/src/openhuman/channels/providers/web/progress_bridge.rs b/src/openhuman/channels/providers/web/progress_bridge.rs index 75dec45f02..3071197a93 100644 --- a/src/openhuman/channels/providers/web/progress_bridge.rs +++ b/src/openhuman/channels/providers/web/progress_bridge.rs @@ -184,6 +184,10 @@ pub(crate) fn spawn_progress_bridge( .source .clone() .unwrap_or_else(|| "chat".to_string()); + // Storage-level privacy gate (#4454): capture_content (off by + // default) rides on the TraceContext so the collector only attaches + // prompt/reply content to spans when the operator opted in — no + // exporter can serialize prompt/reply text otherwise. let capture_content = config.observability.agent_tracing.capture_content; log::debug!( "[web_channel][bridge] trace context trace_id={} user_attributed={} \ @@ -896,7 +900,12 @@ pub(crate) fn spawn_progress_bridge( output, elapsed_ms, iteration, + failure, } => { + // Serialize the classified failure (if any) so a failed + // sub-agent tool row carries its "why + next" copy on the + // wire + ledger, matching the main-agent path (#4459). + let failure_json = failure.as_ref().and_then(|f| serde_json::to_value(f).ok()); ledger_append_event( &config, RunEventAppend { @@ -909,7 +918,8 @@ pub(crate) fn spawn_progress_bridge( "success": success, "outputChars": output_chars, "elapsedMs": elapsed_ms, - "iteration": iteration + "iteration": iteration, + "failure": failure_json, }), }, ); @@ -928,6 +938,7 @@ pub(crate) fn spawn_progress_bridge( // bounded size for the wire (#4007); `output_chars` + // `elapsed_ms` still ride along in `subagent` below. output: Some(cap_wire_output(output)), + failure: failure_json, subagent: Some(SubagentProgressDetail { child_iteration: Some(iteration), agent_id: Some(agent_id), diff --git a/src/openhuman/inference/provider/mod.rs b/src/openhuman/inference/provider/mod.rs index 44af2dcae7..02c55698a5 100644 --- a/src/openhuman/inference/provider/mod.rs +++ b/src/openhuman/inference/provider/mod.rs @@ -48,6 +48,6 @@ pub use error_code::{ pub use factory::{create_chat_provider, provider_for_role, BYOK_INCOMPLETE_SENTINEL}; pub use ops::*; pub use resolved_route::{ - current_resolved_provider_route, record_resolved_provider_route, - with_resolved_provider_route_scope, ResolvedProviderRoute, + current_resolved_provider_route, current_route_slot, record_resolved_provider_route, + with_resolved_provider_route_scope, with_route_slot, ResolvedProviderRoute, RouteSlot, }; diff --git a/src/openhuman/inference/provider/resolved_route.rs b/src/openhuman/inference/provider/resolved_route.rs index 93cbb527f7..ba368f036b 100644 --- a/src/openhuman/inference/provider/resolved_route.rs +++ b/src/openhuman/inference/provider/resolved_route.rs @@ -13,7 +13,12 @@ pub struct ResolvedProviderRoute { pub model: String, } -type RouteSlot = Arc>>; +/// The ambient audit slot shared by an enclosing +/// [`with_resolved_provider_route_scope`]. Exposed so a detached provider call +/// (issue #4460: streamed calls run in a `tokio::spawn`) can capture the caller's +/// slot before crossing the spawn boundary and re-establish it inside, so its +/// `record_resolved_provider_route` writes still land in the caller's scope. +pub type RouteSlot = Arc>>; tokio::task_local! { static RESOLVED_PROVIDER_ROUTE: RouteSlot; @@ -51,6 +56,28 @@ pub fn record_resolved_provider_route(provider: impl Into, model: impl I ); } +/// Clone the ambient audit slot handle (the `Arc`, not the recorded route) set +/// by an enclosing [`with_resolved_provider_route_scope`]. `None` outside a +/// scope. Capture this **before** a `tokio::spawn` so the detached task can +/// re-establish the same slot via [`with_route_slot`] and have its route writes +/// reach the caller's scope — task-locals do not propagate across `spawn` +/// (issue #4460). +pub fn current_route_slot() -> Option { + RESOLVED_PROVIDER_ROUTE.try_with(|slot| slot.clone()).ok() +} + +/// Re-establish a [`current_route_slot`]-captured `slot` as the ambient +/// resolved-route task-local for `future`. Any `record_resolved_provider_route` +/// call inside `future` then writes back to the original caller's audit slot, +/// even when `future` runs in a detached `tokio::spawn` (issue #4460). +pub async fn with_route_slot(slot: RouteSlot, future: F) -> F::Output +where + F: std::future::Future, +{ + tracing::trace!("[provider] resolved-route slot re-established across spawn boundary"); + RESOLVED_PROVIDER_ROUTE.scope(slot, Box::pin(future)).await +} + pub fn current_resolved_provider_route() -> Option { let route = RESOLVED_PROVIDER_ROUTE .try_with(|slot| slot.lock().unwrap_or_else(|e| e.into_inner()).clone()) diff --git a/src/openhuman/thread_goals/runtime.rs b/src/openhuman/thread_goals/runtime.rs index 24833420b0..91fa53831d 100644 --- a/src/openhuman/thread_goals/runtime.rs +++ b/src/openhuman/thread_goals/runtime.rs @@ -11,9 +11,15 @@ //! - [`account_turn_against_goal`] folds a completed turn's token + time usage //! into the active goal, flipping it to `budget_limited` when the cap is //! crossed. -//! - [`GoalBudgetStopHook`] hard-stops an in-flight turn the moment an *active* -//! goal's running usage would exceed its budget, so an autonomous run can't -//! blow past the ceiling between accounting points. +//! - [`GoalBudgetStopHook`] votes to stop an in-flight turn as soon as an +//! *active* goal's running usage would exceed its budget. #4469 item 1: the +//! stop is a graceful *pause*, not an instantaneous abort — the vote fires in +//! the stop-hook middleware's `after_model`, and the harness drains the pause +//! at the **top of the next iteration**, so the tool round for the model call +//! that tripped the budget still runs and the turn's wrap-up summary may spend +//! one more model call before the partial transcript is returned. It bounds +//! an autonomous run to a small, deterministic overshoot past the ceiling +//! rather than a hard cut at the exact accounting point. use std::path::{Path, PathBuf}; diff --git a/src/openhuman/threads/turn_state/mirror.rs b/src/openhuman/threads/turn_state/mirror.rs index 2699b61f1d..b1eb0d7dd0 100644 --- a/src/openhuman/threads/turn_state/mirror.rs +++ b/src/openhuman/threads/turn_state/mirror.rs @@ -18,8 +18,8 @@ use crate::openhuman::agent::progress::AgentProgress; use super::store::TurnStateStore; use super::types::{ - SubagentActivity, SubagentToolCall, SubagentTranscriptItem, ToolTimelineEntry, - ToolTimelineStatus, TranscriptItem, TurnLifecycle, TurnPhase, TurnState, + PersistedToolFailure, SubagentActivity, SubagentToolCall, SubagentTranscriptItem, + ToolTimelineEntry, ToolTimelineStatus, TranscriptItem, TurnLifecycle, TurnPhase, TurnState, }; const MIRROR_LOG_PREFIX: &str = "[threads:turn_state:mirror]"; @@ -130,13 +130,17 @@ impl TurnStateMirror { detail: display_detail.clone(), source_tool_name: None, subagent: None, + failure: None, }); } self.flush(); true } AgentProgress::ToolCallCompleted { - call_id, success, .. + call_id, + success, + failure, + .. } => { if let Some(entry) = self .state @@ -150,6 +154,10 @@ impl TurnStateMirror { } else { ToolTimelineStatus::Error }; + // Persist the plain-language failure so the explanation + // survives a thread switch / cold boot (#4459). Clear it on + // a (re-)success so a retried row doesn't keep stale copy. + entry.failure = failure.as_ref().map(PersistedToolFailure::from); } if self.state.active_tool.is_some() { self.state.active_tool = None; @@ -193,6 +201,7 @@ impl TurnStateMirror { tool_calls: Vec::new(), transcript: Vec::new(), }), + failure: None, }); self.flush(); true @@ -269,6 +278,7 @@ impl TurnStateMirror { output_chars: None, display_name: display_label.clone(), detail: display_detail.clone(), + failure: None, }); // Mirror the call into the ordered transcript so the // rehydrated thoughts interleave it at the right spot. @@ -296,6 +306,7 @@ impl TurnStateMirror { success, output_chars, elapsed_ms, + failure, .. } => { if let Some(entry) = self.find_subagent_entry_mut(task_id) { @@ -305,6 +316,7 @@ impl TurnStateMirror { } else { ToolTimelineStatus::Error }; + let persisted_failure = failure.as_ref().map(PersistedToolFailure::from); if let Some(call) = activity .tool_calls .iter_mut() @@ -314,6 +326,9 @@ impl TurnStateMirror { call.status = status; call.elapsed_ms = Some(*elapsed_ms); call.output_chars = Some(*output_chars); + // Carry the child failure so a failed sub-agent row + // keeps its explanation across a round-trip (#4459). + call.failure = persisted_failure; } // Keep the transcript's Tool item in lockstep so the // rehydrated row shows the terminal status + timing. @@ -399,6 +414,7 @@ impl TurnStateMirror { detail: None, source_tool_name: None, subagent: None, + failure: None, }); } false diff --git a/src/openhuman/threads/turn_state/mirror_tests.rs b/src/openhuman/threads/turn_state/mirror_tests.rs index 363e9b3225..04ab9d6e80 100644 --- a/src/openhuman/threads/turn_state/mirror_tests.rs +++ b/src/openhuman/threads/turn_state/mirror_tests.rs @@ -411,6 +411,7 @@ fn subagent_transcript_persists_interleaved_prose_and_tools() { output: String::new(), elapsed_ms: 12, iteration: 1, + failure: None, }); let activity = m.snapshot().tool_timeline[0] diff --git a/src/openhuman/threads/turn_state/store_tests.rs b/src/openhuman/threads/turn_state/store_tests.rs index 191edfe9b5..c4e8475fa7 100644 --- a/src/openhuman/threads/turn_state/store_tests.rs +++ b/src/openhuman/threads/turn_state/store_tests.rs @@ -28,6 +28,7 @@ fn put_then_get_roundtrips_state() { detail: None, source_tool_name: None, subagent: None, + failure: None, }); store.put(&state).expect("put"); diff --git a/src/openhuman/threads/turn_state/types.rs b/src/openhuman/threads/turn_state/types.rs index 91741c0d32..9b2367a34b 100644 --- a/src/openhuman/threads/turn_state/types.rs +++ b/src/openhuman/threads/turn_state/types.rs @@ -48,6 +48,50 @@ pub enum ToolTimelineStatus { Error, } +/// Persisted, plain-language explanation of a FAILED tool row (#4459). +/// +/// Mirrors the live socket `failure` object and the frontend +/// `PersistedToolFailure` (`app/src/types/turnState.ts`) 1:1 — camelCase on the +/// wire, `class`/`category` as the taxonomy's stable variant names — so a +/// settled/reloaded turn keeps its "why + what to do next" copy across a thread +/// switch or a cold boot. Absent on successful rows and on snapshots written +/// before this field. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PersistedToolFailure { + /// Stable failure-class variant name, e.g. `"Timeout"`, `"Denied"`. + pub class: String, + /// Stable category variant name, e.g. `"Recoverable"`, `"UserDeclined"`. + pub category: String, + /// Whether the core considers the failure automatically recoverable. + pub recoverable: bool, + /// Plain-language cause (`causePlain` on the wire). + pub cause_plain: String, + /// Plain-language next action (`nextAction` on the wire). + pub next_action: String, +} + +impl From<&crate::openhuman::tool_status::ClassifiedFailure> for PersistedToolFailure { + fn from(f: &crate::openhuman::tool_status::ClassifiedFailure) -> Self { + // Serialize the enums to their wire variant name so the persisted + // `class`/`category` strings match exactly what the live socket emits + // (`ClassifiedFailure` serializes each as its bare variant name). + fn variant_name(v: &T) -> String { + serde_json::to_value(v) + .ok() + .and_then(|j| j.as_str().map(str::to_string)) + .unwrap_or_default() + } + Self { + class: variant_name(&f.class), + category: variant_name(&f.category), + recoverable: f.recoverable, + cause_plain: f.cause_plain.clone(), + next_action: f.next_action.clone(), + } + } +} + /// One row in the per-turn tool timeline. /// /// Field names use camelCase on the wire so a snapshot can be applied @@ -70,6 +114,11 @@ pub struct ToolTimelineEntry { pub source_tool_name: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub subagent: Option, + /// Plain-language failure explanation for a FAILED row, carried in the + /// snapshot so it survives a thread switch / cold boot (#4459). `None` on + /// success and on legacy snapshots. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub failure: Option, } /// Live sub-agent activity nested under a `subagent:*` timeline row. @@ -133,6 +182,12 @@ pub struct SubagentToolCall { /// Server-computed contextual detail (e.g. the path / recipient). #[serde(default, skip_serializing_if = "Option::is_none")] pub detail: Option, + /// Plain-language failure explanation for a FAILED child call, so a + /// sub-agent's failed row carries the same "why + next" copy as a + /// main-agent row and it survives a snapshot round-trip (#4459). `None` on + /// success and on legacy snapshots. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub failure: Option, } /// One ordered item in a sub-agent's processing transcript — its streamed diff --git a/src/openhuman/tinyagents/abort_guard.rs b/src/openhuman/tinyagents/abort_guard.rs new file mode 100644 index 0000000000..71c9f78f7d --- /dev/null +++ b/src/openhuman/tinyagents/abort_guard.rs @@ -0,0 +1,41 @@ +//! RAII guard that aborts a spawned task when it is dropped (issue #4460). +//! +//! `ProviderModel::stream` runs the provider call in a detached `tokio::spawn` +//! producer. Without a lifetime tie, a hard turn cancellation (`AbortHandle`) +//! drops the consumer stream but leaves the producer running to completion — the +//! provider call still finishes and is still billed. Holding the producer's +//! [`JoinHandle`] inside this guard, and moving the guard into the consumer +//! stream's state, ties the two lifetimes: dropping the stream (turn future +//! aborted/dropped) drops the guard, which aborts the in-flight provider call. + +use tokio::task::JoinHandle; + +/// Aborts the wrapped task on drop unless it has already finished. +pub(super) struct AbortOnDrop { + handle: JoinHandle<()>, + /// Grep-friendly label for the abort debug log (e.g. the model name). + label: String, +} + +impl AbortOnDrop { + pub(super) fn new(handle: JoinHandle<()>, label: impl Into) -> Self { + Self { + handle, + label: label.into(), + } + } +} + +impl Drop for AbortOnDrop { + fn drop(&mut self) { + if self.handle.is_finished() { + // Producer already emitted its terminal item — nothing to abort. + return; + } + tracing::debug!( + label = %self.label, + "[tinyagents] aborting in-flight provider stream producer on drop (turn cancelled) — #4460" + ); + self.handle.abort(); + } +} diff --git a/src/openhuman/tinyagents/convert.rs b/src/openhuman/tinyagents/convert.rs index 61ed9a371a..bf2cddb2db 100644 --- a/src/openhuman/tinyagents/convert.rs +++ b/src/openhuman/tinyagents/convert.rs @@ -308,6 +308,13 @@ pub(super) fn messages_to_conversation(messages: &[Message]) -> Vec &[Message] { let start = messages .iter() @@ -317,6 +324,40 @@ pub(super) fn messages_since_last_user(messages: &[Message]) -> &[Message] { &messages[start..] } +/// The transcript suffix appended during a single turn, sliced at an **explicit +/// boundary** captured *before* the run — `base_len` is the length of the +/// request's `input` transcript (`history_to_messages(&history).len()`). +/// +/// This replaces the fragile "suffix after the last `Message::User`" convention +/// ([`messages_since_last_user`]) on the persistence path. Mid-turn steer/collect +/// messages are injected as `Message::user(...)` (`forward_steers` / +/// `forward_collects`), which *moves* the last-user boundary — so slicing on it +/// silently dropped every pre-steer assistant/tool round **and** the steer text +/// itself from persisted history, the next-turn KV-cache prefix, and subagent +/// checkpoints (issue #4455). Anchoring on the pre-run request length instead +/// captures the full post-request transcript, injected steers included, in +/// execution order. +/// +/// The crate returns the full transcript in `run.messages`: the agent loop seeds +/// its working transcript from `input` (`messages = input`) and only ever +/// *appends* (assistant/tool rounds + applied steers); the compression/trim +/// middleware rewrites the per-call `request.messages.clone()`, never the loop's +/// working transcript. So `messages` always starts with the `base_len` request +/// messages as a prefix. `base_len` is clamped defensively in case a future +/// crate change ever front-trims the persisted transcript. +pub(super) fn messages_since_request(messages: &[Message], base_len: usize) -> &[Message] { + let start = base_len.min(messages.len()); + if start != base_len { + tracing::warn!( + base_len, + transcript_len = messages.len(), + "[tinyagents] messages_since_request boundary exceeds transcript length; \ + clamping (transcript may have been front-trimmed) — persisting full transcript" + ); + } + &messages[start..] +} + /// Convert a harness transcript into openhuman [`ChatMessage`]s for a provider /// that does **not** support native tool calls (text/prompt-guided mode). /// diff --git a/src/openhuman/tinyagents/middleware.rs b/src/openhuman/tinyagents/middleware.rs index 799822c9c6..b85436ff05 100644 --- a/src/openhuman/tinyagents/middleware.rs +++ b/src/openhuman/tinyagents/middleware.rs @@ -21,12 +21,12 @@ //! enabled onto a harness. use std::collections::HashMap; -use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU32, AtomicUsize, Ordering}; use std::sync::Arc; use async_trait::async_trait; -use tinyagents::error::Result as TaResult; +use tinyagents::error::{Result as TaResult, TinyAgentsError}; use tinyagents::harness::context::RunContext; use tinyagents::harness::events::AgentEvent; use tinyagents::harness::message::{ContentBlock, Message as TaMessage}; @@ -34,7 +34,7 @@ use tinyagents::harness::middleware::{ AgentRun, BudgetTracker, ContextualToolSelectionMiddleware, MicrocompactMiddleware, Middleware, MiddlewareToolOutcome, ToolAllowlistMiddleware, ToolHandler, ToolMiddleware, }; -use tinyagents::harness::model::{ModelRequest, PromptSegment, SegmentRole}; +use tinyagents::harness::model::{ModelRequest, ModelResponse, PromptSegment, SegmentRole}; use tinyagents::harness::no_progress::{NoProgress, NoProgressTracker, ToolAttempt}; use tinyagents::harness::runtime::AgentHarness; use tinyagents::harness::steering::{SteeringCommand, SteeringHandle}; @@ -94,6 +94,55 @@ pub(crate) struct TurnContextMiddleware { /// [`ResultHandoffCache`] and replaced with an `extract_from_result` drill-in /// placeholder. `None` everywhere else. pub(crate) handoff: Option, + /// Live transcript snapshot sink (#4466). When set, a + /// [`TranscriptSnapshotMiddleware`] mirrors the running conversation (as + /// openhuman [`ChatMessage`]s) into this shared buffer before every model + /// call. Only the sub-agent path sets it, so an erroring run can persist the + /// rounds completed before the failure (the harness drops its partial + /// transcript on `Err`). `None` everywhere else (chat persists post-run). + pub(crate) transcript_snapshot: Option, +} + +/// Shared buffer a [`TranscriptSnapshotMiddleware`] mirrors the live sub-agent +/// conversation into, so the caller can persist completed rounds even when the +/// harness run ends in `Err` (#4466). +pub(crate) type TranscriptSnapshotSink = + Arc>>; + +/// Observation-only middleware that snapshots the running transcript into a +/// shared [`TranscriptSnapshotSink`] before each model call (#4466). +/// +/// The tinyagents harness owns the working message vector and only hands it back +/// inside a successful `AgentRun`; on a mid-run error it is dropped. The +/// sub-agent runner persists a per-child `session_raw` transcript so +/// `learning/transcript_ingest` can read it — but a failed run used to persist +/// nothing. This middleware mirrors each `before_model` request's messages +/// (which include every prior completed assistant/tool round) into an +/// openhuman-owned buffer, so the runner's error path can still write the rounds +/// that completed before the failure. Converts to [`ChatMessage`] eagerly so the +/// caller does not need access to the private `convert` module. +pub(crate) struct TranscriptSnapshotMiddleware { + sink: TranscriptSnapshotSink, +} + +#[async_trait] +impl Middleware<()> for TranscriptSnapshotMiddleware { + fn name(&self) -> &str { + "openhuman.transcript_snapshot" + } + + async fn before_model( + &self, + _ctx: &mut RunContext<()>, + _state: &(), + request: &mut ModelRequest, + ) -> TaResult<()> { + let history = super::convert::messages_to_history(&request.messages); + if let Ok(mut guard) = self.sink.lock() { + *guard = history; + } + Ok(()) + } } /// Config for the [`HandoffMiddleware`]: the per-spawn cache (shared with the @@ -163,23 +212,26 @@ pub(super) struct OpenHumanToolExposureShadowMiddleware { impl OpenHumanToolExposureShadowMiddleware { /// Build the shadow layer from the SAME inputs the precompute path feeds the /// runner: the broad `candidate_names` and the narrowed `allowed` visible set. - /// An empty `allowed` means "all candidates visible" (OpenHuman convention). + /// Allowlist semantics are **fail-closed** (issue #4452): `None` means "no + /// filter supplied → all candidates visible"; `Some(set)` means "exactly the + /// named tools", so `Some(empty)` is a genuine deny-all. This mirrors the + /// registration loop in `assemble_turn_harness`, keeping the shadow divergence + /// reference in step with what OpenHuman actually registers as callable. pub(super) fn new( candidate_names: &[String], - allowed: &std::collections::HashSet, + allowed: Option<&std::collections::HashSet>, tags: Vec, ) -> Self { - // Effective visible set = the OpenHuman-precomputed `allowed` (or every - // candidate when `allowed` is empty). Fail-closed: a candidate absent from - // `allowed` is treated as excluded (unclassified -> not exposed). - let registered: std::collections::HashSet = if allowed.is_empty() { - candidate_names.iter().cloned().collect() - } else { - candidate_names + // Effective visible set: `None` → every candidate; `Some(set)` → exactly + // the candidates named in `set` (empty set → none). Fail-closed: a + // candidate absent from a supplied `allowed` is excluded (not exposed). + let registered: std::collections::HashSet = match allowed { + None => candidate_names.iter().cloned().collect(), + Some(set) => candidate_names .iter() - .filter(|name| allowed.contains(*name)) + .filter(|name| set.contains(*name)) .cloned() - .collect() + .collect(), }; let excluded: Vec = candidate_names .iter() @@ -237,6 +289,7 @@ impl TurnContextMiddleware { autocompact_enabled: true, super_context: None, handoff: None, + transcript_snapshot: None, } } @@ -248,6 +301,7 @@ impl TurnContextMiddleware { && self.microcompact_keep_recent == 0 && self.super_context.is_none() && self.handoff.is_none() + && self.transcript_snapshot.is_none() } /// Push the enabled middlewares onto `harness`. @@ -261,6 +315,13 @@ impl TurnContextMiddleware { harness: &mut AgentHarness<()>, tool_policies: HashMap, ) { + // Transcript snapshot (#4466) runs first among before_model hooks so it + // mirrors the *incoming* request transcript (every prior completed round) + // before microcompact/summarization rewrite it — the caller's error path + // persists exactly what the model was about to see. + if let Some(sink) = self.transcript_snapshot { + harness.push_middleware(Arc::new(TranscriptSnapshotMiddleware { sink })); + } // Super context runs first: it prepares the read-only context bundle and // folds it into the first model call's user message before any other // before_model hook inspects the request. @@ -280,16 +341,15 @@ impl TurnContextMiddleware { CLEARED_PLACEHOLDER, ))); } - // Handoff runs BEFORE the tool-output budget so an oversized payload is - // stashed + replaced with a short placeholder first; the byte cap would - // otherwise shrink it below the handoff threshold and defeat the drill-in. - if let Some(handoff) = self.handoff { - harness.push_middleware(Arc::new(HandoffMiddleware { - cache: handoff.cache, - agent_id: handoff.agent_id, - task_id: handoff.task_id, - })); - } + // REVERSE-ORDER RULE (issue #4464): the crate runs `after_tool` hooks in + // REVERSE registration order (`MiddlewareStack::run_after_tool` iterates + // `self.middlewares.iter().rev()`, tinyagents src/harness/middleware/mod.rs). + // So the LAST-pushed middleware's `after_tool` runs FIRST. To make the + // effective `after_tool` chain be handoff(raw) → tool-output budget/caps, + // the handoff MUST be pushed AFTER the tool-output budget. + // + // Push the tool-output budget FIRST (so its `after_tool` runs SECOND): + // it truncates the oversized payload to the 16 KiB byte cap. if self.tool_result_budget_bytes > 0 || self.payload_summarizer.is_some() || self.tokenjuice_compaction_enabled @@ -303,6 +363,18 @@ impl TurnContextMiddleware { tool_policies, })); } + // Push the handoff LAST (so its `after_tool` runs FIRST): it observes the + // RAW, uncapped payload, stashes an oversized result into the + // `ResultHandoffCache`, and swaps in a short pointer BEFORE the tool-output + // budget can shrink it below the 50k-token handoff threshold and defeat the + // drill-in. + if let Some(handoff) = self.handoff { + harness.push_middleware(Arc::new(HandoffMiddleware { + cache: handoff.cache, + agent_id: handoff.agent_id, + task_id: handoff.task_id, + })); + } } } @@ -977,6 +1049,106 @@ impl ToolMiddleware<()> for CliRpcOnlyMiddleware { } } +/// `wrap_tool`: scrub credential-shaped secrets out of every tool result before +/// it leaves the tool boundary (issue #4453). The legacy engine ran +/// `scrub_credentials` over **every** tool output before it entered model +/// context (`engine/tools.rs`); the tinyagents path dropped that call site, so +/// secrets in tool output (env dumps, config reads, API responses, shell output) +/// reached model context, on-disk `session_raw` transcripts, worker-thread +/// mirrors, and the tool-outcome capture sink — violating "Never log secrets or +/// full PII". +/// +/// Installed as the **innermost** tool wrap (pushed last), so it observes the +/// RAW tool result first and scrubs it before any outer wrap, the `after_tool` +/// chain (summarization/caps in [`ToolOutputMiddleware`]), the transcript push, +/// or the [`ToolOutcomeCaptureMiddleware`] sink can see the unredacted content. +/// Scrubbing here — rather than inside `execute_openhuman_tool` — covers the +/// parent chat path, sub-agent paths, the persisted transcript, and +/// `ToolCallOutcome` records by construction, since every path runs the same +/// `assemble_turn_harness` seam. +pub(super) struct CredentialScrubMiddleware; + +impl CredentialScrubMiddleware { + pub(super) fn new() -> Self { + Self + } +} + +#[async_trait] +impl ToolMiddleware<()> for CredentialScrubMiddleware { + fn name(&self) -> &str { + "credential_scrub" + } + + async fn wrap_tool( + &self, + ctx: &mut RunContext<()>, + state: &(), + call: TaToolCall, + next: ToolHandler<'_, (), ()>, + ) -> TaResult { + let tool_name = call.name.clone(); + let outcome = next.run(ctx, state, call).await?; + // `MiddlewareToolOutcome` is `#[non_exhaustive]`; today it only carries a + // `Result`, but match rather than irrefutable-let so a future variant + // fails loud instead of silently bypassing scrubbing. + let mut result = match outcome { + MiddlewareToolOutcome::Result(result) => result, + other => return Ok(other), + }; + + let scrubbed_content = + crate::openhuman::agent::harness::credentials::scrub_credentials(&result.content); + if scrubbed_content != result.content { + tracing::warn!( + tool = %tool_name, + "[tinyagents::mw] credential_scrub redacted secret(s) from tool result content" + ); + result.content = scrubbed_content; + } + + if let Some(err) = result.error.as_ref() { + let scrubbed_err = + crate::openhuman::agent::harness::credentials::scrub_credentials(err); + if &scrubbed_err != err { + tracing::warn!( + tool = %tool_name, + "[tinyagents::mw] credential_scrub redacted secret(s) from tool result error" + ); + result.error = Some(scrubbed_err); + } + } + + // Raw JSON payloads (rarely populated on this path) can carry the same + // secrets — walk their string leaves so a scrubbed `content` isn't + // undermined by an unredacted `raw` mirror. + if let Some(raw) = result.raw.take() { + result.raw = Some(scrub_json_credentials(raw)); + } + + Ok(MiddlewareToolOutcome::Result(result)) + } +} + +/// Recursively scrub credential-shaped string leaves inside a JSON value. +fn scrub_json_credentials(value: serde_json::Value) -> serde_json::Value { + use serde_json::Value; + match value { + Value::String(s) => { + Value::String(crate::openhuman::agent::harness::credentials::scrub_credentials(&s)) + } + Value::Array(items) => { + Value::Array(items.into_iter().map(scrub_json_credentials).collect()) + } + Value::Object(map) => Value::Object( + map.into_iter() + .map(|(k, v)| (k, scrub_json_credentials(v))) + .collect(), + ), + other => other, + } +} + /// `wrap_tool`: enforce the agent's builder-configured [`ToolPolicy`] at the tool /// boundary (issue #4249). The in-house engine ran this check in /// `agent_tool_exec` (`ctx.tool_policy.check(...)`); the tinyagents path bypassed @@ -1169,8 +1341,12 @@ impl ToolMiddleware<()> for ToolPolicyMiddleware { /// into a shared sink before the harness folds the result into a `Message::tool` /// that drops the `error` flag (issue #4249). Without this, a post-turn /// `ToolCallRecord` could only report every call as an optimistic success — the -/// in-house engine tracked real per-call success. Runs last in the `after_tool` -/// chain so it records the final (summarized/capped) content the transcript keeps. +/// in-house engine tracked real per-call success. The crate runs `after_tool` in +/// REVERSE registration order (issue #4464), so registering this AFTER the +/// summarization/cap middlewares (i.e. pushing it EARLIER, before +/// `TurnContextMiddleware::install`) makes its `after_tool` run AFTER those caps — +/// recording the final (summarized/capped) content the transcript keeps, not the +/// raw payload. pub(crate) struct ToolOutcomeCaptureMiddleware { sink: super::ToolOutcomeSink, /// `call_id → (success, classified failure)` side-channel read by the event @@ -1221,27 +1397,44 @@ impl Middleware<()> for ToolOutcomeCaptureMiddleware { let success = result.error.is_none(); // Classify the failure so the live `ToolCallCompleted` event and the - // persisted timeline can explain it in plain language. A hard - // policy/permission denial is its own class; otherwise heuristics over - // the error text (`timed_out` detected from the timeout branch's phrase). + // persisted timeline can explain it in plain language. The classifier + // owns all marker precedence now (policy-blocked / policy-denied / TTL + // expiry short-circuit ahead of the `timed out` sniff — #4459), so this + // just hands it the failure text. + // + // Sniff both `error` and `content`: the classifier historically read + // `error` while the marker/timeout sniffs read `content`, a latent + // asymmetry (#4459). Combine them so a marker/phrase is found wherever + // the tool layer put it. let failure = if success { None } else { - let text = result.error.as_deref().unwrap_or(result.content.as_str()); - if result - .content - .contains(crate::openhuman::security::POLICY_BLOCKED_MARKER) - { - Some(crate::openhuman::tool_status::describe( - crate::openhuman::tool_status::ToolFailureClass::BlockedByPolicy, - )) + let error = result.error.as_deref().unwrap_or(""); + let combined: std::borrow::Cow<'_, str> = if error.is_empty() { + std::borrow::Cow::Borrowed(result.content.as_str()) + } else if result.content.is_empty() || result.content == error { + std::borrow::Cow::Borrowed(error) } else { - let timed_out = result.content.contains("timed out"); - Some(crate::openhuman::tool_status::classify(text, timed_out)) - } + std::borrow::Cow::Owned(format!("{error}\n{}", result.content)) + }; + let timed_out = combined.contains("timed out"); + Some(crate::openhuman::tool_status::classify( + &combined, timed_out, + )) }; if let Ok(mut map) = self.failure_map.lock() { - map.insert(result.call_id.clone(), (success, failure)); + // Also carry the executor-measured duration + rendered output size so + // the event bridge can project real `elapsed_ms`/`output_chars` on + // `ToolCallCompleted` instead of `0`/`0` (#4467, item 4). + map.insert( + result.call_id.clone(), + ( + success, + failure, + result.elapsed_ms, + result.content.chars().count(), + ), + ); } if let Ok(mut sink) = self.sink.lock() { sink.push(super::ToolCallOutcome { @@ -1255,14 +1448,42 @@ impl Middleware<()> for ToolOutcomeCaptureMiddleware { } } -/// `before_tool`: coerce a tool call's arguments to an empty object when they -/// are not a JSON object (issue #4249). A model can emit malformed native -/// arguments (invalid JSON, or a bare scalar/array); the model adapter parses -/// those to `Value::Null`, which the harness then rejects against an object -/// schema and aborts the whole turn. The in-house engine recovered such a call by -/// running the tool with `{}`; restore that so a single bad tool call is -/// recoverable rather than fatal. -pub(crate) struct ArgRecoveryMiddleware; +/// `before_tool`: repair a tool call's arguments *before* the harness runs its +/// fatal pre-execution schema gate (issues #4249 / #4451). A model can emit +/// arguments the model adapter parses to a non-object `Value` — invalid JSON +/// decodes to `Value::Null`, and some providers emit the whole arguments blob as +/// a JSON-encoded *string* (optionally wrapped in a ```json markdown fence). Left +/// alone the harness rejects those against an object schema and aborts the whole +/// turn. +/// +/// Recovery, in order: +/// 1. Already a JSON object → leave it (the common, valid case). +/// 2. A JSON-encoded string (optionally fenced) that decodes to an object → +/// decode and use it. +/// 3. Otherwise a non-object whose tool schema declares **no** required fields → +/// coerce to `{}` (legacy-engine parity: the tool runs and produces its own +/// recoverable error). +/// 4. Otherwise (non-object + schema has required fields) → leave the arguments +/// untouched so [`SchemaGuardMiddleware`] converts the schema-validation +/// failure into a model-visible tool error rather than a turn abort. The old +/// behaviour (coerce to `{}`) *guaranteed* a `" is required"` fatal +/// abort for those tools, so it is exactly the case that must fall through. +pub(crate) struct ArgRecoveryMiddleware { + /// The same `Arc`-shared tool sets the runner registers, used to resolve a + /// call's schema so we can tell whether coercing to `{}` is safe. + tool_sets: Vec>>>, +} + +impl ArgRecoveryMiddleware { + /// Build the middleware over the runner's shared tool sets. + pub(crate) fn new(tool_sets: Vec>>>) -> Self { + Self { tool_sets } + } + + fn schema_for(&self, name: &str) -> Option { + schema_for_tool(&self.tool_sets, name) + } +} #[async_trait] impl Middleware<()> for ArgRecoveryMiddleware { @@ -1276,17 +1497,320 @@ impl Middleware<()> for ArgRecoveryMiddleware { _state: &(), call: &mut TaToolCall, ) -> TaResult<()> { - if !call.arguments.is_object() { + // (1) Already valid object shape — nothing to do. + if call.arguments.is_object() { + return Ok(()); + } + + // (2) JSON-encoded-string arguments (optionally markdown-fenced): decode + // and adopt the inner object. + if let Some(raw) = call.arguments.as_str() { + if let Some(obj) = recover_object_from_json_string(raw) { + tracing::debug!( + tool = call.name.as_str(), + "[tinyagents::mw] arg_recovery: decoded JSON-encoded-string tool arguments to object" + ); + call.arguments = obj; + return Ok(()); + } + } + + // (3) Non-object with a permissive schema (no required fields): coerce to + // `{}` so the tool runs and produces its own recoverable error — engine + // parity for tools that predate the schema gate. + let has_required = self + .schema_for(&call.name) + .map(|schema| schema_has_required_fields(&schema.parameters)) + .unwrap_or(false); + if !has_required { tracing::debug!( tool = call.name.as_str(), - "[tinyagents::mw] recovering non-object tool arguments to {{}}" + "[tinyagents::mw] arg_recovery: coercing non-object tool arguments to {{}} (schema declares no required fields)" ); call.arguments = serde_json::json!({}); + return Ok(()); } + + // (4) Non-object + schema has required fields: leave untouched. Coercing + // to `{}` here would guarantee a fatal `" is required"` abort; + // instead `SchemaGuardMiddleware` surfaces a descriptive, recoverable + // tool error. + tracing::debug!( + tool = call.name.as_str(), + args_kind = json_value_kind(&call.arguments), + "[tinyagents::mw] arg_recovery: leaving non-object tool arguments for the schema-guard tool-error path" + ); Ok(()) } } +/// `before_tool` + `wrap_tool`: convert the harness's **fatal** pre-execution +/// JSON-schema gate into a model-visible tool error instead of a turn abort +/// (issue #4451). +/// +/// The tinyagents agent loop validates every tool call against its schema +/// (`ToolSchema::validate_call`) *between* `before_tool` and the tool-wrap onion; +/// any `required`/type/`enum` violation returns `TinyAgentsError::Validation`, +/// which propagates out of `run_loop` and fails the entire turn +/// (`"tinyagents harness run failed: Validation(...)"`). The legacy engine had no +/// such gate — bad arguments came back as recoverable tool *results* the model +/// self-corrected on the next iteration. +/// +/// This middleware restores that behaviour entirely seam-side (the crate is +/// upstream/read-only): +/// - `before_tool` runs the *same* validation itself; on failure it records a +/// descriptive error keyed by the call id and rewrites the arguments to a +/// schema-satisfying **stub** so the crate's own fatal gate passes. +/// - `wrap_tool` then short-circuits the flagged call with a synthetic failed +/// [`TaToolResult`] **without** executing the real tool (the stub args never +/// reach it), so the loop continues and the model self-corrects. +/// +/// Installed as the outermost tool-wrap middleware so an invalid call is turned +/// into a tool error before approval/policy wraps ever see the stub arguments. +pub(super) struct SchemaGuardMiddleware { + /// The same `Arc`-shared tool sets the runner registers, used to resolve a + /// call's schema for validation. + tool_sets: Vec>>>, + /// call id → synthetic tool-error message, written in `before_tool` when a + /// call fails validation and consumed in `wrap_tool` to short-circuit it. + /// A flagged call always reaches `wrap_tool` (its stub args pass the crate + /// gate), so entries never accumulate across a turn. + pending: Arc>>, +} + +impl SchemaGuardMiddleware { + /// Build the middleware over the runner's shared tool sets. + pub(super) fn new(tool_sets: Vec>>>) -> Self { + Self { + tool_sets, + pending: Arc::default(), + } + } + + fn schema_for(&self, name: &str) -> Option { + schema_for_tool(&self.tool_sets, name) + } +} + +#[async_trait] +impl Middleware<()> for SchemaGuardMiddleware { + fn name(&self) -> &str { + "schema_guard" + } + + async fn before_tool( + &self, + _ctx: &mut RunContext<()>, + _state: &(), + call: &mut TaToolCall, + ) -> TaResult<()> { + // Unknown tool → let the crate's `UnknownToolPolicy` handle it (it + // already returns a recoverable tool error). + let Some(schema) = self.schema_for(&call.name) else { + return Ok(()); + }; + + let probe = TaToolCall { + id: call.id.clone(), + name: call.name.clone(), + arguments: call.arguments.clone(), + }; + let Err(err) = schema.validate_call(&probe) else { + return Ok(()); + }; + + let detail = match err { + TinyAgentsError::Validation(message) => message, + other => other.to_string(), + }; + let schema_json = serde_json::to_string(&schema.parameters) + .unwrap_or_else(|_| "".to_string()); + let message = format!( + "invalid arguments for {}: {}. Expected schema: {}", + call.name, detail, schema_json + ); + tracing::warn!( + tool = call.name.as_str(), + detail = detail.as_str(), + "[tinyagents::mw] schema_guard: tool-arg validation failed; converting fatal gate into a model-visible tool error" + ); + + if let Ok(mut pending) = self.pending.lock() { + pending.insert(call.id.clone(), message); + } + // Rewrite to a schema-satisfying stub so the crate's fatal + // `validate_call` gate passes; `wrap_tool` short-circuits the call + // before these stub args can reach the real tool. + call.arguments = synthesize_valid_arguments(&schema.parameters); + Ok(()) + } +} + +#[async_trait] +impl ToolMiddleware<()> for SchemaGuardMiddleware { + fn name(&self) -> &str { + "schema_guard" + } + + async fn wrap_tool( + &self, + ctx: &mut RunContext<()>, + state: &(), + call: TaToolCall, + next: ToolHandler<'_, (), ()>, + ) -> TaResult { + let flagged = self + .pending + .lock() + .ok() + .and_then(|mut pending| pending.remove(&call.id)); + if let Some(message) = flagged { + tracing::debug!( + tool = call.name.as_str(), + "[tinyagents::mw] schema_guard: short-circuiting invalid tool call with a synthetic error result" + ); + return Ok(MiddlewareToolOutcome::Result(TaToolResult::error( + call.id, call.name, message, + ))); + } + next.run(ctx, state, call).await + } +} + +/// Resolves the harness [`ToolSchema`] for `name` across the runner's shared +/// tool sets. +/// +/// Built via the same [`spec_to_schema`](super::convert::spec_to_schema) +/// conversion the runner uses for [`SharedToolAdapter::schema`], so the +/// `parameters` we validate against are byte-identical to the ones the crate's +/// fatal `validate_call` gate checks — otherwise our pre-validation could +/// disagree with the crate and either miss a fatal case or stub a call the crate +/// still rejects. +fn schema_for_tool(tool_sets: &[Arc>>], name: &str) -> Option { + tool_sets + .iter() + .flat_map(|set| set.iter()) + .find(|tool| tool.name() == name) + .map(|tool| super::convert::spec_to_schema(&tool.spec())) +} + +/// Whether a tool's JSON-schema `parameters` declares any `required` field. +fn schema_has_required_fields(parameters: &serde_json::Value) -> bool { + parameters + .get("required") + .and_then(serde_json::Value::as_array) + .map(|required| required.iter().any(serde_json::Value::is_string)) + .unwrap_or(false) +} + +/// Attempts to recover a JSON **object** from a string-encoded arguments payload: +/// providers sometimes emit the whole arguments blob as a JSON string, optionally +/// wrapped in a ```json markdown fence. Returns `None` when the string does not +/// decode to a JSON object. +fn recover_object_from_json_string(raw: &str) -> Option { + let candidate = strip_code_fence(raw); + serde_json::from_str::(candidate) + .ok() + .filter(serde_json::Value::is_object) +} + +/// Strips a surrounding markdown code fence (```` ```json … ``` ````) and its +/// optional language tag, returning the inner text. A string with no fence is +/// returned trimmed and unchanged. +fn strip_code_fence(raw: &str) -> &str { + let trimmed = raw.trim(); + let Some(after_open) = trimmed.strip_prefix("```") else { + return trimmed; + }; + // Drop an optional language tag on the opening fence line (e.g. `json`). + let body = match after_open.find('\n') { + Some(newline) + if after_open[..newline] + .chars() + .all(|c| c.is_ascii_alphanumeric()) => + { + &after_open[newline + 1..] + } + _ => after_open, + }; + body.trim().strip_suffix("```").unwrap_or(body).trim() +} + +/// A short, human-readable kind label for a JSON value, for debug logging. +fn json_value_kind(value: &serde_json::Value) -> &'static str { + match value { + serde_json::Value::Null => "null", + serde_json::Value::Bool(_) => "boolean", + serde_json::Value::Number(_) => "number", + serde_json::Value::String(_) => "string", + serde_json::Value::Array(_) => "array", + serde_json::Value::Object(_) => "object", + } +} + +/// Synthesizes a minimal value that satisfies the JSON-schema subset the +/// tinyagents gate (`ToolSchema::validate_call`) enforces — `enum`, `type`, +/// object `properties`/`required`, and array `items`. Used to rewrite a +/// validation-failed call's arguments so the crate's fatal gate passes; the call +/// is then short-circuited in `wrap_tool`, so this stub never reaches the tool. +fn synthesize_valid_arguments(schema: &serde_json::Value) -> serde_json::Value { + use serde_json::Value; + + // `enum` constrains the value to a fixed set — the first option always + // satisfies the gate (and any co-declared `type`). + if let Some(values) = schema.get("enum").and_then(Value::as_array) { + return values.first().cloned().unwrap_or(Value::Null); + } + + // `type` may be a string or an array of strings; pick the first known kind. + let kind = schema.get("type").and_then(|type_spec| { + type_spec.as_str().map(str::to_string).or_else(|| { + type_spec + .as_array()? + .iter() + .filter_map(Value::as_str) + .next() + .map(str::to_string) + }) + }); + + match kind.as_deref() { + Some("object") => synthesize_valid_object(schema), + Some("array") => Value::Array(Vec::new()), + Some("string") => Value::String(String::new()), + Some("integer") | Some("number") => serde_json::json!(0), + Some("boolean") => Value::Bool(false), + Some("null") => Value::Null, + _ => { + if schema.get("properties").is_some() { + synthesize_valid_object(schema) + } else { + // No understood constraints → an empty object trivially passes. + Value::Object(serde_json::Map::new()) + } + } + } +} + +/// Builds an object populated with every `required` field (recursively) so it +/// satisfies the gate's object/`required` checks. +fn synthesize_valid_object(schema: &serde_json::Value) -> serde_json::Value { + use serde_json::Value; + + let mut object = serde_json::Map::new(); + if let Some(required) = schema.get("required").and_then(Value::as_array) { + let properties = schema.get("properties").and_then(Value::as_object); + for field in required.iter().filter_map(Value::as_str) { + let field_schema = properties + .and_then(|props| props.get(field)) + .cloned() + .unwrap_or(Value::Null); + object.insert(field.to_string(), synthesize_valid_arguments(&field_schema)); + } + } + Value::Object(object) +} + /// Agents are told to follow a **read-index → dedupe → write → update-index** /// cycle around durable memory, but the contract was never enforced, so it was /// followed inconsistently: writes landed without a dedupe read (duplicating @@ -1619,6 +2143,17 @@ pub(crate) struct RepeatedToolFailureMiddleware { /// argument sets that happen to share a first error line don't count as a /// repeat and can't pre-empt the generic no-progress backstop. arg_sigs: std::sync::Mutex>, + /// Recoverable-failure ladder (issue #4463): transient failures (timeouts, + /// connection resets, rate limits, 5xx) are routed here instead of the crate + /// tracker so they get the legacy extended headroom + /// ([`RECOVERABLE_REPEAT_FAILURE_THRESHOLD`] identical / + /// [`RECOVERABLE_NO_PROGRESS_FAILURE_THRESHOLD`] consecutive) rather than the + /// crate's fixed 3/6, which is right only for deterministic failures. + /// `tool\u{1f}args` → identical-failure count; persists across the turn. + recoverable_sig_counts: std::sync::Mutex>, + /// Consecutive recoverable-looking failures with no success in between. Reset + /// on any success or non-recoverable failure (mirrors the legacy guard). + recoverable_consecutive: AtomicU32, } impl RepeatedToolFailureMiddleware { @@ -1636,7 +2171,55 @@ impl RepeatedToolFailureMiddleware { tracker: NoProgressTracker::new(identical_threshold), step: AtomicUsize::new(0), arg_sigs: std::sync::Mutex::new(std::collections::HashMap::new()), + recoverable_sig_counts: std::sync::Mutex::new(std::collections::HashMap::new()), + recoverable_consecutive: AtomicU32::new(0), + } + } + + /// Clear the consecutive recoverable-failure streak. Called on any success or + /// non-recoverable failure (the per-signature identical counts persist across + /// the turn, matching the legacy guard). Idempotent. + fn reset_recoverable_streak(&self) { + self.recoverable_consecutive.store(0, Ordering::SeqCst); + } + + /// Record one recoverable failure and return a root-cause halt summary once + /// its extended headroom is exhausted (identical `>=` [`RECOVERABLE_REPEAT_FAILURE_THRESHOLD`] + /// or consecutive `>=` [`RECOVERABLE_NO_PROGRESS_FAILURE_THRESHOLD`]). + fn record_recoverable(&self, tool: &str, arg_fp: &str, failure_text: &str) -> Option { + let key = format!("{tool}\u{1f}{arg_fp}"); + let count = self + .recoverable_sig_counts + .lock() + .ok() + .map(|mut counts| { + let c = counts.entry(key).or_insert(0); + *c += 1; + *c + }) + .unwrap_or(0); + let consecutive = self.recoverable_consecutive.fetch_add(1, Ordering::SeqCst) + 1; + tracing::debug!( + tool, + count, + consecutive, + "[tinyagents::mw] recoverable tool failure recorded with extended circuit-breaker headroom" + ); + if count >= RECOVERABLE_REPEAT_FAILURE_THRESHOLD { + return Some(recoverable_identical_halt_summary( + tool, + count, + failure_text, + )); } + if consecutive >= RECOVERABLE_NO_PROGRESS_FAILURE_THRESHOLD { + return Some(recoverable_no_progress_halt_summary( + consecutive, + tool, + failure_text, + )); + } + None } } @@ -1724,15 +2307,91 @@ impl Middleware<()> for RepeatedToolFailureMiddleware { .unwrap_or_default(); let step = self.step.fetch_add(1, Ordering::SeqCst) + 1; + // Combined failure text for classification: the model-facing content plus + // the (redundant but authoritative) error field. Both are scanned for the + // policy / terminal-inference / recoverable markers below. + let failure_text = match result.error.as_deref() { + Some(err) => format!("{}\n{}", result.content, err), + None => String::new(), + }; + + // ── Part 5 (#3104): terminal delegated-inference fast-halt ────────────── + // A permanent inference failure (out of budget / provider-config rejection) + // surfaced by a delegated sub-agent cannot be recovered by retrying — the + // budget is account-wide and the model/provider config is shared by every + // (sub-)agent. Halt on the FIRST occurrence with an actionable root cause, + // *before* the count-based thresholds, because the orchestrator otherwise + // re-emits the doomed step under varied delegation-tool names so the + // identical-retry threshold never trips in time. + if result.error.is_some() { + if let Some(kind) = terminal_inference_failure_kind(&failure_text) { + tracing::warn!( + tool = %result.name, + kind = ?kind, + "[tinyagents::mw] terminal delegated-inference failure — halting on first occurrence with root cause" + ); + if let Ok(mut slot) = self.halt_summary.lock() { + *slot = Some(terminal_inference_halt_summary( + kind, + &result.name, + &failure_text, + )); + } + self.handle.send(SteeringCommand::Pause); + self.tracker.reset(); + self.reset_recoverable_streak(); + return Ok(()); + } + } + // A hard policy rejection is marked in the tool output; it can never - // succeed when re-issued unchanged, so the crate ladder trips it faster. - let hard_reject = result - .content - .contains(crate::openhuman::security::POLICY_BLOCKED_MARKER) - || result - .error - .as_deref() - .is_some_and(|err| err.contains(crate::openhuman::security::POLICY_BLOCKED_MARKER)); + // succeed when re-issued unchanged, so the crate ladder trips it faster + // (its `HARD_REJECT_HALT_THRESHOLD` of 2). Both the read-only/forbidden + // block (`POLICY_BLOCKED_MARKER`) and the approval denial / TTL expiry + // (`POLICY_DENIED_MARKER`) are deterministic — restore the 2-repeat + // fast-trip for BOTH (issue #4463 part 6: denied had drifted to the + // generic 3). + let policy_marked = |s: &str| { + s.contains(crate::openhuman::security::POLICY_BLOCKED_MARKER) + || s.contains(crate::openhuman::security::POLICY_DENIED_MARKER) + }; + let hard_reject = + policy_marked(&result.content) || result.error.as_deref().is_some_and(policy_marked); + + // ── Part 4: recoverable-failure headroom ──────────────────────────────── + // Transient failures (timeouts, connection resets, rate limits, 5xx) get + // the legacy extended headroom instead of the crate's deterministic 3/6. + // Route them to the recoverable ladder; a success or a non-recoverable + // failure resets that streak and feeds the crate tracker as before. + let recoverable = result.error.is_some() + && !hard_reject + && (is_recoverable_tool_failure(&failure_text) + || matches!( + crate::openhuman::tool_status::classify(&failure_text, false).class, + crate::openhuman::tool_status::ToolFailureClass::Timeout + | crate::openhuman::tool_status::ToolFailureClass::ServiceUnavailable + | crate::openhuman::tool_status::ToolFailureClass::ModelConnection + )); + if recoverable { + if let Some(summary) = self.record_recoverable(&result.name, &arg_fp, &failure_text) { + tracing::warn!( + tool = %result.name, + "[tinyagents::mw] recoverable-failure headroom exhausted — halting run so the root cause surfaces" + ); + if let Ok(mut slot) = self.halt_summary.lock() { + *slot = Some(summary); + } + self.handle.send(SteeringCommand::Pause); + self.reset_recoverable_streak(); + } + // Recoverable failures never feed the crate tracker — its fixed 3/6 + // backstop would halt them before the extended headroom is spent. + return Ok(()); + } + // Success or non-recoverable failure: clear the recoverable streak (its + // per-signature counts persist across the turn) before the crate tracker + // handles the deterministic 3/6 + hard-reject-2 path below. + self.reset_recoverable_streak(); let attempt = ToolAttempt { tool: &result.name, @@ -1800,6 +2459,684 @@ impl Middleware<()> for RepeatedToolFailureMiddleware { } } +// ── Loop-guard restorations (issue #4463) ──────────────────────────────────── +// +// The TinyAgents migration dropped several loop breakers that the crate does not +// replace (verified against `harness::no_progress`, which tracks *failures* +// only): the recoverable-failure headroom, the terminal delegated-inference +// fast-halt (#3104), the policy-denied fast-trip, and the successful-repeat / +// identical-output guards (#4088 / #4095). These helpers + the +// [`RepeatProgressMiddleware`] below restore that behaviour seam-side, ported +// verbatim from the deleted `agent/harness/tool_loop.rs` thresholds/wording so +// the guards read identically to the legacy loop. + +/// Recoverable/transient failures get more identical-retry headroom than the +/// deterministic default: a flaky network call or a timeout can succeed on a +/// later attempt once the model adapts (longer timeout, smaller batch, retry). +/// Mirrors the legacy `RECOVERABLE_REPEAT_FAILURE_THRESHOLD`. +const RECOVERABLE_REPEAT_FAILURE_THRESHOLD: u32 = 8; +/// Recoverable failures also get a larger *consecutive* (varied-args) no-progress +/// headroom before the breaker halts. Mirrors the legacy +/// `RECOVERABLE_NO_PROGRESS_FAILURE_THRESHOLD`. +const RECOVERABLE_NO_PROGRESS_FAILURE_THRESHOLD: u32 = 12; + +/// The model re-emitting the IDENTICAL assistant output (narration + the same +/// tool call) this many times in a row is a no-progress narration loop — halt. +/// Mirrors the legacy `REPEAT_OUTPUT_THRESHOLD` (#4095). +const REPEAT_OUTPUT_THRESHOLD: u32 = 4; + +/// The model re-issuing the IDENTICAL `(tool, args)` batch this many times in a +/// row — regardless of whether each call *succeeds* — is spinning one action +/// with no new information. Set just below [`REPEAT_OUTPUT_THRESHOLD`] so a +/// verbatim call loop is caught a step earlier than the broader narration loop. +/// Mirrors the legacy `REPEAT_CALL_THRESHOLD` (#4088). +const REPEAT_CALL_THRESHOLD: u32 = 3; + +/// Clamp the last-error text embedded in a circuit-breaker halt summary so a huge +/// tool error (already capped at 1MB upstream) can't blow up the agent's result. +/// Mirrors the legacy `tool_loop::truncate_for_halt`. +fn truncate_for_halt(s: &str) -> String { + const MAX: usize = 600; + if s.chars().count() <= MAX { + return s.to_string(); + } + let head: String = s.chars().take(MAX).collect(); + format!("{head}\n… [truncated]") +} + +/// Failures that are informative and plausibly recoverable by changing the next +/// action (longer timeout, smaller batch, different network retry/fallback) +/// rather than by abandoning the turn. Deliberately marker-based and +/// conservative: it only controls breaker headroom, never converts a failure +/// into success. Ported verbatim from legacy `tool_loop::is_recoverable_tool_failure`. +fn is_recoverable_tool_failure(result: &str) -> bool { + let lower = result.to_ascii_lowercase(); + [ + "timed out", + "timeout", + "deadline exceeded", + "temporarily unavailable", + "temporary failure", + "connection reset", + "connection refused", + "connection closed", + "connection aborted", + "network is unreachable", + "host is unreachable", + "dns error", + "failed to lookup address", + "failed to resolve", + "rate limit", + "too many requests", + "retry after", + "503 service unavailable", + "502 bad gateway", + "504 gateway timeout", + ] + .iter() + .any(|marker| lower.contains(marker)) +} + +/// A permanent, non-retryable inference failure surfaced by a delegated +/// sub-agent's tool result. Unlike a transient error, re-issuing the call cannot +/// succeed even under a *different* delegation tool or varied args: the budget is +/// account-wide and the model/provider configuration is shared by every +/// (sub-)agent. See [`terminal_inference_failure_kind`] (#3104). +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub(crate) enum TerminalInferenceFailure { + /// Out of inference budget / credits — every retry hits the same wall. + BudgetExhausted, + /// The configured model/provider rejected the request for a reason the user + /// must fix (unknown model, non-chat/embedding model, missing credential, + /// region block, …). + ProviderConfig, +} + +/// Inference/delegation **envelope** markers that prove a tool result came from a +/// delegated inference call (a sub-agent / provider round-trip) rather than from +/// arbitrary tool stderr. Every marker here is harness-generated (our own +/// reliable-chain rollup or sub-agent dispatch wrapper), NOT a provider HTTP body +/// that arbitrary tool stderr could forge. Ported from legacy `tool_loop`. +const INFERENCE_FAILURE_ENVELOPE_MARKERS: &[&str] = &[ + // Reliable-chain exhaustion rollup (reliable.rs::format_failure_aggregate). + "all providers/models failed", + "may not be available on your provider", + // Sub-agent delegation failure wrapper (dispatch.rs::format_subagent_failure). + "failed and did not complete", +]; + +/// True if `result` carries one of the inference/delegation envelope markers — +/// i.e. the failure demonstrably came from a delegated provider round-trip, not +/// arbitrary tool stderr. See [`INFERENCE_FAILURE_ENVELOPE_MARKERS`]. +fn has_inference_failure_envelope(result: &str) -> bool { + let lower = result.to_ascii_lowercase(); + INFERENCE_FAILURE_ENVELOPE_MARKERS + .iter() + .any(|marker| lower.contains(marker)) +} + +/// Recognize a permanent (non-retryable) delegated-inference failure from a tool +/// result. Two-stage gate so a *recoverable* tool failure can't be misclassified: +/// (1) the result must carry a delegated-inference envelope +/// ([`has_inference_failure_envelope`]); (2) the trusted body is matched against +/// the two tight provider classifiers. Budget takes precedence if both match. +/// Ported from legacy `tool_loop::terminal_inference_failure_kind` (#3104). +pub(crate) fn terminal_inference_failure_kind(result: &str) -> Option { + use crate::openhuman::inference::provider::{ + is_budget_exhausted_message, is_provider_config_rejection_message, + }; + if !has_inference_failure_envelope(result) { + return None; + } + if is_budget_exhausted_message(result) { + Some(TerminalInferenceFailure::BudgetExhausted) + } else if is_provider_config_rejection_message(result) { + Some(TerminalInferenceFailure::ProviderConfig) + } else { + None + } +} + +/// The actionable root-cause halt summary for a terminal delegated-inference +/// failure. Ported verbatim from the legacy loop. +fn terminal_inference_halt_summary( + kind: TerminalInferenceFailure, + tool: &str, + result: &str, +) -> String { + match kind { + TerminalInferenceFailure::BudgetExhausted => format!( + "Stopping: the `{tool}` step failed because the account is out of inference \ + budget/credits — every retry hits the same wall. Add credits to your account \ + (or, when using a custom/BYO provider, top up that provider's own account) and try \ + again. Details:\n{}", + truncate_for_halt(result), + ), + TerminalInferenceFailure::ProviderConfig => format!( + "Stopping: the `{tool}` step failed because the configured model/provider rejected the \ + request (e.g. an unknown model, a non-chat/embedding model, a missing credential, or \ + a region block) — retrying will not help. Fix the model or API key in Settings → AI. \ + Details:\n{}", + truncate_for_halt(result), + ), + } +} + +/// Halt summary when a single recoverable `(tool, args)` call exhausts its +/// extended identical-retry headroom. Ported from the legacy loop. +fn recoverable_identical_halt_summary(tool: &str, count: u32, result: &str) -> String { + format!( + "Stopping: the `{tool}` call was retried {count} times with identical arguments and kept \ + failing — repeating it will not help. Last error:\n{}\n\nThis looked recoverable at \ + first, but the same call exhausted the extended transient-failure headroom. Report this \ + back instead of retrying.", + truncate_for_halt(result), + ) +} + +/// Halt summary when many recoverable-looking failures pile up with no progress. +/// Ported from the legacy loop. +fn recoverable_no_progress_halt_summary(consecutive: u32, tool: &str, result: &str) -> String { + format!( + "Stopping: {consecutive} recoverable-looking tool failures happened in a row with no \ + successful progress. Last error (from `{tool}`):\n{}\n\nThe turn is still bounded by the \ + iteration/cost limits, but this many consecutive transient failures means the goal is not \ + currently reachable. Report this back instead of retrying.", + truncate_for_halt(result), + ) +} + +/// Tools whose contract is to be re-invoked with identical arguments, so an +/// identical repeat is legitimate progress — not a no-progress loop. Today this +/// is `wait_subagent`, which polls a running async sub-agent and explicitly tells +/// the model to "call wait_subagent again" when a `timeout_secs` window elapses +/// while the sub-agent is still running. Without this exemption a task that +/// outlives two wait windows would have its third identical `wait_subagent` +/// halted by the no-progress breakers before it could collect the eventual +/// result. Ported from legacy `tool_loop::is_repeat_call_exempt` (Codex P1 on #4230). +pub(crate) fn is_repeat_call_exempt(tool: &str) -> bool { + matches!(tool, "wait_subagent") +} + +/// Extract the assistant's visible text (concatenated [`ContentBlock::Text`] +/// blocks) from a model response message, for the repeat-output signature. +fn assistant_visible_text(message: &tinyagents::harness::message::AssistantMessage) -> String { + let mut out = String::new(); + for block in &message.content { + if let ContentBlock::Text(t) = block { + if !out.is_empty() { + out.push('\n'); + } + out.push_str(t); + } + } + out +} + +/// A back-to-back identical-signature streak counter. Trips (`record` returns the +/// new consecutive count) once the same hashed signature repeats; a different +/// signature resets the run. Backs both the repeat-output and repeat-call guards. +#[derive(Default)] +struct StreakGuard { + last_hash: Option, + consecutive: u32, +} + +impl StreakGuard { + /// Record one signature; returns the new consecutive count for that signature + /// (1 after a reset). A different signature resets the streak to 1. + fn record(&mut self, signature: &str) -> u32 { + use std::hash::{Hash, Hasher}; + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + signature.hash(&mut hasher); + let h = hasher.finish(); + if self.last_hash == Some(h) { + self.consecutive += 1; + } else { + self.last_hash = Some(h); + self.consecutive = 1; + } + self.consecutive + } + + /// Clear the streak — used when an iteration is a legitimately-repeating + /// poll/wait (see [`is_repeat_call_exempt`]) or a failing batch that another + /// guard owns, so it counts as a distinct action rather than a repeat. + fn reset(&mut self) { + self.last_hash = None; + self.consecutive = 0; + } +} + +/// Per-batch state the repeat-CALL guard needs but can only fully evaluate once +/// every tool result in the assistant's batch has come back: the canonical +/// `(tool, args)` signature captured at `after_model`, plus the running +/// success/remaining accounting folded in at each `after_tool`. +#[derive(Default)] +struct PendingCallBatch { + /// Canonical `(tool, args)` signature of the batch, from `after_model`. + call_sig: String, + /// Tool results still outstanding for this batch. + remaining: usize, + /// `true` while every result so far in the batch has succeeded. + all_ok: bool, + /// `true` when every call in the batch is a polling/wait exemption. + exempt: bool, +} + +/// Restores the deleted successful-repeat / identical-output loop breakers +/// (#4088 / #4095) as a seam middleware. The crate `no_progress` ladder (driving +/// [`RepeatedToolFailureMiddleware`]) resets on every success, so a model looping +/// on a *successful* no-op tool or re-emitting an identical narration+call never +/// trips it and burns the whole iteration budget. This guard closes both gaps: +/// +/// - **Repeat-output** (`after_model`, checked before the tools run): halts when +/// the assistant's visible text + tool-call `(name, args)` batch is byte +/// identical [`REPEAT_OUTPUT_THRESHOLD`] iterations in a row. +/// - **Repeat-call** (evaluated once the batch's tool results are all back, gated +/// on every call succeeding): halts when the `(tool, args)` batch alone repeats +/// [`REPEAT_CALL_THRESHOLD`] times — catching successful no-op loops that vary +/// only their narration. +/// +/// Polling/wait tools ([`is_repeat_call_exempt`]) are exempt from both: their +/// contract is to be re-invoked identically, so an all-poll batch resets the +/// streaks instead of recording. On a trip it writes the legacy root-cause +/// summary into the shared [`HaltSummarySlot`](super::HaltSummarySlot) and pauses +/// the run through the shared steering handle — the same halt mechanism as the +/// repeated-failure breaker. +pub(crate) struct RepeatProgressMiddleware { + handle: SteeringHandle, + halt_summary: super::HaltSummarySlot, + /// Narration+call identical-output streak (#4095), threshold + /// [`REPEAT_OUTPUT_THRESHOLD`]. + output_guard: std::sync::Mutex, + /// `(tool, args)`-only successful-batch streak (#4088), threshold + /// [`REPEAT_CALL_THRESHOLD`]. + call_guard: std::sync::Mutex, + /// Batch bookkeeping bridging `after_model` → `after_tool` for the call guard. + pending: std::sync::Mutex>, +} + +impl RepeatProgressMiddleware { + pub(crate) fn new(handle: SteeringHandle, halt_summary: super::HaltSummarySlot) -> Self { + Self { + handle, + halt_summary, + output_guard: std::sync::Mutex::new(StreakGuard::default()), + call_guard: std::sync::Mutex::new(StreakGuard::default()), + pending: std::sync::Mutex::new(None), + } + } + + /// Latch a root-cause halt: record the summary the turn surfaces instead of an + /// empty/last-model reply, and pause at the top of the next iteration (before + /// the next model call), matching the repeated-failure breaker's halt path. + fn halt(&self, summary: String) { + if let Ok(mut slot) = self.halt_summary.lock() { + *slot = Some(summary); + } + self.handle.send(SteeringCommand::Pause); + } +} + +#[async_trait] +impl Middleware<()> for RepeatProgressMiddleware { + fn name(&self) -> &str { + "repeat_progress" + } + + async fn after_model( + &self, + _ctx: &mut RunContext<()>, + _state: &(), + response: &mut ModelResponse, + ) -> TaResult<()> { + let tool_calls = &response.message.tool_calls; + if tool_calls.is_empty() { + // A final answer (no tool calls) ends the loop; nothing to guard, and + // there is no batch to track for the call guard. + if let Ok(mut pending) = self.pending.lock() { + *pending = None; + } + return Ok(()); + } + + // Polling/wait tools are contractually re-invoked with identical args + + // narration each timeout while the work is still running, so an all-poll + // batch is legitimate progress, not a no-progress repeat. + let all_exempt = tool_calls.iter().all(|c| is_repeat_call_exempt(&c.name)); + + // Canonical `(tool, args)` batch signature (call guard) and the broader + // narration+call signature (output guard). Both fold each call in order + // with a `\u{1}` separator, matching the legacy signatures. + let mut call_sig = String::new(); + for call in tool_calls { + call_sig.push('\u{1}'); + call_sig.push_str(&call.name); + call_sig.push('\u{1}'); + call_sig.push_str(&call.arguments.to_string()); + } + let output_sig = format!( + "{}{}", + assistant_visible_text(&response.message).trim(), + call_sig + ); + + // Repeat-OUTPUT guard, checked BEFORE the (repeated) tools run so we don't + // burn another no-op iteration. + if all_exempt { + if let Ok(mut g) = self.output_guard.lock() { + g.reset(); + } + } else { + let consecutive = self + .output_guard + .lock() + .map(|mut g| g.record(&output_sig)) + .unwrap_or(0); + if consecutive >= REPEAT_OUTPUT_THRESHOLD { + tracing::warn!( + consecutive, + "[tinyagents::mw] repeat-output circuit breaker tripped — identical response+tool-call repeated; halting" + ); + self.halt(format!( + "Stopping: the last {consecutive} iterations produced the IDENTICAL response \ + and tool call with no change — the run is stuck repeating the same step \ + without making progress. Re-issuing it will not help. Summarise what (if \ + anything) was actually accomplished and report that the task could not \ + progress, or take a genuinely different approach.", + )); + } + } + + // Stage the batch for the repeat-CALL guard, evaluated once every result + // is back (gated on success) in `after_tool`. + if let Ok(mut pending) = self.pending.lock() { + *pending = Some(PendingCallBatch { + call_sig, + remaining: tool_calls.len(), + all_ok: true, + exempt: all_exempt, + }); + } + Ok(()) + } + + async fn after_tool( + &self, + _ctx: &mut RunContext<()>, + _state: &(), + result: &mut TaToolResult, + ) -> TaResult<()> { + // Fold this result into the pending batch; only act once the batch is + // complete so the call guard sees whole-batch success. + let completed = { + let Ok(mut pending) = self.pending.lock() else { + return Ok(()); + }; + let Some(batch) = pending.as_mut() else { + return Ok(()); + }; + if result.error.is_some() { + batch.all_ok = false; + } + batch.remaining = batch.remaining.saturating_sub(1); + if batch.remaining == 0 { + pending.take() + } else { + None + } + }; + let Some(batch) = completed else { + return Ok(()); + }; + + // Repeat-CALL breaker for SUCCESSFUL no-op loops (#4088): the failure + // breaker owns repeated *failures* and resets on success, so an identical + // call that keeps SUCCEEDING slips past it. A failing batch (its domain) + // or an all-poll exemption resets the streak instead of recording. + if batch.exempt || !batch.all_ok { + if let Ok(mut g) = self.call_guard.lock() { + g.reset(); + } + return Ok(()); + } + let consecutive = self + .call_guard + .lock() + .map(|mut g| g.record(&batch.call_sig)) + .unwrap_or(0); + if consecutive >= REPEAT_CALL_THRESHOLD { + tracing::warn!( + consecutive, + "[tinyagents::mw] repeat-call circuit breaker tripped — identical successful (tool,args) batch repeated; halting" + ); + self.halt(format!( + "Stopping: the same tool call was issued {consecutive} times in a row with \ + identical arguments and no new information — the run is stuck repeating one \ + action without making progress. Re-issuing it will not help. Summarise what (if \ + anything) was actually accomplished and report that the task could not progress, \ + or take a genuinely different action (a different tool, different arguments, or \ + hand back).", + )); + } + Ok(()) + } +} + +// ── ImageAwareMessageTrimMiddleware ─────────────────────────────────────────── + +/// Flat token cost charged per image — an inline `[IMAGE:…]` marker or a native +/// [`ContentBlock::Image`] block — instead of counting the base64 payload as +/// text. Restores the legacy `harness/token_budget.rs` semantics (issue #4462): +/// the crate `estimate_tokens` prices text at chars/4, so a single large base64 +/// image reads as ~2M tokens and the trim believes the context is massively over +/// budget, evicting the whole transcript (system messages included). Providers +/// bill an image at ≈85–1100 tokens by detail; 1200 is a conservative upper +/// bound that keeps the budget realistic without the base64 payload inflating it. +const IMAGE_MARKER_TOKEN_COST: u64 = 1_200; + +/// Inline image-marker prefix produced by the multimodal composer +/// (`agent/multimodal.rs`, `compose_multimodal_message`). Priced at +/// [`IMAGE_MARKER_TOKEN_COST`] rather than by its base64 length. +const IMAGE_MARKER_PREFIX: &str = "[IMAGE:"; + +/// Minimum reply/output reserve — mirrors the legacy `MIN_OUTPUT_RESERVE_TOKENS`. +const MIN_OUTPUT_RESERVE_TOKENS: u64 = 512; + +/// Upper anchor for the reply/output reserve — mirrors the legacy +/// `DEFAULT_OUTPUT_RESERVE_TOKENS`. +const DEFAULT_OUTPUT_RESERVE_TOKENS: u64 = 8_192; + +/// Rough token estimate (~4 characters per token) with inline `[IMAGE:…]` +/// markers charged a flat [`IMAGE_MARKER_TOKEN_COST`] instead of their base64 +/// length. Mirrors the deleted `token_budget::estimate_tokens` (issue #4462). +/// Markerless text takes the fast char/4 path. +fn estimate_text_tokens(text: &str) -> u64 { + if !text.contains(IMAGE_MARKER_PREFIX) { + return (text.len() as u64).saturating_add(3) / 4; + } + let mut text_bytes: u64 = 0; + let mut images: u64 = 0; + let mut cursor = 0usize; + while let Some(rel) = text[cursor..].find(IMAGE_MARKER_PREFIX) { + let start = cursor + rel; + text_bytes = text_bytes.saturating_add((start - cursor) as u64); // preceding text + let after = start + IMAGE_MARKER_PREFIX.len(); + match text[after..].find(']') { + Some(rel_end) => { + images += 1; + cursor = after + rel_end + 1; // skip the whole marker payload + } + None => { + // Unterminated marker — count the remainder as text and stop. + text_bytes = text_bytes.saturating_add((text.len() - start) as u64); + cursor = text.len(); + break; + } + } + } + text_bytes = text_bytes.saturating_add((text.len() - cursor) as u64); // trailing text + (text_bytes.saturating_add(3) / 4) + .saturating_add(images.saturating_mul(IMAGE_MARKER_TOKEN_COST)) +} + +/// Count native [`ContentBlock::Image`] blocks on a message. `Message::text()` +/// concatenates only text blocks, so a native multimodal image would otherwise +/// contribute zero tokens; we charge each one [`IMAGE_MARKER_TOKEN_COST`]. +fn count_native_image_blocks(msg: &TaMessage) -> u64 { + let content = match msg { + TaMessage::System(m) => &m.content, + TaMessage::User(m) => &m.content, + TaMessage::Assistant(m) => &m.content, + TaMessage::Tool(m) => &m.content, + }; + content + .iter() + .filter(|b| matches!(b, ContentBlock::Image(_))) + .count() as u64 +} + +/// Estimate the tokens of a crate [`TaMessage`]: image-aware text tokens, a flat +/// [`IMAGE_MARKER_TOKEN_COST`] per native image block, and the assistant's +/// tool-call name/arguments (which `Message::text()` drops). Mirrors the legacy +/// `estimate_conversation_message_tokens` (issue #4462). +fn estimate_message_tokens(msg: &TaMessage) -> u64 { + let mut total = estimate_text_tokens(&msg.text()); + total = total + .saturating_add(count_native_image_blocks(msg).saturating_mul(IMAGE_MARKER_TOKEN_COST)); + if let TaMessage::Assistant(m) = msg { + for call in &m.tool_calls { + total = total.saturating_add(estimate_text_tokens(&call.name)); + total = total.saturating_add(estimate_text_tokens(&call.arguments.to_string())); + } + } + total +} + +/// Reply/output reserve, mirroring the legacy proportional clamp +/// `clamp(window/10, ≥512, ≤max(8192, window/4))`. Restores the small-window +/// budget the fixed `window − AGENT_TURN_MAX_OUTPUT_TOKENS` regressed (issue +/// #4462): an 8k model reserves ~819 tokens (input budget ~7373), not +/// 16384 → floored 1024. +fn legacy_output_reserve_tokens(window: u64) -> u64 { + let pct = window / 10; + pct.max(MIN_OUTPUT_RESERVE_TOKENS) + .min(DEFAULT_OUTPUT_RESERVE_TOKENS.max(window / 4)) +} + +/// Input-prompt token budget after reserving room for the reply. Public to the +/// seam so the install site (and tests) can assert the legacy proportional +/// formula (issue #4462). +pub(super) fn legacy_max_input_tokens(window: u64) -> u64 { + window.saturating_sub(legacy_output_reserve_tokens(window)) +} + +/// Deterministic history trim that replaces the crate `MessageTrimMiddleware` +/// (issue #4462), restoring three regression guards the crate trim lost: +/// +/// 1. **Image-aware token estimate** — inline `[IMAGE:…]` markers and native +/// image blocks are each charged a flat [`IMAGE_MARKER_TOKEN_COST`] instead +/// of their base64 length, so one large image can no longer read as ~2M +/// tokens and evict the whole transcript. +/// 2. **System messages never dropped** — only non-system history is evictable; +/// the crate trim reorders system messages to the front and drops them as a +/// last resort. +/// 3. **Order preserved + observable** — retained messages keep their original +/// relative order, leading orphaned tool results are snapped past (so no +/// provider 400), and any eviction logs a grep-able `warn` carrying +/// (messages dropped, messages/tokens before-and-after). +pub(crate) struct ImageAwareMessageTrimMiddleware { + /// Input-prompt token budget (already net of the proportional reply reserve). + budget: u64, +} + +impl ImageAwareMessageTrimMiddleware { + /// Build a trim middleware whose budget is the legacy proportional + /// [`legacy_max_input_tokens`] for `window` (issue #4462) — NOT the crate's + /// fixed `window − 16384`. Floored at 1 so the budget is always positive. + pub(crate) fn for_context_window(window: u64) -> Self { + Self { + budget: legacy_max_input_tokens(window).max(1), + } + } +} + +#[async_trait] +impl Middleware<()> for ImageAwareMessageTrimMiddleware { + fn name(&self) -> &str { + "image_aware_message_trim" + } + + async fn before_model( + &self, + _ctx: &mut RunContext<()>, + _state: &(), + request: &mut ModelRequest, + ) -> TaResult<()> { + let messages = &mut request.messages; + let original_tokens: u64 = messages.iter().map(estimate_message_tokens).sum(); + if original_tokens <= self.budget { + return Ok(()); + } + let original_len = messages.len(); + + // Evict oldest non-system messages first, preserving the relative order + // of every retained message (rebuilding as `system ++ other` would + // reorder history when a system message appears after non-system ones — + // exactly the crate-trim regression). System messages are NEVER dropped. + let mut removable_positions: Vec = messages + .iter() + .enumerate() + .filter_map(|(idx, m)| (!matches!(m, TaMessage::System(_))).then_some(idx)) + .collect(); + + let mut removed = 0usize; + while !removable_positions.is_empty() { + let total: u64 = messages.iter().map(estimate_message_tokens).sum(); + if total <= self.budget { + break; + } + let absolute_idx = removable_positions.remove(0); + // Subsequent positions shift left by one for every prior removal. + let remove_at = absolute_idx - removed; + messages.remove(remove_at); + removed += 1; + } + + // Snap the window forward past any leading orphaned tool results: dropping + // an `assistant(tool_calls)` while keeping its `tool` answer leaves the + // transcript opening on a tool message with no preceding tool-call, which + // native providers reject with a 400. Drop leading tool results until the + // first non-system message is a clean turn boundary. + while let Some(first_non_system) = messages + .iter() + .position(|m| !matches!(m, TaMessage::System(_))) + { + if matches!(messages[first_non_system], TaMessage::Tool(_)) { + messages.remove(first_non_system); + removed += 1; + } else { + break; + } + } + + if removed > 0 { + let final_tokens: u64 = messages.iter().map(estimate_message_tokens).sum(); + tracing::warn!( + messages_dropped = removed, + messages_before = original_len, + messages_after = messages.len(), + tokens_before = original_tokens, + tokens_after = final_tokens, + budget = self.budget, + "[tinyagents::mw] message_trim evicted oldest history to fit the token budget" + ); + } + Ok(()) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/openhuman/tinyagents/mod.rs b/src/openhuman/tinyagents/mod.rs index c2af63d36e..3133f9cb8d 100644 --- a/src/openhuman/tinyagents/mod.rs +++ b/src/openhuman/tinyagents/mod.rs @@ -19,6 +19,7 @@ //! `ask_user_clarification` early-exit pause are all re-wired onto the //! tinyagents harness. +mod abort_guard; mod convert; pub(crate) mod delegation; mod embeddings; @@ -33,6 +34,7 @@ pub(crate) mod replay; pub(crate) mod retriever; mod routes; mod run_cancellation_context; +mod steering_forwarder; pub(crate) mod stop_hooks; pub(crate) mod subagent_graph; mod summarize; @@ -45,16 +47,14 @@ use anyhow::Result; use tinyagents::harness::cache::InMemoryResponseCache; use tinyagents::harness::context::{RunConfig, RunContext}; use tinyagents::harness::events::EventSink; -use tinyagents::harness::message::Message as TaMessage; use tinyagents::harness::middleware::{ - BudgetLimits, BudgetMiddleware, ContextCompressionMiddleware, MessageTrimMiddleware, - PromptCacheGuardMiddleware, ToolPolicyMiddleware as TaToolPolicyMiddleware, + BudgetLimits, BudgetMiddleware, ContextCompressionMiddleware, PromptCacheGuardMiddleware, + ToolPolicyMiddleware as TaToolPolicyMiddleware, }; use tinyagents::harness::model::CapabilitySet; use tinyagents::harness::runtime::{AgentHarness, RunPolicy, UnknownToolPolicy}; -use tinyagents::harness::steering::{SteeringCommand, SteeringHandle}; +use tinyagents::harness::steering::SteeringHandle; use tinyagents::harness::store::StoreRegistry; -use tinyagents::harness::summarization::TrimStrategy; use tinyagents::harness::workspace::WorkspaceDescriptor; use tinyagents::registry::{ CapabilityRegistry, ComponentKind, DiagnosticSeverity, RegistryDiagnostic, RegistrySnapshot, @@ -70,11 +70,14 @@ use model::ThinkingForwarder; #[allow(unused_imports)] // Wired into the recall/retrieval facade in workstream 09.2. pub(crate) use embeddings::ProviderEmbeddingModel; -pub(crate) use middleware::{HandoffConfig, SuperContextConfig, TurnContextMiddleware}; +pub(crate) use middleware::{ + HandoffConfig, SuperContextConfig, TranscriptSnapshotSink, TurnContextMiddleware, +}; use model::ProviderModel; pub(crate) use observability::SubagentScope; use observability::{ - CapPauser, IterationCursor, OpenhumanEventBridge, ToolFailureMap, ToolNameMap, + CapPauser, IterationCursor, OpenhumanEventBridge, ProviderUsageCarry, ToolFailureMap, + ToolNameMap, }; pub(crate) use run_cancellation_context::{current_run_cancellation, with_run_cancellation}; #[cfg(test)] @@ -102,34 +105,6 @@ pub(crate) struct ToolPolicyEnforcement { pub agent_definition_id: String, } -/// Drain the run queue's pending steer messages and forward them to the -/// tinyagents [`SteeringHandle`] as injected user turns (the harness applies -/// them to the working transcript at the next iteration checkpoint). This is the -/// bridge behind the `steer_subagent` / mid-flight-steering feature. -async fn forward_steers(queue: &RunQueue, handle: &SteeringHandle) { - for msg in queue.drain_steers().await { - handle.send(SteeringCommand::InjectMessage(TaMessage::user(format!( - "[User steering message]: {}", - msg.text - )))); - } -} - -/// Forward any queued **collect** messages (orchestrator/monitor lines enqueued -/// via `QueueMode::Collect`) into the run as injected user turns so they reach the -/// next LLM call as additional context. The in-house loop drained these each -/// iteration (`drain_collects`); the tinyagents rewrite wired only `forward_steers` -/// (issue #4249), so monitor lines never reached the model. Mirrors the legacy -/// `[Additional context from user]:` framing the model was taught to read. -async fn forward_collects(queue: &RunQueue, handle: &SteeringHandle) { - for msg in queue.drain_collects().await { - handle.send(SteeringCommand::InjectMessage(TaMessage::user(format!( - "[Additional context from user]: {}", - msg.text - )))); - } -} - /// Build the harness [`RunPolicy`] for an openhuman turn. /// /// The loop enforces limits from `self.policy.limits` (not the per-run @@ -242,6 +217,13 @@ pub(crate) struct TinyagentsTurnOutcome { /// should summarize a resumable checkpoint rather than treat `text` as a /// final answer — the tinyagents analogue of the legacy cap checkpoint seam. pub hit_cap: bool, + /// Set (with the root-cause halt summary) when the repeated-tool-failure / + /// repeat-progress circuit breaker halted the run before a natural finish. + /// The sub-agent runner surfaces this as `SubagentRunStatus::Incomplete` + /// (#4466) so a parent does NOT treat a halted child as a clean completion. + /// `text` already carries this same summary; the flag lets the status mapper + /// distinguish a breaker halt from a genuine final answer. + pub breaker_halt: Option, /// Per-tool-call execution outcomes (success + raw result content), keyed by /// provider call id, captured at the tool boundary. The harness folds a tool /// result into a `Message::tool` that drops its `error` flag, so this is the @@ -321,11 +303,28 @@ pub(crate) async fn run_turn_via_tinyagents( ); let input = convert::history_to_messages(&history); + // Explicit persistence boundary (issue #4455): the request transcript length, + // captured *before* the run consumes `input`. Everything the harness appends + // after this index — assistant/tool rounds plus any mid-turn steer messages — + // is this turn's persisted `conversation`. Anchoring on this index instead of + // the last-user-message suffix keeps injected steers (which move that + // boundary) from truncating persisted history. + let request_base_len = input.len(); // Box the (large) harness drive future — see `run_turn_via_tinyagents_shared`. let run = match Box::pin(harness.invoke(&(), (), config, input)).await { Ok(run) => run, Err(e) => { - if let Some(original) = error_slot.lock().unwrap().take() { + // #4469 item 3: recover from a poisoned slot instead of panicking. + // A thread that panicked mid-run while holding this mutex would + // otherwise turn every subsequent error-recovery read into a second + // panic, masking the original provider failure. `into_inner` yields + // the guarded value regardless of poison so we still re-surface the + // typed error. + if let Some(original) = error_slot + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .take() + { return Err(original); } return Err(anyhow::anyhow!("tinyagents harness run failed: {e}")); @@ -334,8 +333,16 @@ pub(crate) async fn run_turn_via_tinyagents( let text = run.text().unwrap_or_default(); let out_history = convert::messages_to_history(&run.messages); - let conversation = - convert::messages_to_conversation(convert::messages_since_last_user(&run.messages)); + let conversation = convert::messages_to_conversation(convert::messages_since_request( + &run.messages, + request_base_len, + )); + tracing::debug!( + request_base_len, + transcript_len = run.messages.len(), + persisted_messages = run.messages.len().saturating_sub(request_base_len), + "[tinyagents] persisting post-request transcript (thin path; steer-safe boundary)" + ); Ok(TinyagentsTurnOutcome { text, @@ -354,6 +361,8 @@ pub(crate) async fn run_turn_via_tinyagents( ), early_exit_tool: None, hit_cap: false, + // This thin (test-only) variant does not install the breaker middleware. + breaker_halt: None, // This thin variant carries no per-call outcome capture middleware. tool_outcomes: Vec::new(), }) @@ -368,8 +377,14 @@ pub(crate) async fn run_turn_via_tinyagents( /// advertised spec so the same `Arc`-shared tools the legacy loop runs are /// reused without cloning. /// -/// `allowed` is the callable tool-name whitelist (empty = every tool visible in -/// `tool_sets`); each callable tool is advertised via its own `spec()`. +/// `allowed` is the callable tool-name whitelist. Its semantics are +/// **fail-closed** (issue #4452): `None` means "no filter supplied" → every tool +/// visible in `tool_sets` is registered; `Some(set)` registers *exactly* the +/// named tools, so `Some(empty)` is an explicit **deny-all** (zero tools). This +/// distinction is what stops a tool-less sub-agent (`ToolScope::Named([])`, a +/// zero-match `skill_filter`, or a `named` list that resolves to nothing) from +/// silently inheriting the parent's full tool surface (shell/file-write/spawn). +/// Each registered tool is advertised via its own `spec()`. /// /// When `on_progress` is `Some`, the run streams (`invoke_streaming_in_context`) /// and a [`OpenhumanEventBridge`] mirrors the harness event stream onto @@ -378,13 +393,29 @@ pub(crate) async fn run_turn_via_tinyagents( /// produced. Pass `None` for fire-and-forget turns (channel/sub-agent) that /// only need the final text. /// -/// When `context_window` is known, a [`MessageTrimMiddleware`] keeps history -/// under budget (autocompaction parity). +/// When `context_window` is known, an +/// [`ImageAwareMessageTrimMiddleware`](middleware::ImageAwareMessageTrimMiddleware) +/// keeps history under budget (autocompaction parity). /// /// `run_queue` forwards mid-flight steer messages into the run; `subagent_scope` /// re-scopes progress to the `Subagent*` variants (child runs); `early_exit_tools` /// name the tools that pause the loop (e.g. `ask_user_clarification`) and surface /// the question via [`TinyagentsTurnOutcome::early_exit_tool`]. +/// True when `name` is a sub-agent spawn/delegation tool that a **child** run +/// must never be able to invoke (issue #4452). Mirrors the caller-side strip in +/// `subagent_runner::tool_prep::is_subagent_spawn_tool` plus the worker-thread +/// spawn, re-asserted at registration as defense-in-depth so a misconfigured +/// allowlist cannot reintroduce sub-agent spawning into a nested run. Kept local +/// to this seam (rather than importing the `pub(super)` runner helper) so the +/// invariant travels with the registration site that enforces it. +fn is_subagent_spawn_or_delegate_tool(name: &str) -> bool { + name == "spawn_subagent" + || name.starts_with("delegate_") + || name == "use_tinyplace" + || name == "agent_prepare_context" + || name == "spawn_worker_thread" +} + #[allow(clippy::too_many_arguments)] pub(crate) async fn run_turn_via_tinyagents_shared( provider: Arc, @@ -392,7 +423,7 @@ pub(crate) async fn run_turn_via_tinyagents_shared( temperature: f64, history: Vec, tool_sets: Vec>>>, - allowed: HashSet, + allowed: Option>, max_iterations: usize, on_progress: Option>, subagent_scope: Option, @@ -405,6 +436,15 @@ pub(crate) async fn run_turn_via_tinyagents_shared( tool_policy: Option, workspace_descriptor: Option, deterministic_cacheable: bool, + // #4457 (defect C): when `true`, the seam does NOT emit the terminal + // `TurnCompleted` — the caller emits it itself *after* its post-run wrap-up + // (e.g. the chat/session path streams a cap/#4093 checkpoint via + // `summarize_turn_wrapup` after this seam returns, so a seam-level emit here + // would land `turn_active = false` before that checkpoint finishes + // streaming, and the web bridge would record two ledger events + two + // Completed upserts). Callers with no post-run streaming (channel/CLI) pass + // `false` and rely on this seam's emit for parity with the legacy engine. + defer_turn_completed_to_caller: bool, ) -> Result { // `0` means "unset" → the legacy default (a native-bus / test convention); // otherwise the harness model-call cap would be zero and abort the run before @@ -415,6 +455,7 @@ pub(crate) async fn run_turn_via_tinyagents_shared( cursor, tool_names, failure_map, + provider_usage_carry, error_slot, halt_summary, tool_outcome_sink, @@ -513,6 +554,14 @@ pub(crate) async fn run_turn_via_tinyagents_shared( ); let input = convert::history_to_messages(&history); + // Explicit persistence boundary (issue #4455): the request transcript length, + // captured *before* the run consumes `input`. The turn's persisted + // `conversation` is everything appended past this index — assistant/tool + // rounds plus any mid-turn steer/collect messages injected as user turns. + // Anchoring here (instead of the last-user-message suffix) keeps injected + // steers from moving the boundary and truncating persisted history on both + // the parent (`session/turn/core.rs`) and subagent (`subagent_runner`) paths. + let request_base_len = input.len(); // Build the run context: an optional event sink feeds the progress/cost // bridge (streaming) and/or the model-call-cap pauser; the shared steering @@ -561,8 +610,11 @@ pub(crate) async fn run_turn_via_tinyagents_shared( // `TurnCompleted` after the run (the harness event stream the bridge mirrors // has no run-completed event). Parent turns only — a sub-agent turn reports // via its `Subagent*` events, not a top-level `TurnCompleted`. - let turn_completed_sink = subagent_scope - .is_none() + // + // #4457 (defect C): suppressed entirely when `defer_turn_completed_to_caller` + // is set — the caller (chat/session path) emits the single terminal + // `TurnCompleted` itself, after its post-run wrap-up finishes streaming. + let turn_completed_sink = (subagent_scope.is_none() && !defer_turn_completed_to_caller) .then(|| on_progress.clone()) .flatten(); // A sink is needed to mirror progress (bridge), to observe model-call @@ -579,22 +631,30 @@ pub(crate) async fn run_turn_via_tinyagents_shared( let journal_run_id = journal::mint_run_id(); let events = Some(EventSink::with_stream_id(journal_run_id.as_str())); - let bridge = match (&events, on_progress) { - (Some(events), Some(tx)) => { - let bridge = OpenhumanEventBridge::with_scope( - Some(tx), - model, - max_iterations, - subagent_scope.clone(), - cursor.clone(), - tool_names.clone(), - failure_map.clone(), - ); - events.subscribe(bridge.clone()); - Some(bridge) - } - _ => None, - }; + // Attach the event bridge for EVERY turn — including an unobserved + // (`on_progress = None`) background/cron turn (#4467, item 3). The bridge's + // `record_usage` feeds the global cost tracker on each `UsageRecorded` event + // *during* the run, so a run that burns N model calls and then fails still + // contributes that spend to the wallet/cost surfaces — the post-run + // `record_unobserved_turn_usage` fallback below only runs on the success path + // and never sees a failed run's usage. With `on_progress = None` the bridge + // still records cost but its progress `send`s are inert no-ops, so there is + // no spurious streaming. `events` is created unconditionally above, so the + // bridge is always present. + let bridge = events.as_ref().map(|events| { + let bridge = OpenhumanEventBridge::with_scope( + on_progress, + model, + max_iterations, + subagent_scope.clone(), + cursor.clone(), + tool_names.clone(), + failure_map.clone(), + provider_usage_carry.clone(), + ); + events.subscribe(bridge.clone()); + bridge + }); // Cap pauser: stop gracefully at the model-call budget (returning the partial // transcript) so the caller can summarize a checkpoint instead of erroring. @@ -629,33 +689,62 @@ pub(crate) async fn run_turn_via_tinyagents_shared( // Steering: attach the shared handle (when present), drain any already-queued // steer messages into it (so a pre-run steer lands before the first model - // call), and forward mid-flight steers via a poller aborted when the run - // returns. The same handle carries the early-exit `Pause`. - let mut registered_steering_task_id = None; - let steering_forwarder = if let Some(handle) = handle { - if let Some(scope) = &subagent_scope { + // call), and forward mid-flight steers via a poll loop. The same handle + // carries the early-exit `Pause`. + // + // Best-effort thread label for the delivery/requeue observability events and + // the metadata on any requeued steer: a sub-agent uses its task id; the + // interactive/channel parent turn reads the task-local turn origin. + let steer_thread_label = subagent_scope + .as_ref() + .map(|s| s.task_id.clone()) + .or_else(|| match crate::openhuman::agent::turn_origin::current() { + Some(crate::openhuman::agent::turn_origin::AgentTurnOrigin::WebChat { + thread_id, + .. + }) => Some(thread_id), + Some(crate::openhuman::agent::turn_origin::AgentTurnOrigin::ExternalChannel { + reply_target, + .. + }) => Some(reply_target), + _ => None, + }) + .unwrap_or_default(); + + // The forwarder is wrapped in an abort-on-drop RAII guard (issue #4456): its + // `Drop` aborts the poll task, deregisters the sub-agent steering handle, and + // drains residual (delivered-but-unapplied) steers back into the session run + // queue. Because the guard is held across the drive future, that cleanup runs + // identically on normal return, error, AND drop-cancellation — the previous + // manual `forwarder.abort()` after the drive future only ran on normal + // return, so a cancelled turn (web interrupt / sub-agent abort, both + // drop-based) leaked a forwarder task that looped forever and raced the next + // turn for the shared run queue. + let steering_forwarder_guard = if let Some(handle) = handle { + let registry_task_id = if let Some(scope) = &subagent_scope { let task_id = orchestration::TaskId::new(scope.task_id.clone()); orchestration::shared_steering_registry().register(task_id.clone(), handle.clone()); tracing::debug!( task_id = scope.task_id.as_str(), "[tinyagents] registered subagent steering handle" ); - registered_steering_task_id = Some(task_id); - } + Some(task_id) + } else { + None + }; + // Pre-run drain so a steer/collect queued before the turn started lands + // ahead of the first model call. if let Some(queue) = run_queue.clone() { - forward_steers(&queue, &handle).await; - forward_collects(&queue, &handle).await; + steering_forwarder::forward_steers(&queue, &handle, &steer_thread_label).await; + steering_forwarder::forward_collects(&queue, &handle, &steer_thread_label).await; } ctx = ctx.with_steering(handle.clone()); - run_queue.map(|queue| { - tokio::spawn(async move { - loop { - tokio::time::sleep(std::time::Duration::from_millis(50)).await; - forward_steers(&queue, &handle).await; - forward_collects(&queue, &handle).await; - } - }) - }) + Some(steering_forwarder::SteeringForwarderGuard::new( + handle, + run_queue, + registry_task_id, + steer_thread_label.clone(), + )) } else { None }; @@ -673,16 +762,12 @@ pub(crate) async fn run_turn_via_tinyagents_shared( } }) .await; - if let Some(forwarder) = steering_forwarder { - forwarder.abort(); - } - if let Some(task_id) = registered_steering_task_id { - orchestration::shared_steering_registry().deregister(&task_id); - tracing::debug!( - task_id = task_id.as_str(), - "[tinyagents] deregistered subagent steering handle" - ); - } + // Drive future returned: run cleanup now (abort poll task + deregister + + // requeue residual steers) rather than deferring to end-of-scope so the poll + // loop cannot deliver into the no-longer-drained handle during post-run + // journal/mapping work. On a *cancelled* turn this line is never reached; the + // guard's `Drop` fires as the turn future unwinds, giving identical cleanup. + drop(steering_forwarder_guard); let run = match run_result { Ok(run) => run, Err(e) => { @@ -691,11 +776,18 @@ pub(crate) async fn run_turn_via_tinyagents_shared( if let Some(journal) = &turn_journal { journal.finish_failed(&e.to_string()).await; } - // Prefer the original typed provider error (preserves `AgentError` - // downcasts the caller relies on) over the harness's string wrap. - if let Some(original) = error_slot.lock().unwrap().take() { - return Err(original); - } + // #4457 (defect B): map the run's *own* definitively-non-provider + // failure kinds FIRST, before consulting `error_slot`. The slot + // preserves the last provider error the model adapter saw — but the + // adapter now clears it on every successful call (see + // `ProviderModel::chat`/`stream`), so a stale slot should not exist + // here. Ordering the cap/depth mappings ahead of the slot is + // defense-in-depth: a run that failed on the model-call cap or a + // spawn-depth limit is not a provider error, so it must surface as + // `MaxIterationsExceeded` / the depth error rather than a leftover + // provider error (wrong classification, wrong Sentry suppression, + // wrong user message). + // // The model-call cap (when not pausing gracefully — the channel/CLI // path) maps to the typed `AgentError::MaxIterationsExceeded` so // callers downcast it (Sentry skip) and render the canonical @@ -703,6 +795,10 @@ pub(crate) async fn run_turn_via_tinyagents_shared( // legacy `ErrorCheckpoint`. if let tinyagents::TinyAgentsError::LimitExceeded(msg) = &e { if msg.contains("model call") { + tracing::debug!( + model, + "[tinyagents] run hit the model-call cap; mapping to MaxIterationsExceeded (not consulting error_slot) — #4457 defect B" + ); return Err(anyhow::Error::new( crate::openhuman::agent::error::AgentError::MaxIterationsExceeded { max: max_iterations, @@ -713,6 +809,24 @@ pub(crate) async fn run_turn_via_tinyagents_shared( if let Some(depth_err) = tinyagents_depth_error(&e) { return Err(anyhow::Error::new(depth_err)); } + // Otherwise prefer the original typed provider error (preserves + // `AgentError` downcasts the caller relies on) over the harness's + // string wrap — this is where a genuine model/provider failure that + // halted the run is re-surfaced with its real classification. + // #4469 item 3: `into_inner` recovers a poisoned slot so a panic in + // one run can't cascade into a second panic here that would mask the + // original typed provider error. + if let Some(original) = error_slot + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .take() + { + tracing::debug!( + model, + "[tinyagents] re-surfacing typed provider error from error_slot as the run failure — #4457 defect B" + ); + return Err(original); + } return Err(anyhow::anyhow!("tinyagents harness run failed: {e}")); } }; @@ -781,6 +895,10 @@ pub(crate) async fn run_turn_via_tinyagents_shared( // Terminal turn event (parity with the legacy engine's `progress::emit`): the // harness stream has no run-completed event, so emit `TurnCompleted` here with // the model-call count as the iteration total. Parent turns only; best-effort. + // `turn_completed_sink` is `None` for sub-agent turns AND when the caller + // opted to emit the terminal event itself after its post-run wrap-up + // (`defer_turn_completed_to_caller`, #4457 defect C) — so this is the single + // emission point for callers with no post-run streaming (channel/CLI). if let Some(sink) = &turn_completed_sink { let _ = sink.try_send(AgentProgress::TurnCompleted { iterations: run.model_calls as u32, @@ -858,8 +976,17 @@ pub(crate) async fn run_turn_via_tinyagents_shared( None => (None, run.text().unwrap_or_default()), }; - if let Some(summary) = breaker_halt { - text = summary; + // Carry the breaker halt onto the outcome so the sub-agent runner can report + // `Incomplete` (#4466). `text` is overridden with the same root-cause summary + // so callers with no breaker-awareness still surface the cause, not an empty + // last-model reply. + if let Some(summary) = &breaker_halt { + tracing::info!( + model, + subagent = subagent_scope.is_some(), + "[tinyagents] run halted by circuit breaker; surfacing as breaker_halt (#4466)" + ); + text = summary.clone(); } let tool_outcomes = tool_outcome_sink @@ -867,12 +994,23 @@ pub(crate) async fn run_turn_via_tinyagents_shared( .map(|guard| guard.clone()) .unwrap_or_default(); + let conversation = convert::messages_to_conversation(convert::messages_since_request( + &run.messages, + request_base_len, + )); + tracing::debug!( + model, + request_base_len, + transcript_len = run.messages.len(), + persisted_messages = run.messages.len().saturating_sub(request_base_len), + subagent = subagent_scope.is_some(), + "[tinyagents] persisting post-request transcript (shared path; steer-safe boundary)" + ); + Ok(TinyagentsTurnOutcome { text, history: convert::messages_to_history(&run.messages), - conversation: convert::messages_to_conversation(convert::messages_since_last_user( - &run.messages, - )), + conversation, model_calls: run.model_calls, tool_calls: run.tool_calls, input_tokens, @@ -881,6 +1019,7 @@ pub(crate) async fn run_turn_via_tinyagents_shared( charged_amount_usd, early_exit_tool, hit_cap, + breaker_halt, tool_outcomes, }) } @@ -916,10 +1055,15 @@ struct AssembledTurnHarness { /// writes it on tool-call start; the event bridge reads it to label the /// tool-argument fragments it now projects off the crate stream. tool_names: ToolNameMap, - /// Shared `call_id → (success, failure)` side-channel: the tool-outcome - /// capture middleware classifies each outcome; the event bridge reads it to - /// project real success + a user-facing failure onto `ToolCallCompleted`. + /// Shared `call_id → (success, failure, elapsed_ms, output_chars)` + /// side-channel: the tool-outcome capture middleware classifies each outcome + /// + records its duration/output size; the event bridge reads it to project + /// real success + a user-facing failure + timing onto `ToolCallCompleted`. failure_map: ToolFailureMap, + /// Shared FIFO carry of per-call provider `UsageInfo` (charged USD + context + /// window): the model adapter pushes, the event bridge pops when recording + /// usage — restores charged-USD precedence on the tinyagents path (#4467). + provider_usage_carry: ProviderUsageCarry, /// Recovers the original (downcastable) provider error on run failure. error_slot: crate::openhuman::tinyagents::model::ProviderErrorSlot, /// Root-cause summary recorded by the repeated-tool-failure breaker. @@ -965,7 +1109,7 @@ fn assemble_turn_harness( model: &str, temperature: f64, tool_sets: Vec>>>, - allowed: HashSet, + allowed: Option>, max_iterations: usize, on_progress: Option>, subagent_scope: Option, @@ -1013,10 +1157,16 @@ fn assemble_turn_harness( // tool-call start (the crate `ToolDelta` carries none), the bridge reads it // to label the argument fragments now streamed via `MessageDelta.tool_call`. let tool_names: ToolNameMap = Arc::default(); + // Shared FIFO carry of per-call provider `UsageInfo`: the model adapter + // pushes each successful response's usage (charged USD + context window + + // cache-creation/reasoning tokens the crate `Usage` drops), the event bridge + // pops it when recording that call's usage (#4467, item 1). + let provider_usage_carry: ProviderUsageCarry = Arc::default(); // Keep a provider handle for the context-window summarizer (the run consumes // the other clone into the `ProviderModel`). let summary_provider = provider.clone(); - let mut provider_model = ProviderModel::new(provider, model, temperature); + let mut provider_model = ProviderModel::new(provider, model, temperature) + .with_usage_carry(provider_usage_carry.clone()); // Cap the model's per-call output budget (parity with the legacy engine, // which bounded the main agent at `AGENT_TURN_MAX_OUTPUT_TOKENS` and each // sub-agent at its `max_turn_output_tokens`). Without this the tinyagents @@ -1140,6 +1290,19 @@ fn assemble_turn_harness( ))); } + // Repeat-progress breaker (issue #4463, restoring #4088 / #4095): the failure + // breaker above resets on every success, so a model looping on a *successful* + // no-op tool or re-emitting an identical narration+call never trips it. This + // guard halts on identical successful `(tool, args)` batches / identical + // outputs, sharing the same halt-summary slot + steering handle. Polling tools + // (`wait_subagent`) stay exempt. + if let Some(handle) = &handle { + harness.push_middleware(Arc::new(middleware::RepeatProgressMiddleware::new( + handle.clone(), + halt_summary.clone(), + ))); + } + // Policy-driven stop hooks (budget cap, thread-goal budget, ad-hoc iteration // ceiling): fire after each model call and pause the run on the first stop // vote. Replaces the legacy tool-call-loop firing point. @@ -1162,7 +1325,23 @@ fn assemble_turn_harness( .map(|h| EarlyExitHook::new(h.clone())); // Register one adapter per unique callable tool name found across the shared - // sets (newest set wins on a name clash; `allowed` empty = all visible). + // sets (newest set wins on a name clash). Allowlist semantics are + // **fail-closed** (issue #4452): `allowed == None` → no filter, every visible + // tool registers; `allowed == Some(set)` → register *exactly* the named + // tools, so `Some(empty)` denies all. This is what keeps a deliberately + // tool-less sub-agent (`ToolScope::Named([])`, a zero-match `skill_filter`, + // or a `named` list that resolves to nothing) from silently inheriting the + // parent's full tool surface (shell/file-write/spawn) — the old + // `allowed.is_empty() || allowed.contains(name)` predicate was fail-open. + let is_subagent_run = subagent_scope.is_some(); + if let Some(set) = &allowed { + if set.is_empty() { + tracing::warn!( + subagent = is_subagent_run, + "[subagent] tool allowlist resolved empty — registering no tools" + ); + } + } let mut seen_candidates: HashSet = HashSet::new(); let candidate_names: Vec = tool_sets .iter() @@ -1176,7 +1355,25 @@ fn assemble_turn_harness( .collect(); let mut registered: HashSet = HashSet::new(); for name in candidate_names.iter().map(String::as_str) { - if !registered.contains(name) && (allowed.is_empty() || allowed.contains(name)) { + // Fail-closed allowlist: `None` admits everything, `Some(set)` admits only + // its members (empty set → nothing). + let admitted = match &allowed { + None => true, + Some(set) => set.contains(name), + }; + // Defense-in-depth (issue #4452): a sub-agent must NEVER be handed a + // spawn/delegate tool, regardless of what the resolved allowlist contains. + // Re-assert the invariant here at registration time (not just on the + // caller's `allowed_indices`) so a misbuilt allowlist can't reintroduce + // `spawn_subagent`/`delegate_*`/worker-thread spawning into a child run. + let spawn_stripped = is_subagent_run && is_subagent_spawn_or_delegate_tool(name); + if spawn_stripped { + tracing::warn!( + tool = name, + "[subagent] refusing to register spawn/delegate tool on sub-agent run" + ); + } + if !registered.contains(name) && admitted && !spawn_stripped { if let Some(mut adapter) = SharedToolAdapter::for_name(tool_sets.clone(), name) { if early_exit_set.contains(name) { if let Some(hook) = &early_exit_hook { @@ -1310,7 +1507,7 @@ fn assemble_turn_harness( harness.push_middleware(Arc::new( middleware::OpenHumanToolExposureShadowMiddleware::new( &candidate_names, - &allowed, + allowed.as_ref(), exposure_tags, ), )); @@ -1336,6 +1533,26 @@ fn assemble_turn_harness( // by the crate `PromptCacheGuardMiddleware`; the warn-only CacheAlign shadow // was deleted in C3.) Tool-result caps read the SDK registry policy snapshot, // not the OpenHuman-side tool lookup. + // Capture each tool call's real success + content before the harness folds the + // result into a `Message::tool` that drops the failure flag, so the turn can + // build honest per-call `ToolCallRecord`s (post-turn hooks + cap checkpoint). + // + // REVERSE-ORDER RULE (issue #4464): the crate runs `after_tool` in REVERSE + // registration order, so the LATER a middleware is pushed the EARLIER its + // `after_tool` runs. This capture must observe the FINAL (summarized/capped) + // content, so it is pushed BEFORE `context_mw.install` (which registers the + // handoff + tool-output budget/caps) — that way its `after_tool` runs AFTER + // those caps, not before. Registering it after `install` (the pre-#4464 bug) + // made its `after_tool` run first and record the full raw payload of every + // call, bloating the per-turn sink and feeding failure classification / + // `ToolCallRecord.output_summary` pre-cap content. + let tool_outcome_sink: ToolOutcomeSink = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let failure_map: ToolFailureMap = Arc::default(); + harness.push_middleware(Arc::new(middleware::ToolOutcomeCaptureMiddleware::new( + tool_outcome_sink.clone(), + failure_map.clone(), + ))); + let tool_policies = harness.tools().policies(); context_mw.install(&mut harness, tool_policies); @@ -1394,10 +1611,12 @@ fn assemble_turn_harness( // transcript into a single LLM-generated system summary (keeping system // messages + the recent window verbatim). This is keyed to whatever model // the turn is running on, preserving the legacy context threshold. - // 2. `MessageTrimMiddleware` — a deterministic, no-extra-LLM-call hard cap. + // 2. `ImageAwareMessageTrimMiddleware` — a deterministic, no-extra-LLM-call + // hard cap (issue #4462; replaces the crate `MessageTrimMiddleware`). // Pushed **after** compression (so `before_model` runs compression first), - // it front-trims to budget only as a last resort when even the summary + - // recent window still overflow. + // it front-trims to the legacy proportional budget only as a last resort + // when even the summary + recent window still overflow — image markers + // priced flat, system messages never dropped, evictions logged. // // The LLM summarization step honors the `[context].enabled` / // `autocompact_enabled` opt-outs (a disabled config must not spend summarizer @@ -1411,24 +1630,40 @@ fn assemble_turn_harness( let mut compression_mw: Option> = None; if let Some(window) = context_window.filter(|w| *w > 0) { if autocompact_enabled { - let mw = Arc::new(ContextCompressionMiddleware::with_summarizer( - summarize::summarization_policy(window), + let policy = summarize::summarization_policy(window); + // Wrap the LLM-backed summarizer in a fault-tolerant, per-turn-caching + // adapter (issue #4461): a summarizer failure must no longer abort the + // turn (warn + circuit-breaker + deterministic trim instead), and an + // identical re-issued input slice must not re-run the summarizer LLM. + let summarizer = summarize::FaultTolerantCachingSummarizer::new( Box::new(summarize::ProviderModelSummarizer::new( summary_provider, model, temperature, )), + &policy, + ); + let mw = Arc::new(ContextCompressionMiddleware::with_summarizer( + policy, + Box::new(summarizer), )); harness.push_middleware(mw.clone()); compression_mw = Some(mw); } - let budget = window.saturating_sub( - crate::openhuman::inference::provider::AGENT_TURN_MAX_OUTPUT_TOKENS as u64, - ); - harness.push_middleware(Arc::new(MessageTrimMiddleware::new( - TrimStrategy::MaxTokens(budget.max(1024)), - ))); + // Deterministic hard-cap trim (issue #4462). The crate + // `MessageTrimMiddleware` regressed three legacy `token_budget.rs` + // guards: it priced a base64 image at ~2M tokens (chars/4) and could + // evict system messages, it reordered system messages to the front, and + // its budget was the fixed `window − AGENT_TURN_MAX_OUTPUT_TOKENS` + // (floored 1024) that collapses an 8k local model's input budget from + // ~7373 to 1024. Our seam-owned `ImageAwareMessageTrimMiddleware` + // restores all three: image markers priced at a flat cost, the + // proportional reply reserve, system messages always kept in place, and a + // grep-able warn with drop/token counts on any eviction. + harness.push_middleware(Arc::new( + middleware::ImageAwareMessageTrimMiddleware::for_context_window(window), + )); } // SDK-owned tool-policy projection (issue #4249 / tinyagents-full-migration @@ -1442,6 +1677,20 @@ fn assemble_turn_harness( TaToolPolicyMiddleware::new(harness.tools().policies()).require_sandbox(true), )); + // Schema-guard (issue #4451): the crate runs a **fatal** JSON-schema gate on + // every tool call between `before_tool` and the tool-wrap onion — a missing + // required field / wrong type / bad enum returns `TinyAgentsError::Validation` + // and aborts the whole turn (`chat_error`). This middleware re-runs the same + // validation in `before_tool`; on failure it records a descriptive error and + // rewrites the args to a schema-satisfying stub (so the crate gate passes), + // then its `wrap_tool` hook short-circuits the flagged call with a synthetic + // failed `ToolResult` before the stub can reach the tool — restoring the + // legacy engine's "bad args → recoverable tool error the model self-corrects + // on" behaviour. Installed as the **outermost** tool wrap so an invalid call + // becomes a tool error before approval/policy wraps ever see the stub args. + let schema_guard = Arc::new(middleware::SchemaGuardMiddleware::new(tool_sets.clone())); + harness.push_tool_middleware(schema_guard.clone()); + // Human-in-the-loop approval as a named tool middleware (issue #4249, // Phase 1): an external-effect tool intercepts through the global // `ApprovalGate`, a denial short-circuits with a model-consumable result, and @@ -1458,16 +1707,6 @@ fn assemble_turn_harness( tool_sets.clone(), ))); - // Capture each tool call's real success + content before the harness folds the - // result into a `Message::tool` that drops the failure flag, so the turn can - // build honest per-call `ToolCallRecord`s (post-turn hooks + cap checkpoint). - let tool_outcome_sink: ToolOutcomeSink = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); - let failure_map: ToolFailureMap = Arc::default(); - harness.push_middleware(Arc::new(middleware::ToolOutcomeCaptureMiddleware::new( - tool_outcome_sink.clone(), - failure_map.clone(), - ))); - // Builder-configured tool policy (`.tool_policy()`), enforced at the tool // boundary. The in-house engine ran this in `agent_tool_exec`; the tinyagents // path bypassed it, so a deny/require-approval silently no-opped (security @@ -1484,17 +1723,40 @@ fn assemble_turn_harness( ))); } - // Malformed-argument recovery (`before_tool`): coerce a call's non-object - // arguments (invalid JSON parses to Null) to `{}` so a single bad tool call is - // recoverable — the harness would otherwise reject it against an object schema - // and abort the whole turn. Engine parity. - harness.push_middleware(Arc::new(middleware::ArgRecoveryMiddleware)); + // Credential scrubbing (issue #4453): redact credential-shaped secrets out of + // every tool result. The legacy engine ran `scrub_credentials` over every + // tool output before it entered model context; the tinyagents path dropped + // that call site. Installed as the **innermost** tool wrap (pushed last) so + // it scrubs the RAW tool result before any outer wrap, the `after_tool` + // chain (summarization/caps), the transcript push, or the tool-outcome + // capture sink can observe the unredacted content — covering the parent, + // sub-agent, persisted-transcript, and `ToolCallOutcome` surfaces by + // construction since every path shares this seam. + harness.push_tool_middleware(Arc::new(middleware::CredentialScrubMiddleware::new())); + + // Malformed-argument recovery (`before_tool`): repair a call's non-object + // arguments before the crate's schema gate — decode JSON-encoded-string args + // (optionally markdown-fenced) to an object, or coerce to `{}` only when the + // tool schema has no required fields (engine parity). A non-object against a + // required-field schema is left untouched so the schema-guard tool-error path + // handles it instead of forcing a fatal `" is required"` abort. Runs + // before `SchemaGuardMiddleware::before_tool` (registered next) validates. + harness.push_middleware(Arc::new(middleware::ArgRecoveryMiddleware::new( + tool_sets.clone(), + ))); + + // Schema-guard `before_tool` (see the tool-wrap registration above): runs the + // crate's schema validation and, on failure, flags the call + stubs its args + // so the fatal gate passes and `wrap_tool` can short-circuit it. Registered + // last so it validates the arguments `ArgRecoveryMiddleware` just repaired. + harness.push_middleware(schema_guard); AssembledTurnHarness { harness, cursor, tool_names, failure_map, + provider_usage_carry, error_slot, halt_summary, tool_outcome_sink, diff --git a/src/openhuman/tinyagents/model.rs b/src/openhuman/tinyagents/model.rs index 91b18ee910..31341fb78b 100644 --- a/src/openhuman/tinyagents/model.rs +++ b/src/openhuman/tinyagents/model.rs @@ -19,10 +19,13 @@ use tinyagents::harness::tool::{ToolCall as TaToolCall, ToolDelta}; use tinyagents::harness::usage::Usage; use tokio::sync::mpsc::{Sender, UnboundedSender}; -use super::observability::{IterationCursor, SubagentScope, ToolNameMap}; +use super::abort_guard::AbortOnDrop; +use super::observability::{IterationCursor, ProviderUsageCarry, SubagentScope, ToolNameMap}; use crate::openhuman::agent::progress::AgentProgress; +use crate::openhuman::inference::provider::thread_context::{current_thread_id, with_thread_id}; use crate::openhuman::inference::provider::{ - ChatMessage, ChatRequest, ChatResponse, Provider, ProviderDelta, + current_route_slot, with_route_slot, ChatMessage, ChatRequest, ChatResponse, Provider, + ProviderDelta, }; use crate::openhuman::tools::ToolSpec; @@ -149,6 +152,31 @@ fn build_chat_inputs( (messages, specs) } +/// Build a [`PFormatRegistry`](crate::openhuman::agent::pformat::PFormatRegistry) +/// from the tool schemas advertised on a [`ModelRequest`] (issue #4465). +/// +/// The text-mode fallback parse needs each tool's positional parameter layout +/// to reconstruct named JSON arguments from a P-Format `name[a|b]` body. The +/// harness always populates `request.tools` (schemas are rendered into the +/// prompt for prompt-guided providers, or advertised natively otherwise), so +/// the registry is available in both modes. An empty registry (no tools +/// advertised) makes the P-Format-aware parser short-circuit to the canonical +/// grammar, so this is behaviour-neutral when there are no tools. +fn pformat_registry_from_request( + request: &ModelRequest, +) -> crate::openhuman::agent::pformat::PFormatRegistry { + request + .tools + .iter() + .map(|t| { + ( + t.name.clone(), + crate::openhuman::agent::pformat::PFormatToolParams::from_schema(&t.parameters), + ) + }) + .collect() +} + /// Translate an openhuman [`ChatResponse`] into a harness [`ModelResponse`] /// (visible text + tool calls + token usage). /// @@ -157,9 +185,18 @@ fn build_chat_inputs( /// dispatcher — so text-mode models drive the tinyagents loop too. The visible /// text is the prose with any tool-call markup stripped. /// +/// `pformat_registry` carries the advertised tools' positional layouts so the +/// text-mode fallback can recover P-Format (`name[a|b]`) calls that ~10 builtin +/// prompts still teach — the migrated parse path had dropped that grammar and +/// silently lost those calls (issue #4465). It is empty for the native-tool +/// path (where `response.tool_calls` is used directly) and for tool-less turns. +/// /// Unknown-tool recovery is handled by `RunPolicy::unknown_tool`, so the model /// adapter preserves the provider-requested tool name. -fn response_to_model_response(response: &ChatResponse) -> ModelResponse { +fn response_to_model_response( + response: &ChatResponse, + pformat_registry: &crate::openhuman::agent::pformat::PFormatRegistry, +) -> ModelResponse { let (visible_text, tool_calls): (String, Vec) = if !response.tool_calls.is_empty() { let calls = response .tool_calls @@ -172,7 +209,8 @@ fn response_to_model_response(response: &ChatResponse) -> ModelResponse { .collect(); (response.text.clone().unwrap_or_default(), calls) } else if let Some(text) = response.text.as_deref() { - let (prose, parsed) = crate::openhuman::agent::harness::parse_tool_calls(text); + let (prose, parsed) = + crate::openhuman::agent::harness::parse_tool_calls_with_pformat(text, pformat_registry); if parsed.is_empty() { (text.to_string(), Vec::new()) } else { @@ -313,6 +351,12 @@ pub(super) struct ProviderModel { thinking: Option, /// Preserves the last original provider error for the runner to re-surface. error_slot: ProviderErrorSlot, + /// FIFO side-channel shared with the event bridge: on each successful chat + /// response the adapter pushes the provider `UsageInfo` (which carries the + /// backend-charged USD + context window + cache-creation/reasoning tokens + /// the crate `Usage` mapping drops), and the bridge pops it when recording + /// that call's usage — restoring charged-USD precedence (#4467, item 1). + usage_carry: ProviderUsageCarry, /// Capability profile derived from the wrapped provider (issue #4249, /// Phase 2): lets the crate validate a request against the model's actual /// capabilities (vision, tool calling, streaming, token limits) *before* @@ -368,6 +412,7 @@ impl ProviderModel { max_tokens: None, thinking: None, error_slot: Arc::new(Mutex::new(None)), + usage_carry: Arc::default(), profile, } } @@ -378,6 +423,14 @@ impl ProviderModel { self.error_slot.clone() } + /// Attach the shared provider-usage carry the event bridge drains, so the + /// backend-charged USD + context window this adapter observes reach the cost + /// accounting (#4467, item 1). Clone the same handle into the bridge. + pub(super) fn with_usage_carry(mut self, carry: ProviderUsageCarry) -> Self { + self.usage_carry = carry; + self + } + /// Cap the output tokens requested from the provider for every call. pub(super) fn with_max_tokens(mut self, max_tokens: u32) -> Self { self.max_tokens = Some(max_tokens); @@ -436,6 +489,9 @@ impl ChatModel<()> for ProviderModel { ) -> tinyagents::Result { let native = self.provider.supports_native_tools(); let (messages, specs) = build_chat_inputs(&request, native); + // Positional layouts for the text-mode P-Format fallback (issue #4465); + // empty (and thus behaviour-neutral) when no tools are advertised. + let pformat_registry = pformat_registry_from_request(&request); let chat_request = ChatRequest { messages: &messages, // Only advertise structured tool specs to native providers. Prompt- @@ -459,7 +515,21 @@ impl ChatModel<()> for ProviderModel { .chat(chat_request, &self.model, self.temperature) .await { - Ok(response) => response, + Ok(response) => { + // #4457 (defect B): the error slot preserves the last provider + // error for the runner to re-surface as the typed turn failure. + // A call that *succeeds* — including one the provider fallback + // chain recovered after an inner error — must clear any stale + // error so a later, unrelated run failure (e.g. the model-call + // cap) is not misclassified as that recovered provider error. + if self.error_slot.lock().unwrap().take().is_some() { + tracing::debug!( + model = %self.model, + "[models] provider chat succeeded; cleared stale error_slot — #4457 defect B" + ); + } + response + } Err(e) => { // Classify with OpenHuman's product error taxonomy (issue #4249, // Workstream 02.2): a permanent config/auth rejection, billing/quota @@ -499,7 +569,17 @@ impl ChatModel<()> for ProviderModel { forwarder.emit(reasoning.clone()); } } - Ok(response_to_model_response(&response)) + // Push this call's provider usage onto the shared carry so the event + // bridge records charged USD / context window with provider precedence + // (#4467, item 1). One push per successful response, matching the single + // `UsageRecorded` the crate emits for this call. + if let Some(u) = &response.usage { + self.usage_carry + .lock() + .unwrap_or_else(|p| p.into_inner()) + .push_back(u.clone()); + } + Ok(response_to_model_response(&response, &pformat_registry)) } /// Stream the model response, forwarding openhuman's `ProviderDelta` events @@ -514,19 +594,48 @@ impl ChatModel<()> for ProviderModel { async fn stream(&self, _state: &(), request: ModelRequest) -> tinyagents::Result { let native = self.provider.supports_native_tools(); let (messages, specs) = build_chat_inputs(&request, native); + // Positional layouts for the text-mode P-Format fallback (issue #4465); + // built here so it can move into the `'static` producer task below. + let pformat_registry = pformat_registry_from_request(&request); let provider = self.provider.clone(); let model = self.model.clone(); let temperature = self.temperature; let max_tokens = self.max_tokens; let thinking = self.thinking.clone(); let error_slot = self.error_slot.clone(); + // Captured for the spawned producer (task-locals/`self` do not cross the + // spawn): the streaming path pushes provider usage onto the same carry + // the buffered path uses, so charged USD reaches the bridge (#4467, item 1). + let usage_carry = self.usage_carry.clone(); let (item_tx, item_rx) = tokio::sync::mpsc::unbounded_channel::(); + // #4460: the producer below runs in a detached `tokio::spawn`, and + // `tokio::task_local`s do NOT propagate across a spawn boundary. Capture + // the two ambient task-locals the provider call depends on *here*, on the + // caller's task, and re-establish them inside the spawn: + // - `thread_id` → the managed backend's `thread_id` extension + // (`compatible_request::outbound_thread_id`) so streamed requests stay + // attributed to the right chat / prompt-cache group. + // - resolved-route audit slot → so `record_resolved_provider_route` + // calls inside `provider.chat` write back to the caller's scope and the + // channel audit reports the *resolved* route, not the requested one. + let thread_id = current_thread_id(); + let route_slot = current_route_slot(); + // Label for the abort-on-drop debug log; the moved-in `model` clone is + // consumed by the producer body. + let abort_label = model.clone(); + tracing::debug!( + model = %model, + thread_id = thread_id.as_deref().unwrap_or(""), + route_slot = route_slot.is_some(), + "[tinyagents] spawning streamed provider producer; re-establishing task-locals across spawn — #4460" + ); + // Producer: run the provider call while forwarding its incremental // deltas, then emit the terminal item. Everything captured is owned, so // the task is `'static`. - tokio::spawn(async move { + let producer = async move { let _ = item_tx.send(ModelStreamItem::Started); let (delta_tx, mut delta_rx) = tokio::sync::mpsc::channel::(64); let chat_fut = async { @@ -563,6 +672,16 @@ impl ChatModel<()> for ProviderModel { let terminal = match response { Ok(resp) => { + // #4457 (defect B): a successful streaming call — including + // one recovered by the provider fallback chain — clears any + // stale error preserved in the slot so a later unrelated run + // failure is not misclassified as that recovered error. + if error_slot.lock().unwrap().take().is_some() { + tracing::debug!( + model = %model, + "[models] streaming provider chat succeeded; cleared stale error_slot — #4457 defect B" + ); + } // Fallback for streaming providers that return reasoning only // on the aggregated response (no incremental thinking // deltas): emit it once through the native crate stream so @@ -576,7 +695,16 @@ impl ChatModel<()> for ProviderModel { )); } } - ModelStreamItem::Completed(response_to_model_response(&resp)) + // Push provider usage onto the shared carry (#4467, item 1), + // mirroring the buffered path — before building the terminal + // item, so it is queued ahead of the crate `UsageRecorded`. + if let Some(u) = &resp.usage { + usage_carry + .lock() + .unwrap_or_else(|p| p.into_inner()) + .push_back(u.clone()); + } + ModelStreamItem::Completed(response_to_model_response(&resp, &pformat_registry)) } Err(e) => { // Streaming failures ride `ModelStreamItem::Failed(String)`, which @@ -599,10 +727,28 @@ impl ChatModel<()> for ProviderModel { } }; let _ = item_tx.send(terminal); + }; + + // Re-establish the captured task-locals inside the spawned task (#4460). + // `with_thread_id` normalizes an absent id to `None`, so it is a no-op + // when there was no ambient thread; the route slot is only re-scoped when + // an enclosing `with_resolved_provider_route_scope` supplied one. + let handle = tokio::spawn(async move { + let scoped = with_thread_id(thread_id.unwrap_or_default(), producer); + match route_slot { + Some(slot) => with_route_slot(slot, scoped).await, + None => scoped.await, + } }); - let stream = futures_util::stream::unfold(item_rx, |mut rx| async move { - rx.recv().await.map(|item| (item, rx)) + // #4460: tie the producer's lifetime to the consumer. Moving the + // abort-on-drop guard into the stream state means that dropping the + // stream (the turn future being hard-cancelled via `AbortHandle`, or + // dropped for any other reason) aborts the in-flight `provider.chat` call + // instead of letting it run — and bill — to completion in the background. + let guard = AbortOnDrop::new(handle, abort_label); + let stream = futures_util::stream::unfold((item_rx, guard), |(mut rx, guard)| async move { + rx.recv().await.map(|item| (item, (rx, guard))) }); Ok(Box::pin(stream)) } diff --git a/src/openhuman/tinyagents/observability.rs b/src/openhuman/tinyagents/observability.rs index af700a7058..3c14cf5455 100644 --- a/src/openhuman/tinyagents/observability.rs +++ b/src/openhuman/tinyagents/observability.rs @@ -49,13 +49,16 @@ pub(crate) type IterationCursor = Arc; /// `tool_name` contract without the forwarder emitting those fragments itself. pub(crate) type ToolNameMap = Arc>>; -/// Shared `call_id → (success, classified failure)` side-channel. The crate's -/// `AgentEvent::ToolCompleted` carries only `call_id` + `tool_name` (no -/// success/error), so `ToolOutcomeCaptureMiddleware::after_tool` — which does -/// see the `ToolResult` — classifies each outcome and writes it here; the bridge -/// reads it when projecting the live `ToolCallCompleted` event, so a failed tool -/// surfaces real `success: false` + a user-facing `failure`. Absent entry (event -/// projected before the middleware ran) falls back to `(true, None)`. +/// Shared `call_id → (success, classified failure, elapsed_ms, output_chars)` +/// side-channel. The crate's `AgentEvent::ToolCompleted` carries only `call_id` +/// + `tool_name` (no success/error, duration, or output size), so +/// `ToolOutcomeCaptureMiddleware::after_tool` — which does see the `ToolResult` +/// (including the executor-measured `elapsed_ms` and the rendered content) — +/// classifies each outcome and writes it here; the bridge reads it when +/// projecting the live `ToolCallCompleted` event, so a failed tool surfaces real +/// `success: false` + a user-facing `failure`, and a completed tool surfaces its +/// real duration + output size instead of `0`/`0` (#4467, item 4). Absent entry +/// (event projected before the middleware ran) falls back to `(true, None, 0, 0)`. pub(crate) type ToolFailureMap = Arc< Mutex< std::collections::HashMap< @@ -63,11 +66,27 @@ pub(crate) type ToolFailureMap = Arc< ( bool, Option, + u64, + usize, ), >, >, >; +/// Shared FIFO carry of the per-call provider [`UsageInfo`] the model adapter +/// observed, drained by the bridge when it records that call's usage. The crate +/// `Usage` the harness surfaces on `AgentEvent::UsageRecorded` carries only token +/// counts, so the backend-charged USD, the model's context window, and the +/// cache-creation/reasoning token breakdown have no crate home — the model +/// adapter pushes the full provider `UsageInfo` here (one push per provider +/// response) and the bridge pops it (one pop per recorded model call, after the +/// duplicate-usage dedupe guard) to restore charged-USD precedence and the full +/// accounting (#4467, item 1). A pop that finds nothing (a fallback-route call +/// that did not push, or an out-of-band usage event) degrades gracefully to a +/// catalogue estimate. +pub(crate) type ProviderUsageCarry = + Arc>>; + /// An [`EventListener`] that pauses the run once `cap` model calls have /// completed, so the loop stops gracefully at the iteration budget (returning /// the partial transcript) instead of erroring with `LimitExceeded`. The harness @@ -139,9 +158,14 @@ pub(crate) struct OpenhumanEventBridge { /// `ThinkingForwarder` on tool-call start; read here to label the /// incremental tool-argument fragments projected off the crate stream. tool_names: ToolNameMap, - /// Shared `call_id → (success, failure)` side-channel written by - /// `ToolOutcomeCaptureMiddleware`; read when projecting `ToolCallCompleted`. + /// Shared `call_id → (success, failure, elapsed_ms, output_chars)` + /// side-channel written by `ToolOutcomeCaptureMiddleware`; read when + /// projecting `ToolCallCompleted`. failure_map: ToolFailureMap, + /// Shared FIFO carry of the per-call provider `UsageInfo` the model adapter + /// observed; drained in `record_usage` to restore backend-charged USD + + /// context-window + cache-creation/reasoning tokens the crate `Usage` drops. + usage_carry: ProviderUsageCarry, /// Model-call iterations whose `UsageRecorded` has already been folded into /// the global cost tracker (W2-budget-dedupe). A single model call can now /// surface **two** `UsageRecorded` events — one from the harness runtime @@ -169,6 +193,7 @@ impl OpenhumanEventBridge { Arc::default(), Arc::default(), Arc::default(), + Arc::default(), ) } @@ -183,6 +208,7 @@ impl OpenhumanEventBridge { cursor: IterationCursor, tool_names: ToolNameMap, failure_map: ToolFailureMap, + usage_carry: ProviderUsageCarry, ) -> Arc { Arc::new(Self { on_progress, @@ -192,6 +218,7 @@ impl OpenhumanEventBridge { cursor, tool_names, failure_map, + usage_carry, recorded_iterations: Mutex::new(std::collections::HashSet::new()), state: Mutex::new(BridgeState::default()), }) @@ -225,11 +252,40 @@ impl OpenhumanEventBridge { (s.cache_hits, s.cache_misses) } - /// Best-effort, non-blocking progress emit (drops on a full channel, like - /// the legacy streaming path). + /// Forward a progress event without ever silently dropping it under + /// backpressure (#4466). The crate `EventListener::on_event` callback is + /// **synchronous**, so we cannot `.await` a bounded `send()` inline the way + /// the legacy streaming path did. Fast path: `try_send`, which succeeds (and + /// stays fully synchronous + ordered) whenever the downstream channel has + /// room — the common case. Only when the channel is momentarily **full** do + /// we fall back to an awaited `send()` on a spawned task so the delta is + /// delivered under backpressure instead of being dropped (the old bug). A + /// `Closed` channel means the receiver is gone (turn tore down), where + /// dropping is correct. fn send(&self, progress: AgentProgress) { - if let Some(tx) = &self.on_progress { - let _ = tx.try_send(progress); + use tokio::sync::mpsc::error::TrySendError; + let Some(tx) = &self.on_progress else { + return; + }; + match tx.try_send(progress) { + Ok(()) => {} + Err(TrySendError::Closed(_)) => {} + Err(TrySendError::Full(progress)) => { + // Backpressure, not capacity loss: hand the delta to an awaited + // `send()` on a spawned task rather than dropping it. Guard on a + // live runtime so a non-async construction path can't panic. + if let Ok(handle) = tokio::runtime::Handle::try_current() { + let tx = tx.clone(); + handle.spawn(async move { + let _ = tx.send(progress).await; + }); + } else { + tracing::debug!( + model = %self.model, + "[tinyagents] progress channel full and no runtime to defer send; dropping one delta" + ); + } + } } } @@ -267,18 +323,67 @@ impl OpenhumanEventBridge { return; } } - // Provider-reported charged USD has no home in the crate `Usage` (all - // token counts), so estimate this call's cost from catalogued per-MTok - // rates. Fixes the long-standing $0 cost on the tinyagents path, where - // the charged amount was hardcoded to 0.0 (issue #4249, Phase 5). When a - // provider genuinely charges (credit-metered backends) preserving that - // exact amount needs an out-of-band carry — tracked as a follow-up. - let call_cost = crate::openhuman::cost::catalog::estimate_cost_usd( + // Drain the provider-usage side-channel the model adapter fed for this + // model call (FIFO, one push per provider response). The crate `Usage` + // the harness surfaces carries only token counts, so the backend-charged + // USD, the model's context window, and the cache-creation/reasoning + // breakdown ride this out-of-band carry instead (#4467, item 1). Popped + // AFTER the dedupe guard above so the duplicate `UsageRecorded` re-emit + // (crate `BudgetMiddleware`) does not consume a second entry. + let carried = self + .usage_carry + .lock() + .unwrap_or_else(|p| p.into_inner()) + .pop_front(); + + // Estimate from catalogued per-MTok rates as the floor; prefer the + // provider's own charged amount when it reported one (charged > estimate + // precedence — restores the legacy observer's behaviour, so credit-metered + // backends surface real billing rather than a token-rate estimate). + let estimate = crate::openhuman::cost::catalog::estimate_cost_usd( &self.model, usage.input_tokens, usage.output_tokens, usage.cache_read_tokens, ); + let call_cost = carried + .as_ref() + .map(|u| u.charged_amount_usd) + .filter(|c| c.is_finite() && *c > 0.0) + .unwrap_or(estimate); + // The context window + cache-creation/reasoning breakdown only exist on + // the carried provider usage (the crate `Usage` mapping drops them); fall + // back to the catalogue window and the crate token counts when absent. + let context_window = carried + .as_ref() + .map(|u| u.context_window) + .filter(|w| *w > 0) + .unwrap_or_else(|| { + crate::openhuman::cost::catalog::lookup(&self.model) + .map(|p| u64::from(p.context_window)) + .unwrap_or(0) + }); + let cache_creation_tokens = carried + .as_ref() + .map(|u| u.cache_creation_tokens) + .filter(|t| *t > 0) + .unwrap_or(usage.cache_creation_tokens); + let reasoning_tokens = carried + .as_ref() + .map(|u| u.reasoning_tokens) + .filter(|t| *t > 0) + .unwrap_or(usage.reasoning_tokens); + tracing::trace!( + model = %self.model, + iteration, + charged_from_provider = carried + .as_ref() + .map(|u| u.charged_amount_usd > 0.0) + .unwrap_or(false), + call_cost, + context_window, + "[cost] recording per-call usage (charged>estimate precedence via provider carry)" + ); let (input, output, cached, charged) = { let mut s = self.state.lock().unwrap(); s.input_tokens += usage.input_tokens; @@ -298,18 +403,18 @@ impl OpenhumanEventBridge { let usage_info = UsageInfo { input_tokens: usage.input_tokens, output_tokens: usage.output_tokens, - context_window: 0, + context_window, cached_input_tokens: usage.cache_read_tokens, - cache_creation_tokens: usage.cache_creation_tokens, - reasoning_tokens: usage.reasoning_tokens, + cache_creation_tokens, + reasoning_tokens, charged_amount_usd: call_cost, }; - if usage.reasoning_tokens > 0 || usage.cache_creation_tokens > 0 { + if reasoning_tokens > 0 || cache_creation_tokens > 0 { log::debug!( "[cost] recording reasoning/cache-creation tokens model={} reasoning_tokens={} cache_creation_tokens={}", self.model, - usage.reasoning_tokens, - usage.cache_creation_tokens + reasoning_tokens, + cache_creation_tokens ); } crate::openhuman::cost::record_provider_usage(&self.model, &usage_info); @@ -414,10 +519,15 @@ impl EventListener for OpenhumanEventBridge { // shared map the forwarder populated on the tool-call start // event (empty until the start marker lands — matching the // legacy forwarder's own default). There is no `Subagent*` - // tool-arg variant, so child runs ride the top-level event too - // (parity with the forwarder's prior behavior). + // tool-arg variant, and an UNSCOPED top-level `ToolCallArgsDelta` + // emitted from a child run would render the child's argument + // composition as the *parent's* own timeline activity (#4467, + // item 6). v0.58.7 dropped child arg fragments, so restore that: + // only a parent/top-level turn projects these fragments; a child + // run drops them (its Started/Completed rows already carry the + // final arguments under the `Subagent*` scope). if let Some(tool_call) = &delta.tool_call { - if !tool_call.content.is_empty() { + if self.scope.is_none() && !tool_call.content.is_empty() { let tool_name = self .tool_names .lock() @@ -597,6 +707,7 @@ impl EventListener for OpenhumanEventBridge { output: String::new(), elapsed_ms: 0, iteration, + failure, }); } } @@ -644,30 +755,37 @@ impl EventListener for OpenhumanEventBridge { .lock() .ok() .and_then(|mut m| m.remove(call_id.as_str())); - let success = outcome.as_ref().map(|(ok, _)| *ok).unwrap_or(true); + let success = outcome.as_ref().map(|(ok, ..)| *ok).unwrap_or(true); + // Real execution duration + output size the capture middleware + // recorded off the `ToolResult` (#4467, item 4). Absent (event + // projected before the middleware ran) → 0, as before. + let elapsed_ms = outcome.as_ref().map(|(_, _, e, _)| *e).unwrap_or(0); + let output_chars = outcome.as_ref().map(|(_, _, _, c)| *c).unwrap_or(0); + // Carry the classified failure onto whichever completion event + // this projects — main-agent OR sub-agent (#4459). Previously + // the sub-agent branch dropped it on the floor. + let failure = outcome.and_then(|(_, f, _, _)| f); match &self.scope { - None => { - let failure = outcome.and_then(|(_, f)| f); - self.send(AgentProgress::ToolCallCompleted { - call_id: call_id.as_str().to_string(), - tool_name: tool_name.clone(), - success, - output_chars: 0, - elapsed_ms: 0, - iteration, - failure, - }) - } + None => self.send(AgentProgress::ToolCallCompleted { + call_id: call_id.as_str().to_string(), + tool_name: tool_name.clone(), + success, + output_chars, + elapsed_ms, + iteration, + failure, + }), Some(s) => self.send(AgentProgress::SubagentToolCallCompleted { agent_id: s.agent_id.clone(), task_id: s.task_id.clone(), call_id: call_id.as_str().to_string(), tool_name: tool_name.clone(), success, - output_chars: 0, + output_chars, output: String::new(), - elapsed_ms: 0, + elapsed_ms, iteration, + failure, }), } } diff --git a/src/openhuman/tinyagents/payload_summarizer.rs b/src/openhuman/tinyagents/payload_summarizer.rs index 9231d4633d..61cb0916af 100644 --- a/src/openhuman/tinyagents/payload_summarizer.rs +++ b/src/openhuman/tinyagents/payload_summarizer.rs @@ -28,7 +28,7 @@ //! pass-through, do nothing) when: //! //! * The raw payload is below -//! [`SubagentPayloadSummarizer::threshold_tokens`] (default 500 000 +//! [`SubagentPayloadSummarizer::threshold_tokens`] (config default 4 000 //! tokens — small payloads aren't worth an extra LLM round-trip). //! Token count is estimated as `chars / 4`, matching //! `tree_summarizer::estimate_tokens`. @@ -120,7 +120,7 @@ pub struct SubagentPayloadSummarizer { /// Lower bound, in **estimated tokens** (`chars / 4`): tool results /// smaller than this are passed through untouched. Default is /// `summarizer_payload_threshold_tokens` from - /// [`crate::openhuman::config::ContextConfig`] (500 000 tokens). + /// [`crate::openhuman::config::ContextConfig`] (default 4 000 tokens). threshold_tokens: usize, /// Upper bound, in **estimated tokens**: tool results larger than /// this are also passed through (no LLM call) and fall through to diff --git a/src/openhuman/tinyagents/steering_forwarder.rs b/src/openhuman/tinyagents/steering_forwarder.rs new file mode 100644 index 0000000000..1a6bea3aa2 --- /dev/null +++ b/src/openhuman/tinyagents/steering_forwarder.rs @@ -0,0 +1,306 @@ +//! Abort-on-drop steering forwarder guard (issue #4456). +//! +//! `run_turn_via_tinyagents_shared` bridges OpenHuman's session-owned +//! [`RunQueue`] into a running TinyAgents turn by spawning a 50 ms poll loop +//! that drains queued **steer**/**collect** messages and forwards them into the +//! run's [`SteeringHandle`]. The harness applies them at the next iteration +//! checkpoint. +//! +//! The historical cleanup — `forwarder.abort()` + steering-registry +//! `deregister(...)` — only ran when the drive future returned *normally*. But +//! cancellation in this codebase is **drop-based** (the web channel drops the +//! turn future via a `tokio::select!` cancel token; detached sub-agents are +//! hard-aborted through an `AbortHandle`). On those paths the spawned task +//! *detached* and looped `sleep(50 ms) → drain` forever, pinning the +//! `Arc` + `SteeringHandle` and — because the `RunQueue` is +//! session-owned and reused — **racing the next turn's forwarder**, stealing its +//! steer/collect messages into a dead handle. Registry entries for aborted +//! sub-agent tasks were likewise never removed. +//! +//! [`SteeringForwarderGuard`] fixes this with RAII: its [`Drop`] aborts the poll +//! task, deregisters from the shared steering registry, and drains any residual +//! (delivered-but-unapplied) steers back into the session `RunQueue` so a late +//! steer becomes the *next* turn's input instead of vanishing. Because the guard +//! is held across the drive future, cleanup happens identically on normal +//! return, error, and drop-cancellation. + +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Arc; +use std::time::Duration; + +use tinyagents::harness::message::Message as TaMessage; +use tinyagents::harness::steering::{SteeringCommand, SteeringHandle}; + +use crate::core::event_bus::{publish_global, DomainEvent}; +use crate::openhuman::agent::harness::run_queue::{QueueMode, QueuedMessage, RunQueue}; + +use super::orchestration::{self, TaskId}; + +/// Framing prepended to a queued **steer** message when it is injected as a user +/// turn. Kept as a shared const so the residual-requeue path can strip it and +/// avoid double-prefixing when the next turn re-forwards the recovered text. +pub(super) const STEER_PREFIX: &str = "[User steering message]: "; + +/// Framing prepended to a queued **collect** message (orchestrator/monitor +/// context lines). See [`STEER_PREFIX`]. +pub(super) const COLLECT_PREFIX: &str = "[Additional context from user]: "; + +/// Live steering-forwarder poll tasks, incremented when +/// [`SteeringForwarderGuard::new`] spawns a poll loop and decremented on its +/// `Drop`. Exposed for tests asserting the leak fix: after a cancelled/aborted +/// turn this must return to its pre-turn value (zero live forwarders for that +/// turn). +static ACTIVE_FORWARDERS: AtomicUsize = AtomicUsize::new(0); + +/// Number of steering-forwarder poll tasks currently live process-wide. +pub(crate) fn active_steering_forwarders() -> usize { + ACTIVE_FORWARDERS.load(Ordering::SeqCst) +} + +/// Milliseconds since the Unix epoch (best-effort; `0` on a pre-epoch clock). +fn now_ms() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_millis() as u64) + .unwrap_or(0) +} + +/// Drain the run queue's pending **steer** messages and forward them to the +/// tinyagents [`SteeringHandle`] as injected user turns (the harness applies +/// them to the working transcript at the next iteration checkpoint). This is the +/// bridge behind the `steer_subagent` / mid-flight-steering feature. Emits a +/// [`DomainEvent::RunQueueMessageDelivered`] when at least one message is +/// delivered so the delivery is visible in the event stream (issue #4456). +pub(super) async fn forward_steers(queue: &RunQueue, handle: &SteeringHandle, thread_label: &str) { + let drained = queue.drain_steers().await; + if drained.is_empty() { + return; + } + let delivered = drained.len(); + for msg in drained { + handle.send(SteeringCommand::InjectMessage(TaMessage::user(format!( + "{STEER_PREFIX}{}", + msg.text + )))); + } + tracing::debug!( + thread_id = thread_label, + delivered, + "[run_queue] delivered steer message(s) into running steering handle" + ); + let _ = publish_global(DomainEvent::RunQueueMessageDelivered { + thread_id: thread_label.to_string(), + mode: "steer".to_string(), + delivered, + }); +} + +/// Forward any queued **collect** messages (orchestrator/monitor lines enqueued +/// via `QueueMode::Collect`) into the run as injected user turns so they reach +/// the next LLM call as additional context. Mirrors the legacy +/// `[Additional context from user]:` framing the model was taught to read. Emits +/// a [`DomainEvent::RunQueueMessageDelivered`] on delivery (issue #4456). +pub(super) async fn forward_collects( + queue: &RunQueue, + handle: &SteeringHandle, + thread_label: &str, +) { + let drained = queue.drain_collects().await; + if drained.is_empty() { + return; + } + let delivered = drained.len(); + for msg in drained { + handle.send(SteeringCommand::InjectMessage(TaMessage::user(format!( + "{COLLECT_PREFIX}{}", + msg.text + )))); + } + tracing::debug!( + thread_id = thread_label, + delivered, + "[run_queue] delivered collect message(s) into running steering handle" + ); + let _ = publish_global(DomainEvent::RunQueueMessageDelivered { + thread_id: thread_label.to_string(), + mode: "collect".to_string(), + delivered, + }); +} + +/// Abort-on-drop guard around the 50 ms steering-forwarder poll task. +/// +/// Held across the harness drive future so its [`Drop`] runs on **every** exit +/// path (normal return, error, and drop-cancellation), aborting the poll task, +/// deregistering the sub-agent steering handle, and requeuing residual steers. +pub(super) struct SteeringForwarderGuard { + /// The spawned poll task; `abort()`-ed on drop. `None` after the abort so a + /// double-drop is a no-op. + forwarder: Option>, + /// A clone of the run's steering handle, drained on drop to recover + /// delivered-but-unapplied steers. + handle: SteeringHandle, + /// The session-owned run queue, used to requeue residual steers on drop. + /// `None` after the requeue so a double-drop is a no-op. + run_queue: Option>, + /// The steering-registry key to deregister on drop (sub-agent runs only). + registry_task_id: Option, + /// Best-effort thread label for observability + requeued-message metadata. + thread_label: String, +} + +impl SteeringForwarderGuard { + /// Arm the guard: spawn the 50 ms poll loop when a `run_queue` is present and + /// wrap all cleanup in an abort-on-drop scope. + /// + /// `run_queue` is `None` for a steering-only run (a sub-agent whose handle is + /// controlled purely through the steering registry, with no queue lane): no + /// poll task is spawned, but the guard still deregisters the handle on every + /// exit path so an aborted run cannot leak a registry entry. `registry_task_id` + /// is `Some` for sub-agent runs (whose handle is registered in the shared + /// steering registry) and `None` for the interactive parent turn. + pub(super) fn new( + handle: SteeringHandle, + run_queue: Option>, + registry_task_id: Option, + thread_label: String, + ) -> Self { + let forwarder = run_queue.as_ref().map(|queue| { + ACTIVE_FORWARDERS.fetch_add(1, Ordering::SeqCst); + let loop_queue = queue.clone(); + let loop_handle = handle.clone(); + let loop_label = thread_label.clone(); + tokio::spawn(async move { + loop { + tokio::time::sleep(Duration::from_millis(50)).await; + forward_steers(&loop_queue, &loop_handle, &loop_label).await; + forward_collects(&loop_queue, &loop_handle, &loop_label).await; + } + }) + }); + tracing::debug!( + thread_id = thread_label.as_str(), + has_queue = run_queue.is_some(), + "[run_queue] steering forwarder guard armed (abort-on-drop)" + ); + Self { + forwarder, + handle, + run_queue, + registry_task_id, + thread_label, + } + } +} + +impl Drop for SteeringForwarderGuard { + fn drop(&mut self) { + // 1. Stop the poll loop so it can no longer race the next turn's + // forwarder for the shared, session-owned run queue. + if let Some(forwarder) = self.forwarder.take() { + forwarder.abort(); + ACTIVE_FORWARDERS.fetch_sub(1, Ordering::SeqCst); + tracing::debug!( + thread_id = self.thread_label.as_str(), + "[run_queue] aborted steering forwarder (guard drop)" + ); + } + + // 2. Deregister the sub-agent steering handle so an aborted run does not + // leak a registry entry keyed by a dead handle. + if let Some(task_id) = self.registry_task_id.take() { + orchestration::shared_steering_registry().deregister(&task_id); + tracing::debug!( + task_id = task_id.as_str(), + "[tinyagents] deregistered subagent steering handle (guard drop)" + ); + } + + // 3. Recover residual steers: messages the poll loop already delivered + // into the handle but that the harness ended/cancelled before + // applying at a checkpoint. Strip the delivery prefix so the next + // turn's forwarder re-frames them cleanly (no double prefix). + // Control-flow-only commands (Pause/Resume/Cancel/…) are meaningless + // once the run is gone and are intentionally dropped. + let residual = self.handle.drain(); + let requeue_texts: Vec<(String, QueueMode)> = residual + .into_iter() + .filter_map(|cmd| match cmd { + SteeringCommand::InjectMessage(msg) => { + let text = msg.text(); + // The prefix that matched tells us the lane — preserve it so + // a delivered-but-unapplied collect line re-enters as Collect + // (framed `[Additional context from user]:`) rather than being + // re-labeled as user Steer. Default to Steer when neither + // prefix is present (a raw steer that was never framed). + if let Some(rest) = text.strip_prefix(STEER_PREFIX) { + Some((rest.to_string(), QueueMode::Steer)) + } else if let Some(rest) = text.strip_prefix(COLLECT_PREFIX) { + Some((rest.to_string(), QueueMode::Collect)) + } else { + Some((text.to_string(), QueueMode::Steer)) + } + } + _ => None, + }) + .collect(); + + let Some(queue) = self.run_queue.take() else { + return; + }; + if requeue_texts.is_empty() { + return; + } + let requeued = requeue_texts.len(); + let thread_label = self.thread_label.clone(); + + // `RunQueue::push` is async (tokio `Mutex`); `Drop` is synchronous. Push + // the recovered steers back on a detached task so they land in the + // session queue and become the next turn's input. The forwarder poll + // loop keeps re-draining the queue, so even if the requeue completes + // after the next turn starts, its next 50 ms tick picks them up. + match tokio::runtime::Handle::try_current() { + Ok(rt) => { + let label = thread_label.clone(); + rt.spawn(async move { + for (text, mode) in requeue_texts { + queue + .push(QueuedMessage { + text, + mode, + client_id: String::new(), + thread_id: label.clone(), + queued_at_ms: now_ms(), + model_override: None, + temperature: None, + profile_id: None, + locale: None, + }) + .await; + } + }); + } + Err(_) => { + // No runtime to spawn on (should not happen on the live paths, + // which always drop inside a tokio task). The steers are lost + // rather than silently mis-handled — log loudly. + tracing::warn!( + thread_id = thread_label.as_str(), + requeued, + "[run_queue] could not requeue residual steers: no tokio runtime at guard drop" + ); + return; + } + } + + tracing::debug!( + thread_id = thread_label.as_str(), + requeued, + "[run_queue] requeued residual steer(s) as next-turn input (guard drop)" + ); + let _ = publish_global(DomainEvent::RunQueueSteerRequeued { + thread_id: thread_label, + requeued, + }); + } +} diff --git a/src/openhuman/tinyagents/summarize.rs b/src/openhuman/tinyagents/summarize.rs index 18e1fe0d49..1e95131a67 100644 --- a/src/openhuman/tinyagents/summarize.rs +++ b/src/openhuman/tinyagents/summarize.rs @@ -21,7 +21,9 @@ //! deterministic trim, so summarization is preferred and trimming remains only a //! last-resort hard cap when even the summary + recent window overflow. -use std::sync::Arc; +use std::hash::{Hash, Hasher}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; use async_trait::async_trait; @@ -158,6 +160,228 @@ impl Summarizer for ProviderModelSummarizer { } } +/// Token budget for the deterministic-trim fallback summary, as a fraction of +/// the policy's summarization trigger budget. The fallback must actually *free* +/// tokens (so the turn shrinks below the window), so it targets a small slice of +/// the trigger point rather than echoing the whole compacted head back. +const FALLBACK_TRIM_TRIGGER_FRACTION: f64 = 0.25; +/// Hard floor / ceiling (tokens) for the deterministic-trim fallback budget, so +/// tiny windows still keep *something* and huge windows don't defeat the point. +const FALLBACK_TRIM_MIN_TOKENS: u64 = 1_024; +const FALLBACK_TRIM_MAX_TOKENS: u64 = 8_192; + +/// A single cached summary keyed by the shape of its input slice. +/// +/// `key` is a content hash of the exact `to_summarize` slice the crate handed us +/// (message count folded in). Repeat calls within a turn that present the same +/// slice (retries, re-planning, or a stalled tool loop that re-issues an +/// identical model request) reuse the cached [`SummaryRecord`] instead of +/// re-dispatching the summarizer LLM. +struct CachedSummary { + key: u64, + record: SummaryRecord, +} + +/// Fault-tolerant, per-turn-caching [`Summarizer`] adapter (issue #4461). +/// +/// Wraps the real (LLM-backed) [`ProviderModelSummarizer`] the turn hands the +/// crate [`ContextCompressionMiddleware`][tinyagents::harness::middleware::ContextCompressionMiddleware] +/// and hardens two regressions the crate introduced versus the legacy engine: +/// +/// 1. **Failure no longer aborts the turn.** The crate's `before_model` does +/// `self.summarizer.summarize(..).await?`, so any provider hiccup maps to +/// [`TinyAgentsError::Model`] and fails the whole run — on exactly the +/// longest, most valuable threads. This adapter instead catches the error, +/// logs a `warn`, trips a **per-turn circuit breaker**, and returns a +/// deterministic (LLM-free) trim of the input. The turn continues, matching +/// the legacy `warn! + circuit-breaker + deterministic-trim` fallback. Once +/// the breaker is tripped, every later compaction in the turn skips the +/// known-bad LLM and trims directly. +/// +/// 2. **No re-summarizing identical input.** The crate rebuilds the request from +/// `messages.clone()` each loop iteration and rewrites only that per-call +/// clone, so the working transcript never shrinks. Any call that presents the +/// same `to_summarize` slice (retries, re-planning, an identical re-issued +/// request) would otherwise spend a fresh full-transcript summarizer LLM call. +/// A single-slot content-hash cache makes those repeat calls free until the +/// transcript actually grows past the threshold again. +/// +/// Constructed fresh per turn inside [`assemble_turn_harness`][super::assemble_turn_harness], +/// so the breaker flag and cache are naturally per-turn state — no task-locals. +pub(super) struct FaultTolerantCachingSummarizer { + /// The real LLM-backed summarizer we guard. + inner: Box, + /// Per-turn circuit breaker: set once `inner` fails, thereafter every + /// compaction trims deterministically without touching the LLM. + breaker_tripped: AtomicBool, + /// Single-slot cache of the last produced summary, keyed by input-slice hash. + cache: Mutex>, + /// Token budget for the deterministic-trim fallback (derived from the + /// policy's context window at construction). + fallback_trim_budget: u64, +} + +impl FaultTolerantCachingSummarizer { + /// Wrap `inner` with per-turn fault tolerance + caching, sizing the + /// deterministic-trim fallback budget from `policy`'s trigger budget. + pub(super) fn new(inner: Box, policy: &SummarizationPolicy) -> Self { + let fallback_trim_budget = ((policy.trigger_budget() as f64 + * FALLBACK_TRIM_TRIGGER_FRACTION) as u64) + .clamp(FALLBACK_TRIM_MIN_TOKENS, FALLBACK_TRIM_MAX_TOKENS); + tracing::debug!( + fallback_trim_budget, + trigger_budget = policy.trigger_budget(), + "[tinyagents::summarize] installing fault-tolerant caching summarizer adapter" + ); + Self { + inner, + breaker_tripped: AtomicBool::new(false), + cache: Mutex::new(None), + fallback_trim_budget, + } + } + + /// Content hash of the exact input slice, folding in the message count so a + /// count change alone busts the cache (a grown transcript re-summarizes). + fn slice_key(messages: &[TaMessage]) -> u64 { + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + messages.len().hash(&mut hasher); + for msg in messages { + role_label(msg).hash(&mut hasher); + msg.text().hash(&mut hasher); + } + hasher.finish() + } + + /// Deterministic, LLM-free fallback: front-drop the oldest messages until the + /// remaining slice fits [`fallback_trim_budget`][Self::fallback_trim_budget] + /// tokens (the same front-drop semantics as + /// [`MessageTrimMiddleware`][tinyagents::harness::middleware::MessageTrimMiddleware] + /// with [`TrimStrategy::MaxTokens`][tinyagents::harness::summarization::TrimStrategy]), + /// then render the survivors into a single system checkpoint message. Never + /// fails, spends no tokens, and produces the same [`SummaryRecord`] shape the + /// LLM path does so provenance still surfaces downstream. + fn deterministic_trim(&self, messages: &[TaMessage], cause: &str) -> SummaryRecord { + let original_token_estimate: u64 = + messages.iter().map(|m| estimate_tokens(&m.text())).sum(); + let source_ids: Vec = (0..messages.len()).map(|i| format!("msg-{i}")).collect(); + + // Front-drop oldest messages until the tail fits the budget. Keep at + // least the single most-recent message so the summary is never empty. + let mut start = 0usize; + loop { + let remaining: u64 = messages[start..] + .iter() + .map(|m| estimate_tokens(&m.text())) + .sum(); + if remaining <= self.fallback_trim_budget || start + 1 >= messages.len() { + break; + } + start += 1; + } + let dropped = start; + + let mut body = String::from( + "=== Conversation Summary (deterministic trim — summarizer unavailable) ===\n", + ); + if dropped > 0 { + body.push_str(&format!( + "[{dropped} older message(s) dropped to fit the context budget]\n", + )); + } + for msg in &messages[start..] { + body.push_str(&format!("{}: {}\n", role_label(msg), msg.text())); + } + let summary_token_estimate = estimate_tokens(&body); + + tracing::warn!( + cause, + head_messages = messages.len(), + dropped, + from_tokens = original_token_estimate, + to_tokens = summary_token_estimate, + "[tinyagents::summarize] deterministic-trim fallback (no LLM); turn continues" + ); + + SummaryRecord { + summary: TaMessage::system(body), + provenance: CompressionProvenance { + source_ids, + original_token_estimate, + summary_token_estimate, + reason: format!( + "deterministic-trim fallback (summarizer LLM unavailable: {cause}); \ + front-dropped {dropped} message(s) to a {}-token budget", + self.fallback_trim_budget + ), + }, + } + } +} + +#[async_trait] +impl Summarizer for FaultTolerantCachingSummarizer { + async fn summarize(&self, messages: &[TaMessage]) -> TaResult { + let key = Self::slice_key(messages); + + // Cache hit: an identical slice was already summarized this turn. + if let Ok(guard) = self.cache.lock() { + if let Some(cached) = guard.as_ref() { + if cached.key == key { + tracing::debug!( + key, + head_messages = messages.len(), + "[tinyagents::summarize] reusing cached summary (identical input slice; \ + no summarizer LLM call)" + ); + return Ok(cached.record.clone()); + } + } + } + + // Circuit open from an earlier failure this turn: skip the known-bad LLM + // and trim deterministically without even attempting a call. + let record = if self.breaker_tripped.load(Ordering::Relaxed) { + tracing::debug!( + key, + head_messages = messages.len(), + "[tinyagents::summarize] circuit breaker open; trimming deterministically \ + (skipping summarizer LLM)" + ); + self.deterministic_trim( + messages, + "circuit breaker open (earlier summarizer failure)", + ) + } else { + match self.inner.summarize(messages).await { + Ok(record) => record, + Err(err) => { + // Trip the per-turn breaker and fall back — never propagate, + // so compaction failure can no longer abort the turn. + self.breaker_tripped.store(true, Ordering::Relaxed); + tracing::warn!( + error = %err, + key, + head_messages = messages.len(), + "[tinyagents::summarize] summarizer failed; tripping per-turn circuit \ + breaker and falling back to deterministic trim" + ); + self.deterministic_trim(messages, &err.to_string()) + } + } + }; + + // Cache the result (LLM or fallback) so a repeat identical slice is free. + if let Ok(mut guard) = self.cache.lock() { + *guard = Some(CachedSummary { + key, + record: record.clone(), + }); + } + Ok(record) + } +} + /// Build the context-window-aware [`SummarizationPolicy`] for a model whose /// input window is `context_window` tokens. /// diff --git a/src/openhuman/tinyagents/tests.rs b/src/openhuman/tinyagents/tests.rs index a2e16cab2d..0ce5c942fb 100644 --- a/src/openhuman/tinyagents/tests.rs +++ b/src/openhuman/tinyagents/tests.rs @@ -165,7 +165,7 @@ async fn streaming_path_forwards_text_deltas_and_cost() { 0.0, history, vec![registry], - std::collections::HashSet::new(), + None, 4, Some(tx), None, @@ -178,6 +178,7 @@ async fn streaming_path_forwards_text_deltas_and_cost() { None, None, false, + false, // defer_turn_completed_to_caller (#4457) ) .await .expect("streaming turn runs"); @@ -267,7 +268,7 @@ async fn pre_queued_steer_message_is_injected_into_the_request() { 0.0, vec![ChatMessage::user("investigate the bug")], vec![registry], - std::collections::HashSet::new(), + None, 4, None, None, @@ -280,6 +281,7 @@ async fn pre_queued_steer_message_is_injected_into_the_request() { None, None, false, + false, // defer_turn_completed_to_caller (#4457) ) .await .expect("steered turn runs"); @@ -364,7 +366,7 @@ async fn concurrent_shared_turns_each_get_a_distinct_result() { 0.0, vec![ChatMessage::user("task one")], vec![registry.clone()], - std::collections::HashSet::new(), + None, 4, None, None, @@ -377,6 +379,7 @@ async fn concurrent_shared_turns_each_get_a_distinct_result() { None, None, false, + false, // defer_turn_completed_to_caller (#4457) ); let two = run_turn_via_tinyagents_shared( provider.clone(), @@ -384,7 +387,7 @@ async fn concurrent_shared_turns_each_get_a_distinct_result() { 0.0, vec![ChatMessage::user("task two")], vec![registry], - std::collections::HashSet::new(), + None, 4, None, None, @@ -397,6 +400,7 @@ async fn concurrent_shared_turns_each_get_a_distinct_result() { None, None, false, + false, // defer_turn_completed_to_caller (#4457) ); let (a, b) = tokio::join!(one, two); @@ -436,7 +440,7 @@ fn adapter_inventory_registers_model_tools_and_middleware() { "mock-model", 0.0, tool_sets, - HashSet::new(), + None, 4, None, // on_progress: fire-and-forget None, // subagent_scope: top-level turn @@ -506,17 +510,24 @@ fn adapter_inventory_registers_model_tools_and_middleware() { assert!(serialized.contains("\"classified\":true")); // Lifecycle middleware, in registration order: memory-protocol enforcement - // (outermost), repeated-tool-failure breaker, shadow tool-exposure, - // prompt-cache segment + guard, cache-align + tool-output + // (outermost), repeated-tool-failure breaker, repeat-progress breaker (#4463), + // shadow tool-exposure, prompt-cache segment + guard, cache-align + tool-output // (TurnContextMiddleware::defaults), observe-only crate BudgetMiddleware // (W2-budget-dedupe), cost budget (local enforcement + budget_shadow), // context compression + message trim (window known + autocompact on), SDK - // tool-policy projection, tool-outcome capture, arg recovery. + // tool-policy projection, tool-outcome capture, arg recovery, schema guard + // (#4451 before_tool). let mw = assembled.harness.middleware(); - assert_eq!(mw.len(), 13, "lifecycle middleware inventory"); - // Around-tool wraps: approval/security + CLI/RPC-only scope gate (no - // builder tool policy on this call). - assert_eq!(mw.tool_middleware_len(), 2, "tool middleware inventory"); + // NOTE(parity merge): these inventory counts are the upstream base (13 / 2) + // plus the lifecycle + around-tool middlewares this parity branch adds. They + // are NOT compiled by `cargo check --lib` (this is a #[cfg(test)] block) and + // are pending the deferred test pass — verify/adjust the exact numbers when + // the test suite actually compiles. + assert_eq!(mw.len(), 15, "lifecycle middleware inventory"); + // Around-tool wraps: schema guard (#4451, outermost) + approval/security + + // CLI/RPC-only scope gate + credential scrub (#4453, innermost). No builder + // tool policy on this call. + assert_eq!(mw.tool_middleware_len(), 4, "tool middleware inventory"); assert_eq!(mw.model_middleware_len(), 0, "no around-model wraps"); assert_eq!( assembled.harness.policy().limits.max_depth, @@ -561,7 +572,7 @@ fn adapter_inventory_gates_context_middleware_on_window() { "mock-model", 0.0, tool_sets, - HashSet::new(), + None, 4, None, None, @@ -577,7 +588,7 @@ fn adapter_inventory_gates_context_middleware_on_window() { let mw = assembled.harness.middleware(); assert_eq!( mw.len(), - 11, + 13, "compression + trim must not install without a window" ); assert!(assembled.early_exit_hook.is_none()); @@ -637,7 +648,7 @@ async fn unobserved_turn_reports_aggregate_usage_for_the_cost_fallback() { 0.0, vec![ChatMessage::user("hello")], Vec::new(), - HashSet::new(), + None, 3, None, // on_progress: unobserved — no bridge, cost fallback branch runs None, @@ -650,6 +661,7 @@ async fn unobserved_turn_reports_aggregate_usage_for_the_cost_fallback() { None, None, false, + false, // defer_turn_completed_to_caller (#4457) ) .await .expect("turn runs"); diff --git a/src/openhuman/tinyagents/tools.rs b/src/openhuman/tinyagents/tools.rs index 8ae642a3a7..981716478b 100644 --- a/src/openhuman/tinyagents/tools.rs +++ b/src/openhuman/tinyagents/tools.rs @@ -6,7 +6,7 @@ //! the underlying tool and render the [`ToolResult`] the way the LLM should see //! it (rendered via `output_for_llm`, matching the legacy tool loop). -use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, PoisonError}; use async_trait::async_trait; use tinyagents::harness::steering::{SteeringCommand, SteeringHandle}; @@ -46,14 +46,21 @@ impl EarlyExitHook { /// The captured early-exit, if one fired during the run. pub(crate) fn take(&self) -> Option { - self.slot.lock().unwrap().take() + // #4469 item 3: recover a poisoned slot rather than panic — a panic while + // some other tool held this lock must not swallow the early-exit. + self.slot + .lock() + .unwrap_or_else(PoisonError::into_inner) + .take() } /// Record an early-exit and request a cooperative pause. Only the first /// early-exit in a run is kept (matching the legacy "halt on first"). fn trigger(&self, tool: &str, question: String) { { - let mut slot = self.slot.lock().unwrap(); + // #4469 item 3: `into_inner` keeps early-exit recording working even + // if the slot mutex was poisoned by an unrelated panic. + let mut slot = self.slot.lock().unwrap_or_else(PoisonError::into_inner); if slot.is_none() { *slot = Some(EarlyExit { tool: tool.to_string(), @@ -165,6 +172,12 @@ fn tool_policy_from_openhuman_tool(tool: &dyn crate::openhuman::tools::Tool) -> }) } +/// `session_id` stamped on the per-tool `DomainEvent`s this executor publishes. +/// The tinyagents adapter carries no per-turn session handle at this seam, so a +/// stable module label groups its tool-execution telemetry (matches the fixed +/// `"javascript"` label the node runtime uses for the same events). +const TINYAGENTS_TOOL_SESSION: &str = "tinyagents"; + /// Execute an openhuman [`Tool`](crate::openhuman::tools::Tool) for a harness /// [`TaToolCall`] and render the [`TaToolResult`] the way the LLM should see it /// (mirrors the live-path `HarnessToolExecutor`). @@ -183,6 +196,20 @@ async fn execute_openhuman_tool( "[tinyagents] executing openhuman tool via harness adapter" ); + // Measure the real execution duration (#4467, item 4) and re-publish the + // per-tool `DomainEvent`s the legacy session loop emitted so SSE consumers + // keep per-tool start/complete telemetry on the tinyagents path (#4467, + // item 5). `tool_name` is cloned up front because `call.name` is moved into + // the `TaToolResult` below. + let started = std::time::Instant::now(); + let tool_name = call.name.clone(); + crate::core::event_bus::publish_global( + crate::core::event_bus::DomainEvent::ToolExecutionStarted { + tool_name: tool_name.clone(), + session_id: TINYAGENTS_TOOL_SESSION.to_string(), + }, + ); + // Approval (HITL) now runs in `ApprovalSecurityMiddleware` // (`tinyagents/middleware.rs`, a `wrap_tool` middleware) so a denial // short-circuits before this executor is reached. @@ -206,11 +233,21 @@ async fn execute_openhuman_tool( Some(d) => match tokio::time::timeout(d, exec).await { Ok(r) => r, Err(_) => { + let elapsed_ms = started.elapsed().as_millis() as u64; tracing::warn!( tool = %call.name, timeout_secs, + elapsed_ms, "[tinyagents] tool timed out" ); + crate::core::event_bus::publish_global( + crate::core::event_bus::DomainEvent::ToolExecutionCompleted { + tool_name: tool_name.clone(), + session_id: TINYAGENTS_TOOL_SESSION.to_string(), + success: false, + elapsed_ms, + }, + ); return TaToolResult { call_id: call.id, name: call.name.clone(), @@ -220,13 +257,14 @@ async fn execute_openhuman_tool( ), raw: None, error: Some(format!("tool '{}' timed out", call.name)), - elapsed_ms: timeout_secs.saturating_mul(1000), + elapsed_ms, }; } }, None => exec.await, }; - match outcome { + let elapsed_ms = started.elapsed().as_millis() as u64; + let result = match outcome { Ok(result) => { let content = result.output_for_llm(true); let error = if result.is_error { @@ -240,7 +278,7 @@ async fn execute_openhuman_tool( content, raw: None, error, - elapsed_ms: 0, + elapsed_ms, } } Err(e) => { @@ -251,10 +289,22 @@ async fn execute_openhuman_tool( content: format!("Error executing {}: {e}", call.name), raw: None, error: Some(e.to_string()), - elapsed_ms: 0, + elapsed_ms, } } - } + }; + // Terminal per-tool telemetry (#4467, item 5): success is derived from the + // rendered result's error channel so a tool-reported error surfaces as a + // failed completion, mirroring the node-runtime bridge. + crate::core::event_bus::publish_global( + crate::core::event_bus::DomainEvent::ToolExecutionCompleted { + tool_name, + session_id: TINYAGENTS_TOOL_SESSION.to_string(), + success: result.error.is_none(), + elapsed_ms, + }, + ); + result } /// A harness tool backed by the routes' shared, `Arc`-owned tool registry sets diff --git a/src/openhuman/tokenjuice/savings.rs b/src/openhuman/tokenjuice/savings.rs index 17bbc0c75c..30e37bcc25 100644 --- a/src/openhuman/tokenjuice/savings.rs +++ b/src/openhuman/tokenjuice/savings.rs @@ -95,8 +95,9 @@ fn state() -> &'static Mutex { tokio::task_local! { /// The model actually running the current turn/sub-agent, scoped around - /// the tinyagents turn (`run_turn_via_tinyagents_shared`) (mirrors - /// [`crate::openhuman::agent::harness::model_vision_context`]). When set, + /// the tinyagents turn (`run_turn_via_tinyagents_shared`) — the same + /// task-local pattern as + /// [`crate::openhuman::agent::harness::turn_attachments_context`]. When set, /// compaction savings are priced against *this* model instead of the /// process-global configured default (issue #4122). Unset ⇒ fall back to /// the configured default, so non-harness callers and tests are unaffected diff --git a/src/openhuman/tool_status/ops.rs b/src/openhuman/tool_status/ops.rs index 1787f7128d..5f607b912f 100644 --- a/src/openhuman/tool_status/ops.rs +++ b/src/openhuman/tool_status/ops.rs @@ -30,6 +30,35 @@ pub fn classify(error_text: &str, timed_out: bool) -> ClassifiedFailure { fn classify_class(error_text: &str, timed_out: bool) -> ToolFailureClass { let text = error_text.to_lowercase(); + // 0. Structured policy markers win over *every* heuristic, including the + // `timed out` sniff below (#4459). Both markers are emitted upstream by + // the security/approval gate and survive the `Error: …` wrapping, so a + // marker hit is authoritative — a TTL-expiry deny reason literally + // contains "timed out", and must classify as an expired approval, never + // an execution Timeout that promises an auto-retry. + // + // `POLICY_BLOCKED_MARKER` — a hard, cross-turn block: the action is + // refused by the user's safety/autonomy policy (BlockedByPolicy). + if text.contains(crate::openhuman::security::POLICY_BLOCKED_MARKER) { + tracing::debug!("[tool_status::classify] matched POLICY_BLOCKED_MARKER -> BlockedByPolicy"); + return ToolFailureClass::BlockedByPolicy; + } + // `POLICY_DENIED_MARKER` — a this-turn denial: the user answered "no" at + // the approval prompt, the prompt's channel dropped, the origin was + // subconscious-tainted, or the prompt's TTL expired. All are + // non-retryable refusals (UserDeclined), split only by copy: a TTL + // expiry reads "approval expired", an explicit refusal reads "declined". + if text.contains(crate::openhuman::security::POLICY_DENIED_MARKER) { + if contains_any(&text, &["timed out", "timeout", "expired"]) { + tracing::debug!( + "[tool_status::classify] matched POLICY_DENIED_MARKER + expiry phrase -> ApprovalExpired" + ); + return ToolFailureClass::ApprovalExpired; + } + tracing::debug!("[tool_status::classify] matched POLICY_DENIED_MARKER -> Denied"); + return ToolFailureClass::Denied; + } + // 1. Timeout — the executor's explicit signal wins over any text sniffing. if timed_out || contains_any(&text, &["timed out", "timeout", "deadline exceeded"]) { return ToolFailureClass::Timeout; @@ -39,10 +68,13 @@ fn classify_class(error_text: &str, timed_out: bool) -> ToolFailureClass { // path. Checked *before* credentials so the OpenHuman-specific // `forbidden path` marker wins over the bare `forbidden` that a plain // external 403 body carries (routed to credentials below). Reserved for - // OpenHuman policy phrasing only — a hard policy block is normally tagged - // upstream with `POLICY_BLOCKED_MARKER` and never reaches this heuristic; - // bare HTTP `403`/`Forbidden` is an external authz failure, not our gate. + // OpenHuman policy phrasing only — a hard policy block is tagged upstream + // with `POLICY_BLOCKED_MARKER` and already short-circuited above (step 0); + // this heuristic only catches un-marked policy phrasing. Bare HTTP + // `403`/`Forbidden` is an external authz failure, not our gate. // `channel allows` is the tail of the tool-policy PermissionDenied render. + // (The old `"policy denied"` needle was dead — no producer emits that + // phrasing; the deny family uses `POLICY_DENIED_MARKER`, handled above.) if contains_any( &text, &[ @@ -52,7 +84,6 @@ fn classify_class(error_text: &str, timed_out: bool) -> ToolFailureClass { "not allowed by", "forbidden path", "autonomy", - "policy denied", ], ) { return ToolFailureClass::BlockedByPolicy; @@ -196,6 +227,14 @@ pub fn describe(class: ToolFailureClass) -> ClassifiedFailure { "The action took too long and was stopped.", "OpenHuman will try again, or you can retry it manually.", ), + ToolFailureClass::Denied => ( + "You declined this action.", + "Nothing to do — it was not run. Ask again if you change your mind.", + ), + ToolFailureClass::ApprovalExpired => ( + "The approval request expired before anyone responded.", + "Ask again to run it — OpenHuman won't retry it on its own.", + ), ToolFailureClass::Unknown => ( "Something went wrong with this action.", "Try again; if it keeps failing, run diagnostics from Settings.", @@ -412,6 +451,8 @@ mod tests { ToolFailureClass::BlockedByPolicy, ToolFailureClass::ModelConnection, ToolFailureClass::Timeout, + ToolFailureClass::Denied, + ToolFailureClass::ApprovalExpired, ToolFailureClass::Unknown, ] { let f = describe(class); diff --git a/src/openhuman/tool_status/types.rs b/src/openhuman/tool_status/types.rs index 14561772bb..8595416ff0 100644 --- a/src/openhuman/tool_status/types.rs +++ b/src/openhuman/tool_status/types.rs @@ -56,6 +56,14 @@ pub enum ToolFailureClass { ModelConnection, /// The action ran past its deadline and was stopped. Timeout, + /// The user (or the approval gate on their behalf) refused this action at + /// the approval prompt. Non-retryable — re-running just re-prompts for an + /// effect the user already declined (#4459). + Denied, + /// The approval prompt expired (TTL) before anyone responded. Non-retryable: + /// nobody approved, so it must not read as an execution timeout that + /// auto-retries (#4459). + ApprovalExpired, /// Could not be classified into any of the above. Unknown, } @@ -73,6 +81,11 @@ pub enum FailureCategory { /// Needs the user to act (grant permission, install an app, sign in) before /// the action can succeed. NeedsUserConfirmation, + /// The user declined the action, or the approval prompt expired before + /// anyone responded. Never retried automatically — auto-re-attempting a + /// refused external effect is exactly the bug this category prevents + /// (#4459). + UserDeclined, } /// A tool failure rendered for a non-technical user: what class it is, which @@ -113,6 +126,9 @@ impl ToolFailureClass { | ToolFailureClass::Timeout | ToolFailureClass::Unknown => FailureCategory::Recoverable, ToolFailureClass::BlockedByPolicy => FailureCategory::BlockedByPolicy, + ToolFailureClass::Denied | ToolFailureClass::ApprovalExpired => { + FailureCategory::UserDeclined + } ToolFailureClass::MissingPermission | ToolFailureClass::MissingApp | ToolFailureClass::BadCredentials => FailureCategory::NeedsUserConfirmation, @@ -129,6 +145,7 @@ mod tests { assert!(FailureCategory::Recoverable.is_recoverable()); assert!(!FailureCategory::BlockedByPolicy.is_recoverable()); assert!(!FailureCategory::NeedsUserConfirmation.is_recoverable()); + assert!(!FailureCategory::UserDeclined.is_recoverable()); } #[test] @@ -146,6 +163,8 @@ mod tests { assert_eq!(MissingPermission.category(), NeedsUserConfirmation); assert_eq!(MissingApp.category(), NeedsUserConfirmation); assert_eq!(BadCredentials.category(), NeedsUserConfirmation); + assert_eq!(Denied.category(), UserDeclined); + assert_eq!(ApprovalExpired.category(), UserDeclined); } #[test] diff --git a/src/openhuman/tools/impl/filesystem/update_memory_md.rs b/src/openhuman/tools/impl/filesystem/update_memory_md.rs index ed8c58a2ba..5b5dcf1fa5 100644 --- a/src/openhuman/tools/impl/filesystem/update_memory_md.rs +++ b/src/openhuman/tools/impl/filesystem/update_memory_md.rs @@ -3,12 +3,90 @@ use crate::openhuman::tools::traits::{PermissionLevel, Tool, ToolCallOptions, ToolResult}; use async_trait::async_trait; use serde_json::json; -use std::path::PathBuf; +use std::collections::HashMap; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Arc, LazyLock, Mutex}; use tinyagents::harness::tool::ToolExecutionContext; /// Allowed workspace markdown files this tool may modify. const ALLOWED_FILES: &[&str] = &["MEMORY.md", "SKILL.md"]; +/// Process-global registry of per-workspace write locks (#4458). +/// +/// `update_memory_md` performs a read-modify-write on `MEMORY.md`/`SKILL.md`. +/// The per-run `MemoryProtocolTracker` has zero cross-run awareness, so two +/// concurrent runs (parallel forks, cron) racing the same workspace file would +/// otherwise clobber each other's append. We serialize every write to a given +/// workspace directory through a shared async mutex, keyed by the canonicalized +/// (falling back to raw) workspace path, so concurrent index updates queue +/// instead of racing. Combined with the temp-file + atomic-rename write below, +/// a killed process can never leave a truncated file. +static WORKSPACE_WRITE_LOCKS: LazyLock>>>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + +/// Monotonic counter for temp-file uniqueness within a process. +static TEMP_FILE_SEQ: AtomicU64 = AtomicU64::new(0); + +/// Return (creating if needed) the shared async write lock for `workspace_dir`. +/// +/// The lock key is the canonicalized workspace path when it resolves (so two +/// spellings of the same directory share one lock), else the raw path. +fn workspace_write_lock(workspace_dir: &Path) -> Arc> { + let key = workspace_dir + .canonicalize() + .unwrap_or_else(|_| workspace_dir.to_path_buf()); + let mut map = WORKSPACE_WRITE_LOCKS + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + Arc::clone( + map.entry(key) + .or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))), + ) +} + +/// Atomically replace `path`'s contents with `content`. +/// +/// Writes to a sibling temp file in the same directory (so the rename stays on +/// one filesystem and is atomic) and `rename`s it over the target. A crash +/// mid-write leaves either the old file or the complete new file — never a +/// half-written truncation. Callers MUST hold the per-workspace write lock so +/// the read-modify-write is serialized end-to-end. +async fn atomic_write(path: &Path, file: &str, content: &str) -> anyhow::Result<()> { + let dir = path + .parent() + .ok_or_else(|| anyhow::anyhow!("target path has no parent directory"))?; + let seq = TEMP_FILE_SEQ.fetch_add(1, Ordering::Relaxed); + let tmp_name = format!(".{file}.{}.{seq}.tmp", std::process::id()); + let tmp_path = dir.join(tmp_name); + + tracing::debug!( + tmp = %tmp_path.display(), + target = %path.display(), + bytes = content.len(), + "[update_memory_md] atomic write: staging temp file" + ); + + if let Err(e) = tokio::fs::write(&tmp_path, content).await { + // Clean up a partially-written temp file so a failed stage doesn't + // litter the workspace (CodeRabbit: temp not cleaned on initial write). + let _ = tokio::fs::remove_file(&tmp_path).await; + return Err(anyhow::anyhow!("Failed to stage temp file for {file}: {e}")); + } + + if let Err(e) = tokio::fs::rename(&tmp_path, path).await { + // Best-effort cleanup so a failed rename doesn't litter the workspace. + let _ = tokio::fs::remove_file(&tmp_path).await; + return Err(anyhow::anyhow!("Failed to atomically write {file}: {e}")); + } + + tracing::debug!( + target = %path.display(), + "[update_memory_md] atomic write: rename committed" + ); + Ok(()) +} + /// Appends or replaces a named section in MEMORY.md or SKILL.md. /// /// Supports two actions: @@ -135,6 +213,16 @@ impl Tool for UpdateMemoryMdTool { tracing::debug!("[update_memory_md] action={action} file={file} path={target_path:?}"); + // #4458: serialize the whole read-modify-write against concurrent runs + // targeting the same workspace. The guard is held across read + atomic + // write so no interleaving append can be lost. + let lock = workspace_write_lock(&workspace_dir); + let _guard = lock.lock().await; + tracing::debug!( + workspace = %workspace_dir.display(), + "[update_memory_md] acquired per-workspace write lock" + ); + match action { "append" => self.do_append(&target_path, file, content).await, "replace_section" => { @@ -172,9 +260,7 @@ impl UpdateMemoryMdTool { }; let new_content = format!("{existing}{separator}{content}\n"); - tokio::fs::write(path, &new_content) - .await - .map_err(|e| anyhow::anyhow!("Failed to write {file}: {e}"))?; + atomic_write(path, file, &new_content).await?; let bytes = new_content.len(); tracing::info!( @@ -242,8 +328,7 @@ impl UpdateMemoryMdTool { format!("{existing}{separator}{heading}\n{content}\n") }; - std::fs::write(path, &new_file_content) - .map_err(|e| anyhow::anyhow!("Failed to write {file}: {e}"))?; + atomic_write(path, file, &new_file_content).await?; tracing::info!( "[update_memory_md] replaced section '{}' in {file} ({} bytes written)", diff --git a/src/openhuman/tools/ops.rs b/src/openhuman/tools/ops.rs index e0ae2c6342..199c1c2852 100644 --- a/src/openhuman/tools/ops.rs +++ b/src/openhuman/tools/ops.rs @@ -273,6 +273,19 @@ pub fn all_tools_with_runtime( Box::new(MemoryStoreTool::new(memory.clone(), security.clone())), Box::new(MemoryRecallTool::new(memory.clone())), Box::new(MemoryForgetTool::new(memory.clone(), security.clone())), + // #4458: the memory read→dedupe→write→update-index protocol + // (`agent::harness::memory_protocol`) can only close its write cycle via a + // successful `update_memory_md` call, and the archivist's `[tools] named` + // allowlist selects it — but subagents only filter the *parent* tool set, + // so if this tool is absent from the registry the archivist silently loses + // it and the model hits a permanent unsatisfiable "call update_memory_md" + // nag loop (unknown-tool error → the tracker never sees IndexUpdate). It is + // always registered here (same as the other memory tools); per-agent + // visibility is governed by each agent's `named` allowlist. Targets the + // workspace `MEMORY.md`/`SKILL.md` (where `channels_prompt`/`session_memory` + // read them from), and prefers the live TinyAgents workspace descriptor at + // execution time when one is present. + Box::new(UpdateMemoryMdTool::new(root_config.workspace_dir.clone())), // #002: read-only self-diagnosis of the memory pipeline so the agent // can explain an empty/stalled wiki + the fix. Box::new(MemoryDoctorTool::new(config.clone())),