-
Notifications
You must be signed in to change notification settings - Fork 3.9k
perf(agent): share tool-spec leaves and reuse Config — 46% less RSS per agent #6225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
39 commits
Select commit
Hold shift + click to select a range
18e0edb
chore: files changed src/openhuman/agent/harness/session/builder/sett…
senamakel 2138250
chore: files changed src/openhuman/agent/harness/session/builder/sett…
senamakel 2d99e72
chore: files changed src/openhuman/agent/harness/session/types.rs
senamakel ce9ad1c
chore: files changed src/openhuman/agent/harness/session/builder/sett…
senamakel 6c92603
chore: files changed src/openhuman/agent/harness/session/builder/mod.rs
senamakel a2043fd
chore: files changed src/openhuman/agent/harness/session/builder/mod.rs
senamakel 7a4b906
chore: files changed src/openhuman/agent/harness/fork_context.rs,src/…
senamakel 7bee71f
chore: files changed src/openhuman/agent/harness/session/builder/mod.rs
senamakel d28b417
chore: files changed src/openhuman/agent/harness/session/builder/mod.rs
senamakel 5ad8c17
chore: Let me analyze this diff carefully.
senamakel 035345e
chore: files changed src/openhuman/agent/harness/session/turn/context…
senamakel 029b28a
chore: files changed src/openhuman/agent/orchestration/agent_teams/ru…
senamakel 79c2770
chore: files changed src/openhuman/agent/harness/session/session_test…
senamakel 5ad56d4
chore: files changed src/openhuman/agent/orchestration/tools/agent_pr…
senamakel 9a72c6a
chore: files changed src/openhuman/agent/orchestration/tools/agent_pr…
senamakel b18d9d3
chore: files changed tests/raw_coverage/agent_session_turn_raw_covera…
senamakel 9bc94f2
chore: files changed src/openhuman/agent/harness/session/builder/buil…
senamakel 95f250e
chore: files changed src/openhuman/agent/harness/session/builder/buil…
senamakel 35b4136
chore: files changed src/openhuman/agent/harness/session/builder/buil…
senamakel 507a4cd
chore: files changed src/openhuman/agent/harness/session/builder/buil…
senamakel 5618663
chore: files changed src/openhuman/agent/harness/session/builder/fact…
senamakel 526fcb4
chore: files changed src/openhuman/agent/harness/archivist/lifecycle.…
senamakel 37da10c
chore: files changed src/openhuman/agent/harness/archivist/lifecycle.rs
senamakel 8ecb27f
chore: files changed src/openhuman/agent/experience/ops.rs,src/openhu…
senamakel 10405ee
chore: files changed src/openhuman/agent/experience/ops.rs,src/openhu…
senamakel d284ccb
chore: files changed src/openhuman/agent/harness/session/builder/mod.rs
senamakel 929b880
chore: files changed src/openhuman/agent/harness/session/turn/context.rs
senamakel fb67e6e
chore: files changed src/openhuman/agent/orchestration/agent_teams/ru…
senamakel 0866c61
docs(agent): document shared tool spec schemas
senamakel f3fbb10
Merge remote-tracking branch 'origin/main' into library-agent-handle
senamakel 9c48902
test(session): wrap tool specs in Arc in builder tests
senamakel 6e85cd9
test(session): assert load_skill is scoped rather than shared
senamakel e68e1c9
test(agent): move spec-sharing tests to a new part file
senamakel b6a6c77
test(agent): cover session builder edge cases
senamakel fe0c231
refactor(agent): move agent build logic out of setters
senamakel 52d3022
feat(agent): add session builder module
senamakel 4146f1f
refactor(agent): extract session builder build logic
senamakel bc5c838
refactor(agent): extract session builder setters into a module
senamakel 221ac0e
chore: I don't see an actual diff in your message — the "Diff:" secti…
senamakel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
305 changes: 305 additions & 0 deletions
305
src/openhuman/agent/harness/session/builder/builder_build.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,305 @@ | ||
| //! `AgentBuilder::build` — validates and assembles the final [`Agent`]. | ||
|
|
||
| use super::{dedup_visible_tool_specs, visible_tool_specs_for_policy}; | ||
| use crate::openhuman::agent::context::ContextManager; | ||
| use crate::openhuman::agent::harness::session::types::{Agent, AgentBuilder}; | ||
| use crate::openhuman::tools::agent_policy::ToolPolicyEngine; | ||
| use crate::openhuman::tools::{Tool, ToolSpec}; | ||
| use anyhow::Result; | ||
| use std::sync::Arc; | ||
|
|
||
| impl AgentBuilder { | ||
| /// Validates the configuration and constructs a new `Agent` instance. | ||
| /// | ||
| /// This method is responsible for wiring together the provided components, | ||
| /// setting up the context manager, and initializing the conversation history. | ||
| /// It ensures that all required fields (provider, tools, memory, etc.) are present. | ||
| pub fn build(self) -> Result<Agent> { | ||
| let tools = self | ||
| .tools | ||
| .ok_or_else(|| anyhow::anyhow!("tools are required"))?; | ||
| // The synthesised set lives beside the durable registry, never inside | ||
| // it (`Agent::synthesized_tools`); a durable name wins a collision. | ||
| let synthesized_tools = super::drop_synthesized_name_collisions( | ||
| &tools, | ||
| self.synthesized_tools.unwrap_or_default(), | ||
| ); | ||
| let synthesized_tool_names: std::collections::HashSet<String> = synthesized_tools | ||
| .iter() | ||
| .map(|tool| tool.name().to_string()) | ||
| .collect(); | ||
| // Durable specs first, synthesised after — every reader's order. | ||
| // | ||
| // Each schema is built once and handed out behind an `Arc`. The three | ||
| // spec views an agent keeps (`durable_tool_specs`, `tool_specs`, | ||
| // `visible_tool_specs`) overlap heavily — the durable set is a prefix | ||
| // of the full set, and the visible set is a filtered subset of it — so | ||
| // materialising them as independent `Vec<ToolSpec>` kept every | ||
| // JSON-Schema `parameters` value resident up to three times per agent. | ||
| // Sharing the leaves makes the extra views cost one pointer per entry | ||
| // (openhuman#6218). | ||
| let durable_tool_specs: Vec<Arc<ToolSpec>> = | ||
| tools.iter().map(|tool| Arc::new(tool.spec())).collect(); | ||
| let tool_specs: Vec<Arc<ToolSpec>> = durable_tool_specs | ||
| .iter() | ||
| .cloned() | ||
| .chain(synthesized_tools.iter().map(|tool| Arc::new(tool.spec()))) | ||
| .collect(); | ||
|
|
||
| let mut visible_names = self.visible_tool_names.unwrap_or_default(); | ||
| // Resolved here rather than at its historical position below: the pack | ||
| // withholding is per-agent (a pack is skipped for the specialist that | ||
| // owns its family), so the id has to exist before the strip. | ||
| let agent_definition_name = self | ||
| .agent_definition_name | ||
| .clone() | ||
| .unwrap_or_else(|| "main".to_string()); | ||
| // On-demand tool disclosure: withhold packed tools' schemas from the | ||
| // provider and advertise `load_skill` / `use_skill` in their place. The | ||
| // tools stay in the registry below and stay executable — only the | ||
| // advertised surface shrinks. Applied here, before the policy filter, | ||
| // so the visible set and the policy session cannot disagree. | ||
| if visible_names.is_empty() { | ||
| visible_names = tools | ||
| .iter() | ||
| .chain(synthesized_tools.iter()) | ||
| .map(|tool| tool.name().to_string()) | ||
| .collect(); | ||
| } | ||
| crate::openhuman::tools::toolpacks::strip_packed_from_visible( | ||
| &mut visible_names, | ||
| &agent_definition_name, | ||
| ); | ||
| let config = self.config.clone().unwrap_or_default(); | ||
| let event_session_id = self | ||
| .event_session_id | ||
| .clone() | ||
| .unwrap_or_else(|| "standalone".to_string()); | ||
| let event_channel = self | ||
| .event_channel | ||
| .clone() | ||
| .unwrap_or_else(|| "internal".to_string()); | ||
| // Classify both sets: a synthesised delegate needs a decision too. | ||
| let all_tools: Vec<&dyn Tool> = tools | ||
| .iter() | ||
| .chain(synthesized_tools.iter()) | ||
| .map(|tool| tool.as_ref()) | ||
| .collect(); | ||
| let tool_policy_session = ToolPolicyEngine::build_session_from_refs( | ||
| &agent_definition_name, | ||
| &event_channel, | ||
| "session", | ||
| &config.channel_permissions, | ||
| &all_tools, | ||
| &visible_names, | ||
| ); | ||
|
|
||
| // A child agent inherits explicit profile and channel restrictions, but | ||
| // not the primary agent's own role-specific tool scope. The Master Agent | ||
| // can write directly, while specialists may still need tools outside its | ||
| // intentionally compact default surface. Conflating those two surfaces | ||
| // silently strips specialist capabilities (#5118 merge). | ||
| // | ||
| // Build a second policy snapshot without the role visibility filter. | ||
| // `tool_policy_session` marks both channel-blocked and role-hidden tools | ||
| // as restricted, so deriving the child ceiling from it would reintroduce | ||
| // exactly that conflation. | ||
| let channel_policy_session = ToolPolicyEngine::build_session_from_refs( | ||
| &agent_definition_name, | ||
| &event_channel, | ||
| "session", | ||
| &config.channel_permissions, | ||
| &all_tools, | ||
| &std::collections::HashSet::new(), | ||
| ); | ||
| let mut subagent_tool_ceiling_names = self.subagent_tool_ceiling_names.unwrap_or_default(); | ||
| if channel_policy_session.has_restrictions() { | ||
| let policy_allowed: std::collections::HashSet<String> = tool_specs | ||
| .iter() | ||
| .filter(|spec| channel_policy_session.is_allowed(&spec.name)) | ||
| .map(|spec| spec.name.clone()) | ||
| .collect(); | ||
| if subagent_tool_ceiling_names.is_empty() { | ||
| subagent_tool_ceiling_names = policy_allowed; | ||
| } else { | ||
| subagent_tool_ceiling_names.retain(|name| policy_allowed.contains(name)); | ||
| if subagent_tool_ceiling_names.is_empty() { | ||
| subagent_tool_ceiling_names.insert("__subagent_no_tools__".to_string()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Build the filtered spec list that the main agent sends to the | ||
| // provider. The explicit visible-tool allowlist and the resolved | ||
| // channel permission policy must stay aligned so prompt-visible | ||
| // tools cannot exceed the runtime execution boundary. | ||
| let visible_tool_specs_unfiltered = | ||
| visible_tool_specs_for_policy(&tool_specs, &visible_names, &tool_policy_session); | ||
|
|
||
| // Dedupe by tool name. Anthropic (and other strict providers) | ||
| // rejects a chat/completions request that lists two tools with | ||
| // the same name — OpenHuman's own backend and OpenAI silently | ||
| // accept duplicates, which hid this bug until #1710's per-role | ||
| // routing started sending the same tool list to Anthropic. | ||
| let visible_tool_specs: Vec<Arc<ToolSpec>> = | ||
| dedup_visible_tool_specs(visible_tool_specs_unfiltered); | ||
|
|
||
| let visible_names_list: Vec<&str> = | ||
| visible_tool_specs.iter().map(|s| s.name.as_str()).collect(); | ||
| log::info!( | ||
| "[agent] tool spec filter: total={} visible={} (filter_active={} policy_restricted={}) names=[{}]", | ||
| tool_specs.len(), | ||
| visible_tool_specs.len(), | ||
| !visible_names.is_empty(), | ||
| tool_policy_session.has_restrictions(), | ||
| visible_names_list.join(", ") | ||
| ); | ||
|
|
||
| // Pull the model source out of the builder once; the Agent holds it and | ||
| // builds a fresh tiered crate `ChatModel` set from it per turn. | ||
| let turn_model_source = self | ||
| .turn_model_source | ||
| .ok_or_else(|| anyhow::anyhow!("provider is required"))?; | ||
|
|
||
| let prompt_builder = self.prompt_builder.unwrap_or_else( | ||
| crate::openhuman::agent::context::prompt::SystemPromptBuilder::with_defaults, | ||
| ); | ||
|
|
||
| let model_name = self | ||
| .model_name | ||
| .unwrap_or_else(|| crate::openhuman::config::DEFAULT_MODEL.into()); | ||
|
|
||
| // Assemble the per-session ContextManager. The manager owns | ||
| // the prompt builder, the reduction pipeline, and the | ||
| // summarizer — every concern that touches "what's in the | ||
| // model's context window" routes through this single handle. | ||
| let context_config = self.context_config.unwrap_or_default(); | ||
|
|
||
| // Live history reduction moved to the tinyagents graph | ||
| // (`ContextCompressionMiddleware` + `MessageTrimMiddleware`, issue | ||
| // #4249), so the session no longer constructs an in-turn summarizer | ||
| // here. The archivist hook still drives durable segment recaps on its | ||
| // own post-turn path; it is no longer coupled to context compaction. | ||
| let context = ContextManager::new(&context_config, prompt_builder); | ||
|
|
||
| let workspace_dir = self | ||
| .workspace_dir | ||
| .unwrap_or_else(|| std::path::PathBuf::from(".")); | ||
| let action_dir = self.action_dir.unwrap_or_else(|| workspace_dir.clone()); | ||
| let memory_subdir = self.memory_subdir.unwrap_or_else(|| "memory".to_string()); | ||
| let session_raw_subdir = self | ||
| .session_raw_subdir | ||
| .unwrap_or_else(|| "session_raw".to_string()); | ||
|
|
||
| let tools = Arc::new(tools); | ||
| // The pack tools live inside this registry, so they can only be pointed | ||
| // at it once it exists. Re-bind after any later rebuild of this `Arc`. | ||
| crate::openhuman::tools::toolpacks::bind_pack_registry(&tools); | ||
|
|
||
| Ok(Agent { | ||
| turn_model_source, | ||
| tools, | ||
| synthesized_tools: Arc::new(synthesized_tools), | ||
| tool_specs: Arc::new(tool_specs), | ||
| durable_tool_specs: Arc::new(durable_tool_specs), | ||
| visible_tool_specs: Arc::new(visible_tool_specs), | ||
| visible_tool_names: visible_names, | ||
| subagent_tool_ceiling_names, | ||
| tool_policy_session, | ||
| memory: self | ||
| .memory | ||
| .ok_or_else(|| anyhow::anyhow!("memory is required"))?, | ||
| shared_experience_memory: self.shared_experience_memory, | ||
| auto_recall: self.auto_recall, | ||
| tool_dispatcher: std::sync::Arc::from( | ||
| self.tool_dispatcher | ||
| .ok_or_else(|| anyhow::anyhow!("tool_dispatcher is required"))?, | ||
| ), | ||
| config, | ||
| model_name, | ||
| model_vision: self.model_vision.unwrap_or(false), | ||
| temperature: self.temperature.unwrap_or(0.7), | ||
| workspace_dir, | ||
| action_dir, | ||
| workspace_descriptor: self.workspace_descriptor, | ||
| workflows: self.workflows.unwrap_or_default(), | ||
| auto_save: self.auto_save.unwrap_or(false), | ||
| last_memory_context: None, | ||
| last_turn_citations: Vec::new(), | ||
| pending_citations: None, | ||
| last_turn_usage_totals: None, | ||
| last_turn_hit_cap: false, | ||
| history: Vec::new(), | ||
| post_turn_hooks: self.post_turn_hooks, | ||
| learning_enabled: self.learning_enabled, | ||
| explicit_preferences_enabled: self.explicit_preferences_enabled, | ||
| event_session_id, | ||
| event_channel, | ||
| agent_definition_name: agent_definition_name.clone(), | ||
| // Canonical registry id — captured here at build time | ||
| // before any caller can call `set_agent_definition_name` | ||
| // and clobber the transcript-facing name. Used by | ||
| // `refresh_delegation_tools` to re-resolve the agent's | ||
| // `subagents` declaration against the global registry. | ||
| agent_definition_id: agent_definition_name.clone(), | ||
| active_profile_id: self.active_profile_id, | ||
| personality_soul_md: self.personality_soul_md, | ||
| personality_memory_md: self.personality_memory_md, | ||
| memory_subdir, | ||
| session_raw_subdir, | ||
| session_transcript_path: None, | ||
| session_history: None, | ||
| session_history_locator: self.session_history_locator, | ||
| persisted_transcript_messages: Vec::new(), | ||
| session_key: { | ||
| let unix_ts = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .map(|d| d.as_secs()) | ||
| .unwrap_or(0); | ||
| let sanitized: String = agent_definition_name | ||
| .chars() | ||
| .map(|c| { | ||
| if c.is_ascii_alphanumeric() || c == '_' || c == '-' { | ||
| c | ||
| } else { | ||
| '_' | ||
| } | ||
| }) | ||
| .collect(); | ||
| format!("{unix_ts}_{sanitized}") | ||
| }, | ||
| session_parent_prefix: self.session_parent_prefix, | ||
| cached_transcript_messages: None, | ||
| context, | ||
| on_progress: None, | ||
| run_queue: None, | ||
| connected_integrations: Vec::new(), | ||
| connected_integrations_initialized: false, | ||
| runtime_config: None, | ||
| // Default to `true` (omit) so legacy / custom agents built | ||
| // without a definition stay lean. Opt-in agents thread their | ||
| // `omit_profile = false` through the builder. | ||
| omit_profile: self.omit_profile.unwrap_or(true), | ||
| omit_memory_md: self.omit_memory_md.unwrap_or(true), | ||
| payload_summarizer: self.payload_summarizer, | ||
| trigger_memory_agent: self.trigger_memory_agent.unwrap_or_default(), | ||
| tokenjuice_compression: self.tokenjuice_compression, | ||
| tool_policy: self.tool_policy.unwrap_or_else(|| { | ||
| Arc::new(crate::openhuman::agent::tool_policy::AllowAllToolPolicy) | ||
| }), | ||
| last_seen_integrations_hash: 0, | ||
| composio_integrations_rx: None, | ||
| skill_events_rx: None, | ||
| announced_integrations: std::collections::HashSet::new(), | ||
| pending_integration_announcement: Vec::new(), | ||
| announced_mcp_servers: std::collections::HashSet::new(), | ||
| pending_mcp_announcement: Vec::new(), | ||
| announced_skills: std::collections::HashSet::new(), | ||
| pending_skill_announcement: Vec::new(), | ||
| pending_skill_retraction: Vec::new(), | ||
| archivist_hook: self.archivist_hook, | ||
| synthesized_tool_names, | ||
| pending_turn_overrides: super::super::types::TurnOverrides::default(), | ||
| }) | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.