Allow negotiate workflow with 2 agents#16
Merged
justinmoon merged 1 commit intomasterfrom Mar 4, 2026
Merged
Conversation
… command The negotiate workflow previously required --agents >= 3 at the command layer, even though StrictNegotiationPolicy only required min_agents: 2. This lowered the gate to 2 and added a NegotiateRunCommand so agents can invoke `/rally negotiate <topic> <role>` directly via the skill surface. - Lower minimum agent check from 3 to 2 in commands.rs - Add NegotiateRunCommand with topic slugification and --agents flag - Wire run_command() into ComposedNegotiateWorkflow - Add negotiate_two_agent_happy_path Rust integration test - Add tests/e2e_negotiate.py for agent-based e2e testing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThese changes lower the minimum agent threshold from 3 to 2 for workflow negotiations, introduce a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+163
to
+172
| 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}"); |
There was a problem hiding this comment.
🟡 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}"); |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/e2e_negotiate.py (1)
29-52: Unused lambda parameters are intentional but could be cleaner.The lambda signatures include
promptandlogfileparameters for interface consistency, but some don't use them. This is flagged by static analysis but is acceptable for test utilities. Consider using_prefix for unused parameters if you want to suppress warnings.♻️ Optional: Use underscore prefix for unused params
AGENTS = { "claude": { - "launch_stdin": lambda prompt, logfile: [ + "launch_stdin": lambda _prompt, _logfile: [ "claude", "--dangerously-skip-permissions", "--no-session-persistence" ], "prompt_style": "stdin", "skill_prefix": "/rally", }, "codex": { - "launch_exec": lambda prompt, logfile: [ + "launch_exec": lambda prompt, _logfile: [ "codex", "exec", prompt, "--dangerously-bypass-approvals-and-sandbox", "--ephemeral" ], "prompt_style": "exec", "skill_prefix": "$rally", }, "pi": { - "launch_stdin": lambda prompt, logfile: [ + "launch_stdin": lambda _prompt, _logfile: [ "pi", "--no-session", "--tools", "bash" ], "prompt_style": "stdin", "skill_prefix": "Use the rally skill to:", }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e_negotiate.py` around lines 29 - 52, The lambdas inside the AGENTS mapping declare unused parameters (prompt, logfile) which trigger static-analysis warnings; update the lambdas that don't use those arguments by renaming them to _prompt and/or _logfile (for example the "claude" and "pi" launch_stdin lambdas, and the "codex" launch_exec lambda if it doesn't use both) so unused params are prefixed with an underscore while leaving behavior unchanged and preserving the AGENTS keys and launch_stdin/launch_exec/prompt_style/skill_prefix symbols.
🤖 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/workflow/builtin.rs`:
- Around line 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.
In `@tests/e2e_negotiate.py`:
- Around line 97-101: The build_prompt function currently ignores the context
parameter so agents never see the negotiation context; update build_prompt (and
the returned string that uses cfg["skill_prefix"] / prefix and the negotiate
invocation) to include the provided context value in the prompt (e.g., append or
embed the context text alongside the /rally negotiate command, properly quoted
or labeled) so the agent receives the error-handling strategy discussion when
invoked.
- Around line 69-94: The launch_agent function leaks file handles because
open(logfile, "w") is called inline in subprocess.Popen; create explicit
file-handle variables (e.g., out_f = open(logfile, "w")) before calling Popen
for both the stdin and exec branches, pass out_f as stdout, and ensure you close
out_f if subprocess.Popen raises (use try/except to close on failure) and/or
close the handle in a finally block after successfully starting the process if
appropriate; also ensure proc.stdin is managed similarly when used. Reference:
function launch_agent, variables cmd, proc, and the inline open(...) calls.
---
Nitpick comments:
In `@tests/e2e_negotiate.py`:
- Around line 29-52: The lambdas inside the AGENTS mapping declare unused
parameters (prompt, logfile) which trigger static-analysis warnings; update the
lambdas that don't use those arguments by renaming them to _prompt and/or
_logfile (for example the "claude" and "pi" launch_stdin lambdas, and the
"codex" launch_exec lambda if it doesn't use both) so unused params are prefixed
with an underscore while leaving behavior unchanged and preserving the AGENTS
keys and launch_stdin/launch_exec/prompt_style/skill_prefix symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbaf5087-0d8c-4579-8296-70961b821b3f
📒 Files selected for processing (4)
src/commands.rssrc/workflow/builtin.rstests/e2e_negotiate.pytests/extensibility_mvp.rs
Comment on lines
+163
to
+172
| 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}"); |
There was a problem hiding this comment.
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.
Comment on lines
+69
to
+94
| def launch_agent(agent_name, prompt, logfile): | ||
| cfg = AGENTS[agent_name] | ||
| # Remove CLAUDECODE env var to allow launching from within a Claude session | ||
| env = {k: v for k, v in os.environ.items() if k != "CLAUDECODE"} | ||
| if cfg["prompt_style"] == "stdin": | ||
| cmd = cfg["launch_stdin"](prompt, logfile) | ||
| proc = subprocess.Popen( | ||
| cmd, | ||
| stdin=subprocess.PIPE, | ||
| stdout=open(logfile, "w"), | ||
| stderr=subprocess.STDOUT, | ||
| cwd=str(WORKSPACE), | ||
| env=env, | ||
| ) | ||
| proc.stdin.write(prompt.encode()) | ||
| proc.stdin.close() | ||
| else: | ||
| cmd = cfg["launch_exec"](prompt, logfile) | ||
| proc = subprocess.Popen( | ||
| cmd, | ||
| stdout=open(logfile, "w"), | ||
| stderr=subprocess.STDOUT, | ||
| cwd=str(WORKSPACE), | ||
| env=env, | ||
| ) | ||
| return proc |
There was a problem hiding this comment.
File handles for log files are not explicitly managed.
The open(logfile, "w") calls create file handles passed to Popen, but if Popen fails, the handles leak. Using a context manager or storing the handle ensures proper cleanup.
🛡️ Proposed fix using explicit handle management
def launch_agent(agent_name, prompt, logfile):
cfg = AGENTS[agent_name]
# Remove CLAUDECODE env var to allow launching from within a Claude session
env = {k: v for k, v in os.environ.items() if k != "CLAUDECODE"}
+ log_handle = open(logfile, "w")
if cfg["prompt_style"] == "stdin":
cmd = cfg["launch_stdin"](prompt, logfile)
proc = subprocess.Popen(
cmd,
stdin=subprocess.PIPE,
- stdout=open(logfile, "w"),
+ stdout=log_handle,
stderr=subprocess.STDOUT,
cwd=str(WORKSPACE),
env=env,
)
proc.stdin.write(prompt.encode())
proc.stdin.close()
else:
cmd = cfg["launch_exec"](prompt, logfile)
proc = subprocess.Popen(
cmd,
- stdout=open(logfile, "w"),
+ stdout=log_handle,
stderr=subprocess.STDOUT,
cwd=str(WORKSPACE),
env=env,
)
+ proc._log_handle = log_handle # Store for later cleanup if needed
return proc🧰 Tools
🪛 Ruff (0.15.2)
[error] 75-75: subprocess call: check for execution of untrusted input
(S603)
[error] 87-87: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e_negotiate.py` around lines 69 - 94, The launch_agent function leaks
file handles because open(logfile, "w") is called inline in subprocess.Popen;
create explicit file-handle variables (e.g., out_f = open(logfile, "w")) before
calling Popen for both the stdin and exec branches, pass out_f as stdout, and
ensure you close out_f if subprocess.Popen raises (use try/except to close on
failure) and/or close the handle in a finally block after successfully starting
the process if appropriate; also ensure proc.stdin is managed similarly when
used. Reference: function launch_agent, variables cmd, proc, and the inline
open(...) calls.
Comment on lines
+97
to
+101
| def build_prompt(agent_name, topic, role, context): | ||
| """Build a prompt that gives the agent context + a /rally negotiate invocation.""" | ||
| cfg = AGENTS[agent_name] | ||
| prefix = cfg["skill_prefix"] | ||
| return f"{prefix} negotiate \"{topic}\" {role}" |
There was a problem hiding this comment.
The context parameter is unused - agents won't receive negotiation context.
The build_prompt function accepts a context parameter (containing the error handling strategy discussion) but never includes it in the returned prompt. Agents will only receive the bare /rally negotiate command without understanding what to negotiate about.
🐛 Proposed fix to include context in prompt
def build_prompt(agent_name, topic, role, context):
"""Build a prompt that gives the agent context + a /rally negotiate invocation."""
cfg = AGENTS[agent_name]
prefix = cfg["skill_prefix"]
- return f"{prefix} negotiate \"{topic}\" {role}"
+ return f"""{context}
+
+{prefix} negotiate \"{topic}\" {role}"""📝 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
| def build_prompt(agent_name, topic, role, context): | |
| """Build a prompt that gives the agent context + a /rally negotiate invocation.""" | |
| cfg = AGENTS[agent_name] | |
| prefix = cfg["skill_prefix"] | |
| return f"{prefix} negotiate \"{topic}\" {role}" | |
| def build_prompt(agent_name, topic, role, context): | |
| """Build a prompt that gives the agent context + a /rally negotiate invocation.""" | |
| cfg = AGENTS[agent_name] | |
| prefix = cfg["skill_prefix"] | |
| return f"""{context} | |
| {prefix} negotiate \"{topic}\" {role}""" |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 97-97: Unused function argument: context
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e_negotiate.py` around lines 97 - 101, The build_prompt function
currently ignores the context parameter so agents never see the negotiation
context; update build_prompt (and the returned string that uses
cfg["skill_prefix"] / prefix and the negotiate invocation) to include the
provided context value in the prompt (e.g., append or embed the context text
alongside the /rally negotiate command, properly quoted or labeled) so the agent
receives the error-handling strategy discussion when invoked.
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
1 participant
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
StrictNegotiationPolicy.min_agents)NegotiateRunCommandso agents can invoke/rally negotiate <topic> <role>via the skill surfacemin(policy=2, n_agents-1=1) = 1Test plan
negotiate_two_agent_happy_path)NegotiateRunCommandargument parsing and edge cases🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes