From a0a1c3d970f83d162b8cf7a3071ca53189a54b63 Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Sun, 1 Mar 2026 23:37:21 -0600 Subject: [PATCH 01/11] Add command namespace CLI and run/exec contract scaffolding --- src/cli.rs | 91 ++++++++++++++++++++++++++++++++++++++++++ src/command_surface.rs | 22 ++++++++++ src/lib.rs | 14 ++++++- 3 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 src/command_surface.rs diff --git a/src/cli.rs b/src/cli.rs index 1229783..ca78c20 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -24,6 +24,7 @@ COMMAND GROUPS: Core loop: create, join, next, done, status, sessions Canonical built-ins: plan ..., build ... Generic workflows: workflow list, workflow action + Wrapper surface: command install|run|exec|doctor|uninstall Compatibility aliases (deprecated): file-issue, challenge, agree, checkpoint, review Handoff: chain @@ -79,6 +80,8 @@ pub enum Command { Build(BuildCommandArgs), #[command(about = "Generic workflow commands")] Workflow(WorkflowCommandArgs), + #[command(about = "Install and run short `/rally ...` wrapper commands")] + Command(CommandCommandArgs), } #[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)] @@ -286,3 +289,91 @@ pub struct WorkflowActionArgs { #[arg(long, help = "Path to a JSON file containing action arguments")] pub args_file: Option, } + +#[derive(Args, Debug)] +pub struct CommandCommandArgs { + #[command(subcommand)] + pub command: CommandSubcommand, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq, ValueEnum)] +pub enum CommandTargetArg { + Codex, + Droid, + Pi, + ClaudeCode, + Factory, + All, +} + +#[derive(Subcommand, Debug)] +pub enum CommandSubcommand { + #[command(about = "Install Rally-managed wrapper artifacts")] + Install(CommandInstallArgs), + #[command( + about = "Wrapper execution entrypoint", + long_about = "Wrapper execution entrypoint.\n\nArgument contract:\n rally command run\n rally command run \n rally command run \n\nResolution precedence (implemented by this command family):\n 1) explicit args\n 2) invite token\n 3) saved context\n 4) interactive fallback" + )] + Run(CommandRunArgs), + #[command( + about = "Alias for `run` (identical behavior and argument contract)", + long_about = "Alias for `run` with identical behavior.\n\nArgument contract:\n rally command exec\n rally command exec \n rally command exec \n\nResolution precedence (implemented by this command family):\n 1) explicit args\n 2) invite token\n 3) saved context\n 4) interactive fallback" + )] + Exec(CommandRunArgs), + #[command(about = "Validate wrapper installation and target paths")] + Doctor(CommandDoctorArgs), + #[command(about = "Uninstall Rally-managed wrapper artifacts")] + Uninstall(CommandUninstallArgs), +} + +#[derive(Args, Debug)] +pub struct CommandInstallArgs { + #[arg(long, value_enum, help = "Install target harness")] + pub target: CommandTargetArg, + #[arg(long, help = "Overwrite existing non-managed files")] + pub force: bool, + #[arg(long, help = "Preview writes without touching files")] + pub dry_run: bool, +} + +#[derive(Args, Debug, Clone)] +pub struct CommandRunArgs { + #[arg( + value_name = "FIRST", + help = "Optional workflow shorthand (for example `build`) or invite token" + )] + pub first: Option, + #[arg(value_name = "SECOND", help = "Optional role/agent selector")] + pub second: Option, + #[arg( + value_name = "EXTRA", + trailing_var_arg = true, + allow_hyphen_values = true, + help = "Additional wrapper tokens passed to run-resolution logic" + )] + pub extra: Vec, + #[arg(long, help = "Explicit session override")] + pub session: Option, + #[arg(long = "as", help = "Explicit agent/role override")] + pub agent: Option, + #[arg(long, help = "Explicit invite token override")] + pub invite: Option, + #[arg(long, help = "Disable interactive fallback when unresolved")] + pub non_interactive: bool, +} + +#[derive(Args, Debug)] +pub struct CommandDoctorArgs { + #[arg(long, value_enum, default_value = "all", help = "Target harness to inspect")] + pub target: CommandTargetArg, + #[arg(long, help = "Print checks without changing filesystem")] + pub dry_run: bool, +} + +#[derive(Args, Debug)] +pub struct CommandUninstallArgs { + #[arg(long, value_enum, help = "Uninstall target harness")] + pub target: CommandTargetArg, + #[arg(long, help = "Preview removals without touching files")] + pub dry_run: bool, +} diff --git a/src/command_surface.rs b/src/command_surface.rs new file mode 100644 index 0000000..9ef055c --- /dev/null +++ b/src/command_surface.rs @@ -0,0 +1,22 @@ +use anyhow::{Result, bail}; + +use crate::{ + WorkflowRegistry, + cli::{CommandDoctorArgs, CommandInstallArgs, CommandRunArgs, CommandUninstallArgs}, +}; + +pub fn install(_args: &CommandInstallArgs) -> Result<()> { + bail!("`rally command install` is not implemented yet") +} + +pub fn run(_args: &CommandRunArgs, _registry: &WorkflowRegistry) -> Result<()> { + bail!("`rally command run` is wired but not implemented yet") +} + +pub fn doctor(_args: &CommandDoctorArgs) -> Result<()> { + bail!("`rally command doctor` is not implemented yet") +} + +pub fn uninstall(_args: &CommandUninstallArgs) -> Result<()> { + bail!("`rally command uninstall` is not implemented yet") +} diff --git a/src/lib.rs b/src/lib.rs index 6b2d69b..83dc51f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,4 +1,5 @@ pub mod cli; +pub mod command_surface; pub mod commands; pub mod state; pub mod watch; @@ -6,7 +7,7 @@ pub mod workflow; use anyhow::Result; use clap::Parser; -use cli::{BuildSubcommand, Cli, Command, PlanSubcommand, WorkflowSubcommand}; +use cli::{BuildSubcommand, Cli, Command, CommandSubcommand, PlanSubcommand, WorkflowSubcommand}; pub use workflow::{Workflow, WorkflowId, WorkflowRegistry}; @@ -92,5 +93,16 @@ pub fn run_cli_with_registry(cli: Cli, registry: &WorkflowRegistry) -> Result { + match args.command { + CommandSubcommand::Install(inner) => command_surface::install(&inner)?, + CommandSubcommand::Run(inner) | CommandSubcommand::Exec(inner) => { + command_surface::run(&inner, registry)? + } + CommandSubcommand::Doctor(inner) => command_surface::doctor(&inner)?, + CommandSubcommand::Uninstall(inner) => command_surface::uninstall(&inner)?, + } + Ok(0) + } } } From 2511f0d8f22b76466e417ae7f8278773e05ce880 Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Sun, 1 Mar 2026 23:41:43 -0600 Subject: [PATCH 02/11] Add target adapter layer for command wrapper discovery --- src/command_surface.rs | 271 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 267 insertions(+), 4 deletions(-) diff --git a/src/command_surface.rs b/src/command_surface.rs index 9ef055c..2a51075 100644 --- a/src/command_surface.rs +++ b/src/command_surface.rs @@ -1,12 +1,36 @@ -use anyhow::{Result, bail}; +use std::{env, fmt, path::PathBuf}; + +use anyhow::{Result, anyhow, bail}; use crate::{ WorkflowRegistry, - cli::{CommandDoctorArgs, CommandInstallArgs, CommandRunArgs, CommandUninstallArgs}, + cli::{ + CommandDoctorArgs, CommandInstallArgs, CommandRunArgs, CommandTargetArg, + CommandUninstallArgs, + }, }; -pub fn install(_args: &CommandInstallArgs) -> Result<()> { - bail!("`rally command install` is not implemented yet") +const RALLY_DEFAULT_BIN: &str = "rally"; + +pub fn install(args: &CommandInstallArgs) -> Result<()> { + let env = AdapterEnvironment::detect()?; + let adapters = adapters_for_target(args.target); + let render_ctx = WrapperRenderContext::default(); + let mut planned = Vec::new(); + for adapter in adapters { + let path = adapter.discover_install_path(&env)?; + let content = adapter.render_wrapper(&render_ctx); + planned.push((adapter.target(), path, content)); + } + + for (target, path, _) in &planned { + if args.dry_run { + println!("[dry-run] target={target} wrapper={}", path.display()); + } else { + println!("target={target} wrapper={}", path.display()); + } + } + Ok(()) } pub fn run(_args: &CommandRunArgs, _registry: &WorkflowRegistry) -> Result<()> { @@ -20,3 +44,242 @@ pub fn doctor(_args: &CommandDoctorArgs) -> Result<()> { pub fn uninstall(_args: &CommandUninstallArgs) -> Result<()> { bail!("`rally command uninstall` is not implemented yet") } + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum HarnessTarget { + Codex, + Droid, + Pi, + ClaudeCode, + Factory, +} + +impl HarnessTarget { + fn all() -> Vec { + vec![ + Self::Codex, + Self::Droid, + Self::Pi, + Self::ClaudeCode, + Self::Factory, + ] + } + + fn slug(self) -> &'static str { + match self { + Self::Codex => "codex", + Self::Droid => "droid", + Self::Pi => "pi", + Self::ClaudeCode => "claude-code", + Self::Factory => "factory", + } + } + + fn env_var(self) -> &'static str { + match self { + Self::Codex => "RALLY_CODEX_HOME", + Self::Droid => "RALLY_DROID_HOME", + Self::Pi => "RALLY_PI_HOME", + Self::ClaudeCode => "RALLY_CLAUDE_CODE_HOME", + Self::Factory => "RALLY_FACTORY_HOME", + } + } + + fn default_root(self, home: &std::path::Path) -> PathBuf { + match self { + Self::Codex => home.join(".codex"), + Self::Droid => home.join(".droid"), + Self::Pi => home.join(".pi"), + Self::ClaudeCode => home.join(".claude"), + Self::Factory => home.join(".factory"), + } + } + + fn wrapper_path(self, root: &std::path::Path) -> PathBuf { + root.join("commands").join("rally.md") + } + + fn metadata(self) -> &'static str { + match self { + Self::Codex => "codex-slash-command", + Self::Droid => "droid-slash-command", + Self::Pi => "pi-slash-command", + Self::ClaudeCode => "claude-code-slash-command", + Self::Factory => "factory-slash-command", + } + } +} + +impl fmt::Display for HarnessTarget { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.slug()) + } +} + +#[derive(Clone, Debug)] +struct AdapterEnvironment { + home_dir: PathBuf, +} + +impl AdapterEnvironment { + fn detect() -> Result { + let home_dir = dirs::home_dir().ok_or_else(|| anyhow!("unable to resolve home directory"))?; + Ok(Self { home_dir }) + } + + fn root_for(&self, target: HarnessTarget) -> Result { + if let Some(value) = env::var_os(target.env_var()) { + return Ok(PathBuf::from(value)); + } + Ok(target.default_root(&self.home_dir)) + } +} + +#[derive(Clone, Debug)] +struct WrapperRenderContext { + rally_bin: String, +} + +impl Default for WrapperRenderContext { + fn default() -> Self { + Self { + rally_bin: RALLY_DEFAULT_BIN.to_string(), + } + } +} + +trait TargetAdapter { + fn target(&self) -> HarnessTarget; + fn discover_install_path(&self, env: &AdapterEnvironment) -> Result; + fn render_wrapper(&self, ctx: &WrapperRenderContext) -> String; +} + +struct DotDirAdapter { + target: HarnessTarget, +} + +impl DotDirAdapter { + fn new(target: HarnessTarget) -> Self { + Self { target } + } +} + +impl TargetAdapter for DotDirAdapter { + fn target(&self) -> HarnessTarget { + self.target + } + + fn discover_install_path(&self, env: &AdapterEnvironment) -> Result { + let root = env.root_for(self.target)?; + if !root.exists() { + bail!( + "target '{}' environment not found at {}. Install the harness first or set {} to the harness home directory", + self.target, + root.display(), + self.target.env_var() + ); + } + if !root.is_dir() { + bail!( + "target '{}' path {} is not a directory. Set {} to a valid harness home directory", + self.target, + root.display(), + self.target.env_var() + ); + } + + Ok(self.target.wrapper_path(&root)) + } + + fn render_wrapper(&self, ctx: &WrapperRenderContext) -> String { + format!( + "\n\n\n# /rally\n\nManaged by Rally. Do not edit by hand.\n\n```bash\n{} command run \"$@\"\n```\n", + self.target, + self.target.metadata(), + ctx.rally_bin + ) + } +} + +fn adapter_for(target: HarnessTarget) -> Box { + Box::new(DotDirAdapter::new(target)) +} + +fn adapters_for_target(target: CommandTargetArg) -> Vec> { + match target { + CommandTargetArg::All => HarnessTarget::all().into_iter().map(adapter_for).collect(), + CommandTargetArg::Codex => vec![adapter_for(HarnessTarget::Codex)], + CommandTargetArg::Droid => vec![adapter_for(HarnessTarget::Droid)], + CommandTargetArg::Pi => vec![adapter_for(HarnessTarget::Pi)], + CommandTargetArg::ClaudeCode => vec![adapter_for(HarnessTarget::ClaudeCode)], + CommandTargetArg::Factory => vec![adapter_for(HarnessTarget::Factory)], + } +} + +#[cfg(test)] +mod tests { + use std::{fs, path::Path, time::{SystemTime, UNIX_EPOCH}}; + + use anyhow::Context; + + use super::*; + + fn unique_dir(prefix: &str) -> PathBuf { + let ts = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("time") + .as_nanos(); + std::env::temp_dir().join(format!("{prefix}-{ts}")) + } + + fn env_with_home(home: &Path) -> AdapterEnvironment { + AdapterEnvironment { + home_dir: home.to_path_buf(), + } + } + + #[test] + fn all_target_expands_to_all_adapters() { + let adapters = adapters_for_target(CommandTargetArg::All); + let names = adapters + .into_iter() + .map(|a| a.target().to_string()) + .collect::>(); + assert_eq!( + names, + vec!["codex", "droid", "pi", "claude-code", "factory"] + ); + } + + #[test] + fn discover_install_path_reports_missing_environment() { + let home = unique_dir("rally-command-adapter-missing"); + let adapter = DotDirAdapter::new(HarnessTarget::Codex); + let err = adapter + .discover_install_path(&env_with_home(&home)) + .expect_err("missing root should fail"); + let msg = err.to_string(); + assert!(msg.contains("target 'codex' environment not found")); + assert!(msg.contains("RALLY_CODEX_HOME")); + } + + #[test] + fn discover_install_path_uses_default_dot_directory_layout() -> Result<()> { + let home = unique_dir("rally-command-adapter-layout"); + let root = home.join(".droid"); + fs::create_dir_all(&root) + .with_context(|| format!("creating {}", root.display()))?; + let adapter = DotDirAdapter::new(HarnessTarget::Droid); + let path = adapter.discover_install_path(&env_with_home(&home))?; + assert_eq!(path, root.join("commands").join("rally.md")); + Ok(()) + } + + #[test] + fn wrapper_rendering_delegates_to_command_run() { + let adapter = DotDirAdapter::new(HarnessTarget::Factory); + let rendered = adapter.render_wrapper(&WrapperRenderContext::default()); + assert!(rendered.contains("rally command run \"$@\"")); + assert!(rendered.contains("rally-target: factory")); + } +} From 9129f2ff7737a06aa7f7715744577d5a41584997 Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Sun, 1 Mar 2026 23:42:59 -0600 Subject: [PATCH 03/11] Implement safe idempotent wrapper install operations --- src/command_surface.rs | 241 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 228 insertions(+), 13 deletions(-) diff --git a/src/command_surface.rs b/src/command_surface.rs index 2a51075..256f80d 100644 --- a/src/command_surface.rs +++ b/src/command_surface.rs @@ -1,6 +1,6 @@ -use std::{env, fmt, path::PathBuf}; +use std::{env, fmt, fs, path::PathBuf}; -use anyhow::{Result, anyhow, bail}; +use anyhow::{Context, Result, anyhow, bail}; use crate::{ WorkflowRegistry, @@ -11,23 +11,27 @@ use crate::{ }; const RALLY_DEFAULT_BIN: &str = "rally"; +const RALLY_MANAGED_MARKER: &str = ""; pub fn install(args: &CommandInstallArgs) -> Result<()> { let env = AdapterEnvironment::detect()?; - let adapters = adapters_for_target(args.target); let render_ctx = WrapperRenderContext::default(); - let mut planned = Vec::new(); - for adapter in adapters { - let path = adapter.discover_install_path(&env)?; - let content = adapter.render_wrapper(&render_ctx); - planned.push((adapter.target(), path, content)); - } - - for (target, path, _) in &planned { + let outcomes = install_with_environment(args, &env, &render_ctx)?; + for outcome in outcomes { if args.dry_run { - println!("[dry-run] target={target} wrapper={}", path.display()); + println!( + "[dry-run] {} target={} wrapper={}", + outcome.action, + outcome.target, + outcome.path.display() + ); } else { - println!("target={target} wrapper={}", path.display()); + println!( + "{} target={} wrapper={}", + outcome.action, + outcome.target, + outcome.path.display() + ); } } Ok(()) @@ -148,6 +152,40 @@ impl Default for WrapperRenderContext { } } +#[derive(Clone, Debug)] +struct WrapperArtifact { + target: HarnessTarget, + path: PathBuf, + content: String, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum InstallAction { + Create, + UpdateManaged, + OverwriteForced, + Noop, +} + +impl fmt::Display for InstallAction { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let label = match self { + Self::Create => "create", + Self::UpdateManaged => "update-managed", + Self::OverwriteForced => "overwrite-forced", + Self::Noop => "noop", + }; + f.write_str(label) + } +} + +#[derive(Clone, Debug)] +struct InstallOutcome { + target: HarnessTarget, + path: PathBuf, + action: InstallAction, +} + trait TargetAdapter { fn target(&self) -> HarnessTarget; fn discover_install_path(&self, env: &AdapterEnvironment) -> Result; @@ -216,6 +254,95 @@ fn adapters_for_target(target: CommandTargetArg) -> Vec> } } +fn install_with_environment( + args: &CommandInstallArgs, + env: &AdapterEnvironment, + render_ctx: &WrapperRenderContext, +) -> Result> { + let artifacts = planned_wrappers(args.target, env, render_ctx)?; + let mut plans = Vec::new(); + for artifact in artifacts { + let action = classify_install_action(&artifact.path, &artifact.content, args.force)?; + plans.push((artifact, action)); + } + + let mut outcomes = Vec::new(); + for (artifact, action) in plans { + if !args.dry_run { + apply_install_action(&artifact, action)?; + } + outcomes.push(InstallOutcome { + target: artifact.target, + path: artifact.path, + action, + }); + } + + Ok(outcomes) +} + +fn planned_wrappers( + target: CommandTargetArg, + env: &AdapterEnvironment, + render_ctx: &WrapperRenderContext, +) -> Result> { + let mut out = Vec::new(); + for adapter in adapters_for_target(target) { + let path = adapter.discover_install_path(env)?; + let content = adapter.render_wrapper(render_ctx); + out.push(WrapperArtifact { + target: adapter.target(), + path, + content, + }); + } + Ok(out) +} + +fn classify_install_action(path: &std::path::Path, content: &str, force: bool) -> Result { + if !path.exists() { + return Ok(InstallAction::Create); + } + + let current = fs::read(path).with_context(|| format!("reading existing wrapper {}", path.display()))?; + if current == content.as_bytes() { + return Ok(InstallAction::Noop); + } + + if contains_managed_marker(¤t) { + return Ok(InstallAction::UpdateManaged); + } + + if force { + return Ok(InstallAction::OverwriteForced); + } + + bail!( + "refusing to overwrite non-Rally file at {}. Re-run with --force to replace it", + path.display() + ) +} + +fn apply_install_action(artifact: &WrapperArtifact, action: InstallAction) -> Result<()> { + match action { + InstallAction::Noop => Ok(()), + InstallAction::Create | InstallAction::UpdateManaged | InstallAction::OverwriteForced => { + if let Some(parent) = artifact.path.parent() { + fs::create_dir_all(parent) + .with_context(|| format!("creating {}", parent.display()))?; + } + fs::write(&artifact.path, &artifact.content) + .with_context(|| format!("writing {}", artifact.path.display())) + } + } +} + +fn contains_managed_marker(content: &[u8]) -> bool { + content + .windows(RALLY_MANAGED_MARKER.len()) + .any(|window| window == RALLY_MANAGED_MARKER.as_bytes()) +} + #[cfg(test)] mod tests { use std::{fs, path::Path, time::{SystemTime, UNIX_EPOCH}}; @@ -282,4 +409,92 @@ mod tests { assert!(rendered.contains("rally command run \"$@\"")); assert!(rendered.contains("rally-target: factory")); } + + fn prepare_target_root(home: &Path, target: HarnessTarget) -> Result { + let root = target.default_root(home); + fs::create_dir_all(&root)?; + Ok(root) + } + + fn install_args(target: CommandTargetArg, force: bool, dry_run: bool) -> CommandInstallArgs { + CommandInstallArgs { + target, + force, + dry_run, + } + } + + #[test] + fn install_writes_wrapper_and_is_idempotent() -> Result<()> { + let home = unique_dir("rally-command-install-idempotent"); + let root = prepare_target_root(&home, HarnessTarget::Codex)?; + let env = env_with_home(&home); + let render_ctx = WrapperRenderContext::default(); + + let first = install_with_environment( + &install_args(CommandTargetArg::Codex, false, false), + &env, + &render_ctx, + )?; + assert_eq!(first.len(), 1); + assert_eq!(first[0].action, InstallAction::Create); + + let wrapper = root.join("commands").join("rally.md"); + let content = fs::read_to_string(&wrapper)?; + assert!(content.contains("rally command run \"$@\"")); + assert!(content.contains(RALLY_MANAGED_MARKER)); + + let second = install_with_environment( + &install_args(CommandTargetArg::Codex, false, false), + &env, + &render_ctx, + )?; + assert_eq!(second.len(), 1); + assert_eq!(second[0].action, InstallAction::Noop); + Ok(()) + } + + #[test] + fn install_refuses_to_overwrite_unknown_file_without_force() -> Result<()> { + let home = unique_dir("rally-command-install-no-force"); + let root = prepare_target_root(&home, HarnessTarget::Pi)?; + let wrapper = root.join("commands").join("rally.md"); + fs::create_dir_all(wrapper.parent().expect("wrapper parent"))?; + fs::write(&wrapper, "custom wrapper")?; + let env = env_with_home(&home); + let render_ctx = WrapperRenderContext::default(); + + let err = install_with_environment( + &install_args(CommandTargetArg::Pi, false, false), + &env, + &render_ctx, + ) + .expect_err("unknown file should not be overwritten"); + let msg = err.to_string(); + assert!(msg.contains("refusing to overwrite non-Rally file")); + assert!(msg.contains("--force")); + assert_eq!(fs::read_to_string(&wrapper)?, "custom wrapper"); + Ok(()) + } + + #[test] + fn install_overwrites_unknown_file_with_force() -> Result<()> { + let home = unique_dir("rally-command-install-force"); + let root = prepare_target_root(&home, HarnessTarget::ClaudeCode)?; + let wrapper = root.join("commands").join("rally.md"); + fs::create_dir_all(wrapper.parent().expect("wrapper parent"))?; + fs::write(&wrapper, "custom wrapper")?; + let env = env_with_home(&home); + let render_ctx = WrapperRenderContext::default(); + + let outcome = install_with_environment( + &install_args(CommandTargetArg::ClaudeCode, true, false), + &env, + &render_ctx, + )?; + assert_eq!(outcome[0].action, InstallAction::OverwriteForced); + let content = fs::read_to_string(&wrapper)?; + assert!(content.contains(RALLY_MANAGED_MARKER)); + Ok(()) + } } From 7da972397e68de3c759561214eeab7da1cb80ce9 Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Sun, 1 Mar 2026 23:45:40 -0600 Subject: [PATCH 04/11] Implement command run context resolution with persistence --- src/command_surface.rs | 417 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 412 insertions(+), 5 deletions(-) diff --git a/src/command_surface.rs b/src/command_surface.rs index 256f80d..0a5442f 100644 --- a/src/command_surface.rs +++ b/src/command_surface.rs @@ -1,17 +1,28 @@ -use std::{env, fmt, fs, path::PathBuf}; +use std::{ + collections::BTreeMap, + env, fmt, fs, + io::{self, Write}, + path::{Path, PathBuf}, +}; use anyhow::{Context, Result, anyhow, bail}; +use serde::{Deserialize, Serialize}; use crate::{ WorkflowRegistry, cli::{ - CommandDoctorArgs, CommandInstallArgs, CommandRunArgs, CommandTargetArg, + CommandDoctorArgs, CommandInstallArgs, CommandRunArgs, CommandTargetArg, JoinArgs, CommandUninstallArgs, }, + commands, + state::SessionHandle, + watch, }; const RALLY_DEFAULT_BIN: &str = "rally"; const RALLY_MANAGED_MARKER: &str = ""; +const CONTEXT_STORE_FILE: &str = "command-context.json"; +const DEFAULT_RUN_TIMEOUT_SECS: u64 = 300; pub fn install(args: &CommandInstallArgs) -> Result<()> { let env = AdapterEnvironment::detect()?; @@ -37,8 +48,62 @@ pub fn install(args: &CommandInstallArgs) -> Result<()> { Ok(()) } -pub fn run(_args: &CommandRunArgs, _registry: &WorkflowRegistry) -> Result<()> { - bail!("`rally command run` is wired but not implemented yet") +pub fn run(args: &CommandRunArgs, registry: &WorkflowRegistry) -> Result<()> { + let workspace = workspace_key()?; + let store_path = context_store_path()?; + let mut store = load_context_store(&store_path)?; + let saved = store.by_workspace.get(&workspace).cloned(); + + let explicit_invite = args + .invite + .as_deref() + .and_then(parse_invite_token) + .or_else(|| args.first.as_deref().and_then(parse_invite_token)); + let first_is_invite = args + .first + .as_deref() + .is_some_and(|value| parse_invite_token(value).is_some()); + let workflow_selector = if first_is_invite { + None + } else { + args.first.clone() + }; + + let mut prompter = StdinPrompter; + let resolved = resolve_run_context( + args, + workflow_selector, + args.second.clone(), + explicit_invite, + saved.as_ref(), + &mut prompter, + )?; + + ensure_agent_registered(&resolved.session, &resolved.agent, registry)?; + let code = watch::next_with_registry( + &resolved.session, + &resolved.agent, + Some(DEFAULT_RUN_TIMEOUT_SECS), + registry, + )?; + if code == 1 { + println!( + "No instruction became available within {DEFAULT_RUN_TIMEOUT_SECS}s. Re-run `/rally`." + ); + } else if code == 2 { + println!("session complete; stop working"); + } + + store.by_workspace.insert( + workspace, + SavedRunContext { + session: resolved.session, + agent: resolved.agent, + workflow: resolved.workflow, + }, + ); + save_context_store(&store_path, &store)?; + Ok(()) } pub fn doctor(_args: &CommandDoctorArgs) -> Result<()> { @@ -186,6 +251,51 @@ struct InstallOutcome { action: InstallAction, } +#[derive(Clone, Debug)] +struct InviteContext { + session: String, + agent: String, +} + +#[derive(Clone, Debug)] +struct ResolvedRunContext { + session: String, + agent: String, + workflow: Option, +} + +#[derive(Clone, Debug, Default, Serialize, Deserialize)] +struct RunContextStore { + #[serde(default)] + by_workspace: BTreeMap, +} + +#[derive(Clone, Debug, Serialize, Deserialize)] +struct SavedRunContext { + session: String, + agent: String, + #[serde(default)] + workflow: Option, +} + +trait Prompter { + fn prompt(&mut self, label: &str) -> Result; +} + +struct StdinPrompter; + +impl Prompter for StdinPrompter { + fn prompt(&mut self, label: &str) -> Result { + let mut input = String::new(); + print!("{label}: "); + io::stdout().flush().context("flushing prompt")?; + io::stdin() + .read_line(&mut input) + .context("reading interactive input")?; + Ok(input.trim().to_string()) + } +} + trait TargetAdapter { fn target(&self) -> HarnessTarget; fn discover_install_path(&self, env: &AdapterEnvironment) -> Result; @@ -343,9 +453,169 @@ fn contains_managed_marker(content: &[u8]) -> bool { .any(|window| window == RALLY_MANAGED_MARKER.as_bytes()) } +fn parse_invite_token(raw: &str) -> Option { + let trimmed = raw.trim(); + let parsed = if let Some(rest) = trimmed.strip_prefix("rally://") { + let (session, agent) = rest.split_once('/')?; + Some((session, agent)) + } else if let Some(rest) = trimmed.strip_prefix("rly:") { + let (session, agent) = rest.split_once(':')?; + Some((session, agent)) + } else if let Some(rest) = trimmed.strip_prefix("invite:") { + let (session, agent) = rest.split_once(':')?; + Some((session, agent)) + } else if let Some((session, agent)) = trimmed.split_once('@') { + Some((session, agent)) + } else { + None + }?; + + if parsed.0.trim().is_empty() || parsed.1.trim().is_empty() { + return None; + } + + Some(InviteContext { + session: parsed.0.trim().to_string(), + agent: normalize_agent_selector(parsed.1), + }) +} + +fn normalize_agent_selector(raw: &str) -> String { + let lowered = raw.trim().to_ascii_lowercase(); + match lowered.as_str() { + "implementer" => "implementer".to_string(), + "reviewer" | "reviewer1" | "reviewer-1" => "reviewer-1".to_string(), + _ => raw.trim().to_string(), + } +} + +fn resolve_run_context( + args: &CommandRunArgs, + workflow_selector: Option, + role_selector: Option, + invite: Option, + saved: Option<&SavedRunContext>, + prompter: &mut dyn Prompter, +) -> Result { + let mut session = args.session.clone(); + let mut agent = args.agent.clone().map(|value| normalize_agent_selector(&value)); + let workflow = workflow_selector.clone().or_else(|| saved.and_then(|ctx| ctx.workflow.clone())); + + if session.is_none() && let Some(invite_ctx) = &invite { + session = Some(invite_ctx.session.clone()); + } + + if agent.is_none() && let Some(selector) = role_selector { + agent = Some(normalize_agent_selector(&selector)); + } + + if agent.is_none() && let Some(invite_ctx) = &invite { + agent = Some(invite_ctx.agent.clone()); + } + + if session.is_none() && let Some(saved_ctx) = saved { + session = Some(saved_ctx.session.clone()); + } + if agent.is_none() && let Some(saved_ctx) = saved { + agent = Some(saved_ctx.agent.clone()); + } + + if session.is_none() { + if args.non_interactive { + bail!( + "unable to resolve session from explicit args, invite token, or saved context; re-run with --session or enable interactive mode" + ); + } + let value = prompter.prompt("Session name")?; + if value.is_empty() { + bail!("session is required"); + } + session = Some(value); + } + + if agent.is_none() { + if args.non_interactive { + bail!( + "unable to resolve agent from explicit args, invite token, or saved context; re-run with --as or enable interactive mode" + ); + } + let value = prompter.prompt("Agent/role")?; + if value.is_empty() { + bail!("agent is required"); + } + agent = Some(normalize_agent_selector(&value)); + } + + Ok(ResolvedRunContext { + session: session.expect("session resolved"), + agent: agent.expect("agent resolved"), + workflow, + }) +} + +fn ensure_agent_registered(session: &str, agent: &str, registry: &WorkflowRegistry) -> Result<()> { + let known = match SessionHandle::open(session) { + Ok(handle) => { + let state = handle.load_state()?; + state.agents.contains_key(agent) + } + Err(err) => { + return Err(err) + .with_context(|| format!("resolving session '{}' before join", session)); + } + }; + if known { + return Ok(()); + } + + commands::join_with_registry( + &JoinArgs { + session: session.to_string(), + agent: agent.to_string(), + timeout: Some(2), + }, + registry, + ) +} + +fn workspace_key() -> Result { + let cwd = env::current_dir().context("resolving current working directory")?; + let canonical = cwd.canonicalize().unwrap_or(cwd); + Ok(canonical.display().to_string()) +} + +fn context_store_path() -> Result { + let home = dirs::home_dir().ok_or_else(|| anyhow!("unable to resolve home directory"))?; + Ok(home.join(".rally").join(CONTEXT_STORE_FILE)) +} + +fn load_context_store(path: &Path) -> Result { + if !path.exists() { + return Ok(RunContextStore::default()); + } + let raw = + fs::read_to_string(path).with_context(|| format!("reading context store {}", path.display()))?; + let parsed: RunContextStore = serde_json::from_str(&raw) + .with_context(|| format!("parsing context store {}", path.display()))?; + Ok(parsed) +} + +fn save_context_store(path: &Path, store: &RunContextStore) -> Result<()> { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).with_context(|| format!("creating {}", parent.display()))?; + } + let encoded = serde_json::to_string_pretty(store).context("encoding context store JSON")?; + fs::write(path, encoded).with_context(|| format!("writing context store {}", path.display())) +} + #[cfg(test)] mod tests { - use std::{fs, path::Path, time::{SystemTime, UNIX_EPOCH}}; + use std::{ + collections::VecDeque, + fs, + path::Path, + time::{SystemTime, UNIX_EPOCH}, + }; use anyhow::Context; @@ -424,6 +694,38 @@ mod tests { } } + struct StaticPrompter { + answers: VecDeque, + } + + impl StaticPrompter { + fn new(values: &[&str]) -> Self { + Self { + answers: values.iter().map(|v| (*v).to_string()).collect(), + } + } + } + + impl Prompter for StaticPrompter { + fn prompt(&mut self, _label: &str) -> Result { + self.answers + .pop_front() + .ok_or_else(|| anyhow!("no prompt values left")) + } + } + + fn run_args() -> CommandRunArgs { + CommandRunArgs { + first: None, + second: None, + extra: Vec::new(), + session: None, + agent: None, + invite: None, + non_interactive: false, + } + } + #[test] fn install_writes_wrapper_and_is_idempotent() -> Result<()> { let home = unique_dir("rally-command-install-idempotent"); @@ -497,4 +799,109 @@ mod tests { assert!(content.contains(RALLY_MANAGED_MARKER)); Ok(()) } + + #[test] + fn run_resolution_precedence_is_explicit_then_token_then_saved() -> Result<()> { + let args = CommandRunArgs { + session: Some("explicit-session".to_string()), + agent: Some("explicit-agent".to_string()), + ..run_args() + }; + let saved = SavedRunContext { + session: "saved-session".to_string(), + agent: "saved-agent".to_string(), + workflow: Some("build".to_string()), + }; + let invite = InviteContext { + session: "token-session".to_string(), + agent: "token-agent".to_string(), + }; + let mut prompter = StaticPrompter::new(&[]); + let resolved = resolve_run_context( + &args, + Some("plan".to_string()), + Some("reviewer".to_string()), + Some(invite), + Some(&saved), + &mut prompter, + )?; + assert_eq!(resolved.session, "explicit-session"); + assert_eq!(resolved.agent, "explicit-agent"); + assert_eq!(resolved.workflow.as_deref(), Some("plan")); + Ok(()) + } + + #[test] + fn run_resolution_uses_invite_when_explicit_missing() -> Result<()> { + let args = CommandRunArgs { + non_interactive: true, + ..run_args() + }; + let saved = SavedRunContext { + session: "saved-session".to_string(), + agent: "saved-agent".to_string(), + workflow: None, + }; + let invite = InviteContext { + session: "token-session".to_string(), + agent: "reviewer-1".to_string(), + }; + let mut prompter = StaticPrompter::new(&[]); + let resolved = resolve_run_context( + &args, + None, + None, + Some(invite), + Some(&saved), + &mut prompter, + )?; + assert_eq!(resolved.session, "token-session"); + assert_eq!(resolved.agent, "reviewer-1"); + Ok(()) + } + + #[test] + fn run_resolution_uses_saved_context_before_prompting() -> Result<()> { + let args = CommandRunArgs { + non_interactive: true, + ..run_args() + }; + let saved = SavedRunContext { + session: "saved-session".to_string(), + agent: "saved-agent".to_string(), + workflow: Some("build".to_string()), + }; + let mut prompter = StaticPrompter::new(&[]); + let resolved = + resolve_run_context(&args, None, None, None, Some(&saved), &mut prompter)?; + assert_eq!(resolved.session, "saved-session"); + assert_eq!(resolved.agent, "saved-agent"); + assert_eq!(resolved.workflow.as_deref(), Some("build")); + Ok(()) + } + + #[test] + fn run_resolution_prompts_interactively_when_unresolved() -> Result<()> { + let args = run_args(); + let mut prompter = StaticPrompter::new(&["prompt-session", "reviewer"]); + let resolved = resolve_run_context(&args, None, None, None, None, &mut prompter)?; + assert_eq!(resolved.session, "prompt-session"); + assert_eq!(resolved.agent, "reviewer-1"); + Ok(()) + } + + #[test] + fn run_resolution_non_interactive_errors_when_unresolved() { + let args = CommandRunArgs { + non_interactive: true, + ..run_args() + }; + let mut prompter = StaticPrompter::new(&[]); + let err = resolve_run_context(&args, None, None, None, None, &mut prompter) + .expect_err("missing context should error"); + assert!( + err.to_string() + .contains("unable to resolve session from explicit args") + ); + } } From 95412690e216a2408572ccdaa710feec65027a17 Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Sun, 1 Mar 2026 23:47:01 -0600 Subject: [PATCH 05/11] Implement command doctor and managed-only uninstall --- src/command_surface.rs | 279 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 275 insertions(+), 4 deletions(-) diff --git a/src/command_surface.rs b/src/command_surface.rs index 0a5442f..e8a8ba0 100644 --- a/src/command_surface.rs +++ b/src/command_surface.rs @@ -106,12 +106,53 @@ pub fn run(args: &CommandRunArgs, registry: &WorkflowRegistry) -> Result<()> { Ok(()) } -pub fn doctor(_args: &CommandDoctorArgs) -> Result<()> { - bail!("`rally command doctor` is not implemented yet") +pub fn doctor(args: &CommandDoctorArgs) -> Result<()> { + let env = AdapterEnvironment::detect()?; + let render_ctx = WrapperRenderContext::default(); + let outcomes = doctor_with_environment(args, &env, &render_ctx)?; + for outcome in outcomes { + if args.dry_run { + println!( + "[dry-run] target={} status={} root={} wrapper={}", + outcome.target, + outcome.status, + outcome.root.display(), + outcome.wrapper.display() + ); + } else { + println!( + "target={} status={} root={} wrapper={}", + outcome.target, + outcome.status, + outcome.root.display(), + outcome.wrapper.display() + ); + } + } + Ok(()) } -pub fn uninstall(_args: &CommandUninstallArgs) -> Result<()> { - bail!("`rally command uninstall` is not implemented yet") +pub fn uninstall(args: &CommandUninstallArgs) -> Result<()> { + let env = AdapterEnvironment::detect()?; + let outcomes = uninstall_with_environment(args, &env)?; + for outcome in outcomes { + if args.dry_run { + println!( + "[dry-run] target={} action={} wrapper={}", + outcome.target, + outcome.action, + outcome.wrapper.display() + ); + } else { + println!( + "target={} action={} wrapper={}", + outcome.target, + outcome.action, + outcome.wrapper.display() + ); + } + } + Ok(()) } #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -251,6 +292,69 @@ struct InstallOutcome { action: InstallAction, } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum DoctorStatus { + EnvironmentMissing, + InvalidEnvironment, + WrapperMissing, + InstalledManaged, + InstalledManagedModified, + InstalledUnmanaged, +} + +impl fmt::Display for DoctorStatus { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let label = match self { + Self::EnvironmentMissing => "environment-missing", + Self::InvalidEnvironment => "invalid-environment", + Self::WrapperMissing => "wrapper-missing", + Self::InstalledManaged => "installed-managed", + Self::InstalledManagedModified => "installed-managed-modified", + Self::InstalledUnmanaged => "installed-unmanaged", + }; + f.write_str(label) + } +} + +#[derive(Clone, Debug)] +struct DoctorOutcome { + target: HarnessTarget, + root: PathBuf, + wrapper: PathBuf, + status: DoctorStatus, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum UninstallAction { + SkippedEnvironmentMissing, + SkippedInvalidEnvironment, + SkippedWrapperMissing, + SkippedUnmanaged, + RemovedManaged, + WouldRemoveManaged, +} + +impl fmt::Display for UninstallAction { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let label = match self { + Self::SkippedEnvironmentMissing => "skip-environment-missing", + Self::SkippedInvalidEnvironment => "skip-invalid-environment", + Self::SkippedWrapperMissing => "skip-wrapper-missing", + Self::SkippedUnmanaged => "skip-unmanaged", + Self::RemovedManaged => "removed-managed", + Self::WouldRemoveManaged => "would-remove-managed", + }; + f.write_str(label) + } +} + +#[derive(Clone, Debug)] +struct UninstallOutcome { + target: HarnessTarget, + wrapper: PathBuf, + action: UninstallAction, +} + #[derive(Clone, Debug)] struct InviteContext { session: String, @@ -364,6 +468,17 @@ fn adapters_for_target(target: CommandTargetArg) -> Vec> } } +fn selected_targets(target: CommandTargetArg) -> Vec { + match target { + CommandTargetArg::All => HarnessTarget::all(), + CommandTargetArg::Codex => vec![HarnessTarget::Codex], + CommandTargetArg::Droid => vec![HarnessTarget::Droid], + CommandTargetArg::Pi => vec![HarnessTarget::Pi], + CommandTargetArg::ClaudeCode => vec![HarnessTarget::ClaudeCode], + CommandTargetArg::Factory => vec![HarnessTarget::Factory], + } +} + fn install_with_environment( args: &CommandInstallArgs, env: &AdapterEnvironment, @@ -453,6 +568,81 @@ fn contains_managed_marker(content: &[u8]) -> bool { .any(|window| window == RALLY_MANAGED_MARKER.as_bytes()) } +fn doctor_with_environment( + args: &CommandDoctorArgs, + env: &AdapterEnvironment, + render_ctx: &WrapperRenderContext, +) -> Result> { + let mut outcomes = Vec::new(); + for target in selected_targets(args.target) { + let root = env.root_for(target)?; + let wrapper = target.wrapper_path(&root); + let status = if !root.exists() { + DoctorStatus::EnvironmentMissing + } else if !root.is_dir() { + DoctorStatus::InvalidEnvironment + } else if !wrapper.exists() { + DoctorStatus::WrapperMissing + } else { + let bytes = fs::read(&wrapper) + .with_context(|| format!("reading wrapper {}", wrapper.display()))?; + if contains_managed_marker(&bytes) { + let expected = adapter_for(target).render_wrapper(render_ctx); + if bytes == expected.as_bytes() { + DoctorStatus::InstalledManaged + } else { + DoctorStatus::InstalledManagedModified + } + } else { + DoctorStatus::InstalledUnmanaged + } + }; + outcomes.push(DoctorOutcome { + target, + root, + wrapper, + status, + }); + } + Ok(outcomes) +} + +fn uninstall_with_environment( + args: &CommandUninstallArgs, + env: &AdapterEnvironment, +) -> Result> { + let mut outcomes = Vec::new(); + for target in selected_targets(args.target) { + let root = env.root_for(target)?; + let wrapper = target.wrapper_path(&root); + let action = if !root.exists() { + UninstallAction::SkippedEnvironmentMissing + } else if !root.is_dir() { + UninstallAction::SkippedInvalidEnvironment + } else if !wrapper.exists() { + UninstallAction::SkippedWrapperMissing + } else { + let bytes = fs::read(&wrapper) + .with_context(|| format!("reading wrapper {}", wrapper.display()))?; + if !contains_managed_marker(&bytes) { + UninstallAction::SkippedUnmanaged + } else if args.dry_run { + UninstallAction::WouldRemoveManaged + } else { + fs::remove_file(&wrapper) + .with_context(|| format!("removing wrapper {}", wrapper.display()))?; + UninstallAction::RemovedManaged + } + }; + outcomes.push(UninstallOutcome { + target, + wrapper, + action, + }); + } + Ok(outcomes) +} + fn parse_invite_token(raw: &str) -> Option { let trimmed = raw.trim(); let parsed = if let Some(rest) = trimmed.strip_prefix("rally://") { @@ -694,6 +884,14 @@ mod tests { } } + fn doctor_args(target: CommandTargetArg, dry_run: bool) -> CommandDoctorArgs { + CommandDoctorArgs { target, dry_run } + } + + fn uninstall_args(target: CommandTargetArg, dry_run: bool) -> CommandUninstallArgs { + CommandUninstallArgs { target, dry_run } + } + struct StaticPrompter { answers: VecDeque, } @@ -800,6 +998,79 @@ mod tests { Ok(()) } + #[test] + fn doctor_reports_managed_install_status() -> Result<()> { + let home = unique_dir("rally-command-doctor-managed"); + let _root = prepare_target_root(&home, HarnessTarget::Factory)?; + let env = env_with_home(&home); + let render_ctx = WrapperRenderContext::default(); + install_with_environment( + &install_args(CommandTargetArg::Factory, false, false), + &env, + &render_ctx, + )?; + + let result = doctor_with_environment( + &doctor_args(CommandTargetArg::Factory, false), + &env, + &render_ctx, + )?; + assert_eq!(result.len(), 1); + assert_eq!(result[0].status, DoctorStatus::InstalledManaged); + Ok(()) + } + + #[test] + fn uninstall_removes_only_managed_wrappers() -> Result<()> { + let home = unique_dir("rally-command-uninstall-managed"); + let managed_root = prepare_target_root(&home, HarnessTarget::Codex)?; + let unmanaged_root = prepare_target_root(&home, HarnessTarget::Droid)?; + let env = env_with_home(&home); + let render_ctx = WrapperRenderContext::default(); + + install_with_environment( + &install_args(CommandTargetArg::Codex, false, false), + &env, + &render_ctx, + )?; + let unmanaged = unmanaged_root.join("commands").join("rally.md"); + fs::create_dir_all(unmanaged.parent().expect("wrapper parent"))?; + fs::write(&unmanaged, "custom content")?; + + let outcome = uninstall_with_environment(&uninstall_args(CommandTargetArg::All, false), &env)?; + let codex = outcome + .iter() + .find(|o| o.target == HarnessTarget::Codex) + .expect("codex result"); + let droid = outcome + .iter() + .find(|o| o.target == HarnessTarget::Droid) + .expect("droid result"); + assert_eq!(codex.action, UninstallAction::RemovedManaged); + assert_eq!(droid.action, UninstallAction::SkippedUnmanaged); + assert!(!managed_root.join("commands").join("rally.md").exists()); + assert_eq!(fs::read_to_string(&unmanaged)?, "custom content"); + Ok(()) + } + + #[test] + fn uninstall_dry_run_keeps_managed_file() -> Result<()> { + let home = unique_dir("rally-command-uninstall-dry-run"); + let root = prepare_target_root(&home, HarnessTarget::Pi)?; + let env = env_with_home(&home); + let render_ctx = WrapperRenderContext::default(); + install_with_environment( + &install_args(CommandTargetArg::Pi, false, false), + &env, + &render_ctx, + )?; + let wrapper = root.join("commands").join("rally.md"); + let outcome = uninstall_with_environment(&uninstall_args(CommandTargetArg::Pi, true), &env)?; + assert_eq!(outcome[0].action, UninstallAction::WouldRemoveManaged); + assert!(wrapper.exists()); + Ok(()) + } + #[test] fn run_resolution_precedence_is_explicit_then_token_then_saved() -> Result<()> { let args = CommandRunArgs { From 2ab62d12fced179dece6c701f827c899a4b5e3bc Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Sun, 1 Mar 2026 23:50:39 -0600 Subject: [PATCH 06/11] Add integration coverage for command install and run entrypoint --- tests/command_install_run.rs | 299 +++++++++++++++++++++++++++++++++++ 1 file changed, 299 insertions(+) create mode 100644 tests/command_install_run.rs diff --git a/tests/command_install_run.rs b/tests/command_install_run.rs new file mode 100644 index 0000000..c51417b --- /dev/null +++ b/tests/command_install_run.rs @@ -0,0 +1,299 @@ +use std::{ + env, fs, + io::Write, + path::{Path, PathBuf}, + process::{Command, Stdio}, + time::{SystemTime, UNIX_EPOCH}, +}; + +use anyhow::{Context, Result}; +use clap::Parser; +use rally::{cli::Cli, run_cli_with_registry, workflow}; +use serde_json::Value; + +fn unique_name(prefix: &str) -> String { + let ts = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("time") + .as_nanos(); + format!("{prefix}-{ts}") +} + +fn run_cli(args: &[&str], registry: &rally::WorkflowRegistry) -> Result { + let cli = Cli::try_parse_from(args)?; + run_cli_with_registry(cli, registry) +} + +fn write_todo(path: &Path, title: &str) -> Result<()> { + fs::write( + path, + format!( + "## Spec\n\ncommand run test\n\n## Plan\n\n1. {title}\nAcceptance criteria:\n- done\n" + ), + ) + .with_context(|| format!("writing {}", path.display())) +} + +fn read_agent_last_seen(home: &Path, session: &str, agent: &str) -> Result { + let path = home + .join(".rally") + .join("sessions") + .join(session) + .join("state.json"); + let raw = fs::read_to_string(&path).with_context(|| format!("reading {}", path.display()))?; + let parsed: Value = serde_json::from_str(&raw)?; + let value = parsed + .get("agents") + .and_then(|agents| agents.get(agent)) + .and_then(|agent_obj| agent_obj.get("last_seen")) + .and_then(Value::as_str) + .ok_or_else(|| anyhow::anyhow!("missing last_seen for agent '{agent}'"))?; + Ok(value.to_string()) +} + +fn read_context_session(home: &Path, workspace: &Path) -> Result { + let store = home.join(".rally").join("command-context.json"); + let raw = fs::read_to_string(&store).with_context(|| format!("reading {}", store.display()))?; + let parsed: Value = serde_json::from_str(&raw)?; + let key = workspace + .canonicalize() + .unwrap_or_else(|_| workspace.to_path_buf()) + .display() + .to_string(); + let session = parsed + .get("by_workspace") + .and_then(|m| m.get(&key)) + .and_then(|ctx| ctx.get("session")) + .and_then(Value::as_str) + .ok_or_else(|| anyhow::anyhow!("missing saved context for workspace '{key}'"))?; + Ok(session.to_string()) +} + +#[test] +fn command_install_run_entrypoint_integration() -> Result<()> { + let temp_home = env::temp_dir().join(unique_name("rally-command-run-it-home")); + fs::create_dir_all(&temp_home)?; + let old_home = env::var_os("HOME"); + // SAFETY: this integration test runs in its own process and restores HOME before returning. + unsafe { env::set_var("HOME", &temp_home) }; + + let registry = workflow::default_registry()?; + let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + + let codex_root = temp_home.join(".codex"); + let droid_root = temp_home.join(".droid"); + let pi_root = temp_home.join(".pi"); + let claude_root = temp_home.join(".claude"); + let factory_root = temp_home.join(".factory"); + for root in [&codex_root, &droid_root, &pi_root, &claude_root, &factory_root] { + fs::create_dir_all(root)?; + } + + run_cli( + &["rally", "command", "install", "--target", "all"], + ®istry, + )?; + + for (target, root) in [ + ("codex", &codex_root), + ("droid", &droid_root), + ("pi", &pi_root), + ("claude-code", &claude_root), + ("factory", &factory_root), + ] { + let wrapper = root.join("commands").join("rally.md"); + let content = fs::read_to_string(&wrapper)?; + assert!(content.contains("rally command run \"$@\"")); + assert!(content.contains(&format!("rally-target: {target}"))); + } + + let codex_wrapper = codex_root.join("commands").join("rally.md"); + let before = fs::read_to_string(&codex_wrapper)?; + run_cli( + &["rally", "command", "install", "--target", "codex"], + ®istry, + )?; + let after = fs::read_to_string(&codex_wrapper)?; + assert_eq!(before, after); + + let droid_wrapper = droid_root.join("commands").join("rally.md"); + fs::write(&droid_wrapper, "custom wrapper content")?; + let err = run_cli( + &["rally", "command", "install", "--target", "droid"], + ®istry, + ) + .expect_err("unmanaged wrapper should require --force"); + assert!(err.to_string().contains("refusing to overwrite non-Rally file")); + + run_cli( + &["rally", "command", "doctor", "--target", "all", "--dry-run"], + ®istry, + )?; + + run_cli( + &[ + "rally", + "command", + "uninstall", + "--target", + "codex", + "--dry-run", + ], + ®istry, + )?; + assert!(codex_wrapper.exists()); + + run_cli( + &["rally", "command", "uninstall", "--target", "codex"], + ®istry, + )?; + assert!(!codex_wrapper.exists()); + assert_eq!(fs::read_to_string(&droid_wrapper)?, "custom wrapper content"); + + let todo_one = temp_home.join("todo-one.md"); + let todo_two = temp_home.join("todo-two.md"); + write_todo(&todo_one, "Token/session one")?; + write_todo(&todo_two, "Token/session two")?; + let session_one = unique_name("command-run-saved"); + let session_two = unique_name("command-run-token"); + + run_cli( + &[ + "rally", + "create", + "implement", + "--name", + &session_one, + "--todo", + todo_one.to_str().expect("utf8 path"), + "--workspace", + manifest_dir.to_str().expect("utf8 path"), + "--reviewers", + "0", + ], + ®istry, + )?; + run_cli( + &[ + "rally", + "join", + "--session", + &session_one, + "--as", + "implementer", + ], + ®istry, + )?; + run_cli( + &[ + "rally", + "create", + "implement", + "--name", + &session_two, + "--todo", + todo_two.to_str().expect("utf8 path"), + "--workspace", + manifest_dir.to_str().expect("utf8 path"), + "--reviewers", + "0", + ], + ®istry, + )?; + run_cli( + &[ + "rally", + "join", + "--session", + &session_two, + "--as", + "implementer", + ], + ®istry, + )?; + + let workspace_key = manifest_dir + .canonicalize() + .unwrap_or_else(|_| manifest_dir.clone()) + .display() + .to_string(); + let context_store = temp_home.join(".rally").join("command-context.json"); + fs::create_dir_all(context_store.parent().expect("context store parent"))?; + fs::write( + &context_store, + format!( + "{{\"by_workspace\":{{\"{workspace_key}\":{{\"session\":\"{session_one}\",\"agent\":\"implementer\"}}}}}}" + ), + )?; + + let session_one_seen_before = read_agent_last_seen(&temp_home, &session_one, "implementer")?; + run_cli( + &["rally", "command", "run", "--non-interactive"], + ®istry, + )?; + let session_one_seen_after = read_agent_last_seen(&temp_home, &session_one, "implementer")?; + assert_ne!(session_one_seen_before, session_one_seen_after); + + run_cli( + &[ + "rally", + "command", + "run", + &format!("rly:{session_two}:implementer"), + "--non-interactive", + ], + ®istry, + )?; + assert_eq!(read_context_session(&temp_home, &manifest_dir)?, session_two); + + run_cli( + &[ + "rally", + "command", + "run", + "--session", + &session_one, + "--as", + "implementer", + "--invite", + &format!("rly:{session_two}:implementer"), + "--non-interactive", + ], + ®istry, + )?; + assert_eq!(read_context_session(&temp_home, &manifest_dir)?, session_one); + + fs::remove_file(&context_store)?; + let mut child = Command::new(env!("CARGO_BIN_EXE_rally")) + .arg("command") + .arg("run") + .env("HOME", &temp_home) + .current_dir(&manifest_dir) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .context("spawning interactive rally command run")?; + { + let stdin = child.stdin.as_mut().expect("stdin"); + writeln!(stdin, "{session_two}")?; + writeln!(stdin, "implementer")?; + } + let output = child.wait_with_output()?; + assert!( + output.status.success(), + "interactive run failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(read_context_session(&temp_home, &manifest_dir)?, session_two); + + if let Some(value) = old_home { + // SAFETY: restore original HOME at test teardown. + unsafe { env::set_var("HOME", value) }; + } else { + // SAFETY: restore original HOME at test teardown. + unsafe { env::remove_var("HOME") }; + } + Ok(()) +} From 01337ccaf76bc77b8dabd6dc842267e7da8e9895 Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Sun, 1 Mar 2026 23:52:07 -0600 Subject: [PATCH 07/11] Document command wrapper install/run workflow --- README.md | 51 +++++++++++++++++ docs/command-install-run.md | 108 ++++++++++++++++++++++++++++++++++++ 2 files changed, 159 insertions(+) create mode 100644 README.md create mode 100644 docs/command-install-run.md diff --git a/README.md b/README.md new file mode 100644 index 0000000..a1047a9 --- /dev/null +++ b/README.md @@ -0,0 +1,51 @@ +# Rally + +Step-by-step coordinator for multi-agent coding sessions. + +## Command Wrappers + +Rally now provides a wrapper-oriented command surface: + +- `rally command install --target ` +- `rally command run ...` (canonical wrapper entrypoint) +- `rally command exec ...` (alias for `run`) +- `rally command doctor --target ...` +- `rally command uninstall --target ...` + +Install wrappers everywhere: + +```bash +rally command install --target all +``` + +Installed `/rally` wrappers are thin adapters that delegate back to Rally core: + +```bash +rally command run "$@" +``` + +## /rally Usage + +Typical wrapper invocations: + +- `/rally` +- `/rally build` +- `/rally build reviewer` +- `/rally ` + +`rally command run` resolves context in this order: + +1. explicit args (`--session`, `--as`, positional selectors) +2. invite token input +3. saved local workspace context +4. interactive prompt fallback + +## Troubleshooting + +Use doctor to inspect path/status per target: + +```bash +rally command doctor --target all +``` + +For details and target path mapping, see [docs/command-install-run.md](docs/command-install-run.md). diff --git a/docs/command-install-run.md b/docs/command-install-run.md new file mode 100644 index 0000000..0fc989e --- /dev/null +++ b/docs/command-install-run.md @@ -0,0 +1,108 @@ +# Rally Command Install/Run + +This document describes Rally's wrapper entrypoint for harness slash commands. + +## Install Targets + +Install wrapper artifacts for one harness: + +```bash +rally command install --target codex +``` + +Install for all supported harnesses: + +```bash +rally command install --target all +``` + +Supported targets: + +- `codex` -> `~/.codex/commands/rally.md` +- `droid` -> `~/.droid/commands/rally.md` +- `pi` -> `~/.pi/commands/rally.md` +- `claude-code` -> `~/.claude/commands/rally.md` +- `factory` -> `~/.factory/commands/rally.md` + +Each wrapper is Rally-managed content and delegates back to: + +```bash +rally command run "$@" +``` + +## Wrapper Behavior + +The generated `/rally` wrapper is intentionally thin: + +- accepts wrapper arguments unchanged +- forwards arguments to `rally command run` +- keeps orchestration logic centralized in Rally + +Examples: + +- `/rally` +- `/rally build` +- `/rally build reviewer` +- `/rally ` + +## `rally command run` Patterns + +`run` and `exec` are equivalent: + +```bash +rally command run +rally command exec +``` + +Resolution precedence: + +1. explicit args (`--session`, `--as`, positional selectors) +2. invite token (for example `rly::`, `rally:///`) +3. saved workspace context (`~/.rally/command-context.json`) +4. interactive fallback prompt + +## Troubleshooting with Doctor + +Inspect current installation health: + +```bash +rally command doctor --target all +``` + +Doctor reports per target: + +- environment/path availability +- wrapper presence +- managed/unmanaged file state +- managed content drift + +For preview mode: + +```bash +rally command doctor --target all --dry-run +``` + +## Uninstall Behavior + +Remove Rally-managed wrappers only: + +```bash +rally command uninstall --target all +``` + +Unmanaged user files are never removed. + +Preview removal decisions: + +```bash +rally command uninstall --target all --dry-run +``` + +## Mapping to Rally Core Commands + +Wrappers map to Rally core loop behavior through `rally command run`, which then joins/polls the existing session workflows: + +- registration path: `rally join --session --as ` (as needed) +- instruction polling path: `rally next --session --as ` +- review flow remains canonical under `rally build checkpoint|review` +- plan actions remain canonical under `rally plan ...` From 91d1ed8cdfa2bd319d34de2b1ef9d9035ca80879 Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Sun, 1 Mar 2026 23:53:48 -0600 Subject: [PATCH 08/11] Record manual QA for command install and run workflow --- docs/manual-qa-command-install-run.md | 101 ++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 docs/manual-qa-command-install-run.md diff --git a/docs/manual-qa-command-install-run.md b/docs/manual-qa-command-install-run.md new file mode 100644 index 0000000..c534df8 --- /dev/null +++ b/docs/manual-qa-command-install-run.md @@ -0,0 +1,101 @@ +# Manual QA: Command Install/Run + +Date: 2026-03-02 + +Environment: + +- Rally binary: `/Users/justin/code/rally/target/debug/rally` +- Workspace: `/Users/justin/code/rally/worktrees/command-install-run` +- Temporary HOME used for isolated QA harness roots + +## Scenario 1: Install wrappers for two targets + +Commands: + +```bash +HOME="$TMP_HOME" rally command install --target codex +HOME="$TMP_HOME" rally command install --target droid +``` + +Observed: + +- `~/.codex/commands/rally.md` created +- `~/.droid/commands/rally.md` created + +## Scenario 2: `/rally` routing behavior + +Created implement session `qa-command-install-run-1772430801` with a single step. + +Commands and observed results: + +```bash +HOME="$TMP_HOME" rally command run --session "$SESSION" --as implementer --non-interactive +``` + +- Routed to implement instruction (`Implement step 1: Validate wrapper routing`) + +```bash +HOME="$TMP_HOME" rally command run build --non-interactive +``` + +- Routed through saved context fallback and printed same instruction + +```bash +HOME="$TMP_HOME" rally command run "rly:${SESSION}:implementer" --non-interactive +``` + +- Invite-token resolution worked and printed same instruction + +## Scenario 3: `doctor`, uninstall, reinstall, and custom file safety + +Doctor output validated per-target status/path reporting: + +```bash +HOME="$TMP_HOME" rally command doctor --target all +``` + +Created unmanaged custom file: + +```bash +echo "custom user wrapper" > "$TMP_HOME/.pi/commands/rally.md" +``` + +Confirmed non-destructive install behavior: + +```bash +HOME="$TMP_HOME" rally command install --target pi +``` + +- Failed as expected without `--force` and did not overwrite custom file + +Verified uninstall behavior: + +```bash +HOME="$TMP_HOME" rally command uninstall --target all --dry-run +HOME="$TMP_HOME" rally command uninstall --target all +``` + +- Managed codex/droid wrappers removed +- Unmanaged `~/.pi/commands/rally.md` preserved + +Reinstall check: + +```bash +HOME="$TMP_HOME" rally command install --target codex +HOME="$TMP_HOME" rally command install --target droid +HOME="$TMP_HOME" rally command doctor --target all --dry-run +``` + +- Managed wrappers recreated cleanly +- Doctor reported managed status for codex/droid and unmanaged status for pi custom file + +## Result + +Manual QA passed for: + +- two-target wrapper installation +- `/rally`, `/rally build`, and invite-token routing +- doctor diagnostics +- uninstall cleanup +- reinstall behavior +- preservation of custom non-Rally wrapper files From 8bbf6ba67ea54dab600280d60eb7b274e17a9c9b Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Mon, 2 Mar 2026 00:07:12 -0600 Subject: [PATCH 09/11] Fix command wrapper safety and run exit-code propagation --- src/command_surface.rs | 353 ++++++++++++++++++++++++++--------- src/lib.rs | 21 ++- tests/command_install_run.rs | 69 ++++++- 3 files changed, 339 insertions(+), 104 deletions(-) diff --git a/src/command_surface.rs b/src/command_surface.rs index e8a8ba0..ef5cde8 100644 --- a/src/command_surface.rs +++ b/src/command_surface.rs @@ -48,10 +48,10 @@ pub fn install(args: &CommandInstallArgs) -> Result<()> { Ok(()) } -pub fn run(args: &CommandRunArgs, registry: &WorkflowRegistry) -> Result<()> { +pub fn run(args: &CommandRunArgs, registry: &WorkflowRegistry) -> Result { let workspace = workspace_key()?; let store_path = context_store_path()?; - let mut store = load_context_store(&store_path)?; + let mut store = load_context_store(&store_path); let saved = store.by_workspace.get(&workspace).cloned(); let explicit_invite = args @@ -103,7 +103,7 @@ pub fn run(args: &CommandRunArgs, registry: &WorkflowRegistry) -> Result<()> { }, ); save_context_store(&store_path, &store)?; - Ok(()) + Ok(code) } pub fn doctor(args: &CommandDoctorArgs) -> Result<()> { @@ -267,6 +267,8 @@ struct WrapperArtifact { #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum InstallAction { + SkipEnvironmentMissing, + SkipInvalidEnvironment, Create, UpdateManaged, OverwriteForced, @@ -276,6 +278,8 @@ enum InstallAction { impl fmt::Display for InstallAction { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let label = match self { + Self::SkipEnvironmentMissing => "skip-environment-missing", + Self::SkipInvalidEnvironment => "skip-invalid-environment", Self::Create => "create", Self::UpdateManaged => "update-managed", Self::OverwriteForced => "overwrite-forced", @@ -299,6 +303,7 @@ enum DoctorStatus { WrapperMissing, InstalledManaged, InstalledManagedModified, + InstalledManagedWrongTarget, InstalledUnmanaged, } @@ -310,6 +315,7 @@ impl fmt::Display for DoctorStatus { Self::WrapperMissing => "wrapper-missing", Self::InstalledManaged => "installed-managed", Self::InstalledManagedModified => "installed-managed-modified", + Self::InstalledManagedWrongTarget => "installed-managed-wrong-target", Self::InstalledUnmanaged => "installed-unmanaged", }; f.write_str(label) @@ -355,6 +361,13 @@ struct UninstallOutcome { action: UninstallAction, } +#[derive(Clone, Debug, Eq, PartialEq)] +enum ManagedOwnership { + ManagedForTarget, + ManagedForOtherTarget(String), + NotManaged, +} + #[derive(Clone, Debug)] struct InviteContext { session: String, @@ -401,7 +414,6 @@ impl Prompter for StdinPrompter { } trait TargetAdapter { - fn target(&self) -> HarnessTarget; fn discover_install_path(&self, env: &AdapterEnvironment) -> Result; fn render_wrapper(&self, ctx: &WrapperRenderContext) -> String; } @@ -417,10 +429,6 @@ impl DotDirAdapter { } impl TargetAdapter for DotDirAdapter { - fn target(&self) -> HarnessTarget { - self.target - } - fn discover_install_path(&self, env: &AdapterEnvironment) -> Result { let root = env.root_for(self.target)?; if !root.exists() { @@ -457,17 +465,6 @@ fn adapter_for(target: HarnessTarget) -> Box { Box::new(DotDirAdapter::new(target)) } -fn adapters_for_target(target: CommandTargetArg) -> Vec> { - match target { - CommandTargetArg::All => HarnessTarget::all().into_iter().map(adapter_for).collect(), - CommandTargetArg::Codex => vec![adapter_for(HarnessTarget::Codex)], - CommandTargetArg::Droid => vec![adapter_for(HarnessTarget::Droid)], - CommandTargetArg::Pi => vec![adapter_for(HarnessTarget::Pi)], - CommandTargetArg::ClaudeCode => vec![adapter_for(HarnessTarget::ClaudeCode)], - CommandTargetArg::Factory => vec![adapter_for(HarnessTarget::Factory)], - } -} - fn selected_targets(target: CommandTargetArg) -> Vec { match target { CommandTargetArg::All => HarnessTarget::all(), @@ -484,21 +481,59 @@ fn install_with_environment( env: &AdapterEnvironment, render_ctx: &WrapperRenderContext, ) -> Result> { - let artifacts = planned_wrappers(args.target, env, render_ctx)?; - let mut plans = Vec::new(); - for artifact in artifacts { - let action = classify_install_action(&artifact.path, &artifact.content, args.force)?; - plans.push((artifact, action)); - } - let mut outcomes = Vec::new(); - for (artifact, action) in plans { + let allow_missing = matches!(args.target, CommandTargetArg::All); + for target in selected_targets(args.target) { + let root = env.root_for(target)?; + let wrapper_path = target.wrapper_path(&root); + let action = if !root.exists() { + if allow_missing { + InstallAction::SkipEnvironmentMissing + } else { + bail!( + "target '{}' environment not found at {}. Install the harness first or set {} to the harness home directory", + target, + root.display(), + target.env_var() + ); + } + } else if !root.is_dir() { + if allow_missing { + InstallAction::SkipInvalidEnvironment + } else { + bail!( + "target '{}' path {} is not a directory. Set {} to a valid harness home directory", + target, + root.display(), + target.env_var() + ); + } + } else { + let adapter = adapter_for(target); + let artifact = WrapperArtifact { + target, + path: adapter.discover_install_path(env)?, + content: adapter.render_wrapper(render_ctx), + }; + let action = + classify_install_action(&artifact.path, &artifact.content, target, args.force)?; + if !args.dry_run { + apply_install_action(&artifact, action)?; + } + outcomes.push(InstallOutcome { + target: artifact.target, + path: artifact.path, + action, + }); + continue; + }; + if !args.dry_run { - apply_install_action(&artifact, action)?; + // Skip actions intentionally do not write. } outcomes.push(InstallOutcome { - target: artifact.target, - path: artifact.path, + target, + path: wrapper_path, action, }); } @@ -506,25 +541,12 @@ fn install_with_environment( Ok(outcomes) } -fn planned_wrappers( - target: CommandTargetArg, - env: &AdapterEnvironment, - render_ctx: &WrapperRenderContext, -) -> Result> { - let mut out = Vec::new(); - for adapter in adapters_for_target(target) { - let path = adapter.discover_install_path(env)?; - let content = adapter.render_wrapper(render_ctx); - out.push(WrapperArtifact { - target: adapter.target(), - path, - content, - }); - } - Ok(out) -} - -fn classify_install_action(path: &std::path::Path, content: &str, force: bool) -> Result { +fn classify_install_action( + path: &std::path::Path, + content: &str, + target: HarnessTarget, + force: bool, +) -> Result { if !path.exists() { return Ok(InstallAction::Create); } @@ -534,23 +556,37 @@ fn classify_install_action(path: &std::path::Path, content: &str, force: bool) - return Ok(InstallAction::Noop); } - if contains_managed_marker(¤t) { - return Ok(InstallAction::UpdateManaged); - } - - if force { - return Ok(InstallAction::OverwriteForced); + match managed_ownership(¤t, target) { + ManagedOwnership::ManagedForTarget => Ok(InstallAction::UpdateManaged), + ManagedOwnership::ManagedForOtherTarget(other_target) => { + if force { + Ok(InstallAction::OverwriteForced) + } else { + bail!( + "refusing to overwrite Rally-managed file for target '{}' at {}. Re-run with --force to replace it", + other_target, + path.display() + ) + } + } + ManagedOwnership::NotManaged => { + if force { + Ok(InstallAction::OverwriteForced) + } else { + bail!( + "refusing to overwrite non-Rally file at {}. Re-run with --force to replace it", + path.display() + ) + } + } } - - bail!( - "refusing to overwrite non-Rally file at {}. Re-run with --force to replace it", - path.display() - ) } fn apply_install_action(artifact: &WrapperArtifact, action: InstallAction) -> Result<()> { match action { - InstallAction::Noop => Ok(()), + InstallAction::SkipEnvironmentMissing + | InstallAction::SkipInvalidEnvironment + | InstallAction::Noop => Ok(()), InstallAction::Create | InstallAction::UpdateManaged | InstallAction::OverwriteForced => { if let Some(parent) = artifact.path.parent() { fs::create_dir_all(parent) @@ -562,10 +598,35 @@ fn apply_install_action(artifact: &WrapperArtifact, action: InstallAction) -> Re } } -fn contains_managed_marker(content: &[u8]) -> bool { - content - .windows(RALLY_MANAGED_MARKER.len()) - .any(|window| window == RALLY_MANAGED_MARKER.as_bytes()) +fn managed_ownership(content: &[u8], target: HarnessTarget) -> ManagedOwnership { + let Some(parsed_target) = parse_managed_target(content) else { + return ManagedOwnership::NotManaged; + }; + if parsed_target == target.slug() { + ManagedOwnership::ManagedForTarget + } else { + ManagedOwnership::ManagedForOtherTarget(parsed_target) + } +} + +fn parse_managed_target(content: &[u8]) -> Option { + let text = std::str::from_utf8(content).ok()?; + let mut lines = text.lines(); + let marker = lines.next()?.trim(); + if marker != RALLY_MANAGED_MARKER { + return None; + } + let target_line = lines.next()?.trim(); + let prefix = ""; + if !target_line.starts_with(prefix) || !target_line.ends_with(suffix) { + return None; + } + let target = &target_line[prefix.len()..target_line.len() - suffix.len()]; + if target.trim().is_empty() { + return None; + } + Some(target.trim().to_string()) } fn doctor_with_environment( @@ -586,15 +647,17 @@ fn doctor_with_environment( } else { let bytes = fs::read(&wrapper) .with_context(|| format!("reading wrapper {}", wrapper.display()))?; - if contains_managed_marker(&bytes) { - let expected = adapter_for(target).render_wrapper(render_ctx); - if bytes == expected.as_bytes() { - DoctorStatus::InstalledManaged - } else { - DoctorStatus::InstalledManagedModified + match managed_ownership(&bytes, target) { + ManagedOwnership::ManagedForTarget => { + let expected = adapter_for(target).render_wrapper(render_ctx); + if bytes == expected.as_bytes() { + DoctorStatus::InstalledManaged + } else { + DoctorStatus::InstalledManagedModified + } } - } else { - DoctorStatus::InstalledUnmanaged + ManagedOwnership::ManagedForOtherTarget(_) => DoctorStatus::InstalledManagedWrongTarget, + ManagedOwnership::NotManaged => DoctorStatus::InstalledUnmanaged, } }; outcomes.push(DoctorOutcome { @@ -624,14 +687,19 @@ fn uninstall_with_environment( } else { let bytes = fs::read(&wrapper) .with_context(|| format!("reading wrapper {}", wrapper.display()))?; - if !contains_managed_marker(&bytes) { - UninstallAction::SkippedUnmanaged - } else if args.dry_run { - UninstallAction::WouldRemoveManaged - } else { - fs::remove_file(&wrapper) - .with_context(|| format!("removing wrapper {}", wrapper.display()))?; - UninstallAction::RemovedManaged + match managed_ownership(&bytes, target) { + ManagedOwnership::ManagedForTarget => { + if args.dry_run { + UninstallAction::WouldRemoveManaged + } else { + fs::remove_file(&wrapper) + .with_context(|| format!("removing wrapper {}", wrapper.display()))?; + UninstallAction::RemovedManaged + } + } + ManagedOwnership::ManagedForOtherTarget(_) | ManagedOwnership::NotManaged => { + UninstallAction::SkippedUnmanaged + } } }; outcomes.push(UninstallOutcome { @@ -779,15 +847,30 @@ fn context_store_path() -> Result { Ok(home.join(".rally").join(CONTEXT_STORE_FILE)) } -fn load_context_store(path: &Path) -> Result { +fn load_context_store(path: &Path) -> RunContextStore { if !path.exists() { - return Ok(RunContextStore::default()); + return RunContextStore::default(); + } + let raw = match fs::read_to_string(path) { + Ok(raw) => raw, + Err(err) => { + eprintln!( + "warning: failed to read context store {}: {err}; ignoring saved context", + path.display() + ); + return RunContextStore::default(); + } + }; + match serde_json::from_str::(&raw) { + Ok(store) => store, + Err(err) => { + eprintln!( + "warning: failed to parse context store {}: {err}; ignoring saved context", + path.display() + ); + RunContextStore::default() + } } - let raw = - fs::read_to_string(path).with_context(|| format!("reading context store {}", path.display()))?; - let parsed: RunContextStore = serde_json::from_str(&raw) - .with_context(|| format!("parsing context store {}", path.display()))?; - Ok(parsed) } fn save_context_store(path: &Path, store: &RunContextStore) -> Result<()> { @@ -827,10 +910,9 @@ mod tests { #[test] fn all_target_expands_to_all_adapters() { - let adapters = adapters_for_target(CommandTargetArg::All); - let names = adapters + let names = selected_targets(CommandTargetArg::All) .into_iter() - .map(|a| a.target().to_string()) + .map(|target| target.to_string()) .collect::>(); assert_eq!( names, @@ -998,6 +1080,63 @@ mod tests { Ok(()) } + #[test] + fn install_all_skips_missing_environments_instead_of_failing() -> Result<()> { + let home = unique_dir("rally-command-install-all-skips"); + let codex_root = prepare_target_root(&home, HarnessTarget::Codex)?; + let env = env_with_home(&home); + let render_ctx = WrapperRenderContext::default(); + + let outcomes = install_with_environment( + &install_args(CommandTargetArg::All, false, false), + &env, + &render_ctx, + )?; + let codex = outcomes + .iter() + .find(|o| o.target == HarnessTarget::Codex) + .expect("codex outcome"); + let droid = outcomes + .iter() + .find(|o| o.target == HarnessTarget::Droid) + .expect("droid outcome"); + assert_eq!(codex.action, InstallAction::Create); + assert_eq!(droid.action, InstallAction::SkipEnvironmentMissing); + assert!(codex_root.join("commands").join("rally.md").exists()); + Ok(()) + } + + #[test] + fn marker_substring_does_not_make_file_managed() -> Result<()> { + let home = unique_dir("rally-command-marker-substring"); + let root = prepare_target_root(&home, HarnessTarget::Pi)?; + let wrapper = root.join("commands").join("rally.md"); + fs::create_dir_all(wrapper.parent().expect("wrapper parent"))?; + fs::write( + &wrapper, + "user file\ncontains somewhere\n", + )?; + let env = env_with_home(&home); + let render_ctx = WrapperRenderContext::default(); + + let install_err = install_with_environment( + &install_args(CommandTargetArg::Pi, false, false), + &env, + &render_ctx, + ) + .expect_err("marker substring should not be treated as managed"); + assert!( + install_err + .to_string() + .contains("refusing to overwrite non-Rally file") + ); + + let uninstall = uninstall_with_environment(&uninstall_args(CommandTargetArg::Pi, false), &env)?; + assert_eq!(uninstall[0].action, UninstallAction::SkippedUnmanaged); + assert!(wrapper.exists()); + Ok(()) + } + #[test] fn doctor_reports_managed_install_status() -> Result<()> { let home = unique_dir("rally-command-doctor-managed"); @@ -1020,6 +1159,27 @@ mod tests { Ok(()) } + #[test] + fn doctor_reports_wrong_target_when_header_target_mismatches_path() -> Result<()> { + let home = unique_dir("rally-command-doctor-target-mismatch"); + let root = prepare_target_root(&home, HarnessTarget::Codex)?; + let wrapper = root.join("commands").join("rally.md"); + fs::create_dir_all(wrapper.parent().expect("wrapper parent"))?; + fs::write( + &wrapper, + "\n\n\n# /rally\n", + )?; + let env = env_with_home(&home); + let render_ctx = WrapperRenderContext::default(); + let result = doctor_with_environment( + &doctor_args(CommandTargetArg::Codex, false), + &env, + &render_ctx, + )?; + assert_eq!(result[0].status, DoctorStatus::InstalledManagedWrongTarget); + Ok(()) + } + #[test] fn uninstall_removes_only_managed_wrappers() -> Result<()> { let home = unique_dir("rally-command-uninstall-managed"); @@ -1175,4 +1335,15 @@ mod tests { .contains("unable to resolve session from explicit args") ); } + + #[test] + fn load_context_store_ignores_malformed_json() -> Result<()> { + let home = unique_dir("rally-command-context-malformed"); + let path = home.join(".rally").join(CONTEXT_STORE_FILE); + fs::create_dir_all(path.parent().expect("context dir"))?; + fs::write(&path, "{not-json}")?; + let loaded = load_context_store(&path); + assert!(loaded.by_workspace.is_empty()); + Ok(()) + } } diff --git a/src/lib.rs b/src/lib.rs index 83dc51f..b9254d9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -94,15 +94,24 @@ pub fn run_cli_with_registry(cli: Cli, registry: &WorkflowRegistry) -> Result { - match args.command { - CommandSubcommand::Install(inner) => command_surface::install(&inner)?, + let exit_code = match args.command { + CommandSubcommand::Install(inner) => { + command_surface::install(&inner)?; + 0 + } CommandSubcommand::Run(inner) | CommandSubcommand::Exec(inner) => { command_surface::run(&inner, registry)? } - CommandSubcommand::Doctor(inner) => command_surface::doctor(&inner)?, - CommandSubcommand::Uninstall(inner) => command_surface::uninstall(&inner)?, - } - Ok(0) + CommandSubcommand::Doctor(inner) => { + command_surface::doctor(&inner)?; + 0 + } + CommandSubcommand::Uninstall(inner) => { + command_surface::uninstall(&inner)?; + 0 + } + }; + Ok(exit_code) } } } diff --git a/tests/command_install_run.rs b/tests/command_install_run.rs index c51417b..89535fc 100644 --- a/tests/command_install_run.rs +++ b/tests/command_install_run.rs @@ -8,7 +8,12 @@ use std::{ use anyhow::{Context, Result}; use clap::Parser; -use rally::{cli::Cli, run_cli_with_registry, workflow}; +use rally::{ + cli::Cli, + run_cli_with_registry, + state::{SessionHandle, SessionPhase}, + workflow, +}; use serde_json::Value; fn unique_name(prefix: &str) -> String { @@ -227,14 +232,18 @@ fn command_install_run_entrypoint_integration() -> Result<()> { )?; let session_one_seen_before = read_agent_last_seen(&temp_home, &session_one, "implementer")?; - run_cli( + assert_eq!( + run_cli( &["rally", "command", "run", "--non-interactive"], ®istry, - )?; + )?, + 0 + ); let session_one_seen_after = read_agent_last_seen(&temp_home, &session_one, "implementer")?; assert_ne!(session_one_seen_before, session_one_seen_after); - run_cli( + assert_eq!( + run_cli( &[ "rally", "command", @@ -243,10 +252,13 @@ fn command_install_run_entrypoint_integration() -> Result<()> { "--non-interactive", ], ®istry, - )?; + )?, + 0 + ); assert_eq!(read_context_session(&temp_home, &manifest_dir)?, session_two); - run_cli( + assert_eq!( + run_cli( &[ "rally", "command", @@ -260,9 +272,29 @@ fn command_install_run_entrypoint_integration() -> Result<()> { "--non-interactive", ], ®istry, - )?; + )?, + 0 + ); assert_eq!(read_context_session(&temp_home, &manifest_dir)?, session_one); + fs::write(&context_store, "{not-json}")?; + assert_eq!( + run_cli( + &[ + "rally", + "command", + "run", + "--session", + &session_one, + "--as", + "implementer", + "--non-interactive", + ], + ®istry, + )?, + 0 + ); + fs::remove_file(&context_store)?; let mut child = Command::new(env!("CARGO_BIN_EXE_rally")) .arg("command") @@ -288,6 +320,29 @@ fn command_install_run_entrypoint_integration() -> Result<()> { ); assert_eq!(read_context_session(&temp_home, &manifest_dir)?, session_two); + { + let handle = SessionHandle::open(&session_two)?; + let mut state = handle.load_state()?; + state.phase = SessionPhase::Done; + handle.save_state(&state)?; + } + assert_eq!( + run_cli( + &[ + "rally", + "command", + "run", + "--session", + &session_two, + "--as", + "implementer", + "--non-interactive", + ], + ®istry, + )?, + 2 + ); + if let Some(value) = old_home { // SAFETY: restore original HOME at test teardown. unsafe { env::set_var("HOME", value) }; From cea7e6f0fcddf7b8d360db6db600626adaaad3b9 Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Mon, 2 Mar 2026 00:19:34 -0600 Subject: [PATCH 10/11] Make context save failure non-fatal after command run --- src/command_surface.rs | 7 ++++++- tests/command_install_run.rs | 24 ++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/command_surface.rs b/src/command_surface.rs index ef5cde8..8c3eee0 100644 --- a/src/command_surface.rs +++ b/src/command_surface.rs @@ -102,7 +102,12 @@ pub fn run(args: &CommandRunArgs, registry: &WorkflowRegistry) -> Result { workflow: resolved.workflow, }, ); - save_context_store(&store_path, &store)?; + if let Err(err) = save_context_store(&store_path, &store) { + eprintln!( + "warning: failed to save context store {}: {err}; continuing without persisted context", + store_path.display() + ); + } Ok(code) } diff --git a/tests/command_install_run.rs b/tests/command_install_run.rs index 89535fc..867a30e 100644 --- a/tests/command_install_run.rs +++ b/tests/command_install_run.rs @@ -343,6 +343,30 @@ fn command_install_run_entrypoint_integration() -> Result<()> { 2 ); + // If persisted-context saving fails after `next` succeeds, `command run` must still + // return the `next` exit code instead of failing the command. + if context_store.exists() { + fs::remove_file(&context_store)?; + } + fs::create_dir_all(&context_store)?; + assert_eq!( + run_cli( + &[ + "rally", + "command", + "run", + "--session", + &session_one, + "--as", + "implementer", + "--non-interactive", + ], + ®istry, + )?, + 0 + ); + fs::remove_dir_all(&context_store)?; + if let Some(value) = old_home { // SAFETY: restore original HOME at test teardown. unsafe { env::set_var("HOME", value) }; From f96c9f4ee2aea934153da2771b0551f8eed593f8 Mon Sep 17 00:00:00 2001 From: Justin Moon Date: Mon, 2 Mar 2026 00:47:46 -0600 Subject: [PATCH 11/11] Use builtin workflow registry in command integration test --- tests/command_install_run.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/command_install_run.rs b/tests/command_install_run.rs index 867a30e..5375379 100644 --- a/tests/command_install_run.rs +++ b/tests/command_install_run.rs @@ -12,7 +12,7 @@ use rally::{ cli::Cli, run_cli_with_registry, state::{SessionHandle, SessionPhase}, - workflow, + workflow::{self}, }; use serde_json::Value; @@ -29,6 +29,14 @@ fn run_cli(args: &[&str], registry: &rally::WorkflowRegistry) -> Result { run_cli_with_registry(cli, registry) } +fn builtin_registry() -> Result { + let mut registry = rally::WorkflowRegistry::new(); + registry.register(workflow::builtin::PlanWorkflow)?; + registry.register(workflow::builtin::BuildWorkflow)?; + registry.register(workflow::builtin::ComposedNegotiateWorkflow)?; + Ok(registry) +} + fn write_todo(path: &Path, title: &str) -> Result<()> { fs::write( path, @@ -82,7 +90,7 @@ fn command_install_run_entrypoint_integration() -> Result<()> { // SAFETY: this integration test runs in its own process and restores HOME before returning. unsafe { env::set_var("HOME", &temp_home) }; - let registry = workflow::default_registry()?; + let registry = builtin_registry()?; let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); let codex_root = temp_home.join(".codex");