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
1 change: 1 addition & 0 deletions crates/rally-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub enum SessionPhase {
Implement,
Review,
FinalReview,
Paused,
Done,
}

Expand Down
41 changes: 39 additions & 2 deletions crates/rally-workflow-build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ pub struct BuildWorkflowState {
pub reviews: Vec<ReviewState>,
#[serde(default)]
pub pending_feedback: Vec<String>,
#[serde(default)]
pub pause_after_step: bool,
}

impl Default for BuildWorkflowState {
Expand All @@ -75,6 +77,7 @@ impl Default for BuildWorkflowState {
checkpoint: None,
reviews: Vec::new(),
pending_feedback: Vec::new(),
pause_after_step: false,
}
}
}
Expand Down Expand Up @@ -107,10 +110,15 @@ impl BuildWorkflowState {
}
}

pub fn initial_workflow_state(implementer_agent: String, steps: Vec<StepState>) -> Result<Value> {
pub fn initial_workflow_state(
implementer_agent: String,
steps: Vec<StepState>,
pause_after_step: bool,
) -> Result<Value> {
let state = BuildWorkflowState {
implementer_agent,
steps,
pause_after_step,
..BuildWorkflowState::default()
};
encode_workflow_state(&state)
Expand Down Expand Up @@ -285,10 +293,35 @@ pub fn get_wait_hint(state: &SessionState, agent: &str) -> Option<String> {
None
}
}
SessionPhase::Paused => {
Some(format!(
"Session paused for user feedback after step {}. \
Waiting for `rally build continue --session {}`.",
workflow_state.current_step_idx, state.name
))
}
Comment on lines +296 to +302
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Paused wait hint displays 0-based index instead of completed step's number

In get_wait_hint for the Paused phase, the message uses workflow_state.current_step_idx (a 0-based vector index that has already been incremented to point to the next step) rather than the just-completed step's actual number. For a 2-step todo with sequential numbering (1, 2), completing step 1 sets current_step_idx = 1, so the message "paused after step 1" accidentally looks correct. However, if step numbers are non-sequential (e.g., steps numbered 5 and 10), current_step_idx would be 1 after completing the first step, producing the misleading message "paused after step 1" instead of "paused after step 5". The correct value would be workflow_state.steps[workflow_state.current_step_idx - 1].number.

Suggested change
SessionPhase::Paused => {
Some(format!(
"Session paused for user feedback after step {}. \
Waiting for `rally build continue --session {}`.",
workflow_state.current_step_idx, state.name
))
}
SessionPhase::Paused => {
let completed_step_label = if workflow_state.current_step_idx > 0 {
if let Some(step) = workflow_state.steps.get(workflow_state.current_step_idx - 1) {
format!("{}", step.number)
} else {
format!("{}", workflow_state.current_step_idx)
}
} else {
format!("{}", workflow_state.current_step_idx)
};
Some(format!(
"Session paused for user feedback after step {}. \
Waiting for `rally build continue --session {}`.",
completed_step_label, state.name
))
}
Open in Devin Review

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

_ => None,
}
}

pub fn continue_session(state: &mut SessionState, feedback: Option<String>) -> Result<String> {
if state.phase != SessionPhase::Paused {
bail!("session is not paused (current phase: {:?})", state.phase);
}
let mut workflow_state = read_workflow_state(state)?;
if let Some(fb) = feedback {
workflow_state.pending_feedback.push(fb);
}
state.phase = SessionPhase::Implement;
write_workflow_state(state, &workflow_state)?;
let step_label = if let Some(step) = workflow_state.current_step() {
format!("step {}: {}", step.number, step.title)
} else {
"next step".to_string()
};
Ok(format!("resumed; advancing to {step_label}"))
}

pub fn mark_done(state: &mut SessionState, _agent: &str) -> Result<String> {
match state.phase {
SessionPhase::Done => Ok("session already complete".to_string()),
Expand Down Expand Up @@ -580,7 +613,11 @@ fn complete_step(
}

workflow_state.current_step_idx += 1;
state.phase = SessionPhase::Implement;
state.phase = if workflow_state.pause_after_step {
SessionPhase::Paused
} else {
SessionPhase::Implement
};
Ok(())
Comment on lines 615 to 621
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 pause_after_step also pauses on blocked/escalated steps, contradicting intended behavior

The complete_step function unconditionally checks workflow_state.pause_after_step to decide whether to set SessionPhase::Paused (line 616), but it is called for all step completions — including when a review is BLOCKED and the step is escalated (crates/rally-workflow-build/src/lib.rs:500). The PR description explicitly states the flag is for "pausing for user feedback after each step approval", not after escalation. When a step is blocked with pause_after_step enabled, the session enters Paused instead of immediately advancing, and the returned message says "step escalated" with no indication of the pause. The caller at line 502 checks state.phase == SessionPhase::FinalReview which won't match Paused, so it falls through to an incorrect message.

Prompt for agents
In crates/rally-workflow-build/src/lib.rs, the complete_step function at line 615-621 unconditionally applies the pause_after_step logic for all step completion types. To match the stated intent (pause only after approval), the pause should be conditional on the step status. One approach: pass the StepStatus to the phase-decision logic:

    state.phase = if workflow_state.pause_after_step && status == StepStatus::Approved {
        SessionPhase::Paused
    } else {
        SessionPhase::Implement
    };

This requires that the status variable is accessible at this point in complete_step (it is — it's the status parameter). Also update the callers in process_review_state (around lines 500-507) to account for the possibility that state.phase could be Paused when approval triggers it, so the return messages are accurate.
Open in Devin Review

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

}

Expand Down
16 changes: 16 additions & 0 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,12 @@ pub struct CreateArgs {
pub turn_timeout_secs: u64,
#[arg(long, default_value_t = 1200)]
pub review_timeout_secs: u64,
#[arg(
long,
default_value_t = false,
help = "Pause for user feedback after each step is approved"
)]
pub pause_after_step: bool,
}

#[derive(Args, Debug)]
Expand Down Expand Up @@ -266,6 +272,16 @@ pub enum BuildSubcommand {
Checkpoint(CheckpointArgs),
#[command(about = "Submit a review verdict")]
Review(ReviewArgs),
#[command(about = "Resume a paused session (advance to the next step)")]
Continue(ContinueArgs),
}
Comment on lines +275 to +277
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

Update exit-code help text to include paused sessions.

With build continue added, exit code 2 now also represents a paused implement session. The help text at Line 16 and Line 56 still says “session complete” only.

[suggested update]

📝 Suggested doc patch
-  2  session complete; stop working
+  2  session complete or paused; stop working

@@
-        long_about = "Block until you have work, then print instructions.\n\nExit codes:\n  0  instruction ready (printed to stdout)\n  1  timeout expired; retry later\n  2  session complete; stop working"
+        long_about = "Block until you have work, then print instructions.\n\nExit codes:\n  0  instruction ready (printed to stdout)\n  1  timeout expired; retry later\n  2  session complete or paused; stop working"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/cli.rs` around lines 275 - 277, Help text for exit code 2 still says
"session complete" but needs to include paused implement sessions now that the
Continue(ContinueArgs) command exists; locate the help strings that describe
exit codes (the textual help block that mentions exit code 2 / "session
complete") and update them to say something like "session complete or paused
implement session" (or equivalent wording), ensuring any references in
usage/help output reflect that exit code 2 can mean a paused session as well;
check help text near the Continue enum variant and any constants or functions
that build the CLI help output to make the change.


#[derive(Args, Debug)]
pub struct ContinueArgs {
#[arg(long)]
pub session: String,
#[arg(long, help = "Optional feedback to pass to the implementer for the next step")]
pub feedback: Option<String>,
}

#[derive(Args, Debug)]
Expand Down
19 changes: 16 additions & 3 deletions src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ use serde_json::{Value, json};

use crate::{
cli::{
AgreeArgs, ChainArgs, ChallengeArgs, CheckpointArgs, CreateArgs, DoneArgs, FileIssueArgs,
JoinArgs, NextArgs, ReviewArgs, ReviewVerdictArg, SessionTypeArg, StatusArgs,
WorkflowActionArgs,
AgreeArgs, ChainArgs, ChallengeArgs, CheckpointArgs, ContinueArgs, CreateArgs, DoneArgs,
FileIssueArgs, JoinArgs, NextArgs, ReviewArgs, ReviewVerdictArg, SessionTypeArg,
StatusArgs, WorkflowActionArgs,
},
state::{
AgentState, Config, SessionHandle, SessionPhase, SessionState, SessionType,
Expand Down Expand Up @@ -84,6 +84,7 @@ pub fn create_with_registry(args: &CreateArgs, registry: &WorkflowRegistry) -> R
Some(implement::initial_workflow_state(
args.implementer.clone(),
steps,
args.pause_after_step,
)?),
)
}
Expand Down Expand Up @@ -411,6 +412,17 @@ pub fn review_with_registry(args: &ReviewArgs, registry: &WorkflowRegistry) -> R
Ok(())
}

pub fn continue_session_with_registry(args: &ContinueArgs) -> Result<()> {
let handle = SessionHandle::open(&args.session)?;
let mut state = handle.load_state()?;

let msg = implement::continue_session(&mut state, args.feedback.clone())?;

handle.save_state(&state)?;
println!("{msg}");
Ok(())
}
Comment on lines +415 to +424
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

Status output should advertise rally build continue.

Now that continue_session_with_registry exists, build session status text should include the new canonical command. Otherwise paused users may not discover the resume path from rally status.

📝 Suggested patch (in src/commands.rs)
-            println!("Canonical commands: rally build checkpoint|review");
+            println!("Canonical commands: rally build checkpoint|review|continue");
             println!("Legacy aliases: rally checkpoint|review");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands.rs` around lines 415 - 424, Update the build session status text
to advertise the new canonical resume command by adding a line that tells users
to run "rally build continue" (optionally with --session or the current session
id) when a session is paused; locate where status output is emitted in
src/commands.rs (the same area that handles SessionHandle::open / load_state and
prints session info) and append this resume hint so it points to the
continue_session_with_registry flow (implement::continue_session).


pub fn chain_with_registry(args: &ChainArgs, registry: &WorkflowRegistry) -> Result<()> {
let plan_session_dir;
loop {
Expand Down Expand Up @@ -454,6 +466,7 @@ pub fn chain_with_registry(args: &ChainArgs, registry: &WorkflowRegistry) -> Res
max_rounds: 4,
turn_timeout_secs: 300,
review_timeout_secs: 1200,
pause_after_step: false,
};

let session_name = create_with_registry(&create_args, registry)?;
Expand Down
7 changes: 6 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,12 @@ pub fn run_cli_with_registry(cli: Cli, registry: &WorkflowRegistry) -> Result<i3
BuildSubcommand::Checkpoint(inner) => {
commands::checkpoint_with_registry(&inner, registry)?
}
BuildSubcommand::Review(inner) => commands::review_with_registry(&inner, registry)?,
BuildSubcommand::Review(inner) => {
commands::review_with_registry(&inner, registry)?
}
BuildSubcommand::Continue(inner) => {
commands::continue_session_with_registry(&inner)?
}
}
Ok(0)
}
Expand Down
19 changes: 18 additions & 1 deletion src/workflow/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ impl RunCommand for ImplementRunCommand {
}

fn usage(&self) -> &str {
"implement <todo-file> <role> [--reviewers N]"
"implement <todo-file> <role> [--reviewers N] [--pause-after-step]"
}

fn resolve(&self, args: &[String], workspace: &Path) -> Result<RunCommandResolution> {
Expand All @@ -59,6 +59,7 @@ impl RunCommand for ImplementRunCommand {
let role = &args[1];

let mut reviewers = 1u32;
let mut pause_after_step = false;
let mut i = 2;
while i < args.len() {
if args[i] == "--reviewers" {
Expand All @@ -72,6 +73,11 @@ impl RunCommand for ImplementRunCommand {
i += 1;
continue;
}
if args[i] == "--pause-after-step" {
pause_after_step = true;
i += 1;
continue;
}
i += 1;
}

Expand Down Expand Up @@ -109,6 +115,7 @@ impl RunCommand for ImplementRunCommand {
max_rounds: 4,
turn_timeout_secs: 300,
review_timeout_secs: 1200,
pause_after_step,
};

Ok(RunCommandResolution::Session {
Expand Down Expand Up @@ -185,6 +192,7 @@ impl RunCommand for NegotiateRunCommand {
max_rounds: 4,
turn_timeout_secs: 300,
review_timeout_secs: 1200,
pause_after_step: false,
};

Ok(RunCommandResolution::Session {
Expand Down Expand Up @@ -429,6 +437,15 @@ impl Workflow for BuildWorkflow {
});
}

if ctx.host.state.phase == SessionPhase::Paused {
return Ok(NextPollDispatch::Complete {
message: format!(
"session paused for user feedback; run `rally build continue --session {}` to advance",
ctx.host.state.name
),
});
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +440 to +447
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Paused session's informative message is silently discarded; agent sees misleading "session complete; stop working"

When a session enters the Paused phase, BuildWorkflow::poll_next at src/workflow/builtin.rs:440-447 returns NextPollDispatch::Complete with a message explaining the pause and the rally build continue command. However, watch.rs:77 matches Complete { .. } and discards the message entirely, returning only exit code 2. Then command_surface.rs:96-97 and command_surface.rs:155-156 print "session complete; stop working" for any exit code 2. The result: the agent is told to stop permanently instead of being informed the session is paused. The agent never sees the helpful rally build continue --session <name> guidance, and may terminate thinking the job is finished.

Code path that discards the message

In src/watch.rs:77:

NextPollDispatch::Complete { .. } => {
    return Ok(2);
}

Then in src/command_surface.rs:96-97:

} else if code == 2 {
    println!("session complete; stop working");
}
Prompt for agents
The Complete dispatch message from BuildWorkflow::poll_next is discarded by watch.rs and command_surface.rs always prints "session complete; stop working" for exit code 2. To fix this:

1. In src/watch.rs around line 77, change the Complete match arm to print the message before returning:
   NextPollDispatch::Complete { message } => {
       println!("{message}");
       return Ok(2);
   }

2. In src/command_surface.rs around lines 96-98 and 155-157, remove or conditionalize the "session complete; stop working" println for code == 2, since the message is now printed by watch.rs. Alternatively, keep it but only when the message isn't already printed.

This ensures agents see the pause-specific guidance ("run rally build continue --session <name>") instead of the misleading "stop working" message.
Open in Devin Review

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


Ok(NextPollDispatch::Wait {
hint: implement::get_wait_hint(ctx.host.state, ctx.agent),
})
Expand Down
Loading