diff --git a/Cargo.toml b/Cargo.toml index 9be8a8f..7d4df65 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,8 +76,11 @@ tui = [ # the current source layout; the epic's agent rework (#100) is what will let # them separate. pr = ["tui", "dep:sendit"] -# The code-agent integration (PR auto-fill). Currently gates no external -# dependency; it will gate `agent-text` once the issue flow lands. +# The code-agent integration (the PR auto-fill and the `wt issue` branch/brief +# proposal). Currently gates no external dependency; it will gate `agent-text` +# if and when the multi-provider agent migration lands. Note the issue flow is +# deliberately *not* gated on it: `wt issue` must work with no agent at all +# (issue #98), so it falls back rather than disappearing from the CLI. agent = ["cli"] [dependencies] diff --git a/README.md b/README.md index aa7f602..6006913 100644 --- a/README.md +++ b/README.md @@ -71,15 +71,15 @@ step that sets up everything; there's no separate completions install. (A static values-unaware script is still available via `wt completions ` if you want to manage it yourself.) -### 3. Authenticate `gh` (only for PR commands) +### 3. Authenticate `gh` (for PR and issue commands) ```bash gh auth login ``` -Everything except `wt pr` works fully offline. If `gh` is missing or -unauthenticated, only the PR commands fail (with an actionable message); the rest -keep working. +Everything except `wt pr` and `wt issue` works fully offline. If `gh` is missing +or unauthenticated, only those commands fail (with an actionable message); the +rest keep working. ### 4. Open it @@ -89,6 +89,7 @@ sorting, and filtering are all discoverable from there. For example: ```bash wt new feature/login # create the branch + worktree and switch into it +wt issue 123 # branch + worktree for a GitHub issue, name proposed for you wt switch # fuzzy-pick a worktree to jump to ``` @@ -109,6 +110,20 @@ These are the things worth knowing up front; the rest is discoverable from suggest existing local **and** remote branches to fork from or check out — type to filter, `↑/↓` to pick, `Enter` to accept, or just type a brand-new name. The PR compose form's model and effort fields list their choices the same way. +- **Start from a GitHub issue.** `wt issue 123` fetches the issue's title, body, + labels, type and milestone, proposes a conventional `type/123-slug` branch and a + short implementation brief, lets you edit both, then creates the worktree and + records the link. `wt list` can show it with the opt-in `issue` column + (`wt config set list.columns status,dirty,branch,issue,path`), and running + `wt issue 123` again reuses the branch it already made. + + Generation is best-effort and never blocks you: if the agent is missing, hangs, + or returns nonsense, `wt` falls back to a deterministic name built from the + issue's own labels and title, tells you it did, and carries on. Set which model + it uses under `[agent.generation]`, or per-run with `--model`/`--effort`. + + `wt` stops at the prepared worktree — it does not run a coding agent for you. + Handing the work to an agent is [karet](https://github.com/getkono/karet)'s job. - **Where worktrees are created.** New worktrees follow a configurable path template. The default keeps them beside the repo, out of it, and prefixes each worktree directory with the repo name so it's obvious which repo you're in: @@ -173,6 +188,20 @@ These are the things worth knowing up front; the rest is discoverable from the user config); precedence is flags > repo > global. `wt init` is an optional convenience that scaffolds a starter `.wt.toml` and, for a subdir store, offers to add it to `.gitignore`. +- **Pick the generation model.** The short, structured generation steps `wt` + owns — the `wt pr open --ai` draft and the `wt issue` branch/brief proposal — + read one profile: + + ```toml + [agent.generation] + provider = "claude" + model = "sonnet" # opus | sonnet | haiku + effort = "medium" # low | medium | high + ``` + + The older flat `agent.model` / `agent.effort` keys still work and mean the same + thing. `[agent.work]` is deliberately not a `wt` setting: running a coding agent + on the work belongs to [karet](https://github.com/getkono/karet). - **Theme the TUI.** Pick a built-in palette and tweak individual colors under `[ui.theme]`: `preset` selects the base (`one-dark` (default) or `solarized`), and the named slots (`accent`, `green`, `red`, `yellow`, `orange`, `cyan`, diff --git a/src/agent/mod.rs b/src/agent/mod.rs index a9c41e5..36e8365 100644 --- a/src/agent/mod.rs +++ b/src/agent/mod.rs @@ -11,8 +11,10 @@ pub mod model; pub mod spec; pub mod types; +use std::io::Read; use std::path::Path; -use std::process::Command; +use std::process::{Command, ExitStatus, Stdio}; +use std::time::{Duration, Instant}; use crate::error::{Error, Result}; pub use model::{AgentModel, AgentOptions, Effort}; @@ -70,7 +72,7 @@ impl AgentClient for RealAgent { /// [`RealAgent::detect`] so tests can drive every branch with a stand-in /// binary. A missing binary maps to `Ok(None)`; other failures propagate. fn detect_with(binary: &str, kind: AgentKind, spec: &AgentSpec) -> Result> { - match run_agent(binary, None, &spec::version_argv(spec)) { + match run_agent(binary, None, &spec::version_argv(spec), None) { Ok(stdout) => Ok(Some(DetectedAgent { kind, binary: binary.to_string(), @@ -93,38 +95,125 @@ fn run_with( ) -> Result { let prompt = spec::apply_effort(opts.effort, prompt); let argv = spec::prompt_argv(spec, &prompt, opts.model); - let stdout = run_agent(binary, Some(dir), &argv)?; + let stdout = run_agent(binary, Some(dir), &argv, opts.timeout)?; spec::parse_result(kind, spec.result_format, &stdout) } /// Runs an agent `binary` (optionally in `dir`), mapping a missing binary to /// [`Error::AgentUnavailable`] and a non-zero exit to [`Error::Subprocess`]. /// Mirrors `gh`'s `run_gh` helper. -fn run_agent(binary: &str, dir: Option<&Path>, args: &[String]) -> Result { +/// +/// With `timeout` set, the child is killed and [`Error::AgentTimeout`] returned +/// once the deadline passes. `None` waits indefinitely — the historical +/// behaviour, kept for version detection and for callers that pass no deadline. +fn run_agent( + binary: &str, + dir: Option<&Path>, + args: &[String], + timeout: Option, +) -> Result { let mut cmd = Command::new(binary); if let Some(dir) = dir { cmd.current_dir(dir); } cmd.args(args); - let output = match cmd.output() { - Ok(output) => output, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - return Err(Error::AgentUnavailable(format!( - "{binary} is not installed or not on PATH" - ))); + + let Some(limit) = timeout else { + return match cmd.output() { + Ok(output) => finish(binary, output.status, &output.stdout, &output.stderr), + Err(e) => Err(spawn_error(binary, &e)), + }; + }; + + // `Command::output()` blocks until the pipes close, so it cannot be given a + // deadline. Read the pipes on their own threads and keep the `Child` here, + // so this thread still owns the handle it needs to kill. + cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + let mut child = match cmd.spawn() { + Ok(child) => child, + Err(e) => return Err(spawn_error(binary, &e)), + }; + let mut out_pipe = child.stdout.take(); + let mut err_pipe = child.stderr.take(); + // Draining both pipes concurrently matters: a child that fills the stderr + // pipe buffer blocks forever if only stdout is being read. + let out_reader = std::thread::spawn(move || read_pipe(out_pipe.as_mut())); + let err_reader = std::thread::spawn(move || read_pipe(err_pipe.as_mut())); + + let deadline = Instant::now() + limit; + let status = loop { + match child.try_wait() { + Ok(Some(status)) => break Some(status), + Ok(None) => {} + Err(e) => return Err(spawn_error(binary, &e)), } - Err(e) => { - return Err(Error::AgentUnavailable(format!( - "failed to run {binary}: {e}" - ))); + if Instant::now() >= deadline { + // Killing closes the child's pipes, which is what lets the reader + // threads below finish rather than block forever. + let _ = child.kill(); + let _ = child.wait(); + break None; } + std::thread::sleep(POLL_INTERVAL); }; - if output.status.success() { - return Ok(String::from_utf8_lossy(&output.stdout).into_owned()); + + match status { + Some(status) => { + // A panicking reader yields no output rather than poisoning the run. + let stdout = out_reader.join().unwrap_or_default(); + let stderr = err_reader.join().unwrap_or_default(); + finish(binary, status, &stdout, &stderr) + } + None => { + // Deliberately *not* joined. Killing the child does not close the + // pipes if it left a grandchild holding them — an agent CLI that is + // a wrapper script is exactly that shape — so a reader would block + // for as long as the grandchild lives, which is precisely what the + // deadline exists to prevent. The output is unwanted anyway, so the + // readers are detached; they end on their own when the pipes close. + drop(out_reader); + drop(err_reader); + Err(Error::AgentTimeout { + binary: binary.to_string(), + // Sub-second deadlines still report `1s`; the message is for + // humans, and "did not respond within 0s" reads as a bug. + seconds: limit.as_secs().max(1), + }) + } + } +} + +/// How often the deadline loop checks whether the child has exited. Short +/// enough that a fast agent is not held up perceptibly, long enough not to spin. +const POLL_INTERVAL: Duration = Duration::from_millis(10); + +/// Drains a child pipe to end, yielding empty bytes if it is absent or fails. +fn read_pipe(pipe: Option<&mut impl Read>) -> Vec { + let mut buf = Vec::new(); + if let Some(pipe) = pipe { + let _ = pipe.read_to_end(&mut buf); + } + buf +} + +/// Maps a spawn failure to [`Error::AgentUnavailable`], distinguishing a missing +/// binary from any other launch failure. +fn spawn_error(binary: &str, e: &std::io::Error) -> Error { + if e.kind() == std::io::ErrorKind::NotFound { + Error::AgentUnavailable(format!("{binary} is not installed or not on PATH")) + } else { + Error::AgentUnavailable(format!("failed to run {binary}: {e}")) + } +} + +/// Maps a finished process to its stdout, or to [`Error::Subprocess`]. +fn finish(binary: &str, status: ExitStatus, stdout: &[u8], stderr: &[u8]) -> Result { + if status.success() { + return Ok(String::from_utf8_lossy(stdout).into_owned()); } Err(Error::Subprocess { program: binary.to_string(), - stderr: String::from_utf8_lossy(&output.stderr).trim().to_string(), + stderr: String::from_utf8_lossy(stderr).trim().to_string(), }) } @@ -205,7 +294,7 @@ mod tests { #[test] fn run_agent_maps_missing_binary_to_unavailable() { - let err = run_agent(MISSING, None, &["--version".to_string()]).unwrap_err(); + let err = run_agent(MISSING, None, &["--version".to_string()], None).unwrap_err(); assert!(matches!(err, Error::AgentUnavailable(_))); } @@ -254,20 +343,97 @@ mod tests { #[test] fn run_agent_returns_stdout_on_success() { - let out = - run_agent("sh", None, &["-c".to_string(), "printf hello".to_string()]).unwrap(); + let out = run_agent( + "sh", + None, + &["-c".to_string(), "printf hello".to_string()], + None, + ) + .unwrap(); assert_eq!(out, "hello"); } #[test] fn run_agent_maps_nonzero_exit_to_subprocess() { - let err = run_agent("sh", None, &["-c".to_string(), "exit 3".to_string()]).unwrap_err(); + let err = + run_agent("sh", None, &["-c".to_string(), "exit 3".to_string()], None).unwrap_err(); match err { Error::Subprocess { program, .. } => assert_eq!(program, "sh"), other => panic!("expected subprocess error, got {other:?}"), } } + #[test] + fn run_agent_kills_a_child_that_outlives_its_deadline() { + // Two defects in one test. + // + // First: `Command::output()` waits forever, so an agent that hangs + // used to hang `wt`. Sleeping far longer than the deadline proves the + // deadline — not the sleep — is what ends the run. + // + // Second, and the subtler one: `sleep 30 & wait` makes `sh` fork a + // *grandchild* that inherits the stdout/stderr pipes. Killing the + // child does not close them, so joining the reader threads blocks + // until the grandchild dies — reintroducing the full 30s wait behind + // a timeout that appears to work. An agent CLI that is a wrapper + // script has exactly this shape, so the plain `sleep 30` this test + // first used was too weak: it passes on a shell that `exec`s. + let started = Instant::now(); + let err = run_agent( + "sh", + None, + &["-c".to_string(), "sleep 30 & wait".to_string()], + Some(Duration::from_millis(100)), + ) + .unwrap_err(); + let elapsed = started.elapsed(); + match err { + Error::AgentTimeout { binary, seconds } => { + assert_eq!(binary, "sh"); + // Sub-second deadlines still report a whole second. + assert_eq!(seconds, 1); + } + other => panic!("expected a timeout, got {other:?}"), + } + assert!( + elapsed < Duration::from_secs(10), + "returned after {elapsed:?}; the child was not killed" + ); + } + + #[test] + fn a_deadline_does_not_disturb_a_process_that_finishes() { + // The deadline path reads the pipes on separate threads, so prove it + // still returns stdout intact rather than only working on timeout. + let out = run_agent( + "sh", + None, + &["-c".to_string(), "printf hello".to_string()], + Some(Duration::from_secs(30)), + ) + .unwrap(); + assert_eq!(out, "hello"); + } + + #[test] + fn a_deadline_still_maps_a_nonzero_exit_to_subprocess() { + let err = run_agent( + "sh", + None, + &["-c".to_string(), "printf oops >&2; exit 3".to_string()], + Some(Duration::from_secs(30)), + ) + .unwrap_err(); + match err { + Error::Subprocess { program, stderr } => { + assert_eq!(program, "sh"); + // Proves stderr is drained too, not just stdout. + assert_eq!(stderr, "oops"); + } + other => panic!("expected subprocess error, got {other:?}"), + } + } + #[test] fn detect_with_parses_version_from_real_process() { let detected = detect_with("sh", AgentKind::Claude, &SH_VERSION) diff --git a/src/agent/model.rs b/src/agent/model.rs index ada4a56..ebc2e3d 100644 --- a/src/agent/model.rs +++ b/src/agent/model.rs @@ -6,6 +6,8 @@ //! drive the config layer, the CLI flags, and the TUI's live cycle keys without //! any process or I/O. +use std::time::Duration; + use serde::Serialize; /// A selectable model tier for a code agent. The variants currently encode the @@ -159,13 +161,20 @@ impl Effort { } } -/// The model and effort selected for a single agent run. +/// The model, effort, and deadline selected for a single agent run. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub struct AgentOptions { /// The model tier to drive. pub model: AgentModel, /// How much effort to spend. pub effort: Effort, + /// How long to wait before killing the agent. `None` waits indefinitely, + /// which is the historical behaviour and remains the default. + /// + /// Deliberately a [`Duration`] rather than anything owned: `AgentOptions` is + /// `Copy`, and the compose form, the TUI and `pr_open`'s tests all rely on + /// that. + pub timeout: Option, } #[cfg(test)] diff --git a/src/cli.rs b/src/cli.rs index 1c7344d..dc9e7f3 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -85,6 +85,8 @@ pub(crate) enum Command { /// Check out a GitHub PR into its own worktree. #[command(args_conflicts_with_subcommands = true)] Pr(PrArgs), + /// Create a worktree for a GitHub issue, with a generated branch and brief. + Issue(IssueArgs), /// Detailed status for one or all worktrees. Status(StatusArgs), /// Print the absolute path of a matching worktree. @@ -250,6 +252,57 @@ fn submodule_override(init: bool, no_init: bool) -> Option { } } +/// Arguments for `wt issue`. +/// +/// `wt` proposes a branch and brief and creates the worktree; it does not run a +/// coding agent, so there are no agent-execution flags here (issue #100). The +/// `--model`/`--effort` pair tunes only the short generation step, and matches +/// `wt pr open`'s flags because both drive the same `[agent.generation]` +/// profile. +#[derive(Debug, Clone, Args)] +pub(crate) struct IssueArgs { + /// GitHub issue number or URL. + pub(crate) target: String, + /// Use this branch instead of the generated one. + #[arg(long, value_name = "NAME")] + pub(crate) branch: Option, + /// Base ref for a new branch (default: `origin/HEAD`, then the repo default). + #[arg(long, value_name = "REF")] + pub(crate) from: Option, + /// Use this brief instead of the generated one. + #[arg(long, value_name = "TEXT")] + pub(crate) brief: Option, + /// Model for generating the branch and brief (overrides `agent.generation.model`). + #[arg(long, value_name = "MODEL")] + pub(crate) model: Option, + /// Effort for generating the branch and brief (overrides `agent.generation.effort`). + #[arg(long, value_name = "LEVEL")] + pub(crate) effort: Option, + /// Do not switch into the new worktree. + #[arg(long = "no-switch")] + pub(crate) no_switch: bool, + /// Skip the post_create hook. + #[arg(long = "no-hooks")] + pub(crate) no_hooks: bool, + /// Worktree to copy configured files from (default: the current one). + #[arg(long = "copy-from", value_name = "QUERY")] + pub(crate) copy_from: Option, + /// Initialize git submodules after creating the worktree. + #[arg(long = "init-submodules", conflicts_with = "no_init_submodules")] + pub(crate) init_submodules: bool, + /// Do not initialize git submodules. + #[arg(long = "no-init-submodules")] + pub(crate) no_init_submodules: bool, +} + +impl IssueArgs { + /// Resolves the submodule-init flags to an override for the config policy + /// (see [`NewArgs::submodule_override`]). + pub(crate) fn submodule_override(&self) -> Option { + submodule_override(self.init_submodules, self.no_init_submodules) + } +} + /// Arguments for `wt list`. #[derive(Debug, Args)] pub(crate) struct ListArgs { @@ -505,6 +558,7 @@ impl Cli { Some(Command::Drop(_)) => "drop", Some(Command::Prune(_)) => "prune", Some(Command::Pr(_)) => "pr", + Some(Command::Issue(_)) => "issue", Some(Command::Status(_)) => "status", Some(Command::Path(_)) => "path", Some(Command::Root) => "root", @@ -608,6 +662,9 @@ fn route(cli: Cli, cx: &mut Cx) -> Result { Some(Command::Pr(args)) => { crate::commands::pr::run(cx, &crate::hooks::RealHookRunner, &args, json) } + Some(Command::Issue(args)) => { + crate::commands::issue::run(cx, &crate::hooks::RealHookRunner, &args) + } Some(Command::Status(args)) => crate::commands::status_cmd::run(cx, &args, json), Some(Command::Path(args)) => crate::commands::path::run(cx, &args), Some(Command::Root) => crate::commands::root::run(cx), diff --git a/src/commands/complete.rs b/src/commands/complete.rs index 0fc35dc..87831ad 100644 --- a/src/commands/complete.rs +++ b/src/commands/complete.rs @@ -63,6 +63,18 @@ fn candidates(cx: &Cx, kind: &str) -> Result> { .map(|n| n.to_string()) .collect()) } + "issue-numbers" => { + // Best-effort, exactly like `pr-numbers`: GitHub being unreachable + // must never make shell completion fail. + let dir = repo.current_workdir().unwrap_or_else(|| cx.cwd.clone()); + Ok(cx + .gh + .list_open_issues(&dir) + .unwrap_or_default() + .into_iter() + .map(|issue| issue.number.to_string()) + .collect()) + } _ => Ok(Vec::new()), } } @@ -70,6 +82,7 @@ fn candidates(cx: &Cx, kind: &str) -> Result> { #[cfg(test)] mod tests { use crate::cli::CompleteArgs; + use crate::gh::IssueSummary; use crate::testutil::TestRepo; fn args(kind: &str, partial: Option<&str>) -> CompleteArgs { @@ -102,6 +115,37 @@ mod tests { assert!(out.contains("topic")); } + #[test] + fn completes_open_issue_numbers() { + let repo = TestRepo::init(); + let mut t = crate::testutil::test_cx(&[], repo.root().to_str().unwrap()); + t.cx.gh = std::sync::Arc::new(crate::testutil::FakeGh::default().with_issue_list(vec![ + IssueSummary { + number: 7, + title: "Add login".into(), + state: "OPEN".into(), + labels: Vec::new(), + issue_type: None, + milestone: None, + created_at: String::new(), + url: String::new(), + }, + ])); + super::run(&mut t.cx, &args("issue-numbers", None)).unwrap(); + assert!(t.out.contents().contains('7')); + } + + #[test] + fn issue_completion_is_silent_when_github_is_unreachable() { + // Completion runs on every keystroke; an unauthenticated `gh` must + // produce an empty list, never an error. + let repo = TestRepo::init(); + let mut t = crate::testutil::test_cx(&[], repo.root().to_str().unwrap()); + t.cx.gh = std::sync::Arc::new(crate::testutil::FakeGh::unavailable()); + super::run(&mut t.cx, &args("issue-numbers", None)).unwrap(); + assert!(t.out.contents().trim().is_empty()); + } + #[test] fn completes_all_branches_including_remote() { // `checkout` completion (the `all-branches` kind) lists local branches and diff --git a/src/commands/config_cmd.rs b/src/commands/config_cmd.rs index ec241dd..1e7a1e6 100644 --- a/src/commands/config_cmd.rs +++ b/src/commands/config_cmd.rs @@ -26,6 +26,9 @@ const KEYS: &[&str] = &[ "submodules.init", "submodules.seed", "create.reflink", + "agent.generation.provider", + "agent.generation.model", + "agent.generation.effort", "agent.model", "agent.effort", "list.show_untracked", @@ -204,6 +207,9 @@ fn key_type(key: &str) -> Option { | "submodules.init" | "submodules.seed" | "create.reflink" + | "agent.generation.provider" + | "agent.generation.model" + | "agent.generation.effort" | "agent.model" | "agent.effort" | "ui.color" @@ -244,8 +250,14 @@ fn config_value(config: &Config, key: &str) -> Result> { "submodules.init" => Some(config.submodules_init.identifier().to_string()), "submodules.seed" => Some(config.submodules_seed.identifier().to_string()), "create.reflink" => Some(config.create_reflink.identifier().to_string()), - "agent.model" => Some(config.agent_model.id().to_string()), - "agent.effort" => Some(config.agent_effort.id().to_string()), + "agent.generation.provider" => Some(config.agent_generation.provider.as_str().to_string()), + // The flat keys remain readable aliases for the same two settings. + "agent.generation.model" | "agent.model" => { + Some(config.agent_generation.model.id().to_string()) + } + "agent.generation.effort" | "agent.effort" => { + Some(config.agent_generation.effort.id().to_string()) + } "list.show_untracked" => Some(config.list_show_untracked.to_string()), "list.columns" => Some( config diff --git a/src/commands/issue.rs b/src/commands/issue.rs new file mode 100644 index 0000000..548e819 --- /dev/null +++ b/src/commands/issue.rs @@ -0,0 +1,962 @@ +//! `wt issue` — create (or reuse) a worktree for a GitHub issue, with a branch +//! name and implementation brief proposed by the generation agent. +//! +//! `wt` deliberately stops here: it fetches the issue, proposes a branch and +//! brief, creates the worktree, and records the link. It does **not** launch a +//! coding agent — running the work is karet's job over the Agent Client +//! Protocol (issue #100). What this command hands back is a prepared worktree +//! and the metadata to find it again. +//! +//! Generation is best-effort throughout. A model that returns nonsense, an agent +//! that is not installed, and an agent that hangs all degrade to a deterministic +//! fallback branch built from the issue's own labels and title (issue #98), so +//! **worktree creation never depends on model behaviour**. +//! +//! The `TYPE/{number}-SLUG` contract itself lives in [`crate::naming`], which is +//! pure and shared: the prompt fragment the model is given and the validator its +//! answer is checked against come from the same place and cannot drift +//! (issue #96). + +use crate::agent::{AgentClient, AgentKind, AgentModel, AgentOptions, Effort}; +use crate::cli::IssueArgs; +use crate::commands::{Session, confirm, finish_worktree, open_session, report_created}; +use crate::config::{Config, wtconfig}; +use crate::cx::Cx; +use crate::error::{Error, Result}; +use crate::gh::IssueView; +use crate::git::cli::GitCli; +use crate::git::discover::Repo; +use crate::git::{all_branches, default_base_ref, local_branches, validate_branch_name}; +use crate::hooks::{HookContext, HookRunner}; +use crate::naming::{self, BranchKind}; +use crate::progress; +use crate::worktree::{ + CreateOptions, MetaUpdate, apply_meta, create_in, lock_repo, preview_target, +}; + +use super::Nav; + +/// How long the generation agent gets before it is killed and the fallback is +/// used. Generous enough for a short structured answer, short enough that a +/// hung agent does not hold up a worktree. +const GENERATION_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(120); + +/// The branch name and implementation brief for an issue worktree. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct IssuePlan { + /// The branch to create or reuse. + pub(crate) branch: String, + /// The implementation brief; empty when generation produced none. + pub(crate) brief: String, +} + +/// Runs `wt issue`. +pub(crate) fn run(cx: &mut Cx, hooks: &dyn HookRunner, args: &IssueArgs) -> Result { + // Gate before anything costly. `wt issue` reviews values with the user, so + // without a terminal it needs `--yes` — and finding that out *after* a + // network round trip and an LLM call would be rude and slow. + if !cx.err.is_tty() && !cx.assume_yes { + return Err(Error::usage( + "`wt issue` requires an interactive terminal or `--yes`", + )); + } + + let git = cx.git.clone(); + let session = open_session(cx, git.as_ref())?; + let dir = session + .repo + .current_workdir() + .unwrap_or_else(|| session.primary_root.clone()); + + let gh = cx.gh.clone(); + let target = args.target.clone(); + let issue = progress::run(&mut cx.err, "Fetching issue", move || { + gh.view_issue(&dir, &target) + })?; + + // An issue already linked to a branch reuses it, which costs no LLM call at + // all — the common case of returning to work in progress. + let linked = linked_branch(session.repo.gix(), issue.number)?; + let mut plan = match &linked { + Some(branch) => { + if let Some(requested) = &args.branch + && requested != branch + { + return Err(Error::usage(format!( + "issue #{} is already linked to branch {branch:?}; \ + drop --branch or pass that branch", + issue.number + ))); + } + cx.err.line(&format!( + "issue #{} is already linked to branch {branch:?}", + issue.number + ))?; + IssuePlan { + branch: branch.clone(), + brief: args.brief.clone().unwrap_or_default(), + } + } + None => resolve_plan(cx, &session, &issue, args)?, + }; + + let mut base = args + .from + .clone() + .or_else(|| default_base_ref(session.repo.gix())); + + // Review the proposal. Fields the user pinned with a flag are not re-asked. + if !cx.assume_yes { + let branches = all_branches(session.repo.gix()).unwrap_or_default(); + if args.branch.is_none() && linked.is_none() { + plan.branch = prompt_value(cx, "branch", &plan.branch)?; + } + if args.from.is_none() { + base = prompt_base(cx, base.as_deref(), &branches)?; + } + if args.brief.is_none() { + plan.brief = prompt_value(cx, "brief", &plan.brief)?; + } + } + + // A human-chosen name only has to be a legal git branch. The naming contract + // exists to constrain the *model*; the issue link lives in git config, never + // in the branch name, so an off-contract name still resolves back to its + // issue and breaks nothing. + validate_branch_name(&plan.branch).map_err(Error::usage)?; + if naming::parse_and_validate(&plan.branch, issue.number).is_err() { + cx.err.line(&format!( + "note: {:?} does not follow TYPE/{}-SLUG; the issue link is recorded \ + in git config, so this still works", + plan.branch, issue.number + ))?; + } + + let env = cx.env.clone(); + let preview = preview_target(&session.parts(&env), &plan.branch, base.as_deref())?; + let prompt = format!( + "Issue #{}: {}\nBranch: {}\nBase: {}\nWorktree: {}\nBrief: {}\n\ + Create this issue worktree? [y/N] ", + issue.number, + issue.title, + plan.branch, + base.as_deref().unwrap_or("(repository default)"), + preview.display(), + if plan.brief.trim().is_empty() { + "(none)" + } else { + plan.brief.trim() + }, + ); + if !confirm(cx, &prompt)? { + cx.err.line("aborted: issue worktree was not created")?; + return Ok(0); + } + + let options = CreateOptions { + branch: plan.branch.clone(), + base, + track: None, + copy_from: args.copy_from.clone(), + // The service never prompts; the interactive submodule policy is applied + // below, exactly as `wt new` does it. + init_submodules: false, + seed_submodules: session.config.submodules_seed.is_enabled(), + reflink: session.config.create_reflink.is_enabled(), + no_hooks: args.no_hooks, + }; + let created = create_in(&session.parts(&env), git.as_ref(), hooks, &options)?; + report_created(cx, &created)?; + if !created.reused { + super::maybe_init_submodules_interactive( + cx, + git.as_ref(), + &created.path, + session.config.submodules_init, + args.submodule_override(), + !cx.assume_yes, + session.config.submodules_seed.is_enabled(), + )?; + } + + link_issue( + git.as_ref(), + &session.primary_root, + &created.branch, + &issue, + &plan.brief, + )?; + + let ctx = HookContext { + worktree_path: created.path.clone(), + branch: created.branch.clone(), + repo_root: session.primary_root.clone(), + base_ref: created.base_ref.clone(), + pr_number: None, + }; + finish_worktree( + cx, + hooks, + &created.path, + &ctx, + Nav { + json: false, + no_switch: args.no_switch, + note: if created.reused { + "reusing issue worktree at" + } else { + "created issue worktree at" + }, + start: None, + }, + ) +} + +/// Proposes a branch and brief for `issue`, never failing for a model reason. +/// +/// Only a bad `--model`/`--effort` value returns `Err`; every generation failure +/// mode degrades instead: +/// +/// | outcome | branch | brief | +/// | --- | --- | --- | +/// | valid | as generated | as generated | +/// | malformed branch | [`naming::fallback`] | kept — still usable | +/// | unavailable / timeout / unparseable | [`naming::fallback`] | empty | +fn resolve_plan( + cx: &mut Cx, + session: &Session, + issue: &IssueView, + args: &IssueArgs, +) -> Result { + let fallback = naming::fallback(fallback_kind(issue), issue.number, &issue.title); + + // Nothing left to generate: both values were supplied. + if let (Some(branch), Some(brief)) = (&args.branch, &args.brief) { + cx.err + .line("generation: skipped (branch and brief supplied)")?; + return Ok(IssuePlan { + branch: branch.clone(), + brief: brief.clone(), + }); + } + + let opts = resolve_generation_options(args, &session.config)?; + let kind = session.config.agent_generation.provider; + let agent = cx.agent.clone(); + let root = session.primary_root.clone(); + let prompt = build_generation_prompt(issue); + let generated = progress::run(&mut cx.err, "Generating issue setup", move || { + generate(agent.as_ref(), kind, &prompt, &root, &opts) + }); + + let (branch, brief) = match generated { + Ok(plan) => match naming::parse_and_validate(&plan.branch, issue.number) { + Ok(name) => (name.to_string(), plan.brief), + // Only the branch was wrong; the brief is independent and still useful. + Err(e) => { + cx.err.line(&format!( + "note: generated branch {:?} rejected ({e}); using {fallback} instead", + plan.branch + ))?; + (fallback.to_string(), plan.brief) + } + }, + Err(e) => { + cx.err.line(&format!( + "note: could not generate issue setup ({e}); using {fallback} with no brief" + ))?; + (fallback.to_string(), String::new()) + } + }; + + Ok(IssuePlan { + branch: args.branch.clone().unwrap_or(branch), + brief: args.brief.clone().unwrap_or(brief), + }) +} + +/// The branch kind implied by the issue's labels, then its issue type, then +/// `feat` (issue #98). Deterministic, so the fallback branch for a given issue +/// is always the same name. +fn fallback_kind(issue: &IssueView) -> BranchKind { + issue + .labels + .iter() + .map(|label| label.name.as_str()) + .chain(issue.issue_type.iter().map(|ty| ty.name.as_str())) + .find_map(BranchKind::from_label) + .unwrap_or(BranchKind::Feat) +} + +/// Resolves the generation model and effort: a `--model`/`--effort` flag +/// overrides `[agent.generation]`. An unknown flag value is a usage error — the +/// one failure in this flow that is the user's to fix, so it is *not* swallowed +/// by the fallback. +fn resolve_generation_options(args: &IssueArgs, config: &Config) -> Result { + let model = match &args.model { + Some(m) => AgentModel::parse(m).ok_or_else(|| { + Error::usage(format!( + "unknown --model {m:?}; expected one of: opus, sonnet, haiku" + )) + })?, + None => config.agent_generation.model, + }; + let effort = match &args.effort { + Some(e) => Effort::parse(e).ok_or_else(|| { + Error::usage(format!( + "unknown --effort {e:?}; expected one of: low, medium, high" + )) + })?, + None => config.agent_generation.effort, + }; + Ok(AgentOptions { + model, + effort, + timeout: Some(GENERATION_TIMEOUT), + }) +} + +/// The generation prompt. +/// +/// The branch contract is [`naming::branch_rule`] verbatim, so the rule the +/// model is asked to follow is the same one [`naming::parse_and_validate`] +/// enforces. Only the issue fields `wt` actually fetched are included; comments, +/// assignees and project data are deliberately absent. +fn build_generation_prompt(issue: &IssueView) -> String { + let labels = issue + .labels + .iter() + .map(|label| label.name.as_str()) + .collect::>() + .join(", "); + let issue_type = issue.issue_type.as_ref().map_or("", |ty| ty.name.as_str()); + let milestone = issue.milestone.as_ref().map_or("", |m| m.title.as_str()); + format!( + "Generate setup values for a coding agent working on a GitHub issue.\n\ + Return only one JSON object with string fields `branch` and `brief`.\n\ + {}\n\ + Keep `brief` under 120 words and describe the implementation outcome and \ + validation, without inventing requirements.\n\n\ + Issue number: {}\nURL: {}\nTitle: {}\nType: {}\nLabels: {}\nMilestone: {}\nBody:\n{}", + naming::branch_rule(issue.number), + issue.number, + issue.url, + issue.title, + issue_type, + labels, + milestone, + issue.body, + ) +} + +/// Runs the generation agent and parses its answer. Every failure here is +/// recoverable by the caller's fallback. +fn generate( + agent: &dyn AgentClient, + kind: AgentKind, + prompt: &str, + dir: &std::path::Path, + opts: &AgentOptions, +) -> Result { + let run = agent.run(kind, prompt, dir, opts)?; + if run.is_error { + return Err(Error::operation(format!( + "generation agent reported an error: {}", + run.result.trim() + ))); + } + parse_issue_plan(&run.result) +} + +/// Parses the generated JSON object, tolerating a markdown code fence around it +/// (models add one often enough that rejecting it would waste a good answer). +fn parse_issue_plan(output: &str) -> Result { + let trimmed = output.trim(); + let body = trimmed + .strip_prefix("```json") + .or_else(|| trimmed.strip_prefix("```")) + .and_then(|rest| rest.trim_end().strip_suffix("```")) + .unwrap_or(trimmed) + .trim(); + + #[derive(serde::Deserialize)] + struct Raw { + branch: String, + #[serde(default)] + brief: String, + } + let raw: Raw = serde_json::from_str(body).map_err(|e| { + Error::operation(format!("generated output was not the expected JSON: {e}")) + })?; + Ok(IssuePlan { + branch: raw.branch.trim().to_string(), + brief: raw.brief.trim().to_string(), + }) +} + +/// The local branch already linked to `issue_number`, if any. More than one is +/// ambiguous and refused rather than guessed at. +fn linked_branch(repo: &gix::Repository, issue_number: u64) -> Result> { + let mut found: Vec = local_branches(repo) + .unwrap_or_default() + .into_iter() + .filter(|branch| wtconfig::read_meta(repo, branch).issue_number == Some(issue_number)) + .collect(); + match found.len() { + 0 => Ok(None), + 1 => Ok(found.pop()), + _ => { + found.sort(); + Err(Error::operation(format!( + "issue #{issue_number} is linked to more than one branch: {}", + found.join(", ") + ))) + } + } +} + +/// Records the issue link for `branch` as a locked read-check-write. +/// +/// Two traps this avoids. First, `create_in` wrote metadata through the `git` +/// subprocess but `gix` snapshots config at open, so the session's handle is +/// stale — the check has to run against a freshly discovered repository. +/// Second, this cannot go through `Workspace::write_meta`, which takes the lock +/// itself and so cannot enclose the preceding read; the advisory lock is neither +/// reentrant nor owner-aware, so taking it twice would deadlock until it times +/// out. +fn link_issue( + git: &dyn GitCli, + root: &std::path::Path, + branch: &str, + issue: &IssueView, + brief: &str, +) -> Result<()> { + let repo = Repo::discover(root)?; + wtconfig::ensure_schema_supported(repo.gix())?; + let _lock = lock_repo(root)?; + + let meta = wtconfig::read_meta(repo.gix(), branch); + if let Some(other) = meta.issue_number + && other != issue.number + { + return Err(Error::operation(format!( + "branch {branch:?} is already linked to issue #{other}" + ))); + } + + apply_meta( + git, + root, + branch, + &MetaUpdate { + issue_number: Some(issue.number), + issue_title: Some(issue.title.clone()), + issue_url: Some(issue.url.clone()), + // An empty brief leaves the key alone rather than recording a blank. + issue_brief: (!brief.trim().is_empty()).then(|| brief.trim().to_string()), + ..MetaUpdate::default() + }, + ) +} + +/// Prompts for one field on stderr, keeping `current` when the line is blank. +fn prompt_value(cx: &mut Cx, label: &str, current: &str) -> Result { + let shown = if current.trim().is_empty() { + "(none)".to_string() + } else { + current.trim().to_string() + }; + cx.err.text(&format!("{label} [{shown}]: "))?; + cx.err.flush()?; + let line = cx.input.read_line()?; + let typed = line.trim(); + Ok(if typed.is_empty() { + current.to_string() + } else { + typed.to_string() + }) +} + +/// Prompts for the base ref as a numbered menu of the repository's branches, +/// with the repository default and a free-form entry. +fn prompt_base(cx: &mut Cx, current: Option<&str>, branches: &[String]) -> Result> { + let shown = current.unwrap_or("(repository default)"); + cx.err.line(&format!("base [{shown}]:"))?; + for (index, branch) in branches.iter().enumerate() { + let marker = if Some(branch.as_str()) == current { + " (current)" + } else { + "" + }; + cx.err.line(&format!(" {}) {branch}{marker}", index + 1))?; + } + cx.err + .line(" or type a ref; blank keeps the current value")?; + cx.err.text("base: ")?; + cx.err.flush()?; + + let line = cx.input.read_line()?; + let typed = line.trim(); + if typed.is_empty() { + return Ok(current.map(str::to_string)); + } + // A bare number selects from the menu; anything else is a ref, so a branch + // literally named "2" is still reachable by typing it at a non-menu prompt. + if let Ok(choice) = typed.parse::() { + return match branches.get(choice.wrapping_sub(1)) { + Some(branch) => Ok(Some(branch.clone())), + None => Err(Error::usage(format!( + "{choice} is not one of the {} listed branches", + branches.len() + ))), + }; + } + Ok(Some(typed.to_string())) +} + +/// The worktree path for `branch`, for tests that assert what was created. +#[cfg(test)] +fn target_of(session: &Session, env: &crate::cx::Env, branch: &str) -> std::path::PathBuf { + preview_target(&session.parts(env), branch, None).unwrap_or_default() +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::cx::Stream; + use crate::gh::{IssueLabel, IssueType}; + use crate::git::cli::RealGit; + use crate::hooks::RealHookRunner; + use crate::testutil::{CannedInput, FakeAgent, FakeGh, SharedBuf, TestRepo, test_cx}; + use std::sync::Arc; + + fn issue() -> IssueView { + IssueView { + number: 7, + title: "Add login page".into(), + body: "Users need to sign in.".into(), + state: "OPEN".into(), + labels: vec![IssueLabel { + name: "enhancement".into(), + }], + issue_type: None, + milestone: None, + created_at: String::new(), + updated_at: String::new(), + url: "https://github.com/o/r/issues/7".into(), + } + } + + fn args() -> IssueArgs { + IssueArgs { + target: "7".into(), + branch: None, + from: None, + brief: None, + model: None, + effort: None, + no_switch: true, + no_hooks: true, + copy_from: None, + init_submodules: false, + no_init_submodules: true, + } + } + + /// A repo plus a `--yes` context wired to the given agent output. + fn run_with_agent( + agent: FakeAgent, + args: &IssueArgs, + ) -> (TestRepo, crate::testutil::TestCx, u8) { + let repo = TestRepo::init(); + let mut t = test_cx(&[], repo.root().to_str().unwrap_or_default()); + t.cx.assume_yes = true; + t.cx.gh = Arc::new(FakeGh::with_issue(issue())); + t.cx.agent = Arc::new(agent); + let code = run(&mut t.cx, &RealHookRunner, args).unwrap(); + (repo, t, code) + } + + /// The `wt..*` metadata recorded for `branch`. + fn meta(repo: &TestRepo, branch: &str) -> crate::config::wtconfig::WtMeta { + let r = Repo::discover(repo.root()).unwrap(); + wtconfig::read_meta(r.gix(), branch) + } + + fn branches(repo: &TestRepo) -> Vec { + let r = Repo::discover(repo.root()).unwrap(); + local_branches(r.gix()).unwrap_or_default() + } + + #[test] + fn generation_creates_the_branch_and_links_the_issue() { + let (repo, t, code) = run_with_agent( + FakeAgent::drafting(r#"{"branch":"feat/7-add-login","brief":"Wire up the form."}"#), + &args(), + ); + assert_eq!(code, 0); + assert!(branches(&repo).iter().any(|b| b == "feat/7-add-login")); + let recorded = meta(&repo, "feat/7-add-login"); + assert_eq!(recorded.issue_number, Some(7)); + assert_eq!(recorded.issue_title.as_deref(), Some("Add login page")); + assert_eq!( + recorded.issue_url.as_deref(), + Some("https://github.com/o/r/issues/7") + ); + assert_eq!(recorded.issue_brief.as_deref(), Some("Wire up the form.")); + assert!(t.err.contents().contains("created issue worktree at")); + } + + #[test] + fn a_malformed_branch_falls_back_but_keeps_the_brief() { + // Only the branch violated the contract; the brief is independent. + let (repo, t, _) = run_with_agent( + FakeAgent::drafting(r#"{"branch":"NOT A BRANCH","brief":"Wire up the form."}"#), + &args(), + ); + assert!(branches(&repo).iter().any(|b| b == "feat/7-add-login-page")); + assert_eq!( + meta(&repo, "feat/7-add-login-page").issue_brief.as_deref(), + Some("Wire up the form.") + ); + assert!( + t.err.contents().contains("rejected"), + "{}", + t.err.contents() + ); + } + + #[test] + fn garbage_output_still_produces_a_valid_branch_and_worktree() { + // Issue #98's headline requirement: a flaky generation call must not be + // able to block worktree creation. + let (repo, t, code) = + run_with_agent(FakeAgent::drafting("I'm afraid I can't do that"), &args()); + assert_eq!(code, 0); + assert!(branches(&repo).iter().any(|b| b == "feat/7-add-login-page")); + assert_eq!(meta(&repo, "feat/7-add-login-page").issue_number, Some(7)); + // A failed generation leaves no brief rather than a bogus one. + assert_eq!(meta(&repo, "feat/7-add-login-page").issue_brief, None); + assert!(t.err.contents().contains("could not generate")); + } + + #[test] + fn an_unavailable_agent_falls_back() { + let (repo, _, code) = run_with_agent(FakeAgent::unavailable(), &args()); + assert_eq!(code, 0); + assert!(branches(&repo).iter().any(|b| b == "feat/7-add-login-page")); + } + + #[test] + fn a_timed_out_agent_falls_back() { + let (repo, t, code) = run_with_agent(FakeAgent::timing_out(), &args()); + assert_eq!(code, 0); + assert!(branches(&repo).iter().any(|b| b == "feat/7-add-login-page")); + assert!(t.err.contents().contains("did not respond")); + } + + #[test] + fn an_erroring_agent_falls_back() { + let (repo, _, code) = run_with_agent(FakeAgent::erroring("rate limited"), &args()); + assert_eq!(code, 0); + assert!(branches(&repo).iter().any(|b| b == "feat/7-add-login-page")); + } + + #[test] + fn explicit_branch_and_brief_skip_generation_entirely() { + let mut a = args(); + a.branch = Some("fix/7-mine".into()); + a.brief = Some("Do the thing.".into()); + // An unavailable agent proves no generation call was attempted. + let (repo, t, code) = run_with_agent(FakeAgent::unavailable(), &a); + assert_eq!(code, 0); + assert!(branches(&repo).iter().any(|b| b == "fix/7-mine")); + assert_eq!( + meta(&repo, "fix/7-mine").issue_brief.as_deref(), + Some("Do the thing.") + ); + assert!(t.err.contents().contains("generation: skipped")); + } + + #[test] + fn fallback_kind_prefers_labels_then_type_then_feat() { + let mut i = issue(); + i.labels = vec![IssueLabel { name: "bug".into() }]; + assert_eq!(fallback_kind(&i), BranchKind::Fix); + + // Case-insensitive, and unknown labels fall through to the next source. + i.labels = vec![IssueLabel { + name: "Documentation".into(), + }]; + assert_eq!(fallback_kind(&i), BranchKind::Docs); + + i.labels = vec![IssueLabel { + name: "help wanted".into(), + }]; + i.issue_type = Some(IssueType { + name: "Feature".into(), + }); + assert_eq!(fallback_kind(&i), BranchKind::Feat); + + i.labels = Vec::new(); + i.issue_type = None; + assert_eq!(fallback_kind(&i), BranchKind::Feat); + } + + #[test] + fn an_already_linked_branch_is_reused_without_generation() { + let repo = TestRepo::init(); + repo.git(&["branch", "feat/7-existing"]); + wtconfig::write_issue_number(&RealGit, repo.root(), "feat/7-existing", 7).unwrap(); + + let mut t = test_cx(&[], repo.root().to_str().unwrap_or_default()); + t.cx.assume_yes = true; + t.cx.gh = Arc::new(FakeGh::with_issue(issue())); + // Unavailable: reaching generation at all would fail this expectation. + t.cx.agent = Arc::new(FakeAgent::unavailable()); + let code = run(&mut t.cx, &RealHookRunner, &args()).unwrap(); + + assert_eq!(code, 0); + assert!(t.err.contents().contains("already linked to branch")); + assert!(!t.err.contents().contains("could not generate")); + } + + #[test] + fn a_branch_flag_conflicting_with_the_link_is_a_usage_error() { + let repo = TestRepo::init(); + repo.git(&["branch", "feat/7-existing"]); + wtconfig::write_issue_number(&RealGit, repo.root(), "feat/7-existing", 7).unwrap(); + + let mut t = test_cx(&[], repo.root().to_str().unwrap_or_default()); + t.cx.assume_yes = true; + t.cx.gh = Arc::new(FakeGh::with_issue(issue())); + let mut a = args(); + a.branch = Some("feat/7-other".into()); + let err = run(&mut t.cx, &RealHookRunner, &a).unwrap_err(); + assert_eq!(err.exit_code(), 2); + assert!(err.to_string().contains("already linked")); + } + + #[test] + fn a_branch_linked_to_another_issue_is_refused() { + let repo = TestRepo::init(); + let mut t = test_cx(&[], repo.root().to_str().unwrap_or_default()); + t.cx.assume_yes = true; + t.cx.gh = Arc::new(FakeGh::with_issue(issue())); + t.cx.agent = Arc::new(FakeAgent::unavailable()); + + let mut a = args(); + a.branch = Some("feat/shared".into()); + a.brief = Some("x".into()); + repo.git(&["branch", "feat/shared"]); + wtconfig::write_issue_number(&RealGit, repo.root(), "feat/shared", 9).unwrap(); + + let err = run(&mut t.cx, &RealHookRunner, &a).unwrap_err(); + assert!( + err.to_string().contains("already linked to issue #9"), + "{err}" + ); + } + + #[test] + fn without_a_terminal_or_yes_it_fails_before_the_fetch() { + let repo = TestRepo::init(); + let mut t = test_cx(&[], repo.root().to_str().unwrap_or_default()); + // An unavailable `gh` would make a fetch fail loudly; a `Usage` error + // instead proves the gate ran first. + t.cx.gh = Arc::new(FakeGh::unavailable()); + let err = run(&mut t.cx, &RealHookRunner, &args()).unwrap_err(); + assert_eq!(err.exit_code(), 2); + assert!(err.to_string().contains("interactive terminal")); + } + + /// A `--yes`-free context whose stderr is a TTY, so the review prompts run. + fn interactive(repo: &TestRepo, answers: &[&str]) -> crate::testutil::TestCx { + let mut t = test_cx(&[], repo.root().to_str().unwrap_or_default()); + let err = SharedBuf::new(); + t.cx.err = Stream::new(Box::new(err.clone()), true); + t.err = err; + t.cx.input = Box::new(CannedInput::new(answers)); + t.cx.gh = Arc::new(FakeGh::with_issue(issue())); + t + } + + #[test] + fn declining_the_confirmation_creates_nothing() { + let repo = TestRepo::init(); + // branch, base, brief, then decline. + let mut t = interactive(&repo, &["", "", "", "n"]); + t.cx.agent = Arc::new(FakeAgent::drafting( + r#"{"branch":"feat/7-add-login","brief":"b"}"#, + )); + let code = run(&mut t.cx, &RealHookRunner, &args()).unwrap(); + + assert_eq!(code, 0); + assert!(t.err.contents().contains("aborted")); + assert!(!branches(&repo).iter().any(|b| b == "feat/7-add-login")); + assert_eq!(meta(&repo, "feat/7-add-login").issue_number, None); + } + + #[test] + fn interactive_edits_override_the_generated_values() { + let repo = TestRepo::init(); + let mut t = interactive(&repo, &["fix/7-mine", "", "my brief", "y"]); + t.cx.agent = Arc::new(FakeAgent::drafting( + r#"{"branch":"feat/7-add-login","brief":"generated"}"#, + )); + let code = run(&mut t.cx, &RealHookRunner, &args()).unwrap(); + + assert_eq!(code, 0); + assert!(branches(&repo).iter().any(|b| b == "fix/7-mine")); + assert_eq!( + meta(&repo, "fix/7-mine").issue_brief.as_deref(), + Some("my brief") + ); + } + + #[test] + fn an_edited_off_contract_branch_warns_but_proceeds() { + let repo = TestRepo::init(); + let mut t = interactive(&repo, &["hotfix-thing", "", "", "y"]); + t.cx.agent = Arc::new(FakeAgent::drafting( + r#"{"branch":"feat/7-add-login","brief":"b"}"#, + )); + let code = run(&mut t.cx, &RealHookRunner, &args()).unwrap(); + + assert_eq!(code, 0); + assert!(branches(&repo).iter().any(|b| b == "hotfix-thing")); + assert!(t.err.contents().contains("does not follow TYPE/7-SLUG")); + // The link still resolves, which is why the warning is not an error. + assert_eq!(meta(&repo, "hotfix-thing").issue_number, Some(7)); + } + + #[test] + fn an_edited_illegal_branch_name_is_still_refused() { + let repo = TestRepo::init(); + let mut t = interactive(&repo, &["not a branch", "", "", "y"]); + t.cx.agent = Arc::new(FakeAgent::drafting( + r#"{"branch":"feat/7-add-login","brief":"b"}"#, + )); + let err = run(&mut t.cx, &RealHookRunner, &args()).unwrap_err(); + assert_eq!(err.exit_code(), 2); + } + + #[test] + fn the_prompt_carries_the_naming_contract_verbatim() { + let prompt = build_generation_prompt(&issue()); + assert!(prompt.contains(&naming::branch_rule(7))); + assert!(prompt.contains("Add login page")); + assert!(prompt.contains("Users need to sign in.")); + assert!(prompt.contains("enhancement")); + // Only the fetched fields; nothing invented. + assert!(!prompt.contains("comments")); + } + + #[test] + fn plan_parsing_tolerates_code_fences() { + let bare = parse_issue_plan(r#"{"branch":"feat/7-x","brief":"b"}"#).unwrap(); + assert_eq!(bare.branch, "feat/7-x"); + let fenced = + parse_issue_plan("```json\n{\"branch\":\"feat/7-x\",\"brief\":\"b\"}\n```").unwrap(); + assert_eq!(fenced, bare); + let plain = + parse_issue_plan("```\n{\"branch\":\"feat/7-x\",\"brief\":\"b\"}\n```").unwrap(); + assert_eq!(plain, bare); + // A missing brief is tolerated; a missing branch is not. + assert_eq!( + parse_issue_plan(r#"{"branch":"feat/7-x"}"#).unwrap().brief, + "" + ); + assert!(parse_issue_plan(r#"{"brief":"b"}"#).is_err()); + assert!(parse_issue_plan("not json at all").is_err()); + } + + #[test] + fn generation_flags_override_the_config_profile() { + let config = Config::default(); + let mut a = args(); + let opts = resolve_generation_options(&a, &config).unwrap(); + assert_eq!(opts.model, config.agent_generation.model); + assert_eq!(opts.effort, config.agent_generation.effort); + // Generation is always bounded, so a hung agent cannot block a worktree. + assert_eq!(opts.timeout, Some(GENERATION_TIMEOUT)); + + a.model = Some("opus".into()); + a.effort = Some("high".into()); + let opts = resolve_generation_options(&a, &config).unwrap(); + assert_eq!(opts.model, AgentModel::Opus); + assert_eq!(opts.effort, Effort::High); + + a.model = Some("gpt".into()); + let err = resolve_generation_options(&a, &config).unwrap_err(); + assert_eq!(err.exit_code(), 2); + } + + #[test] + fn a_bad_model_flag_fails_rather_than_falling_back() { + // The one failure in this flow that is the user's to fix. + let repo = TestRepo::init(); + let mut t = test_cx(&[], repo.root().to_str().unwrap_or_default()); + t.cx.assume_yes = true; + t.cx.gh = Arc::new(FakeGh::with_issue(issue())); + let mut a = args(); + a.effort = Some("nonsense".into()); + let err = run(&mut t.cx, &RealHookRunner, &a).unwrap_err(); + assert_eq!(err.exit_code(), 2); + } + + #[test] + fn base_choice_accepts_a_number_a_ref_and_a_blank() { + let repo = TestRepo::init(); + let all = vec!["main".to_string(), "topic".to_string()]; + + let mut t = interactive(&repo, &["2"]); + assert_eq!( + prompt_base(&mut t.cx, Some("main"), &all).unwrap(), + Some("topic".to_string()) + ); + + let mut t = interactive(&repo, &["origin/release"]); + assert_eq!( + prompt_base(&mut t.cx, Some("main"), &all).unwrap(), + Some("origin/release".to_string()) + ); + + let mut t = interactive(&repo, &[""]); + assert_eq!( + prompt_base(&mut t.cx, Some("main"), &all).unwrap(), + Some("main".to_string()) + ); + + let mut t = interactive(&repo, &["9"]); + let err = prompt_base(&mut t.cx, Some("main"), &all).unwrap_err(); + assert_eq!(err.exit_code(), 2); + } + + #[test] + fn a_field_prompt_keeps_the_current_value_on_a_blank_line() { + let repo = TestRepo::init(); + let mut t = interactive(&repo, &["", "typed"]); + assert_eq!(prompt_value(&mut t.cx, "branch", "kept").unwrap(), "kept"); + assert_eq!(prompt_value(&mut t.cx, "branch", "kept").unwrap(), "typed"); + assert!(t.err.contents().contains("branch [kept]: ")); + } + + #[test] + fn the_worktree_lands_where_the_confirmation_previewed() { + let (repo, t, _) = run_with_agent( + FakeAgent::drafting(r#"{"branch":"feat/7-add-login","brief":"b"}"#), + &args(), + ); + let mut probe = test_cx(&[], repo.root().to_str().unwrap_or_default()); + probe.cx.assume_yes = true; + let session = open_session(&probe.cx, &RealGit).unwrap(); + let expected = target_of(&session, &probe.cx.env.clone(), "feat/7-add-login"); + assert!( + expected.is_dir(), + "{} is not a directory", + expected.display() + ); + assert!(t.err.contents().contains(&expected.display().to_string())); + } +} diff --git a/src/commands/mod.rs b/src/commands/mod.rs index ae78889..d948181 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -7,6 +7,7 @@ pub mod completions; pub mod config_cmd; pub mod drop; pub mod init; +pub mod issue; pub mod list; pub mod new; pub mod path; @@ -32,7 +33,7 @@ use crate::git::discover::Repo; use crate::hooks::{HookContext, HookRunner}; use crate::model::Worktree; use crate::query::{self, Resolved}; -use crate::worktree::{Workspace, WorkspaceParts, build_worktrees}; +use crate::worktree::{CreatedWorktree, HookOutcome, Workspace, WorkspaceParts, build_worktrees}; // Path/template helpers shared with the service layer, re-exported so command // modules keep their historical import paths. @@ -396,6 +397,29 @@ pub(crate) fn finish_worktree( crate::hooks::run_start(runner, command, hook_ctx) } +/// Reports the non-fatal outcomes of a completed creation: the copy step and +/// the `post_create` hook. +/// +/// The hook already ran inside the service, where a failure is a warning rather +/// than a rollback (spec §8). Shared by `wt new` and `wt issue` so both report +/// the same way. +pub(crate) fn report_created(cx: &mut Cx, created: &CreatedWorktree) -> Result<()> { + log_copy_outcome(cx, &created.copy); + match &created.post_create { + HookOutcome::ExitedNonZero(code) => { + cx.err.line(&format!( + "warning: post_create hook exited with status {code}" + ))?; + } + HookOutcome::Failed(error) => { + cx.err + .line(&format!("warning: post_create hook failed: {error}"))?; + } + HookOutcome::Skipped | HookOutcome::Succeeded => {} + } + Ok(()) +} + /// Writes `target` to the file named by `$WT_CD_FILE` so the shell wrapper can /// `cd` there once `wt` exits (issue #89). /// diff --git a/src/commands/new.rs b/src/commands/new.rs index 579b4bd..aebd90d 100644 --- a/src/commands/new.rs +++ b/src/commands/new.rs @@ -7,7 +7,7 @@ use crate::error::Result; use crate::git::discover::Repo; use crate::git::{branch_ref, resolve_hex}; use crate::hooks::{HookContext, HookRunner}; -use crate::worktree::{CreateOptions, HookOutcome, create_in, resolve_base}; +use crate::worktree::{CreateOptions, create_in, resolve_base}; /// Creates a linked worktree for `branch`, prompting first when the base it would /// fork from is behind its origin counterpart (issue #56): the user can update the @@ -95,21 +95,7 @@ pub(crate) fn run_core( let env = cx.env.clone(); let created = create_in(&session.parts(&env), git, hooks, &options)?; - crate::commands::log_copy_outcome(cx, &created.copy); - // The post-create hook already ran in the service; a failure is a warning, - // not a rollback (§8). - match &created.post_create { - HookOutcome::ExitedNonZero(code) => { - cx.err.line(&format!( - "warning: post_create hook exited with status {code}" - ))?; - } - HookOutcome::Failed(error) => { - cx.err - .line(&format!("warning: post_create hook failed: {error}"))?; - } - HookOutcome::Skipped | HookOutcome::Succeeded => {} - } + crate::commands::report_created(cx, &created)?; // Initialize submodules per the policy/flag, prompting (default yes) at an // interactive terminal when the policy is left at its default (issue #50). diff --git a/src/commands/pr.rs b/src/commands/pr.rs index 410d9f5..2f94453 100644 --- a/src/commands/pr.rs +++ b/src/commands/pr.rs @@ -326,6 +326,9 @@ fn pr_meta(view: &crate::gh::PrView, base: &str, created_here: bool) -> MetaUpda pr_title: Some(view.title.clone()), pr_url: Some(view.url.clone()), created_by_wt: created_here, + // A PR checkout records no issue link; `None` leaves any existing one + // (e.g. from `wt issue`) untouched. + ..MetaUpdate::default() } } diff --git a/src/commands/pr_open.rs b/src/commands/pr_open.rs index 85de561..0f2f844 100644 --- a/src/commands/pr_open.rs +++ b/src/commands/pr_open.rs @@ -317,7 +317,7 @@ pub(crate) fn resolve_agent_options(args: &PrOpenArgs, config: &Config) -> Resul "unknown --model {m:?}; expected one of: opus, sonnet, haiku" )) })?, - None => config.agent_model, + None => config.agent_generation.model, }; let effort = match &args.effort { Some(e) => Effort::parse(e).ok_or_else(|| { @@ -325,9 +325,15 @@ pub(crate) fn resolve_agent_options(args: &PrOpenArgs, config: &Config) -> Resul "unknown --effort {e:?}; expected one of: low, medium, high" )) })?, - None => config.agent_effort, + None => config.agent_generation.effort, }; - Ok(AgentOptions { model, effort }) + Ok(AgentOptions { + model, + effort, + // The PR auto-fill has always waited indefinitely; `wt issue` is what + // introduces a deadline, and only for its own generation step. + timeout: None, + }) } /// Runs the code agent (`claude`) to draft a PR `(title, body)` from `ctx` with @@ -440,6 +446,9 @@ pub(crate) fn record_pr_metadata( // rather than recording a blank. pr_url: (!outcome.url.is_empty()).then(|| outcome.url.clone()), created_by_wt: false, + // Opening a PR records no issue link; `None` leaves any existing one + // (e.g. from `wt issue`) untouched. + ..MetaUpdate::default() }; let _lock = lock_repo(root)?; apply_meta(git, root, branch, &update) @@ -448,6 +457,7 @@ pub(crate) fn record_pr_metadata( #[cfg(test)] mod tests { use super::*; + use crate::config::GenerationAgentConfig; use crate::git::cli::RealGit; use crate::git::discover::Repo; use crate::testutil::{FakeAgent, FakeGh, TestRepo}; @@ -717,6 +727,7 @@ mod tests { let opts = AgentOptions { model: AgentModel::Opus, effort: Effort::High, + timeout: None, }; draft_with_ai(&agent, &ctx_for("feat", "main", false), dir.path(), &opts).unwrap(); assert_eq!(agent.last_opts(), Some(opts)); @@ -753,8 +764,11 @@ mod tests { #[test] fn resolve_agent_options_flags_override_config() { let config = Config { - agent_model: AgentModel::Haiku, - agent_effort: Effort::Low, + agent_generation: GenerationAgentConfig { + model: AgentModel::Haiku, + effort: Effort::Low, + ..GenerationAgentConfig::default() + }, ..Config::default() }; // No flags: config defaults win. diff --git a/src/commands/shell_init.rs b/src/commands/shell_init.rs index 0384a00..859c5ee 100644 --- a/src/commands/shell_init.rs +++ b/src/commands/shell_init.rs @@ -45,7 +45,7 @@ pub(crate) fn run(cx: &mut Cx, args: &ShellInitArgs) -> Result { const BASH: &str = r#"# wt shell integration (bash) — source this from your ~/.bashrc wt() { case "${1-}" in - switch|sw|checkout|co|new|pr|drop|ui|tui|"") + switch|sw|checkout|co|new|issue|pr|drop|ui|tui|"") local __wt_arg for __wt_arg in "$@"; do case "$__wt_arg" in @@ -99,9 +99,16 @@ _wt_complete() { if [ "$COMP_CWORD" -eq 2 ]; then COMPREPLY=($(command wt __complete pr-numbers "$cur" 2>/dev/null)); return fi ;; + issue) + if [ "$prev" = "--from" ]; then + COMPREPLY=($(command wt __complete branches "$cur" 2>/dev/null)); return + fi + if [ "$COMP_CWORD" -eq 2 ]; then + COMPREPLY=($(command wt __complete issue-numbers "$cur" 2>/dev/null)); return + fi ;; esac if [ "$COMP_CWORD" -eq 1 ]; then - COMPREPLY=($(compgen -W "new checkout co list ls switch sw remove rm drop prune pr status path root init config completions shell-init ui tui" -- "$cur")) + COMPREPLY=($(compgen -W "new checkout co issue list ls switch sw remove rm drop prune pr status path root init config completions shell-init ui tui" -- "$cur")) fi } complete -F _wt_complete wt @@ -110,7 +117,7 @@ complete -F _wt_complete wt const ZSH: &str = r#"# wt shell integration (zsh) — source this from your ~/.zshrc wt() { case "${1-}" in - switch|sw|checkout|co|new|pr|drop|ui|tui|"") + switch|sw|checkout|co|new|issue|pr|drop|ui|tui|"") local __wt_arg for __wt_arg in "$@"; do case "$__wt_arg" in @@ -153,8 +160,10 @@ _wt() { compadd -- ${(f)"$(command wt __complete all-branches "${words[CURRENT]}" 2>/dev/null)"}; return ;; pr) compadd -- ${(f)"$(command wt __complete pr-numbers "${words[CURRENT]}" 2>/dev/null)"}; return ;; + issue) + compadd -- ${(f)"$(command wt __complete issue-numbers "${words[CURRENT]}" 2>/dev/null)"}; return ;; esac - compadd -- new checkout co list ls switch sw remove rm drop prune pr status path root init config completions shell-init ui tui + compadd -- new checkout co issue list ls switch sw remove rm drop prune pr status path root init config completions shell-init ui tui } compdef _wt wt "#; @@ -162,7 +171,7 @@ compdef _wt wt const FISH: &str = r#"# wt shell integration (fish) — source this from your config.fish function wt set -l cmd $argv[1] - if test (count $argv) -eq 0; or contains -- "$cmd" switch sw checkout co new pr drop ui tui + if test (count $argv) -eq 0; or contains -- "$cmd" switch sw checkout co new issue pr drop ui tui # --start runs a command in the new worktree, so it needs the real # terminal: run wt with stdio inherited and take the path via a file. if contains -- --start $argv; or string match -q -- '--start=*' $argv @@ -206,13 +215,15 @@ complete -c wt -n '__fish_seen_subcommand_from checkout co' \ -a '(command wt __complete all-branches 2>/dev/null)' complete -c wt -n '__fish_seen_subcommand_from pr' \ -a '(command wt __complete pr-numbers 2>/dev/null)' +complete -c wt -n '__fish_seen_subcommand_from issue' \ + -a '(command wt __complete issue-numbers 2>/dev/null)' complete -c wt -n '__fish_use_subcommand' \ - -a 'new checkout co list switch remove drop prune pr status path root init config completions shell-init ui tui' + -a 'new checkout co issue list switch remove drop prune pr status path root init config completions shell-init ui tui' "#; const POWERSHELL: &str = r#"# wt shell integration (PowerShell) — add to your $PROFILE function wt { - $nav = @('switch','sw','checkout','co','new','pr','drop','ui','tui') + $nav = @('switch','sw','checkout','co','new','issue','pr','drop','ui','tui') $exe = (Get-Command wt -CommandType Application | Select-Object -First 1).Source if ($args.Count -eq 0 -or $nav -contains $args[0]) { # --start runs a command in the new worktree, so it needs the real @@ -251,14 +262,15 @@ Register-ArgumentCompleter -CommandName wt -Native -ScriptBlock { '^new$' { & $exe __complete branches $wordToComplete } '^(checkout|co)$' { & $exe __complete all-branches $wordToComplete } '^pr$' { & $exe __complete pr-numbers $wordToComplete } - default { 'new','checkout','co','list','switch','remove','drop','prune','pr','status','path','root','init','config','completions','shell-init','ui','tui' | Where-Object { $_ -like "$wordToComplete*" } } + '^issue$' { & $exe __complete issue-numbers $wordToComplete } + default { 'new','checkout','co','issue','list','switch','remove','drop','prune','pr','status','path','root','init','config','completions','shell-init','ui','tui' | Where-Object { $_ -like "$wordToComplete*" } } } } "#; const ELVISH: &str = r#"# wt shell integration (elvish) — source this from your rc.elv fn wt {|@a| - var nav = [switch sw checkout co new pr drop ui tui] + var nav = [switch sw checkout co new issue pr drop ui tui] if (or (== (count $a) 0) (and (> (count $a) 0) (has-value $nav $a[0]))) { # --start runs a command in the new worktree, so it needs the real # terminal: run wt with stdio inherited and take the path via a file. @@ -480,6 +492,70 @@ mod tests { } } + #[test] + fn every_wrapper_routes_issue_like_new() { + // `wt issue` prints the worktree path on stdout like `wt new`, so it is + // cd-eligible in every wrapper and offered as a subcommand. + for shell in [ + Shell::Bash, + Shell::Zsh, + Shell::Fish, + Shell::PowerShell, + Shell::Elvish, + ] { + let s = snippet(shell); + assert!( + s.contains("issue"), + "{shell:?} wrapper does not wire up issue" + ); + } + } + + #[test] + fn no_wrapper_special_cases_issue_for_inherited_stdio() { + // Regression guard. `wt issue` is interactive, which invites the + // assumption that it needs the `--start`-style `WT_CD_FILE` handoff. It + // does not: the wrappers capture *stdout* only, so stdin and stderr + // already reach the review prompts — exactly as `wt new`'s own prompts + // rely on. Forcing the handoff would also make stdout a TTY, so the path + // would be printed *and* cd'd into: double output. + // + // `WT_CD_FILE` must therefore appear only under the `--start` branch. + for shell in [ + Shell::Bash, + Shell::Zsh, + Shell::Fish, + Shell::PowerShell, + Shell::Elvish, + ] { + let s = snippet(shell); + for line in s.lines() { + if line.contains("WT_CD_FILE") { + continue; + } + assert!( + !(line.contains("issue") && line.contains("mktemp")), + "{shell:?} appears to special-case issue for stdio: {line}" + ); + } + let handoffs = s.matches("WT_CD_FILE").count(); + assert!(handoffs > 0, "{shell:?} lost the --start handoff entirely"); + } + } + + #[test] + fn issue_completes_open_issue_numbers() { + // The shells with a subcommand-dispatch completion offer issue numbers + // the same way they offer PR numbers. + for shell in [Shell::Bash, Shell::Zsh, Shell::Fish, Shell::PowerShell] { + let s = snippet(shell); + assert!( + s.contains("__complete issue-numbers"), + "{shell:?} does not complete issue numbers" + ); + } + } + #[test] fn checkout_completes_local_and_remote_branches() { // `wt checkout` switches to a local *or* remote-only branch (DWIM), so its diff --git a/src/config/mod.rs b/src/config/mod.rs index 09166ce..fbbc63f 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -17,7 +17,9 @@ use crate::cx::Env; use crate::error::{Error, Result}; pub use parse::parse_layer; -pub use schema::{Config, ConfigLayer, CreateReflink, SubmoduleInit, SubmoduleSeed}; +pub use schema::{ + Config, ConfigLayer, CreateReflink, GenerationAgentConfig, SubmoduleInit, SubmoduleSeed, +}; pub use wtconfig::WtMeta; /// The path to the global `config.toml`, honoring `$XDG_CONFIG_HOME` and falling diff --git a/src/config/parse.rs b/src/config/parse.rs index 2d591b3..75c016a 100644 --- a/src/config/parse.rs +++ b/src/config/parse.rs @@ -7,7 +7,7 @@ use ratatui::style::Color; use toml::Value; -use crate::agent::{AgentModel, Effort}; +use crate::agent::{AgentKind, AgentModel, Effort}; use crate::config::schema::{ConfigLayer, CreateReflink, SubmoduleInit, SubmoduleSeed}; use crate::error::{Error, Result}; #[cfg(feature = "tui")] @@ -168,22 +168,102 @@ fn parse_submodules(file: &str, value: &Value, layer: &mut ConfigLayer) -> Resul Ok(()) } -/// Parses the `[agent]` table, validating the model and effort identifiers. +/// Parses the `[agent]` table. +/// +/// `[agent.generation]` is the canonical home for the generation profile. The +/// flat `agent.model` / `agent.effort` keys are kept as deprecated aliases for +/// the same two settings, so existing configurations keep working; setting a +/// key both ways in one file is rejected rather than silently resolved by +/// table order. +/// +/// `[agent.work]` is refused outright: `wt` does not run coding agents, so +/// there is no work profile to configure here. fn parse_agent(file: &str, value: &Value, layer: &mut ConfigLayer) -> Result<()> { + let mut flat_model = false; + let mut flat_effort = false; + let mut generation_model = false; + let mut generation_effort = false; + for (sub, val) in as_table(file, "agent", value)? { let key = format!("agent.{sub}"); match sub.as_str() { "model" => { - let text = as_string(file, &key, val)?; - let model = AgentModel::parse(&text) - .ok_or_else(|| cfg_err(file, &key, "expected one of: opus, sonnet, haiku"))?; - layer.agent_model = Some(model); + layer.agent_generation_model = Some(parse_agent_model(file, &key, val)?); + flat_model = true; + tracing::warn!("{file}: `agent.model` is deprecated; use `agent.generation.model`"); } "effort" => { + layer.agent_generation_effort = Some(parse_agent_effort(file, &key, val)?); + flat_effort = true; + tracing::warn!( + "{file}: `agent.effort` is deprecated; use `agent.generation.effort`" + ); + } + "generation" => { + parse_agent_generation( + file, + val, + layer, + &mut generation_model, + &mut generation_effort, + )?; + } + "work" => { + return Err(cfg_err( + file, + &key, + "`wt` does not run coding agents, so it has no work profile; \ + agent execution belongs to karet (https://github.com/getkono/karet). \ + `wt` owns only `[agent.generation]`", + )); + } + _ => return Err(cfg_err(file, &key, "unknown configuration key")), + } + } + + // Order within a table is not something a user should have to reason about, + // so a key set both ways is an error rather than a last-one-wins race. + if flat_model && generation_model { + return Err(cfg_err( + file, + "agent.model", + "conflicts with `agent.generation.model` in the same file; set only one", + )); + } + if flat_effort && generation_effort { + return Err(cfg_err( + file, + "agent.effort", + "conflicts with `agent.generation.effort` in the same file; set only one", + )); + } + Ok(()) +} + +/// Parses the `[agent.generation]` sub-table. +fn parse_agent_generation( + file: &str, + value: &Value, + layer: &mut ConfigLayer, + saw_model: &mut bool, + saw_effort: &mut bool, +) -> Result<()> { + for (sub, val) in as_table(file, "agent.generation", value)? { + let key = format!("agent.generation.{sub}"); + match sub.as_str() { + "provider" => { let text = as_string(file, &key, val)?; - let effort = Effort::parse(&text) - .ok_or_else(|| cfg_err(file, &key, "expected one of: low, medium, high"))?; - layer.agent_effort = Some(effort); + let provider = AgentKind::parse(&text) + .ok_or_else(|| cfg_err(file, &key, "expected one of: claude"))?; + layer.agent_generation_provider = Some(provider); + } + "model" => { + layer.agent_generation_model = Some(parse_agent_model(file, &key, val)?); + *saw_model = true; + } + "effort" => { + layer.agent_generation_effort = Some(parse_agent_effort(file, &key, val)?); + *saw_effort = true; } _ => return Err(cfg_err(file, &key, "unknown configuration key")), } @@ -191,6 +271,20 @@ fn parse_agent(file: &str, value: &Value, layer: &mut ConfigLayer) -> Result<()> Ok(()) } +/// Validates a model identifier, shared by the canonical key and its alias so +/// the two cannot drift. +fn parse_agent_model(file: &str, key: &str, value: &Value) -> Result { + let text = as_string(file, key, value)?; + AgentModel::parse(&text) + .ok_or_else(|| cfg_err(file, key, "expected one of: opus, sonnet, haiku")) +} + +/// Validates an effort identifier, shared by the canonical key and its alias. +fn parse_agent_effort(file: &str, key: &str, value: &Value) -> Result { + let text = as_string(file, key, value)?; + Effort::parse(&text).ok_or_else(|| cfg_err(file, key, "expected one of: low, medium, high")) +} + /// Parses the `[list]` table, validating column identifiers. fn parse_list(file: &str, value: &Value, layer: &mut ConfigLayer) -> Result<()> { for (sub, val) in as_table(file, "list", value)? { @@ -375,8 +469,8 @@ mod tests { assert_eq!(layer.remove_untracked_blocks, Some(true)); assert_eq!(layer.pr_default_remote.as_deref(), Some("upstream")); assert_eq!(layer.submodules_init, Some(SubmoduleInit::Always)); - assert_eq!(layer.agent_model, Some(AgentModel::Opus)); - assert_eq!(layer.agent_effort, Some(Effort::High)); + assert_eq!(layer.agent_generation_model, Some(AgentModel::Opus)); + assert_eq!(layer.agent_generation_effort, Some(Effort::High)); assert_eq!(layer.list_show_untracked, Some(false)); assert_eq!(layer.list_columns, Some(vec![Column::Branch, Column::Pr])); assert_eq!(layer.ui_nerd_fonts, Some(true)); @@ -457,6 +551,60 @@ mod tests { assert_eq!(key, "agent.wiggle"); } + #[test] + fn generation_profile_parses_every_key() { + let layer = + parse("[agent.generation]\nprovider = \"claude\"\nmodel = \"opus\"\neffort = \"high\"") + .unwrap(); + assert_eq!(layer.agent_generation_provider, Some(AgentKind::Claude)); + assert_eq!(layer.agent_generation_model, Some(AgentModel::Opus)); + assert_eq!(layer.agent_generation_effort, Some(Effort::High)); + } + + #[test] + fn flat_agent_keys_are_aliases_for_the_generation_profile() { + // The released flat keys keep working and land on exactly the same + // layer fields, so an existing config needs no migration. + let layer = parse("[agent]\nmodel = \"opus\"\neffort = \"high\"").unwrap(); + assert_eq!(layer.agent_generation_model, Some(AgentModel::Opus)); + assert_eq!(layer.agent_generation_effort, Some(Effort::High)); + } + + #[test] + fn a_key_set_both_ways_in_one_file_is_rejected() { + // Table iteration order is not something a user should have to reason + // about, so this is an error rather than a last-one-wins race. + let (key, reason) = config_reason( + parse("[agent]\nmodel = \"opus\"\n[agent.generation]\nmodel = \"haiku\"").unwrap_err(), + ); + assert_eq!(key, "agent.model"); + assert!(reason.contains("agent.generation.model"), "{reason}"); + let (key, reason) = config_reason( + parse("[agent]\neffort = \"low\"\n[agent.generation]\neffort = \"high\"").unwrap_err(), + ); + assert_eq!(key, "agent.effort"); + assert!(reason.contains("agent.generation.effort"), "{reason}"); + } + + #[test] + fn agent_work_is_rejected_with_a_pointer_to_karet() { + // `wt` keeps the generation profile; running the agent is karet's job. + let (key, reason) = + config_reason(parse("[agent.work]\nprovider = \"claude\"").unwrap_err()); + assert_eq!(key, "agent.work"); + assert!(reason.contains("karet"), "{reason}"); + } + + #[test] + fn unknown_generation_keys_are_rejected_with_the_dotted_path() { + let (key, _) = config_reason(parse("[agent.generation]\nwiggle = true").unwrap_err()); + assert_eq!(key, "agent.generation.wiggle"); + let (key, reason) = + config_reason(parse("[agent.generation]\nprovider = \"codex\"").unwrap_err()); + assert_eq!(key, "agent.generation.provider"); + assert!(reason.contains("claude"), "{reason}"); + } + #[test] fn submodules_init_parses_and_validates() { assert_eq!( diff --git a/src/config/schema.rs b/src/config/schema.rs index f9d7836..8dc1c17 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -4,7 +4,7 @@ #[cfg(feature = "tui")] use ratatui::style::Color; -use crate::agent::{AgentModel, Effort}; +use crate::agent::{AgentKind, AgentModel, Effort}; use crate::cx::Env; #[cfg(feature = "tui")] use crate::keys::{KeyAction, KeyChord, Keymap}; @@ -147,6 +147,35 @@ impl CreateReflink { } } +/// The generation agent profile (`[agent.generation]`): which agent `wt` drives +/// for the short, structured generation steps it owns — the PR auto-fill and +/// the `wt issue` branch/brief proposal. +/// +/// `wt` owns *generation* only. Running a coding agent on the work itself is +/// karet's job, which is why there is no work profile here and why +/// `[agent.work]` is rejected with a pointer to karet. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct GenerationAgentConfig { + /// Which agent CLI to drive. + pub provider: AgentKind, + /// The model to request. + pub model: AgentModel, + /// The reasoning effort to request. + pub effort: Effort, +} + +impl Default for GenerationAgentConfig { + fn default() -> Self { + // Unchanged from the flat `[agent]` defaults these keys replace, so + // `wt pr open --ai` behaves identically before and after the move. + GenerationAgentConfig { + provider: AgentKind::Claude, + model: AgentModel::default(), + effort: Effort::default(), + } + } +} + /// The fully-resolved configuration after merging all layers. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Config { @@ -177,12 +206,11 @@ pub struct Config { /// Whether to copy-on-write clone a new worktree's content from an existing /// worktree instead of checking it out. pub create_reflink: CreateReflink, - /// Default model for the AI PR auto-fill (`wt pr open --ai`); overridable - /// per-invocation by `--model` or the TUI's `Ctrl-M` key. - pub agent_model: AgentModel, - /// Default effort for the AI PR auto-fill; overridable by `--effort` or the - /// TUI's `Ctrl-E` key. - pub agent_effort: Effort, + /// The generation agent profile (`[agent.generation]`), used by the AI PR + /// auto-fill (`wt pr open --ai`) and by `wt issue`. Model and effort are + /// overridable per-invocation by `--model`/`--effort` or the TUI's + /// `Ctrl-M`/`Ctrl-E` keys. + pub agent_generation: GenerationAgentConfig, /// Show `?` in the dirty column for untracked files. pub list_show_untracked: bool, /// Ordered list of columns to display in `wt list`. @@ -219,8 +247,7 @@ impl Default for Config { submodules_init: SubmoduleInit::default(), submodules_seed: SubmoduleSeed::default(), create_reflink: CreateReflink::default(), - agent_model: AgentModel::default(), - agent_effort: Effort::default(), + agent_generation: GenerationAgentConfig::default(), list_show_untracked: true, list_columns: Column::ALL.to_vec(), ui_nerd_fonts: false, @@ -279,11 +306,14 @@ impl Config { if let Some(v) = layer.create_reflink { self.create_reflink = v; } - if let Some(v) = layer.agent_model { - self.agent_model = v; + if let Some(v) = layer.agent_generation_provider { + self.agent_generation.provider = v; + } + if let Some(v) = layer.agent_generation_model { + self.agent_generation.model = v; } - if let Some(v) = layer.agent_effort { - self.agent_effort = v; + if let Some(v) = layer.agent_generation_effort { + self.agent_generation.effort = v; } if let Some(v) = layer.list_show_untracked { self.list_show_untracked = v; @@ -370,10 +400,12 @@ pub struct ConfigLayer { pub submodules_seed: Option, /// `create.reflink`. pub create_reflink: Option, - /// `agent.model`. - pub agent_model: Option, - /// `agent.effort`. - pub agent_effort: Option, + /// `agent.generation.provider`. + pub agent_generation_provider: Option, + /// `agent.generation.model`, or its deprecated `agent.model` alias. + pub agent_generation_model: Option, + /// `agent.generation.effort`, or its deprecated `agent.effort` alias. + pub agent_generation_effort: Option, /// `list.show_untracked`. pub list_show_untracked: Option, /// `list.columns`. @@ -489,8 +521,9 @@ mod tests { assert!(!c.remove_untracked_blocks); assert_eq!(c.pr_default_remote, "origin"); assert_eq!(c.submodules_init, SubmoduleInit::Prompt); - assert_eq!(c.agent_model, AgentModel::Sonnet); - assert_eq!(c.agent_effort, Effort::Medium); + assert_eq!(c.agent_generation.provider, AgentKind::Claude); + assert_eq!(c.agent_generation.model, AgentModel::Sonnet); + assert_eq!(c.agent_generation.effort, Effort::Medium); assert!(c.list_show_untracked); assert_eq!(c.list_columns, Column::ALL.to_vec()); assert!(!c.ui_nerd_fonts); @@ -542,8 +575,8 @@ mod tests { remove_delete_merged_branch: Some(false), remove_untracked_blocks: Some(true), submodules_init: Some(SubmoduleInit::Always), - agent_model: Some(AgentModel::Haiku), - agent_effort: Some(Effort::Low), + agent_generation_model: Some(AgentModel::Haiku), + agent_generation_effort: Some(Effort::Low), list_show_untracked: Some(false), ui_nerd_fonts: Some(true), ui_color: Some(ColorChoice::Never), @@ -557,8 +590,8 @@ mod tests { assert!(!c.remove_delete_merged_branch); assert!(c.remove_untracked_blocks); assert_eq!(c.submodules_init, SubmoduleInit::Always); - assert_eq!(c.agent_model, AgentModel::Haiku); - assert_eq!(c.agent_effort, Effort::Low); + assert_eq!(c.agent_generation.model, AgentModel::Haiku); + assert_eq!(c.agent_generation.effort, Effort::Low); assert!(!c.list_show_untracked); assert!(c.ui_nerd_fonts); assert_eq!(c.ui_color, ColorChoice::Never); diff --git a/src/config/wtconfig.rs b/src/config/wtconfig.rs index a89938f..bb5561b 100644 --- a/src/config/wtconfig.rs +++ b/src/config/wtconfig.rs @@ -74,6 +74,15 @@ pub struct WtMeta { pub pr_url: Option, /// Whether the branch/worktree was created by `wt` (§10). pub created_by_wt: bool, + /// Linked GitHub issue number, for issue worktrees (issue #100). + pub issue_number: Option, + /// Cached issue title, so `wt list` can show it offline. + pub issue_title: Option, + /// Cached issue URL. + pub issue_url: Option, + /// The generated implementation brief. Persisted so an embedder (karet) + /// can read it instead of regenerating it. + pub issue_brief: Option, } /// The config key for `wt..`. @@ -102,6 +111,18 @@ pub fn read_meta(repo: &gix::Repository, branch: &str) -> WtMeta { let created_by_wt = config .boolean(key(branch, "createdByWt").as_str()) .unwrap_or(false); + let issue_number = config + .string(key(branch, "issueNumber").as_str()) + .and_then(|v| v.to_string().parse::().ok()); + let issue_title = config + .string(key(branch, "issueTitle").as_str()) + .map(|v| v.to_string()); + let issue_url = config + .string(key(branch, "issueUrl").as_str()) + .map(|v| v.to_string()); + let issue_brief = config + .string(key(branch, "issueBrief").as_str()) + .map(|v| v.to_string()); WtMeta { base_ref, pr_number, @@ -109,6 +130,10 @@ pub fn read_meta(repo: &gix::Repository, branch: &str) -> WtMeta { pr_title, pr_url, created_by_wt, + issue_number, + issue_title, + issue_url, + issue_brief, } } @@ -170,6 +195,48 @@ pub fn write_pr_number( Ok(()) } +/// Records the linked issue number for `branch`. +pub fn write_issue_number( + git: &dyn GitCli, + repo_root: &Path, + branch: &str, + number: u64, +) -> Result<()> { + git.run( + repo_root, + &["config", &key(branch, "issueNumber"), &number.to_string()], + )?; + Ok(()) +} + +/// Records the cached issue title for `branch`. +pub fn write_issue_title( + git: &dyn GitCli, + repo_root: &Path, + branch: &str, + title: &str, +) -> Result<()> { + git.run(repo_root, &["config", &key(branch, "issueTitle"), title])?; + Ok(()) +} + +/// Records the issue URL for `branch`. +pub fn write_issue_url(git: &dyn GitCli, repo_root: &Path, branch: &str, url: &str) -> Result<()> { + git.run(repo_root, &["config", &key(branch, "issueUrl"), url])?; + Ok(()) +} + +/// Records the generated implementation brief for `branch`. +pub fn write_issue_brief( + git: &dyn GitCli, + repo_root: &Path, + branch: &str, + brief: &str, +) -> Result<()> { + git.run(repo_root, &["config", &key(branch, "issueBrief"), brief])?; + Ok(()) +} + /// Marks `branch` as created by `wt`. pub fn mark_created_by_wt(git: &dyn GitCli, repo_root: &Path, branch: &str) -> Result<()> { git.run(repo_root, &["config", &key(branch, "createdByWt"), "true"])?; @@ -298,11 +365,47 @@ mod tests { } } + #[test] + fn issue_link_round_trips() { + let repo = TestRepo::init(); + write_issue_number(&RealGit, repo.root(), "topic", 7).unwrap(); + write_issue_title(&RealGit, repo.root(), "topic", "Add login").unwrap(); + write_issue_url(&RealGit, repo.root(), "topic", "https://example.com/7").unwrap(); + write_issue_brief(&RealGit, repo.root(), "topic", "Wire up the form.").unwrap(); + let got = meta(&repo, "topic"); + assert_eq!(got.issue_number, Some(7)); + assert_eq!(got.issue_title.as_deref(), Some("Add login")); + assert_eq!(got.issue_url.as_deref(), Some("https://example.com/7")); + assert_eq!(got.issue_brief.as_deref(), Some("Wire up the form.")); + } + + #[test] + fn issue_keys_are_absent_until_written() { + // The issue keys are purely additive: a repository written by an older + // build has none of them, and every one must read back as `None` rather + // than fail. This is what makes them safe without a `wt.schema` bump. + let repo = TestRepo::init(); + write_base_ref(&RealGit, repo.root(), "topic", "main").unwrap(); + let got = meta(&repo, "topic"); + assert_eq!(got.issue_number, None); + assert_eq!(got.issue_title, None); + assert_eq!(got.issue_url, None); + assert_eq!(got.issue_brief, None); + } + #[test] fn clear_removes_all_metadata() { let repo = TestRepo::init(); write_base_ref(&RealGit, repo.root(), "topic", "main").unwrap(); mark_created_by_wt(&RealGit, repo.root(), "topic").unwrap(); + // Every key the section can hold, so `--remove-section` is proven to + // clear the issue keys too and not just the ones it predates. + write_pr(&RealGit, repo.root(), "topic", 42, "open", "Title").unwrap(); + write_pr_url(&RealGit, repo.root(), "topic", "https://example.com/42").unwrap(); + write_issue_number(&RealGit, repo.root(), "topic", 7).unwrap(); + write_issue_title(&RealGit, repo.root(), "topic", "Add login").unwrap(); + write_issue_url(&RealGit, repo.root(), "topic", "https://example.com/7").unwrap(); + write_issue_brief(&RealGit, repo.root(), "topic", "Wire up the form.").unwrap(); clear_meta(&RealGit, repo.root(), "topic").unwrap(); assert_eq!(meta(&repo, "topic"), WtMeta::default()); // Clearing again (no section) is not an error. diff --git a/src/error.rs b/src/error.rs index fbf8d2d..a233986 100644 --- a/src/error.rs +++ b/src/error.rs @@ -73,6 +73,17 @@ pub enum Error { #[error("{0}")] AgentUnavailable(String), + /// A code-agent run exceeded its deadline and was killed. Generation is + /// always best-effort, so callers are expected to fall back rather than + /// surface this to the user as a failure. + #[error("{binary} did not respond within {seconds}s")] + AgentTimeout { + /// The agent binary that was killed. + binary: String, + /// The deadline it exceeded, in seconds. + seconds: u64, + }, + /// The repository's `wt.schema` was written by a newer `wt` (or embedder) /// than this one; reading it could silently misinterpret metadata, so the /// operation is refused (issue #99). diff --git a/src/gh/mod.rs b/src/gh/mod.rs index f795b03..ffebf7e 100644 --- a/src/gh/mod.rs +++ b/src/gh/mod.rs @@ -9,10 +9,32 @@ use std::path::Path; use std::process::Command; use crate::error::{Error, Result}; -pub use types::{Author, OpenPr, PrSummary, PrView, pr_state}; +pub use types::{ + Author, IssueLabel, IssueMilestone, IssueSummary, IssueType, IssueView, OpenPr, PrSummary, + PrView, pr_state, +}; /// Performs GitHub pull-request operations via `gh`. pub trait GhClient { + /// Lists open issues for the repository at `dir`. + /// + /// Defaulted so third-party [`GhClient`] implementations stay + /// source-compatible; clients that support the issue workflow override it. + fn list_open_issues(&self, _dir: &Path) -> Result> { + Err(Error::GhUnavailable( + "GitHub issue operations are not supported by this client".into(), + )) + } + + /// Views the issue identified by `target` (a number or URL). + /// + /// Defaulted for the same reason as [`GhClient::list_open_issues`]. + fn view_issue(&self, _dir: &Path, _target: &str) -> Result { + Err(Error::GhUnavailable( + "GitHub issue operations are not supported by this client".into(), + )) + } + /// Lists open PRs for the repository at `dir`. fn list_open_prs(&self, dir: &Path) -> Result>; @@ -51,6 +73,37 @@ pub trait GhClient { pub struct RealGh; impl GhClient for RealGh { + fn list_open_issues(&self, dir: &Path) -> Result> { + let output = run_gh( + dir, + &[ + "issue", + "list", + "--state", + "open", + "--limit", + "100", + "--json", + "number,title,state,labels,issueType,milestone,createdAt,url", + ], + )?; + serde_json::from_str(&output).map_err(Error::from) + } + + fn view_issue(&self, dir: &Path, target: &str) -> Result { + let output = run_gh( + dir, + &[ + "issue", + "view", + target, + "--json", + "number,title,body,state,labels,issueType,milestone,createdAt,updatedAt,url", + ], + )?; + serde_json::from_str(&output).map_err(Error::from) + } + fn list_open_prs(&self, dir: &Path) -> Result> { let output = run_gh( dir, diff --git a/src/gh/types.rs b/src/gh/types.rs index 9d97114..84e9816 100644 --- a/src/gh/types.rs +++ b/src/gh/types.rs @@ -1,9 +1,96 @@ //! `gh` JSON shapes and their mapping to the domain model (spec §4). -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use crate::model::PrState; +/// A GitHub issue label. +#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize, Serialize)] +pub struct IssueLabel { + /// Label name. + #[serde(default)] + pub name: String, +} + +/// A GitHub issue type (the organization-defined "type" field, distinct from +/// labels). +#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize, Serialize)] +pub struct IssueType { + /// Issue type name. + #[serde(default)] + pub name: String, +} + +/// A GitHub issue milestone. +#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize, Serialize)] +pub struct IssueMilestone { + /// Milestone title. + #[serde(default)] + pub title: String, +} + +/// An open issue as returned by `gh issue list --json ...`. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] +pub struct IssueSummary { + /// Issue number. + pub number: u64, + /// Issue title. + pub title: String, + /// Issue state (`OPEN`/`CLOSED`). + pub state: String, + /// Labels attached to the issue. + #[serde(default)] + pub labels: Vec, + /// Optional organization-defined issue type. + #[serde(rename = "issueType", default)] + pub issue_type: Option, + /// Optional milestone. + #[serde(default)] + pub milestone: Option, + /// ISO-8601 creation time. + #[serde(rename = "createdAt", default)] + pub created_at: String, + /// Issue web URL. + #[serde(default)] + pub url: String, +} + +/// A full issue as returned by `gh issue view --json ...`. +/// +/// This carries only the token-efficient issue context the generation step +/// needs; comments, assignees, reactions and project bookkeeping are +/// deliberately not requested. +#[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] +pub struct IssueView { + /// Issue number. + pub number: u64, + /// Issue title. + pub title: String, + /// Issue body. + #[serde(default)] + pub body: String, + /// Issue state (`OPEN`/`CLOSED`). + pub state: String, + /// Labels attached to the issue. + #[serde(default)] + pub labels: Vec, + /// Optional organization-defined issue type. + #[serde(rename = "issueType", default)] + pub issue_type: Option, + /// Optional milestone. + #[serde(default)] + pub milestone: Option, + /// ISO-8601 creation time. + #[serde(rename = "createdAt", default)] + pub created_at: String, + /// ISO-8601 update time. + #[serde(rename = "updatedAt", default)] + pub updated_at: String, + /// Issue web URL. + #[serde(default)] + pub url: String, +} + /// A PR author (`{ "login": ... }`). #[derive(Debug, Clone, Default, Deserialize)] pub struct Author { @@ -124,6 +211,53 @@ pub fn pr_state(state: &str, is_draft: bool) -> PrState { mod tests { use super::*; + #[test] + fn parses_issue_list_json() { + let json = r#"[ + {"number": 12, "title": "Broken login", "state": "OPEN", + "labels": [{"name": "bug"}], "issueType": {"name": "Bug"}, + "milestone": {"title": "v2"}, "createdAt": "2024-01-15T10:30:00Z", + "url": "https://github.com/o/r/issues/12"}, + {"number": 13, "title": "Bare", "state": "OPEN"} + ]"#; + let issues: Vec = serde_json::from_str(json).unwrap(); + assert_eq!(issues.len(), 2); + assert_eq!(issues[0].number, 12); + assert_eq!(issues[0].labels[0].name, "bug"); + assert_eq!(issues[0].issue_type.as_ref().unwrap().name, "Bug"); + assert_eq!(issues[0].milestone.as_ref().unwrap().title, "v2"); + // Every optional field defaults, so a sparse `gh` response still parses. + assert!(issues[1].labels.is_empty()); + assert_eq!(issues[1].issue_type, None); + assert_eq!(issues[1].milestone, None); + assert_eq!(issues[1].url, ""); + } + + #[test] + fn parses_issue_view_json() { + let json = r#"{"number": 7, "title": "Add login", "body": "Please add it.", + "state": "OPEN", "labels": [{"name": "enhancement"}], + "issueType": null, "milestone": null, + "createdAt": "2024-01-15T10:30:00Z", "updatedAt": "2024-02-01T09:00:00Z", + "url": "https://github.com/o/r/issues/7"}"#; + let issue: IssueView = serde_json::from_str(json).unwrap(); + assert_eq!(issue.number, 7); + assert_eq!(issue.body, "Please add it."); + assert_eq!(issue.labels[0].name, "enhancement"); + assert_eq!(issue.issue_type, None); + assert_eq!(issue.url, "https://github.com/o/r/issues/7"); + } + + #[test] + fn issue_view_tolerates_a_body_only_response() { + // `gh` omits fields the repository does not use; only number/title/state + // are required, so the rest must not make parsing fail. + let issue: IssueView = + serde_json::from_str(r#"{"number": 1, "title": "T", "state": "OPEN"}"#).unwrap(); + assert_eq!(issue.body, ""); + assert_eq!(issue.updated_at, ""); + } + #[test] fn parses_pr_list_json() { let json = r#"[ diff --git a/src/git/mod.rs b/src/git/mod.rs index d6597ea..ab24817 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -27,10 +27,12 @@ pub(crate) use refs::{ validate_branch_name, }; // Only the command handlers reach for these; the core library does not. +// `default_base_ref` moved here from the TUI group when `wt issue` (a CLI +// command) started resolving its base from `origin/HEAD`. #[cfg(feature = "cli")] -pub(crate) use refs::{all_branches, current_branch, remote_branches}; -// Only the TUI reaches for these. +pub(crate) use refs::{all_branches, current_branch, default_base_ref, remote_branches}; +// Only the TUI reaches for this. #[cfg(feature = "tui")] -pub(crate) use refs::{default_base_ref, origin_head_branch}; +pub(crate) use refs::origin_head_branch; pub(crate) use status::status_of; pub(crate) use worktrees::enumerate; diff --git a/src/lib.rs b/src/lib.rs index 2313876..127a26e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -26,6 +26,9 @@ pub mod keys; pub mod model; pub mod naming; pub mod output; +/// Human-facing progress for slow foreground operations (CLI only). +#[cfg(feature = "cli")] +pub(crate) mod progress; pub mod query; pub mod slug; pub mod template; diff --git a/src/model.rs b/src/model.rs index 2b29730..3f76354 100644 --- a/src/model.rs +++ b/src/model.rs @@ -53,6 +53,8 @@ pub struct Worktree { pub commit: Option, /// Recorded pull request; `None` when none. pub pr: Option, + /// Linked GitHub issue; `None` when none (issue #100). + pub issue: Option, /// Whether a checked-out worktree exists for this row. `false` marks a /// "branch row": a local branch with no worktree, listed beneath the real /// worktrees with its ahead/behind relative to its base (issue #47). Not part @@ -103,6 +105,7 @@ impl Worktree { base_ref: None, commit: None, pr: None, + issue: None, has_worktree: true, recent_commits: Vec::new(), pr_url: None, @@ -166,6 +169,21 @@ pub struct Pr { pub title: String, } +/// A linked GitHub issue, recorded by `wt issue` (issue #100). +/// +/// The link lives in `wt..issue*` git config rather than in the branch +/// name, so a branch that does not follow the `TYPE/{number}-SLUG` convention is +/// still resolvable back to its issue. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct IssueLink { + /// Issue number. + pub number: u64, + /// Issue title. + pub title: String, + /// Issue web URL. + pub url: String, +} + /// Pull-request state, mirroring `gh` (spec §7). #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] #[serde(rename_all = "lowercase")] @@ -293,6 +311,9 @@ pub enum Column { Commit, /// PR number and state. Pr, + /// Linked issue number. Deliberately absent from [`Column::ALL`]: it is + /// opt-in via `list.columns`, so the default table is unchanged. + Issue, } impl Column { @@ -317,6 +338,7 @@ impl Column { "ahead-behind" => Column::AheadBehind, "commit" => Column::Commit, "pr" => Column::Pr, + "issue" => Column::Issue, _ => return None, }) } @@ -331,6 +353,7 @@ impl Column { Column::AheadBehind => "ahead-behind", Column::Commit => "commit", Column::Pr => "pr", + Column::Issue => "issue", } } } @@ -361,7 +384,8 @@ mod tests { "author": "Alice", "timestamp": "2024-01-15T10:30:00Z" }, - "pr": { "number": 42, "state": "open", "title": "Add login page" } + "pr": { "number": 42, "state": "open", "title": "Add login page" }, + "issue": null }"#; fn spec_example_worktree() -> Worktree { @@ -391,6 +415,7 @@ mod tests { state: PrState::Open, title: "Add login page".into(), }), + issue: None, has_worktree: true, recent_commits: Vec::new(), pr_url: None, @@ -398,6 +423,15 @@ mod tests { } } + #[test] + fn issue_column_parses_but_is_not_a_default_column() { + assert_eq!(Column::parse("issue"), Some(Column::Issue)); + assert_eq!(Column::Issue.identifier(), "issue"); + // Deliberate: `issue` is opt-in via `list.columns`, so adding it to + // `ALL` would silently widen every default table (issue #100). + assert!(!Column::ALL.contains(&Column::Issue)); + } + #[test] fn serializes_to_spec_schema() { let got: serde_json::Value = serde_json::to_value(spec_example_worktree()).unwrap(); diff --git a/src/output/render.rs b/src/output/render.rs index 2aee711..cd4eb1c 100644 --- a/src/output/render.rs +++ b/src/output/render.rs @@ -71,6 +71,14 @@ pub fn pr_cell(worktree: &Worktree) -> String { } } +/// The issue cell: `#`, or empty when the branch has no linked issue. +pub fn issue_cell(worktree: &Worktree) -> String { + match &worktree.issue { + Some(issue) => format!("#{}", issue.number), + None => String::new(), + } +} + /// The path cell: relative to the repo root, or absolute if outside it. pub fn path_cell(worktree: &Worktree, repo_root: &Path) -> String { match worktree.path.strip_prefix(repo_root) { @@ -103,6 +111,7 @@ pub fn cell(worktree: &Worktree, column: Column, ctx: &RenderCtx) -> String { Column::AheadBehind => ahead_behind_cell(worktree), Column::Commit => commit_cell(worktree, ctx.now), Column::Pr => pr_cell(worktree), + Column::Issue => issue_cell(worktree), } } @@ -142,6 +151,9 @@ pub(crate) fn status_block(worktree: &Worktree, entries: &[StatusEntry]) -> Stri pr.title ); } + if let Some(issue) = &worktree.issue { + let _ = writeln!(out, "issue: #{} \"{}\"", issue.number, issue.title); + } if !entries.is_empty() { let _ = writeln!(out, "dirty:"); for entry in entries { @@ -154,7 +166,7 @@ pub(crate) fn status_block(worktree: &Worktree, entries: &[StatusEntry]) -> Stri #[cfg(test)] mod tests { use super::*; - use crate::model::{Commit, Pr, PrState}; + use crate::model::{Commit, IssueLink, Pr, PrState}; use std::path::PathBuf; fn base() -> Worktree { @@ -222,6 +234,18 @@ mod tests { assert_eq!(path_cell(&w, root), "/elsewhere/y"); } + #[test] + fn issue_cell_renders_the_linked_number() { + let mut w = base(); + assert_eq!(issue_cell(&w), ""); + w.issue = Some(IssueLink { + number: 7, + title: "Add login".into(), + url: "https://github.com/o/r/issues/7".into(), + }); + assert_eq!(issue_cell(&w), "#7"); + } + #[test] fn pr_cell_renders_number_and_state() { let mut w = base(); diff --git a/src/output/table.rs b/src/output/table.rs index 524c24b..2f80861 100644 --- a/src/output/table.rs +++ b/src/output/table.rs @@ -64,6 +64,7 @@ fn cell_color(column: Column, value: &str) -> Option<&'static str> { _ if trimmed.contains("(draft)") => Some(ansi::DIM), _ => None, }, + Column::Issue if !trimmed.is_empty() => Some(ansi::CYAN), _ => None, } } diff --git a/src/progress.rs b/src/progress.rs new file mode 100644 index 0000000..c6bed24 --- /dev/null +++ b/src/progress.rs @@ -0,0 +1,96 @@ +//! Human-facing progress for slow foreground operations. + +use std::sync::mpsc; +use std::time::{Duration, Instant}; + +use crate::cx::Stream; +use crate::error::{Error, Result}; + +const FRAMES: &[&str] = &["|", "/", "-", "\\"]; +const TICK: Duration = Duration::from_millis(80); + +/// Runs `operation` while displaying an animated TTY spinner on stderr. +/// Non-TTY streams receive stable start/finish lines with no control codes. +pub(crate) fn run(stream: &mut Stream, label: &str, operation: F) -> Result +where + T: Send, + F: FnOnce() -> Result + Send, +{ + let started = Instant::now(); + if !stream.is_tty() { + stream.line(&format!("… {label}"))?; + let result = operation(); + finish_line(stream, label, started, result.is_ok())?; + return result; + } + + std::thread::scope(|scope| { + let (tx, rx) = mpsc::sync_channel(1); + scope.spawn(move || { + let _ = tx.send(operation()); + }); + let mut frame = 0; + let result = loop { + stream.text(&format!( + "\r\x1b[2K{} {label}", + FRAMES[frame % FRAMES.len()] + ))?; + stream.flush()?; + match rx.recv_timeout(TICK) { + Ok(result) => break result, + Err(mpsc::RecvTimeoutError::Timeout) => frame = frame.wrapping_add(1), + Err(mpsc::RecvTimeoutError::Disconnected) => { + break Err(Error::operation(format!("{label} task panicked"))); + } + } + }; + stream.text("\r\x1b[2K")?; + finish_line(stream, label, started, result.is_ok())?; + result + }) +} + +fn finish_line(stream: &mut Stream, label: &str, started: Instant, success: bool) -> Result<()> { + let marker = if success { "✓" } else { "✗" }; + stream.line(&format!( + "{marker} {label} ({:.1}s)", + started.elapsed().as_secs_f64() + )) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::testutil::SharedBuf; + + #[test] + fn non_tty_progress_is_stable_and_reports_success() { + let buffer = SharedBuf::new(); + let mut stream = Stream::new(Box::new(buffer.clone()), false); + assert_eq!(run(&mut stream, "Generating", || Ok(42)).unwrap(), 42); + let output = buffer.contents(); + assert!(output.starts_with("… Generating\n")); + assert!(output.contains("✓ Generating (")); + assert!(!output.contains("\x1b")); + } + + #[test] + fn non_tty_progress_reports_failure_and_preserves_error() { + let buffer = SharedBuf::new(); + let mut stream = Stream::new(Box::new(buffer.clone()), false); + let error = + run::<(), _>(&mut stream, "Generating", || Err(Error::operation("boom"))).unwrap_err(); + assert_eq!(error.to_string(), "boom"); + assert!(buffer.contents().contains("✗ Generating (")); + } + + #[test] + fn tty_progress_uses_spinner_and_clears_the_line() { + let buffer = SharedBuf::new(); + let mut stream = Stream::new(Box::new(buffer.clone()), true); + run(&mut stream, "Fetching", || Ok(())).unwrap(); + let output = buffer.contents(); + assert!(output.contains("\r\x1b[2K| Fetching")); + assert!(output.contains("✓ Fetching (")); + } +} diff --git a/src/testutil.rs b/src/testutil.rs index b4663bf..6da8340 100644 --- a/src/testutil.rs +++ b/src/testutil.rs @@ -17,7 +17,7 @@ use std::collections::VecDeque; use crate::agent::{AgentClient, AgentKind, AgentOptions, AgentRun, AgentVersion, DetectedAgent}; use crate::cx::{Cx, Env, Input, Stream}; use crate::error::Error; -use crate::gh::{GhClient, OpenPr, PrSummary, PrView, RealGh}; +use crate::gh::{GhClient, IssueSummary, IssueView, OpenPr, PrSummary, PrView, RealGh}; use crate::git::cli::{GitCli, RealGit}; /// A fake [`GhClient`] returning canned PR data or simulating an unavailable @@ -33,6 +33,8 @@ pub(crate) struct FakeGh { edit_stdout: String, create_args: Arc>>>, edit_args: Arc>>>, + issue_list: Vec, + issue: Option, } #[allow(dead_code)] @@ -65,6 +67,22 @@ impl FakeGh { } } + /// A fake that returns `issue` from `view_issue`. + pub(crate) fn with_issue(issue: IssueView) -> Self { + FakeGh { + issue: Some(issue), + available: true, + ..Default::default() + } + } + + /// Sets the issues returned by `list_open_issues`. + pub(crate) fn with_issue_list(mut self, list: Vec) -> Self { + self.issue_list = list; + self.available = true; + self + } + /// A fake simulating a missing/unauthenticated `gh`. pub(crate) fn unavailable() -> Self { FakeGh::default() @@ -94,6 +112,23 @@ impl FakeGh { } impl GhClient for FakeGh { + fn list_open_issues(&self, _dir: &std::path::Path) -> crate::error::Result> { + if self.available { + Ok(self.issue_list.clone()) + } else { + Err(Error::GhUnavailable("gh unavailable".into())) + } + } + + fn view_issue(&self, _dir: &std::path::Path, _target: &str) -> crate::error::Result { + if !self.available { + return Err(Error::GhUnavailable("gh unavailable".into())); + } + self.issue + .clone() + .ok_or_else(|| Error::operation("no issue configured")) + } + fn list_open_prs(&self, _dir: &std::path::Path) -> crate::error::Result> { if self.available { Ok(self.list.clone()) @@ -158,6 +193,9 @@ pub(crate) enum AgentBehavior { Erroring(String), /// Agent absent: `detect` returns `Ok(None)` and `run` returns `AgentUnavailable`. Unavailable, + /// Agent present but unresponsive: `run` returns `AgentTimeout`, as the real + /// client does once a run exceeds its deadline. + TimingOut, } /// A fake [`AgentClient`] for tests: returns a canned draft, simulates an @@ -192,6 +230,11 @@ impl FakeAgent { FakeAgent::new(AgentBehavior::Unavailable) } + /// A present but unresponsive agent (`run` → `AgentTimeout`). + pub(crate) fn timing_out() -> Self { + FakeAgent::new(AgentBehavior::TimingOut) + } + /// The [`AgentOptions`] passed to the most recent `run`, if any. pub(crate) fn last_opts(&self) -> Option { *self.last_opts.lock().expect("lock") @@ -235,6 +278,10 @@ impl AgentClient for FakeAgent { raw: serde_json::Value::Null, }), AgentBehavior::Unavailable => Err(Error::AgentUnavailable("claude unavailable".into())), + AgentBehavior::TimingOut => Err(Error::AgentTimeout { + binary: kind.as_str().to_string(), + seconds: 120, + }), } } } diff --git a/src/tui/runtime/effects.rs b/src/tui/runtime/effects.rs index 9ea150e..33080eb 100644 --- a/src/tui/runtime/effects.rs +++ b/src/tui/runtime/effects.rs @@ -756,6 +756,7 @@ pub(crate) fn do_draft_pr_ai( Mode::PrCompose(state) => crate::agent::AgentOptions { model: state.model, effort: state.effort, + timeout: None, }, _ => crate::agent::AgentOptions::default(), }; diff --git a/src/tui/runtime/mod.rs b/src/tui/runtime/mod.rs index 137a558..3294b79 100644 --- a/src/tui/runtime/mod.rs +++ b/src/tui/runtime/mod.rs @@ -1734,6 +1734,7 @@ mod tests { Some(crate::agent::AgentOptions { model: crate::agent::AgentModel::Opus, effort: crate::agent::Effort::High, + timeout: None, }) ); } diff --git a/src/worktree/mod.rs b/src/worktree/mod.rs index 2818868..1fe80a6 100644 --- a/src/worktree/mod.rs +++ b/src/worktree/mod.rs @@ -35,6 +35,6 @@ pub use service::{ }; #[cfg(feature = "cli")] pub(crate) use service::{ - WorkspaceParts, apply_meta, create_in, lock_repo, remove_in, resolve_base, resolve_target, - rollback_worktree, run_best_effort, same_path, + WorkspaceParts, apply_meta, create_in, lock_repo, preview_target, remove_in, resolve_base, + resolve_target, rollback_worktree, run_best_effort, same_path, }; diff --git a/src/worktree/rows.rs b/src/worktree/rows.rs index 808e9dd..43d2192 100644 --- a/src/worktree/rows.rs +++ b/src/worktree/rows.rs @@ -20,7 +20,7 @@ use crate::git::{ CommitInfo, Upstream, abbrev_len, ahead_behind, branch_ref, commit_info, default_branch, enumerate, is_ancestor, local_branches, recent_commits, resolve_hex, status_of, upstream_of, }; -use crate::model::{Commit, MergeState, Pr, PrState, Worktree}; +use crate::model::{Commit, IssueLink, MergeState, Pr, PrState, Worktree}; use crate::slug::slugify; use crate::time::iso8601; @@ -67,6 +67,7 @@ pub(crate) fn enrich_worktree(repo: &Repo, git: &dyn GitCli, abbrev: usize, wt: let meta = wtconfig::read_meta(repo.gix(), &branch); wt.base_ref = meta.base_ref.clone(); wt.pr = build_pr(&meta); + wt.issue = build_issue(&meta); wt.pr_url = meta.pr_url.clone(); // Upstream is read from config and is known even for a missing worktree; // ahead/behind needs the working directory, so it is computed only when @@ -208,6 +209,18 @@ fn compute_merge_state( } /// Builds the PR row from cached `wt.*` metadata, if a PR number is recorded. +/// Builds the [`IssueLink`] from `wt..issue*`, or `None` when the branch +/// has no recorded issue. The number is what makes the link real; title and URL +/// are cached conveniences that default to empty. +fn build_issue(meta: &WtMeta) -> Option { + let number = meta.issue_number?; + Some(IssueLink { + number, + title: meta.issue_title.clone().unwrap_or_default(), + url: meta.issue_url.clone().unwrap_or_default(), + }) +} + fn build_pr(meta: &WtMeta) -> Option { let number = meta.pr_number?; let state = meta @@ -281,6 +294,7 @@ fn enrich_branch_row(repo: &Repo, git: &dyn GitCli, abbrev: usize, row: &mut Wor let meta = wtconfig::read_meta(repo.gix(), &branch); row.base_ref = meta.base_ref.clone(); row.pr = build_pr(&meta); + row.issue = build_issue(&meta); row.pr_url = meta.pr_url.clone(); if let Some(up) = upstream_of(repo.gix(), &branch) { row.upstream = Some(up.display); diff --git a/src/worktree/service.rs b/src/worktree/service.rs index 3c39599..9cce5ba 100644 --- a/src/worktree/service.rs +++ b/src/worktree/service.rs @@ -275,6 +275,14 @@ pub struct MetaUpdate { pub pr_title: Option, /// The cached PR URL. pub pr_url: Option, + /// The linked GitHub issue number (issue #100). + pub issue_number: Option, + /// The cached issue title. + pub issue_title: Option, + /// The cached issue URL. + pub issue_url: Option, + /// The generated implementation brief, persisted for embedders. + pub issue_brief: Option, /// Marks the branch as created by `wt` (spec §10), which is what allows a /// later remove to delete it. There is no un-marking: `false` leaves the /// recorded flag as it is. @@ -304,6 +312,18 @@ pub(crate) fn apply_meta( if let Some(url) = &update.pr_url { wtconfig::write_pr_url(git, root, branch, url)?; } + if let Some(number) = update.issue_number { + wtconfig::write_issue_number(git, root, branch, number)?; + } + if let Some(title) = &update.issue_title { + wtconfig::write_issue_title(git, root, branch, title)?; + } + if let Some(url) = &update.issue_url { + wtconfig::write_issue_url(git, root, branch, url)?; + } + if let Some(brief) = &update.issue_brief { + wtconfig::write_issue_brief(git, root, branch, brief)?; + } if update.created_by_wt { wtconfig::mark_created_by_wt(git, root, branch)?; } @@ -485,6 +505,46 @@ pub(crate) fn resolve_base(repo: &Repo, config: &Config, explicit: Option<&str>) ("HEAD".to_string(), true) } +/// The path `create_in` would materialize `branch` at, without creating +/// anything. +/// +/// Exists so a caller that previews the target before confirming (`wt issue`) +/// derives it from the same slug and template logic `create_in` uses, rather +/// than reimplementing it and drifting. +#[cfg(feature = "cli")] +pub(crate) fn preview_target( + ws: &WorkspaceParts<'_>, + branch: &str, + base: Option<&str>, +) -> Result { + let (slug, _) = target_slug(ws, branch, base)?; + render_target(ws.config, ws.root, branch, &slug, ws.env) +} + +/// The directory slug for `branch`, derived exactly as [`create_in`] derives it. +/// +/// The commit only matters for a branch name that slugifies to nothing, where it +/// is the fallback — but that is precisely the case a preview would otherwise get +/// wrong, so the rule is shared rather than restated: an existing branch keys off +/// its own tip, a new one off the base it will fork from. +fn target_slug( + ws: &WorkspaceParts<'_>, + branch: &str, + base: Option<&str>, +) -> Result<(String, String)> { + let commit = match resolve_hex(ws.repo.gix(), &branch_ref(branch)) { + Some(oid) => oid, + None => { + let base_ref = resolve_base(ws.repo, ws.config, base).0; + resolve_hex(ws.repo.gix(), &base_ref) + .ok_or_else(|| Error::operation(format!("base ref {base_ref:?} not found")))? + } + }; + let short_hash = commit.get(..7).unwrap_or(&commit).to_string(); + let slug = slugify_with_fallback(branch, &short_hash); + Ok((slug, short_hash)) +} + /// Creates (or reuses) a worktree per `options`; see [`Workspace::create`]. pub(crate) fn create_in( ws: &WorkspaceParts<'_>, @@ -502,13 +562,10 @@ pub(crate) fn create_in( } else { Some(resolve_base(ws.repo, ws.config, options.base.as_deref()).0) }; - let base_commit = match &base_ref { - Some(base) => resolve_hex(ws.repo.gix(), base) - .ok_or_else(|| Error::operation(format!("base ref {base:?} not found")))?, - None => resolve_hex(ws.repo.gix(), &branch_ref(&branch)).unwrap_or_default(), - }; - let short_hash = base_commit.get(..7).unwrap_or(&base_commit).to_string(); - let slug = slugify_with_fallback(&branch, &short_hash); + // Shared with `preview_target`, so a confirmation preview cannot name a + // different directory than the one that gets created. This also performs the + // base-ref existence check, erroring before anything is created. + let (slug, short_hash) = target_slug(ws, &branch, options.base.as_deref())?; // If the branch is already checked out, either reuse (same target) or // refuse. The reuse path is idempotent: no copy step, no hook. @@ -1161,6 +1218,61 @@ mod tests { assert!(err.to_string().contains("already checked out")); } + // `preview_target` exists for the CLI's confirmation preview, so its tests + // only compile where it does. + #[cfg(feature = "cli")] + #[test] + fn preview_target_names_the_directory_create_actually_makes() { + // The slug's commit is only consulted for a branch name that slugifies to + // nothing — and there a *new* branch keys off the base it forks from, not + // off itself. A preview deriving the hash from the (absent) branch would + // name a different directory than the one created, so `wt issue` would + // confirm one path and produce another. + // "_" is a legal git branch name whose slug is empty, so it is the case + // that actually consults the commit hash. + for branch in ["feat/7-add-login", "_"] { + let repo = TestRepo::init(); + let ws = workspace(&repo); + let fresh = Repo::discover(repo.root()).unwrap(); + let previewed = preview_target(&ws.parts(&fresh), branch, None).unwrap(); + let created = ws + .create(&RealGit, &RealHookRunner, &create_opts(branch)) + .unwrap(); + assert_eq!( + previewed, created.path, + "preview and creation disagree for {branch:?}" + ); + } + } + + #[test] + fn a_slugless_branch_is_named_after_the_base_it_forks_from() { + // Rule 5 of the slug contract: an empty slug falls back to the short hash + // of the *base ref*, not of the branch (which does not exist yet). Pinned + // separately from the preview test, which can only prove the two agree — + // not that the rule they share is the right one. + let repo = TestRepo::init(); + let ws = workspace(&repo); + let base = Repo::discover(repo.root()) + .ok() + .and_then(|r| resolve_hex(r.gix(), "main")) + .expect("main resolves"); + let short = &base[..7]; + + let created = ws + .create(&RealGit, &RealHookRunner, &create_opts("_")) + .unwrap(); + let name = created + .path + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_default(); + assert!( + name.ends_with(short), + "{name} should end with the base short hash {short}" + ); + } + #[test] fn create_with_explicit_base_records_it() { let repo = TestRepo::init();