Auto-number reviewers, verb-form roles, and rally reset#18
Merged
justinmoon merged 3 commits intomasterfrom Mar 5, 2026
Merged
Auto-number reviewers, verb-form roles, and rally reset#18justinmoon merged 3 commits intomasterfrom
justinmoon merged 3 commits intomasterfrom
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRenames the default agent identifier from "implementer" to "implement", adds interactive Reset and workspace Context CLI commands, changes agent registration to return/auto-assign actual agent names (e.g., review-N), and updates calls/tests to use the returned agent name throughout run/join flows. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant CLI
end
rect rgba(200,255,200,0.5)
participant Registry
end
rect rgba(255,200,200,0.5)
participant FS
end
CLI->>Registry: ensure_agent_registered(session, requested_agent)
Registry-->>CLI: actual_agent (e.g., "review-1" or "implement")
CLI->>FS: create/join session files using actual_agent
CLI->>Registry: dispatch hooks with actual_agent
Registry-->>CLI: confirmation
sequenceDiagram
rect rgba(200,200,255,0.5)
participant User
end
rect rgba(200,255,200,0.5)
participant CLI
end
rect rgba(255,200,200,0.5)
participant FZF
end
rect rgba(200,255,200,0.5)
participant FS
end
User->>CLI: command reset()
CLI->>FS: list sessions (status, date, integrity)
CLI->>FZF: present selectable list
FZF-->>CLI: selected sessions
CLI->>FS: delete selected session dirs
FS-->>CLI: deletion results
CLI-->>User: report deletions
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 |
- join_with_registry returns Result<String> with the actual assigned agent name, enabling auto-numbering: "review" base name assigns review-1, review-2, etc. - Standardize all role names from noun forms (implementer/reviewer) to verb forms (implement/review) with no backward-compat fallbacks - Add `rally skill context` command for workspace intelligence - Update SKILL.md template, all tests, and e2e matrix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
justinmoon
force-pushed
the
fix/reviewer-auto-number
branch
from
764bd80 to
612d088
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/extensibility_mvp.rs (1)
102-109: Prefer using the reviewer name returned byjoin_with_registryinstead of hardcodingreview-1.This makes the test robust to reviewer auto-assignment behavior and directly exercises the new join contract.
♻️ Suggested refactor
- commands::join_with_registry( + let reviewer = commands::join_with_registry( &JoinArgs { session: build_session.clone(), - agent: "review-1".to_string(), + agent: "review".to_string(), timeout: None, }, ®istry, - )?; + )?; @@ - agent: "review-1".to_string(), + agent: reviewer.clone(), timeout: Some(2), @@ - "review-1", + &reviewer, "--verdict", "approve",Also applies to: 134-166, 169-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/extensibility_mvp.rs` around lines 102 - 109, The test currently hardcodes the agent name "review-1" when calling commands::join_with_registry; instead, capture and use the reviewer name returned by join_with_registry so the test exercises the join contract and handles auto-assignment. Update the call that passes a JoinArgs (with session: build_session.clone(), agent: "...", timeout: None) to call commands::join_with_registry and assign its returned reviewer identifier to a local variable, then use that variable wherever "review-1" is currently hardcoded (including the other join_with_registry call sites in this file) so the test uses the actual assigned reviewer name.prompts/skill-wrapper.md (1)
68-68: Fix punctuation in the instruction text (etc.).Line 68 should use
etc.in American English.Suggested edit
-3. **RUN** the follow-up commands listed at the end of your instruction (checkpoint, review, etc). Run them ALL, in order. +3. **RUN** the follow-up commands listed at the end of your instruction (checkpoint, review, etc.). Run them ALL, in order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/skill-wrapper.md` at line 68, Update the instruction text in the line that reads "3. **RUN** the follow-up commands listed at the end of your instruction (checkpoint, review, etc). Run them ALL, in order." to use American-English punctuation by ensuring "etc" is written as "etc." — i.e., change the occurrence of "etc" inside the parentheses to "etc." and keep surrounding punctuation consistent so the sentence reads correctly.
🤖 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/command_surface.rs`:
- Around line 258-277: The loop currently opens each session with
SessionHandle::open (which acquires an exclusive lock) and calls
handle.load_state(); instead, avoid locking by reading the session's state.json
directly and deserializing it for listing: replace the SessionHandle::open +
handle.load_state() sequence with a non-blocking read (std::fs::read_to_string
or similar) of the session's state file path and parse it via serde into the
same state type used by load_state (the struct returned by load_state), then use
that state to check workspace and print details; keep error handling (skip
unreadable/parse-failed sessions) and preserve the use of
crate::state::list_sessions(), state.agents, state.name, state.session_type,
state.phase, and state.config.expected_agents when formatting output.
In `@src/commands.rs`:
- Around line 560-561: The session-name extraction is losing internal spaces
because it uses line.split_whitespace().next(); fix the parsing for session_name
by taking the remainder of the input (not just the first token). Replace the
split_whitespace().next() logic used to compute session_name with a
delimiter-safe approach (e.g., use splitn(2, |c: char|
c.is_whitespace()).nth(1).unwrap_or("").trim() or simply take line.trim() if the
whole line is the session name) so names like "feature sync" are preserved;
update the code where session_name is computed to use this new approach.
- Around line 505-516: The loop currently calls SessionHandle::open (which
acquires an exclusive lock) and handle.load_state, causing reset to hang if
another process holds the lock; change the logic to attempt a non-blocking open
(e.g., a try_open/try_open_readonly/open_nonblocking variant on SessionHandle)
and if that fails, push a placeholder entry like "(locked)" into entries and
continue without blocking, deferring any heavy or locking state loads to after
the fzf selection or to a background task; update references to
SessionHandle::open and handle.load_state accordingly so the list preparation
never waits on an exclusive lock.
- Around line 195-205: The current logic computing actual_agent for args.agent
== "review" can return the literal "review" when all numbered slots are taken
(state.config.expected_agents), risking key collisions in state.agents; change
the fallback so it does not return args.agent.clone() but instead produces a
guaranteed-unique name (e.g., format!("review-{}", Uuid::new_v4()) or append a
timestamp) and use that as actual_agent; update the block that builds name (the
loop that builds review-{n}, the unwrap_or_else) to create and return the
unique-suffixed name (or propagate an explicit error) so no two joiners can map
to the shared "review" key.
---
Nitpick comments:
In `@prompts/skill-wrapper.md`:
- Line 68: Update the instruction text in the line that reads "3. **RUN** the
follow-up commands listed at the end of your instruction (checkpoint, review,
etc). Run them ALL, in order." to use American-English punctuation by ensuring
"etc" is written as "etc." — i.e., change the occurrence of "etc" inside the
parentheses to "etc." and keep surrounding punctuation consistent so the
sentence reads correctly.
In `@tests/extensibility_mvp.rs`:
- Around line 102-109: The test currently hardcodes the agent name "review-1"
when calling commands::join_with_registry; instead, capture and use the reviewer
name returned by join_with_registry so the test exercises the join contract and
handles auto-assignment. Update the call that passes a JoinArgs (with session:
build_session.clone(), agent: "...", timeout: None) to call
commands::join_with_registry and assign its returned reviewer identifier to a
local variable, then use that variable wherever "review-1" is currently
hardcoded (including the other join_with_registry call sites in this file) so
the test uses the actual assigned reviewer name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bef0be2e-696d-42f4-8906-56ea40ed0412
📒 Files selected for processing (12)
crates/rally-workflow-build/src/lib.rsprompts/skill-wrapper.mdsrc/cli.rssrc/command_surface.rssrc/commands.rssrc/lib.rssrc/workflow/builtin.rssrc/workflow/interop.rssrc/workflow/mod.rstests/command_install_run.rstests/e2e_matrix.pytests/extensibility_mvp.rs
Comment on lines
+258
to
+277
| let sessions = crate::state::list_sessions().unwrap_or_default(); | ||
| let mut found_sessions = false; | ||
| for session_name in &sessions { | ||
| if let Ok(handle) = SessionHandle::open(session_name) { | ||
| if let Ok(state) = handle.load_state() { | ||
| if state.workspace.as_deref() == Some(&workspace_str) { | ||
| let agents: Vec<_> = state.agents.keys().cloned().collect(); | ||
| println!( | ||
| "- **{}** (type: {:?}, phase: {:?}, agents: {}/{}, registered: [{}])", | ||
| state.name, | ||
| state.session_type, | ||
| state.phase, | ||
| state.agents.len(), | ||
| state.config.expected_agents, | ||
| agents.join(", ") | ||
| ); | ||
| found_sessions = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid exclusive session locks in read-only skill context listing.
Line 261 calls SessionHandle::open, which takes an exclusive lock; this can block rally skill context while another command is holding a session lock. For listing, prefer a non-locking read of state.json.
Possible non-blocking approach
- for session_name in &sessions {
- if let Ok(handle) = SessionHandle::open(session_name) {
- if let Ok(state) = handle.load_state() {
- if state.workspace.as_deref() == Some(&workspace_str) {
- let agents: Vec<_> = state.agents.keys().cloned().collect();
- println!(
- "- **{}** (type: {:?}, phase: {:?}, agents: {}/{}, registered: [{}])",
- state.name,
- state.session_type,
- state.phase,
- state.agents.len(),
- state.config.expected_agents,
- agents.join(", ")
- );
- found_sessions = true;
- }
- }
- }
- }
+ for session_name in &sessions {
+ let state_path = crate::state::session_dir(session_name).join("state.json");
+ let Ok(raw) = fs::read_to_string(&state_path) else {
+ continue;
+ };
+ let Ok(state) = serde_json::from_str::<crate::state::SessionState>(&raw) else {
+ continue;
+ };
+ if state.workspace.as_deref() == Some(&workspace_str) {
+ let agents: Vec<_> = state.agents.keys().cloned().collect();
+ println!(
+ "- **{}** (type: {:?}, phase: {:?}, agents: {}/{}, registered: [{}])",
+ state.name,
+ state.session_type,
+ state.phase,
+ state.agents.len(),
+ state.config.expected_agents,
+ agents.join(", ")
+ );
+ found_sessions = true;
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command_surface.rs` around lines 258 - 277, The loop currently opens each
session with SessionHandle::open (which acquires an exclusive lock) and calls
handle.load_state(); instead, avoid locking by reading the session's state.json
directly and deserializing it for listing: replace the SessionHandle::open +
handle.load_state() sequence with a non-blocking read (std::fs::read_to_string
or similar) of the session's state file path and parse it via serde into the
same state type used by load_state (the struct returned by load_state), then use
that state to check workspace and print details; keep error handling (skip
unreadable/parse-failed sessions) and preserve the use of
crate::state::list_sessions(), state.agents, state.name, state.session_type,
state.phase, and state.config.expected_agents when formatting output.
Comment on lines
+505
to
+516
| let handle = match SessionHandle::open(name) { | ||
| Ok(h) => h, | ||
| Err(_) => { | ||
| entries.push((name.clone(), String::new(), format!("{:<40} (corrupt)", name))); | ||
| continue; | ||
| } | ||
| }; | ||
| let state = match handle.load_state() { | ||
| Ok(s) => s, | ||
| Err(_) => { | ||
| entries.push((name.clone(), String::new(), format!("{:<40} (corrupt)", name))); | ||
| continue; |
There was a problem hiding this comment.
reset can hang while preparing the fzf list.
Line 505 uses SessionHandle::open for each session, and that path takes an exclusive lock. If another process is active, reset can stall before showing the picker.
Non-blocking display-state load sketch
- let handle = match SessionHandle::open(name) {
- Ok(h) => h,
- Err(_) => {
- entries.push((name.clone(), String::new(), format!("{:<40} (corrupt)", name)));
- continue;
- }
- };
- let state = match handle.load_state() {
- Ok(s) => s,
- Err(_) => {
- entries.push((name.clone(), String::new(), format!("{:<40} (corrupt)", name)));
- continue;
- }
- };
+ let state_path = session_dir(name).join("state.json");
+ let state = match fs::read_to_string(&state_path)
+ .ok()
+ .and_then(|raw| serde_json::from_str::<SessionState>(&raw).ok())
+ {
+ Some(s) => s,
+ None => {
+ entries.push((name.clone(), String::new(), format!("{:<40} (corrupt)", name)));
+ continue;
+ }
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands.rs` around lines 505 - 516, The loop currently calls
SessionHandle::open (which acquires an exclusive lock) and handle.load_state,
causing reset to hang if another process holds the lock; change the logic to
attempt a non-blocking open (e.g., a try_open/try_open_readonly/open_nonblocking
variant on SessionHandle) and if that fails, push a placeholder entry like
"(locked)" into entries and continue without blocking, deferring any heavy or
locking state loads to after the fzf selection or to a background task; update
references to SessionHandle::open and handle.load_state accordingly so the list
preparation never waits on an exclusive lock.
Comment on lines
+560
to
+561
| let session_name = line.split_whitespace().next().unwrap_or("").trim(); | ||
| if session_name.is_empty() { |
There was a problem hiding this comment.
Session-name parsing is lossy for whitespace-containing names.
Line 560 uses split_whitespace().next(), so a selected session like "feature sync" resolves to "feature" and may delete the wrong directory.
Delimiter-safe parsing option
- let line = format!(
- "{:<40} {:>2} agents {:?} {}",
- name, agent_count, state.phase, most_recent.format("%Y-%m-%d %H:%M")
- );
+ let line = format!(
+ "{}\t{:>2} agents {:?} {}",
+ name, agent_count, state.phase, most_recent.format("%Y-%m-%d %H:%M")
+ );
...
- let session_name = line.split_whitespace().next().unwrap_or("").trim();
+ let session_name = line.split('\t').next().unwrap_or("").trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands.rs` around lines 560 - 561, The session-name extraction is
losing internal spaces because it uses line.split_whitespace().next(); fix the
parsing for session_name by taking the remainder of the input (not just the
first token). Replace the split_whitespace().next() logic used to compute
session_name with a delimiter-safe approach (e.g., use splitn(2, |c: char|
c.is_whitespace()).nth(1).unwrap_or("").trim() or simply take line.trim() if the
whole line is the session name) so names like "feature sync" are preserved;
update the code where session_name is computed to use this new approach.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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
join_with_registryreturns the actual assigned agent name, enabling auto-numbering:reviewbase name assignsreview-1,review-2, etc.implementer/reviewer) to verb forms (implement/review) — hard switch, no backward compatrally skill contextcommand for workspace intelligencerally reset— interactive fzf session picker for deleting sessions (multi-select with TAB)Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation