From c737ebd2fcf9f5d8fb8ff53f861fb33660eb85dc Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Mon, 13 Apr 2026 21:54:04 +0530 Subject: [PATCH] feat(web-channel): route Tauri in-app chat to welcome/orchestrator + Windows fsync fix (#525) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the web-channel-side half of the #525 welcome agent routing. Also bundles a small pre-existing Windows compatibility fix for `app_state::ops::sync_parent_dir` that was blocking end-to-end testing of this branch on Windows. ## Web channel routing (primary change) Commit 4b (274b50ed) wired the welcome-vs-orchestrator routing into `channels/runtime/dispatch.rs::process_channel_message` — the handler for inbound messages on external channels (Telegram, Discord, Slack, iMessage, etc.). That path reads `config.onboarding_completed` and selects the right agent via the bus. End-to-end testing on the Tauri desktop app revealed a gap: the in-app chat window does NOT route through `process_channel_message`. It uses its own JSON-RPC method (`openhuman.channel_web_chat` → `start_chat` → `run_chat_task` → `build_session_agent` → `Agent::from_config`) which was hardcoded to build a generic main-agent every turn. So a new user typing in the Tauri app saw the orchestrator immediately, never the welcome agent. This commit closes that gap without adding a third code path: ### `src/openhuman/agent/harness/session/builder.rs` * Split the existing `Agent::from_config` into a thin wrapper + a new `Agent::from_config_for_agent(config, agent_id)`. The wrapper calls the new function with `agent_id = "orchestrator"`, preserving the legacy behaviour byte-identically for all existing callers (CLI, REPL, tests, every call site that doesn't care which agent they get). * `from_config_for_agent` looks up `agent_id` in `AgentDefinitionRegistry::global()` up front; unknown ids fail fast with a clear error. Missing orchestrator is tolerated as a legacy / pre-startup fallback so existing tests that run without a populated registry continue to work. * When a target definition is resolved: - `prompt_builder` uses `SystemPromptBuilder::for_subagent` with the agent's `system_prompt` body (read from the registry, which already has it inlined via `include_str!` from `agents/*/prompt.md` at crate-build time) and the three `omit_*` flags from its TOML. - `visible_tool_names` is built from the agent's `ToolScope::Named` list, unioned with the names of any synthesised delegation tools produced by `collect_orchestrator_tools` (for agents that declare `subagents = [...]`). - `ToolScope::Wildcard` yields an empty filter, matching the legacy "no filter, all visible" semantics. - `temperature` comes from the agent's TOML (e.g. welcome is 0.7, orchestrator is 0.4) instead of `config.default_temperature`. * Legacy behaviour for plain `from_config(config)` — which passes `agent_id = "orchestrator"` — is preserved: the prompt builder continues to use `SystemPromptBuilder::with_defaults()`, temperature comes from `config.default_temperature`, and the orchestrator's `subagents` list drives delegation tool synthesis exactly as before. No test expectations changed. ### `src/openhuman/channels/providers/web.rs` * `build_session_agent` now reads `config.onboarding_completed` and picks `"welcome"` or `"orchestrator"` as the target agent id, then calls `Agent::from_config_for_agent`. Structured info log records the routing decision + the flag state for observability, matching the `[dispatch::routing]` trace pattern from Commit 4b. * `config.onboarding_completed` is read fresh every turn because `run_chat_task` loads the config via `config_rpc::load_config_with_timeout`, which reads from disk on every call (no in-process cache). Welcome → orchestrator handoff therefore happens automatically on the next chat message after `complete_onboarding(complete)` flips the flag, with no explicit event or notification plumbing. * `set_event_context` call is unchanged — the event context is a (session_id, channel_name) pair for telemetry, not the agent id. ## Windows fsync fix (bundled) ### `src/openhuman/app_state/ops.rs` `sync_parent_dir` was calling `File::open(parent).and_then(|dir| dir.sync_all())` on the save path for `app_state.json`. On Windows, opening a directory as a regular file requires `FILE_FLAG_BACKUP_SEMANTICS` which `std::fs::File::open` does not set, so the call fails with "Access is denied. (os error 5)". Mirrored the existing `#[cfg(unix)]` guard already in `config/schema/load.rs::sync_directory`. On non-Unix the function is a no-op and returns `Ok(())`. Durability on Windows is provided by `NamedTempFile::persist`'s atomic `MoveFileEx`, which is sufficient for config files. This is a pre-existing upstream bug, not caused by this PR, but it blocks end-to-end testing of any code path that touches `app_state::save_stored_app_state_unlocked` on Windows — which includes the web channel's agent_server_status RPC. Fixing it here so the #525 routing can actually be tested on the developer's machine, and the fix is tiny and self-contained. The `File` import on line 1 is also gated behind `#[cfg(unix)]` to avoid an unused-import warning on Windows. ## Tests All 35 `agent::harness::session` tests pass, including the transcript round-trip + session queue + runtime tests that exercise the builder path most heavily. No test expectations were modified; the refactor is a pure delegation chain (`from_config` → new inner method → existing builder). CLI dump-prompt testing from the earlier session still validates: * `openhuman agent dump-prompt welcome` → 2 tools, Step 2.5 bare-install prompt embedded * `openhuman agent dump-prompt main` → 6 tools, zero skill leakage * `openhuman agent dump-prompt main --stub-composio` → still 6 tools, composio meta-tools correctly filtered out End-to-end Tauri testing (next step after this commit) will exercise the new `build_session_agent` branch live — user types `hi`, web channel routes to welcome, welcome replies with the updated bare-install prompt from commit 990a7264. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../agent/harness/session/builder.rs | 325 +++++++++++++++--- src/openhuman/app_state/ops.rs | 20 +- src/openhuman/channels/providers/web.rs | 28 +- 3 files changed, 325 insertions(+), 48 deletions(-) diff --git a/src/openhuman/agent/harness/session/builder.rs b/src/openhuman/agent/harness/session/builder.rs index 8c3379603..e664fdd98 100644 --- a/src/openhuman/agent/harness/session/builder.rs +++ b/src/openhuman/agent/harness/session/builder.rs @@ -303,18 +303,130 @@ impl AgentBuilder { impl Agent { /// Constructs an `Agent` instance from a global system configuration. /// - /// This is the primary factory method for initializing an agent with all - /// standard system integrations (memory, tools, skills, providers, learning) - /// configured according to the user's `config.toml`. + /// Thin wrapper around [`Agent::from_config_for_agent`] that always + /// targets the orchestrator definition. This preserves the legacy + /// "main agent = orchestrator" behaviour for CLI / REPL / any caller + /// that does not participate in the #525 onboarding-routing flow. /// - /// It performs the heavy lifting of: + /// Callers that need to select a different agent at session-build + /// time (for example the Tauri web chat path, which routes to the + /// welcome agent pre-onboarding) should call + /// [`Agent::from_config_for_agent`] directly. + pub fn from_config(config: &Config) -> Result { + Self::from_config_for_agent(config, "orchestrator") + } + + /// Constructs an `Agent` instance scoped to a specific agent + /// definition loaded from the global [`AgentDefinitionRegistry`]. + /// + /// `agent_id` is looked up in the registry; the returned agent + /// inherits that definition's `ToolScope`, `system_prompt`, + /// `temperature`, `max_iterations`, and `omit_*` flags. Unknown + /// agent ids produce a registry-lookup error rather than silently + /// falling back to the orchestrator. + /// + /// Shared infrastructure between agent ids is identical: /// 1. Initializing the host runtime (native or docker). /// 2. Setting up security policies. /// 3. Initializing memory and embedding services. /// 4. Registering all built-in and orchestrator tools. /// 5. Configuring the routed AI provider. /// 6. Setting up the learning system and post-turn hooks. - pub fn from_config(config: &Config) -> Result { + /// + /// What differs per agent id: + /// * `visible_tool_names` is the agent's `ToolScope::Named` list + /// (unioned with the names of synthesised delegation tools when + /// the agent declares `subagents = [...]`). `ToolScope::Wildcard` + /// yields an empty filter, matching the legacy unfiltered path. + /// * `prompt_builder` uses [`SystemPromptBuilder::for_subagent`] + /// with the agent's inline/file prompt body and `omit_*` flags, + /// so each agent renders its own persona rather than the default + /// orchestrator workspace-files identity dump. + /// * `temperature` comes from the agent's TOML (falls back to + /// `config.default_temperature` for the orchestrator to preserve + /// legacy behaviour). + /// + /// The welcome agent uses this entry point when routed from the + /// Tauri web channel (see `channels::providers::web::build_session_agent`). + pub fn from_config_for_agent(config: &Config, agent_id: &str) -> Result { + use crate::openhuman::agent::harness::definition::{AgentDefinitionRegistry, ToolScope}; + + // Look up the target definition up front so we can fail fast + // with a clear error instead of building half an agent and then + // discovering the id is unknown. The registry is a singleton + // initialised at startup; if it's not yet populated we + // conservatively fall back to the legacy "orchestrator-shaped" + // build by proceeding without a definition override. + let target_def: Option< + crate::openhuman::agent::harness::definition::AgentDefinition, + > = match AgentDefinitionRegistry::global() { + Some(reg) => match reg.get(agent_id) { + Some(def) => Some(def.clone()), + None if agent_id == "orchestrator" => { + // Orchestrator is allowed to be missing from the + // registry (legacy path, tests, pre-startup) — + // fall back to default behaviour. + log::debug!( + "[agent::builder] orchestrator definition not in registry — \ + using legacy default prompt + filter" + ); + None + } + None => { + return Err(anyhow::anyhow!( + "agent definition '{}' not found in registry", + agent_id + )); + } + }, + None => { + if agent_id != "orchestrator" { + return Err(anyhow::anyhow!( + "AgentDefinitionRegistry is not initialised — cannot \ + resolve agent '{}'. Call AgentDefinitionRegistry::init_global \ + at startup.", + agent_id + )); + } + log::debug!( + "[agent::builder] registry not initialised, orchestrator requested — \ + using legacy default prompt + filter" + ); + None + } + }; + + log::info!( + "[agent::builder] building session agent id={} \ + (scope={}, omit_identity={}, temperature={:.2})", + agent_id, + target_def + .as_ref() + .map(|d| match &d.tools { + ToolScope::Named(names) => format!("named({})", names.len()), + ToolScope::Wildcard => "wildcard".to_string(), + }) + .unwrap_or_else(|| "legacy".to_string()), + target_def.as_ref().map(|d| d.omit_identity).unwrap_or(false), + target_def + .as_ref() + .map(|d| d.temperature) + .unwrap_or(config.default_temperature) + ); + + Self::build_session_agent_inner(config, agent_id, target_def.as_ref()) + } + + /// Internal constructor that consumes the optionally-resolved agent + /// definition. Split out from [`Agent::from_config_for_agent`] so + /// the lookup + logging live in one place and the heavy-lifting + /// body stays readable. + fn build_session_agent_inner( + config: &Config, + agent_id: &str, + target_def: Option<&crate::openhuman::agent::harness::definition::AgentDefinition>, + ) -> Result { + use crate::openhuman::agent::harness::definition::{PromptSource, ToolScope}; let runtime: Arc = Arc::from(host_runtime::create_runtime(&config.runtime)?); let security = Arc::new(SecurityPolicy::from_config( @@ -385,8 +497,68 @@ impl Agent { let dispatcher_choice = config.agent.tool_dispatcher.clone(); let supports_native = provider.supports_native_tools(); - // Build prompt builder, optionally with learning sections - let mut prompt_builder = SystemPromptBuilder::with_defaults(); + // Build prompt builder — either the default "orchestrator / + // main agent" layout that bootstraps from workspace identity + // files, OR a narrow per-agent builder that injects the target + // definition's `prompt.md` body and respects its `omit_*` flags. + // + // The narrow path is selected whenever we resolved a + // non-orchestrator definition from the registry. Welcome agent + // is the first real consumer: its TOML sets + // `omit_identity = true`, `omit_memory_context = false`, + // `omit_safety_preamble = true`, `omit_skills_catalog = true`, + // so the rendered prompt becomes: + // + // (welcome persona body) + // ── Memory context (user profile, learned observations) + // ── Tools (2 entries: complete_onboarding + memory_recall) + // ── Workspace directory + // + // The orchestrator continues to use `with_defaults` so its + // prompt stays byte-identical to the legacy CLI/REPL behaviour + // except for the tool-scope tightening we already landed in + // earlier commits. + let mut prompt_builder = match target_def { + Some(def) if agent_id != "orchestrator" => { + // Resolve the prompt body. For built-in agents, + // `system_prompt` is `PromptSource::Inline(...)` populated + // at crate-build time from the sibling `prompt.md` via + // `include_str!` in `agents/mod.rs`. File-based prompts + // (custom workspace overrides) read from disk. + let body = match &def.system_prompt { + PromptSource::Inline(text) => text.clone(), + PromptSource::File { path } => { + let workspace_path = config + .workspace_dir + .join("agent") + .join("prompts") + .join(path); + if workspace_path.is_file() { + std::fs::read_to_string(&workspace_path).unwrap_or_else(|e| { + log::warn!( + "[agent::builder] failed to read prompt {}: {e} — using empty body", + workspace_path.display() + ); + String::new() + }) + } else { + log::warn!( + "[agent::builder] prompt file {} not found — using empty body", + path + ); + String::new() + } + } + }; + SystemPromptBuilder::for_subagent( + body, + def.omit_identity, + def.omit_safety_preamble, + def.omit_skills_catalog, + ) + } + _ => SystemPromptBuilder::with_defaults(), + }; if config.learning.enabled { prompt_builder = prompt_builder .add_section(Box::new( @@ -453,33 +625,61 @@ impl Agent { } } - // Generate the orchestrator's delegation tool set from its - // declarative `subagents = [...]` field: one `ArchetypeDelegationTool` - // per named sub-agent, plus one `SkillDelegationTool` per connected - // Composio toolkit when the orchestrator includes a - // `{ skills = "*" }` wildcard. These are the only delegation tools - // the main-agent LLM sees; sub-agents themselves still access the - // full `tools` registry via `ParentExecutionContext` and apply - // their own per-definition whitelist in the subagent runner. + // Resolve the per-agent delegation tool set and visible-tool + // whitelist from the target definition (when we have one) or + // fall back to the orchestrator's synthesis path. // - // This builder is synchronous and sits on the CLI / REPL code - // path. It does not have access to the async Composio fetcher, - // so we pass an empty slice of connected integrations here — the - // skill-wildcard expansion therefore produces zero delegation - // tools in this path, which is correct behaviour for CLI (the - // CLI user can still reach any toolkit via the generic - // `composio_execute` tool or `spawn_subagent`). + // For an agent with `subagents = [...]` in its TOML (today: + // orchestrator), `collect_orchestrator_tools` synthesises one + // `ArchetypeDelegationTool` per named sub-agent plus one + // `SkillDelegationTool` per connected Composio toolkit. // - // The channel-dispatch path (`channels::runtime::dispatch`) has - // its own tool registry assembly and will populate the - // connected integrations list from the live Composio fetch in a - // later commit (#525/#526 dispatch routing). - let orchestrator_tools = - match crate::openhuman::agent::harness::definition::AgentDefinitionRegistry::global() { - Some(reg) => match reg.get("orchestrator") { - Some(orch_def) => { - tools::orchestrator_tools::collect_orchestrator_tools(orch_def, reg, &[]) + // For an agent without `subagents` (today: welcome, critic, + // archivist, etc.), no delegation tools are synthesised — the + // LLM only sees the agent's own `ToolScope::Named` entries + // from the global registry, narrowed by the visible-tool + // filter. + // + // This builder is synchronous and sits on the CLI / REPL / + // Tauri-web code path. It does not have access to the async + // Composio fetcher, so we pass an empty slice of connected + // integrations here — the skill-wildcard expansion therefore + // produces zero delegation tools. That is correct behaviour: + // callers that need live integration expansion go through the + // bus-based `channels::runtime::dispatch` path instead. + let (delegation_tools, filter_from_scope): ( + Vec>, + Option>, + ) = match ( + target_def, + crate::openhuman::agent::harness::definition::AgentDefinitionRegistry::global(), + ) { + (Some(def), Some(reg)) => { + let synthed = tools::orchestrator_tools::collect_orchestrator_tools(def, reg, &[]); + let filter: Option> = match &def.tools { + ToolScope::Named(names) => { + let mut set: std::collections::HashSet = + names.iter().cloned().collect(); + for t in &synthed { + set.insert(t.name().to_string()); + } + Some(set) } + ToolScope::Wildcard => None, + }; + (synthed, filter) + } + (None, Some(reg)) => { + // Legacy orchestrator fallback (no target definition). + // Keeps the pre-refactor behaviour byte-identical for + // callers that invoke the old `from_config` on a + // pre-startup or test registry state. + let synthed = match reg.get("orchestrator") { + Some(orch_def) => tools::orchestrator_tools::collect_orchestrator_tools( + orch_def, + reg, + &[], + ), None => { log::debug!( "[agent::builder] orchestrator definition not in registry — \ @@ -487,26 +687,41 @@ impl Agent { ); Vec::new() } - }, - None => { - log::debug!( - "[agent::builder] AgentDefinitionRegistry not initialised — \ - skipping delegation tool synthesis" - ); - Vec::new() - } - }; - let visible: std::collections::HashSet = orchestrator_tools - .iter() - .map(|t| t.name().to_string()) - .collect(); + }; + (synthed, None) + } + (_, None) => { + log::debug!( + "[agent::builder] AgentDefinitionRegistry not initialised — \ + skipping delegation tool synthesis" + ); + (Vec::new(), None) + } + }; + + // The final visible-tool whitelist is the union of whatever the + // definition scope produced (for named scopes) and every tool + // we just synthesised as a delegation wrapper. When the + // definition is `ToolScope::Wildcard` (legacy default, no + // filter), we still populate `visible` from the delegation + // tools alone so the existing `Agent::visible_tool_names` + // contract (empty == no filter) stays intact: an empty set + // means "no filter" for both legacy callers and the new + // agent-scoped path. + let visible: std::collections::HashSet = match filter_from_scope { + Some(set) => set, + None => delegation_tools + .iter() + .map(|t| t.name().to_string()) + .collect(), + }; // De-duplicate: some synthesised tool names may collide with // already-registered tools (unlikely for `delegate_*` names but // cheap to guard against). let existing_names: std::collections::HashSet = tools.iter().map(|t| t.name().to_string()).collect(); tools.extend( - orchestrator_tools + delegation_tools .into_iter() .filter(|t| !existing_names.contains(t.name())), ); @@ -531,6 +746,24 @@ impl Agent { pformat_registry.len() ); + // Temperature override: when we have a target definition, use + // its declared temperature from the TOML (welcome is 0.7, + // orchestrator is 0.4, etc). Fall back to + // `config.default_temperature` for the legacy "no definition" + // path so existing callers keep getting their configured value. + let effective_temperature = target_def + .map(|def| def.temperature) + .unwrap_or(config.default_temperature); + + // `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 + Agent::builder() .provider(provider) .tools(tools) @@ -545,7 +778,7 @@ impl Agent { .config(config.agent.clone()) .context_config(config.context.clone()) .model_name(model_name) - .temperature(config.default_temperature) + .temperature(effective_temperature) .workspace_dir(config.workspace_dir.clone()) .skills(crate::openhuman::skills::load_skills(&config.workspace_dir)) .auto_save(config.memory.auto_save) diff --git a/src/openhuman/app_state/ops.rs b/src/openhuman/app_state/ops.rs index cd8f456e0..442b69349 100644 --- a/src/openhuman/app_state/ops.rs +++ b/src/openhuman/app_state/ops.rs @@ -1,4 +1,6 @@ -use std::fs::{self, File}; +use std::fs; +#[cfg(unix)] +use std::fs::File; use std::io::Write; use std::path::{Path, PathBuf}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; @@ -170,11 +172,27 @@ fn load_stored_app_state(config: &Config) -> Result { } fn sync_parent_dir(path: &Path) -> Result<(), String> { + // Directory fsync is a POSIX-only durability guarantee — on Unix we + // open the parent dir and call `sync_all()` so the rename of the + // temp file into place is persisted even if the host crashes before + // the next buffer flush. On Windows, opening a directory as a + // regular file requires `FILE_FLAG_BACKUP_SEMANTICS` which + // `std::fs::File::open` does not set, so the call fails with + // "Access is denied. (os error 5)". Since Windows uses a different + // durability model (and `NamedTempFile::persist` issues an atomic + // MoveFileEx which is already durable enough for our config files), + // we skip the fsync entirely on non-Unix and return Ok. Mirrors the + // existing `sync_directory` guard in `config/schema/load.rs`. + #[cfg(unix)] if let Some(parent) = path.parent() { File::open(parent) .and_then(|dir| dir.sync_all()) .map_err(|e| format!("failed to sync directory {}: {e}", parent.display()))?; } + #[cfg(not(unix))] + { + let _ = path; + } Ok(()) } diff --git a/src/openhuman/channels/providers/web.rs b/src/openhuman/channels/providers/web.rs index c626b3ca6..ab74dfc64 100644 --- a/src/openhuman/channels/providers/web.rs +++ b/src/openhuman/channels/providers/web.rs @@ -508,7 +508,33 @@ fn build_session_agent( effective.default_temperature = temp; } - Agent::from_config(&effective) + // Route to welcome vs orchestrator based on the per-user onboarding + // flag. #525 fix: pre-onboarding users see the welcome agent's + // persona with its 2-tool TOML scope (complete_onboarding + + // memory_recall) instead of the orchestrator's default delegation + // surface. Post-onboarding they transition automatically on the + // next chat turn because `Config::load_or_init` reads fresh from + // disk every call. + // + // The config reached here has already been loaded by + // `run_chat_task` via `config_rpc::load_config_with_timeout`, so + // the `onboarding_completed` field reflects the current persisted + // state — no cache to invalidate. + let target_agent_id = if effective.onboarding_completed { + "orchestrator" + } else { + "welcome" + }; + + log::info!( + "[web-channel] routing chat turn to '{}' (onboarding_completed={}, client_id={}, thread_id={})", + target_agent_id, + effective.onboarding_completed, + client_id, + thread_id + ); + + Agent::from_config_for_agent(&effective, target_agent_id) .map(|mut agent| { agent.set_event_context(event_session_id_for(client_id, thread_id), "web_channel"); agent