diff --git a/CHANGELOG.md b/CHANGELOG.md index 15fdc114e..c9fb1c78b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -133,6 +133,36 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). `split_messages`/`split_messages_structured` functions are no longer imported at the `claude` module's top level, so a future request path cannot easily split a history without the strip being applied. +- MagicDocs registration (`crates/zeph-core/src/agent/magic_docs.rs`) never fired for a + turn's terminal assistant text response — the two sites in `tier_loop.rs` that push the + final response (`process_response_native_tools`'s semantic-cache-hit branch and + `process_single_native_turn`'s `ChatResponse::Text` branch) wrote directly to + `self.msg.messages` instead of calling `self.push_message(...)`, bypassing + `detect_magic_docs_in_messages()` entirely (#6127). Detection only ran retroactively, the + next time an `Assistant` message was pushed via `push_message` — typically a *further* + tool call later in the same conversation — so a single read-then-respond turn (the + canonical `--bare -p "read X"` usage) never registered a `# MAGIC DOC:` file, even though + the read call itself succeeded and returned the marked content. Not a `--bare`-conditional + gate: `with_bare_mode` was unaffected and required no change; this was a general tool-loop + message-push coverage gap that `--bare`'s single-shot invocation pattern exposed + deterministically, while interactive multi-tool-call sessions usually masked it. Both push + sites now route through `push_message`, matching the pattern already used correctly by + `push_assistant_tool_use_message` and `process_tool_result_batch`. As a side effect, both + paths now also update `cached_prompt_tokens` and `last_assistant_at`, which the raw pushes + were silently skipping — a pre-existing token-accounting gap on the semantic-cache-hit and + plain-text-response paths, corrected incidentally by the same fix. + `detect_magic_docs_in_messages()`'s own detection guard was the deeper defect underneath + the above: it only scanned when the *last* pushed message was `Role::Assistant`, so + detection for a magic-doc read was always deferred to the next assistant push — a turn + that instead exited the native tool loop via a shutdown/user-cancel/doom-loop break or + `max_iterations` exhaustion, leaving an unpaired `Role::User` tool-result as the last + message, would still silently drop the doc even after the two `push_message` routing + fixes above. The guard now also scans when the last message is a `Role::User` message + carrying `ToolResult`/`ToolOutput` parts, so detection fires uniformly at the point the + content actually arrives, covering all native-loop exit paths, not just the two terminal + push sites. `focus.rs`'s context-compression checkpoint re-push (a `ToolUse`-only + assistant message with no paired result yet present) is intentionally left as a raw push + and documented inline as such. - `.github/deny.toml`: the `cargo-deny` advisories gate scanned only the `candle` and `tui` features, excluding `pdf` and every other feature shipped by the blocking `bundle-check` (`full`) CI job and release builds — any RUSTSEC diff --git a/crates/zeph-core/src/agent/magic_docs.rs b/crates/zeph-core/src/agent/magic_docs.rs index e9187d735..ceaa668ae 100644 --- a/crates/zeph-core/src/agent/magic_docs.rs +++ b/crates/zeph-core/src/agent/magic_docs.rs @@ -38,24 +38,40 @@ impl MagicDocsState { } impl super::Agent { - /// Detect `# MAGIC DOC:` headers in `ToolOutput` parts and register their paths. + /// Detect `# MAGIC DOC:` headers in `ToolOutput`/`ToolResult` parts and register their paths. /// - /// Call this after pushing an assistant message that may contain `ToolOutput` parts. - /// No-op when `MagicDocs` is disabled. + /// Call this after pushing an assistant message, or a user message carrying tool + /// results, that may contain `ToolOutput`/`ToolResult` parts. No-op when `MagicDocs` + /// is disabled. pub(super) fn detect_magic_docs_in_messages(&mut self) { if !self.services.memory.subsystems.magic_docs_config.enabled { return; } - // Scan the last assistant message for ToolOutput parts from file-read tools. + // Scan on the last assistant message (may carry fresh ToolUse requests) or the + // last user message carrying tool results — the latter is where a magic-doc + // header actually arrives (`process_tool_result_batch`'s push). Scanning only on + // assistant pushes deferred detection to the *next* assistant message, which + // never comes for a single read-then-respond turn or for a turn that exits the + // native loop via shutdown/cancel/doom-loop/max-iterations with an unpaired tool + // result as the last message (#6127). let Some(last_msg) = self.msg.messages.last() else { return; }; - if last_msg.role != Role::Assistant { + let has_tool_result = last_msg.parts.iter().any(|p| { + matches!( + p, + MessagePart::ToolResult { .. } | MessagePart::ToolOutput { .. } + ) + }); + if last_msg.role != Role::Assistant && !(last_msg.role == Role::User && has_tool_result) { return; } - // Walk all messages looking for ToolUse → ToolOutput pairs where ToolOutput has magic header. + // Walk all messages looking for ToolUse → ToolOutput pairs where ToolOutput has magic + // header. Scanning on every tool-result push (not just assistant pushes) makes this + // O(n) per tool-result turn instead of only per assistant turn — accepted trade-off to + // close the coverage gap above; typical turn/tool-call counts keep it cheap. self.scan_messages_for_magic_docs(); } diff --git a/crates/zeph-core/src/agent/tool_execution/focus.rs b/crates/zeph-core/src/agent/tool_execution/focus.rs index ecdd0de45..0df10bebc 100644 --- a/crates/zeph-core/src/agent/tool_execution/focus.rs +++ b/crates/zeph-core/src/agent/tool_execution/focus.rs @@ -164,6 +164,9 @@ impl Agent { }; self.msg.messages.truncate(checkpoint_pos); if let Some(assistant_msg) = current_turn_assistant { + // TODO(critic): focus checkpoint re-pushes a ToolUse-only assistant message; + // no pairable ToolResult present, so magic-doc detection is intentionally + // skipped here. self.msg.messages.push(assistant_msg); } self.recompute_prompt_tokens(); diff --git a/crates/zeph-core/src/agent/tool_execution/tests/sanitize_and_native_tests.rs b/crates/zeph-core/src/agent/tool_execution/tests/sanitize_and_native_tests.rs index 6a9de1127..6f8b5497c 100644 --- a/crates/zeph-core/src/agent/tool_execution/tests/sanitize_and_native_tests.rs +++ b/crates/zeph-core/src/agent/tool_execution/tests/sanitize_and_native_tests.rs @@ -1370,3 +1370,286 @@ async fn nli_and_secret_masking_are_independently_toggleable() { assert!(!rx.borrow().nli_enabled); assert!(!rx.borrow().secret_masking_enabled); } + +// --------------------------------------------------------------------------- +// #6127 regression: MagicDocs registration for a single read-then-respond turn with no +// subsequent tool call (the shape `--bare -p "read X"` sessions always take). +// --------------------------------------------------------------------------- + +/// #6127: a single read-then-respond turn — `Assistant{ToolUse(read)}` -> +/// `User{ToolResult(# MAGIC DOC: ...)}` -> terminal `Assistant{Text}` with NO further tool +/// call, exactly the shape `--bare -p "read X"` sessions always take — must register the doc. +/// +/// Before the fix, `detect_magic_docs_in_messages()` only scanned when the *last pushed +/// message* had `role == Assistant`. The `ToolResult` carrying the magic-doc header is pushed +/// with `role == User` (`process_tool_result_batch`, `tier_loop.rs`), so that push's scan bailed +/// immediately; detection was deferred to the *next* `Assistant` push, which never comes for a +/// single read-then-respond turn. The fix broadens the guard in +/// `crates/zeph-core/src/agent/magic_docs.rs` to also scan when the last message is a `User` +/// message carrying `ToolResult`/`ToolOutput` parts, so registration happens synchronously at +/// the tool-result push instead of being deferred to a tool call that may never arrive. Drives +/// the real `process_response()` -> `process_response_native_tools()` -> +/// `process_single_native_turn()` path (not a hand-constructed message history), so this test +/// only passes if the production code actually detects on the `ToolResult` push. +#[tokio::test] +#[allow(clippy::large_futures)] +async fn magic_doc_registered_after_single_read_then_respond_turn() { + use crate::agent::agent_tests::*; + use zeph_llm::any::AnyProvider; + use zeph_llm::mock::MockProvider; + use zeph_llm::provider::{ChatResponse, Message, MessageMetadata, Role, ToolUseRequest}; + + let tool_call = ToolUseRequest { + id: "tu_readme".into(), + name: "read".into(), + input: serde_json::json!({"file_path": "/docs/readme.md"}), + }; + let (mock, call_count) = MockProvider::with_responses(vec![]).with_tool_use(vec![ + ChatResponse::ToolUse { + text: None, + tool_calls: vec![tool_call], + thinking_blocks: vec![], + }, + ChatResponse::Text("Here's a summary of the file.".into()), + ]); + let provider = AnyProvider::Mock(mock); + + let channel = MockChannel::new(vec![]); + let registry = create_test_registry(); + let executor = MockToolExecutor::with_output("read", "# MAGIC DOC: readme\nSome content."); + let mut agent = crate::agent::Agent::new(provider, channel, registry, None, 5, executor); + + agent.services.memory.subsystems.magic_docs_config.enabled = true; + // Disable sanitizer spotlighting so the ToolResult content is the raw tool summary, + // keeping this test focused on the push_message wiring, not sanitizer output shape. + agent.services.security.sanitizer = + zeph_sanitizer::ContentSanitizer::new(&zeph_sanitizer::ContentIsolationConfig { + enabled: false, + ..Default::default() + }); + + agent.msg.messages.push(Message { + role: Role::User, + content: "read /docs/readme.md and summarize it".into(), + parts: vec![], + metadata: MessageMetadata::default(), + }); + + agent.process_response().await.unwrap(); + + assert_eq!( + *call_count.lock().unwrap(), + 2, + "provider must be called twice: once for ToolUse, once for the terminal Text response" + ); + let sent = agent.channel.sent_messages(); + assert!( + sent.iter().any(|s| s == "Here's a summary of the file."), + "terminal text response must be sent to the channel; got: {sent:?}" + ); + + assert!( + agent + .services + .memory + .subsystems + .magic_docs + .registered + .contains_key(&std::path::PathBuf::from("/docs/readme.md")), + "magic doc must be registered after the terminal text response of a single \ + read-then-respond turn, without requiring a second tool call; registered = {:?}", + agent.services.memory.subsystems.magic_docs.registered + ); +} + +/// #6127 companion regression: the `CacheCheckResult::Hit` branch (semantic-cache-hit path) +/// shares the same raw-push bug as the plain-text branch. Seeds message history with a +/// `read` `ToolUse`/`ToolResult` pair carrying a `# MAGIC DOC:` header (as if loaded from a +/// prior turn's persisted session state, before this session ever ran detection on it), primes +/// the response cache so `check_response_cache()` returns a `Hit` for the current last-user +/// message, then drives the real `process_response()` path. The LLM must never be called (pure +/// cache hit), yet the doc must still be registered — proving the `CacheCheckResult::Hit` arm's +/// terminal push also goes through `push_message()`. +#[tokio::test] +#[allow(clippy::large_futures)] +async fn magic_doc_registered_on_semantic_cache_hit_branch() { + use crate::agent::agent_tests::*; + use std::sync::Arc; + use zeph_llm::any::AnyProvider; + use zeph_llm::mock::MockProvider; + use zeph_llm::provider::{Message, MessageMetadata, MessagePart, Role}; + use zeph_memory::{ResponseCache, store::SqliteStore}; + + let (mock, call_count) = MockProvider::with_responses(vec![]).with_tool_use(vec![]); + let provider = AnyProvider::Mock(mock); + + let channel = MockChannel::new(vec![]); + let registry = create_test_registry(); + let executor = MockToolExecutor::no_tools(); + let mut agent = crate::agent::Agent::new(provider, channel, registry, None, 5, executor); + + agent.services.memory.subsystems.magic_docs_config.enabled = true; + + // Fixture setup: seed the ToolUse/ToolResult pair directly (not via push_message) to + // simulate history already present before this turn's cache-hit branch runs — this test + // targets the Hit branch's own push, not the (already covered) plain-text branch. + agent.msg.messages.push(Message { + role: Role::User, + content: "read /docs/design.md and summarize it".into(), + parts: vec![], + metadata: MessageMetadata::default(), + }); + agent.msg.messages.push(Message::from_parts( + Role::Assistant, + vec![MessagePart::ToolUse { + id: "tu_design".into(), + name: "read".into(), + input: serde_json::json!({"file_path": "/docs/design.md"}), + }], + )); + let tool_result_msg = Message::from_parts( + Role::User, + vec![MessagePart::ToolResult { + tool_use_id: "tu_design".into(), + content: "# MAGIC DOC: Design\nSome content.".into(), + is_error: false, + }], + ); + agent.msg.messages.push(tool_result_msg.clone()); + + let store = SqliteStore::new(":memory:").await.unwrap(); + let cache = Arc::new(ResponseCache::new(store.pool().clone(), 3600)); + let key = + ResponseCache::compute_key(&tool_result_msg.content, &agent.runtime.config.model_name); + cache + .put(&key, "cached summary", &agent.runtime.config.model_name) + .await + .unwrap(); + agent.services.session.response_cache = Some(cache); + + agent.process_response().await.unwrap(); + + assert_eq!( + *call_count.lock().unwrap(), + 0, + "provider must not be called at all on a semantic cache hit" + ); + let sent = agent.channel.sent_messages(); + assert!( + sent.iter().any(|s| s == "cached summary"), + "cached response must be sent to the channel; got: {sent:?}" + ); + + assert!( + agent + .services + .memory + .subsystems + .magic_docs + .registered + .contains_key(&std::path::PathBuf::from("/docs/design.md")), + "magic doc must be registered after the CacheCheckResult::Hit branch's terminal push; \ + registered = {:?}", + agent.services.memory.subsystems.magic_docs.registered + ); +} + +/// #6127 companion regression: a native-loop exit where the LAST message of the turn is the +/// `ToolResult` push itself — no terminal `Assistant` text ever follows, because the loop exits +/// via `max_iterations` exhaustion right after the tool result is recorded. This is one of four +/// exit branches (shutdown/user-cancel/doom-loop/`max_iterations`) that share the same shape: +/// `process_tool_result_batch` pushes the `ToolResult` via `push_message()` (this call site was +/// never buggy), then the native loop simply stops without ever reaching a terminal +/// `ChatResponse::Text` or `CacheCheckResult::Hit` push. +/// +/// Before the fix, `detect_magic_docs_in_messages()`'s guard only scanned when the *last +/// pushed message* had `role == Assistant`; a `ToolResult` push (`role == User`) always +/// returned early, so a turn that terminates on this `ToolResult` (no further Assistant push ever +/// arrives) could never register the doc — independent of the two `tier_loop.rs` raw-push +/// sites. The fix broadens the guard in `crates/zeph-core/src/agent/magic_docs.rs` to also +/// scan when the last message is a `User` message carrying `ToolResult`/`ToolOutput` parts, +/// which covers this exit path (and the other three) uniformly with no `tier_loop.rs` change +/// required for them specifically. +#[tokio::test] +#[allow(clippy::large_futures)] +async fn magic_doc_registered_when_tool_result_is_final_message_of_max_iterations_exit() { + use crate::agent::agent_tests::*; + use zeph_llm::any::AnyProvider; + use zeph_llm::mock::MockProvider; + use zeph_llm::provider::{ChatResponse, Message, MessageMetadata, Role, ToolUseRequest}; + + let tool_call = ToolUseRequest { + id: "tu_arch".into(), + name: "read".into(), + input: serde_json::json!({"file_path": "/docs/architecture.md"}), + }; + // Only ONE ToolUse response is queued: with max_iterations = 1, the native loop calls + // chat_with_tools exactly once, processes the tool result, then exhausts its iteration + // budget and exits — the provider is never asked for a follow-up terminal response. + let (mock, call_count) = + MockProvider::with_responses(vec![]).with_tool_use(vec![ChatResponse::ToolUse { + text: None, + tool_calls: vec![tool_call], + thinking_blocks: vec![], + }]); + let provider = AnyProvider::Mock(mock); + + let channel = MockChannel::new(vec![]); + let registry = create_test_registry(); + let executor = + MockToolExecutor::with_output("read", "# MAGIC DOC: architecture\nSome content."); + let mut agent = crate::agent::Agent::new(provider, channel, registry, None, 5, executor); + + agent.services.memory.subsystems.magic_docs_config.enabled = true; + agent.services.security.sanitizer = + zeph_sanitizer::ContentSanitizer::new(&zeph_sanitizer::ContentIsolationConfig { + enabled: false, + ..Default::default() + }); + agent.tool_orchestrator.max_iterations = 1; + + agent.msg.messages.push(Message { + role: Role::User, + content: "read /docs/architecture.md and summarize it".into(), + parts: vec![], + metadata: MessageMetadata::default(), + }); + + agent.process_response().await.unwrap(); + + assert_eq!( + *call_count.lock().unwrap(), + 1, + "provider must be called exactly once: the loop must exit on max_iterations \ + exhaustion without a follow-up call for a terminal response" + ); + + let last = agent.msg.messages.last().expect("at least one message"); + assert_eq!( + last.role, + Role::User, + "the last message of the turn must be the ToolResult push itself, with no \ + terminal Assistant push ever following; got role {:?}", + last.role + ); + assert!( + last.parts + .iter() + .any(|p| matches!(p, zeph_llm::provider::MessagePart::ToolResult { .. })), + "last message must carry the ToolResult part; got: {:?}", + last.parts + ); + + assert!( + agent + .services + .memory + .subsystems + .magic_docs + .registered + .contains_key(&std::path::PathBuf::from("/docs/architecture.md")), + "magic doc must be registered even when the turn ends on the ToolResult push itself \ + (max_iterations exhaustion, no further Assistant push); registered = {:?}", + agent.services.memory.subsystems.magic_docs.registered + ); +} diff --git a/crates/zeph-core/src/agent/tool_execution/tier_loop.rs b/crates/zeph-core/src/agent/tool_execution/tier_loop.rs index a440d31a4..15dde3e19 100644 --- a/crates/zeph-core/src/agent/tool_execution/tier_loop.rs +++ b/crates/zeph-core/src/agent/tool_execution/tier_loop.rs @@ -2945,9 +2945,7 @@ impl Agent { CacheCheckResult::Hit(cached) => { self.persist_message(Role::Assistant, &cached, &[], false) .await; - self.msg - .messages - .push(Message::from_legacy(Role::Assistant, cached.as_str())); + self.push_message(Message::from_legacy(Role::Assistant, cached.as_str())); if cached.contains(zeph_llm::provider::MAX_TOKENS_TRUNCATION_MARKER) { let _ = self.channel.send_stop_hint(StopHint::MaxTokens).await; } @@ -3172,9 +3170,7 @@ impl Agent { } self.persist_message(Role::Assistant, &cleaned, &[], false) .await; - self.msg - .messages - .push(Message::from_legacy(Role::Assistant, cleaned.as_str())); + self.push_message(Message::from_legacy(Role::Assistant, cleaned.as_str())); // Detect context loss after compaction and log failure pair if found. self.maybe_log_compression_failure(&cleaned).await; if cleaned.contains(zeph_llm::provider::MAX_TOKENS_TRUNCATION_MARKER) {