Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ pub fn create_with_registry(args: &CreateArgs, registry: &WorkflowRegistry) -> R
.ok_or_else(|| anyhow::anyhow!("--workflow is required for create workflow"))?;
let _ = registry.resolve_str(&workflow_id)?;
if workflow_id == workflow::builtin::NEGOTIATE_WORKFLOW_ID
&& expected_agents < 3
&& expected_agents < 2
{
bail!(
"workflow '{}' requires --agents >= 3 (got {})",
"workflow '{}' requires --agents >= 2 (got {})",
workflow_id,
expected_agents
);
Expand Down
133 changes: 133 additions & 0 deletions src/workflow/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pub struct ConsultWorkflow;
pub struct ComposedNegotiateWorkflow;
pub struct HelloWorkflow;
pub struct ImplementRunCommand;
pub struct NegotiateRunCommand;
pub struct HelloRunCommand;

impl RunCommand for ImplementRunCommand {
Expand Down Expand Up @@ -118,6 +119,82 @@ impl RunCommand for ImplementRunCommand {
}
}

impl RunCommand for NegotiateRunCommand {
fn name(&self) -> &str {
"negotiate"
}

fn description(&self) -> &str {
"Create or join a negotiate session for multi-agent planning"
}

fn usage(&self) -> &str {
"negotiate <topic> <role> [--agents N]"
}

fn resolve(&self, args: &[String], _workspace: &Path) -> Result<RunCommandResolution> {
if args.len() < 2 {
bail!(
"usage: /rally {}\n\nExample: /rally negotiate \"auth design\" agent-alpha",
self.usage()
);
}

let topic = &args[0];
let role = &args[1];

let mut agents = 2u32;
let mut i = 2;
while i < args.len() {
if args[i] == "--agents" {
i += 1;
if i >= args.len() {
bail!("--agents requires a number argument");
}
agents = args[i]
.parse()
.map_err(|_| anyhow!("--agents must be a number, got '{}'", args[i]))?;
i += 1;
continue;
}
i += 1;
}

let slug: String = topic
.to_ascii_lowercase()
.chars()
.map(|c| if c.is_alphanumeric() { c } else { '-' })
.collect::<String>()
.split('-')
.filter(|s| !s.is_empty())
.collect::<Vec<_>>()
.join("-");
let session_name = format!("negotiate-{slug}");
Comment on lines +163 to +172
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 NegotiateRunCommand slug can be empty, producing degenerate session name

When the topic contains only non-alphanumeric characters (e.g., "!!!" or "---"), the inline slug computation at src/workflow/builtin.rs:163-171 produces an empty string, resulting in session name "negotiate-". This causes: (1) a meaningless session name, and (2) session name collisions for any two distinct non-alphanumeric-only topics. The codebase already handles this correctly elsewhere — the slugify function in crates/rally-workflow-plan/src/lib.rs:1360-1381 falls back to "issue" when the result is empty.

Suggested change
let slug: String = topic
.to_ascii_lowercase()
.chars()
.map(|c| if c.is_alphanumeric() { c } else { '-' })
.collect::<String>()
.split('-')
.filter(|s| !s.is_empty())
.collect::<Vec<_>>()
.join("-");
let session_name = format!("negotiate-{slug}");
let slug: String = topic
.to_ascii_lowercase()
.chars()
.map(|c| if c.is_alphanumeric() { c } else { '-' })
.collect::<String>()
.split('-')
.filter(|s| !s.is_empty())
.collect::<Vec<_>>()
.join("-");
let slug = if slug.is_empty() { "session".to_string() } else { slug };
let session_name = format!("negotiate-{slug}");
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +163 to +172
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Edge case: Empty slug from topic with only special characters.

If the topic contains only non-alphanumeric characters (e.g., "---" or "!!!"), the slug will become empty, resulting in a session name of just "negotiate-". Consider adding validation.

🛡️ Proposed fix to validate slug
         let slug: String = topic
             .to_ascii_lowercase()
             .chars()
             .map(|c| if c.is_alphanumeric() { c } else { '-' })
             .collect::<String>()
             .split('-')
             .filter(|s| !s.is_empty())
             .collect::<Vec<_>>()
             .join("-");
+        if slug.is_empty() {
+            bail!("topic must contain at least one alphanumeric character");
+        }
         let session_name = format!("negotiate-{slug}");
📝 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
let slug: String = topic
.to_ascii_lowercase()
.chars()
.map(|c| if c.is_alphanumeric() { c } else { '-' })
.collect::<String>()
.split('-')
.filter(|s| !s.is_empty())
.collect::<Vec<_>>()
.join("-");
let session_name = format!("negotiate-{slug}");
let slug: String = topic
.to_ascii_lowercase()
.chars()
.map(|c| if c.is_alphanumeric() { c } else { '-' })
.collect::<String>()
.split('-')
.filter(|s| !s.is_empty())
.collect::<Vec<_>>()
.join("-");
if slug.is_empty() {
bail!("topic must contain at least one alphanumeric character");
}
let session_name = format!("negotiate-{slug}");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/workflow/builtin.rs` around lines 163 - 172, The computed slug variable
may be empty for topics containing only non-alphanumeric characters; after the
slug calculation (the slug binding), add a validation step that checks if
slug.is_empty() and replaces it with a safe fallback (e.g., "untitled" or a
hashed/encoded short form of topic) before using it to build session_name so
session_name is never "negotiate-". Update usage of slug (the session_name =
format!("negotiate-{slug}") line) to use the validated/fallback slug and
optionally surface a warning or error if you prefer explicit handling.

let agent_name = normalize_agent_selector(role);

let create_args = CreateArgs {
session_type: SessionTypeArg::Workflow,
name: session_name.clone(),
agents: Some(agents),
reviewers: None,
todo: None,
workspace: None,
workflow: Some("negotiate".to_string()),
implementer: String::new(),
topic: Some(topic.clone()),
max_rounds: 4,
turn_timeout_secs: 300,
review_timeout_secs: 1200,
};

Ok(RunCommandResolution::Session {
session_name,
agent_name,
create_args,
})
}
}

impl RunCommand for HelloRunCommand {
fn name(&self) -> &str {
"hello"
Expand Down Expand Up @@ -253,6 +330,11 @@ impl Workflow for ComposedNegotiateWorkflow {
&["file-issue", "position", "challenge", "agree"]
}

fn run_command(&self) -> Option<&dyn RunCommand> {
static COMMAND: NegotiateRunCommand = NegotiateRunCommand;
Some(&COMMAND)
}

fn on_create(&self, ctx: &mut super::CreateContext<'_>) -> Result<super::CreateDispatch> {
ctx.host.state.workflow_state = Some(plan::initial_workflow_state()?);
for p in [
Expand Down Expand Up @@ -738,6 +820,57 @@ mod tests {
assert!(!cmd.usage().is_empty());
}

#[test]
fn negotiate_run_command_resolve_valid_args() {
let workspace = temp_workspace("rally-negotiate-cmd-valid");
let cmd = NegotiateRunCommand;
let args = vec!["auth design".to_string(), "agent-alpha".to_string()];
let (session_name, agent_name, create_args) =
unwrap_session(cmd.resolve(&args, &workspace).expect("resolve"));
assert_eq!(session_name, "negotiate-auth-design");
assert_eq!(agent_name, "agent-alpha");
assert_eq!(create_args.agents, Some(2));
assert_eq!(create_args.workflow.as_deref(), Some("negotiate"));
assert_eq!(create_args.topic.as_deref(), Some("auth design"));
}

#[test]
fn negotiate_run_command_resolve_custom_agents() {
let workspace = temp_workspace("rally-negotiate-cmd-agents");
let cmd = NegotiateRunCommand;
let args = vec![
"my topic".to_string(),
"agent-beta".to_string(),
"--agents".to_string(),
"4".to_string(),
];
let (_, _, create_args) =
unwrap_session(cmd.resolve(&args, &workspace).expect("resolve"));
assert_eq!(create_args.agents, Some(4));
}

#[test]
fn negotiate_run_command_resolve_missing_args() {
let workspace = temp_workspace("rally-negotiate-cmd-missing");
let cmd = NegotiateRunCommand;
let err = cmd
.resolve(&[], &workspace)
.expect_err("should fail with missing args");
assert!(err.to_string().contains("usage:"));

let err = cmd
.resolve(&["only-topic".to_string()], &workspace)
.expect_err("should fail with one arg");
assert!(err.to_string().contains("usage:"));
}

#[test]
fn negotiate_workflow_exposes_run_command() {
let workflow = ComposedNegotiateWorkflow;
let cmd = workflow.run_command().expect("should have run command");
assert_eq!(cmd.name(), "negotiate");
}

#[test]
fn plan_workflow_has_no_run_command() {
let workflow = PlanWorkflow;
Expand Down
Loading