Add --pause-after-step flag to implement workflow#17
Conversation
📝 WalkthroughWalkthroughAdds a pause-after-step workflow feature: introduces a new Changes
sequenceDiagram
participant User
participant CLI as CLI/Commands
participant Workflow as Workflow State
participant Session as Session Phase
User->>CLI: implement --pause-after-step
CLI->>Workflow: initial_workflow_state(pause_after_step=true)
Workflow->>Session: create session in Implement phase
Note over Workflow,Session: Execute step(s)
User->>Workflow: complete_step()
Workflow->>Session: check pause_after_step flag
alt pause_after_step is true
Session->>Session: transition to Paused
Workflow-->>User: emit pause hint (how to resume)
else pause_after_step is false
Session->>Session: remain/transition to Implement
end
User->>CLI: continue --session ID [--feedback "text"]
CLI->>Workflow: continue_session(feedback)
Workflow->>Session: validate Paused phase
Session->>Session: transition to Implement
Workflow-->>User: return resume message with next step label
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Allow implement sessions to optionally pause for user feedback after each step is approved, instead of automatically advancing. Resume with `rally build continue --session <name> [--feedback "..."]`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
justinmoon
force-pushed
the
pause-after-step
branch
from
3a8ade8 to
762322c
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| SessionPhase::Paused => { | ||
| Some(format!( | ||
| "Session paused for user feedback after step {}. \ | ||
| Waiting for `rally build continue --session {}`.", | ||
| workflow_state.current_step_idx, state.name | ||
| )) | ||
| } |
There was a problem hiding this comment.
🟡 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.
| 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 | |
| )) | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| workflow_state.current_step_idx += 1; | ||
| state.phase = SessionPhase::Implement; | ||
| state.phase = if workflow_state.pause_after_step { | ||
| SessionPhase::Paused | ||
| } else { | ||
| SessionPhase::Implement | ||
| }; | ||
| Ok(()) |
There was a problem hiding this comment.
🔴 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.
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
🤖 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/cli.rs`:
- Around line 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.
In `@src/commands.rs`:
- Around line 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).
In `@src/workflow/builtin.rs`:
- Around line 357-364: The Pause branch in src/workflow/builtin.rs returns
NextPollDispatch::Complete with a resume hint message, but
watch::next_with_registry currently ignores that Complete { message } variant
and returns 2 so users never see the hint; update watch::next_with_registry to
handle NextPollDispatch::Complete by extracting and displaying or returning the
provided message (instead of silently mapping to code 2), ensuring the handler
recognizes NextPollDispatch::Complete and emits the message produced by the
pause branch (refer to the NextPollDispatch::Complete enum variant and the
watch::next_with_registry function).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59a85ee7-d779-4cec-9720-a36a657f7582
📒 Files selected for processing (7)
crates/rally-core/src/lib.rscrates/rally-workflow-build/src/lib.rssrc/cli.rssrc/commands.rssrc/lib.rssrc/workflow/builtin.rstests/extensibility_mvp.rs
| #[command(about = "Resume a paused session (advance to the next step)")] | ||
| Continue(ContinueArgs), | ||
| } |
There was a problem hiding this comment.
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.
| 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(()) | ||
| } |
There was a problem hiding this comment.
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).
| 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 | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔴 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/workflow/builtin.rs (1)
64-82: Unknown implement flags are silently ignored; fail fast instead.Right now unexpected tokens are skipped, which can hide typos in CLI usage and lead to surprising defaults.
Suggested patch
let mut i = 2; while i < args.len() { if args[i] == "--reviewers" { i += 1; if i >= args.len() { bail!("--reviewers requires a number argument"); } reviewers = args[i] .parse() .map_err(|_| anyhow!("--reviewers must be a number, got '{}'", args[i]))?; i += 1; continue; } if args[i] == "--pause-after-step" { pause_after_step = true; i += 1; continue; } - i += 1; + bail!("unknown argument '{}' (usage: /rally {})", args[i], self.usage()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workflow/builtin.rs` around lines 64 - 82, The argument parser loop in src/workflow/builtin.rs currently skips unrecognized tokens (inside the while loop handling args) which silently ignores typos; change the loop so that when args[i] is not a known flag (not "--reviewers" and not "--pause-after-step") it returns an error (e.g., bail! or Err) indicating an unknown argument and include the offending token (args[i]) in the message instead of silently incrementing i; update the logic around the reviewers parsing and pause_after_step handling to keep existing behavior for known flags but fail fast for any other args[i].src/commands.rs (1)
415-420: Add an explicit build-workflow guard beforecontinue_session.
rally build continuecurrently relies on downstream errors for non-build sessions. A local pre-check would produce clearer operator feedback.Suggested patch
pub fn continue_session_with_registry(args: &ContinueArgs) -> Result<()> { let handle = SessionHandle::open(&args.session)?; let mut state = handle.load_state()?; + let workflow_id = workflow::workflow_id_for_session(&state)?; + if workflow_id != workflow::builtin::BUILD_WORKFLOW_ID { + bail!( + "session '{}' is not a build session (workflow: {})", + args.session, + workflow_id + ); + } let msg = implement::continue_session(&mut state, args.feedback.clone())?; handle.save_state(&state)?; println!("{msg}"); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands.rs` around lines 415 - 420, The continue_session_with_registry function calls implement::continue_session unconditionally; add an explicit pre-check that the loaded session is a build workflow and return a clear error before calling implement::continue_session to avoid relying on downstream errors. After handle.load_state() (in continue_session_with_registry), inspect the session metadata on the returned state (e.g., a workflow/type flag on the session struct inside state or a method like state.is_build()) and if it is not a build session return an Err with a user-friendly message explaining that "rally build continue" only applies to build sessions; only call implement::continue_session(&mut state, ...) when the guard passes. Ensure you reference ContinueArgs and SessionHandle::open unchanged.tests/extensibility_mvp.rs (1)
2165-2168: Consider invokingrally build continueviarun_clihere for full CLI-path coverage.Using the command function directly is valid, but one CLI-level invocation would better protect parser/dispatch integration from regressions.
Suggested patch
- // Continue the session with feedback - commands::continue_session_with_registry(&ContinueArgs { - session: session.clone(), - feedback: Some("please also handle edge cases".to_string()), - })?; + // Continue the session with feedback (exercise CLI path) + run_cli( + &[ + "rally", + "build", + "continue", + "--session", + &session, + "--feedback", + "please also handle edge cases", + ], + ®istry, + )?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/extensibility_mvp.rs` around lines 2165 - 2168, Replace the direct call to commands::continue_session_with_registry(&ContinueArgs { session: session.clone(), feedback: Some(...)} ) with a test invocation of the CLI using run_cli to execute the equivalent "rally build continue" command so the parser/dispatch path is covered; construct the CLI args to include the session identifier and the feedback text (matching ContinueArgs), call run_cli(...) and assert the same success outcome as the original direct call, referencing the existing ContinueArgs, commands::continue_session_with_registry, and run_cli helpers to locate where to change the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/commands.rs`:
- Around line 415-420: The continue_session_with_registry function calls
implement::continue_session unconditionally; add an explicit pre-check that the
loaded session is a build workflow and return a clear error before calling
implement::continue_session to avoid relying on downstream errors. After
handle.load_state() (in continue_session_with_registry), inspect the session
metadata on the returned state (e.g., a workflow/type flag on the session struct
inside state or a method like state.is_build()) and if it is not a build session
return an Err with a user-friendly message explaining that "rally build
continue" only applies to build sessions; only call
implement::continue_session(&mut state, ...) when the guard passes. Ensure you
reference ContinueArgs and SessionHandle::open unchanged.
In `@src/workflow/builtin.rs`:
- Around line 64-82: The argument parser loop in src/workflow/builtin.rs
currently skips unrecognized tokens (inside the while loop handling args) which
silently ignores typos; change the loop so that when args[i] is not a known flag
(not "--reviewers" and not "--pause-after-step") it returns an error (e.g.,
bail! or Err) indicating an unknown argument and include the offending token
(args[i]) in the message instead of silently incrementing i; update the logic
around the reviewers parsing and pause_after_step handling to keep existing
behavior for known flags but fail fast for any other args[i].
In `@tests/extensibility_mvp.rs`:
- Around line 2165-2168: Replace the direct call to
commands::continue_session_with_registry(&ContinueArgs { session:
session.clone(), feedback: Some(...)} ) with a test invocation of the CLI using
run_cli to execute the equivalent "rally build continue" command so the
parser/dispatch path is covered; construct the CLI args to include the session
identifier and the feedback text (matching ContinueArgs), call run_cli(...) and
assert the same success outcome as the original direct call, referencing the
existing ContinueArgs, commands::continue_session_with_registry, and run_cli
helpers to locate where to change the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfa03659-e138-413c-96ea-5f090da570af
📒 Files selected for processing (7)
crates/rally-core/src/lib.rscrates/rally-workflow-build/src/lib.rssrc/cli.rssrc/commands.rssrc/lib.rssrc/workflow/builtin.rstests/extensibility_mvp.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/rally-workflow-build/src/lib.rs
- src/cli.rs
1 participant
Summary
--pause-after-stepflag to implement sessions, pausing for user feedback after each step approval instead of auto-advancingrally build continue --session <name> [--feedback "..."]to resume paused sessions with optional feedbackPausedsession phase: agents get exit code 2 (stop) when paused, and resume cleanly when continuedInvocation
Test plan
pause_after_step_pauses_and_continue_resumes— full lifecycle: create → step 1 → approve → verify Paused → continue with feedback → step 2 → FinalReview (no pause at end) → Done; also verifies continue errors on non-paused sessionspause_after_step_flag_parsed_by_run_command— verifies flag parsing in ImplementRunCommand::resolve()🤖 Generated with Claude Code
Summary by CodeRabbit