feat: Agent Factory — preset system, factory tools, and cortex integration#315
Merged
feat: Agent Factory — preset system, factory tools, and cortex integration#315
Conversation
…ation Implement the Agent Factory: a conversational system for creating, configuring, and refining agents through presets and LLM-callable tools. Phase 0 — Deprecate USER.md: - Add description field to HumanDef for rich human context on the org graph - Remove user field from Identity, stop loading/scaffolding USER.md - Rewrite build_org_context() to render human roles and descriptions - Update API types, human CRUD handlers, dashboard UI Phase 1 — Preset system: - 9 embedded preset archetypes (8 specialists + main-agent) in presets/ - PresetRegistry using rust-embed with list/load API - API endpoints: GET /api/factory/presets, GET /api/factory/presets/:id - 5 unit tests Phase 2 — Factory tools (6 LLM-callable tools): - factory_list_presets, factory_load_preset, factory_search_context - factory_create_agent (full lifecycle: create + identity files + org links) - factory_update_identity, factory_update_config (refinement) - Extracted create_agent_internal() from API handler for tool reuse Phase 3 — Factory prompt: - prompts/en/factory.md.j2 with creation flow, soul writing guide, synthesis rules Phase 5 — Cortex chat integration: - Factory tools wired into cortex chat via add_factory_tools() - Conditional Agent Factory section in cortex_chat.md.j2 Phase 6 — Polish: - Fix LinkSpec schema/parser mismatch (critical: direction/kind enum values) - Agent ID format validation (lowercase, hyphens, 2-64 chars) - Identity content non-empty validation in create and update tools - Model routing provider validation in factory_update_config - Tuning value range validation (branches 1-32, workers 1-64, etc.) - Clamp max_results in factory_search_context (1-25) - Best-effort identity file writes with error reporting - Add config_write_mutex to ApiState for safe read-modify-write cycles
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis diff introduces a comprehensive Agent Factory feature enabling natural-language-driven agent creation, renames the identity "user" field to "role" across the codebase, relocates identity files from workspace to agent root (replacing USER.md with ROLE.md), adds human description support, and expands org-context with human metadata propagation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| @@ -580,22 +598,22 @@ pub(super) async fn create_agent( | |||
| ] { | |||
| std::fs::create_dir_all(dir).map_err(|error| { | |||
Contributor
There was a problem hiding this comment.
This is in an async fn on the API path; std::fs::create_dir_all blocks the tokio runtime. tokio::fs::create_dir_all would avoid blocking here.
Suggested change
| std::fs::create_dir_all(dir).map_err(|error| { | |
| tokio::fs::create_dir_all(dir).await.map_err(|error| { | |
| tracing::error!(%error, dir = %dir.display(), "failed to create agent directory"); | |
| format!("failed to create directory {}: {error}", dir.display()) | |
| })?; |
| /// Maximum number of results to return. | ||
| #[serde(default = "default_max_results")] | ||
| pub max_results: usize, | ||
| /// Optional memory type filter: "fact", "preference", "decision", "identity", "event", "observation". |
Contributor
There was a problem hiding this comment.
Minor docs nit: this list doesn’t match the actual accepted values in parse_memory_type / MemoryType::ALL.
Suggested change
| /// Optional memory type filter: "fact", "preference", "decision", "identity", "event", "observation". | |
| /// Optional memory type filter (see MemoryType::ALL). |
| .await; | ||
| } | ||
| } | ||
| Err(error) => { |
Contributor
There was a problem hiding this comment.
Right now if config.toml reload fails after writing, the tool still returns success with no signal to the caller (only a log). In practice the LLM/user probably needs to know it didn’t hot-reload (and may need a restart) to avoid confusing “it didn’t change” follow-ups.
src/tools/file.rs
Outdated
| if self.sandbox.mode_enabled() { | ||
| let file_name = path.file_name().and_then(|n| n.to_str()).unwrap_or(""); | ||
| const PROTECTED_FILES: &[&str] = &["SOUL.md", "IDENTITY.md", "USER.md"]; | ||
| const PROTECTED_FILES: &[&str] = &["SOUL.md", "IDENTITY.md"]; |
Contributor
There was a problem hiding this comment.
Even if USER.md is deprecated/ignored at runtime now, it still seems like an identity-ish file that can contain sensitive human context. Keeping it protected from file writes avoids accidental edits via generic tooling.
Suggested change
| const PROTECTED_FILES: &[&str] = &["SOUL.md", "IDENTITY.md"]; | |
| const PROTECTED_FILES: &[&str] = &["SOUL.md", "IDENTITY.md", "USER.md"]; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/main.rs (1)
1842-1883:⚠️ Potential issue | 🟠 MajorProvider-setup initialization uses stale humans state.
In setup mode,
initialize_agents()is called with the startupagent_humansArcSwap, notnew_config.humans. If humans changed before providers were configured, agents initialize with outdated human context.💡 Suggested patch
match new_config { Ok(new_config) if has_provider_credentials(&new_config.llm, &new_config.instance_dir) => { + // Refresh shared humans before building AgentDeps. + agent_humans.store(Arc::new(new_config.humans.clone())); + api_state.set_agent_humans(new_config.humans.clone()); // Refresh in-memory defaults so newly created agents // inherit the latest routing from the updated config. api_state.set_defaults_config(new_config.defaults.clone()).await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1842 - 1883, The initialize_agents call is using the startup agent_humans ArcSwap (agent_humans) instead of the humans from the updated config, so agents can be initialized with stale human context; fix this by ensuring the updated humans are used when building providers—either store new_config.humans into the ArcSwap (agent_humans.store(new_config.humans.clone())) before calling initialize_agents, or replace the argument agent_humans.clone() with new_config.humans.clone() in the initialize_agents invocation so the new human set is passed through to agent setup.src/identity/files.rs (1)
52-61:⚠️ Potential issue | 🟡 MinorScaffold
ROLE.mdby default to match the new identity model.
Identity::load/rendernow treatROLE.mdas first-class, but new-agent scaffolding still omits it. That leaves new workspaces without the expected role template.💡 Suggested patch
const DEFAULT_IDENTITY_FILES: &[(&str, &str)] = &[ ( "SOUL.md", "<!-- Define this agent's soul: personality, values, communication style, boundaries. -->\n", ), ( "IDENTITY.md", "<!-- Define this agent's identity: name, nature, purpose. -->\n", ), + ( + "ROLE.md", + "<!-- Define this agent's role: responsibilities, scope, and expected outcomes. -->\n", + ), ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/identity/files.rs` around lines 52 - 61, DEFAULT_IDENTITY_FILES currently omits ROLE.md causing new-agent scaffolds to miss the role template expected by Identity::load and Identity::render; add an entry for "ROLE.md" to the DEFAULT_IDENTITY_FILES array with a sensible placeholder/comment (e.g., guidance on role responsibilities, tone, and behavior) so new workspaces include the role scaffold by default and Identity::load/render will find it as first-class.interface/src/routes/AgentConfig.tsx (2)
226-226:⚠️ Potential issue | 🔴 CriticalRemove
userfrom fallback object to fix TypeScript error.Same issue as line 183 — the fallback object includes
user: nullwhich no longer matches the narrowed type.🐛 Fix for line 226
- content={getIdentityField(identityQuery.data ?? { soul: null, identity: null, user: null }, active.id)} + content={getIdentityField(identityQuery.data ?? { soul: null, identity: null }, active.id)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentConfig.tsx` at line 226, The fallback object passed into getIdentityField should match the narrowed type by removing the obsolete "user" property; update the call using identityQuery.data ?? { soul: null, identity: null } (i.e., remove user: null) so getIdentityField(identityQuery.data ?? { soul: null, identity: null }, active.id) type-checks correctly.
183-183:⚠️ Potential issue | 🔴 CriticalRemove
userfrom fallback object to fix TypeScript error.The pipeline failure indicates that the fallback object
{ soul: null, identity: null, user: null }includesuserwhich is no longer part of the expected type. Sinceuserwas removed from the identity model, this fallback should be updated.🐛 Fix for line 183
- const hasContent = !!getIdentityField(identityQuery.data ?? { soul: null, identity: null, user: null }, section.id)?.trim(); + const hasContent = !!getIdentityField(identityQuery.data ?? { soul: null, identity: null }, section.id)?.trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentConfig.tsx` at line 183, The TypeScript error is caused by the fallback object passed to getIdentityField including a removed property `user`; update the call that computes hasContent to use a fallback matching the current identity type by removing `user` (i.e., pass { soul: null, identity: null } instead of { soul: null, identity: null, user: null }) when calling getIdentityField(identityQuery.data ?? ..., section.id), ensuring getIdentityField and identityQuery.data usage stays consistent.src/api/agents.rs (2)
410-427:⚠️ Potential issue | 🟡 MinorWarmup path initializes
humanswith an empty snapshot.Line 426 sets
humansto an emptyArcSwap, while normal startup (Line 763-765) usesstate.agent_humans. This can make warmup behavior diverge from live runtime context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 410 - 427, The warmup path creates AgentDeps with humans set to a fresh empty ArcSwap snapshot which diverges from normal startup; change the warmup tokio::spawn block to initialize the humans field using the same snapshot used in live startup (state.agent_humans or equivalent shared snapshot) instead of Arc::new(arc_swap::ArcSwap::from_pointee(Vec::new())), so AgentDeps.humans matches runtime behavior during warmup.
465-531:⚠️ Potential issue | 🔴 CriticalSerialize
create_agent_internalcritical section to avoid create races.Line 465-531 has a TOCTOU window: concurrent requests can both pass limit/duplicate checks, then race on
config.tomlread/modify/write. This can lose updates or create inconsistent runtime/config state.Suggested direction
pub async fn create_agent_internal( state: &Arc<ApiState>, request: CreateAgentRequest, ) -> Result<CreateAgentResult, String> { + let _config_write_guard = state.config_write_mutex.lock().await; + if let Some(limit) = hosted_agent_limit() { let existing = state.agent_configs.load(); if existing.len() >= limit { return Err(format!( "agent limit reached for this instance: up to {limit} agent{}", if limit == 1 { "" } else { "s" } )); } } let agent_id = request.agent_id.trim().to_string(); if agent_id.is_empty() { return Err("Agent ID cannot be empty".into()); } { let existing = state.agent_configs.load(); if existing.iter().any(|a| a.id == agent_id) { return Err(format!("Agent '{agent_id}' already exists")); } } // read/parse/mutate/write config.toml while guard is heldAs per coding guidelines: “For changes in async/stateful paths ... include explicit race/terminal-state reasoning ...”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 465 - 531, The code has a TOCTOU race between the limit/duplicate checks and the config.toml read/modify/write (using state.agent_configs, request.agent_id, config_path, and agents_array); serialize this critical section by acquiring an async lock (e.g., add and use a Mutex/RwLock on a new state field like agent_creation_lock or config_write_lock) around: re-checking hosted_agent_limit() and existing agent ids, reading/parsing config.toml, modifying agents_array, writing the file, and updating in-memory state; ensure the lock scope covers all those steps so concurrent create_agent_internal callers cannot interleave and lose updates.
🧹 Nitpick comments (6)
src/tools/factory_search_context.rs (1)
67-67: Avoid abbreviated parameter names in this Rust path.Line 67 uses
sas a parameter name; please use a descriptive identifier for readability and consistency with repo conventions.As per coding guidelines, "Don't abbreviate variable names. Use `queue` not `q`, `message` not `msg`, `channel` not `ch`."Proposed rename
-fn parse_memory_type(s: &str) -> Result<crate::memory::MemoryType, FactorySearchContextError> { +fn parse_memory_type(memory_type: &str) -> Result<crate::memory::MemoryType, FactorySearchContextError> { use crate::memory::MemoryType; - match s { + match memory_type {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/factory_search_context.rs` at line 67, Rename the abbreviated parameter s in the function parse_memory_type to a descriptive identifier (e.g., type_str or memory_type_str) across the function signature and all its internal usages to match repository naming conventions; update the function signature fn parse_memory_type(type_str: &str) -> Result<crate::memory::MemoryType, FactorySearchContextError> and replace any occurrences of s inside parse_memory_type accordingly so compilation and semantics remain unchanged.src/agent/cortex_chat.rs (1)
470-470: Add a focused regression test for factory prompt gating.A unit/integration test that asserts factory prompt content is gated by
factory_enabledwould prevent accidental always-on/off regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex_chat.rs` at line 470, Add a focused test in cortex_chat.rs (or in tests/) that toggles the CortexChat (or the struct/type that holds factory_enabled) between factory_enabled = true and false and asserts the factory prompt content is included when true and excluded when false; locate the code path that builds the prompt (call the method that assembles/returns the prompt string—e.g., the prompt-building function used by CortexChat) and invoke it with both settings, then assert the expected presence/absence of the factory prompt text to prevent regressions.src/agent/channel.rs (1)
966-967: Use a non-abbreviated closure variable name here.Line 967 uses
|h|; prefer|human|for readability and consistency with repo conventions.As per coding guidelines: "Don't abbreviate variable names. Use `queue` not `q`, `message` not `msg`, `channel` not `ch`. Common abbreviations like `config` are fine".Proposed cleanup
- let humans_by_id: std::collections::HashMap<&str, &crate::config::HumanDef> = - all_humans.iter().map(|h| (h.id.as_str(), h)).collect(); + let humans_by_id: std::collections::HashMap<&str, &crate::config::HumanDef> = + all_humans + .iter() + .map(|human| (human.id.as_str(), human)) + .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 966 - 967, The closure in the collection into humans_by_id uses a short, ambiguous parameter name `|h|`; rename it to a descriptive name like `|human|` to follow the repository naming guideline. Update the map call on all_humans (the expression all_humans.iter().map(|h| (h.id.as_str(), h)).collect()) to use `|human|` and ensure types still match so the resulting std::collections::HashMap<&str, &crate::config::HumanDef> assigned to humans_by_id compiles unchanged.presets/customer-support/IDENTITY.md (1)
16-16: Consider smoothing repeated sentence openings in Scope.Line 16 starts multiple consecutive clauses with “You…”, which reads a bit choppy. A tighter rewrite would improve flow without changing meaning.
Possible rewrite
-You handle first-line and second-line support. You resolve what you can and escalate what you can't. You do not make policy decisions, issue refunds, or modify billing without explicit authorization. You do not promise features or timelines. +You handle first-line and second-line support: resolve what you can, escalate what you can’t. Policy decisions, refunds, billing changes, and feature/timeline commitments require explicit authorization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presets/customer-support/IDENTITY.md` at line 16, The Scope paragraph that begins "You handle first-line and second-line support. You resolve what you can and escalate what you can't. You do not make policy decisions, issue refunds, or modify billing without explicit authorization. You do not promise features or timelines." reads choppy due to repeated "You" openings; rewrite it into a single smoother sentence or two by combining clauses and varying openings (e.g., "Handle first- and second-line support, resolving issues you can and escalating those you cannot; do not make policy decisions, issue refunds, modify billing without explicit authorization, or promise features or timelines.") and replace the existing lines accordingly.src/tools/factory_create_agent.rs (1)
281-292: Move config loads outside the loop to avoid redundant reads.
agent_configsandhuman_configsare loaded fresh on every iteration of thelinksloop, but they don't change during this function's execution. Loading them once before the loop reduces overhead and improves clarity.♻️ Suggested refactor
+ // Load configs once before iterating + let agent_configs = self.state.agent_configs.load(); + let human_configs = self.state.agent_humans.load(); + for link_spec in &args.links { // ... direction/kind parsing ... // Validate target exists - let agent_configs = self.state.agent_configs.load(); - let human_configs = self.state.agent_humans.load(); let target_exists = agent_configs.iter().any(|a| a.id == link_spec.target) || human_configs.iter().any(|h| h.id == link_spec.target);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/factory_create_agent.rs` around lines 281 - 292, agent_configs and human_configs are being reloaded inside the links loop; move the loads out of the loop by calling self.state.agent_configs.load() and self.state.agent_humans.load() once before iterating over links so the per-iteration code (the block using link_spec.target and target_exists) reuses the cached agent_configs and human_configs variables instead of reloading them each iteration (update references to agent_configs and human_configs inside the loop accordingly).src/tools/factory_update_config.rs (1)
307-331: Hot-reload failure is logged but doesn't fail the operation — verify this is intentional.When
Config::load_from_pathfails, the config.toml has already been written but the runtime won't reflect the changes until restart. The warning log is good, but the LLM (and user) might not realize the config isn't active yet.Consider including a note in the output message when hot-reload fails, so the LLM can inform the user.
♻️ Suggested enhancement
+ let mut reload_warning = None; match crate::config::Config::load_from_path(&config_path) { Ok(new_config) => { // ... existing reload logic ... } Err(error) => { tracing::warn!( %error, "config.toml written but failed to reload immediately" ); + reload_warning = Some(format!( + " (note: config saved but hot-reload failed: {error}; restart may be required)" + )); } } - let message = format!( + let mut message = format!( "Updated {} for agent '{agent_id}'", sections_updated.join(", ") ); + if let Some(warning) = reload_warning { + message.push_str(&warning); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/factory_update_config.rs` around lines 307 - 331, The hot-reload Err arm currently only emits tracing::warn and can leave the runtime unchanged without notifying the LLM/user; update the Err branch (the match on crate::config::Config::load_from_path) to (1) improve the log message to explicitly state "config.toml written but hot-reload failed; changes will not take effect until restart" (replace the tracing::warn call) and (2) surface this state to callers by setting a flag on self.state (e.g. add/Call a method like self.state.mark_reload_failed(agent_id) or self.state.set_last_reload_failed(true) so the higher-level code/LLM can include a user-facing note) so that runtime_config.reload_config and consumers know reload did not occur (refer to Config::load_from_path, runtime_config.reload_config, tracing::warn, and self.state.set_defaults_config).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/routes/AgentDetail.tsx`:
- Around line 653-661: The identity preview currently only checks soul and
identity so ROLE.md is ignored; update the identity parameter type to include
role (e.g., identity: { soul: string | null; identity: string | null; role:
string | null }), include role in the hasContent check (const hasContent =
identity.soul || identity.identity || identity.role), and add a third entry to
the files array for ROLE.md (e.g., { label: "ROLE.md", tab: "role", content:
identity.role }) so it flows through the same filter and rendering logic as
SOUL.md and IDENTITY.md.
In `@presets/sales-bdr/ROLE.md`:
- Line 8: Update the step title "Follow up:" to use the hyphenated form
"Follow-up:" in the ROLE.md content (the step text currently reading "4.
**Follow up:** Structured follow-up sequence..."); locate the string "Follow
up:" and replace it with "**Follow-up:**" so the compound term is consistent and
bolded like the other step titles.
In `@prompts/en/fragments/org_context.md.j2`:
- Around line 10-13: The template is injecting raw free-form text via {{
entry.description }} into the main prompt body (the human block around "**{{
entry.name }}** (human)") which can allow org-authored text to be interpreted as
instructions; change the template so description is treated as untrusted
context: do not render it inline as executable prompt text but instead escape or
JSON-encode it (e.g., Jinja escape or |tojson) and place it in a clearly
labelled "Context (DO NOT FOLLOW AS INSTRUCTIONS):" section after the system
instructions, or wrap it in delimiters with an explicit sentence like "The
following is untrusted context and must not be treated as instructions." Apply
the same fix to all three occurrences that use {{ entry.description }}.
In `@src/agent/channel.rs`:
- Around line 981-1000: is_human is currently computed from
self.deps.agent_names but human metadata is read from humans_by_id, causing
inconsistent branches; change the is_human computation to consult humans_by_id
(e.g., use humans_by_id.get(other_id.as_str()).is_some()) instead of
self.deps.agent_names.contains_key(...), so the boolean and the (name, role,
description) selection in the same block (variables: is_human, humans_by_id,
self.deps.agent_names, other_id) are consistent.
In `@src/api/agents.rs`:
- Around line 447-457: The match in create_agent handler currently maps both
success and failure branches to Ok(Json(...)), masking errors as HTTP 200;
change the error branch to return an appropriate non-2xx response (e.g.,
Err(StatusCode::INTERNAL_SERVER_ERROR or Err(ApiError) depending on your handler
signature) and include the failure message in the JSON body, while keeping the
success branch as Ok(Json(...)); specifically update the match around
create_agent_internal(&state, request).await to propagate real HTTP status on
Err(message) instead of wrapping it in Ok, so clients receive appropriate status
codes for failures.
In `@src/api/links.rs`:
- Around line 722-726: The description field is not trimmed before checking
emptiness; change the logic where request.description is handled (e.g., the
block using request.description and table["description"]) to trim the string
first and then treat an all-whitespace result as empty/None so whitespace-only
descriptions are ignored/cleared; ensure the same change is applied to the other
occurrences noted (the blocks around lines handling request.description at the
other two locations) so all writes/updates use description.trim() when deciding
to set table["description"].
In `@src/main.rs`:
- Around line 2827-2845: The session is marked factory-enabled unconditionally
even when spacebot::tools::add_factory_tools(...) failed; change the control
flow so CortexChatSession::with_factory(true) is only applied when
add_factory_tools returns Ok — e.g., capture the result of add_factory_tools (or
a boolean flag) and call session.with_factory(true) only on success, otherwise
create the session without with_factory and insert that into sessions; reference
add_factory_tools, CortexChatSession::with_factory, and sessions.insert to
locate where to adjust the logic.
In `@src/tools/factory_search_context.rs`:
- Around line 41-43: Update the doc comment for the memory_type field to reflect
the actual accepted values by adding "goal" and "todo" to the listed types so it
matches the parsing logic and the enum/schema; locate the memory_type field (pub
memory_type: Option<String>) and the related enum/schema for memory types (e.g.,
MemoryType) and ensure the comment lists: "fact", "preference", "decision",
"identity", "event", "observation", "goal", and "todo".
---
Outside diff comments:
In `@interface/src/routes/AgentConfig.tsx`:
- Line 226: The fallback object passed into getIdentityField should match the
narrowed type by removing the obsolete "user" property; update the call using
identityQuery.data ?? { soul: null, identity: null } (i.e., remove user: null)
so getIdentityField(identityQuery.data ?? { soul: null, identity: null },
active.id) type-checks correctly.
- Line 183: The TypeScript error is caused by the fallback object passed to
getIdentityField including a removed property `user`; update the call that
computes hasContent to use a fallback matching the current identity type by
removing `user` (i.e., pass { soul: null, identity: null } instead of { soul:
null, identity: null, user: null }) when calling
getIdentityField(identityQuery.data ?? ..., section.id), ensuring
getIdentityField and identityQuery.data usage stays consistent.
In `@src/api/agents.rs`:
- Around line 410-427: The warmup path creates AgentDeps with humans set to a
fresh empty ArcSwap snapshot which diverges from normal startup; change the
warmup tokio::spawn block to initialize the humans field using the same snapshot
used in live startup (state.agent_humans or equivalent shared snapshot) instead
of Arc::new(arc_swap::ArcSwap::from_pointee(Vec::new())), so AgentDeps.humans
matches runtime behavior during warmup.
- Around line 465-531: The code has a TOCTOU race between the limit/duplicate
checks and the config.toml read/modify/write (using state.agent_configs,
request.agent_id, config_path, and agents_array); serialize this critical
section by acquiring an async lock (e.g., add and use a Mutex/RwLock on a new
state field like agent_creation_lock or config_write_lock) around: re-checking
hosted_agent_limit() and existing agent ids, reading/parsing config.toml,
modifying agents_array, writing the file, and updating in-memory state; ensure
the lock scope covers all those steps so concurrent create_agent_internal
callers cannot interleave and lose updates.
In `@src/identity/files.rs`:
- Around line 52-61: DEFAULT_IDENTITY_FILES currently omits ROLE.md causing
new-agent scaffolds to miss the role template expected by Identity::load and
Identity::render; add an entry for "ROLE.md" to the DEFAULT_IDENTITY_FILES array
with a sensible placeholder/comment (e.g., guidance on role responsibilities,
tone, and behavior) so new workspaces include the role scaffold by default and
Identity::load/render will find it as first-class.
In `@src/main.rs`:
- Around line 1842-1883: The initialize_agents call is using the startup
agent_humans ArcSwap (agent_humans) instead of the humans from the updated
config, so agents can be initialized with stale human context; fix this by
ensuring the updated humans are used when building providers—either store
new_config.humans into the ArcSwap
(agent_humans.store(new_config.humans.clone())) before calling
initialize_agents, or replace the argument agent_humans.clone() with
new_config.humans.clone() in the initialize_agents invocation so the new human
set is passed through to agent setup.
---
Nitpick comments:
In `@presets/customer-support/IDENTITY.md`:
- Line 16: The Scope paragraph that begins "You handle first-line and
second-line support. You resolve what you can and escalate what you can't. You
do not make policy decisions, issue refunds, or modify billing without explicit
authorization. You do not promise features or timelines." reads choppy due to
repeated "You" openings; rewrite it into a single smoother sentence or two by
combining clauses and varying openings (e.g., "Handle first- and second-line
support, resolving issues you can and escalating those you cannot; do not make
policy decisions, issue refunds, modify billing without explicit authorization,
or promise features or timelines.") and replace the existing lines accordingly.
In `@src/agent/channel.rs`:
- Around line 966-967: The closure in the collection into humans_by_id uses a
short, ambiguous parameter name `|h|`; rename it to a descriptive name like
`|human|` to follow the repository naming guideline. Update the map call on
all_humans (the expression all_humans.iter().map(|h| (h.id.as_str(),
h)).collect()) to use `|human|` and ensure types still match so the resulting
std::collections::HashMap<&str, &crate::config::HumanDef> assigned to
humans_by_id compiles unchanged.
In `@src/agent/cortex_chat.rs`:
- Line 470: Add a focused test in cortex_chat.rs (or in tests/) that toggles the
CortexChat (or the struct/type that holds factory_enabled) between
factory_enabled = true and false and asserts the factory prompt content is
included when true and excluded when false; locate the code path that builds the
prompt (call the method that assembles/returns the prompt string—e.g., the
prompt-building function used by CortexChat) and invoke it with both settings,
then assert the expected presence/absence of the factory prompt text to prevent
regressions.
In `@src/tools/factory_create_agent.rs`:
- Around line 281-292: agent_configs and human_configs are being reloaded inside
the links loop; move the loads out of the loop by calling
self.state.agent_configs.load() and self.state.agent_humans.load() once before
iterating over links so the per-iteration code (the block using link_spec.target
and target_exists) reuses the cached agent_configs and human_configs variables
instead of reloading them each iteration (update references to agent_configs and
human_configs inside the loop accordingly).
In `@src/tools/factory_search_context.rs`:
- Line 67: Rename the abbreviated parameter s in the function parse_memory_type
to a descriptive identifier (e.g., type_str or memory_type_str) across the
function signature and all its internal usages to match repository naming
conventions; update the function signature fn parse_memory_type(type_str: &str)
-> Result<crate::memory::MemoryType, FactorySearchContextError> and replace any
occurrences of s inside parse_memory_type accordingly so compilation and
semantics remain unchanged.
In `@src/tools/factory_update_config.rs`:
- Around line 307-331: The hot-reload Err arm currently only emits tracing::warn
and can leave the runtime unchanged without notifying the LLM/user; update the
Err branch (the match on crate::config::Config::load_from_path) to (1) improve
the log message to explicitly state "config.toml written but hot-reload failed;
changes will not take effect until restart" (replace the tracing::warn call) and
(2) surface this state to callers by setting a flag on self.state (e.g. add/Call
a method like self.state.mark_reload_failed(agent_id) or
self.state.set_last_reload_failed(true) so the higher-level code/LLM can include
a user-facing note) so that runtime_config.reload_config and consumers know
reload did not occur (refer to Config::load_from_path,
runtime_config.reload_config, tracing::warn, and
self.state.set_defaults_config).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2940368f-2d71-4f0f-88d7-13f38bb1d1f7
⛔ Files ignored due to path filters (9)
presets/community-manager/meta.tomlis excluded by!**/*.tomlpresets/content-writer/meta.tomlis excluded by!**/*.tomlpresets/customer-support/meta.tomlis excluded by!**/*.tomlpresets/engineering-assistant/meta.tomlis excluded by!**/*.tomlpresets/executive-assistant/meta.tomlis excluded by!**/*.tomlpresets/main-agent/meta.tomlis excluded by!**/*.tomlpresets/project-manager/meta.tomlis excluded by!**/*.tomlpresets/research-analyst/meta.tomlis excluded by!**/*.tomlpresets/sales-bdr/meta.tomlis excluded by!**/*.toml
📒 Files selected for processing (73)
docs/design-docs/agent-factory.mdinterface/src/api/client.tsinterface/src/components/TopologyGraph.tsxinterface/src/routes/AgentConfig.tsxinterface/src/routes/AgentDetail.tsxpresets/community-manager/IDENTITY.mdpresets/community-manager/ROLE.mdpresets/community-manager/SOUL.mdpresets/content-writer/IDENTITY.mdpresets/content-writer/ROLE.mdpresets/content-writer/SOUL.mdpresets/customer-support/IDENTITY.mdpresets/customer-support/ROLE.mdpresets/customer-support/SOUL.mdpresets/engineering-assistant/IDENTITY.mdpresets/engineering-assistant/ROLE.mdpresets/engineering-assistant/SOUL.mdpresets/executive-assistant/IDENTITY.mdpresets/executive-assistant/ROLE.mdpresets/executive-assistant/SOUL.mdpresets/main-agent/IDENTITY.mdpresets/main-agent/ROLE.mdpresets/main-agent/SOUL.mdpresets/project-manager/IDENTITY.mdpresets/project-manager/ROLE.mdpresets/project-manager/SOUL.mdpresets/research-analyst/IDENTITY.mdpresets/research-analyst/ROLE.mdpresets/research-analyst/SOUL.mdpresets/sales-bdr/IDENTITY.mdpresets/sales-bdr/ROLE.mdpresets/sales-bdr/SOUL.mdprompts/en/cortex_chat.md.j2prompts/en/factory.md.j2prompts/en/fragments/org_context.md.j2prompts/en/tools/factory_create_agent_description.md.j2prompts/en/tools/factory_list_presets_description.md.j2prompts/en/tools/factory_load_preset_description.md.j2prompts/en/tools/factory_search_context_description.md.j2prompts/en/tools/factory_update_config_description.md.j2prompts/en/tools/factory_update_identity_description.md.j2prompts/en/tools/file_description.md.j2prompts/en/worker.md.j2src/agent/channel.rssrc/agent/cortex_chat.rssrc/api.rssrc/api/agents.rssrc/api/factory.rssrc/api/links.rssrc/api/server.rssrc/api/state.rssrc/config/load.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rssrc/factory.rssrc/factory/presets.rssrc/identity.rssrc/identity/files.rssrc/lib.rssrc/main.rssrc/prompts/engine.rssrc/prompts/text.rssrc/tools.rssrc/tools/factory_create_agent.rssrc/tools/factory_list_presets.rssrc/tools/factory_load_preset.rssrc/tools/factory_search_context.rssrc/tools/factory_update_config.rssrc/tools/factory_update_identity.rssrc/tools/file.rstests/bulletin.rstests/context_dump.rs
| 1. **Research:** Learn about the prospect before reaching out. Company, role, recent activity, potential pain points. | ||
| 2. **Qualify:** Assess fit against qualification criteria before investing effort. | ||
| 3. **Draft:** Write personalized outreach. Reference something specific. Lead with value. | ||
| 4. **Follow up:** Structured follow-up sequence. Each touchpoint adds new value, not just "checking in." |
Contributor
There was a problem hiding this comment.
Hyphenate “Follow-up” in the step title for consistency.
Line 8 reads better as “Follow-up” (compound modifier/term).
Suggested edit
-4. **Follow up:** Structured follow-up sequence. Each touchpoint adds new value, not just "checking in."
+4. **Follow-up:** Structured follow-up sequence. Each touchpoint adds new value, not just "checking in."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 4. **Follow up:** Structured follow-up sequence. Each touchpoint adds new value, not just "checking in." | |
| 4. **Follow-up:** Structured follow-up sequence. Each touchpoint adds new value, not just "checking in." |
🧰 Tools
🪛 LanguageTool
[grammar] ~8-~8: Use a hyphen to join words.
Context: ...g specific. Lead with value. 4. Follow up: Structured follow-up sequence. Eac...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@presets/sales-bdr/ROLE.md` at line 8, Update the step title "Follow up:" to
use the hyphenated form "Follow-up:" in the ROLE.md content (the step text
currently reading "4. **Follow up:** Structured follow-up sequence..."); locate
the string "Follow up:" and replace it with "**Follow-up:**" so the compound
term is consistent and bolded like the other step titles.
src/api/links.rs
Outdated
Comment on lines
+722
to
+726
| if let Some(description) = &request.description | ||
| && !description.is_empty() | ||
| { | ||
| table["description"] = toml_edit::value(description.as_str()); | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize description with trimming before storing/updating.
Whitespace-only descriptions currently pass through. Trim first so " " behaves as empty/clear.
💡 Suggested patch
- if let Some(description) = &request.description
- && !description.is_empty()
+ if let Some(description) = request.description.as_deref().map(str::trim)
+ && !description.is_empty()
{
- table["description"] = toml_edit::value(description.as_str());
+ table["description"] = toml_edit::value(description);
}
@@
- description: request.description.clone().filter(|s| !s.is_empty()),
+ description: request
+ .description
+ .as_deref()
+ .map(str::trim)
+ .filter(|s| !s.is_empty())
+ .map(ToOwned::to_owned),
@@
- if let Some(description) = &request.description {
- updated.description = if description.is_empty() {
+ if let Some(description) = request.description.as_deref().map(str::trim) {
+ updated.description = if description.is_empty() {
None
} else {
- Some(description.clone())
+ Some(description.to_string())
};
}Also applies to: 741-741, 789-795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/links.rs` around lines 722 - 726, The description field is not
trimmed before checking emptiness; change the logic where request.description is
handled (e.g., the block using request.description and table["description"]) to
trim the string first and then treat an all-whitespace result as empty/None so
whitespace-only descriptions are ignored/cleared; ensure the same change is
applied to the other occurrences noted (the blocks around lines handling
request.description at the other two locations) so all writes/updates use
description.trim() when deciding to set table["description"].
Comment on lines
+41
to
+43
| /// Optional memory type filter: "fact", "preference", "decision", "identity", "event", "observation". | ||
| #[serde(default)] | ||
| pub memory_type: Option<String>, |
Contributor
There was a problem hiding this comment.
Keep memory_type docs aligned with actual accepted values.
Line 41 lists valid types but omits goal and todo, which are accepted in parsing and exposed in the enum schema. This mismatch can mislead tool callers.
Proposed doc fix
- /// Optional memory type filter: "fact", "preference", "decision", "identity", "event", "observation".
+ /// Optional memory type filter: "fact", "preference", "decision", "identity", "event", "observation", "goal", "todo".📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Optional memory type filter: "fact", "preference", "decision", "identity", "event", "observation". | |
| #[serde(default)] | |
| pub memory_type: Option<String>, | |
| /// Optional memory type filter: "fact", "preference", "decision", "identity", "event", "observation", "goal", "todo". | |
| #[serde(default)] | |
| pub memory_type: Option<String>, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/factory_search_context.rs` around lines 41 - 43, Update the doc
comment for the memory_type field to reflect the actual accepted values by
adding "goal" and "todo" to the listed types so it matches the parsing logic and
the enum/schema; locate the memory_type field (pub memory_type: Option<String>)
and the related enum/schema for memory types (e.g., MemoryType) and ensure the
comment lists: "fact", "preference", "decision", "identity", "event",
"observation", "goal", and "todo".
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
vsumner
suggested changes
Mar 5, 2026
Contributor
vsumner
left a comment
vsumner
left a comment
There was a problem hiding this comment.
Thanks for the substantial factory integration work. Could we tighten a few concurrency/error-handling edges before merge? I left focused inline suggestions/questions below.
| State(state): State<Arc<ApiState>>, | ||
| Json(request): Json<CreateAgentRequest>, | ||
| ) -> Result<Json<serde_json::Value>, StatusCode> { | ||
| match create_agent_internal(&state, request).await { |
Contributor
There was a problem hiding this comment.
Could we preserve HTTP status semantics here so internal failures still surface as 5xx? A possible path is returning a typed error from create_agent_internal (validation vs internal) and mapping that in the handler while keeping this friendly JSON shape.
| @@ -474,22 +492,22 @@ pub(super) async fn create_agent( | |||
| .await | |||
Contributor
There was a problem hiding this comment.
Would it make sense to take config_write_mutex around this read/modify/write path? I think this currently races with other config writers that do lock, which could lead to clobbered config.toml updates under concurrency.
src/tools/factory_update_config.rs
Outdated
| })?; | ||
|
|
||
| // Hot-reload the config into RuntimeConfig | ||
| match crate::config::Config::load_from_path(&config_path) { |
Contributor
There was a problem hiding this comment.
Could we consider failing the tool call if reload fails after write? Right now we warn and return success, which might leave disk/runtime out of sync and be hard to diagnose for callers.
src/tools/factory_create_agent.rs
Outdated
| } | ||
|
|
||
| // Check for duplicate link | ||
| let existing_links = self.state.agent_links.load(); |
Contributor
There was a problem hiding this comment.
Small race question: should duplicate-link detection move into the same mutex-guarded section as write_link_to_config? The current pre-check on agent_links looks like a check-then-write window where concurrent calls could still append duplicates.
src/tools/file.rs
Outdated
| if self.sandbox.mode_enabled() { | ||
| let file_name = path.file_name().and_then(|n| n.to_str()).unwrap_or(""); | ||
| const PROTECTED_FILES: &[&str] = &["SOUL.md", "IDENTITY.md", "USER.md"]; | ||
| const PROTECTED_FILES: &[&str] = &["SOUL.md", "IDENTITY.md"]; |
Contributor
There was a problem hiding this comment.
Could we include ROLE.md in PROTECTED_FILES as well? Since ROLE is now part of identity behavior, protecting it in sandbox mode seems consistent with SOUL/IDENTITY handling.
# Conflicts: # interface/src/routes/AgentConfig.tsx # src/api/server.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
interface/src/routes/AgentConfig.tsx (1)
18-18:⚠️ Potential issue | 🔴 CriticalFinish removing the deprecated
useridentity shape.
getIdentityField()now only acceptssoul/identity, but both fallback literals still pass{ user: null }, which is exactly what Interface CI is failing on at Lines 221 and 275. This file also still models"user"inSectionId, so it can drift back out of sync with the API again.Suggested fix
-import { api, type AgentConfigResponse, type AgentConfigUpdateRequest } from "@/api/client"; +import { api, type AgentConfigResponse, type AgentConfigUpdateRequest, type IdentityFiles } from "@/api/client"; @@ -type SectionId = "general" | "soul" | "identity" | "user" | "routing" | "tuning" | "compaction" | "cortex" | "coalesce" | "memory" | "browser" | "channel" | "sandbox"; +type SectionId = "general" | "soul" | "identity" | "routing" | "tuning" | "compaction" | "cortex" | "coalesce" | "memory" | "browser" | "channel" | "sandbox"; @@ -const getIdentityField = (data: { soul: string | null; identity: string | null }, field: SectionId): string | null => { +const getIdentityField = (data: IdentityFiles, field: SectionId): string | null => { @@ - const hasContent = !!getIdentityField(identityQuery.data ?? { soul: null, identity: null, user: null }, section.id)?.trim(); + const hasContent = !!getIdentityField(identityQuery.data ?? { soul: null, identity: null }, section.id)?.trim(); @@ - content={getIdentityField(identityQuery.data ?? { soul: null, identity: null, user: null }, active.id)} + content={getIdentityField(identityQuery.data ?? { soul: null, identity: null }, active.id)}Also applies to: 45-50, 221-221, 275-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentConfig.tsx` at line 18, Remove the deprecated "user" identity shape: update the SectionId type by removing "user" from the union (symbol: SectionId) and replace any fallback identity literals that use "{ user: null }" with the supported shape expected by getIdentityField (use "{ soul: null }" or "{ identity: null }" as appropriate where the code calls getIdentityField). Search for occurrences around the getIdentityField calls (the fallback objects at the locations referenced) and update those to the new key, ensuring all references that modeled "user" (including the earlier SectionId definition and the fallback literals) are consistent with the soul/identity-only API.src/api/agents.rs (2)
837-850:⚠️ Potential issue | 🟠 MajorRetain abort handles for live-created background tasks.
These warmup/cortex/association/ready-task/ingestion loops are spawned and immediately dropped.
delete_agent()only removes state and closes the pool, so those tasks keep running against stale deps until process exit.
530-663:⚠️ Potential issue | 🟠 MajorDon't persist the new agent before initialization succeeds.
Line 530 writes the new
[[agents]]entry before the directory/DB/identity setup below. Any later failure returnsErr(...)but leaves the agent on disk, and a retry in the same process can append another entry because the duplicate check only consultsstate.agent_configs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 530 - 663, The new agent TOML is being persisted with tokio::fs::write(&config_path, ...) before the subsequent initialization steps (dir creation, crate::db::Db::connect, SettingsStore::new, EmbeddingTable::open_or_create, scaffold_identity_files, etc.), which can leave a stale/duplicate agent on disk if initialization later fails; move the persistence so it only happens after all initialization succeeds (e.g., perform directory creation, DB connect, settings_store, embedding_table.ensure_fts_index, memory/task store construction, and crate::identity::scaffold_identity_files(&agent_config.workspace).await first), then write the config atomically (or write to a temp file and rename) using tokio::fs::write(&config_path, ...) and only then update in-memory state (state.agent_configs / state.set_defaults_config as appropriate) so failed initializations do not leave partial agent entries on disk.
♻️ Duplicate comments (5)
src/agent/channel.rs (1)
1674-1700:⚠️ Potential issue | 🟡 MinorUse
humans_by_idas the source of truth foris_human.
is_humanis still inferred fromagent_names, while the metadata branch now reads fromhumans_by_id. If an ID is missing fromagent_namesbut also absent fromhumans_by_id, this builds aLinkedAgentwithis_human = trueand no human metadata, so the org prompt will mislabel an unknown link as a human. Make the boolean follow the same lookup used for(name, role, description).Suggested fix
- let is_human = !self.deps.agent_names.contains_key(other_id.as_str()); - - let (name, role, description) = if let Some(human) = humans_by_id.get(other_id.as_str()) - { + let human = humans_by_id.get(other_id.as_str()); + let is_human = human.is_some(); + + let (name, role, description) = if let Some(human) = human { // Human node — use display_name, role, and description from HumanDef let name = human .display_name .clone() .unwrap_or_else(|| other_id.clone());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1674 - 1700, The is_human flag is computed from deps.agent_names but the subsequent metadata lookup uses humans_by_id; change the is_human computation to mirror that lookup (e.g., use humans_by_id.contains_key(other_id.as_str()) or humans_by_id.get(...) to decide truthiness) so the value aligns with the (name, role, description) branch and the LinkedAgent constructed in crate::prompts::engine::LinkedAgent does not mislabel unknown IDs as humans; update the is_human binding near where other_id, humans_by_id, and deps.agent_names are used so the same source of truth is used for both the boolean and the metadata.src/api/agents.rs (3)
821-835:⚠️ Potential issue | 🟠 MajorGate factory mode on
add_factory_tools()here too.This live-create path has the same mismatch as startup: if Lines 821-827 fail, Line 835 still enables factory prompting for a session that does not have factory tools registered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 821 - 835, The code enables factory prompting unconditionally by calling CortexChatSession::with_factory(true) even if crate::tools::add_factory_tools(...) failed; change the flow to only enable factory mode when add_factory_tools succeeds—capture the result of add_factory_tools (e.g., a bool or an Ok branch) and call with_factory(true) only in that success path (otherwise leave the session default or call with_factory(false)); reference the add_factory_tools function and CortexChatSession::with_factory to locate where to gate the flag.
451-460:⚠️ Potential issue | 🟠 MajorReturn a non-2xx status when creation fails.
The
Err(message)branch still returnsOk(Json(...)), so callers cannot distinguish validation/internal failures from success at the HTTP layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 451 - 460, The Err(message) arm currently returns Ok(Json(...)) so failures still produce 2xx responses; change that branch to return a non-2xx HTTP response by converting the error into an appropriate error response (e.g., return Err((StatusCode::BAD_REQUEST, Json(serde_json::json!({"success": false, "message": message}))).into_response()) or otherwise use axum/tide/actix error-response patterns) so callers see a non-2xx status when create_agent_internal(&state, request).await returns Err; update the match in the handler accordingly (the match around create_agent_internal) to use IntoResponse/Error types rather than always Ok.
491-535:⚠️ Potential issue | 🟠 MajorSerialize this
config.tomlread/modify/write path.
create_agent_internal()now sits on the shared API/factory path, but it still editsconfig.tomlwithout takingconfig_write_mutex. Concurrent create/update/delete writes can race and clobber each other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 491 - 535, The config.toml read/modify/write sequence in create_agent_internal is not protected by the shared write mutex, allowing concurrent create/update/delete calls to race; fix this by obtaining the state's config_write_mutex (e.g., let _guard = state.config_write_mutex.lock().await) before reading config_path and hold the guard through parsing, modifying the toml_edit::DocumentMut (agents_array push) and the tokio::fs::write call, then drop the guard after the write completes so the entire read-modify-write is serialized; ensure you reference create_agent_internal, state.config_path, and the mutex field (config_write_mutex) when applying the change.src/main.rs (1)
3144-3161:⚠️ Potential issue | 🟠 MajorGate
with_factory(true)on successful tool registration.If
add_factory_tools()fails here, Line 3161 still enables the factory prompt path. That advertises tools the session does not actually have and can push the model into guaranteed tool-call failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 3144 - 3161, The code enables the factory path unconditionally via CortexChatSession::with_factory(true) even when add_factory_tools(...) failed; change the flow so with_factory(true) is only called when add_factory_tools(...) returns Ok: keep the current add_factory_tools(...) call and its Err branch for logging, capture its success (e.g. if let Ok(_) = spacebot::tools::add_factory_tools(...) .await { session = CortexChatSession::new(...).with_factory(true); } else { session = CortexChatSession::new(...); } so the session only advertises factory tools when add_factory_tools succeeded.
🧹 Nitpick comments (1)
src/tools.rs (1)
636-647: Keep factory tool registration in one place.
create_factory_tool_server()andadd_factory_tools()duplicate the same six-tool sequence. That makes it easy for standalone factory servers and cortex-chat augmentation to drift apart the next time a tool is added or removed. A shared internal registrar would make that surface area much harder to desync.Also applies to: 655-672
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools.rs` around lines 636 - 647, The two duplicated six-tool registration sequences in create_factory_tool_server and add_factory_tools should be unified: add a small shared helper (e.g., register_factory_tools or add_factory_tools_to_server) that accepts a ToolServer (or mutable builder) and registers FactoryListPresetsTool, FactoryLoadPresetTool, FactorySearchContextTool (memory_search), FactoryCreateAgentTool (state.clone()), FactoryUpdateIdentityTool (state.clone()), and FactoryUpdateConfigTool (state) in one place, then call that helper from both create_factory_tool_server and the other location; update the references to memory_search and state parameters so the shared registrar receives the needed Arc<MemorySearch> and Arc<ApiState> (or clones) and remove the duplicated registration blocks in both create_factory_tool_server and add_factory_tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/agents.rs`:
- Around line 425-427: The AgentDeps instance used for API-triggered warmups is
currently created with an empty humans list; instead, use the existing org
context snapshot from state.agent_humans so warmup output matches normal
runtime. Locate the AgentDeps construction (fields links, agent_names, humans)
and replace the humans initialization
(Arc::new(arc_swap::ArcSwap::from_pointee(Vec::new()))) with the live snapshot
or shared Arc from state.agent_humans (e.g., Arc::clone(&state.agent_humans) or
otherwise load the current snapshot via state.agent_humans.load_full()) so
trigger_warmup runs with the same humans context as the live agent path.
In `@src/main.rs`:
- Around line 1448-1450: agent_humans is created with ArcSwap but never wired
into the config file-watcher update path so changes to config.humans never
propagate; create the ArcSwap (keep
Arc::new(ArcSwap::from_pointee(config.humans.clone()))) and pass that same Arc
into the AgentDeps like agent_links, then in the file-watcher/update handler
call agent_humans.load_full() to inspect or
agent_humans.swap(Arc::new(new_humans)) (or ArcSwap::from_pointee equivalent)
when the config reloads; ensure the watcher holds the Arc and updates it the
same way agent_links is updated so running agents see hot-reloaded humans.
---
Outside diff comments:
In `@interface/src/routes/AgentConfig.tsx`:
- Line 18: Remove the deprecated "user" identity shape: update the SectionId
type by removing "user" from the union (symbol: SectionId) and replace any
fallback identity literals that use "{ user: null }" with the supported shape
expected by getIdentityField (use "{ soul: null }" or "{ identity: null }" as
appropriate where the code calls getIdentityField). Search for occurrences
around the getIdentityField calls (the fallback objects at the locations
referenced) and update those to the new key, ensuring all references that
modeled "user" (including the earlier SectionId definition and the fallback
literals) are consistent with the soul/identity-only API.
In `@src/api/agents.rs`:
- Around line 530-663: The new agent TOML is being persisted with
tokio::fs::write(&config_path, ...) before the subsequent initialization steps
(dir creation, crate::db::Db::connect, SettingsStore::new,
EmbeddingTable::open_or_create, scaffold_identity_files, etc.), which can leave
a stale/duplicate agent on disk if initialization later fails; move the
persistence so it only happens after all initialization succeeds (e.g., perform
directory creation, DB connect, settings_store,
embedding_table.ensure_fts_index, memory/task store construction, and
crate::identity::scaffold_identity_files(&agent_config.workspace).await first),
then write the config atomically (or write to a temp file and rename) using
tokio::fs::write(&config_path, ...) and only then update in-memory state
(state.agent_configs / state.set_defaults_config as appropriate) so failed
initializations do not leave partial agent entries on disk.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 1674-1700: The is_human flag is computed from deps.agent_names but
the subsequent metadata lookup uses humans_by_id; change the is_human
computation to mirror that lookup (e.g., use
humans_by_id.contains_key(other_id.as_str()) or humans_by_id.get(...) to decide
truthiness) so the value aligns with the (name, role, description) branch and
the LinkedAgent constructed in crate::prompts::engine::LinkedAgent does not
mislabel unknown IDs as humans; update the is_human binding near where other_id,
humans_by_id, and deps.agent_names are used so the same source of truth is used
for both the boolean and the metadata.
In `@src/api/agents.rs`:
- Around line 821-835: The code enables factory prompting unconditionally by
calling CortexChatSession::with_factory(true) even if
crate::tools::add_factory_tools(...) failed; change the flow to only enable
factory mode when add_factory_tools succeeds—capture the result of
add_factory_tools (e.g., a bool or an Ok branch) and call with_factory(true)
only in that success path (otherwise leave the session default or call
with_factory(false)); reference the add_factory_tools function and
CortexChatSession::with_factory to locate where to gate the flag.
- Around line 451-460: The Err(message) arm currently returns Ok(Json(...)) so
failures still produce 2xx responses; change that branch to return a non-2xx
HTTP response by converting the error into an appropriate error response (e.g.,
return Err((StatusCode::BAD_REQUEST, Json(serde_json::json!({"success": false,
"message": message}))).into_response()) or otherwise use axum/tide/actix
error-response patterns) so callers see a non-2xx status when
create_agent_internal(&state, request).await returns Err; update the match in
the handler accordingly (the match around create_agent_internal) to use
IntoResponse/Error types rather than always Ok.
- Around line 491-535: The config.toml read/modify/write sequence in
create_agent_internal is not protected by the shared write mutex, allowing
concurrent create/update/delete calls to race; fix this by obtaining the state's
config_write_mutex (e.g., let _guard = state.config_write_mutex.lock().await)
before reading config_path and hold the guard through parsing, modifying the
toml_edit::DocumentMut (agents_array push) and the tokio::fs::write call, then
drop the guard after the write completes so the entire read-modify-write is
serialized; ensure you reference create_agent_internal, state.config_path, and
the mutex field (config_write_mutex) when applying the change.
In `@src/main.rs`:
- Around line 3144-3161: The code enables the factory path unconditionally via
CortexChatSession::with_factory(true) even when add_factory_tools(...) failed;
change the flow so with_factory(true) is only called when add_factory_tools(...)
returns Ok: keep the current add_factory_tools(...) call and its Err branch for
logging, capture its success (e.g. if let Ok(_) =
spacebot::tools::add_factory_tools(...) .await { session =
CortexChatSession::new(...).with_factory(true); } else { session =
CortexChatSession::new(...); } so the session only advertises factory tools when
add_factory_tools succeeded.
---
Nitpick comments:
In `@src/tools.rs`:
- Around line 636-647: The two duplicated six-tool registration sequences in
create_factory_tool_server and add_factory_tools should be unified: add a small
shared helper (e.g., register_factory_tools or add_factory_tools_to_server) that
accepts a ToolServer (or mutable builder) and registers FactoryListPresetsTool,
FactoryLoadPresetTool, FactorySearchContextTool (memory_search),
FactoryCreateAgentTool (state.clone()), FactoryUpdateIdentityTool
(state.clone()), and FactoryUpdateConfigTool (state) in one place, then call
that helper from both create_factory_tool_server and the other location; update
the references to memory_search and state parameters so the shared registrar
receives the needed Arc<MemorySearch> and Arc<ApiState> (or clones) and remove
the duplicated registration blocks in both create_factory_tool_server and
add_factory_tools.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77bc6869-54ca-4295-a7f9-fa62c3e3e7f6
📒 Files selected for processing (16)
interface/src/api/client.tsinterface/src/routes/AgentConfig.tsxprompts/en/worker.md.j2src/agent/channel.rssrc/api.rssrc/api/agents.rssrc/api/server.rssrc/api/state.rssrc/config/load.rssrc/config/toml_schema.rssrc/config/types.rssrc/lib.rssrc/main.rssrc/tools.rstests/bulletin.rstests/context_dump.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- src/config/types.rs
- src/api/state.rs
- src/config/load.rs
- src/lib.rs
- src/config/toml_schema.rs
- prompts/en/worker.md.j2
- interface/src/api/client.ts
- tests/bulletin.rs
- src/api.rs
- tests/context_dump.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/tools/factory_update_identity.rs (1)
75-96: Encode the “at least one content field” rule in the schema.Right now the definition says
agent_idis the only required field, but the implementation rejects calls where all three content fields are omitted. Adding ananyOfoversoul_content,identity_content, androle_contentwill let the model avoid an otherwise guaranteed tool error.🔧 Proposed schema tweak
parameters: serde_json::json!({ "type": "object", "required": ["agent_id"], + "anyOf": [ + { "required": ["soul_content"] }, + { "required": ["identity_content"] }, + { "required": ["role_content"] } + ], "properties": {Also applies to: 114-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/factory_update_identity.rs` around lines 75 - 96, The JSON schema under the parameters object currently only requires "agent_id" but the code expects at least one of "soul_content", "identity_content", or "role_content"; update the schema to add an anyOf constraint referencing those three properties so requests with none of them are rejected by validation rather than runtime logic (add an anyOf array with entries like { "required": ["soul_content"] }, { "required": ["identity_content"] }, { "required": ["role_content"] } inside the parameters JSON). Apply the same change to the other similar parameters block noted around the later occurrence (the second parameters definition at lines ~114-122) so both schemas enforce "at least one content field."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/factory_update_config.rs`:
- Around line 263-264: The factory_update_config path currently locks
self.state.config_write_mutex but update_agent_config (in ApiState) performs its
own read-modify-write without that mutex, allowing races; extract the shared
read-modify-write logic into a single async helper (e.g.,
apply_config_update_locked or ConfigState::with_locked_config_update) that
acquires ApiState.config_write_mutex, reads config.toml, applies the update,
writes back and releases the lock, then call that helper from both
factory_update_config and update_agent_config (replace their inline
read-modify-write sequences) so both routes use the same mutex and prevent lost
edits.
- Around line 193-196: The current guard only checks that args.routing or
args.tuning are Some but allows empty objects; update the validation around
args.routing and args.tuning (the block that returns FactoryUpdateConfigError
and the similar block later at 284-295) to reject empty section values by
treating Some(empty) as invalid: inspect the contained routing/tuning struct or
map (the fields inside args.routing and args.tuning) and return
FactoryUpdateConfigError("at least one of routing or tuning must be provided")
if the provided section has no populated fields, and only proceed to modify
sections_updated or write back when the section contains at least one populated
override. Ensure the same non-empty check is applied in both places mentioned.
In `@src/tools/factory_update_identity.rs`:
- Around line 147-161: The current loop over updates aborts on the first write
error (via FactoryUpdateIdentityError) leaving a partially-applied workspace and
never reloading the identity; change it to best-effort: iterate all (filename,
content) in updates, on each write attempt push filename into files_updated on
success or push (filename, error) into files_failed on failure (do not
early-return), then after the loop if files_updated is non-empty call
crate::identity::Identity::load(workspace).await and, if
runtime_configs.load().get(&agent_id) is Some, call
runtime_config.identity.store(Arc::new(identity)); finally return a structured
result (e.g., a new enum/struct containing files_updated and files_failed)
instead of returning FactoryUpdateIdentityError on the first failure so callers
can recover per-file.
---
Nitpick comments:
In `@src/tools/factory_update_identity.rs`:
- Around line 75-96: The JSON schema under the parameters object currently only
requires "agent_id" but the code expects at least one of "soul_content",
"identity_content", or "role_content"; update the schema to add an anyOf
constraint referencing those three properties so requests with none of them are
rejected by validation rather than runtime logic (add an anyOf array with
entries like { "required": ["soul_content"] }, { "required":
["identity_content"] }, { "required": ["role_content"] } inside the parameters
JSON). Apply the same change to the other similar parameters block noted around
the later occurrence (the second parameters definition at lines ~114-122) so
both schemas enforce "at least one content field."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e493a49-4ab7-425c-b116-405d746af6e3
📒 Files selected for processing (5)
src/factory/presets.rssrc/tools/factory_list_presets.rssrc/tools/factory_load_preset.rssrc/tools/factory_update_config.rssrc/tools/factory_update_identity.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tools/factory_list_presets.rs
Comment on lines
+193
to
+196
| if args.routing.is_none() && args.tuning.is_none() { | ||
| return Err(FactoryUpdateConfigError( | ||
| "at least one of routing or tuning must be provided".into(), | ||
| )); |
Contributor
There was a problem hiding this comment.
Reject empty routing / tuning objects.
{"routing":{}} or {"tuning":{}} passes the current guard, gets reported in sections_updated, and routing can even be written back as an empty table. That returns a successful update even though no concrete override was supplied. Please require at least one populated field inside each requested section before writing or reporting success.
Also applies to: 284-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/factory_update_config.rs` around lines 193 - 196, The current guard
only checks that args.routing or args.tuning are Some but allows empty objects;
update the validation around args.routing and args.tuning (the block that
returns FactoryUpdateConfigError and the similar block later at 284-295) to
reject empty section values by treating Some(empty) as invalid: inspect the
contained routing/tuning struct or map (the fields inside args.routing and
args.tuning) and return FactoryUpdateConfigError("at least one of routing or
tuning must be provided") if the provided section has no populated fields, and
only proceed to modify sections_updated or write back when the section contains
at least one populated override. Ensure the same non-empty check is applied in
both places mentioned.
Comment on lines
+147
to
+161
| for (filename, content) in &updates { | ||
| if let Some(content) = content { | ||
| let path = workspace.join(filename); | ||
| tokio::fs::write(&path, content).await.map_err(|error| { | ||
| FactoryUpdateIdentityError(format!("failed to write {filename}: {error}")) | ||
| })?; | ||
| files_updated.push(filename.to_string()); | ||
| } | ||
| } | ||
|
|
||
| // Reload identity into runtime config so the agent picks up changes | ||
| // immediately without requiring a restart. | ||
| let identity = crate::identity::Identity::load(workspace).await; | ||
| if let Some(runtime_config) = self.state.runtime_configs.load().get(&agent_id) { | ||
| runtime_config.identity.store(Arc::new(identity)); |
Contributor
There was a problem hiding this comment.
Avoid returning a plain failure after partially applying writes.
If Line 150 fails on the second or third file, earlier files have already been overwritten, but Lines 159-161 never run. That leaves the workspace partially updated while the live identity still points at the old snapshot, and the caller gets no per-file status to recover from. Either make this batch transactional, or switch to best-effort writes with structured files_updated/files_failed reporting and reload when any write succeeds.
As per coding guidelines, "Tool implementations must return errors as structured results, not panics; tools should be error-as-result for LLM recovery."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/factory_update_identity.rs` around lines 147 - 161, The current
loop over updates aborts on the first write error (via
FactoryUpdateIdentityError) leaving a partially-applied workspace and never
reloading the identity; change it to best-effort: iterate all (filename,
content) in updates, on each write attempt push filename into files_updated on
success or push (filename, error) into files_failed on failure (do not
early-return), then after the loop if files_updated is non-empty call
crate::identity::Identity::load(workspace).await and, if
runtime_configs.load().get(&agent_id) is Some, call
runtime_config.identity.store(Arc::new(identity)); finally return a structured
result (e.g., a new enum/struct containing files_updated and files_failed)
instead of returning FactoryUpdateIdentityError on the first failure so callers
can recover per-file.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
interface/src/routes/AgentConfig.tsx (1)
18-18: Remove"user"fromSectionIdtype for consistency.The
SectionIdunion still includes"user", but theSECTIONSarray no longer has a "user" entry (it was removed as part of USER.md deprecation). This creates a type that accepts a value that will never actually be used.♻️ Suggested fix
-type SectionId = "general" | "soul" | "identity" | "user" | "routing" | "tuning" | "compaction" | "cortex" | "coalesce" | "memory" | "browser" | "channel" | "sandbox"; +type SectionId = "general" | "soul" | "identity" | "routing" | "tuning" | "compaction" | "cortex" | "coalesce" | "memory" | "browser" | "channel" | "sandbox";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentConfig.tsx` at line 18, The SectionId union still includes "user" even though SECTIONS no longer contains a "user" entry; remove "user" from the SectionId type definition so the type matches the actual SECTIONS array. Update the SectionId declaration (the type named SectionId) to exclude "user" and ensure any usages of SectionId elsewhere are still valid against the revised union; check references to SectionId and SECTIONS to ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@interface/src/routes/AgentConfig.tsx`:
- Line 18: The SectionId union still includes "user" even though SECTIONS no
longer contains a "user" entry; remove "user" from the SectionId type definition
so the type matches the actual SECTIONS array. Update the SectionId declaration
(the type named SectionId) to exclude "user" and ensure any usages of SectionId
elsewhere are still valid against the revised union; check references to
SectionId and SECTIONS to ensure consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1f86d71-5d17-46c8-8673-92b47e1b070a
📒 Files selected for processing (2)
interface/src/api/client.tsinterface/src/routes/AgentConfig.tsx
# Conflicts: # prompts/en/tools/file_description.md.j2 # src/api/server.rs # src/prompts/text.rs # src/tools.rs # src/tools/file.rs
…nd UI Move identity files (SOUL.md, IDENTITY.md, ROLE.md) from workspace/ to the agent root directory so sandbox containment naturally prevents worker file tools from accessing them. Remove check_identity_protection() and PROTECTED_FILES from file.rs. Add identity_dir to ResolvedAgentConfig, RuntimeConfig, and ApiState. Update all callers in main.rs, agents.rs, watcher.rs, and factory tools. Expose ROLE.md through the identity API (get/update handlers) and the dashboard UI (AgentConfig editor, AgentDetail preview). Replace deprecated USER.md references with ROLE.md across docs and prompts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/agents.rs (1)
536-541:⚠️ Potential issue | 🟠 MajorAgent creation is still non-atomic after the config entry is written.
From the first config write onward, there are many fallible provisioning steps, and
agent_tx.send(...)is only logged on failure. That means a request can fail after persisting[[agents]], or even return success when the main loop never registered the agent, leaving config, disk, and live state out of sync. Either commit the config change last, or add rollback and treat main-loop registration failure as fatal.Also applies to: 874-876
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 536 - 541, The current flow writes the agent entry to disk via tokio::fs::write(&config_path, ...) before performing follow-up provisioning and before notifying the main loop (agent_tx.send(...)), which can leave the config committed while registration/provisioning fails; change the flow so the config write is performed last (only after provisioning and a successful agent_tx.send/registration) or implement a rollback path that removes the just-written [[agents]] entry on any subsequent failure and treat a failed main-loop registration (agent_tx.send) as fatal; update both the write site around config_path/tokio::fs::write and the other occurrence at the lines referenced (agent_tx.send locations) to either defer the disk write until after successful main-loop registration or ensure a compensating delete of the config entry and cleanup on any provisioning or send failure.src/config/watcher.rs (1)
16-25: 🛠️ Refactor suggestion | 🟠 MajorReplace the positional
agentstuple before merge.Lines 19-25 are already tripping
cargo clippy --all-targetswithtype_complexity, and the addedidentity_dirslot means the twoPathBuffields can now be swapped accidentally without the compiler noticing. A small named struct here would unblock CI and make the later destructuring much safer.♻️ One simple direction
+struct WatchedAgent { + agent_id: String, + workspace: PathBuf, + identity_dir: PathBuf, + runtime_config: Arc<RuntimeConfig>, + mcp_manager: Arc<crate::mcp::McpManager>, +} + pub fn spawn_file_watcher( config_path: PathBuf, instance_dir: PathBuf, - agents: Vec<( - String, - PathBuf, - PathBuf, - Arc<RuntimeConfig>, - Arc<crate::mcp::McpManager>, - )>, + agents: Vec<WatchedAgent>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/watcher.rs` around lines 16 - 25, Replace the positional tuple type used for `agents` in `spawn_file_watcher` with a small named struct to avoid `type_complexity` and accidental field swaps: define e.g. an `AgentConfig` struct with named fields (name: String, agent_dir: PathBuf, identity_dir: PathBuf, runtime_config: Arc<RuntimeConfig>, mcp_manager: Arc<crate::mcp::McpManager>), change the `agents: Vec<(String, PathBuf, PathBuf, Arc<RuntimeConfig>, Arc<crate::mcp::McpManager>)>` parameter to `agents: Vec<AgentConfig>`, update all destructuring/usages inside `spawn_file_watcher` to use the named fields, and fix any call sites that construct the tuple to build `AgentConfig` instances instead.
♻️ Duplicate comments (9)
src/api/agents.rs (3)
431-433:⚠️ Potential issue | 🟠 MajorWarmup still drops org humans context.
This ad-hoc
AgentDepssnapshot uses an emptyhumanslist, so API-triggered warmups run with different org context than the live agent path. Load the currentstate.agent_humanssnapshot here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 431 - 433, The warmup path creates an AgentDeps snapshot with an empty humans list, causing org context divergence; update the AgentDeps construction to load the current state.agent_humans snapshot into the humans field instead of ArcSwap::from_pointee(Vec::new()). Locate where AgentDeps is built (the AgentDeps struct instantiation with fields links, agent_names, humans) and replace the empty humans initialization with the live snapshot from state.agent_humans (wrapped in the same Arc/arc_swap::ArcSwap pattern used for links/humans) so API-triggered warmups use the same org humans context as the live agent path.
497-541:⚠️ Potential issue | 🟠 MajorTake
config_write_mutexaround this config.toml read-modify-write.This helper still does an unlocked read/parse/modify/write cycle, so concurrent create/update/delete requests can clobber each other's changes even though
ApiStatenow exposes a shared mutex for exactly this race.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 497 - 541, Wrap the entire read-parse-modify-write sequence with the shared config write mutex on ApiState: acquire state.config_write_mutex.lock().await before reading config_path and hold it until after the tokio::fs::write completes, then release; specifically enclose the code that reads config_path, parses into doc, modifies doc/agents_array/new_table (using agent_id and request fields), and writes doc back to disk so concurrent create/update/delete requests cannot clobber each other.
457-467:⚠️ Potential issue | 🟠 MajorPreserve non-2xx status codes for create failures.
The error branch still wraps failures in
Ok(Json(...)), so validation and server-side errors come back as HTTP 200. That makes API clients and monitoring treat failed creates as success.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 457 - 467, The handler currently wraps all failures in Ok(Json(...)) which returns HTTP 200; update the match on create_agent_internal(&state, request).await to return an Err variant with an appropriate non-2xx status and JSON body instead of Ok: change the handler's return type to Result<Json<...>, (StatusCode, Json<...>)> (or your framework's equivalent) and in the Err(message) arm return Err((StatusCode::BAD_REQUEST, Json({ "success": false, "message": message }))) or map different errors from create_agent_internal to specific status codes (e.g., validation -> 400, internal -> 500) so failures preserve their intended HTTP status codes.src/tools/factory_update_identity.rs (1)
147-161:⚠️ Potential issue | 🟠 MajorMake the identity write batch best-effort instead of fail-fast.
If one file write succeeds and a later one fails, this returns early after partially mutating the agent on disk and skips the reload step. Please return per-file success/failure details and reload whenever any write succeeds so the LLM can recover cleanly from partial failures.
Based on learnings, "Tool errors are returned as structured results, not panics. The LLM sees the error and can recover (error-as-result for tools pattern)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/factory_update_identity.rs` around lines 147 - 161, The current loop in the update routine (iterating over updates, writing to identity_dir, pushing to files_updated, and returning FactoryUpdateIdentityError on the first failed tokio::fs::write) should be changed to a best-effort batch: for each (filename, content) record a per-file Result (success or error string) instead of early-returning on write failure, continue writing remaining files, and only call Identity::load and runtime_config.identity.store(Arc::new(identity)) if at least one write succeeded; return a structured result containing per-file statuses and an overall indicator rather than a single error (replace the panic/early-return behavior around FactoryUpdateIdentityError with collected results).src/tools/file.rs (1)
93-97:⚠️ Potential issue | 🟠 MajorKeep an explicit identity/memory write guard.
The assumption in Lines 93-97 only holds while sandboxing is enabled. Lines 50-52 return any canonical path when sandbox mode is off, so
file_writeandfile_editcan still overwriteSOUL.md/IDENTITY.md/ROLE.mddirectly instead of returning a tool-directed error. That also means the new tests/docs now describe a stronger isolation guarantee than the code actually enforces.At minimum, reintroduce a protected-write check after
resolve_path()for identity/memory targets in the write/edit tools, even when sandboxing is disabled. As per coding guidelines, "File tools must reject writes to identity/memory paths with an error directing the LLM to the correct tool (workspace path guard pattern)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/file.rs` around lines 93 - 97, Reintroduce an explicit post-resolution write guard in the file write/edit tools: after calling resolve_path() inside file_write and file_edit, check the resolved path against the identity/memory filenames (e.g., SOUL.md, IDENTITY.md, ROLE.md or the PROTECTED_FILES set) and reject any write/edit attempts with an error that directs the LLM to use the appropriate identity/memory tool (follow the workspace path guard pattern); keep this check even when sandboxing is disabled (i.e., do not rely solely on resolve_path() behavior), and return the same tool-directed error used by other file tools for protected-write attempts.src/agent/channel.rs (1)
1813-1832:⚠️ Potential issue | 🟡 MinorDerive
is_humanfromhumans_by_id.Line 1813 still infers human-ness from
agent_names, while Lines 1815-1832 source the actual metadata fromhumans_by_id. If a linked ID is missing fromagent_names, this can emitis_human = truebut still take the agent fallback path, so the org-context payload becomes internally inconsistent.🩹 Minimal fix
- let is_human = !self.deps.agent_names.contains_key(other_id.as_str()); - - let (name, role, description) = if let Some(human) = humans_by_id.get(other_id.as_str()) + let human = humans_by_id.get(other_id.as_str()); + let is_human = human.is_some(); + + let (name, role, description) = if let Some(human) = human🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 1813 - 1832, The is_human flag is derived from agent_names but the human metadata is read from humans_by_id, causing inconsistencies; change the computation of is_human to check humans_by_id.contains_key(other_id.as_str()) instead of self.deps.agent_names.contains_key(...), so the boolean aligns with the later human/agent branch (update the binding where is_human is declared near the block that builds (name, role, description) so both the flag and the selected metadata come from the same humans_by_id source).src/main.rs (2)
3192-3209:⚠️ Potential issue | 🟠 MajorOnly enable factory mode when tool registration succeeds.
with_factory(true)still runs even afteradd_factory_tools(...)logs an error, so Cortex chat can advertise factory flows while some or all factory tools are unavailable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 3192 - 3209, The Cortex chat session unconditionally enables factory mode via CortexChatSession::with_factory(true) even when spacebot::tools::add_factory_tools(...) failed; change this by storing the result of add_factory_tools and only calling .with_factory(true) when that call returns Ok — e.g., assign a boolean like factory_enabled based on the add_factory_tools Result (or pattern-match Err/Ok), and then construct the session with .with_factory(factory_enabled) so factory mode is only advertised when add_factory_tools succeeded for the given tool_server/agent.
1463-1464:⚠️ Potential issue | 🟠 Major
agent_humansstill won't hot-reload.This
ArcSwapis only cloned intoAgentDeps;spawn_file_watcher(...)still never receives it in this file, so edits toconfig.humansremain stale in running agents and factory validation until restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1463 - 1464, agent_humans is wrapped in an ArcSwap but only wired into AgentDeps, so the file watcher never sees updates; pass the same ArcSwap instance into spawn_file_watcher so it can observe hot-reloads and ensure AgentDeps uses Arc::clone(&agent_humans) (not a separate copy of the inner Vec) so both AgentDeps and spawn_file_watcher share the identical ArcSwap<...> for config.humans updates; update the call site that constructs AgentDeps and the spawn_file_watcher invocation to accept and forward agent_humans.src/tools/factory_create_agent.rs (1)
294-329:⚠️ Potential issue | 🟠 MajorLink creation still has a check-then-write race.
Duplicate detection happens before
config_write_mutex, and the lateragent_linksread-clone-store update is another unsynchronized window. Concurrent factory calls can still append duplicate links toconfig.tomlor lose one in-memory update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/factory_create_agent.rs` around lines 294 - 329, The duplicate-check/write/update must be done under the same config write lock to avoid the check-then-write race: acquire the shared mutex (e.g. self.state.config_write_mutex) before calling self.state.agent_links.load(), perform the duplicate detection there, if not duplicate call write_link_to_config while still holding the lock, then update the in-memory links (clone, push new AgentLink, call self.state.set_agent_links) before releasing the lock; ensure all paths that mutate agent_links use this same mutex so concurrent factory_create_agent calls serialize the check/write/update sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/docs/`(core)/agents.mdx:
- Around line 16-17: The docs mention "prompt overrides" as part of the agent
workspace but that feature doesn't exist; remove the phrase "prompt overrides"
from the bullet describing the workspace (the string `sandboxed directory
(~/.spacebot/agents/{id}/workspace/) for working files, ingest, and prompt
overrides`) and update the sentence to list only supported items (e.g., "working
files, ingest, and skills" or simply "working files and ingest") to match the
rest of the docs and config.mdx where system prompts are compile-time assets.
In `@src/identity/files.rs`:
- Around line 28-32: Identity::load currently treats any read error as “missing”
by calling load_optional_file and turning failures into None; change it to
surface real I/O errors instead: update load_optional_file (or create
load_optional_file_result) to return Result<Option<String>, std::io::Error> and
change Identity::load to return Result<Self, std::io::Error> (or another
appropriate error type), calling
load_optional_file_result(&identity_dir.join("SOUL.md")),
load_optional_file_result(&identity_dir.join("IDENTITY.md")), and
load_optional_file_result(&identity_dir.join("ROLE.md")) and propagate errors
with ? so permission/encoding I/O errors are returned rather than silently
collapsed to None; keep the Option semantics only for genuine “file not found”
cases.
In `@src/tools/factory_create_agent.rs`:
- Around line 166-215: The call method currently returns
Err(FactoryCreateAgentError(...)) for validation and creation failures; instead
return a structured FactoryCreateAgentOutput with success: false and an
explanatory message so the LLM can recover. Replace each early return that uses
FactoryCreateAgentError (after agent_id empty check, validate_agent_id failure,
empty content checks for soul_content/identity_content/role_content, and when
agent already exists via self.state.agent_configs.load()) with
Ok(FactoryCreateAgentOutput { success: false, message: "<clear reason>" .into(),
..Default::default() or appropriate fields }). Also change the
create_agent_internal failure handling (both map_err result and
!create_result.success) to return Ok(FactoryCreateAgentOutput { success: false,
message: format!("agent creation failed: ..."), ... }) rather than Err(...);
keep using Err only for truly unexpected/internal errors that should surface as
FactoryCreateAgentError. Ensure you reference the types FactoryCreateAgentOutput
and function create_agent_internal when making these replacements.
---
Outside diff comments:
In `@src/api/agents.rs`:
- Around line 536-541: The current flow writes the agent entry to disk via
tokio::fs::write(&config_path, ...) before performing follow-up provisioning and
before notifying the main loop (agent_tx.send(...)), which can leave the config
committed while registration/provisioning fails; change the flow so the config
write is performed last (only after provisioning and a successful
agent_tx.send/registration) or implement a rollback path that removes the
just-written [[agents]] entry on any subsequent failure and treat a failed
main-loop registration (agent_tx.send) as fatal; update both the write site
around config_path/tokio::fs::write and the other occurrence at the lines
referenced (agent_tx.send locations) to either defer the disk write until after
successful main-loop registration or ensure a compensating delete of the config
entry and cleanup on any provisioning or send failure.
In `@src/config/watcher.rs`:
- Around line 16-25: Replace the positional tuple type used for `agents` in
`spawn_file_watcher` with a small named struct to avoid `type_complexity` and
accidental field swaps: define e.g. an `AgentConfig` struct with named fields
(name: String, agent_dir: PathBuf, identity_dir: PathBuf, runtime_config:
Arc<RuntimeConfig>, mcp_manager: Arc<crate::mcp::McpManager>), change the
`agents: Vec<(String, PathBuf, PathBuf, Arc<RuntimeConfig>,
Arc<crate::mcp::McpManager>)>` parameter to `agents: Vec<AgentConfig>`, update
all destructuring/usages inside `spawn_file_watcher` to use the named fields,
and fix any call sites that construct the tuple to build `AgentConfig` instances
instead.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 1813-1832: The is_human flag is derived from agent_names but the
human metadata is read from humans_by_id, causing inconsistencies; change the
computation of is_human to check humans_by_id.contains_key(other_id.as_str())
instead of self.deps.agent_names.contains_key(...), so the boolean aligns with
the later human/agent branch (update the binding where is_human is declared near
the block that builds (name, role, description) so both the flag and the
selected metadata come from the same humans_by_id source).
In `@src/api/agents.rs`:
- Around line 431-433: The warmup path creates an AgentDeps snapshot with an
empty humans list, causing org context divergence; update the AgentDeps
construction to load the current state.agent_humans snapshot into the humans
field instead of ArcSwap::from_pointee(Vec::new()). Locate where AgentDeps is
built (the AgentDeps struct instantiation with fields links, agent_names,
humans) and replace the empty humans initialization with the live snapshot from
state.agent_humans (wrapped in the same Arc/arc_swap::ArcSwap pattern used for
links/humans) so API-triggered warmups use the same org humans context as the
live agent path.
- Around line 497-541: Wrap the entire read-parse-modify-write sequence with the
shared config write mutex on ApiState: acquire
state.config_write_mutex.lock().await before reading config_path and hold it
until after the tokio::fs::write completes, then release; specifically enclose
the code that reads config_path, parses into doc, modifies
doc/agents_array/new_table (using agent_id and request fields), and writes doc
back to disk so concurrent create/update/delete requests cannot clobber each
other.
- Around line 457-467: The handler currently wraps all failures in Ok(Json(...))
which returns HTTP 200; update the match on create_agent_internal(&state,
request).await to return an Err variant with an appropriate non-2xx status and
JSON body instead of Ok: change the handler's return type to Result<Json<...>,
(StatusCode, Json<...>)> (or your framework's equivalent) and in the
Err(message) arm return Err((StatusCode::BAD_REQUEST, Json({ "success": false,
"message": message }))) or map different errors from create_agent_internal to
specific status codes (e.g., validation -> 400, internal -> 500) so failures
preserve their intended HTTP status codes.
In `@src/main.rs`:
- Around line 3192-3209: The Cortex chat session unconditionally enables factory
mode via CortexChatSession::with_factory(true) even when
spacebot::tools::add_factory_tools(...) failed; change this by storing the
result of add_factory_tools and only calling .with_factory(true) when that call
returns Ok — e.g., assign a boolean like factory_enabled based on the
add_factory_tools Result (or pattern-match Err/Ok), and then construct the
session with .with_factory(factory_enabled) so factory mode is only advertised
when add_factory_tools succeeded for the given tool_server/agent.
- Around line 1463-1464: agent_humans is wrapped in an ArcSwap but only wired
into AgentDeps, so the file watcher never sees updates; pass the same ArcSwap
instance into spawn_file_watcher so it can observe hot-reloads and ensure
AgentDeps uses Arc::clone(&agent_humans) (not a separate copy of the inner Vec)
so both AgentDeps and spawn_file_watcher share the identical ArcSwap<...> for
config.humans updates; update the call site that constructs AgentDeps and the
spawn_file_watcher invocation to accept and forward agent_humans.
In `@src/tools/factory_create_agent.rs`:
- Around line 294-329: The duplicate-check/write/update must be done under the
same config write lock to avoid the check-then-write race: acquire the shared
mutex (e.g. self.state.config_write_mutex) before calling
self.state.agent_links.load(), perform the duplicate detection there, if not
duplicate call write_link_to_config while still holding the lock, then update
the in-memory links (clone, push new AgentLink, call self.state.set_agent_links)
before releasing the lock; ensure all paths that mutate agent_links use this
same mutex so concurrent factory_create_agent calls serialize the
check/write/update sequence.
In `@src/tools/factory_update_identity.rs`:
- Around line 147-161: The current loop in the update routine (iterating over
updates, writing to identity_dir, pushing to files_updated, and returning
FactoryUpdateIdentityError on the first failed tokio::fs::write) should be
changed to a best-effort batch: for each (filename, content) record a per-file
Result (success or error string) instead of early-returning on write failure,
continue writing remaining files, and only call Identity::load and
runtime_config.identity.store(Arc::new(identity)) if at least one write
succeeded; return a structured result containing per-file statuses and an
overall indicator rather than a single error (replace the panic/early-return
behavior around FactoryUpdateIdentityError with collected results).
In `@src/tools/file.rs`:
- Around line 93-97: Reintroduce an explicit post-resolution write guard in the
file write/edit tools: after calling resolve_path() inside file_write and
file_edit, check the resolved path against the identity/memory filenames (e.g.,
SOUL.md, IDENTITY.md, ROLE.md or the PROTECTED_FILES set) and reject any
write/edit attempts with an error that directs the LLM to use the appropriate
identity/memory tool (follow the workspace path guard pattern); keep this check
even when sandboxing is disabled (i.e., do not rely solely on resolve_path()
behavior), and return the same tool-directed error used by other file tools for
protected-write attempts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd891e28-5d88-4d0b-bd8f-882d1137c6c6
📒 Files selected for processing (44)
docs/content/docs/(configuration)/config.mdxdocs/content/docs/(configuration)/permissions.mdxdocs/content/docs/(configuration)/sandbox.mdxdocs/content/docs/(core)/agents.mdxdocs/content/docs/(core)/architecture.mdxdocs/content/docs/(core)/cortex.mdxdocs/content/docs/(core)/memory.mdxdocs/content/docs/(core)/prompts.mdxdocs/content/docs/(deployment)/roadmap.mdxdocs/content/docs/(features)/tools.mdxdocs/content/docs/(features)/workers.mdxdocs/content/docs/(getting-started)/docker.mdxdocs/content/docs/(getting-started)/quickstart.mdxdocs/design-docs/channel-attachment-persistence.mddocs/design-docs/multi-agent-communication-graph.mddocs/design-docs/sandbox.mddocs/docker.mdinterface/src/api/client.tsinterface/src/routes/AgentConfig.tsxinterface/src/routes/AgentDetail.tsxprompts/en/tools/file_write_description.md.j2prompts/en/worker.md.j2src/agent/channel.rssrc/agent/cortex_chat.rssrc/api.rssrc/api/agents.rssrc/api/server.rssrc/api/state.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rssrc/identity/files.rssrc/lib.rssrc/main.rssrc/prompts/engine.rssrc/prompts/text.rssrc/tools.rssrc/tools/factory_create_agent.rssrc/tools/factory_update_identity.rssrc/tools/file.rstests/bulletin.rstests/context_dump.rs
✅ Files skipped from review due to trivial changes (2)
- docs/design-docs/multi-agent-communication-graph.md
- docs/content/docs/(core)/cortex.mdx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/config/toml_schema.rs
- prompts/en/worker.md.j2
- src/prompts/text.rs
- src/agent/cortex_chat.rs
- src/config/load.rs
- src/api/server.rs
- interface/src/api/client.ts
Comment on lines
+16
to
+17
| - **Identity files** — `SOUL.md`, `IDENTITY.md`, and `ROLE.md` in the agent root directory (`~/.spacebot/agents/{id}/`) | ||
| - **A workspace** — sandboxed directory (`~/.spacebot/agents/{id}/workspace/`) for working files, ingest, and prompt overrides; this is the boundary for worker file tools |
Contributor
There was a problem hiding this comment.
Remove prompt overrides here unless that feature actually exists.
The rest of the docs in this PR still describe workspace/ as holding working files, skills/, and ingest/, and config.mdx says system prompts are compile-time assets. Mentioning prompt overrides here documents a workspace capability users don't appear to have.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/docs/`(core)/agents.mdx around lines 16 - 17, The docs mention
"prompt overrides" as part of the agent workspace but that feature doesn't
exist; remove the phrase "prompt overrides" from the bullet describing the
workspace (the string `sandboxed directory (~/.spacebot/agents/{id}/workspace/)
for working files, ingest, and prompt overrides`) and update the sentence to
list only supported items (e.g., "working files, ingest, and skills" or simply
"working files and ingest") to match the rest of the docs and config.mdx where
system prompts are compile-time assets.
…load, UI - Return proper HTTP status codes from create_agent (201/400/409/500) - Add config_write_mutex to create/update/delete agent API handlers - Add config_write_mutex to api/config.rs update_agent_config handler - Move duplicate-link check inside write_link_to_config mutex guard - Surface hot-reload failures via reload_warning in factory_update_config - Gate with_factory(true) on add_factory_tools success - Wire agent_humans into file-watcher for hot-reload on config change - Store new_config.humans before initialize_agents in provider-setup - Fix warmup handler using empty humans vec instead of state.agent_humans - Derive is_human from humans_by_id instead of negating agent_names - Wrap org_context human descriptions in <context> tags - Redesign HumanEditDialog as two-column modal with markdown editor - Use main-agent preset content for default identity file scaffolding
…t agent Fresh instances now get a link from admin → main agent (direction: down, kind: hierarchical) so the agent sees the human's description in its system prompt via org_context. Applied in both load_from_env and from_toml config paths.
…irection Add discord_id, telegram_id, slack_id, email fields to HumanDef for correlating inbound messages to humans. Wired through TOML schema, config loading, API create/update handlers, TS types, and the HumanEditDialog UI (conditionally shown based on messaging status). Fix default admin→main link using invalid direction 'down' — changed to 'one_way'. Improve HumanEditDialog modal sizing and layout.
…s, and UI polish - Add shared ProfileAvatar component with gradient + initials + image support - Add gradient_start/gradient_end fields to AgentConfig and API layer - Add avatar upload/serve/delete endpoints with agent_data_dirs on ApiState - Wire gradient and avatar into topology graph nodes and sidebar - Add gradient color picker with presets to agent General tab - Persist cortex chat tool calls (id, tool, args, result, status) alongside assistant messages via new migration and CortexChatToolCall type - Stream richer tool call events (ToolStarted/ToolCompleted with call_id, args, result) and accumulate for Done event - Show tool call details in CortexChatPanel UI - Show 'This is you, add your details.' prompt on empty human nodes - Fix collapsible-if clippy lint in default admin link creation - Extract WatchedAgent type alias to fix type-complexity clippy lint - Fix cargo fmt on agent_data_dirs.store() calls
ProfileAvatar now catches img onError (e.g. 404 when no avatar is uploaded) and falls back to the gradient + initials SVG instead of showing a broken image icon.
…I fixes - Rewrite CreateAgentDialog with preset card grid and embedded cortex chat - Add factory presets API client (factoryPresets, PresetMeta types) - Add freshThread option to useCortexChat to skip loading history - Add initialPrompt and hideHeader props to CortexChatPanel - Fix chat scroll with min-h-0 on messages container - Fix dialog layout with !important overrides on flex/gap/padding - Pass agentId to CreateAgentDialog from Sidebar and Overview - Remove empty state from AgentTasks, always show kanban board - Add skills_search tool to cortex chat tool server - Add integration setup docs and cortex chat prompt guidance - Change user message bubble color to neutral bg-app-hover/30
- New install_skill tool lets cortex install skills directly from skills.sh (calls install_from_github, reloads RuntimeConfig immediately) - Add rule 7 to cortex prompt: always call skills_search, never guess repos - Update factory flow step 9: search/install skills after creating agents - Update integration setup step 3: reference install_skill for direct install
- replace empty dashboard with welcome card, provider CTA, and cortex pro tip when no LLM configured - hide new agent buttons when no provider is set - remove global SetupBanner (redundant with welcome card) - move human descriptions from config.toml to HUMAN.md files on disk - add install_skill cross-agent targeting via agent_id param - background project auto-discovery to avoid blocking API response - write skeleton config.toml during UI-only onboarding flow - materialize in-memory humans into TOML before config writes
… completion - DetachedSpawnWorkerTool for cortex chat sessions (no parent channel required) - Thread list/delete APIs and UI (popover with history) - Auto-trigger follow-up cortex turns when spawned workers complete - CortexChatUpdate event pipeline through SSE to frontend - Relaxed loop guard thresholds for cortex (matches branch config) - install_skill now accepts agent_id to target other agents - Skill installer requires three-part owner/repo/skill format - Cortex chat panel padding and UI polish
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Adds the Agent Factory — a conversational system for creating, configuring, and refining agents through presets and LLM-callable tools. Instead of manually writing identity files and editing config.toml, the admin describes what they want in natural language via cortex chat, and the factory synthesizes a fully configured agent from presets, organizational memory, and user preferences.
HumanDef.descriptionand is inherited by linked agentsDesign
Full design doc:
docs/design-docs/agent-factory.md(6 phases, 4 skipped structured message types in favor of natural language conversation)What changed
Phase 0: Deprecate USER.md
HumanDefgainsdescription: Option<String>(config types, toml schema, load)Identitydropsuserfield; file watcher stops watching USER.mdbuild_org_context()rewritten to render human roles + descriptions from the org graphPhase 1: Preset System
presets/— 9 directories withmeta.toml,SOUL.md,IDENTITY.md,ROLE.mdPresetRegistryinsrc/factory/presets.rsusingrust-embedGET /api/factory/presets,GET /api/factory/presets/:idPhase 2: Factory Tools
factory_list_presets/factory_load_preset— browse and load preset archetypesfactory_search_context— hybrid search the main agent's memories for org contextfactory_create_agent— full lifecycle: create viacreate_agent_internal(), write identity files, create org links, reload identityfactory_update_identity/factory_update_config— refinement after creationcreate_agent_internal()from the API handler for reusePhase 3: Factory Prompt
prompts/en/factory.md.j2— 166-line system prompt with creation flow, soul writing guide, synthesis rulesPhase 5: Cortex Integration
add_factory_tools()factory_enabledfield onCortexChatSessiongates the prompt sectionPhase 6: Polish
one_way_from,manager, etc.) that the parser rejected; aligned to actual accepted valuesconfig_write_mutexonApiStatefor safe read-modify-write cycles on config.tomlTesting
cargo checkcleantests/bulletin.rs(migration mismatch, unrelated)Note
This PR introduces a comprehensive agent factory system enabling natural language agent configuration. Core additions include 9 preset archetypes with embedded configuration, 6 LLM-callable factory tools for browsing and creating agents, cortex chat integration with guided creation flows, and validation across agent IDs, identity content, routing configs, and search parameters. User context has been moved from USER.md to the organizational graph. All tests pass with clean compilation.
Written by Tembo for commit 3f354c0.