Conversation
Summary of ChangesHello @Grinsven, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's capabilities by integrating a robust multi-agent orchestration framework. It allows users to define and interact with specialized AI agents through intuitive TUI commands and Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Grinsven
force-pushed
the
agents-multi-final
branch
from
8ca1013 to
1dfb5a0
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by adding multi-agent tool support. The changes are extensive, touching core logic, the TUI, and documentation. The implementation of the agent registry, tool handler, and TUI components appears robust and well-designed. The path validation for agent prompts is a good security measure. The documentation is comprehensive and will be very helpful for users. I've found a couple of areas for improvement: one related to improving platform compatibility for locating the home directory, and another to address a potential correctness issue with concurrent agent calls.
| if item_id.is_empty() { | ||
| item_id = "agent-call".to_string(); | ||
| } |
There was a problem hiding this comment.
Using a hardcoded string "agent-call" as a fallback for an empty item_id could lead to issues if multiple agent tool calls with empty call_ids are processed concurrently. This would result in multiple agent executions sharing the same item_id, which could cause confusion in clients trying to track them. A unique ID should be generated instead. You can use the rand crate for this, which is already a dependency. You'll need to add use rand::Rng; at the top of the file.
| if item_id.is_empty() { | |
| item_id = "agent-call".to_string(); | |
| } | |
| if item_id.is_empty() { | |
| let random_id: u64 = rand::thread_rng().gen(); | |
| item_id = format!("agent-call-{}", random_id); | |
| } |
| fn get_agents_directory() -> Option<PathBuf> { | ||
| std::env::var("HOME") | ||
| .or_else(|_| std::env::var("USERPROFILE")) | ||
| .ok() | ||
| .map(|home| PathBuf::from(home).join(".codex")) | ||
| } |
There was a problem hiding this comment.
The current implementation for finding the home directory is platform-specific and less robust than using the dirs crate, which is already a dependency in this file. Using dirs::home_dir() provides a more reliable and cross-platform way to locate the user's home directory.
fn get_agents_directory() -> Option<PathBuf> {
dirs::home_dir().map(|home| home.join(".codex"))
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for a multi-agent system. The changes are well-structured, spanning from core logic for agent registration and execution to TUI enhancements for user interaction, including a new /agents command and @mention support. The new agent tool is cleanly integrated into the existing tool handling infrastructure. I'm particularly impressed with the thoroughness of the implementation, which includes security considerations like path traversal prevention for agent prompts and robust error handling. The accompanying documentation is extensive and provides clear guidance for users.
I have one minor suggestion to improve code consistency in the new agent.rs module. Overall, this is an excellent addition to the project.
| fn get_agents_directory() -> Option<PathBuf> { | ||
| std::env::var("HOME") | ||
| .or_else(|_| std::env::var("USERPROFILE")) | ||
| .ok() | ||
| .map(|home| PathBuf::from(home).join(".codex")) | ||
| } |
There was a problem hiding this comment.
For consistency and robustness, consider using dirs::home_dir() to determine the home directory, as is done elsewhere in this file (e.g., in validate_prompt_path). The dirs crate is designed to handle cross-platform differences and edge cases for locating user directories.
fn get_agents_directory() -> Option<PathBuf> {
dirs::home_dir().map(|home| home.join(".codex"))
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for a multi-agent system, including core logic for an agent registry, a tool handler for invoking agents, and TUI integration for listing agents and using @mentions. The changes are well-documented with a new subagents.md guide and example configurations. My review found a high-severity security vulnerability related to path traversal and a medium-severity bug in the TUI's agent mention parsing. I've provided suggestions to address both issues.
|
|
||
| // Try to load user agents from ~/.codex/agents.toml | ||
| let agents_dir = Self::get_agents_directory(); | ||
| if let Some(ref dir) = agents_dir { |
There was a problem hiding this comment.
There is a path traversal vulnerability here. If a user creates a symbolic link from ~/.codex to a directory like /, the validate_prompt_path function can be tricked into allowing access to any file on the system (e.g., /etc/passwd). This is because canonicalize() resolves the symlink, and the subsequent starts_with check will pass.
To mitigate this, I recommend disallowing the agents directory from being a symlink altogether. This is a safer approach for a security-sensitive operation like loading agent prompts.
if let Some(ref dir) = agents_dir {
if dir.symlink_metadata().map_or(false, |m| m.is_symlink()) {
tracing::error!(
"Security: agents directory at '{}' is a symlink, which is not allowed. Aborting loading of user-defined agents.",
dir.display()
);
return Ok(Self { agents, agents_dir });
}| // Compile regex once at startup | ||
| #[allow(clippy::expect_used)] | ||
| static AGENT_MENTION_RE: Lazy<Regex> = Lazy::new(|| { | ||
| Regex::new(r"@(\w+):?\s+([^\n@]+)") |
There was a problem hiding this comment.
The regex for parsing agent mentions uses \w+ for the agent name, which only matches alphanumeric characters and underscores. However, TOML keys (used for agent names in agents.toml) can contain hyphens. The documentation in docs/subagents.md even provides code-reviewer as an example agent name.
This regex will fail to correctly parse mentions for agents with hyphens in their names. To fix this, the character class for the agent name should be updated to include hyphens.
| Regex::new(r"@(\w+):?\s+([^\n@]+)") | |
| Regex::new(r"@([\w-]+):?\s+([^\n@]+)") |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by adding multi-agent support. The changes include core logic for an agent registry, configuration handling, and a tool handler for invoking agents, complete with security measures to prevent path traversal and agent recursion. The TUI is also enhanced with support for agent mentions (@agent: task) and a new /agents command. The documentation has been extensively updated to reflect these new features. My review identifies a critical discrepancy where the implementation prevents sub-agents from using any tools, which contradicts the documentation. The provided feedback aims to rectify this by enabling tool inheritance for agents, which is crucial for the feature's usability.
| let prompt = Prompt { | ||
| input: vec![ResponseItem::Message { | ||
| id: None, | ||
| role: "user".to_string(), | ||
| content: vec![ContentItem::InputText { text: task_text }], | ||
| }], | ||
| tools: Vec::new(), | ||
| parallel_tool_calls: false, | ||
| base_instructions_override: Some(instructions), | ||
| output_schema: None, | ||
| }; |
There was a problem hiding this comment.
Sub-agents are currently unable to use any tools as an empty tool list is passed to their prompt. This contradicts the documentation which states that agents should inherit tools from the parent context.
To enable tool inheritance, you can reconstruct the toolset for the sub-agent, making sure to exclude the agent tool to prevent recursion.
let mcp_tools = session
.services
.mcp_connection_manager
.read()
.await
.list_all_tools()
.await
.map_err(|e| FunctionCallError::Fatal(e.to_string()))?
.into_iter()
.map(|(name, tool)| (name, tool.tool))
.collect();
let mut sub_tools_config = turn.tools_config.clone();
sub_tools_config.include_agent_tool = false;
let agent_tools = crate::tools::spec::build_specs(&sub_tools_config, Some(mcp_tools))
.build()
.specs()
.to_vec();
let prompt = Prompt {
input: vec![ResponseItem::Message {
id: None,
role: "user".to_string(),
content: vec![ContentItem::InputText { text: task_text }],
}],
tools: agent_tools,
parallel_tool_calls: false,
base_instructions_override: Some(instructions),
output_schema: None,
};
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by adding multi-agent support. The changes are comprehensive, covering core logic, TUI integration, and extensive documentation. The implementation of the agent registry and tool handlers is well-structured. I've identified a critical compilation error and a couple of areas where the logic for security and recursion prevention could be clarified and strengthened. Overall, this is a solid contribution that greatly expands the capabilities of the system.
| let mut message = String::new(); | ||
| let mut item_id = call_id.clone(); | ||
| if item_id.is_empty() { | ||
| let random_id: u64 = rand::rng().random(); |
There was a problem hiding this comment.
This line will cause a compilation error because rand::rng() is not a valid function call. You likely intended to use rand::thread_rng() to get a thread-local random number generator.
| let random_id: u64 = rand::rng().random(); | |
| let random_id: u64 = rand::thread_rng().random(); |
| let canonical_home = home_codex.canonicalize().unwrap_or(home_codex); | ||
|
|
||
| // Security check: path must be within ~/.codex or the base directory | ||
| if !canonical.starts_with(&canonical_home) && !canonical.starts_with(&canonical_base) { | ||
| return Err(anyhow::anyhow!( | ||
| "Security error: Prompt file must be within ~/.codex or the configured agents directory" | ||
| )); | ||
| } |
There was a problem hiding this comment.
The use of unwrap_or on the result of canonicalize() can be confusing and potentially less safe. If home_codex.canonicalize() fails (e.g., because ~/.codex doesn't exist), canonical_home will be a non-canonicalized path, which is then compared to a canonicalized path. This can lead to subtle bugs.
A clearer and safer approach is to handle the Result from canonicalize() explicitly. If it fails, we can safely assume the path is not within ~/.codex.
let is_in_home_codex = if let Ok(canonical_home) = home_codex.canonicalize() {
canonical.starts_with(&canonical_home)
} else {
false
};
// Security check: path must be within ~/.codex or the base directory
if !is_in_home_codex && !canonical.starts_with(&canonical_base) {
return Err(anyhow::anyhow!(
"Security error: Prompt file must be within ~/.codex or the configured agents directory"
));
}| // Prevent recursion by refusing to run if the tool list already excludes agent tool. | ||
| if matches!(turn.client.get_session_source(), SessionSource::SubAgent(_)) { | ||
| warn!("agent tool recursion detected"); | ||
| return Err(FunctionCallError::RespondToModel( | ||
| "Agents cannot spawn other agents".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
This recursion check is likely ineffective. The SessionSource is not updated for the sub-agent's ModelClient, so turn.client.get_session_source() will return the parent's source, and the SubAgent match arm will never be hit.
The effective recursion prevention is on line 153, where include_agent_tool is set to false for the sub-agent.
To avoid confusion, I recommend removing this check. If it's intended as a defense-in-depth measure, the ModelClient for the sub-agent would need to be created with SessionSource::SubAgent.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive multi-agent system, a significant and well-implemented new feature. The changes include the core agent registry, a tool handler for invoking agents, TUI integration for @mentions and the /agents command, and extensive documentation. The overall architecture is solid, with good security considerations like recursion prevention and path validation for prompt files.
My review includes a critical security fix for a potential path traversal vulnerability in AgentConfig::get_prompt. I've also included a couple of medium-severity suggestions to improve code clarity and adhere to best practices, such as renaming a misleadingly named function and replacing a deprecated RNG function.
The documentation additions are excellent and will be very helpful for users to understand and adopt this new feature. Great work on this complex addition!
| /// Get the effective prompt, loading from file if necessary | ||
| pub fn get_prompt(&mut self, agents_dir: Option<&Path>) -> anyhow::Result<String> { | ||
| if let Some(prompt) = &self.prompt { | ||
| return Ok(prompt.clone()); | ||
| } | ||
|
|
||
| if let Some(prompt_file) = &self.prompt_file { | ||
| let full_path = if let Some(dir) = agents_dir { | ||
| dir.join(prompt_file) | ||
| } else { | ||
| PathBuf::from(prompt_file) | ||
| }; | ||
|
|
||
| let prompt_content = std::fs::read_to_string(&full_path).map_err(|e| { | ||
| anyhow::anyhow!("Cannot read prompt file '{}': {}", full_path.display(), e) | ||
| })?; | ||
|
|
||
| // Cache the loaded prompt | ||
| self.prompt = Some(prompt_content.clone()); | ||
| Ok(prompt_content) | ||
| } else { | ||
| Err(anyhow::anyhow!("No prompt or prompt_file specified")) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The get_prompt function is vulnerable to a path traversal attack. It constructs a file path from prompt_file and reads it without validation. If an AgentConfig is constructed manually with a malicious prompt_file (e.g., ../../../etc/passwd), this method could be used to read arbitrary files from the filesystem. The validation performed in AgentRegistry::new does not protect against direct calls to this public method.
To fix this, get_prompt must validate the path before reading from it. You can reuse the existing validate_prompt_path logic for this, but you'll need to make it accessible from AgentConfig (e.g., by making it a pub(crate) or pub function). Additionally, agents_dir should be required when loading from a file to provide a base for validation.
pub fn get_prompt(&mut self, agents_dir: &Path) -> anyhow::Result<String> {
if let Some(prompt) = &self.prompt {
return Ok(prompt.clone());
}
if let Some(prompt_file) = &self.prompt_file {
let safe_path = AgentRegistry::validate_prompt_path(agents_dir, prompt_file)?;
let prompt_content = std::fs::read_to_string(&safe_path).map_err(|e| {
anyhow::anyhow!("Cannot read prompt file '{}': {}", safe_path.display(), e)
})?;
// Cache the loaded prompt
self.prompt = Some(prompt_content.clone());
Ok(prompt_content)
} else {
Err(anyhow::anyhow!("No prompt or prompt_file specified"))
}
}| #[allow(dead_code)] | ||
| pub async fn execute_agent_task( | ||
| agent_name: &str, | ||
| task: String, | ||
| registry: &AgentRegistry, | ||
| ) -> Result<String> { | ||
| // Get the agent's system prompt | ||
| let system_prompt = registry.get_system_prompt(agent_name); | ||
|
|
||
| // Build the specialized prompt for this agent | ||
| let full_prompt = format!("{system_prompt}\n\nTask: {task}"); | ||
|
|
||
| // Note: The actual execution will be handled by the parent context | ||
| // using the existing conversation infrastructure | ||
| Ok(full_prompt) | ||
| } |
There was a problem hiding this comment.
The function execute_agent_task is misleadingly named, as it doesn't execute a task but rather prepares a prompt string for it. It's also marked as async but contains no asynchronous operations, which could confuse future maintainers.
#[allow(dead_code)]
pub fn prepare_agent_task_prompt(
agent_name: &str,
task: String,
registry: &AgentRegistry,
) -> Result<String> {
// Get the agent's system prompt
let system_prompt = registry.get_system_prompt(agent_name);
// Build the specialized prompt for this agent
let full_prompt = format!("{system_prompt}\n\nTask: {task}");
// Note: The actual execution will be handled by the parent context
// using the existing conversation infrastructure
Ok(full_prompt)
}| let random_id: u64 = rand::rng().random(); | ||
| item_id = format!("agent-call-{random_id}"); |
There was a problem hiding this comment.
The rand::rng() function is deprecated. The documentation recommends using rand::thread_rng() for a thread-local random number generator. While not a security risk here, it's best practice to avoid deprecated functions.
let random_id: u64 = rand::thread_rng().gen();
item_id = format!("agent-call-{random_id}");
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-implemented multi-agent system. The changes include the core logic for an agent registry, a tool handler for invoking agents, and UI integrations for mentions and listing agents. The security aspects, such as preventing path traversal and agent recursion, are thoughtfully handled. The documentation is comprehensive and will be very helpful for users. My review includes a couple of suggestions: one to enable parallel agent execution to align with documented use cases, and a security hardening to prevent symlink abuse on the agent configuration file. Overall, this is a solid contribution.
| return Ok(Self { agents, agents_dir }); | ||
| } | ||
| let config_path = dir.join("agents.toml"); | ||
| if config_path.exists() { |
There was a problem hiding this comment.
There's a potential security vulnerability here. The code checks if the ~/.codex directory is a symlink, but it doesn't check if ~/.codex/agents.toml itself is a symlink. std::fs::read_to_string follows symlinks, which could lead to reading sensitive files if agents.toml is a symlink pointing elsewhere (e.g., ~/.ssh/id_rsa). While parsing would likely fail, this could still leak information about file existence or content in error messages. It's best to explicitly check for and disallow symlinks for the configuration file itself.
if config_path.symlink_metadata().map_or(false, |m| m.is_symlink()) {
tracing::error!(
"Security: agents config file at '{}' is a symlink, which is not allowed. Aborting loading of user-defined agents.",
config_path.display()
);
return Ok(Self { agents, agents_dir });
}
if config_path.exists() {| } | ||
|
|
||
| if config.include_agent_tool { | ||
| builder.push_spec(create_agent_tool()); |
There was a problem hiding this comment.
The agent tool is not configured to support parallel execution, but the documentation in AGENTS.md suggests that multiple agents (like @code_reviewer and @code_reviewer_b) could be run concurrently. Enabling parallel tool calls for the agent tool would align with this documented behavior and allow for more efficient multi-agent workflows. The AgentHandler appears to be stateless and safe for parallel execution.
| builder.push_spec(create_agent_tool()); | |
| builder.push_spec_with_parallel_support(create_agent_tool(), true); |
Grinsven
force-pushed
the
agents-multi-final
branch
from
630c5e1 to
41d9933
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant multi-agent support, including core logic, TUI integration, and comprehensive documentation. The implementation is well-structured, with good attention to security, particularly in loading agent prompts. The TUI changes also provide a solid user experience for agent interaction. I have identified a few areas for improvement, including a potential bug in event persistence and a suggestion for code simplification to enhance maintainability.
| | EventMsg::AgentBegin(_) | ||
| | EventMsg::AgentProgress(_) | ||
| | EventMsg::AgentEnd(_) |
There was a problem hiding this comment.
AgentBegin and AgentEnd events are important for understanding the session flow during replay or debugging and should be persisted in the rollout file. ListAgentsResponse and AgentProgress are more transient and are fine to exclude.
| | EventMsg::AgentBegin(_) | |
| | EventMsg::AgentProgress(_) | |
| | EventMsg::AgentEnd(_) | |
| | EventMsg::AgentProgress(_) |
| - If the user asks for "a debugger", run @debugger. | ||
| - If the user asks for "research", run @researcher. | ||
| - Default scope when not provided: latest two commits in the current repo. | ||
| - Always cite file:line in findings and keep summaries concise. No newline at end of file |
There was a problem hiding this comment.
It's a common convention to have a newline at the end of text files. Please add one here for consistency.
| if let Some(ref prompt_file) = config.prompt_file { | ||
| // Validate the path to prevent traversal attacks | ||
| match Self::validate_prompt_path(dir, prompt_file) { | ||
| Ok(safe_path) => { | ||
| match std::fs::read_to_string(&safe_path) { | ||
| Ok(prompt_content) => { | ||
| config.prompt = Some(prompt_content); | ||
| tracing::debug!( | ||
| "Loaded prompt file for agent '{}'", | ||
| name | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| tracing::error!( | ||
| "Cannot read prompt file '{}' for agent '{}': {}", | ||
| prompt_file, | ||
| name, | ||
| e | ||
| ); | ||
| // Skip this agent but continue loading others | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
| Err(e) => { | ||
| tracing::error!( | ||
| "Agent '{}' configuration error: {}", | ||
| name, | ||
| e | ||
| ); | ||
| // Skip this agent but continue loading others | ||
| continue; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for loading prompts from files is a bit deeply nested. It could be simplified by chaining Result operations, which would improve readability and maintainability.
if let Some(ref prompt_file) = config.prompt_file {
match Self::validate_prompt_path(dir, prompt_file).and_then(|path| {
std::fs::read_to_string(&path).map_err(|e| {
anyhow::anyhow!(
"Cannot read prompt file '{}': {}",
path.display(),
e
)
})
}) {
Ok(prompt_content) => {
config.prompt = Some(prompt_content);
tracing::debug!("Loaded prompt file for agent '{}'", name);
}
Err(e) => {
tracing::error!("Agent '{}' configuration error: {}", name, e);
continue;
}
}
}
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant multi-agent support, including a core agent registry, a tool handler for agent invocation, TUI integration for agent mentions and listing, and comprehensive documentation. The implementation is well-structured with strong attention to security aspects like path traversal and symlink protection, and robustness features such as agent recursion prevention. The new documentation is excellent and thorough. I have a couple of suggestions for minor refactoring to improve maintainability and correctness, but overall, this is a very solid and well-executed feature addition.
| // If prompt_file is specified, load the prompt from file | ||
| if let Some(ref prompt_file) = config.prompt_file { | ||
| // Validate the path to prevent traversal attacks | ||
| match Self::validate_prompt_path(dir, prompt_file).and_then(|path| { | ||
| std::fs::read_to_string(&path).map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "Cannot read prompt file '{}': {}", | ||
| path.display(), | ||
| e | ||
| ) | ||
| }) | ||
| }) { | ||
| Ok(prompt_content) => { | ||
| config.prompt = Some(prompt_content); | ||
| tracing::debug!( | ||
| "Loaded prompt file for agent '{}'", | ||
| name | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| tracing::error!( | ||
| "Agent '{}' configuration error: {}", | ||
| name, | ||
| e | ||
| ); | ||
| // Skip this agent but continue loading others | ||
| continue; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| agents.insert(name, config); | ||
| } | ||
| tracing::info!("Loaded {} user-defined agents", agents.len() - 1); | ||
| } | ||
| Err(e) => { |
There was a problem hiding this comment.
The logic for loading a prompt from prompt_file is duplicated in AgentRegistry::new and AgentConfig::get_prompt. The new function eagerly loads prompts, while get_prompt supports lazy loading. This duplication can lead to maintenance issues where one implementation is updated but the other is not.
To improve this, you can refactor AgentRegistry::new to call config.get_prompt() to load prompts, which will centralize the logic and remove the duplicated code. This would make the code more maintainable.
// If prompt_file is specified, load the prompt from file
if config.prompt_file.is_some() {
if let Err(e) = config.get_prompt(Some(dir)) {
tracing::error!(
"Failed to load prompt for agent '{}': {}",
name,
e
);
continue;
}
}| desc.trim_start_matches("You are a ") | ||
| .trim_start_matches("You are an ") | ||
| .trim_start_matches("You are ") | ||
| .trim() | ||
| .to_string() |
There was a problem hiding this comment.
The current logic for extracting an agent's description from its prompt can prematurely truncate the sentence if it contains a period that is not at the end, such as in a version number (e.g., v1.2). This could result in confusing or incomplete agent descriptions in the UI. A more robust approach would be to split at the first occurrence of a period followed by a space, or to take the whole first line if that pattern isn't found.
let desc = if let Some(pos) = first_line.find(". ") {
&first_line[..=pos]
} else {
first_line
};
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality by adding multi-agent tool support. This includes core logic for an agent registry, an agent tool handler, and updates to the protocol. The TUI is also updated to support listing agents and using @mentions to invoke them. The changes are accompanied by extensive documentation and examples, which is excellent.
My review found one critical issue that will prevent compilation and a couple of medium-severity suggestions regarding repository hygiene. Overall, this is a well-structured and comprehensive feature addition.
| match (a.is_builtin, b.is_builtin) { | ||
| (true, false) => std::cmp::Ordering::Less,\n (false, true) => std::cmp::Ordering::Greater, | ||
| _ => a.name.cmp(&b.name), | ||
| } |
There was a problem hiding this comment.
There appears to be a stray newline character \n within the match statement, which will cause a compilation error. Please remove it to ensure the code compiles correctly.
match (a.is_builtin, b.is_builtin) {
(true, false) => std::cmp::Ordering::Less,
(false, true) => std::cmp::Ordering::Greater,
_ => a.name.cmp(&b.name),
}| @@ -0,0 +1,21 @@ | |||
| # Next Session Notes | |||
There was a problem hiding this comment.
This file appears to contain personal development notes and a PR draft. Such files are generally not committed to the repository. Please consider removing it from this pull request to keep the project history clean.
| @@ -0,0 +1,39 @@ | |||
| # Multi-agent merge plan (local fork) | |||
There was a problem hiding this comment.
This file seems to be a local merge plan. It's good practice to exclude such temporary or personal planning documents from the repository's history. Please consider removing this file from the pull request.
Grinsven
force-pushed
the
agents-multi-final
branch
from
7dc6a03 to
652f075
Compare
1 participant
Summary
Testing