From 19aa50a4294ca5b30bdb7a023301d7a747577ec9 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Thu, 16 Apr 2026 05:01:36 +0530 Subject: [PATCH] fix(agent): thread agent_id into build_session_agent_inner (#584) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build_session_agent_inner took an agent_id: &str parameter but never threaded it into the Agent::builder() chain — a `let _ = agent_id;` silenced the unused-variable warning. AgentBuilder::build() fell back to the legacy "main" default at builder.rs:310-312, so every session built via Agent::from_config_for_agent carried agent_definition_name="main" regardless of the id the caller asked for. Only two ids reach this path in production: "orchestrator" (legacy Agent::from_config wrapper, cron, local_ai, escalation) and "welcome" (channels/providers/web.rs routing after #525/#526, and welcome_proactive). Orchestrator is benign — "main" was already an alias for orchestrator everywhere downstream (see debug_dump.rs:249). Welcome is the visible bug — agent_definition_name feeds three surfaces that are all silently wrong pre-fix: 1. Transcript filename — welcome sessions land as main_*.md instead of welcome_*.md. 2. Transcript metadata — the block stamps `agent: main` instead of `agent: welcome`. 3. Transcript resume cross-contamination — try_load_session_transcript calls find_latest_transcript(workspace_dir, "main") which scans all dated session dirs for the latest main_*.md. It finds the last orchestrator session and resumes from it, loading its system prompt + user messages + assistant tool-call history into the welcome agent's `history` as a resume prefix before run_single runs. The written transcript mixes yesterday's orchestrator email thread with today's welcome message under the same main_0.md filename. Reproduced live 2026-04-16: a welcome_proactive session loaded 5 messages from yesterday's 15042026/main_0.md and wrote 6 messages back that included a spawn_subagent(skills_agent, toolkit=gmail) tool-call the welcome agent never made. Prompt rendering is NOT affected. context/prompt.rs only reads ctx.agent_id for the is_skill_executor branch at prompt.rs:655, so welcome/main/orchestrator all fall through the same delegator path. The welcome persona prompt is rendered by the stamped SystemPromptBuilder::for_subagent(target_def.system_prompt) built at session-build time, independent of agent_definition_name. The pre-fix main_0.md on disk contains `# Welcome Agent\n\nYou are the Welcome agent...` as its system prompt body — correct content, wrong filename and metadata. skills_agent and other typed sub-agents are unaffected — they go through subagent_runner, which constructs its prompt directly and writes transcripts under the explicit id it receives. The pre-existing sessions/15042026/skills_agent_0.md on disk with `agent: skills_agent` confirms this code path has always been correct. Changes: * src/openhuman/agent/harness/session/builder.rs: - Add .agent_definition_name(agent_id.to_string()) to the builder chain in build_session_agent_inner. - Delete the `let _ = agent_id;` suppression. - Replace the misleading 8-line comment block at the call site (which claimed event_channel was for transport identity and therefore stamping agent_id wasn't needed — conflating two unrelated fields) with an accurate description of the three load-bearing surfaces. - Expand the docstring on AgentBuilder::agent_definition_name to document the surfaces and the latent prompt-section foot-gun the fix closes for future code. - New log::debug! call at the stamping site for grep-friendly runtime traces ([agent::builder] stamping agent_definition_name= onto session agent). * src/openhuman/agent/harness/session/runtime.rs: - Add pub fn agent_definition_name(&self) -> &str accessor on Agent so tests and runtime callers can introspect the stamped id without reaching into the pub(super) field. * src/openhuman/agent/harness/session/tests.rs: - Add build_minimal_agent_with_definition_name helper. - agent_builder_threads_agent_definition_name_when_set — parameterised over welcome/skills_agent/orchestrator/ trigger_triage, asserts the setter threads the id through. - agent_builder_falls_back_to_main_when_definition_name_unset — pins the legacy fallback contract direct builder users rely on. Tests: 2 passed; 0 failed; 3477 filtered out; finished in 0.21s. cargo check --lib clean; cargo fmt clean. Verified live against G:/projects/vezures/.openhuman on 2026-04-16: - Pre-fix welcome_proactive run wrote sessions/16042026/main_0.md with `agent: main` in the metadata header. - Post-fix welcome_proactive run wrote sessions/16042026/welcome_0.md with `agent: welcome` in the metadata header. - Post-fix web-chat dispatch to orchestrator wrote sessions/16042026/orchestrator_0.md (moved off the historical main_*.md alias — behaviorally unchanged for orchestrator). - The new [agent::builder] stamping debug line fires on both welcome and orchestrator paths. Does not touch: subagent_runner, spawn_subagent / dispatch_subagent, any agent/agents/*/agent.toml, any context::prompt code, or the AgentBuilder::build() fallback itself. Co-authored-by: Claude Opus 4.6 (1M context) --- .../agent/harness/session/builder.rs | 75 +++++++++++-- .../agent/harness/session/runtime.rs | 14 +++ src/openhuman/agent/harness/session/tests.rs | 102 ++++++++++++++++++ 3 files changed, 180 insertions(+), 11 deletions(-) diff --git a/src/openhuman/agent/harness/session/builder.rs b/src/openhuman/agent/harness/session/builder.rs index 010f2dfea..fd051316c 100644 --- a/src/openhuman/agent/harness/session/builder.rs +++ b/src/openhuman/agent/harness/session/builder.rs @@ -186,9 +186,43 @@ impl AgentBuilder { self } - /// Sets the human-readable agent definition name used as the - /// `{agent}` prefix in session transcript filenames - /// (`sessions/DDMMYYYY/{agent}_{index}.md`). + /// Sets the agent definition id this session is running + /// (`welcome`, `orchestrator`, `skills_agent`, …). + /// + /// This value is stamped onto the built [`Agent`] and surfaces in + /// the following places: + /// + /// * **Transcript filename on disk** — `transcript::write_transcript` + /// and `transcript::find_latest_transcript` use it as the + /// `{agent}` prefix in `sessions/DDMMYYYY/{agent}_{index}.md`. + /// Both the write path and the resume-lookup path read the same + /// field on `self`, so a session is always self-consistent; the + /// user-visible signal is which filename the transcript lands + /// under. Leaving it at the legacy `"main"` fallback silently + /// misfiles every non-orchestrator session under `main_*.md`. + /// * **Transcript metadata header** — `transcript::write_transcript` + /// stamps it into the `` + /// block at the top of every `.md` file. This is the ground-truth + /// signal for "which agent definition ran this session" when + /// inspecting transcripts after the fact. + /// * **[`PromptContext::agent_id`]** at prompt-build time (see + /// `turn.rs`). Today only one prompt section reads this field — + /// the `Connected Integrations` branch in `context/prompt.rs` + /// that special-cases `skills_agent` vs every other agent — so + /// the current user-visible impact of a wrong id is limited to + /// the two bullets above. The stamped `prompt_builder` injected + /// by [`Agent::from_config_for_agent`] is what actually drives + /// prompt flavour per archetype, independent of this field. That + /// said, any future prompt section that branches on a + /// non-`skills_agent` id (e.g. welcome-specific banner, planner- + /// specific rubric) would silently never fire if the field were + /// left at `"main"`, so keeping it correctly stamped closes a + /// latent foot-gun for code that hasn't been written yet. + /// + /// Callers building via [`Agent::from_config_for_agent`] get this + /// wired automatically inside `build_session_agent_inner`; direct + /// builder users (tests, CLI) must set it explicitly if they care + /// about any of the surfaces above. pub fn agent_definition_name(mut self, name: impl Into) -> Self { self.agent_definition_name = Some(name.into()); self @@ -789,14 +823,32 @@ impl Agent { let effective_omit_profile = target_def.map(|def| def.omit_profile).unwrap_or(true); let effective_omit_memory_md = target_def.map(|def| def.omit_memory_md).unwrap_or(true); - // `agent_id` is not stamped onto the returned Agent here — the - // `event_channel` field on `Agent` is for transport identity - // (which channel the session belongs to: "web_channel", "cli", - // etc.), not the agent-definition id. Callers that want to - // record which definition they asked for should log it at - // call-site. The `[agent::builder]` info trace at the top of - // `from_config_for_agent` already captures this. - let _ = agent_id; // silence unused-warning if all code paths above move + // Stamp the resolved agent definition id onto the Agent via the + // builder. Without this call, `agent_definition_name` falls + // back to the legacy `"main"` default (see `AgentBuilder::build`) + // for every non-orchestrator caller. In the current codebase + // that is benign for the orchestrator (which is already aliased + // as `"main"` everywhere downstream) but causes two concrete + // bugs for the welcome agent, which is the only other id that + // reaches this function in practice: + // + // 1. Its session transcripts are misfiled on disk under + // `sessions/DDMMYYYY/main_*.md` instead of `welcome_*.md`. + // 2. The `agent:` line inside each transcript's metadata + // header stamps `agent: main` instead of `agent: welcome`. + // + // Skills_agent and every other typed sub-agent are unaffected + // because they never build via `from_config_for_agent` — they + // are spawned through `subagent_runner` which constructs its + // prompt and history directly. + // + // See the docstring on `AgentBuilder::agent_definition_name` + // for the full list of surfaces and the latent prompt-section + // foot-gun this call also closes. + log::debug!( + "[agent::builder] stamping agent_definition_name={} onto session agent", + agent_id + ); Agent::builder() .provider(provider) @@ -818,6 +870,7 @@ impl Agent { .auto_save(config.memory.auto_save) .post_turn_hooks(post_turn_hooks) .learning_enabled(config.learning.enabled) + .agent_definition_name(agent_id.to_string()) .omit_profile(effective_omit_profile) .omit_memory_md(effective_omit_memory_md) .build() diff --git a/src/openhuman/agent/harness/session/runtime.rs b/src/openhuman/agent/harness/session/runtime.rs index 43d055dab..7cb6c1440 100644 --- a/src/openhuman/agent/harness/session/runtime.rs +++ b/src/openhuman/agent/harness/session/runtime.rs @@ -33,6 +33,20 @@ impl Agent { &self.event_channel } + /// The agent definition id this session is running + /// (`"welcome"`, `"orchestrator"`, `"skills_agent"`, …). + /// + /// Exposed so callers that build sessions via + /// [`Agent::from_config_for_agent`] can stamp the resolved id onto + /// correlation logs and progress events without reaching for the + /// source `Config`. See [`AgentBuilder::agent_definition_name`] + /// for the full list of downstream surfaces (transcript filename, + /// transcript metadata header, and `PromptContext::agent_id`) that + /// read this field. + pub fn agent_definition_name(&self) -> &str { + &self.agent_definition_name + } + /// Returns a new `AgentBuilder`. pub fn builder() -> AgentBuilder { AgentBuilder::new() diff --git a/src/openhuman/agent/harness/session/tests.rs b/src/openhuman/agent/harness/session/tests.rs index f58156cd2..f6ccaa8e5 100644 --- a/src/openhuman/agent/harness/session/tests.rs +++ b/src/openhuman/agent/harness/session/tests.rs @@ -138,6 +138,108 @@ fn _assert_builder_is_exported() -> AgentBuilder { Agent::builder() } +/// Minimal in-memory `Agent` build that every agent_definition_name +/// regression test reuses. Spins up a scratch workspace, a `none` +/// memory backend, a one-response `MockProvider`, and a single +/// `MockTool`, then feeds those into [`Agent::builder`]. Returns the +/// built `Agent` so individual tests can assert against the +/// [`Agent::agent_definition_name`] accessor. +fn build_minimal_agent_with_definition_name(definition_name: Option<&str>) -> Agent { + let workspace = tempfile::TempDir::new().expect("temp workspace"); + let workspace_path = workspace.path().to_path_buf(); + + let provider = Box::new(MockProvider { + responses: Mutex::new(vec![]), + }); + + let memory_cfg = crate::openhuman::config::MemoryConfig { + backend: "none".into(), + ..crate::openhuman::config::MemoryConfig::default() + }; + let mem: Arc = Arc::from( + crate::openhuman::memory::create_memory(&memory_cfg, &workspace_path, None).unwrap(), + ); + + let mut builder = Agent::builder() + .provider(provider) + .tools(vec![Box::new(MockTool)]) + .memory(mem) + .tool_dispatcher(Box::new(NativeToolDispatcher)) + .workspace_dir(workspace_path); + + if let Some(name) = definition_name { + builder = builder.agent_definition_name(name); + } + + builder.build().expect("minimal agent build should succeed") +} + +/// Regression test for the `build_session_agent_inner` agent-id +/// threading bug. +/// +/// Prior to the fix, `build_session_agent_inner` took an `agent_id: +/// &str` parameter but never threaded it into the `Agent::builder()` +/// chain. The builder's `.build()` then fell back to the legacy +/// `"main"` default, and every session built via +/// `Agent::from_config_for_agent` carried `agent_definition_name = +/// "main"` at runtime regardless of which id the caller asked for. +/// +/// In the current codebase only two ids actually reach +/// `from_config_for_agent` in production: `"orchestrator"` (via the +/// `Agent::from_config` legacy wrapper and the post-onboarding web +/// dispatch path) and `"welcome"` (via `welcome_proactive` and the +/// pre-onboarding web dispatch path). The orchestrator case is +/// benign — `"main"` is already an alias for orchestrator everywhere +/// downstream, so the behavior is a no-op. The welcome case is the +/// one the user sees: welcome sessions were being misfiled on disk +/// as `sessions/DDMMYYYY/main_*.md` instead of `welcome_*.md`, and +/// the `agent:` line inside each transcript's `` metadata header stamped `agent: main` instead of +/// `agent: welcome`. Skills_agent and the other typed sub-agents are +/// unaffected because they're spawned through `subagent_runner` and +/// never touch the `from_config_for_agent` / builder fallback path. +/// +/// This test pins the builder contract the fix relies on: calling +/// `.agent_definition_name(id)` on the builder chain produces an +/// `Agent` whose [`Agent::agent_definition_name`] accessor returns +/// that id verbatim. `"welcome"` and `"orchestrator"` exercise the +/// two ids that reach `from_config_for_agent` today; `"skills_agent"` +/// and `"trigger_triage"` are defensive coverage so that if a +/// future commit adds a new top-level caller for one of those ids +/// the builder contract is already pinned. +#[test] +fn agent_builder_threads_agent_definition_name_when_set() { + for expected in ["welcome", "skills_agent", "orchestrator", "trigger_triage"] { + let agent = build_minimal_agent_with_definition_name(Some(expected)); + assert_eq!( + agent.agent_definition_name(), + expected, + "agent.agent_definition_name() should return the value passed to the builder" + ); + } +} + +/// Complementary to [`agent_builder_threads_agent_definition_name_when_set`]: +/// when a caller builds an `Agent` without ever calling +/// [`AgentBuilder::agent_definition_name`], the legacy `"main"` +/// fallback still applies. This pins the fallback contract that +/// direct builder users (tests, CLI harnesses) rely on, and +/// documents the exact misbehaviour the threading fix prevents — +/// `build_session_agent_inner` used to hit this fallback even when +/// a caller asked for `welcome`, because the `.agent_definition_name` +/// setter was missing from the builder chain. The result was that +/// welcome sessions landed on disk as `main_*.md` with `agent: main` +/// stamped into their transcript metadata header. +#[test] +fn agent_builder_falls_back_to_main_when_definition_name_unset() { + let agent = build_minimal_agent_with_definition_name(None); + assert_eq!( + agent.agent_definition_name(), + "main", + "AgentBuilder::build should default agent_definition_name to \"main\" when unset" + ); +} + #[tokio::test] async fn turn_without_tools_returns_text() { let workspace = tempfile::TempDir::new().expect("temp workspace");