From 998a722eda69b90c5313d003e8e44590a7a00adf Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 20 Aug 2026 14:21:00 -0300 Subject: [PATCH 01/10] feat(kw): gate build start on kw binary and tree readiness This commit makes StartBuild refuse before touching the tree when kw is missing, the kw env cannot be resolved, or the tree is not ready. The version floor stays advisory. A refused start leaves the actor idle and able to accept a later job. This commit is part of the kw integration's step 5. Signed-off-by: lorenzoberts --- src/kw/actor.rs | 191 ++++++++++++++++++++++++++++++++++++++++++++--- src/kw/errors.rs | 8 +- 2 files changed, 188 insertions(+), 11 deletions(-) diff --git a/src/kw/actor.rs b/src/kw/actor.rs index 3907f70..661c274 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -31,7 +31,7 @@ use crate::{ handle::KwHandle, history::KwHistoryStore, messages::{KwMessage, StartRequest}, - readiness::{self, KwReadiness}, + readiness::{self, KwReadiness, KwVersionCheck, TreeReadiness}, status::{KwJobKind, KwJobStatus, KwPhase, KwStatusSnapshot}, }, }; @@ -241,15 +241,49 @@ impl KwActor { /// caller right after this returns: the job itself keeps running in a /// detached task and is observed via the status snapshot. /// - /// The argv is `kw build --alert=n`. The `branch` on the Running status - /// is the requested branch; this skeleton does not switch the tree. + /// Refusals, in order: a job already running, no kw binary on PATH, + /// unresolvable kw-env state, or a tree that fails the readiness + /// probes. The argv is still the skeleton's minimal `kw build + /// --alert=n`; the real argv builder (reserved flags, extra-args merge) + /// and the checkout policy land later in the build step. The `branch` + /// carried by the Running status is the *requested* branch; the + /// checkout policy is what will make the tree actually sit on it. fn start_job(&mut self, kind: KwJobKind, request: StartRequest) -> Result<(), KwStartError> { if self.job.is_some() { return Err(KwStartError::JobAlreadyRunning); } - // Recorded at accept so RestorePreviousBranch can switch back. - // An unprobed HEAD (detached, or not a git repo) records nothing. + // Hard fail on invoke (integration plan §2.4): with no kw binary on + // PATH no job can run. The version check is advisory only — kw's + // shipped VERSION file is stale, so Below/Unknown are logged, never + // gated. + let kw_binary = readiness::probe_kw_binary(&*self.env, &*self.shell); + if !kw_binary.available { + return Err(KwStartError::KwBinaryMissing); + } + if let KwVersionCheck::Below(version_line) = &kw_binary.check { + tracing::warn!( + version = %version_line, + minimum = ?readiness::KW_MIN_VERSION, + "kw reports a version below the verified floor" + ); + } + + let tree_path = PathBuf::from(request.tree.path()); + // Unresolvable env state refuses the start: the build record this + // job writes at completion must know whether it ran under an O=. + let output_dir = readiness::resolve_output_dir(&*self.fs, &*self.env, &tree_path)?; + let tree_readiness = readiness::probe_tree(&*self.fs, &tree_path, output_dir.as_deref()); + if !matches!(tree_readiness, TreeReadiness::Ready { .. }) { + return Err(KwStartError::TreeNotReady(tree_readiness)); + } + + // Probed before anything touches the tree: once the checkout policy + // lands, `git switch ` goes between this probe and + // the spawn, and the probe must still capture the pre-job HEAD or + // RestorePreviousBranch would "restore" the branch the job switched + // to. An unprobed HEAD (detached, or not a git repo) records + // nothing rather than a wrong branch. let pre_job_branch = match self.head_branch(&request.tree) { branch if branch.is_empty() => None, branch => Some(branch), @@ -619,9 +653,26 @@ mod tests { ) } + /// fs answers for a ready kernel tree with no active kw env: the + /// kernel-root probes pass, `.config` exists, `.kw/env.current` is + /// absent, and `.kw/build.config` is unreadable (arch probes as None). + fn expect_ready_tree(fs: &mut MockFileSystemTrait) { + fs.expect_is_dir().returning(|_| true); + fs.expect_is_file() + .returning(|path| !path.ends_with(".kw/env.current")); + fs.expect_exists().returning(|_| true); + fs.expect_read_to_string().returning(|_| { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + }); + } + /// Spawns the actor with a real temp log dir and exposes the - /// [`FakeProcess`] so tests drive the "running" process. The shell mock - /// answers the pre-job HEAD probe. + /// [`FakeProcess`] so tests drive the "running" process. The env mock + /// has kw on PATH; the shell mock answers the kw version probe and the + /// pre-job HEAD probe. fn spawn_job_actor_with_fs( test_name: &str, fs: MockFileSystemTrait, @@ -629,19 +680,25 @@ mod tests { let process = Arc::new(FakeProcess::new()); let log_dir = tmp_log_dir(test_name); let mut shell = MockShellTrait::new(); - shell.expect_execute().returning(|_| { + shell.expect_execute().returning(|cmd| { + let stdout = match cmd.program.as_str() { + "kw" => b"kw, version 0.10.0\n".to_vec(), + _ => b"master\n".to_vec(), + }; Ok(ShellOutput { - stdout: b"master\n".to_vec(), + stdout, stderr: Vec::new(), success: true, }) }); + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); let handle = KwActor::spawn( Arc::new(MockKwHistoryStore::new()), process.clone(), Arc::new(shell), Arc::new(fs), - Arc::new(MockEnvTrait::new()), + Arc::new(env), log_dir.clone(), ); (handle, process, log_dir) @@ -649,6 +706,7 @@ mod tests { fn spawn_job_actor(test_name: &str) -> (KwHandle, Arc, PathBuf) { let mut fs = MockFileSystemTrait::new(); + expect_ready_tree(&mut fs); fs.expect_create_dir_all().returning(|_| Ok(())); spawn_job_actor_with_fs(test_name, fs) } @@ -961,6 +1019,118 @@ mod tests { std::fs::remove_dir_all(&log_dir).unwrap(); } + #[tokio::test] + async fn start_build_refused_when_kw_binary_missing() { + let process = Arc::new(FakeProcess::new()); + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| false); + let log_dir = tmp_log_dir("kw-missing"); + let handle = KwActor::spawn( + Arc::new(MockKwHistoryStore::new()), + process.clone(), + // No shell or fs calls are expected: the missing binary + // short-circuits the start before any other probe or spawn. + Arc::new(MockShellTrait::new()), + Arc::new(MockFileSystemTrait::new()), + Arc::new(env), + log_dir.clone(), + ); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::KwBinaryMissing)); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_refused_when_tree_not_ready() { + let process = Arc::new(FakeProcess::new()); + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + let mut shell = MockShellTrait::new(); + shell.expect_execute().returning(|_| { + Ok(ShellOutput { + stdout: b"kw, version 0.10.0\n".to_vec(), + stderr: Vec::new(), + success: true, + }) + }); + // The kernel-root probes pass, but there is no .kw directory: kw + // init was never run in this tree. + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir() + .returning(|path| path.file_name().is_none_or(|name| name != ".kw")); + fs.expect_is_file() + .returning(|path| !path.ends_with(".kw/env.current")); + fs.expect_exists().returning(|_| true); + let log_dir = tmp_log_dir("tree-not-ready"); + let handle = KwActor::spawn( + Arc::new(MockKwHistoryStore::new()), + process.clone(), + Arc::new(shell), + Arc::new(fs), + Arc::new(env), + log_dir.clone(), + ); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!( + err, + KwStartError::TreeNotReady(TreeReadiness::MissingKwDir) + )); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_allowed_when_kw_version_below_floor() { + let process = Arc::new(FakeProcess::new()); + let log_dir = tmp_log_dir("version-below"); + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + let mut shell = MockShellTrait::new(); + shell.expect_execute().returning(|cmd| { + let stdout = match cmd.program.as_str() { + // kw's shipped VERSION file is stale (`beta-0.9` even at + // the 0.10 tag): a below-floor report warns but never + // gates the start. + "kw" => b"kw, version beta-0.9\n".to_vec(), + _ => b"master\n".to_vec(), + }; + Ok(ShellOutput { + stdout, + stderr: Vec::new(), + success: true, + }) + }); + let mut fs = MockFileSystemTrait::new(); + expect_ready_tree(&mut fs); + fs.expect_create_dir_all().returning(|_| Ok(())); + let handle = KwActor::spawn( + Arc::new(MockKwHistoryStore::new()), + process.clone(), + Arc::new(shell), + Arc::new(fs), + Arc::new(env), + log_dir.clone(), + ); + + handle.start_build(start_request()).await.unwrap(); + assert_eq!(1, process.spawned().len()); + + process.last_child().finish(0); + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + #[tokio::test] async fn start_deploy_still_refused_until_the_deploy_step() { let handle = spawn_test_actor( @@ -1022,6 +1192,7 @@ mod tests { #[tokio::test] async fn log_dir_creation_failure_refuses_start_and_stays_idle() { let mut fs = MockFileSystemTrait::new(); + expect_ready_tree(&mut fs); fs.expect_create_dir_all().returning(|_| { Err(FileSystemError::IoError(io::Error::other( "read-only filesystem", diff --git a/src/kw/errors.rs b/src/kw/errors.rs index 30179b4..4dc5b98 100644 --- a/src/kw/errors.rs +++ b/src/kw/errors.rs @@ -7,7 +7,7 @@ use thiserror::Error; use crate::infrastructure::process::ProcessError; use crate::{ infrastructure::{file_system::FileSystemError, shell::ShellError}, - kw::readiness::KwReadinessError, + kw::readiness::{KwReadinessError, TreeReadiness}, }; #[derive(Debug, Error)] @@ -35,6 +35,12 @@ pub enum KwStartError { ActorUnavailable(String), #[error("a kw job is already running")] JobAlreadyRunning, + #[error("kw binary not found on PATH; install kw and make sure it is on PATH")] + KwBinaryMissing, + #[error("the kernel tree is not ready: {0}")] + TreeNotReady(TreeReadiness), + #[error("could not resolve the kw env state: {0}")] + Readiness(#[from] KwReadinessError), #[error("kw jobs are not supported yet")] NotImplemented, // Spawning a process is unix-only (ProcessTrait is cfg(unix)). From 70af98673f3bc5087eb0487f4932baf5ea6be40b Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 20 Aug 2026 14:38:55 -0300 Subject: [PATCH 02/10] feat(kw): switch the tree onto the requested branch before building This commit runs each build on the branch the caller asked for. Start refuses on a dirty or unverifiable worktree, switches, and leaves HEAD on that branch after the job. Restore context is recorded only on accept so a refused start cannot clobber a previous job's restore target. This commit is part of the kw integration's step 5. Signed-off-by: lorenzoberts --- src/kw/actor.rs | 333 ++++++++++++++++++++++++++++++++++++--------- src/kw/errors.rs | 6 + src/kw/messages.rs | 3 +- 3 files changed, 278 insertions(+), 64 deletions(-) diff --git a/src/kw/actor.rs b/src/kw/actor.rs index 661c274..21b8731 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -60,6 +60,18 @@ enum JobEvent { Finished(JobOutcome), } +/// Where RestorePreviousBranch switches back to: the branch HEAD was on +/// when the last job was accepted, and the tree that branch lives in. +/// Session-only (integration plan §2.1a), deliberately not persisted. +// Read once RestorePreviousBranch is implemented (the next unit of the +// build step); kept per the CachePolicy precedent +// (src/lore/application/cache.rs). +#[allow(dead_code)] +struct RestoreContext { + tree_path: String, + branch: String, +} + /// What the actor remembers about the running job while the detached task /// owns the process itself (see [`run_job`]). struct JobState { @@ -89,6 +101,9 @@ pub struct KwActor { process: Arc, kw_log_dir: PathBuf, job: Option, + /// Recorded only when a job is accepted: a refused start never + /// clobbers a previous job's restore target. + last_restore: Option, } impl KwActor { @@ -115,6 +130,7 @@ impl KwActor { process, kw_log_dir, job: None, + last_restore: None, } } @@ -242,12 +258,10 @@ impl KwActor { /// detached task and is observed via the status snapshot. /// /// Refusals, in order: a job already running, no kw binary on PATH, - /// unresolvable kw-env state, or a tree that fails the readiness - /// probes. The argv is still the skeleton's minimal `kw build - /// --alert=n`; the real argv builder (reserved flags, extra-args merge) - /// and the checkout policy land later in the build step. The `branch` - /// carried by the Running status is the *requested* branch; the - /// checkout policy is what will make the tree actually sit on it. + /// unresolvable kw-env state, a tree that fails the readiness probes, + /// a dirty worktree, or a failed branch switch. The argv is still the + /// skeleton's minimal `kw build --alert=n`; the real argv builder + /// (reserved flags, extra-args merge) lands later in the build step. fn start_job(&mut self, kind: KwJobKind, request: StartRequest) -> Result<(), KwStartError> { if self.job.is_some() { return Err(KwStartError::JobAlreadyRunning); @@ -278,17 +292,26 @@ impl KwActor { return Err(KwStartError::TreeNotReady(tree_readiness)); } - // Probed before anything touches the tree: once the checkout policy - // lands, `git switch ` goes between this probe and - // the spawn, and the probe must still capture the pre-job HEAD or - // RestorePreviousBranch would "restore" the branch the job switched - // to. An unprobed HEAD (detached, or not a git repo) records - // nothing rather than a wrong branch. + // Refuse on a dirty worktree before touching anything (§2.1a): a + // switch could otherwise carry unrelated changes into the build + // branch. + self.check_worktree_clean(&request.tree)?; + + // Probed before anything touches the tree: the `git switch` below + // goes between this probe and the spawn, and the probe must still + // capture the pre-job HEAD or RestorePreviousBranch would + // "restore" the branch the job switched to. An unprobed HEAD + // (detached, or not a git repo) records nothing rather than a + // wrong branch. let pre_job_branch = match self.head_branch(&request.tree) { branch if branch.is_empty() => None, branch => Some(branch), }; + // The checkout policy (§2.1a): the job runs on the requested + // branch, and HEAD stays there after the job. + self.switch_to_branch(&request.tree, &request.branch)?; + self.fs.create_dir_all(&self.kw_log_dir)?; // Millisecond suffix: two jobs started within the same second must // not share a log file — spawn truncates it. @@ -310,6 +333,14 @@ impl KwActor { log_path = %log_path.display(), "kw build job started" ); + // Recorded only on accept: a refused start — including one refused + // after the switch (log-dir creation, spawn) — never clobbers a + // previous job's restore target. An accepted job with an unprobed + // pre-job HEAD clears it: nothing honest is left to restore to. + self.last_restore = pre_job_branch.clone().map(|branch| RestoreContext { + tree_path: request.tree.path().to_string(), + branch, + }); self.job = Some(JobState { kind, phase, @@ -421,6 +452,45 @@ impl KwActor { )?) } + /// Refuses the start unless the tree's git state verifies clean + /// (§2.1a). A probe that itself fails — git missing, not a repository + /// — refuses too: starting a job on a tree whose state is unknown + /// could carry unrecorded changes into the build branch. + fn check_worktree_clean(&self, tree: &KernelTree) -> Result<(), KwStartError> { + let cmd = ShellCommand::new("git").args(["-C", tree.path(), "status", "--porcelain"]); + let output = self + .shell + .execute(&cmd) + .map_err(|error| KwStartError::GitStateProbe(error.to_string()))?; + if !output.success { + return Err(KwStartError::GitStateProbe( + String::from_utf8_lossy(&output.stderr).trim().to_string(), + )); + } + if !output.stdout.is_empty() { + return Err(KwStartError::DirtyWorktree); + } + Ok(()) + } + + /// Switches the tree onto the branch the job must run on (§2.1a); HEAD + /// stays there after the job. A failure refuses the start with git's + /// stderr, which names the usual causes (no such branch, a rebase or + /// merge in progress). + fn switch_to_branch(&self, tree: &KernelTree, branch: &str) -> Result<(), KwStartError> { + let cmd = ShellCommand::new("git").args(["-C", tree.path(), "switch", branch]); + let output = self + .shell + .execute(&cmd) + .map_err(|error| KwStartError::CheckoutFailed(error.to_string()))?; + if !output.success { + return Err(KwStartError::CheckoutFailed( + String::from_utf8_lossy(&output.stderr).trim().to_string(), + )); + } + Ok(()) + } + /// The tree's current branch, probed via git. An unresolvable HEAD /// (not a git repo, or a detached HEAD, for which `branch /// --show-current` prints nothing) yields an empty string: no build @@ -578,7 +648,10 @@ mod tests { use std::{ io, path::Path, - sync::atomic::{AtomicU64, Ordering}, + sync::{ + atomic::{AtomicU64, Ordering}, + Mutex, + }, time::Duration, }; @@ -587,7 +660,7 @@ mod tests { env::MockEnvTrait, file_system::{FileSystemError, MockFileSystemTrait}, process::FakeProcess, - shell::{MockShellTrait, ShellOutput}, + shell::{MockShellTrait, ShellCommand, ShellOutput}, }, kw::{ errors::KwStartError, @@ -653,6 +726,56 @@ mod tests { ) } + const KW_VERSION_OK: &[u8] = b"kw, version 0.10.0\n"; + /// A clean `git status --porcelain` answer. + const CLEAN_STATUS: (&[u8], &[u8], bool) = (b"", b"", true); + /// A successful `git switch` answer. + const SWITCH_OK: (&[u8], bool) = (b"", true); + + fn command_parts(cmd: &ShellCommand) -> Vec { + let mut parts = vec![cmd.program.clone()]; + parts.extend(cmd.args.clone()); + parts + } + + fn command(parts: &[&str]) -> Vec { + parts.iter().map(|part| part.to_string()).collect() + } + + /// A shell mock that logs every command's argv parts and answers by + /// content: kw's version probe with `kw_version`; `git status + /// --porcelain` with `status` (stdout, stderr, success); `git switch` + /// with `switch` (stderr, success); any other git call — the HEAD + /// branch probe — with `master`. + fn recording_shell( + kw_version: &'static [u8], + status: (&'static [u8], &'static [u8], bool), + switch: (&'static [u8], bool), + ) -> (MockShellTrait, Arc>>>) { + let calls = Arc::new(Mutex::new(Vec::new())); + let calls_in_shell = Arc::clone(&calls); + let mut shell = MockShellTrait::new(); + shell.expect_execute().returning(move |cmd| { + calls_in_shell.lock().unwrap().push(command_parts(cmd)); + let output = |stdout: &[u8], stderr: &[u8], success: bool| ShellOutput { + stdout: stdout.to_vec(), + stderr: stderr.to_vec(), + success, + }; + if cmd.program == "kw" { + return Ok(output(kw_version, b"", true)); + } + if cmd.args.iter().any(|arg| arg == "status") { + return Ok(output(status.0, status.1, status.2)); + } + if cmd.args.iter().any(|arg| arg == "switch") { + return Ok(output(b"", switch.0, switch.1)); + } + Ok(output(b"master\n", b"", true)) + }); + (shell, calls) + } + /// fs answers for a ready kernel tree with no active kw env: the /// kernel-root probes pass, `.config` exists, `.kw/env.current` is /// absent, and `.kw/build.config` is unreadable (arch probes as None). @@ -669,28 +792,24 @@ mod tests { }); } + /// A ready kernel tree whose log dir can be created. + fn ready_fs() -> MockFileSystemTrait { + let mut fs = MockFileSystemTrait::new(); + expect_ready_tree(&mut fs); + fs.expect_create_dir_all().returning(|_| Ok(())); + fs + } + /// Spawns the actor with a real temp log dir and exposes the /// [`FakeProcess`] so tests drive the "running" process. The env mock - /// has kw on PATH; the shell mock answers the kw version probe and the - /// pre-job HEAD probe. - fn spawn_job_actor_with_fs( + /// has kw on PATH. + fn spawn_job_actor_with_mocks( test_name: &str, + shell: MockShellTrait, fs: MockFileSystemTrait, ) -> (KwHandle, Arc, PathBuf) { let process = Arc::new(FakeProcess::new()); let log_dir = tmp_log_dir(test_name); - let mut shell = MockShellTrait::new(); - shell.expect_execute().returning(|cmd| { - let stdout = match cmd.program.as_str() { - "kw" => b"kw, version 0.10.0\n".to_vec(), - _ => b"master\n".to_vec(), - }; - Ok(ShellOutput { - stdout, - stderr: Vec::new(), - success: true, - }) - }); let mut env = MockEnvTrait::new(); env.expect_which().returning(|_| true); let handle = KwActor::spawn( @@ -705,10 +824,8 @@ mod tests { } fn spawn_job_actor(test_name: &str) -> (KwHandle, Arc, PathBuf) { - let mut fs = MockFileSystemTrait::new(); - expect_ready_tree(&mut fs); - fs.expect_create_dir_all().returning(|_| Ok(())); - spawn_job_actor_with_fs(test_name, fs) + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + spawn_job_actor_with_mocks(test_name, shell, ready_fs()) } fn apply_record() -> KwApplyRecord { @@ -1092,41 +1209,130 @@ mod tests { #[tokio::test] async fn start_build_allowed_when_kw_version_below_floor() { - let process = Arc::new(FakeProcess::new()); - let log_dir = tmp_log_dir("version-below"); - let mut env = MockEnvTrait::new(); - env.expect_which().returning(|_| true); - let mut shell = MockShellTrait::new(); - shell.expect_execute().returning(|cmd| { - let stdout = match cmd.program.as_str() { - // kw's shipped VERSION file is stale (`beta-0.9` even at - // the 0.10 tag): a below-floor report warns but never - // gates the start. - "kw" => b"kw, version beta-0.9\n".to_vec(), - _ => b"master\n".to_vec(), + // kw's shipped VERSION file is stale (`beta-0.9` even at the 0.10 + // tag): a below-floor report warns but never gates the start. + let (shell, _calls) = recording_shell(b"kw, version beta-0.9\n", CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("version-below", shell, ready_fs()); + + handle.start_build(start_request()).await.unwrap(); + assert_eq!(1, process.spawned().len()); + + process.last_child().finish(0); + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_switches_to_requested_branch_before_spawning() { + let (shell, calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("checkout-order", shell, ready_fs()); + + handle.start_build(start_request()).await.unwrap(); + + { + let calls = calls.lock().unwrap(); + let git_position = |subcommand: &str| { + calls + .iter() + .position(|call| { + call.first().map(String::as_str) == Some("git") + && call.iter().any(|part| part == subcommand) + }) + .expect("expected git call missing") }; - Ok(ShellOutput { - stdout, - stderr: Vec::new(), - success: true, - }) - }); + let status = git_position("status"); + let head = git_position("--show-current"); + let switch = git_position("switch"); + assert!( + status < head && head < switch, + "checkout policy must probe dirty state, then HEAD, then switch: {calls:?}" + ); + assert_eq!( + &calls[switch], + &command(&[ + "git", + "-C", + "/home/user/linux", + "switch", + "patchset-2026-08-01-17-30-00" + ]) + ); + } + assert_eq!(1, process.spawned().len()); + + process.last_child().finish(0); + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_refused_when_worktree_is_dirty() { + let (shell, calls) = + recording_shell(KW_VERSION_OK, (b" M src/main.c\n", b"", true), SWITCH_OK); let mut fs = MockFileSystemTrait::new(); expect_ready_tree(&mut fs); - fs.expect_create_dir_all().returning(|_| Ok(())); - let handle = KwActor::spawn( - Arc::new(MockKwHistoryStore::new()), - process.clone(), - Arc::new(shell), - Arc::new(fs), - Arc::new(env), - log_dir.clone(), + let (handle, process, log_dir) = spawn_job_actor_with_mocks("dirty", shell, fs); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::DirtyWorktree)); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); + // The refusal happens before any branch mutation. + assert!(!calls + .lock() + .unwrap() + .iter() + .any(|call| call.iter().any(|part| part == "switch"))); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_refused_when_git_state_is_unverifiable() { + let (shell, _calls) = recording_shell( + KW_VERSION_OK, + (b"", b"fatal: not a git repository\n", false), + SWITCH_OK, ); + let mut fs = MockFileSystemTrait::new(); + expect_ready_tree(&mut fs); + let (handle, process, log_dir) = spawn_job_actor_with_mocks("git-probe-fail", shell, fs); - handle.start_build(start_request()).await.unwrap(); - assert_eq!(1, process.spawned().len()); + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::GitStateProbe(_))); + assert!(err.to_string().contains("not a git repository")); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn start_build_refused_when_branch_switch_fails() { + let (shell, _calls) = recording_shell( + KW_VERSION_OK, + CLEAN_STATUS, + ( + b"error: pathspec 'no-such-branch' did not match any file(s) known to git\n", + false, + ), + ); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("switch-fail", shell, ready_fs()); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::CheckoutFailed(_))); + assert!(err.to_string().contains("did not match")); + assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); + assert!(process.spawned().is_empty()); - process.last_child().finish(0); handle.shutdown().await; std::fs::remove_dir_all(&log_dir).unwrap(); } @@ -1191,6 +1397,7 @@ mod tests { #[tokio::test] async fn log_dir_creation_failure_refuses_start_and_stays_idle() { + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); let mut fs = MockFileSystemTrait::new(); expect_ready_tree(&mut fs); fs.expect_create_dir_all().returning(|_| { @@ -1198,7 +1405,7 @@ mod tests { "read-only filesystem", ))) }); - let (handle, process, log_dir) = spawn_job_actor_with_fs("log-dir-fail", fs); + let (handle, process, log_dir) = spawn_job_actor_with_mocks("log-dir-fail", shell, fs); let err = handle.start_build(start_request()).await.unwrap_err(); diff --git a/src/kw/errors.rs b/src/kw/errors.rs index 4dc5b98..bd19a07 100644 --- a/src/kw/errors.rs +++ b/src/kw/errors.rs @@ -41,6 +41,12 @@ pub enum KwStartError { TreeNotReady(TreeReadiness), #[error("could not resolve the kw env state: {0}")] Readiness(#[from] KwReadinessError), + #[error("the kernel tree has staged and/or unstaged changes; commit or stash them first")] + DirtyWorktree, + #[error("could not verify the kernel tree's git state: {0}")] + GitStateProbe(String), + #[error("failed to switch the kernel tree to the requested branch: {0}")] + CheckoutFailed(String), #[error("kw jobs are not supported yet")] NotImplemented, // Spawning a process is unix-only (ProcessTrait is cfg(unix)). diff --git a/src/kw/messages.rs b/src/kw/messages.rs index 067b6da..cff2d66 100644 --- a/src/kw/messages.rs +++ b/src/kw/messages.rs @@ -20,7 +20,8 @@ use crate::{ pub struct StartRequest { pub kernel_tree_id: String, pub tree: KernelTree, - /// Branch the job must run on. + /// Branch the job must run on; the actor switches the tree onto it + /// before spawning and leaves HEAD there after the job. pub branch: String, } From e40e7390d87983df835cb6ba69a48eee77528ebb Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 20 Aug 2026 15:10:29 -0300 Subject: [PATCH 03/10] feat(kw): implement restore-previous-branch This commit lets the user switch the tree back to the branch it was on when the last job was accepted. Restore refuses while a job runs, when nothing was recorded, or when the worktree is dirty, and only a successful switch consumes the context so a failed attempt can be retried. This commit is part of the kw integration's step 5. Signed-off-by: lorenzoberts --- src/kw/actor.rs | 286 +++++++++++++++++++++++++++++++++++++++++------ src/kw/errors.rs | 37 ++++++ 2 files changed, 289 insertions(+), 34 deletions(-) diff --git a/src/kw/actor.rs b/src/kw/actor.rs index 21b8731..7d38cae 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -27,7 +27,7 @@ use crate::{ shell::{ShellCommand, ShellTrait}, }, kw::{ - errors::{KwError, KwStartError}, + errors::{KwError, KwStartError, TreeGitError}, handle::KwHandle, history::KwHistoryStore, messages::{KwMessage, StartRequest}, @@ -63,10 +63,6 @@ enum JobEvent { /// Where RestorePreviousBranch switches back to: the branch HEAD was on /// when the last job was accepted, and the tree that branch lives in. /// Session-only (integration plan §2.1a), deliberately not persisted. -// Read once RestorePreviousBranch is implemented (the next unit of the -// build step); kept per the CachePolicy precedent -// (src/lore/application/cache.rs). -#[allow(dead_code)] struct RestoreContext { tree_path: String, branch: String, @@ -80,10 +76,6 @@ struct JobState { kernel_tree_id: String, branch: String, log_path: PathBuf, - /// HEAD at accept time, so RestorePreviousBranch can offer to switch - /// back after the job. Read once the checkout policy lands. - #[allow(dead_code)] - pre_job_branch: Option, /// `None` once a cancel has been requested; a second `Cancel` is an /// idempotent ack. cancel_tx: Option>, @@ -219,10 +211,8 @@ impl KwActor { ); ControlFlow::Continue(()) } - // Restoring needs the dirty-worktree check and `git switch`, - // which arrive with the checkout policy in the build step. KwMessage::RestorePreviousBranch { reply } => { - send_kw_reply(message_name, reply, Err(KwError::NoRecordedBranch)); + send_kw_reply(message_name, reply, self.restore_previous_branch()); ControlFlow::Continue(()) } KwMessage::Shutdown { reply } => { @@ -295,7 +285,7 @@ impl KwActor { // Refuse on a dirty worktree before touching anything (§2.1a): a // switch could otherwise carry unrelated changes into the build // branch. - self.check_worktree_clean(&request.tree)?; + self.check_worktree_clean(request.tree.path())?; // Probed before anything touches the tree: the `git switch` below // goes between this probe and the spawn, and the probe must still @@ -310,7 +300,7 @@ impl KwActor { // The checkout policy (§2.1a): the job runs on the requested // branch, and HEAD stays there after the job. - self.switch_to_branch(&request.tree, &request.branch)?; + self.switch_to_branch(request.tree.path(), &request.branch)?; self.fs.create_dir_all(&self.kw_log_dir)?; // Millisecond suffix: two jobs started within the same second must @@ -337,7 +327,7 @@ impl KwActor { // after the switch (log-dir creation, spawn) — never clobbers a // previous job's restore target. An accepted job with an unprobed // pre-job HEAD clears it: nothing honest is left to restore to. - self.last_restore = pre_job_branch.clone().map(|branch| RestoreContext { + self.last_restore = pre_job_branch.map(|branch| RestoreContext { tree_path: request.tree.path().to_string(), branch, }); @@ -347,7 +337,6 @@ impl KwActor { kernel_tree_id: request.kernel_tree_id.clone(), branch: request.branch.clone(), log_path: log_path.clone(), - pre_job_branch, cancel_tx: Some(cancel_tx), }); self.set_status(KwJobStatus::Running { @@ -452,45 +441,75 @@ impl KwActor { )?) } - /// Refuses the start unless the tree's git state verifies clean - /// (§2.1a). A probe that itself fails — git missing, not a repository - /// — refuses too: starting a job on a tree whose state is unknown - /// could carry unrecorded changes into the build branch. - fn check_worktree_clean(&self, tree: &KernelTree) -> Result<(), KwStartError> { - let cmd = ShellCommand::new("git").args(["-C", tree.path(), "status", "--porcelain"]); + /// Fails unless the tree's git state verifies clean (§2.1a). A probe + /// that itself fails — git missing, not a repository — fails too: + /// starting a job or restoring a branch on a tree whose state is + /// unknown could carry unrecorded changes across branches. + fn check_worktree_clean(&self, tree_path: &str) -> Result<(), TreeGitError> { + let cmd = ShellCommand::new("git").args(["-C", tree_path, "status", "--porcelain"]); let output = self .shell .execute(&cmd) - .map_err(|error| KwStartError::GitStateProbe(error.to_string()))?; + .map_err(|error| TreeGitError::Probe(error.to_string()))?; if !output.success { - return Err(KwStartError::GitStateProbe( + return Err(TreeGitError::Probe( String::from_utf8_lossy(&output.stderr).trim().to_string(), )); } if !output.stdout.is_empty() { - return Err(KwStartError::DirtyWorktree); + return Err(TreeGitError::DirtyWorktree); } Ok(()) } - /// Switches the tree onto the branch the job must run on (§2.1a); HEAD - /// stays there after the job. A failure refuses the start with git's - /// stderr, which names the usual causes (no such branch, a rebase or - /// merge in progress). - fn switch_to_branch(&self, tree: &KernelTree, branch: &str) -> Result<(), KwStartError> { - let cmd = ShellCommand::new("git").args(["-C", tree.path(), "switch", branch]); + /// Switches the tree onto `branch`. A failure carries git's stderr, + /// which names the usual causes (no such branch, a rebase or merge in + /// progress). + fn switch_to_branch(&self, tree_path: &str, branch: &str) -> Result<(), TreeGitError> { + let cmd = ShellCommand::new("git").args(["-C", tree_path, "switch", branch]); let output = self .shell .execute(&cmd) - .map_err(|error| KwStartError::CheckoutFailed(error.to_string()))?; + .map_err(|error| TreeGitError::Switch(error.to_string()))?; if !output.success { - return Err(KwStartError::CheckoutFailed( + return Err(TreeGitError::Switch( String::from_utf8_lossy(&output.stderr).trim().to_string(), )); } Ok(()) } + /// Switches the tree that ran the last job back to the branch HEAD was + /// on when that job was accepted (§2.1a). Refuses while a job is + /// running (its branch is in use), when nothing was recorded, and on + /// a dirty worktree. Only a successful switch consumes the context — + /// a refused restore stays available for a retry. + fn restore_previous_branch(&mut self) -> Result<(), KwError> { + if self.job.is_some() { + return Err(KwError::JobRunning); + } + let Some(restore) = self.last_restore.take() else { + return Err(KwError::NoRecordedBranch); + }; + match self + .check_worktree_clean(&restore.tree_path) + .and_then(|()| self.switch_to_branch(&restore.tree_path, &restore.branch)) + { + Ok(()) => { + tracing::info!( + tree = restore.tree_path, + branch = restore.branch, + "restored pre-job branch" + ); + Ok(()) + } + Err(error) => { + self.last_restore = Some(restore); + Err(error.into()) + } + } + } + /// The tree's current branch, probed via git. An unresolvable HEAD /// (not a git repo, or a detached HEAD, for which `branch /// --show-current` prints nothing) yields an empty string: no build @@ -649,7 +668,7 @@ mod tests { io, path::Path, sync::{ - atomic::{AtomicU64, Ordering}, + atomic::{AtomicBool, AtomicU64, Ordering}, Mutex, }, time::Duration, @@ -776,6 +795,79 @@ mod tests { (shell, calls) } + /// A stateful shell double for the restore tests: kw's version probe + /// answers 0.10.0, `git status --porcelain` reflects the dirty flag, + /// the HEAD probe reports `head` (with the trailing newline git + /// prints), and a successful `git switch` updates `head` — mirroring + /// a real worktree the actor switches between branches. + struct GitStub { + head: Arc>, + dirty: Arc, + fail_switches: Arc, + } + + impl GitStub { + fn on_branch(branch: &str) -> Self { + Self { + head: Arc::new(Mutex::new(branch.to_string())), + dirty: Arc::new(AtomicBool::new(false)), + fail_switches: Arc::new(AtomicBool::new(false)), + } + } + + fn set_dirty(&self, dirty: bool) { + self.dirty.store(dirty, Ordering::Relaxed); + } + + fn set_fail_switches(&self, fail: bool) { + self.fail_switches.store(fail, Ordering::Relaxed); + } + + fn head(&self) -> String { + self.head.lock().unwrap().clone() + } + + fn shell(&self) -> MockShellTrait { + let head = Arc::clone(&self.head); + let dirty = Arc::clone(&self.dirty); + let fail_switches = Arc::clone(&self.fail_switches); + let mut shell = MockShellTrait::new(); + shell.expect_execute().returning(move |cmd| { + let output = |stdout: &[u8]| ShellOutput { + stdout: stdout.to_vec(), + stderr: Vec::new(), + success: true, + }; + if cmd.program == "kw" { + return Ok(output(KW_VERSION_OK)); + } + if cmd.args.iter().any(|arg| arg == "status") { + let stdout: &[u8] = if dirty.load(Ordering::Relaxed) { + b" M src/main.c\n" + } else { + b"" + }; + return Ok(output(stdout)); + } + if cmd.args.iter().any(|arg| arg == "switch") { + if fail_switches.load(Ordering::Relaxed) { + return Ok(ShellOutput { + stdout: Vec::new(), + stderr: b"error: you need to resolve your current index first\n" + .to_vec(), + success: false, + }); + } + *head.lock().unwrap() = cmd.args.last().unwrap().clone(); + return Ok(output(b"")); + } + let current = format!("{}\n", head.lock().unwrap()); + Ok(output(current.as_bytes())) + }); + shell + } + } + /// fs answers for a ready kernel tree with no active kw env: the /// kernel-root probes pass, `.config` exists, `.kw/env.current` is /// absent, and `.kw/build.config` is unreadable (arch probes as None). @@ -1379,6 +1471,132 @@ mod tests { handle.shutdown().await; } + #[tokio::test] + async fn restore_switches_back_to_pre_job_branch_and_is_consumed() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore", git.shell(), ready_fs()); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + // The checkout policy left HEAD on the build branch. + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + assert!(matches!(status, KwJobStatus::Succeeded { .. })); + + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + // A successful restore consumes the context: a second restore has + // nothing to do. + let err = handle.restore_previous_branch().await.unwrap_err(); + assert!(matches!(err, KwError::NoRecordedBranch)); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn restore_refused_while_job_is_running() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore-running", git.shell(), ready_fs()); + + handle.start_build(start_request()).await.unwrap(); + let err = handle.restore_previous_branch().await.unwrap_err(); + assert!(matches!(err, KwError::JobRunning)); + + // The context survives the refusal: restore works once the job + // ends. + process.last_child().finish(0); + let mut watch = handle.watch_status().await.unwrap(); + let _ = wait_for_terminal_status(&mut watch).await; + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn restore_refused_when_worktree_is_dirty() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore-dirty", git.shell(), ready_fs()); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let _ = wait_for_terminal_status(&mut watch).await; + + git.set_dirty(true); + let err = handle.restore_previous_branch().await.unwrap_err(); + assert!(matches!(err, KwError::DirtyWorktree)); + // The refused restore did not touch the tree. + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); + + // The context survives: clean the tree and retry. + git.set_dirty(false); + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn restore_failure_keeps_the_context_for_a_retry() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore-fail", git.shell(), ready_fs()); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let _ = wait_for_terminal_status(&mut watch).await; + + git.set_fail_switches(true); + let err = handle.restore_previous_branch().await.unwrap_err(); + assert!(matches!(err, KwError::CheckoutFailed(_))); + assert!(err.to_string().contains("resolve your current index")); + + git.set_fail_switches(false); + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn refused_start_does_not_clobber_the_restore_context() { + let git = GitStub::on_branch("master"); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("restore-clobber", git.shell(), ready_fs()); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let _ = wait_for_terminal_status(&mut watch).await; + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); + + // This start is refused at spawn — after its HEAD probe and + // switch — and must not overwrite the recorded restore target. + process.refuse_spawns(true); + let mut second = start_request(); + second.branch = "patchset-two".to_string(); + let err = handle.start_build(second).await.unwrap_err(); + assert!(matches!(err, KwStartError::Spawn(_))); + assert_eq!(git.head(), "patchset-two"); + + handle.restore_previous_branch().await.unwrap(); + assert_eq!(git.head(), "master"); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + #[tokio::test] async fn shutdown_stops_actor() { let handle = spawn_test_actor( diff --git a/src/kw/errors.rs b/src/kw/errors.rs index bd19a07..1b6f434 100644 --- a/src/kw/errors.rs +++ b/src/kw/errors.rs @@ -18,6 +18,14 @@ pub enum KwError { NoJobRunning, #[error("no pre-job branch was recorded")] NoRecordedBranch, + #[error("a kw job is running; restore the branch after it finishes")] + JobRunning, + #[error("the kernel tree has staged and/or unstaged changes; commit or stash them first")] + DirtyWorktree, + #[error("could not verify the kernel tree's git state: {0}")] + GitStateProbe(String), + #[error("failed to switch the kernel tree back to the previous branch: {0}")] + CheckoutFailed(String), #[error("history error: {0}")] History(#[from] FileSystemError), #[error("readiness error: {0}")] @@ -56,3 +64,32 @@ pub enum KwStartError { #[error("filesystem error: {0}")] Fs(#[from] FileSystemError), } + +/// Shared git-state refusal for the start and restore paths, converted +/// into the public error each path reports. +#[derive(Debug)] +pub(crate) enum TreeGitError { + DirtyWorktree, + Probe(String), + Switch(String), +} + +impl From for KwStartError { + fn from(error: TreeGitError) -> Self { + match error { + TreeGitError::DirtyWorktree => KwStartError::DirtyWorktree, + TreeGitError::Probe(detail) => KwStartError::GitStateProbe(detail), + TreeGitError::Switch(detail) => KwStartError::CheckoutFailed(detail), + } + } +} + +impl From for KwError { + fn from(error: TreeGitError) -> Self { + match error { + TreeGitError::DirtyWorktree => KwError::DirtyWorktree, + TreeGitError::Probe(detail) => KwError::GitStateProbe(detail), + TreeGitError::Switch(detail) => KwError::CheckoutFailed(detail), + } + } +} From ad911b1c3087c7bd50068e84df81058ad3bdca29 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 20 Aug 2026 15:16:15 -0300 Subject: [PATCH 04/10] feat(kw): merge kw build argv with reserved flags This commit replaces the skeleton's fixed argv with a reserved-flag merge: patch-hub always wins on --alert and --save-log-to, and user extras cannot override them. --save-log-to is reserved but not passed, because ProcessTrait is already the sole log writer. This commit is part of the kw integration's step 5. Signed-off-by: lorenzoberts --- src/kw/actor.rs | 40 +++++++++-- src/kw/argv.rs | 176 +++++++++++++++++++++++++++++++++++++++++++++ src/kw/messages.rs | 5 ++ src/kw/mod.rs | 1 + 4 files changed, 218 insertions(+), 4 deletions(-) create mode 100644 src/kw/argv.rs diff --git a/src/kw/actor.rs b/src/kw/actor.rs index 7d38cae..17ade57 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -27,6 +27,7 @@ use crate::{ shell::{ShellCommand, ShellTrait}, }, kw::{ + argv, errors::{KwError, KwStartError, TreeGitError}, handle::KwHandle, history::KwHistoryStore, @@ -249,9 +250,9 @@ impl KwActor { /// /// Refusals, in order: a job already running, no kw binary on PATH, /// unresolvable kw-env state, a tree that fails the readiness probes, - /// a dirty worktree, or a failed branch switch. The argv is still the - /// skeleton's minimal `kw build --alert=n`; the real argv builder - /// (reserved flags, extra-args merge) lands later in the build step. + /// a dirty worktree, or a failed branch switch. The argv comes from + /// the reserved-flags merge (§2.1f): patch-hub's own flags win over + /// the request's extra args. fn start_job(&mut self, kind: KwJobKind, request: StartRequest) -> Result<(), KwStartError> { if self.job.is_some() { return Err(KwStartError::JobAlreadyRunning); @@ -309,7 +310,7 @@ impl KwActor { "build-{}.log", chrono::Utc::now().format("%Y%m%d-%H%M%S-%3f") )); - let cmd = ShellCommand::new("kw").args(["build", "--alert=n"]); + let cmd = ShellCommand::new("kw").args(argv::build_argv(&request.extra_args)); let cwd = PathBuf::from(request.tree.path()); let process = self.process.spawn(&cmd, &cwd, &log_path)?; @@ -720,6 +721,7 @@ mod tests { kernel_tree_id: "mainline".to_string(), tree: kernel_tree(Path::new("/home/user/linux")), branch: "patchset-2026-08-01-17-30-00".to_string(), + extra_args: Vec::new(), } } @@ -1128,6 +1130,36 @@ mod tests { std::fs::remove_dir_all(&log_dir).unwrap(); } + #[tokio::test] + async fn start_build_merges_extra_args_into_the_spawned_argv() { + let (handle, process, log_dir) = spawn_job_actor("extra-args"); + + let mut request = start_request(); + request.extra_args = [ + "--verbose", + "--alert=vv", + "--save-log-to", + "/tmp/x.log", + "--ccache", + ] + .into_iter() + .map(String::from) + .collect(); + handle.start_build(request).await.unwrap(); + + let spawned = process.spawned(); + // Reserved options win: the user's --alert and --save-log-to are + // stripped, the rest passes through in order. + assert_eq!( + ["build", "--alert=n", "--verbose", "--ccache"].as_slice(), + spawned[0].args.as_slice() + ); + + process.last_child().finish(0); + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + #[tokio::test] async fn second_start_while_running_is_refused() { let (handle, process, log_dir) = spawn_job_actor("busy"); diff --git a/src/kw/argv.rs b/src/kw/argv.rs new file mode 100644 index 0000000..aaedb53 --- /dev/null +++ b/src/kw/argv.rs @@ -0,0 +1,176 @@ +//! kw argv construction: patch-hub's base command lines plus the user +//! extra-args merge in which reserved options always win (integration +//! plan §2.1f). + +// The only caller is the unix-only actor until the KwOps screen shows +// the final argv before Start (§2.1f item 6). +#![cfg_attr(not(unix), allow(dead_code))] + +/// A CLI option patch-hub controls: user-supplied extra args that set it +/// are stripped, so the occurrence on patch-hub's own base argv wins. +pub struct ReservedOption { + /// Every spelling of the option, e.g. `&["--force", "-f"]`. + spellings: &'static [&'static str], + /// Whether the option takes a value (`--name=value`, or `--name + /// value` as a separate token). + takes_value: bool, +} + +impl ReservedOption { + pub const fn new(spellings: &'static [&'static str], takes_value: bool) -> Self { + Self { + spellings, + takes_value, + } + } +} + +/// Reserved for `kw build`: patch-hub forces non-interactive alerts, and +/// owns the job's log file (ProcessTrait captures kw's stdout/stderr to +/// it) — a user-supplied `--save-log-to` would fork the log to a second +/// file, leaving KwOps tailing only half the output. +const BUILD_RESERVED: &[ReservedOption] = &[ + ReservedOption::new(&["--alert"], true), + ReservedOption::new(&["--save-log-to"], true), +]; + +/// The argv for a build job: `kw build --alert=n `. +pub fn build_argv(extra_args: &[String]) -> Vec { + merge_extra_args(&["build", "--alert=n"], BUILD_RESERVED, extra_args) +} + +/// Appends user-supplied extra args to `base`, stripping every token that +/// would override a reserved option: `--name=value` is stripped whole, +/// `--name value` consumes the following token too, and a boolean +/// reserved option strips only itself. All other extras pass through in +/// order. +pub fn merge_extra_args( + base: &[&str], + reserved: &[ReservedOption], + extra_args: &[String], +) -> Vec { + let mut argv: Vec = base.iter().map(|arg| arg.to_string()).collect(); + let mut extras = extra_args.iter(); + while let Some(token) = extras.next() { + match reserved_option_for(reserved, token) { + Some(option) => { + if option.takes_value && !token.contains('=') { + // `--name value`: the separate value token goes too. + extras.next(); + } + } + None => argv.push(token.clone()), + } + } + argv +} + +/// Finds the reserved option a token sets, if any: an exact spelling +/// match, or `--name=value` for a long spelling. The `=` boundary keeps +/// `--alertness` from matching `--alert`. +fn reserved_option_for<'a>( + reserved: &'a [ReservedOption], + token: &str, +) -> Option<&'a ReservedOption> { + reserved.iter().find(|option| { + option.spellings.iter().any(|spelling| { + token == *spelling + || spelling.starts_with("--") + && token + .strip_prefix(spelling) + .is_some_and(|rest| rest.starts_with('=')) + }) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn extras(tokens: &[&str]) -> Vec { + tokens.iter().map(|token| token.to_string()).collect() + } + + #[test] + fn build_argv_without_extras_is_the_base_command() { + assert_eq!(vec!["build", "--alert=n"], build_argv(&[])); + } + + #[test] + fn extras_are_appended_in_order() { + assert_eq!( + vec!["build", "--alert=n", "--verbose", "--ccache", "-j8"], + build_argv(&extras(&["--verbose", "--ccache", "-j8"])) + ); + } + + #[test] + fn reserved_alert_wins_over_user_value_forms() { + assert_eq!( + vec!["build", "--alert=n", "--verbose"], + build_argv(&extras(&["--alert=vv", "--verbose"])) + ); + // Separate-token value form: the value token is consumed too. + assert_eq!( + vec!["build", "--alert=n", "--verbose"], + build_argv(&extras(&["--alert", "v", "--verbose"])) + ); + } + + #[test] + fn save_log_to_is_reserved_in_both_value_forms() { + assert_eq!( + vec!["build", "--alert=n"], + build_argv(&extras(&["--save-log-to=/tmp/x.log"])) + ); + assert_eq!( + vec!["build", "--alert=n"], + build_argv(&extras(&["--save-log-to", "/tmp/x.log"])) + ); + } + + #[test] + fn similar_prefix_is_not_reserved() { + // `--alertness` only shares a prefix with `--alert`. + assert_eq!( + vec!["build", "--alert=n", "--alertness"], + build_argv(&extras(&["--alertness"])) + ); + } + + #[test] + fn trailing_reserved_option_without_value_is_stripped() { + assert_eq!( + vec!["build", "--alert=n"], + build_argv(&extras(&["--alert"])) + ); + } + + #[test] + fn boolean_reserved_strips_only_itself_in_all_spellings() { + // The deploy step's reserved set has booleans with short forms; + // the merge must not eat the token after one. + let reserved = [ReservedOption::new(&["--force", "-f"], false)]; + assert_eq!( + vec!["deploy", "extra"], + merge_extra_args(&["deploy"], &reserved, &extras(&["--force", "extra"])) + ); + assert_eq!( + vec!["deploy", "extra"], + merge_extra_args(&["deploy"], &reserved, &extras(&["-f", "extra"])) + ); + } + + #[test] + fn tokens_after_a_consumed_value_keep_flowing() { + let reserved = [ReservedOption::new(&["--remote"], true)]; + assert_eq!( + vec!["deploy", "--no-reboot"], + merge_extra_args( + &["deploy"], + &reserved, + &extras(&["--remote", "host:22", "--no-reboot"]) + ) + ); + } +} diff --git a/src/kw/messages.rs b/src/kw/messages.rs index cff2d66..a444ad5 100644 --- a/src/kw/messages.rs +++ b/src/kw/messages.rs @@ -23,6 +23,11 @@ pub struct StartRequest { /// Branch the job must run on; the actor switches the tree onto it /// before spawning and leaves HEAD there after the job. pub branch: String, + /// Extra kw CLI tokens, already whitespace-split by the caller. + /// Reserved options (integration plan §2.1f: `--alert`, + /// `--save-log-to`, and the deploy-time set) are stripped from them — + /// patch-hub's own argv wins. + pub extra_args: Vec, } pub enum KwMessage { diff --git a/src/kw/mod.rs b/src/kw/mod.rs index 24f19ea..d5f0da2 100644 --- a/src/kw/mod.rs +++ b/src/kw/mod.rs @@ -3,6 +3,7 @@ #[cfg(unix)] pub mod actor; +pub mod argv; pub mod errors; pub mod handle; pub mod history; From 4f67dfc7a64fa4a97553334ac7f5d1f578fe5723 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 20 Aug 2026 15:19:19 -0300 Subject: [PATCH 05/10] feat(kw): look up apply records by branch This commit finds the patchset that produced the branch a build runs on by scanning apply history for that tree, newest timestamp winning. The link does not depend on the caller threading a message id, so manually typed branches and cross-session rebuilds still resolve. This commit is part of the kw integration's step 5. Signed-off-by: lorenzoberts --- src/kw/history.rs | 165 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 165 insertions(+) diff --git a/src/kw/history.rs b/src/kw/history.rs index ed95037..227751d 100644 --- a/src/kw/history.rs +++ b/src/kw/history.rs @@ -80,6 +80,20 @@ pub trait KwHistoryStore: Send + Sync { kernel_tree_id: &str, ) -> Result, FileSystemError>; + /// Returns the newest apply record for the tree whose applied branch + /// is `branch` — the link from a build's branch back to the patchset + /// it came from. A missing history file is a normal state, not an + /// error. Records with unparseable `applied_at` values sort oldest, + /// same convention as the build records. + // Read by KwActor when writing build records (the build step); kept + // per the CachePolicy precedent (src/lore/application/cache.rs). + #[allow(dead_code)] + fn apply_record_for_branch( + &self, + kernel_tree_id: &str, + branch: &str, + ) -> Result, FileSystemError>; + /// Inserts or replaces the build record for the record's /// `(kernel_tree_id, branch)` pair. #[allow(dead_code)] @@ -194,6 +208,25 @@ impl KwHistoryStore for FileKwHistoryStore { .map_err(|e| self.error_with_path(&self.apply_history_path, e)) } + fn apply_record_for_branch( + &self, + kernel_tree_id: &str, + branch: &str, + ) -> Result, FileSystemError> { + self.load_records(&self.apply_history_path) + .map(|records: ApplyRecords| { + records + .values() + .filter_map(|by_tree| by_tree.get(kernel_tree_id)) + .filter(|record| record.applied_branch == branch) + .max_by_key(|record| { + chrono::DateTime::parse_from_rfc3339(&record.applied_at).ok() + }) + .cloned() + }) + .map_err(|e| self.error_with_path(&self.apply_history_path, e)) + } + fn record_build(&self, record: KwBuildRecord) -> Result<(), FileSystemError> { self.store_build_record(record) .map_err(|e| self.error_with_path(&self.build_history_path, e)) @@ -284,6 +317,18 @@ mod tests { } } + fn record_at( + message_id: &str, + kernel_tree_id: &str, + branch: &str, + applied_at: &str, + ) -> KwApplyRecord { + KwApplyRecord { + applied_at: applied_at.to_string(), + ..record(message_id, kernel_tree_id, branch) + } + } + fn build(kernel_tree_id: &str, branch: &str, built_at: &str) -> KwBuildRecord { KwBuildRecord { kernel_tree_id: kernel_tree_id.to_string(), @@ -379,6 +424,126 @@ mod tests { fs::remove_dir_all(&dir).unwrap(); } + #[test] + fn apply_record_for_branch_finds_record_across_message_ids() { + let dir = tmp_dir("branch-lookup"); + let store = store_at(&dir); + + store + .record_apply(record("msg-1", "mainline", "patchset-x")) + .unwrap(); + store + .record_apply(record("msg-2", "mainline", "patchset-y")) + .unwrap(); + + assert_eq!( + Some(record("msg-2", "mainline", "patchset-y")), + store + .apply_record_for_branch("mainline", "patchset-y") + .unwrap() + ); + assert_eq!( + None, + store + .apply_record_for_branch("mainline", "never-applied") + .unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn apply_record_for_branch_scopes_to_the_tree() { + let dir = tmp_dir("branch-tree-scope"); + let store = store_at(&dir); + + // The same branch name applied to two trees resolves per tree. + store + .record_apply(record("msg-1", "mainline", "patchset-x")) + .unwrap(); + store + .record_apply(record("msg-2", "stable", "patchset-x")) + .unwrap(); + + assert_eq!( + Some(record("msg-2", "stable", "patchset-x")), + store + .apply_record_for_branch("stable", "patchset-x") + .unwrap() + ); + assert_eq!( + None, + store + .apply_record_for_branch("amd-gfx", "patchset-x") + .unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn apply_record_for_branch_returns_newest_reapply() { + let dir = tmp_dir("branch-newest"); + let store = store_at(&dir); + + // Two patchsets applied onto the same branch name: the newest + // applied_at wins, and unparseable timestamps sort oldest (the + // build records' convention). + store + .record_apply(record_at( + "msg-old", + "mainline", + "patchset-x", + "2026-08-01T10:00:00Z", + )) + .unwrap(); + store + .record_apply(record_at( + "msg-new", + "mainline", + "patchset-x", + "2026-08-02T10:00:00Z", + )) + .unwrap(); + store + .record_apply(record_at( + "msg-broken", + "mainline", + "patchset-x", + "not a timestamp", + )) + .unwrap(); + + assert_eq!( + Some(record_at( + "msg-new", + "mainline", + "patchset-x", + "2026-08-02T10:00:00Z" + )), + store + .apply_record_for_branch("mainline", "patchset-x") + .unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn apply_record_for_branch_missing_history_reads_as_none() { + let dir = tmp_dir("branch-missing"); + let store = store_at(&dir); + + assert_eq!( + None, + store + .apply_record_for_branch("mainline", "patchset-x") + .unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn record_creates_parent_directories() { let dir = tmp_dir("parents"); From 688219a3ce0e2a08fbf8d9de295b34e0a5a548e3 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 20 Aug 2026 15:34:28 -0300 Subject: [PATCH 06/10] feat(kw): record build history when a build job finishes This commit writes a build record on success and failure so deploy-alone readiness has durable evidence of what the job produced. Cancelled jobs write nothing; an unresolvable env at completion skips the record rather than keying a false no-env match; a write error is logged without changing the job's terminal status. This commit is part of the kw integration's step 5. Signed-off-by: lorenzoberts --- src/infrastructure/process/fake.rs | 31 +- src/kw/actor.rs | 471 ++++++++++++++++++++++++++++- src/kw/readiness.rs | 49 +++ 3 files changed, 532 insertions(+), 19 deletions(-) diff --git a/src/infrastructure/process/fake.rs b/src/infrastructure/process/fake.rs index 1cb84eb..aab609b 100644 --- a/src/infrastructure/process/fake.rs +++ b/src/infrastructure/process/fake.rs @@ -46,6 +46,10 @@ struct FakeState { /// "running": the model for a process group that ignores SIGTERM, so /// tests can exercise the SIGKILL escalation. ignores_sigterm: bool, + /// When set, `wait()` resolves with an IO error once the process + /// finishes: the model for a process whose exit status is lost (an + /// already-reaped child, an OS-level wait failure). + fails_wait: bool, } impl FakeControl { @@ -90,6 +94,7 @@ pub struct FakeProcess { spawns: Mutex>, refuse_spawns: AtomicBool, ignore_sigterm: AtomicBool, + fail_waits: AtomicBool, } impl FakeProcess { @@ -110,6 +115,12 @@ impl FakeProcess { self.ignore_sigterm.store(ignore, Ordering::Relaxed); } + /// Make subsequently spawned processes' `wait()` resolve with an IO + /// error once they finish, instead of their exit status. + pub fn fail_waits(&self, fail: bool) { + self.fail_waits.store(fail, Ordering::Relaxed); + } + pub fn spawned(&self) -> Vec { self.spawns .lock() @@ -153,6 +164,7 @@ impl ProcessTrait for FakeProcess { killed: false, force_killed: false, ignores_sigterm: self.ignore_sigterm.load(Ordering::Relaxed), + fails_wait: self.fail_waits.load(Ordering::Relaxed), }), notify: Notify::new(), log_path: log_path.to_path_buf(), @@ -179,11 +191,20 @@ struct FakeRunningProcess { impl RunningProcess for FakeRunningProcess { async fn wait(&mut self) -> Result { loop { - // The state lock is released before the await; a finish()/kill() - // racing the check is not lost because Notify stores one permit. - let raw_status = self.control.state.lock().unwrap().raw_status; - if let Some(raw) = raw_status { - return Ok(ExitStatus::from_raw(raw)); + // Only Copy data escapes the guard scope, so the (non-Send) + // guard is never held across the await; a finish()/kill() + // racing the check is not lost because Notify stores one + // permit. + let finished = { + let state = self.control.state.lock().unwrap(); + state.raw_status.map(|raw| (raw, state.fails_wait)) + }; + if let Some((raw, fails_wait)) = finished { + return if fails_wait { + Err(ProcessError::IoError(io::Error::other("fake wait failure"))) + } else { + Ok(ExitStatus::from_raw(raw)) + }; } self.control.notify.notified().await; } diff --git a/src/kw/actor.rs b/src/kw/actor.rs index 17ade57..3920217 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -8,10 +8,21 @@ //! [`crate::kw::readiness`] and records applies through the shared //! [`KwHistoryStore`](crate::kw::history::KwHistoryStore). //! -//! `create_dir_all`, the HEAD-probe `git` call, and history writes run -//! inline on the actor task. - -use std::{ops::ControlFlow, path::PathBuf, process::ExitStatus, sync::Arc, time::Duration}; +//! Tracked note (same class as the integration plan's §2.1i): the actor +//! runs quick blocking calls inline in its async task — `create_dir_all`, +//! the git probes of the checkout policy, the build-record completion +//! probes, and the history store's atomic writes — per the ConfigActor +//! precedent. They are all milliseconds-scale; if a real stall ever shows +//! up while a long job runs, they should move behind `spawn_blocking` +//! like the other actors' heavy work. + +use std::{ + ops::ControlFlow, + path::{Path, PathBuf}, + process::ExitStatus, + sync::Arc, + time::Duration, +}; use tokio::{ spawn, @@ -30,7 +41,7 @@ use crate::{ argv, errors::{KwError, KwStartError, TreeGitError}, handle::KwHandle, - history::KwHistoryStore, + history::{KwBuildRecord, KwHistoryStore}, messages::{KwMessage, StartRequest}, readiness::{self, KwReadiness, KwVersionCheck, TreeReadiness}, status::{KwJobKind, KwJobStatus, KwPhase, KwStatusSnapshot}, @@ -76,6 +87,10 @@ struct JobState { phase: KwPhase, kernel_tree_id: String, branch: String, + /// Snapshot of the tree path at accept time: the build record carries + /// it so later readiness checks can detect the tree being repointed + /// (§2.1e). + tree_path: String, log_path: PathBuf, /// `None` once a cancel has been requested; a second `Cancel` is an /// idempotent ack. @@ -337,6 +352,7 @@ impl KwActor { phase, kernel_tree_id: request.kernel_tree_id.clone(), branch: request.branch.clone(), + tree_path: request.tree.path().to_string(), log_path: log_path.clone(), cancel_tx: Some(cancel_tx), }); @@ -369,6 +385,10 @@ impl KwActor { tracing::warn!("kw job finished with no job state recorded"); return; }; + // The record is written before the status flips: watchers + // that react to the terminal status find the history + // already durable. + self.record_build_outcome(&job, &outcome); let status = match outcome { JobOutcome::Exited(exit) if exit.success() => { tracing::info!( @@ -419,6 +439,87 @@ impl KwActor { } } + /// Writes the build record for a finished job (§2.1e): success and + /// failure both — KwOps shows "last build failed" from the stored + /// record, and deploy-alone readiness requires `success == true`. A + /// cancelled job writes nothing: it never completed. History and + /// patchset-link errors are logged, never reported in the job's + /// status — the build's real outcome already reached the user. + /// + /// Build-then-deploy jobs (the deploy step) must instead write this + /// record at the Building → Deploying phase transition, so a failed + /// deploy cannot mask a good build. + fn record_build_outcome(&self, job: &JobState, outcome: &JobOutcome) { + let success = match outcome { + JobOutcome::Exited(exit) => exit.success(), + // The exit status is lost: record an honest failure rather + // than guess, so deploy-alone readiness cannot trust it. + JobOutcome::WaitFailed(_) => false, + JobOutcome::Cancelled => return, + }; + + let tree_path = Path::new(&job.tree_path); + // If the env state became unresolvable mid-build, the record + // would be keyed with a wrong `output_dir: None` — a Frankenstein + // match for a later no-env deploy-alone probe. No record fails + // safe. + let output_dir = match readiness::resolve_output_dir(&*self.fs, &*self.env, tree_path) { + Ok(output_dir) => output_dir, + Err(error) => { + tracing::warn!( + %error, + branch = job.branch, + "skipping the build record: kw env state unresolvable" + ); + return; + } + }; + let arch = readiness::read_build_arch(&*self.fs, tree_path); + // A failed build may have left a stale image from an earlier + // successful one behind; only successes record what they + // produced. + let (image_path, kernelrelease) = if success { + let build_root = output_dir.as_deref().unwrap_or(tree_path); + ( + readiness::find_newest_kernel_image(&*self.fs, build_root, arch.as_deref()), + readiness::read_kernelrelease(&*self.fs, build_root), + ) + } else { + (None, None) + }; + let message_id = match self + .history + .apply_record_for_branch(&job.kernel_tree_id, &job.branch) + { + Ok(record) => record.map(|record| record.message_id), + Err(error) => { + tracing::warn!( + %error, + branch = job.branch, + "build record loses its patchset link" + ); + None + } + }; + + let record = KwBuildRecord { + kernel_tree_id: job.kernel_tree_id.clone(), + tree_path: job.tree_path.clone(), + message_id, + branch: job.branch.clone(), + arch, + image_path: image_path.map(|path| path.to_string_lossy().into_owned()), + output_dir: output_dir.map(|path| path.to_string_lossy().into_owned()), + kernelrelease, + log_path: job.log_path.to_string_lossy().into_owned(), + built_at: chrono::Utc::now().to_rfc3339(), + success, + }; + if let Err(error) = self.history.record_build(record) { + tracing::warn!(%error, branch = job.branch, "failed to record kw build history"); + } + } + fn set_status(&mut self, job: KwJobStatus) { // send_replace, not send: no receiver (nobody called WatchStatus // yet) is a normal state, not an error. @@ -684,7 +785,7 @@ mod tests { }, kw::{ errors::KwStartError, - history::{KwApplyRecord, MockKwHistoryStore}, + history::{KwApplyRecord, KwBuildRecord, MockKwHistoryStore}, messages::StartRequest, readiness::{DeployAloneRefusal, TreeReadiness}, status::{KwJobKind, KwJobStatus, KwPhase}, @@ -872,7 +973,8 @@ mod tests { /// fs answers for a ready kernel tree with no active kw env: the /// kernel-root probes pass, `.config` exists, `.kw/env.current` is - /// absent, and `.kw/build.config` is unreadable (arch probes as None). + /// absent, `.kw/build.config` is unreadable (arch probes as None), + /// and there is no arch/ dir to glob images from. fn expect_ready_tree(fs: &mut MockFileSystemTrait) { fs.expect_is_dir().returning(|_| true); fs.expect_is_file() @@ -884,6 +986,12 @@ mod tests { "missing", ))) }); + fs.expect_read_dir().returning(|_| { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + }); } /// A ready kernel tree whose log dir can be created. @@ -894,20 +1002,90 @@ mod tests { fs } - /// Spawns the actor with a real temp log dir and exposes the - /// [`FakeProcess`] so tests drive the "running" process. The env mock - /// has kw on PATH. - fn spawn_job_actor_with_mocks( + /// A ready kernel tree whose build produced an image and a + /// kernelrelease: build.config sets `arch=x86`, `arch/x86/boot/` + /// holds a bzImage, and `include/config/kernel.release` exists. The + /// image's metadata is unreadable, so its mtime falls back to the + /// epoch — still the only, hence newest, candidate. + fn built_tree_fs() -> MockFileSystemTrait { + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir().returning(|_| true); + fs.expect_is_file() + .returning(|path| !path.ends_with(".kw/env.current")); + fs.expect_exists().returning(|_| true); + fs.expect_read_to_string().returning(|path| { + if path.ends_with("build.config") { + Ok("arch=x86\n".to_string()) + } else if path.ends_with("kernel.release") { + Ok("6.17.0\n".to_string()) + } else { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + } + }); + fs.expect_read_dir().returning(|path| { + if path.ends_with("arch/x86/boot") { + Ok(vec![PathBuf::from( + "/home/user/linux/arch/x86/boot/bzImage", + )]) + } else { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + } + }); + fs.expect_metadata() + .returning(|_| Err(FileSystemError::IoError(io::Error::other("no metadata")))); + fs.expect_create_dir_all().returning(|_| Ok(())); + fs + } + + /// History answers for an actor whose jobs complete: no patchset + /// link, build-record writes accepted and dropped. + fn quiet_history() -> MockKwHistoryStore { + let mut history = MockKwHistoryStore::new(); + history + .expect_apply_record_for_branch() + .returning(|_, _| Ok(None)); + history.expect_record_build().returning(|_| Ok(())); + history + } + + /// A history double that captures written build records and answers + /// the patchset-link lookup with `apply_record`. + fn recording_history( + apply_record: Option, + ) -> (MockKwHistoryStore, Arc>>) { + let builds = Arc::new(Mutex::new(Vec::new())); + let builds_in_store = Arc::clone(&builds); + let mut history = MockKwHistoryStore::new(); + history + .expect_apply_record_for_branch() + .returning(move |_, _| Ok(apply_record.clone())); + history.expect_record_build().returning(move |record| { + builds_in_store.lock().unwrap().push(record); + Ok(()) + }); + (history, builds) + } + + /// Spawns the actor with every dependency explicit, a real temp log + /// dir, and the [`FakeProcess`] exposed so tests drive the "running" + /// process. + fn spawn_full_actor( test_name: &str, + history: MockKwHistoryStore, shell: MockShellTrait, fs: MockFileSystemTrait, + env: MockEnvTrait, ) -> (KwHandle, Arc, PathBuf) { let process = Arc::new(FakeProcess::new()); let log_dir = tmp_log_dir(test_name); - let mut env = MockEnvTrait::new(); - env.expect_which().returning(|_| true); let handle = KwActor::spawn( - Arc::new(MockKwHistoryStore::new()), + Arc::new(history), process.clone(), Arc::new(shell), Arc::new(fs), @@ -917,6 +1095,19 @@ mod tests { (handle, process, log_dir) } + /// Spawns the actor with a real temp log dir and exposes the + /// [`FakeProcess`] so tests drive the "running" process. The env mock + /// has kw on PATH. + fn spawn_job_actor_with_mocks( + test_name: &str, + shell: MockShellTrait, + fs: MockFileSystemTrait, + ) -> (KwHandle, Arc, PathBuf) { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + spawn_full_actor(test_name, quiet_history(), shell, fs, env) + } + fn spawn_job_actor(test_name: &str) -> (KwHandle, Arc, PathBuf) { let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); spawn_job_actor_with_mocks(test_name, shell, ready_fs()) @@ -1744,4 +1935,256 @@ mod tests { handle.shutdown().await; std::fs::remove_dir_all(&log_dir).unwrap(); } + + #[tokio::test] + async fn successful_build_writes_a_full_build_record() { + let (history, builds) = recording_history(Some(apply_record())); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("build-record", history, shell, built_tree_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + assert!(matches!(status, KwJobStatus::Succeeded { .. })); + + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + let record = &builds[0]; + assert_eq!("mainline", record.kernel_tree_id); + assert_eq!("/home/user/linux", record.tree_path); + assert_eq!(Some("msg-1"), record.message_id.as_deref()); + assert_eq!("patchset-2026-08-01-17-30-00", record.branch); + assert_eq!(Some("x86"), record.arch.as_deref()); + assert_eq!( + Some("/home/user/linux/arch/x86/boot/bzImage"), + record.image_path.as_deref() + ); + assert_eq!(None, record.output_dir); + assert_eq!(Some("6.17.0"), record.kernelrelease.as_deref()); + assert!(record.log_path.starts_with(log_dir.to_str().unwrap())); + assert!(record.success); + // The readiness latest-lookup parses built_at as RFC3339. + assert!(chrono::DateTime::parse_from_rfc3339(&record.built_at).is_ok()); + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn failed_build_writes_a_failure_record() { + let (history, builds) = recording_history(Some(apply_record())); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("failed-record", history, shell, built_tree_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(2); + let status = wait_for_terminal_status(&mut watch).await; + assert!(matches!( + status, + KwJobStatus::Failed { + exit_code: Some(2), + .. + } + )); + + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + let record = &builds[0]; + assert!(!record.success); + // Config/env facts are still recorded; what the build never + // produced is not — a stale image from an earlier build must + // not leak into a failure record. + assert_eq!(Some("x86"), record.arch.as_deref()); + assert_eq!(None, record.image_path); + assert_eq!(None, record.kernelrelease); + assert_eq!(Some("msg-1"), record.message_id.as_deref()); + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn cancelled_build_writes_no_record() { + let (history, builds) = recording_history(None); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, _process, log_dir) = + spawn_full_actor("cancel-record", history, shell, ready_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + handle.cancel().await.unwrap(); + let status = wait_for_terminal_status(&mut watch).await; + + assert!(matches!(status, KwJobStatus::Cancelled { .. })); + assert!(builds.lock().unwrap().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn lost_exit_status_records_a_failed_build() { + let (history, builds) = recording_history(None); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("wait-failure", history, shell, ready_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + process.fail_waits(true); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + + // A lost exit status is an honest failure: the record must not + // become deploy-alone evidence. + assert!(matches!( + status, + KwJobStatus::Failed { + exit_code: None, + .. + } + )); + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + assert!(!builds[0].success); + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn build_record_write_failure_keeps_the_terminal_status() { + let mut history = MockKwHistoryStore::new(); + history + .expect_apply_record_for_branch() + .returning(|_, _| Ok(None)); + history + .expect_record_build() + .returning(|_| Err(FileSystemError::IoError(io::Error::other("disk full")))); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("record-write-fails", history, shell, ready_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + + // The build's real outcome reached the user; a history-write + // failure must not turn it into a reported failure. + assert!(matches!(status, KwJobStatus::Succeeded { .. })); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn build_record_keeps_no_patchset_link_when_apply_lookup_fails() { + let builds = Arc::new(Mutex::new(Vec::new())); + let builds_in_store = Arc::clone(&builds); + let mut history = MockKwHistoryStore::new(); + history.expect_apply_record_for_branch().returning(|_, _| { + Err(FileSystemError::IoError(io::Error::other( + "corrupt history", + ))) + }); + history.expect_record_build().returning(move |record| { + builds_in_store.lock().unwrap().push(record); + Ok(()) + }); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = + spawn_full_actor("link-lookup-fails", history, shell, ready_fs(), { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env + }); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + process.last_child().finish(0); + let _ = wait_for_terminal_status(&mut watch).await; + + // The lookup error must not drop the record, only the link. + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + assert_eq!(None, builds[0].message_id); + assert!(builds[0].success); + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn build_record_skipped_when_env_state_breaks_mid_build() { + // An env that resolves at Start but not at completion must not + // produce a record keyed with a wrong `output_dir: None`. + let env_broken = Arc::new(AtomicBool::new(false)); + let env_broken_in_fs = Arc::clone(&env_broken); + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir().returning(|_| true); + fs.expect_is_file().returning(|_| true); + fs.expect_exists().returning(|_| true); + fs.expect_read_to_string().returning(move |path| { + if path.ends_with("env.current") && !env_broken_in_fs.load(Ordering::Relaxed) { + Ok("testenv\n".to_string()) + } else { + Err(FileSystemError::IoError(io::Error::other("unreadable"))) + } + }); + fs.expect_create_dir_all().returning(|_| Ok(())); + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env.expect_var() + .returning(|_| Ok("/home/user/.cache".to_string())); + let (history, builds) = recording_history(None); + let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let (handle, process, log_dir) = spawn_full_actor("env-breaks", history, shell, fs, env); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_build(start_request()).await.unwrap(); + // Break the env state after the start probes but before the + // completion probes run. + env_broken.store(true, Ordering::Relaxed); + process.last_child().finish(0); + let status = wait_for_terminal_status(&mut watch).await; + + assert!(matches!(status, KwJobStatus::Succeeded { .. })); + assert!(builds.lock().unwrap().is_empty()); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } } diff --git a/src/kw/readiness.rs b/src/kw/readiness.rs index 28f3d46..d6c2d54 100644 --- a/src/kw/readiness.rs +++ b/src/kw/readiness.rs @@ -285,6 +285,30 @@ fn image_mtime(fs: &dyn FileSystemTrait, path: &Path) -> SystemTime { .unwrap_or(SystemTime::UNIX_EPOCH) } +/// Reads the built kernel's release string from +/// `/include/config/kernel.release`, the file a kernel build +/// generates — cheaper than re-running `make kernelrelease`, and `None` +/// when the build never produced one (or produced an empty one). +// Read by KwActor when writing build records (the build step); kept per +// the CachePolicy precedent (src/lore/application/cache.rs). +#[allow(dead_code)] +pub fn read_kernelrelease(fs: &dyn FileSystemTrait, build_root: &Path) -> Option { + let release = fs + .read_to_string( + &build_root + .join("include") + .join("config") + .join("kernel.release"), + ) + .ok()?; + let release = release.trim(); + if release.is_empty() { + None + } else { + Some(release.to_string()) + } +} + /// Minimum kw version this integration is verified against. pub const KW_MIN_VERSION: (u32, u32) = (0, 10); @@ -1039,6 +1063,31 @@ last_line_without_newline=yes"; ); } + #[test] + fn kernelrelease_reads_and_trims_the_release_file() { + let dir = make_ready_tree("kernelrelease"); + let config_dir = dir.path().join("include").join("config"); + fs::create_dir_all(&config_dir).unwrap(); + fs::write(config_dir.join("kernel.release"), "6.17.0-rc1\n").unwrap(); + + assert_eq!( + Some("6.17.0-rc1".to_string()), + read_kernelrelease(&OsFileSystem, dir.path()) + ); + } + + #[test] + fn kernelrelease_is_none_without_a_release_file() { + let dir = make_ready_tree("kernelrelease-missing"); + + assert_eq!(None, read_kernelrelease(&OsFileSystem, dir.path())); + + let config_dir = dir.path().join("include").join("config"); + fs::create_dir_all(&config_dir).unwrap(); + fs::write(config_dir.join("kernel.release"), "\n").unwrap(); + assert_eq!(None, read_kernelrelease(&OsFileSystem, dir.path())); + } + fn shell_output(stdout: &str, success: bool) -> ShellOutput { ShellOutput { stdout: stdout.as_bytes().to_vec(), From 403e55436a37c1c085a9e522aeaefd7fed8678a7 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 20 Aug 2026 15:43:13 -0300 Subject: [PATCH 07/10] feat(app): block patchset apply while a kw job is running This commit refuses git am while a kw job owns the tree's branch state. The user sees a blocked popup and the apply toggle resets; if the actor cannot be reached, apply proceeds because an unreachable actor cannot be running a job. This commit is part of the kw integration's step 5. Signed-off-by: lorenzoberts --- src/app/integration_tests/patchset_actions.rs | 148 +++++++++++++++++- src/app/mod.rs | 130 +++++++++------ 2 files changed, 223 insertions(+), 55 deletions(-) diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs index 72b0d78..df2cbdb 100644 --- a/src/app/integration_tests/patchset_actions.rs +++ b/src/app/integration_tests/patchset_actions.rs @@ -22,7 +22,7 @@ use crate::{ process::FakeProcess, shell::{MockShellTrait, ShellCommand, ShellOutput}, }, - kw::{actor::KwActor, history::MockKwHistoryStore}, + kw::{actor::KwActor, history::MockKwHistoryStore, messages::StartRequest}, lore::application::{ cache::BootstrapLoreData, handle::LoreApiHandle, messages::LoreApiMessage, }, @@ -251,6 +251,41 @@ async fn apply_failure_does_not_record_history() { shutdown_kw(&app).await; } +#[tokio::test] +async fn apply_is_blocked_while_a_kw_job_runs() { + // No scripted outputs: any apply-time git call fails the test. + let (shell, calls) = shell_with_outputs(vec![]); + let log_dir = kw_log_dir("apply-blocked"); + let mut app = app_with_details_and_kw( + clean_fs(), + shell, + lore_handle_with_persistence(), + apply_details_state(), + apply_config(), + history_store_allowing_writes(), + kw_actor_shell(), + kw_actor_fs(), + kw_actor_env(), + log_dir.clone(), + ); + + let kw = app.services.kw.as_ref().expect("tests run on unix"); + kw.start_build(kw_start_request()).await.unwrap(); + + app.consolidate_patchset_actions().await.unwrap(); + + assert_apply_action(&app, false); + assert_info_popup_contains( + app.state.popup.as_ref(), + "Patchset Apply Blocked", + &["kw job is running", "Wait for the job to finish"], + ); + assert!(calls.lock().unwrap().is_empty()); + + shutdown_kw(&app).await; + std::fs::remove_dir_all(&log_dir).unwrap(); +} + #[tokio::test] async fn reviewed_reply_success_records_persists_and_resets_reply_action() { let saved_reviewed = Arc::new(Mutex::new(None)); @@ -351,16 +386,45 @@ fn app_with_details( details: PatchsetDetailsState, config: ConfigSnapshot, kw_history: MockKwHistoryStore, +) -> App { + app_with_details_and_kw( + fs, + shell, + lore_api, + details, + config, + kw_history, + MockShellTrait::new(), + MockFileSystemTrait::new(), + MockEnvTrait::new(), + PathBuf::from("/tmp/patch-hub-test-kw-logs"), + ) +} + +/// Variant of [`app_with_details`] whose kw actor gets its own mocks and +/// log dir, for tests that start jobs through the app's kw handle. +#[allow(clippy::too_many_arguments)] +fn app_with_details_and_kw( + fs: MockFileSystemTrait, + shell: MockShellTrait, + lore_api: LoreApiHandle, + details: PatchsetDetailsState, + config: ConfigSnapshot, + kw_history: MockKwHistoryStore, + kw_shell: MockShellTrait, + kw_fs: MockFileSystemTrait, + kw_env: MockEnvTrait, + kw_log_dir: PathBuf, ) -> App { // Apply history is recorded through the real actor wrapping the mock // store, mirroring production wiring. let kw = KwActor::spawn( Arc::new(kw_history), Arc::new(FakeProcess::new()), - Arc::new(MockShellTrait::new()), - Arc::new(MockFileSystemTrait::new()), - Arc::new(MockEnvTrait::new()), - PathBuf::from("/tmp/patch-hub-test-kw-logs"), + Arc::new(kw_shell), + Arc::new(kw_fs), + Arc::new(kw_env), + kw_log_dir, ); let mut app = App::new( config, @@ -523,6 +587,80 @@ fn command_parts(cmd: &ShellCommand) -> Vec { parts } +/// kw-actor mocks for tests that start jobs through the app's handle: +/// kw on PATH at the verified version, clean git state, a ready kernel +/// tree — the happy path through the actor's start probes. +fn kw_actor_shell() -> MockShellTrait { + let mut shell = MockShellTrait::new(); + shell.expect_execute().returning(|cmd| { + let stdout = match cmd.program.as_str() { + "kw" => b"kw, version 0.10.0\n".to_vec(), + _ if cmd.args.iter().any(|arg| arg == "status") => Vec::new(), + _ => b"main\n".to_vec(), + }; + Ok(ShellOutput { + stdout, + stderr: Vec::new(), + success: true, + }) + }); + shell +} + +fn kw_actor_fs() -> MockFileSystemTrait { + let mut fs = MockFileSystemTrait::new(); + fs.expect_is_dir().returning(|_| true); + fs.expect_is_file() + .returning(|path| !path.ends_with(".kw/env.current")); + fs.expect_exists().returning(|_| true); + fs.expect_read_to_string().returning(|_| { + Err(FileSystemError::IoError(std::io::Error::new( + std::io::ErrorKind::NotFound, + "missing", + ))) + }); + fs.expect_read_dir().returning(|_| { + Err(FileSystemError::IoError(std::io::Error::new( + std::io::ErrorKind::NotFound, + "missing", + ))) + }); + fs.expect_create_dir_all().returning(|_| Ok(())); + fs +} + +fn kw_actor_env() -> MockEnvTrait { + let mut env = MockEnvTrait::new(); + env.expect_which().returning(|_| true); + env +} + +/// A real, unique directory: FakeProcess creates the job's log file on +/// spawn, even though the fs trait is mocked. +fn kw_log_dir(test_name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "patch-hub-app-kw-logs-{}-{}", + test_name, + std::process::id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +fn kw_start_request() -> StartRequest { + StartRequest { + kernel_tree_id: "linux".to_string(), + tree: serde_json::from_value(serde_json::json!({ + "path": KERNEL_TREE_PATH, + "branch": BASE_BRANCH + })) + .expect("kernel tree should deserialize"), + branch: "patchset-2026-08-20-15-00-00".to_string(), + extra_args: Vec::new(), + } +} + fn command(parts: &[&str]) -> Vec { parts.iter().map(|part| part.to_string()).collect() } diff --git a/src/app/mod.rs b/src/app/mod.rs index c10819c..72d623e 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -50,6 +50,7 @@ use crate::{ kw::{ handle::KwHandle, history::{KwApplyRecord, KwHistoryStore}, + status::KwJobStatus, }, lore::{ application::{ @@ -427,56 +428,13 @@ impl App { if patchset_action_selected(details, &PatchsetAction::Apply) { debug!("applying patchset via git-am"); - let request = apply_patchset_request(details); - let action_service = PatchsetActionService::new( - &*self.services.fs, - &*self.services.shell, - &self.services.lore_api, - ); - let popup = match action_service.apply_patchset(&request, &self.state.config) { - Ok(applied) => { - let popup_body = match kw_apply_record(details, &self.state.config, &applied) { - // Defensive: the apply itself resolved this tree from - // the same snapshot, so this is unreachable unless the - // config changed mid-apply. - None => { - warn!("kw apply history skipped: target kernel tree is no longer configured"); - format!( - "{}\n\nWarning: the apply was not recorded in the kw history: target kernel tree is no longer configured", - applied.message - ) - } - Some(record) => { - // History writes go through KwActor so apply - // recording serializes with job state; without an - // actor (non-unix), write the store directly. - let recorded = match &self.services.kw { - Some(kw) => { - kw.record_apply(record).await.map_err(|e| e.to_string()) - } - None => self - .services - .kw_history - .record_apply(record) - .map_err(|e| e.to_string()), - }; - // The git apply itself succeeded; a history-write - // failure must not turn it into a reported failure. - match recorded { - Ok(()) => applied.message, - Err(e) => { - warn!(error = %e, "failed to record kw apply history"); - format!( - "{}\n\nWarning: the apply was not recorded in the kw history: {e}\nIf this warning keeps appearing, inspect or delete that file.", - applied.message - ) - } - } - } - }; - popup::AppPopup::info("Patchset Apply Success", popup_body) - } - Err(msg) => popup::AppPopup::info("Patchset Apply Fail", msg), + // A running kw job owns the tree: applying would rewrite the + // branch the job is building under it (integration plan + // §2.1i). No Start can race this check: both paths are + // serialized by the AppActor loop. + let popup = match self.kw_job_running_popup().await { + Some(popup) => popup, + None => self.apply_patchset_popup(details).await, }; self.state.popup = Some(popup); @@ -490,6 +448,78 @@ impl App { } } + /// The popup blocking an apply while a kw job runs, if a job is in + /// fact running. An unreachable actor cannot be running a job, so a + /// status-query failure lets the apply proceed. + async fn kw_job_running_popup(&self) -> Option { + let kw = self.services.kw.as_ref()?; + match kw.get_status().await { + Ok(snapshot) if matches!(snapshot.job, KwJobStatus::Running { .. }) => { + Some(popup::AppPopup::info( + "Patchset Apply Blocked", + " A kw job is running on the kernel tree.\n\nApplying a patchset now would rewrite the branch the job is building under it.\n\nWait for the job to finish, then apply again.", + )) + } + _ => None, + } + } + + /// Runs the git-am apply and maps the outcome to the result popup, + /// recording the apply in the kw history on success. + async fn apply_patchset_popup(&self, details: &PatchsetDetailsState) -> popup::AppPopup { + let request = apply_patchset_request(details); + let action_service = PatchsetActionService::new( + &*self.services.fs, + &*self.services.shell, + &self.services.lore_api, + ); + match action_service.apply_patchset(&request, &self.state.config) { + Ok(applied) => { + let popup_body = match kw_apply_record(details, &self.state.config, &applied) { + // Defensive: the apply itself resolved this tree from + // the same snapshot, so this is unreachable unless the + // config changed mid-apply. + None => { + warn!( + "kw apply history skipped: target kernel tree is no longer configured" + ); + format!( + "{}\n\nWarning: the apply was not recorded in the kw history: target kernel tree is no longer configured", + applied.message + ) + } + Some(record) => { + // History writes go through KwActor so apply + // recording serializes with job state; without an + // actor (non-unix), write the store directly. + let recorded = match &self.services.kw { + Some(kw) => kw.record_apply(record).await.map_err(|e| e.to_string()), + None => self + .services + .kw_history + .record_apply(record) + .map_err(|e| e.to_string()), + }; + // The git apply itself succeeded; a history-write + // failure must not turn it into a reported failure. + match recorded { + Ok(()) => applied.message, + Err(e) => { + warn!(error = %e, "failed to record kw apply history"); + format!( + "{}\n\nWarning: the apply was not recorded in the kw history: {e}\nIf this warning keeps appearing, inspect or delete that file.", + applied.message + ) + } + } + } + }; + popup::AppPopup::info("Patchset Apply Success", popup_body) + } + Err(msg) => popup::AppPopup::info("Patchset Apply Fail", msg), + } + } + /// Opens the edit-config screen from the current configuration snapshot. pub fn init_edit_config(&mut self) { self.state.config_state.edit_config = Some(EditConfigState::new(&self.state.config)); From 9662dec159b1f1f00d8159519c4c627e10aa7b72 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 20 Aug 2026 17:47:34 -0300 Subject: [PATCH 08/10] fix(kw): roll back refused starts and snapshot build records at accept This commit rolls HEAD back when a start is refused after the branch switch, so a failed spawn never mutates the tree or clobbers restore context. Build records now reuse the output dir and arch probed at accept instead of re-resolving at completion, which could key a successful job as a no-env build. Checkout git calls run off the actor task, untracked files are ignored by the dirty check, and --menu is reserved so menuconfig cannot hang a redirected build. This commit completes the kw integration's step 5. Signed-off-by: lorenzoberts --- src/app/integration_tests/patchset_actions.rs | 89 ++- src/kw/actor.rs | 525 ++++++++++++------ src/kw/argv.rs | 21 +- src/kw/history.rs | 11 +- src/kw/readiness.rs | 13 +- 5 files changed, 473 insertions(+), 186 deletions(-) diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs index df2cbdb..5105f99 100644 --- a/src/app/integration_tests/patchset_actions.rs +++ b/src/app/integration_tests/patchset_actions.rs @@ -22,7 +22,9 @@ use crate::{ process::FakeProcess, shell::{MockShellTrait, ShellCommand, ShellOutput}, }, - kw::{actor::KwActor, history::MockKwHistoryStore, messages::StartRequest}, + kw::{ + actor::KwActor, history::MockKwHistoryStore, messages::StartRequest, status::KwJobStatus, + }, lore::application::{ cache::BootstrapLoreData, handle::LoreApiHandle, messages::LoreApiMessage, }, @@ -266,6 +268,7 @@ async fn apply_is_blocked_while_a_kw_job_runs() { kw_actor_shell(), kw_actor_fs(), kw_actor_env(), + Arc::new(FakeProcess::new()), log_dir.clone(), ); @@ -286,6 +289,81 @@ async fn apply_is_blocked_while_a_kw_job_runs() { std::fs::remove_dir_all(&log_dir).unwrap(); } +#[tokio::test] +async fn apply_is_allowed_again_after_the_job_finishes() { + let (shell, calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + ]); + let log_dir = kw_log_dir("apply-after-job"); + let process = Arc::new(FakeProcess::new()); + let mut store = MockKwHistoryStore::new(); + store.expect_record_apply().returning(|_| Ok(())); + store + .expect_apply_record_for_branch() + .returning(|_, _| Ok(None)); + store.expect_record_build().returning(|_| Ok(())); + let mut app = app_with_details_and_kw( + clean_fs(), + shell, + lore_handle_with_persistence(), + apply_details_state(), + apply_config(), + store, + kw_actor_shell(), + kw_actor_fs(), + kw_actor_env(), + process.clone(), + log_dir.clone(), + ); + + let kw = app.services.kw.as_ref().expect("tests run on unix").clone(); + kw.start_build(kw_start_request()).await.unwrap(); + + // While the job runs, the apply is blocked. + app.consolidate_patchset_actions().await.unwrap(); + assert_info_popup_contains(app.state.popup.as_ref(), "Patchset Apply Blocked", &[]); + assert!(calls.lock().unwrap().is_empty()); + + // Once the job finishes, the same apply goes through. + process.last_child().finish(0); + let mut watch = kw.watch_status().await.unwrap(); + tokio::time::timeout(std::time::Duration::from_secs(10), async { + loop { + if matches!(watch.borrow().job, KwJobStatus::Succeeded { .. }) { + break; + } + watch.changed().await.unwrap(); + } + }) + .await + .expect("job must reach a terminal state"); + + let details = app + .state + .lore + .details + .as_mut() + .expect("details should remain loaded"); + details.toggle_apply_action(); + app.consolidate_patchset_actions().await.unwrap(); + + assert_apply_action(&app, false); + assert_info_popup_contains( + app.state.popup.as_ref(), + "Patchset Apply Success", + &["applied successfully"], + ); + assert_eq!(6, calls.lock().unwrap().len()); + + shutdown_kw(&app).await; + std::fs::remove_dir_all(&log_dir).unwrap(); +} + #[tokio::test] async fn reviewed_reply_success_records_persists_and_resets_reply_action() { let saved_reviewed = Arc::new(Mutex::new(None)); @@ -397,12 +475,14 @@ fn app_with_details( MockShellTrait::new(), MockFileSystemTrait::new(), MockEnvTrait::new(), + Arc::new(FakeProcess::new()), PathBuf::from("/tmp/patch-hub-test-kw-logs"), ) } -/// Variant of [`app_with_details`] whose kw actor gets its own mocks and -/// log dir, for tests that start jobs through the app's kw handle. +/// Variant of [`app_with_details`] whose kw actor gets its own mocks, +/// process double, and log dir, for tests that start jobs through the +/// app's kw handle. #[allow(clippy::too_many_arguments)] fn app_with_details_and_kw( fs: MockFileSystemTrait, @@ -414,13 +494,14 @@ fn app_with_details_and_kw( kw_shell: MockShellTrait, kw_fs: MockFileSystemTrait, kw_env: MockEnvTrait, + kw_process: Arc, kw_log_dir: PathBuf, ) -> App { // Apply history is recorded through the real actor wrapping the mock // store, mirroring production wiring. let kw = KwActor::spawn( Arc::new(kw_history), - Arc::new(FakeProcess::new()), + kw_process, Arc::new(kw_shell), Arc::new(kw_fs), Arc::new(kw_env), diff --git a/src/kw/actor.rs b/src/kw/actor.rs index 3920217..3b23e8b 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -10,11 +10,12 @@ //! //! Tracked note (same class as the integration plan's §2.1i): the actor //! runs quick blocking calls inline in its async task — `create_dir_all`, -//! the git probes of the checkout policy, the build-record completion -//! probes, and the history store's atomic writes — per the ConfigActor -//! precedent. They are all milliseconds-scale; if a real stall ever shows -//! up while a long job runs, they should move behind `spawn_blocking` -//! like the other actors' heavy work. +//! the build-record completion probes, and the history store's atomic +//! writes — per the ConfigActor precedent. The checkout policy's git +//! calls are different: `git switch` rewrites the worktree (seconds on a +//! kernel tree), so the start and restore paths run them behind +//! `spawn_blocking` and the `Start*`/`RestorePreviousBranch` replies stay +//! immediate. use std::{ ops::ControlFlow, @@ -87,10 +88,13 @@ struct JobState { phase: KwPhase, kernel_tree_id: String, branch: String, - /// Snapshot of the tree path at accept time: the build record carries - /// it so later readiness checks can detect the tree being repointed - /// (§2.1e). + /// Tree path, kw-env output dir, and build arch as probed at accept + /// time. The build runs under these, so the completion record + /// describes this snapshot — not whatever the tree's configuration + /// says by the time the job ends (§2.1e drift detection). tree_path: String, + output_dir: Option, + arch: Option, log_path: PathBuf, /// `None` once a cancel has been requested; a second `Cancel` is an /// idempotent ack. @@ -193,7 +197,7 @@ impl KwActor { send_start_reply( message_name, reply, - self.start_job(KwJobKind::Build, request), + self.start_job(KwJobKind::Build, request).await, ); ControlFlow::Continue(()) } @@ -228,7 +232,7 @@ impl KwActor { ControlFlow::Continue(()) } KwMessage::RestorePreviousBranch { reply } => { - send_kw_reply(message_name, reply, self.restore_previous_branch()); + send_kw_reply(message_name, reply, self.restore_previous_branch().await); ControlFlow::Continue(()) } KwMessage::Shutdown { reply } => { @@ -267,8 +271,14 @@ impl KwActor { /// unresolvable kw-env state, a tree that fails the readiness probes, /// a dirty worktree, or a failed branch switch. The argv comes from /// the reserved-flags merge (§2.1f): patch-hub's own flags win over - /// the request's extra args. - fn start_job(&mut self, kind: KwJobKind, request: StartRequest) -> Result<(), KwStartError> { + /// the request's extra args. A start refused after the branch switch + /// rolls the switch back: a refused start never leaves the tree on a + /// branch the user did not check out. + async fn start_job( + &mut self, + kind: KwJobKind, + request: StartRequest, + ) -> Result<(), KwStartError> { if self.job.is_some() { return Err(KwStartError::JobAlreadyRunning); } @@ -292,42 +302,25 @@ impl KwActor { let tree_path = PathBuf::from(request.tree.path()); // Unresolvable env state refuses the start: the build record this // job writes at completion must know whether it ran under an O=. + // Both values are then snapshotted onto the job — the build runs + // under them, so the completion record describes them, not the + // tree's configuration at whatever time the job ends. let output_dir = readiness::resolve_output_dir(&*self.fs, &*self.env, &tree_path)?; let tree_readiness = readiness::probe_tree(&*self.fs, &tree_path, output_dir.as_deref()); - if !matches!(tree_readiness, TreeReadiness::Ready { .. }) { + let TreeReadiness::Ready { arch } = tree_readiness else { return Err(KwStartError::TreeNotReady(tree_readiness)); - } - - // Refuse on a dirty worktree before touching anything (§2.1a): a - // switch could otherwise carry unrelated changes into the build - // branch. - self.check_worktree_clean(request.tree.path())?; - - // Probed before anything touches the tree: the `git switch` below - // goes between this probe and the spawn, and the probe must still - // capture the pre-job HEAD or RestorePreviousBranch would - // "restore" the branch the job switched to. An unprobed HEAD - // (detached, or not a git repo) records nothing rather than a - // wrong branch. - let pre_job_branch = match self.head_branch(&request.tree) { - branch if branch.is_empty() => None, - branch => Some(branch), }; - // The checkout policy (§2.1a): the job runs on the requested - // branch, and HEAD stays there after the job. - self.switch_to_branch(request.tree.path(), &request.branch)?; + let pre_job_branch = self.checkout_build_branch(&request).await?; - self.fs.create_dir_all(&self.kw_log_dir)?; - // Millisecond suffix: two jobs started within the same second must - // not share a log file — spawn truncates it. - let log_path = self.kw_log_dir.join(format!( - "build-{}.log", - chrono::Utc::now().format("%Y%m%d-%H%M%S-%3f") - )); - let cmd = ShellCommand::new("kw").args(argv::build_argv(&request.extra_args)); - let cwd = PathBuf::from(request.tree.path()); - let process = self.process.spawn(&cmd, &cwd, &log_path)?; + let (process, log_path) = match self.spawn_build_process(&request) { + Ok(spawned) => spawned, + Err(error) => { + self.rollback_switch(request.tree.path(), pre_job_branch.as_deref()) + .await; + return Err(error); + } + }; let (cancel_tx, cancel_rx) = oneshot::channel(); spawn(run_job(process, cancel_rx, self.job_event_tx.clone())); @@ -339,10 +332,11 @@ impl KwActor { log_path = %log_path.display(), "kw build job started" ); - // Recorded only on accept: a refused start — including one refused - // after the switch (log-dir creation, spawn) — never clobbers a - // previous job's restore target. An accepted job with an unprobed - // pre-job HEAD clears it: nothing honest is left to restore to. + // Recorded only on accept. Refused starts never reach here, and — + // with their switch rolled back — can neither clobber a previous + // job's restore target nor strand the tree on a branch the user + // did not check out. An accepted job with an unprobed pre-job + // HEAD clears the target: nothing honest is left to restore to. self.last_restore = pre_job_branch.map(|branch| RestoreContext { tree_path: request.tree.path().to_string(), branch, @@ -353,6 +347,8 @@ impl KwActor { kernel_tree_id: request.kernel_tree_id.clone(), branch: request.branch.clone(), tree_path: request.tree.path().to_string(), + output_dir, + arch, log_path: log_path.clone(), cancel_tx: Some(cancel_tx), }); @@ -366,6 +362,96 @@ impl KwActor { Ok(()) } + /// The checkout policy (§2.1a): refuse on a dirty worktree, probe the + /// pre-job HEAD, then switch the tree onto the requested branch. The + /// git calls rewrite the worktree — seconds on a kernel tree, not + /// milliseconds — so they run on the blocking pool and the accept + /// reply stays immediate. + /// + /// The HEAD probe sits between the dirty check and the switch: it + /// must capture the branch the user was on, or RestorePreviousBranch + /// would "restore" the branch the job switched to. An unprobed HEAD + /// (detached, or not a git repo) yields `None` rather than a wrong + /// branch. + async fn checkout_build_branch( + &self, + request: &StartRequest, + ) -> Result, KwStartError> { + let shell = Arc::clone(&self.shell); + let tree_path = request.tree.path().to_string(); + let branch = request.branch.clone(); + let pre_job_branch = + tokio::task::spawn_blocking(move || -> Result { + check_worktree_clean(&*shell, &tree_path)?; + let pre_job_branch = head_branch(&*shell, &tree_path); + switch_to_branch(&*shell, &tree_path, &branch)?; + Ok(pre_job_branch) + }) + .await + // A join error means the probe task panicked — a bug, surfaced as + // an unverifiable git state rather than wedging the actor. + .map_err(|error| KwStartError::GitStateProbe(error.to_string()))??; + Ok(if pre_job_branch.is_empty() { + None + } else { + Some(pre_job_branch) + }) + } + + /// Switches the tree back after a start that reached the branch + /// switch but failed before accepting the job. A rollback failure is + /// logged, not reported: the caller already holds the actionable + /// refusal. An unprobed pre-job HEAD leaves nothing to roll back to. + async fn rollback_switch(&self, tree_path: &str, pre_job_branch: Option<&str>) { + let Some(branch) = pre_job_branch else { + tracing::warn!( + tree = tree_path, + "cannot roll back the branch switch: pre-job HEAD was unprobed" + ); + return; + }; + let shell = Arc::clone(&self.shell); + let tree_path = tree_path.to_string(); + let branch = branch.to_string(); + let branch_for_task = branch.clone(); + match tokio::task::spawn_blocking(move || { + switch_to_branch(&*shell, &tree_path, &branch_for_task) + }) + .await + { + Ok(Ok(())) => tracing::info!( + branch, + "rolled back the branch switch after a refused start" + ), + Ok(Err(error)) => { + let error = KwStartError::from(error); + tracing::warn!(%error, "failed to roll back the branch switch") + } + Err(error) => { + tracing::warn!(%error, "branch-switch rollback task failed to join") + } + } + } + + /// Creates the job's log dir and spawns the kw process. Split from + /// `start_job` so a failure here can roll the branch switch back. + fn spawn_build_process( + &self, + request: &StartRequest, + ) -> Result<(Box, PathBuf), KwStartError> { + self.fs.create_dir_all(&self.kw_log_dir)?; + // Millisecond suffix: two jobs started within the same second must + // not share a log file — spawn truncates it. + let log_path = self.kw_log_dir.join(format!( + "build-{}.log", + chrono::Utc::now().format("%Y%m%d-%H%M%S-%3f") + )); + let cmd = ShellCommand::new("kw").args(argv::build_argv(&request.extra_args)); + let cwd = PathBuf::from(request.tree.path()); + let process = self.process.spawn(&cmd, &cwd, &log_path)?; + Ok((process, log_path)) + } + fn request_cancel(&mut self) -> Result<(), KwError> { match self.job.as_mut() { Some(job) => { @@ -458,30 +544,21 @@ impl KwActor { JobOutcome::Cancelled => return, }; + // The record describes the accept-time snapshot: the build ran + // under this tree path, output dir, and arch. Re-resolving them + // here could describe a configuration the build never used — an + // env deactivated mid-build would key the record with a wrong + // `output_dir: None` and probe the tree for an image the build + // wrote under O=, a Frankenstein match for a later deploy-alone + // probe. let tree_path = Path::new(&job.tree_path); - // If the env state became unresolvable mid-build, the record - // would be keyed with a wrong `output_dir: None` — a Frankenstein - // match for a later no-env deploy-alone probe. No record fails - // safe. - let output_dir = match readiness::resolve_output_dir(&*self.fs, &*self.env, tree_path) { - Ok(output_dir) => output_dir, - Err(error) => { - tracing::warn!( - %error, - branch = job.branch, - "skipping the build record: kw env state unresolvable" - ); - return; - } - }; - let arch = readiness::read_build_arch(&*self.fs, tree_path); + let build_root = job.output_dir.as_deref().unwrap_or(tree_path); // A failed build may have left a stale image from an earlier // successful one behind; only successes record what they // produced. let (image_path, kernelrelease) = if success { - let build_root = output_dir.as_deref().unwrap_or(tree_path); ( - readiness::find_newest_kernel_image(&*self.fs, build_root, arch.as_deref()), + readiness::find_newest_kernel_image(&*self.fs, build_root, job.arch.as_deref()), readiness::read_kernelrelease(&*self.fs, build_root), ) } else { @@ -507,9 +584,12 @@ impl KwActor { tree_path: job.tree_path.clone(), message_id, branch: job.branch.clone(), - arch, + arch: job.arch.clone(), image_path: image_path.map(|path| path.to_string_lossy().into_owned()), - output_dir: output_dir.map(|path| path.to_string_lossy().into_owned()), + output_dir: job + .output_dir + .as_ref() + .map(|path| path.to_string_lossy().into_owned()), kernelrelease, log_path: job.log_path.to_string_lossy().into_owned(), built_at: chrono::Utc::now().to_rfc3339(), @@ -531,7 +611,7 @@ impl KwActor { kernel_tree_id: &str, tree: &KernelTree, ) -> Result { - let head = self.head_branch(tree); + let head = head_branch(&*self.shell, tree.path()); Ok(readiness::evaluate_readiness( &*self.fs, &*self.env, @@ -543,60 +623,31 @@ impl KwActor { )?) } - /// Fails unless the tree's git state verifies clean (§2.1a). A probe - /// that itself fails — git missing, not a repository — fails too: - /// starting a job or restoring a branch on a tree whose state is - /// unknown could carry unrecorded changes across branches. - fn check_worktree_clean(&self, tree_path: &str) -> Result<(), TreeGitError> { - let cmd = ShellCommand::new("git").args(["-C", tree_path, "status", "--porcelain"]); - let output = self - .shell - .execute(&cmd) - .map_err(|error| TreeGitError::Probe(error.to_string()))?; - if !output.success { - return Err(TreeGitError::Probe( - String::from_utf8_lossy(&output.stderr).trim().to_string(), - )); - } - if !output.stdout.is_empty() { - return Err(TreeGitError::DirtyWorktree); - } - Ok(()) - } - - /// Switches the tree onto `branch`. A failure carries git's stderr, - /// which names the usual causes (no such branch, a rebase or merge in - /// progress). - fn switch_to_branch(&self, tree_path: &str, branch: &str) -> Result<(), TreeGitError> { - let cmd = ShellCommand::new("git").args(["-C", tree_path, "switch", branch]); - let output = self - .shell - .execute(&cmd) - .map_err(|error| TreeGitError::Switch(error.to_string()))?; - if !output.success { - return Err(TreeGitError::Switch( - String::from_utf8_lossy(&output.stderr).trim().to_string(), - )); - } - Ok(()) - } - /// Switches the tree that ran the last job back to the branch HEAD was /// on when that job was accepted (§2.1a). Refuses while a job is /// running (its branch is in use), when nothing was recorded, and on /// a dirty worktree. Only a successful switch consumes the context — /// a refused restore stays available for a retry. - fn restore_previous_branch(&mut self) -> Result<(), KwError> { + async fn restore_previous_branch(&mut self) -> Result<(), KwError> { if self.job.is_some() { return Err(KwError::JobRunning); } let Some(restore) = self.last_restore.take() else { return Err(KwError::NoRecordedBranch); }; - match self - .check_worktree_clean(&restore.tree_path) - .and_then(|()| self.switch_to_branch(&restore.tree_path, &restore.branch)) - { + let shell = Arc::clone(&self.shell); + let tree_path = restore.tree_path.clone(); + let branch = restore.branch.clone(); + // Same spawn_blocking rationale as the start path's checkout: the + // switch rewrites the worktree. + let result = tokio::task::spawn_blocking(move || -> Result<(), TreeGitError> { + check_worktree_clean(&*shell, &tree_path)?; + switch_to_branch(&*shell, &tree_path, &branch) + }) + .await + .map_err(|error| KwError::GitStateProbe(error.to_string())) + .and_then(|result| result.map_err(KwError::from)); + match result { Ok(()) => { tracing::info!( tree = restore.tree_path, @@ -607,34 +658,82 @@ impl KwActor { } Err(error) => { self.last_restore = Some(restore); - Err(error.into()) + Err(error) } } } +} - /// The tree's current branch, probed via git. An unresolvable HEAD - /// (not a git repo, or a detached HEAD, for which `branch - /// --show-current` prints nothing) yields an empty string: no build - /// record can match it, so deploy-alone readiness refuses — the safe - /// direction for an unknown HEAD. - fn head_branch(&self, tree: &KernelTree) -> String { - let cmd = ShellCommand::new("git").args(["-C", tree.path(), "branch", "--show-current"]); - match self.shell.execute(&cmd) { - Ok(output) if output.success => { - String::from_utf8_lossy(&output.stdout).trim().to_string() - } - Ok(output) => { - tracing::warn!( - tree = tree.path(), - stderr = %String::from_utf8_lossy(&output.stderr), - "failed to probe the kernel tree's HEAD branch" - ); - String::new() - } - Err(error) => { - tracing::warn!(tree = tree.path(), %error, "failed to probe the kernel tree's HEAD branch"); - String::new() - } +/// Fails unless the tree's git state verifies clean (§2.1a). Untracked +/// files don't count: kernel trees accumulate local scratch files, and +/// only tracked changes can corrupt the branch a job builds — an +/// untracked file that would collide with the switch is still caught by +/// git itself. A probe that itself fails — git missing, not a repository +/// — fails too: starting a job or restoring a branch on a tree whose +/// state is unknown could carry unrecorded changes across branches. +fn check_worktree_clean(shell: &dyn ShellTrait, tree_path: &str) -> Result<(), TreeGitError> { + let cmd = ShellCommand::new("git").args([ + "-C", + tree_path, + "status", + "--porcelain", + "--untracked-files=no", + ]); + let output = shell + .execute(&cmd) + .map_err(|error| TreeGitError::Probe(error.to_string()))?; + if !output.success { + return Err(TreeGitError::Probe( + String::from_utf8_lossy(&output.stderr).trim().to_string(), + )); + } + if !output.stdout.is_empty() { + return Err(TreeGitError::DirtyWorktree); + } + Ok(()) +} + +/// Switches the tree onto `branch`. A failure carries git's stderr, which +/// names the usual causes (no such branch, a rebase or merge in +/// progress). The `--` keeps a branch named like a flag from being parsed +/// as one. +fn switch_to_branch( + shell: &dyn ShellTrait, + tree_path: &str, + branch: &str, +) -> Result<(), TreeGitError> { + let cmd = ShellCommand::new("git").args(["-C", tree_path, "switch", "--", branch]); + let output = shell + .execute(&cmd) + .map_err(|error| TreeGitError::Switch(error.to_string()))?; + if !output.success { + return Err(TreeGitError::Switch( + String::from_utf8_lossy(&output.stderr).trim().to_string(), + )); + } + Ok(()) +} + +/// The tree's current branch, probed via git. An unresolvable HEAD +/// (not a git repo, or a detached HEAD, for which `branch +/// --show-current` prints nothing) yields an empty string: no build +/// record can match it, so deploy-alone readiness refuses — the safe +/// direction for an unknown HEAD. +fn head_branch(shell: &dyn ShellTrait, tree_path: &str) -> String { + let cmd = ShellCommand::new("git").args(["-C", tree_path, "branch", "--show-current"]); + match shell.execute(&cmd) { + Ok(output) if output.success => String::from_utf8_lossy(&output.stdout).trim().to_string(), + Ok(output) => { + tracing::warn!( + tree = tree_path, + stderr = %String::from_utf8_lossy(&output.stderr), + "failed to probe the kernel tree's HEAD branch" + ); + String::new() + } + Err(error) => { + tracing::warn!(tree = tree_path, %error, "failed to probe the kernel tree's HEAD branch"); + String::new() } } } @@ -898,15 +997,17 @@ mod tests { (shell, calls) } - /// A stateful shell double for the restore tests: kw's version probe - /// answers 0.10.0, `git status --porcelain` reflects the dirty flag, - /// the HEAD probe reports `head` (with the trailing newline git - /// prints), and a successful `git switch` updates `head` — mirroring - /// a real worktree the actor switches between branches. + /// A stateful shell double for the checkout/restore tests: kw's + /// version probe answers 0.10.0, `git status --porcelain` reflects the + /// dirty flag, the HEAD probe reports `head` (with the trailing + /// newline git prints), and a successful `git switch` updates `head` + /// — mirroring a real worktree the actor switches between branches. + /// Switches to `fail_switch_to` fail, so a test can model "going + /// forward works, coming back fails". struct GitStub { head: Arc>, dirty: Arc, - fail_switches: Arc, + fail_switch_to: Arc>>, } impl GitStub { @@ -914,7 +1015,7 @@ mod tests { Self { head: Arc::new(Mutex::new(branch.to_string())), dirty: Arc::new(AtomicBool::new(false)), - fail_switches: Arc::new(AtomicBool::new(false)), + fail_switch_to: Arc::new(Mutex::new(None)), } } @@ -922,8 +1023,8 @@ mod tests { self.dirty.store(dirty, Ordering::Relaxed); } - fn set_fail_switches(&self, fail: bool) { - self.fail_switches.store(fail, Ordering::Relaxed); + fn fail_switches_to(&self, branch: Option<&str>) { + *self.fail_switch_to.lock().unwrap() = branch.map(str::to_string); } fn head(&self) -> String { @@ -933,7 +1034,7 @@ mod tests { fn shell(&self) -> MockShellTrait { let head = Arc::clone(&self.head); let dirty = Arc::clone(&self.dirty); - let fail_switches = Arc::clone(&self.fail_switches); + let fail_switch_to = Arc::clone(&self.fail_switch_to); let mut shell = MockShellTrait::new(); shell.expect_execute().returning(move |cmd| { let output = |stdout: &[u8]| ShellOutput { @@ -953,7 +1054,8 @@ mod tests { return Ok(output(stdout)); } if cmd.args.iter().any(|arg| arg == "switch") { - if fail_switches.load(Ordering::Relaxed) { + let branch = cmd.args.last().unwrap().clone(); + if fail_switch_to.lock().unwrap().as_deref() == Some(branch.as_str()) { return Ok(ShellOutput { stdout: Vec::new(), stderr: b"error: you need to resolve your current index first\n" @@ -961,7 +1063,7 @@ mod tests { success: false, }); } - *head.lock().unwrap() = cmd.args.last().unwrap().clone(); + *head.lock().unwrap() = branch; return Ok(output(b"")); } let current = format!("{}\n", head.lock().unwrap()); @@ -1564,6 +1666,21 @@ mod tests { status < head && head < switch, "checkout policy must probe dirty state, then HEAD, then switch: {calls:?}" ); + // Untracked scratch files don't block a build; only tracked + // changes do. + assert_eq!( + &calls[status], + &command(&[ + "git", + "-C", + "/home/user/linux", + "status", + "--porcelain", + "--untracked-files=no" + ]) + ); + // The `--` keeps a branch named like a flag from being parsed + // as one. assert_eq!( &calls[switch], &command(&[ @@ -1571,6 +1688,7 @@ mod tests { "-C", "/home/user/linux", "switch", + "--", "patchset-2026-08-01-17-30-00" ]) ); @@ -1779,12 +1897,12 @@ mod tests { process.last_child().finish(0); let _ = wait_for_terminal_status(&mut watch).await; - git.set_fail_switches(true); + git.fail_switches_to(Some("master")); let err = handle.restore_previous_branch().await.unwrap_err(); assert!(matches!(err, KwError::CheckoutFailed(_))); assert!(err.to_string().contains("resolve your current index")); - git.set_fail_switches(false); + git.fail_switches_to(None); handle.restore_previous_branch().await.unwrap(); assert_eq!(git.head(), "master"); @@ -1805,13 +1923,14 @@ mod tests { assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); // This start is refused at spawn — after its HEAD probe and - // switch — and must not overwrite the recorded restore target. + // switch — and the switch is rolled back: the tree returns to the + // first job's branch and the recorded restore target is intact. process.refuse_spawns(true); let mut second = start_request(); second.branch = "patchset-two".to_string(); let err = handle.start_build(second).await.unwrap_err(); assert!(matches!(err, KwStartError::Spawn(_))); - assert_eq!(git.head(), "patchset-two"); + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); handle.restore_previous_branch().await.unwrap(); assert_eq!(git.head(), "master"); @@ -1838,7 +1957,7 @@ mod tests { #[tokio::test] async fn log_dir_creation_failure_refuses_start_and_stays_idle() { - let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); + let git = GitStub::on_branch("master"); let mut fs = MockFileSystemTrait::new(); expect_ready_tree(&mut fs); fs.expect_create_dir_all().returning(|_| { @@ -1846,13 +1965,46 @@ mod tests { "read-only filesystem", ))) }); - let (handle, process, log_dir) = spawn_job_actor_with_mocks("log-dir-fail", shell, fs); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("log-dir-fail", git.shell(), fs); let err = handle.start_build(start_request()).await.unwrap_err(); assert!(matches!(err, KwStartError::Fs(_))); assert_eq!(KwJobStatus::Idle, handle.get_status().await.unwrap().job); assert!(process.spawned().is_empty()); + // The switch happened and was rolled back: the tree is back on the + // user's branch, and no restore target was recorded. + assert_eq!(git.head(), "master"); + assert!(matches!( + handle.restore_previous_branch().await, + Err(KwError::NoRecordedBranch) + )); + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn rollback_failure_keeps_the_spawn_refusal() { + let git = GitStub::on_branch("master"); + // Going forward works; coming back fails. + git.fail_switches_to(Some("master")); + let (handle, process, log_dir) = + spawn_job_actor_with_mocks("rollback-fails", git.shell(), ready_fs()); + process.refuse_spawns(true); + + let err = handle.start_build(start_request()).await.unwrap_err(); + + assert!(matches!(err, KwStartError::Spawn(_))); + // The rollback failure is logged, not reported: the caller keeps + // the actionable refusal, and the tree honestly shows where HEAD + // is. No restore target was recorded. + assert_eq!(git.head(), "patchset-2026-08-01-17-30-00"); + assert!(matches!( + handle.restore_previous_branch().await, + Err(KwError::NoRecordedBranch) + )); handle.shutdown().await; std::fs::remove_dir_all(&log_dir).unwrap(); @@ -2148,22 +2300,45 @@ mod tests { } #[tokio::test] - async fn build_record_skipped_when_env_state_breaks_mid_build() { - // An env that resolves at Start but not at completion must not - // produce a record keyed with a wrong `output_dir: None`. - let env_broken = Arc::new(AtomicBool::new(false)); - let env_broken_in_fs = Arc::clone(&env_broken); + async fn successful_build_with_active_env_records_the_output_dir() { + // env.current is read once, at accept time. The completion record + // must describe the env the build ran under — the snapshot — not + // the tree's env state at whatever time the job ends. + let env_current_reads = Arc::new(AtomicU64::new(0)); + let env_current_reads_in_fs = Arc::clone(&env_current_reads); let mut fs = MockFileSystemTrait::new(); fs.expect_is_dir().returning(|_| true); fs.expect_is_file().returning(|_| true); fs.expect_exists().returning(|_| true); fs.expect_read_to_string().returning(move |path| { - if path.ends_with("env.current") && !env_broken_in_fs.load(Ordering::Relaxed) { + if path.ends_with("env.current") { + env_current_reads_in_fs.fetch_add(1, Ordering::SeqCst); Ok("testenv\n".to_string()) + } else if path.ends_with("build.config") { + Ok("arch=x86\n".to_string()) + } else if path.ends_with("kernel.release") { + Ok("6.17.0\n".to_string()) } else { - Err(FileSystemError::IoError(io::Error::other("unreadable"))) + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + } + }); + // Every boot-dir probe answers with an image inside the probed + // dir: the record's image path shows which build root was used. + fs.expect_read_dir().returning(|path| { + if path.ends_with("arch/x86/boot") { + Ok(vec![path.join("bzImage")]) + } else { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) } }); + fs.expect_metadata() + .returning(|_| Err(FileSystemError::IoError(io::Error::other("no metadata")))); fs.expect_create_dir_all().returning(|_| Ok(())); let mut env = MockEnvTrait::new(); env.expect_which().returning(|_| true); @@ -2171,18 +2346,38 @@ mod tests { .returning(|_| Ok("/home/user/.cache".to_string())); let (history, builds) = recording_history(None); let (shell, _calls) = recording_shell(KW_VERSION_OK, CLEAN_STATUS, SWITCH_OK); - let (handle, process, log_dir) = spawn_full_actor("env-breaks", history, shell, fs, env); + let (handle, process, log_dir) = spawn_full_actor("env-build", history, shell, fs, env); let mut watch = handle.watch_status().await.unwrap(); handle.start_build(start_request()).await.unwrap(); - // Break the env state after the start probes but before the - // completion probes run. - env_broken.store(true, Ordering::Relaxed); process.last_child().finish(0); let status = wait_for_terminal_status(&mut watch).await; - assert!(matches!(status, KwJobStatus::Succeeded { .. })); - assert!(builds.lock().unwrap().is_empty()); + + { + let builds = builds.lock().unwrap(); + assert_eq!(1, builds.len()); + let record = &builds[0]; + let output_dir = record + .output_dir + .as_deref() + .expect("an env build records its O= dir"); + assert!( + output_dir.contains("/kw/envs/") && output_dir.ends_with("/testenv"), + "unexpected output dir: {output_dir}" + ); + let image = record.image_path.as_deref().expect("image recorded"); + assert!( + image.starts_with(output_dir) && image.ends_with("bzImage"), + "image {image} must be probed under the env's output dir {output_dir}" + ); + assert_eq!(Some("x86"), record.arch.as_deref()); + assert_eq!(Some("6.17.0"), record.kernelrelease.as_deref()); + assert!(record.success); + } + // Read at accept time only: the completion record uses the + // snapshot, never a re-resolve. + assert_eq!(1, env_current_reads.load(Ordering::SeqCst)); handle.shutdown().await; std::fs::remove_dir_all(&log_dir).unwrap(); diff --git a/src/kw/argv.rs b/src/kw/argv.rs index aaedb53..e122b47 100644 --- a/src/kw/argv.rs +++ b/src/kw/argv.rs @@ -25,13 +25,16 @@ impl ReservedOption { } } -/// Reserved for `kw build`: patch-hub forces non-interactive alerts, and -/// owns the job's log file (ProcessTrait captures kw's stdout/stderr to -/// it) — a user-supplied `--save-log-to` would fork the log to a second -/// file, leaving KwOps tailing only half the output. +/// Reserved for `kw build`: patch-hub forces non-interactive alerts, owns +/// the job's log file (ProcessTrait captures kw's stdout/stderr to it) — +/// a user-supplied `--save-log-to` would fork the log to a second file, +/// leaving KwOps tailing only half the output — and never runs `--menu` +/// from automation (plan §1.1): the job's stdio is a log file, so +/// menuconfig would hang until cancelled. const BUILD_RESERVED: &[ReservedOption] = &[ ReservedOption::new(&["--alert"], true), ReservedOption::new(&["--save-log-to"], true), + ReservedOption::new(&["--menu"], false), ]; /// The argv for a build job: `kw build --alert=n `. @@ -146,6 +149,16 @@ mod tests { ); } + #[test] + fn menu_is_stripped_without_eating_the_next_token() { + // kw build --menu would open menuconfig with the job's stdio + // redirected to a log file — a hang, not a build. + assert_eq!( + vec!["build", "--alert=n", "--verbose"], + build_argv(&extras(&["--menu", "--verbose"])) + ); + } + #[test] fn boolean_reserved_strips_only_itself_in_all_spellings() { // The deploy step's reserved set has booleans with short forms; diff --git a/src/kw/history.rs b/src/kw/history.rs index 227751d..85d35c5 100644 --- a/src/kw/history.rs +++ b/src/kw/history.rs @@ -85,9 +85,9 @@ pub trait KwHistoryStore: Send + Sync { /// it came from. A missing history file is a normal state, not an /// error. Records with unparseable `applied_at` values sort oldest, /// same convention as the build records. - // Read by KwActor when writing build records (the build step); kept - // per the CachePolicy precedent (src/lore/application/cache.rs). - #[allow(dead_code)] + // The only production caller is the unix-only actor's build-record + // writer. + #[cfg_attr(not(unix), allow(dead_code))] fn apply_record_for_branch( &self, kernel_tree_id: &str, @@ -96,7 +96,9 @@ pub trait KwHistoryStore: Send + Sync { /// Inserts or replaces the build record for the record's /// `(kernel_tree_id, branch)` pair. - #[allow(dead_code)] + // The only production caller is the unix-only actor's build-record + // writer. + #[cfg_attr(not(unix), allow(dead_code))] fn record_build(&self, record: KwBuildRecord) -> Result<(), FileSystemError>; /// Returns the build record for the `(kernel_tree_id, branch)` pair, or @@ -120,7 +122,6 @@ pub trait KwHistoryStore: Send + Sync { /// Returns the record for `(kernel_tree_id, branch)` and the newest /// record for the tree across branches from a single load of the /// history file — the pair a readiness snapshot is computed from. - #[allow(dead_code)] fn build_records( &self, kernel_tree_id: &str, diff --git a/src/kw/readiness.rs b/src/kw/readiness.rs index d6c2d54..e577c0f 100644 --- a/src/kw/readiness.rs +++ b/src/kw/readiness.rs @@ -114,7 +114,6 @@ pub fn is_kernel_root(fs: &dyn FileSystemTrait, path: &Path) -> bool { /// refuses to activate an env while an in-tree `.config` exists /// (`kw_env.sh::validate_env_before_switch`), so with an env active the /// `.config` lives only at the env's `O=` dir. -#[allow(dead_code)] pub fn probe_tree( fs: &dyn FileSystemTrait, tree_path: &Path, @@ -177,7 +176,6 @@ pub fn read_build_arch(fs: &dyn FileSystemTrait, tree_path: &Path) -> Option SystemTime { /// `/include/config/kernel.release`, the file a kernel build /// generates — cheaper than re-running `make kernelrelease`, and `None` /// when the build never produced one (or produced an empty one). -// Read by KwActor when writing build records (the build step); kept per -// the CachePolicy precedent (src/lore/application/cache.rs). -#[allow(dead_code)] +// The only production caller is the unix-only actor's build-record +// writer. +#[cfg_attr(not(unix), allow(dead_code))] pub fn read_kernelrelease(fs: &dyn FileSystemTrait, build_root: &Path) -> Option { let release = fs .read_to_string( @@ -451,7 +448,6 @@ pub enum DeployAloneRefusal { /// about the tree's *current* state. [`evaluate_readiness`] conjoins /// [`TreeReadiness`] into its `deploy_alone` verdict; prefer it over /// calling this directly. -#[allow(dead_code)] pub fn check_deploy_alone( record: Option<&KwBuildRecord>, tree: &KernelTree, @@ -511,7 +507,8 @@ pub struct KwReadiness { /// Runs all readiness probes for `tree` and composes them into a /// [`KwReadiness`] snapshot. `head_branch` is the tree's current branch — /// resolving it (via git) is the caller's job, keeping these probes pure. -#[allow(dead_code)] +// The only caller is the unix-only actor's GetReadiness. +#[cfg_attr(not(unix), allow(dead_code))] pub fn evaluate_readiness( fs: &dyn FileSystemTrait, env: &dyn EnvTrait, From 9979cdfbaa6a948cf601057f661e4fed05ad9b8c Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Mon, 24 Aug 2026 14:25:42 -0300 Subject: [PATCH 09/10] docs(kw): describe build jobs without the integration plan This commit drops plan section numbers, KwOps, and comments about deploy that this change does not implement. The remaining comments describe the checkout, argv merge, and apply-blocking behavior as they stand. This commit is part of the kw integration's step 5. Signed-off-by: lorenzoberts Co-authored-by: Cursor --- src/app/mod.rs | 7 +++-- src/kw/actor.rs | 66 ++++++++++++++++------------------------------ src/kw/argv.rs | 16 +++++------ src/kw/messages.rs | 3 +-- 4 files changed, 33 insertions(+), 59 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index 72d623e..7d1a0bf 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -428,10 +428,9 @@ impl App { if patchset_action_selected(details, &PatchsetAction::Apply) { debug!("applying patchset via git-am"); - // A running kw job owns the tree: applying would rewrite the - // branch the job is building under it (integration plan - // §2.1i). No Start can race this check: both paths are - // serialized by the AppActor loop. + // A running kw job owns the tree; applying would rewrite the + // branch it is building. AppActor serializes this with Start, + // so the two cannot race. let popup = match self.kw_job_running_popup().await { Some(popup) => popup, None => self.apply_patchset_popup(details).await, diff --git a/src/kw/actor.rs b/src/kw/actor.rs index 3b23e8b..f2fe004 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -8,14 +8,8 @@ //! [`crate::kw::readiness`] and records applies through the shared //! [`KwHistoryStore`](crate::kw::history::KwHistoryStore). //! -//! Tracked note (same class as the integration plan's §2.1i): the actor -//! runs quick blocking calls inline in its async task — `create_dir_all`, -//! the build-record completion probes, and the history store's atomic -//! writes — per the ConfigActor precedent. The checkout policy's git -//! calls are different: `git switch` rewrites the worktree (seconds on a -//! kernel tree), so the start and restore paths run them behind -//! `spawn_blocking` and the `Start*`/`RestorePreviousBranch` replies stay -//! immediate. +//! Git checkout runs on the blocking pool. `create_dir_all`, history +//! writes, and completion probes run inline on the actor task. use std::{ ops::ControlFlow, @@ -75,7 +69,7 @@ enum JobEvent { /// Where RestorePreviousBranch switches back to: the branch HEAD was on /// when the last job was accepted, and the tree that branch lives in. -/// Session-only (integration plan §2.1a), deliberately not persisted. +/// Session-only, deliberately not persisted. struct RestoreContext { tree_path: String, branch: String, @@ -91,7 +85,7 @@ struct JobState { /// Tree path, kw-env output dir, and build arch as probed at accept /// time. The build runs under these, so the completion record /// describes this snapshot — not whatever the tree's configuration - /// says by the time the job ends (§2.1e drift detection). + /// says by the time the job ends. tree_path: String, output_dir: Option, arch: Option, @@ -269,11 +263,10 @@ impl KwActor { /// /// Refusals, in order: a job already running, no kw binary on PATH, /// unresolvable kw-env state, a tree that fails the readiness probes, - /// a dirty worktree, or a failed branch switch. The argv comes from - /// the reserved-flags merge (§2.1f): patch-hub's own flags win over - /// the request's extra args. A start refused after the branch switch - /// rolls the switch back: a refused start never leaves the tree on a - /// branch the user did not check out. + /// a dirty worktree, or a failed branch switch. Reserved flags on + /// patch-hub's own argv win over the request's extra args. A start + /// refused after the branch switch rolls the switch back: a refused + /// start never leaves the tree on a branch the user did not check out. async fn start_job( &mut self, kind: KwJobKind, @@ -283,10 +276,9 @@ impl KwActor { return Err(KwStartError::JobAlreadyRunning); } - // Hard fail on invoke (integration plan §2.4): with no kw binary on - // PATH no job can run. The version check is advisory only — kw's - // shipped VERSION file is stale, so Below/Unknown are logged, never - // gated. + // Hard fail on invoke: with no kw binary on PATH no job can run. + // The version check is advisory only — kw's shipped VERSION file + // is stale, so Below/Unknown are logged, never gated. let kw_binary = readiness::probe_kw_binary(&*self.env, &*self.shell); if !kw_binary.available { return Err(KwStartError::KwBinaryMissing); @@ -362,15 +354,13 @@ impl KwActor { Ok(()) } - /// The checkout policy (§2.1a): refuse on a dirty worktree, probe the - /// pre-job HEAD, then switch the tree onto the requested branch. The - /// git calls rewrite the worktree — seconds on a kernel tree, not - /// milliseconds — so they run on the blocking pool and the accept - /// reply stays immediate. + /// Refuse a dirty worktree, record HEAD, then `git switch` to the + /// requested branch. The git calls run on the blocking pool so the + /// accept reply stays immediate. /// /// The HEAD probe sits between the dirty check and the switch: it /// must capture the branch the user was on, or RestorePreviousBranch - /// would "restore" the branch the job switched to. An unprobed HEAD + /// would restore the branch the job switched to. An unprobed HEAD /// (detached, or not a git repo) yields `None` rather than a wrong /// branch. async fn checkout_build_branch( @@ -525,16 +515,9 @@ impl KwActor { } } - /// Writes the build record for a finished job (§2.1e): success and - /// failure both — KwOps shows "last build failed" from the stored - /// record, and deploy-alone readiness requires `success == true`. A - /// cancelled job writes nothing: it never completed. History and - /// patchset-link errors are logged, never reported in the job's - /// status — the build's real outcome already reached the user. - /// - /// Build-then-deploy jobs (the deploy step) must instead write this - /// record at the Building → Deploying phase transition, so a failed - /// deploy cannot mask a good build. + /// Persist a finished build (success or failure). Cancel writes + /// nothing. History and patchset-link errors are logged, not folded + /// into job status — the build's real outcome already reached the user. fn record_build_outcome(&self, job: &JobState, outcome: &JobOutcome) { let success = match outcome { JobOutcome::Exited(exit) => exit.success(), @@ -624,7 +607,7 @@ impl KwActor { } /// Switches the tree that ran the last job back to the branch HEAD was - /// on when that job was accepted (§2.1a). Refuses while a job is + /// on when that job was accepted. Refuses while a job is /// running (its branch is in use), when nothing was recorded, and on /// a dirty worktree. Only a successful switch consumes the context — /// a refused restore stays available for a retry. @@ -664,13 +647,10 @@ impl KwActor { } } -/// Fails unless the tree's git state verifies clean (§2.1a). Untracked -/// files don't count: kernel trees accumulate local scratch files, and -/// only tracked changes can corrupt the branch a job builds — an -/// untracked file that would collide with the switch is still caught by -/// git itself. A probe that itself fails — git missing, not a repository -/// — fails too: starting a job or restoring a branch on a tree whose -/// state is unknown could carry unrecorded changes across branches. +/// Fails unless the tree's git state verifies clean. Untracked files +/// don't count: kernel trees accumulate local scratch files, and only +/// tracked changes can corrupt the branch a job builds. A probe that +/// itself fails — git missing, not a repository — fails too. fn check_worktree_clean(shell: &dyn ShellTrait, tree_path: &str) -> Result<(), TreeGitError> { let cmd = ShellCommand::new("git").args([ "-C", diff --git a/src/kw/argv.rs b/src/kw/argv.rs index e122b47..b4b0727 100644 --- a/src/kw/argv.rs +++ b/src/kw/argv.rs @@ -1,9 +1,7 @@ //! kw argv construction: patch-hub's base command lines plus the user -//! extra-args merge in which reserved options always win (integration -//! plan §2.1f). +//! extra-args merge in which reserved options always win. -// The only caller is the unix-only actor until the KwOps screen shows -// the final argv before Start (§2.1f item 6). +// Production caller is the unix-only actor. #![cfg_attr(not(unix), allow(dead_code))] /// A CLI option patch-hub controls: user-supplied extra args that set it @@ -27,10 +25,9 @@ impl ReservedOption { /// Reserved for `kw build`: patch-hub forces non-interactive alerts, owns /// the job's log file (ProcessTrait captures kw's stdout/stderr to it) — -/// a user-supplied `--save-log-to` would fork the log to a second file, -/// leaving KwOps tailing only half the output — and never runs `--menu` -/// from automation (plan §1.1): the job's stdio is a log file, so -/// menuconfig would hang until cancelled. +/// a user-supplied `--save-log-to` would split stdout/stderr away from +/// the job log — and never runs `--menu` from automation: the job's +/// stdio is a log file, so menuconfig would hang until cancelled. const BUILD_RESERVED: &[ReservedOption] = &[ ReservedOption::new(&["--alert"], true), ReservedOption::new(&["--save-log-to"], true), @@ -161,8 +158,7 @@ mod tests { #[test] fn boolean_reserved_strips_only_itself_in_all_spellings() { - // The deploy step's reserved set has booleans with short forms; - // the merge must not eat the token after one. + // A boolean reserved option must not eat the following token. let reserved = [ReservedOption::new(&["--force", "-f"], false)]; assert_eq!( vec!["deploy", "extra"], diff --git a/src/kw/messages.rs b/src/kw/messages.rs index a444ad5..44a067f 100644 --- a/src/kw/messages.rs +++ b/src/kw/messages.rs @@ -24,8 +24,7 @@ pub struct StartRequest { /// before spawning and leaves HEAD there after the job. pub branch: String, /// Extra kw CLI tokens, already whitespace-split by the caller. - /// Reserved options (integration plan §2.1f: `--alert`, - /// `--save-log-to`, and the deploy-time set) are stripped from them — + /// Reserved options (`--alert`, `--save-log-to`) are stripped — /// patch-hub's own argv wins. pub extra_args: Vec, } From 2fe571c09dd4a684581d9e9934c128b51834e44b Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Mon, 24 Aug 2026 15:22:58 -0300 Subject: [PATCH 10/10] fix(kw): drop --alert=n and treat bash SIGTERM exits as cancel This commit stops injecting --alert=n on kw build: lab kw (beta-0.9) rejects unrecognized options as a hard failure, while kw's own default is already alert=n. User --alert extras remain stripped. Cancel after a process-group SIGTERM/SIGKILL now maps exit 143/137 to Cancelled, because the bash kw wrapper often reports those as plain exits rather than WIFSIGNALED. This commit is part of the kw integration's step 5. Signed-off-by: lorenzoberts --- src/kw/actor.rs | 24 ++++++++++++++++-------- src/kw/argv.rs | 42 ++++++++++++++++++++++-------------------- 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/src/kw/actor.rs b/src/kw/actor.rs index f2fe004..3a82131 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -784,10 +784,18 @@ fn outcome_after_cancel(result: Result) -> JobOutcome use std::os::unix::process::ExitStatusExt; match result { - Ok(status) => match status.signal() { - Some(_) => JobOutcome::Cancelled, - None => JobOutcome::Exited(status), - }, + Ok(status) => { + if status.signal().is_some() { + return JobOutcome::Cancelled; + } + // kw is a bash wrapper: a process-group SIGTERM/SIGKILL often + // surfaces as a plain exit of 128+signum (143 / 137) rather than + // WIFSIGNALED. After a cancel request those are still cancels. + match status.code() { + Some(143 | 137) => JobOutcome::Cancelled, + _ => JobOutcome::Exited(status), + } + } Err(error) => JobOutcome::WaitFailed(error), } } @@ -1367,7 +1375,7 @@ mod tests { let spawned = process.spawned(); assert_eq!(1, spawned.len()); assert_eq!("kw", spawned[0].program); - assert_eq!(["build", "--alert=n"], spawned[0].args.as_slice()); + assert_eq!(["build"], spawned[0].args.as_slice()); assert_eq!(Path::new("/home/user/linux"), spawned[0].cwd); assert!(spawned[0].log_path.starts_with(&log_dir)); @@ -1421,10 +1429,10 @@ mod tests { handle.start_build(request).await.unwrap(); let spawned = process.spawned(); - // Reserved options win: the user's --alert and --save-log-to are - // stripped, the rest passes through in order. + // Reserved options are stripped: the user's --alert and --save-log-to + // do not reach kw; the rest passes through in order. assert_eq!( - ["build", "--alert=n", "--verbose", "--ccache"].as_slice(), + ["build", "--verbose", "--ccache"].as_slice(), spawned[0].args.as_slice() ); diff --git a/src/kw/argv.rs b/src/kw/argv.rs index b4b0727..1870364 100644 --- a/src/kw/argv.rs +++ b/src/kw/argv.rs @@ -23,20 +23,25 @@ impl ReservedOption { } } -/// Reserved for `kw build`: patch-hub forces non-interactive alerts, owns -/// the job's log file (ProcessTrait captures kw's stdout/stderr to it) — -/// a user-supplied `--save-log-to` would split stdout/stderr away from -/// the job log — and never runs `--menu` from automation: the job's -/// stdio is a log file, so menuconfig would hang until cancelled. +/// Reserved for `kw build`: patch-hub owns the job's log file +/// (`ProcessTrait` captures kw's stdout/stderr to it) — a user-supplied +/// `--save-log-to` would split stdout/stderr away from the job log — and +/// never runs `--menu` from automation: the job's stdio is a log file, so +/// menuconfig would hang until cancelled. +/// +/// `--alert` is stripped from extras (and not injected on the base argv): +/// kw beta-0.9 (still what many installs report, including this lab) treats +/// unrecognized options as hard failures (`Invalid option`), and the +/// unattended default is already `alert=n` in kw's own config. const BUILD_RESERVED: &[ReservedOption] = &[ ReservedOption::new(&["--alert"], true), ReservedOption::new(&["--save-log-to"], true), ReservedOption::new(&["--menu"], false), ]; -/// The argv for a build job: `kw build --alert=n `. +/// The argv for a build job: `kw build ` (reserved extras stripped). pub fn build_argv(extra_args: &[String]) -> Vec { - merge_extra_args(&["build", "--alert=n"], BUILD_RESERVED, extra_args) + merge_extra_args(&["build"], BUILD_RESERVED, extra_args) } /// Appends user-supplied extra args to `base`, stripping every token that @@ -93,26 +98,26 @@ mod tests { #[test] fn build_argv_without_extras_is_the_base_command() { - assert_eq!(vec!["build", "--alert=n"], build_argv(&[])); + assert_eq!(vec!["build"], build_argv(&[])); } #[test] fn extras_are_appended_in_order() { assert_eq!( - vec!["build", "--alert=n", "--verbose", "--ccache", "-j8"], + vec!["build", "--verbose", "--ccache", "-j8"], build_argv(&extras(&["--verbose", "--ccache", "-j8"])) ); } #[test] - fn reserved_alert_wins_over_user_value_forms() { + fn reserved_alert_is_stripped_from_extras() { assert_eq!( - vec!["build", "--alert=n", "--verbose"], + vec!["build", "--verbose"], build_argv(&extras(&["--alert=vv", "--verbose"])) ); // Separate-token value form: the value token is consumed too. assert_eq!( - vec!["build", "--alert=n", "--verbose"], + vec!["build", "--verbose"], build_argv(&extras(&["--alert", "v", "--verbose"])) ); } @@ -120,11 +125,11 @@ mod tests { #[test] fn save_log_to_is_reserved_in_both_value_forms() { assert_eq!( - vec!["build", "--alert=n"], + vec!["build"], build_argv(&extras(&["--save-log-to=/tmp/x.log"])) ); assert_eq!( - vec!["build", "--alert=n"], + vec!["build"], build_argv(&extras(&["--save-log-to", "/tmp/x.log"])) ); } @@ -133,17 +138,14 @@ mod tests { fn similar_prefix_is_not_reserved() { // `--alertness` only shares a prefix with `--alert`. assert_eq!( - vec!["build", "--alert=n", "--alertness"], + vec!["build", "--alertness"], build_argv(&extras(&["--alertness"])) ); } #[test] fn trailing_reserved_option_without_value_is_stripped() { - assert_eq!( - vec!["build", "--alert=n"], - build_argv(&extras(&["--alert"])) - ); + assert_eq!(vec!["build"], build_argv(&extras(&["--alert"]))); } #[test] @@ -151,7 +153,7 @@ mod tests { // kw build --menu would open menuconfig with the job's stdio // redirected to a log file — a hang, not a build. assert_eq!( - vec!["build", "--alert=n", "--verbose"], + vec!["build", "--verbose"], build_argv(&extras(&["--menu", "--verbose"])) ); }