From d545f43ce5208deae7597fde5c63891b7d8c50db Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Thu, 19 Mar 2026 17:21:53 +0100 Subject: [PATCH] refactor(epic-1977): agent state accessors, OpenAI structured path fix, builder warnings ABS-04: Add state-group accessor methods to Agent (accessors.rs) for all sub-structs. Accessors are pub(super), named identically to fields, enabling incremental migration without breaking existing direct field access. Migration pattern: self.msg.x -> self.msg().x. ABS-05: Fix convert_messages_structured() in OpenAI provider to preserve Recall, CodeContext, Summary, CrossSession variants in tool-use messages. Previously, the MessagePart::Text-only filter on assistant messages and the wildcard `_ => {}` on user messages silently dropped these internal variants. Both arms now use as_plain_text() which handles all text-like variants uniformly. Add regression test. ABS-07: Add tracing::warn in with_context_budget() when budget_tokens == 0. Add debug_assert in Agent::new_with_registry_arc() for max_active_skills > 0. --- CHANGELOG.md | 4 + crates/zeph-core/src/agent/accessors.rs | 245 ++++++++++++++++++++++++ crates/zeph-core/src/agent/builder.rs | 3 + crates/zeph-core/src/agent/mod.rs | 2 + crates/zeph-llm/src/openai/mod.rs | 22 +-- crates/zeph-llm/src/openai/tests.rs | 52 +++++ 6 files changed, 316 insertions(+), 12 deletions(-) create mode 100644 crates/zeph-core/src/agent/accessors.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index dbf7df3e..01e3d192 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Changed +- refactor(core): add state-group accessor methods to `Agent` for all sub-structs (`msg`, `memory_state`, `skill_state`, `runtime`, etc.); migration from direct field access is incremental per file (ABS-04, epic #1977) +- fix(llm): `convert_messages_structured()` now preserves `Recall`, `CodeContext`, `Summary`, and `CrossSession` variants in OpenAI tool-use messages instead of silently dropping them (ABS-05, epic #1977) +- refactor(core): `with_context_budget()` emits `tracing::warn` when `budget_tokens == 0`; `Agent::new()` has `debug_assert` for `max_active_skills > 0` (ABS-07, epic #1977) + - refactor(llm): extract `UsageTracker` struct to consolidate duplicate token usage tracking across Claude, OpenAI, Ollama, and Gemini providers (DRY-01+06, epic #1975) - refactor(memory): remove duplicate `BoxFuture` type alias from `in_memory_store.rs`; import canonical definition from `vector_store.rs` (DRY-05, epic #1975) - refactor(channels): add `ChannelError::other()` helper; replace 15 `.map_err(|e| ChannelError::Other(e.to_string()))` sites in telegram, discord, slack, and cli channels (DRY-04, epic #1975) diff --git a/crates/zeph-core/src/agent/accessors.rs b/crates/zeph-core/src/agent/accessors.rs new file mode 100644 index 00000000..e4a1546f --- /dev/null +++ b/crates/zeph-core/src/agent/accessors.rs @@ -0,0 +1,245 @@ +// SPDX-FileCopyrightText: 2026 Andrei G +// SPDX-License-Identifier: MIT OR Apache-2.0 + +//! State-group accessor methods for `Agent`. +//! +//! These methods return shared or mutable references to entire state sub-structs, +//! providing encapsulation and a stable API boundary for internal agent state. +//! Mutable access requires an exclusive borrow of `Agent` — simultaneous +//! mutable + immutable access to different sub-structs must use direct field +//! access or sequence the borrows explicitly. +//! +//! Migration pattern: `self.memory_state.field` → `self.memory_state().field` + +#[cfg(feature = "context-compression")] +use super::state::CompressionState; +use super::{ + Agent, + focus::FocusState, + learning_engine::LearningEngine, + sidequest::SidequestState, + state::{ + DebugState, ExperimentState, FeedbackState, IndexState, InstructionState, LifecycleState, + McpState, MemoryState, MessageState, MetricsState, OrchestrationState, ProviderState, + RuntimeConfig, SecurityState, SessionState, SkillState, + }, + tool_orchestrator::ToolOrchestrator, +}; +use crate::channel::Channel; + +// Migration is incremental: accessors are defined here and callers are migrated file-by-file. +// During the transition period, not all accessors may be called yet. +#[allow(dead_code)] +impl Agent { + #[must_use] + pub(super) fn msg(&self) -> &MessageState { + &self.msg + } + + #[must_use] + pub(super) fn msg_mut(&mut self) -> &mut MessageState { + &mut self.msg + } + + #[must_use] + pub(super) fn memory_state(&self) -> &MemoryState { + &self.memory_state + } + + #[must_use] + pub(super) fn memory_state_mut(&mut self) -> &mut MemoryState { + &mut self.memory_state + } + + #[must_use] + pub(super) fn skill_state(&self) -> &SkillState { + &self.skill_state + } + + #[must_use] + pub(super) fn skill_state_mut(&mut self) -> &mut SkillState { + &mut self.skill_state + } + + #[must_use] + pub(super) fn runtime(&self) -> &RuntimeConfig { + &self.runtime + } + + #[must_use] + pub(super) fn runtime_mut(&mut self) -> &mut RuntimeConfig { + &mut self.runtime + } + + #[must_use] + pub(super) fn session(&self) -> &SessionState { + &self.session + } + + #[must_use] + pub(super) fn session_mut(&mut self) -> &mut SessionState { + &mut self.session + } + + #[must_use] + pub(super) fn debug_state(&self) -> &DebugState { + &self.debug_state + } + + #[must_use] + pub(super) fn debug_state_mut(&mut self) -> &mut DebugState { + &mut self.debug_state + } + + #[must_use] + pub(super) fn security(&self) -> &SecurityState { + &self.security + } + + #[must_use] + pub(super) fn security_mut(&mut self) -> &mut SecurityState { + &mut self.security + } + + #[must_use] + pub(super) fn mcp(&self) -> &McpState { + &self.mcp + } + + #[must_use] + pub(super) fn mcp_mut(&mut self) -> &mut McpState { + &mut self.mcp + } + + #[must_use] + pub(super) fn index(&self) -> &IndexState { + &self.index + } + + #[must_use] + pub(super) fn index_mut(&mut self) -> &mut IndexState { + &mut self.index + } + + #[must_use] + pub(super) fn feedback(&self) -> &FeedbackState { + &self.feedback + } + + #[must_use] + pub(super) fn feedback_mut(&mut self) -> &mut FeedbackState { + &mut self.feedback + } + + #[must_use] + pub(super) fn instructions(&self) -> &InstructionState { + &self.instructions + } + + #[must_use] + pub(super) fn instructions_mut(&mut self) -> &mut InstructionState { + &mut self.instructions + } + + #[must_use] + pub(super) fn lifecycle(&self) -> &LifecycleState { + &self.lifecycle + } + + #[must_use] + pub(super) fn lifecycle_mut(&mut self) -> &mut LifecycleState { + &mut self.lifecycle + } + + #[must_use] + pub(super) fn providers(&self) -> &ProviderState { + &self.providers + } + + #[must_use] + pub(super) fn providers_mut(&mut self) -> &mut ProviderState { + &mut self.providers + } + + #[must_use] + pub(super) fn metrics(&self) -> &MetricsState { + &self.metrics + } + + #[must_use] + pub(super) fn metrics_mut(&mut self) -> &mut MetricsState { + &mut self.metrics + } + + #[must_use] + pub(super) fn orchestration(&self) -> &OrchestrationState { + &self.orchestration + } + + #[must_use] + pub(super) fn orchestration_mut(&mut self) -> &mut OrchestrationState { + &mut self.orchestration + } + + #[must_use] + pub(super) fn experiments(&self) -> &ExperimentState { + &self.experiments + } + + #[must_use] + pub(super) fn experiments_mut(&mut self) -> &mut ExperimentState { + &mut self.experiments + } + + #[must_use] + pub(super) fn focus(&self) -> &FocusState { + &self.focus + } + + #[must_use] + pub(super) fn focus_mut(&mut self) -> &mut FocusState { + &mut self.focus + } + + #[must_use] + pub(super) fn sidequest(&self) -> &SidequestState { + &self.sidequest + } + + #[must_use] + pub(super) fn sidequest_mut(&mut self) -> &mut SidequestState { + &mut self.sidequest + } + + #[must_use] + pub(super) fn tool_orchestrator(&self) -> &ToolOrchestrator { + &self.tool_orchestrator + } + + #[must_use] + pub(super) fn tool_orchestrator_mut(&mut self) -> &mut ToolOrchestrator { + &mut self.tool_orchestrator + } + + #[must_use] + pub(super) fn learning_engine(&self) -> &LearningEngine { + &self.learning_engine + } + + #[must_use] + pub(super) fn learning_engine_mut(&mut self) -> &mut LearningEngine { + &mut self.learning_engine + } + + #[cfg(feature = "context-compression")] + #[must_use] + pub(super) fn compression(&self) -> &CompressionState { + &self.compression + } + + #[cfg(feature = "context-compression")] + #[must_use] + pub(super) fn compression_mut(&mut self) -> &mut CompressionState { + &mut self.compression + } +} diff --git a/crates/zeph-core/src/agent/builder.rs b/crates/zeph-core/src/agent/builder.rs index 9efbbf49..cbedbde2 100644 --- a/crates/zeph-core/src/agent/builder.rs +++ b/crates/zeph-core/src/agent/builder.rs @@ -538,6 +538,9 @@ impl Agent { compaction_preserve_tail: usize, prune_protect_tokens: usize, ) -> Self { + if budget_tokens == 0 { + tracing::warn!("context budget is 0 — agent will have no token tracking"); + } if budget_tokens > 0 { self.context_manager.budget = Some(ContextBudget::new(budget_tokens, reserve_ratio)); } diff --git a/crates/zeph-core/src/agent/mod.rs b/crates/zeph-core/src/agent/mod.rs index 34f1236d..e149e24c 100644 --- a/crates/zeph-core/src/agent/mod.rs +++ b/crates/zeph-core/src/agent/mod.rs @@ -1,6 +1,7 @@ // SPDX-FileCopyrightText: 2026 Andrei G // SPDX-License-Identifier: MIT OR Apache-2.0 +mod accessors; mod builder; pub(crate) mod compaction_strategy; #[cfg(feature = "compression-guidelines")] @@ -192,6 +193,7 @@ impl Agent { max_active_skills: usize, tool_executor: impl ToolExecutor + 'static, ) -> Self { + debug_assert!(max_active_skills > 0, "max_active_skills must be > 0"); let all_skills: Vec = { let reg = registry.read().expect("registry read lock poisoned"); reg.all_meta() diff --git a/crates/zeph-llm/src/openai/mod.rs b/crates/zeph-llm/src/openai/mod.rs index 57d2ef02..0e10d985 100644 --- a/crates/zeph-llm/src/openai/mod.rs +++ b/crates/zeph-llm/src/openai/mod.rs @@ -1052,10 +1052,7 @@ fn convert_messages_structured(messages: &[Message]) -> Vec Some(text.as_str()), - _ => None, - }) + .filter_map(|p| p.as_plain_text()) .collect::>() .join(""); @@ -1106,15 +1103,16 @@ fn convert_messages_structured(messages: &[Message]) -> Vec { - result.push(StructuredApiMessage { - role: "user".to_owned(), - content: Some(text.clone()), - tool_calls: None, - tool_call_id: None, - }); + other => { + if let Some(text) = other.as_plain_text().filter(|t| !t.is_empty()) { + result.push(StructuredApiMessage { + role: "user".to_owned(), + content: Some(text.to_owned()), + tool_calls: None, + tool_call_id: None, + }); + } } - _ => {} } } } diff --git a/crates/zeph-llm/src/openai/tests.rs b/crates/zeph-llm/src/openai/tests.rs index 693b3ff4..553d6f23 100644 --- a/crates/zeph-llm/src/openai/tests.rs +++ b/crates/zeph-llm/src/openai/tests.rs @@ -1397,3 +1397,55 @@ fn normalize_nested_objects_get_additional_properties() { assert_eq!(schema["additionalProperties"], false); assert_eq!(schema["properties"]["inner"]["additionalProperties"], false); } + +#[test] +fn convert_messages_structured_preserves_internal_variants() { + // Recall/CodeContext/Summary/CrossSession in tool-use messages must not be silently dropped. + // Previously, the wildcard `_ => {}` in convert_messages_structured() dropped these variants. + let messages = vec![ + // Assistant message with Recall + ToolUse: Recall text must appear in content + Message::from_parts( + Role::Assistant, + vec![ + MessagePart::Recall { + text: "past context".into(), + }, + MessagePart::ToolUse { + id: "call_1".into(), + name: "search".into(), + input: serde_json::json!({}), + }, + ], + ), + // User message with ToolResult + CodeContext: CodeContext must not be dropped + Message::from_parts( + Role::User, + vec![ + MessagePart::ToolResult { + tool_use_id: "call_1".into(), + content: "result".into(), + is_error: false, + }, + MessagePart::CodeContext { + text: "fn main() {}".into(), + }, + ], + ), + ]; + let result = convert_messages_structured(&messages); + // Assistant message: content must include the Recall text + assert_eq!(result[0].role, "assistant"); + assert_eq!(result[0].content.as_deref(), Some("past context")); + assert!(result[0].tool_calls.is_some()); + // User messages: ToolResult becomes role "tool", CodeContext becomes role "user" + let tool_msg = result + .iter() + .find(|m| m.role == "tool") + .expect("tool message"); + assert_eq!(tool_msg.content.as_deref(), Some("result")); + let user_msg = result + .iter() + .find(|m| m.role == "user") + .expect("user message"); + assert_eq!(user_msg.content.as_deref(), Some("fn main() {}")); +}