From 270af7017c5be7a3fbf6165bd4fd202b344e93c2 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 13 Aug 2026 17:08:01 -0300 Subject: [PATCH 1/7] feat(config): add stay_on_applied_branch setting This commit adds a configuration knob, defaulting to true, so apply can leave HEAD on the new patchset branch. The setting goes through the existing edit-config round-trip, and existing config files pick up the default without a migration. This commit is part of the kw integration's step 2. Signed-off-by: lorenzoberts --- src/app/screens/edit_config.rs | 13 +++++++++ src/config/actor.rs | 2 +- src/config/errors.rs | 2 ++ src/config/handle.rs | 7 +++-- src/config/messages.rs | 4 ++- src/config/service.rs | 10 +++++++ src/config/state.rs | 11 ++++++++ src/config/tests.rs | 48 ++++++++++++++++++++++++++++++++-- src/config/update.rs | 2 ++ 9 files changed, 93 insertions(+), 6 deletions(-) diff --git a/src/app/screens/edit_config.rs b/src/app/screens/edit_config.rs index ef1e825c..65b50ad5 100644 --- a/src/app/screens/edit_config.rs +++ b/src/app/screens/edit_config.rs @@ -41,6 +41,10 @@ impl EditConfigState { config.cover_renderer().to_string(), ); config_buffer.insert(EditableConfig::MaxLogAge, config.max_log_age().to_string()); + config_buffer.insert( + EditableConfig::StayOnAppliedBranch, + config.stay_on_applied_branch().to_string(), + ); EditConfigState { config_buffer, @@ -137,6 +141,10 @@ impl EditConfigState { .get(&EditableConfig::CoverRenderer) .cloned(), max_log_age: self.config_buffer.get(&EditableConfig::MaxLogAge).cloned(), + stay_on_applied_branch: self + .config_buffer + .get(&EditableConfig::StayOnAppliedBranch) + .cloned(), } } } @@ -151,6 +159,7 @@ enum EditableConfig { PatchRenderer, CoverRenderer, MaxLogAge, + StayOnAppliedBranch, } impl TryFrom for EditableConfig { @@ -166,6 +175,7 @@ impl TryFrom for EditableConfig { 5 => Ok(EditableConfig::PatchRenderer), 6 => Ok(EditableConfig::CoverRenderer), 7 => Ok(EditableConfig::MaxLogAge), + 8 => Ok(EditableConfig::StayOnAppliedBranch), _ => bail!("Invalid index {} for EditableConfig", value), // Handle out of bounds } } @@ -186,6 +196,9 @@ impl Display for EditableConfig { EditableConfig::GitSendEmailOpt => write!(f, "`git send email` option"), EditableConfig::MaxLogAge => write!(f, "Max Log Age (0 = forever)"), EditableConfig::GitAmOpt => write!(f, "`git am` option"), + EditableConfig::StayOnAppliedBranch => { + write!(f, "Stay On Applied Branch (true/false)") + } } } } diff --git a/src/config/actor.rs b/src/config/actor.rs index ccb2b50e..e321fc3f 100644 --- a/src/config/actor.rs +++ b/src/config/actor.rs @@ -72,7 +72,7 @@ where ControlFlow::Continue(()) } ConfigMessage::ValidateAndApply { draft, reply } => { - let result = self.apply(draft); + let result = self.apply(*draft); send_config_reply(message_name, reply, result); ControlFlow::Continue(()) } diff --git a/src/config/errors.rs b/src/config/errors.rs index 42478c17..8a95f358 100644 --- a/src/config/errors.rs +++ b/src/config/errors.rs @@ -18,6 +18,8 @@ pub enum ConfigError { InvalidCoverRenderer(String), #[error("invalid max log age: {0}")] InvalidMaxLogAge(String), + #[error("invalid stay-on-applied-branch value: {0}")] + InvalidStayOnAppliedBranch(String), #[error("filesystem error: {0}")] Fs(#[from] FileSystemError), } diff --git a/src/config/handle.rs b/src/config/handle.rs index 6abfe5e8..a0ae5dd4 100644 --- a/src/config/handle.rs +++ b/src/config/handle.rs @@ -24,8 +24,11 @@ impl ConfigHandle { &self, draft: ConfigUpdateDraft, ) -> ConfigResult { - self.request_result(|reply| ConfigMessage::ValidateAndApply { draft, reply }) - .await + self.request_result(|reply| ConfigMessage::ValidateAndApply { + draft: Box::new(draft), + reply, + }) + .await } /// Signals the actor to stop processing messages and exit its run loop. diff --git a/src/config/messages.rs b/src/config/messages.rs index 94bee87c..72e1ef7e 100644 --- a/src/config/messages.rs +++ b/src/config/messages.rs @@ -9,7 +9,9 @@ pub enum ConfigMessage { reply: oneshot::Sender, }, ValidateAndApply { - draft: ConfigUpdateDraft, + // Boxed to keep the enum small (clippy::large_enum_variant): the draft + // holds one `Option` per editable field. + draft: Box, reply: oneshot::Sender>, }, Shutdown, diff --git a/src/config/service.rs b/src/config/service.rs index d1e54ee3..331049fe 100644 --- a/src/config/service.rs +++ b/src/config/service.rs @@ -133,6 +133,15 @@ pub(crate) fn validate_update( ), }; + let stay_on_applied_branch = match &draft.stay_on_applied_branch { + None => None, + Some(s) => Some( + s.trim() + .parse::() + .map_err(|_| ConfigError::InvalidStayOnAppliedBranch(s.clone()))?, + ), + }; + Ok(ValidatedConfigUpdate { page_size, cache_dir, @@ -142,6 +151,7 @@ pub(crate) fn validate_update( patch_renderer, cover_renderer, max_log_age, + stay_on_applied_branch, }) } diff --git a/src/config/state.rs b/src/config/state.rs index 963d9659..89341ab9 100644 --- a/src/config/state.rs +++ b/src/config/state.rs @@ -38,6 +38,7 @@ pub struct ConfigState { pub(crate) target_kernel_tree: Option, pub(crate) git_am_options: String, pub(crate) git_am_branch_prefix: String, + pub(crate) stay_on_applied_branch: bool, } impl Default for ConfigState { @@ -79,6 +80,7 @@ impl ConfigState { target_kernel_tree: None, git_am_options: String::new(), git_am_branch_prefix: String::from("patchset-"), + stay_on_applied_branch: true, } } @@ -124,6 +126,10 @@ impl ConfigState { self.max_log_age = max_log_age; } + fn set_stay_on_applied_branch(&mut self, stay_on_applied_branch: bool) { + self.stay_on_applied_branch = stay_on_applied_branch; + } + /// Merges validated field updates from the edit-config flow. pub fn apply_update(&mut self, u: &ValidatedConfigUpdate) { if let Some(page_size) = u.page_size { @@ -150,6 +156,9 @@ impl ConfigState { if let Some(max_log_age) = u.max_log_age { self.set_max_log_age(max_log_age); } + if let Some(stay_on_applied_branch) = u.stay_on_applied_branch { + self.set_stay_on_applied_branch(stay_on_applied_branch); + } } pub fn to_snapshot(&self) -> ConfigSnapshot { @@ -189,6 +198,7 @@ pub struct ConfigSnapshot { target_kernel_tree: Option, git_am_options: String, git_am_branch_prefix: String, + stay_on_applied_branch: bool, } impl ConfigSnapshot { @@ -210,6 +220,7 @@ impl ConfigSnapshot { target_kernel_tree: s.target_kernel_tree.clone(), git_am_options: s.git_am_options.clone(), git_am_branch_prefix: s.git_am_branch_prefix.clone(), + stay_on_applied_branch: s.stay_on_applied_branch, } } diff --git a/src/config/tests.rs b/src/config/tests.rs index db9a5931..34c0e717 100644 --- a/src/config/tests.rs +++ b/src/config/tests.rs @@ -11,7 +11,10 @@ use crate::config::actor::ConfigActor; use crate::config::repository::{ConfigRepository, JsonConfigRepository}; use crate::config::service::{bootstrap_parts, validate_update}; use crate::config::state::{normalize_derived_paths, ConfigState}; -use crate::config::{ConfigError, ConfigSnapshot, ConfigUpdateDraft, DEFAULT_CONFIG_PATH_SUFFIX}; +use crate::config::{ + ConfigError, ConfigSnapshot, ConfigUpdateDraft, ValidatedConfigUpdate, + DEFAULT_CONFIG_PATH_SUFFIX, +}; use crate::infrastructure::{ env::{EnvTrait, MockEnvTrait}, file_system::OsFileSystem, @@ -98,7 +101,8 @@ fn config_fixture_json(root: &Path) -> String { }, "target_kernel_tree": "linux", "git_am_options": "--foo-bar foobar -s -n -o -r -l -a -x", - "git_am_branch_prefix": "really-creative-prefix-" + "git_am_branch_prefix": "really-creative-prefix-", + "stay_on_applied_branch": false }); serde_json::to_string_pretty(&v).unwrap() } @@ -139,6 +143,7 @@ fn bootstrap_with_default_values() { assert!(config.target_kernel_tree().is_none()); assert_eq!("", config.git_am_options().as_str()); assert_eq!("patchset-", config.git_am_branch_prefix().as_str()); + assert!(config.stay_on_applied_branch()); } #[test] @@ -224,6 +229,7 @@ fn bootstrap_with_config_file() { "really-creative-prefix-", config.git_am_branch_prefix().as_str() ); + assert!(!config.stay_on_applied_branch()); } #[test] @@ -366,6 +372,9 @@ fn deserialize_config_state_with_missing_field() { assert_eq!(state.page_size(), 30); assert_eq!(state.max_log_age(), 500); + // Missing fields fall back to the compiled-in defaults; in particular the + // kw-integration apply toggle defaults to staying on the applied branch. + assert!(state.stay_on_applied_branch()); } #[test] @@ -531,6 +540,41 @@ fn validate_update_rejects_invalid_max_log_age() { )); } +#[test] +fn validate_update_rejects_invalid_stay_on_applied_branch() { + for raw in ["not-a-bool", ""] { + let err = validate_update( + ConfigUpdateDraft { + stay_on_applied_branch: Some(raw.into()), + ..Default::default() + }, + &os_fs(), + ) + .unwrap_err(); + assert!(matches!( + err, + ConfigError::InvalidStayOnAppliedBranch(ref s) if s == raw + )); + } +} + +#[test] +fn apply_update_toggles_stay_on_applied_branch() { + let (env, _home) = default_env(); + let mut state = ConfigState::new_with_defaults(&env); + assert!(state.stay_on_applied_branch()); + + state.apply_update(&ValidatedConfigUpdate { + stay_on_applied_branch: Some(false), + ..Default::default() + }); + assert!(!state.stay_on_applied_branch()); + + // A draft that omits the field leaves the current value untouched. + state.apply_update(&ValidatedConfigUpdate::default()); + assert!(!state.stay_on_applied_branch()); +} + #[test] fn validate_update_rejects_cache_dir_that_is_existing_file() { let root = unique_test_dir("not-a-dir"); diff --git a/src/config/update.rs b/src/config/update.rs index eab002e5..995a9c16 100644 --- a/src/config/update.rs +++ b/src/config/update.rs @@ -11,6 +11,7 @@ pub struct ConfigUpdateDraft { pub patch_renderer: Option, pub cover_renderer: Option, pub max_log_age: Option, + pub stay_on_applied_branch: Option, } /// Parsed and validated update ready to merge into [`crate::config::ConfigState`](super::state::ConfigState). @@ -24,4 +25,5 @@ pub struct ValidatedConfigUpdate { pub patch_renderer: Option, pub cover_renderer: Option, pub max_log_age: Option, + pub stay_on_applied_branch: Option, } From 36f8af9acbd3234bbc19df023daf45a550b26318 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 13 Aug 2026 17:16:35 -0300 Subject: [PATCH 2/7] feat(app): stay on applied branch after successful git am by default This commit makes apply honor stay_on_applied_branch. After a successful git am, patch-hub leaves HEAD on the new patchset branch so kw build can run from the applied state without an extra checkout; setting the option to false keeps the previous switch-back. Failed applies still abort and restore the original branch. This commit is part of the kw integration's step 2. Signed-off-by: lorenzoberts --- src/app/actions/apply.rs | 153 +++++++++++++----- src/app/actions/mod.rs | 4 +- src/app/integration_tests/patchset_actions.rs | 60 ++++++- src/app/mod.rs | 2 +- 4 files changed, 173 insertions(+), 46 deletions(-) diff --git a/src/app/actions/apply.rs b/src/app/actions/apply.rs index 6db047b7..382459fe 100644 --- a/src/app/actions/apply.rs +++ b/src/app/actions/apply.rs @@ -15,12 +15,21 @@ pub(crate) struct ApplyPatchsetRequest { pub patchset_path: String, } +#[derive(Debug)] +pub(crate) struct AppliedPatchset { + pub message: String, + // Consumed by the apply-history record write (kw integration); no + // production reader exists until that wiring lands. + #[allow(dead_code)] + pub applied_branch: String, +} + pub(crate) fn apply_patchset( request: &ApplyPatchsetRequest, fs: &dyn FileSystemTrait, shell: &dyn ShellTrait, config: &ConfigSnapshot, -) -> Result { +) -> Result { let kernel_tree = validate_kernel_tree(fs, config)?; check_git_state(fs, shell, kernel_tree)?; @@ -28,17 +37,32 @@ pub(crate) fn apply_patchset( let target_branch = create_target_branch(shell, kernel_tree, config)?; let git_am_result = run_git_am(request, shell, kernel_tree, config); - switch_to_branch(shell, kernel_tree, &original_branch)?; match git_am_result { - Ok(_) => Ok(format!( - " Patchset '{}' applied successfully!\n\n - Kernel Tree: '{}'\n\n - Base Branch: '{}'\n\n - Applied branch: '{}'", - request.patch_title, - kernel_tree.path(), - kernel_tree.branch(), - &target_branch - )), - Err(e) => Err(format!(" `git am` failed\n{}{}", &original_branch, e)), + Ok(_) => { + let current_branch = if config.stay_on_applied_branch() { + target_branch.clone() + } else { + switch_to_branch(shell, kernel_tree, &original_branch)?; + original_branch + }; + + Ok(AppliedPatchset { + message: format!( + " Patchset '{}' applied successfully!\n\n - Kernel Tree: '{}'\n\n - Base Branch: '{}'\n\n - Applied branch: '{}'\n\n - Current branch: '{}'", + request.patch_title, + kernel_tree.path(), + kernel_tree.branch(), + &target_branch, + current_branch + ), + applied_branch: target_branch, + }) + } + Err(e) => { + switch_to_branch(shell, kernel_tree, &original_branch)?; + Err(format!(" `git am` failed\n{}{}", &original_branch, e)) + } } } @@ -250,6 +274,8 @@ mod tests { const BASE_BRANCH: &str = "main"; const PATCHSET_PATH: &str = "/tmp/patchset.mbx"; + // `stay_on_applied_branch` is deliberately absent so the tests below + // exercise the serde default (true) that existing config files inherit. fn config() -> ConfigSnapshot { serde_json::from_value::(serde_json::json!({ "kernel_trees": { @@ -266,6 +292,23 @@ mod tests { .to_snapshot() } + fn config_stay_disabled() -> ConfigSnapshot { + serde_json::from_value::(serde_json::json!({ + "kernel_trees": { + "linux": { + "path": KERNEL_TREE_PATH, + "branch": BASE_BRANCH + } + }, + "target_kernel_tree": "linux", + "git_am_options": "--signoff --3way", + "git_am_branch_prefix": "patchset-", + "stay_on_applied_branch": false + })) + .expect("test config should deserialize") + .to_snapshot() + } + fn config_without_target() -> ConfigSnapshot { ConfigState::default().to_snapshot() } @@ -327,7 +370,7 @@ mod tests { } #[test] - fn apply_success_runs_expected_git_sequence() { + fn apply_success_stays_on_applied_branch_by_default() { let fs = clean_fs(); let (shell, calls) = shell_with_outputs(vec![ output("", "", true), @@ -336,14 +379,20 @@ mod tests { output("", "", true), output("", "", true), output("", "", true), - output("", "", true), ]); - let result = apply_patchset(&request(), &fs, &shell, &config()).unwrap(); + let applied = apply_patchset(&request(), &fs, &shell, &config()).unwrap(); - assert!(result.contains("Patchset '[PATCH] test' applied successfully")); - assert!(result.contains("Applied branch: 'patchset-")); + assert!(applied.applied_branch.starts_with("patchset-")); + assert!(applied + .message + .contains("Patchset '[PATCH] test' applied successfully")); + assert!(applied.message.contains("Applied branch: 'patchset-")); + assert!(applied + .message + .contains(&format!("Current branch: '{}'", applied.applied_branch))); let calls = calls.lock().unwrap(); + assert_eq!(6, calls.len()); assert_eq!( &calls[0], &command(&["git", "-C", KERNEL_TREE_PATH, "status", "--porcelain"]) @@ -392,6 +441,26 @@ mod tests { "--3way" ]) ); + } + + #[test] + fn apply_success_switches_back_when_stay_disabled() { + let fs = clean_fs(); + let (shell, calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + ]); + + let applied = apply_patchset(&request(), &fs, &shell, &config_stay_disabled()).unwrap(); + + assert!(applied.message.contains("Current branch: 'feature'")); + let calls = calls.lock().unwrap(); + assert_eq!(7, calls.len()); assert_eq!( &calls[6], &command(&["git", "-C", KERNEL_TREE_PATH, "switch", "feature"]) @@ -438,31 +507,33 @@ mod tests { #[test] fn failed_git_am_aborts_and_switches_back() { - let fs = clean_fs(); - let (shell, calls) = shell_with_outputs(vec![ - output("", "", true), - output("", "", true), - output("feature\n", "", true), - output("", "", true), - output("", "", true), - output("", "apply failed", false), - output("", "", true), - output("", "", true), - ]); - - let result = apply_patchset(&request(), &fs, &shell, &config()).unwrap_err(); - - assert!(result.contains("`git am` failed")); - assert!(result.contains("feature")); - assert!(result.contains("apply failed")); - let calls = calls.lock().unwrap(); - assert_eq!( - &calls[6], - &command(&["git", "-C", KERNEL_TREE_PATH, "am", "--abort"]) - ); - assert_eq!( - &calls[7], - &command(&["git", "-C", KERNEL_TREE_PATH, "switch", "feature"]) - ); + for config in [config(), config_stay_disabled()] { + let fs = clean_fs(); + let (shell, calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "apply failed", false), + output("", "", true), + output("", "", true), + ]); + + let result = apply_patchset(&request(), &fs, &shell, &config).unwrap_err(); + + assert!(result.contains("`git am` failed")); + assert!(result.contains("feature")); + assert!(result.contains("apply failed")); + let calls = calls.lock().unwrap(); + assert_eq!( + &calls[6], + &command(&["git", "-C", KERNEL_TREE_PATH, "am", "--abort"]) + ); + assert_eq!( + &calls[7], + &command(&["git", "-C", KERNEL_TREE_PATH, "switch", "feature"]) + ); + } } } diff --git a/src/app/actions/mod.rs b/src/app/actions/mod.rs index 82d0967e..ee35f68a 100644 --- a/src/app/actions/mod.rs +++ b/src/app/actions/mod.rs @@ -9,7 +9,7 @@ use crate::{ lore::application::handle::LoreApiHandle, }; -use apply::ApplyPatchsetRequest; +use apply::{AppliedPatchset, ApplyPatchsetRequest}; use reviewed_reply::{ReviewedReplyRequest, ReviewedReplyResult}; pub(crate) struct PatchsetActionService<'a> { @@ -35,7 +35,7 @@ impl<'a> PatchsetActionService<'a> { &self, request: &ApplyPatchsetRequest, config: &ConfigSnapshot, - ) -> Result { + ) -> Result { apply::apply_patchset(request, self.fs, self.shell, config) } diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs index 31f3eae5..5739489e 100644 --- a/src/app/integration_tests/patchset_actions.rs +++ b/src/app/integration_tests/patchset_actions.rs @@ -47,7 +47,6 @@ async fn apply_success_sets_success_popup_and_resets_apply_action() { output("", "", true), output("", "", true), output("", "", true), - output("", "", true), ]); let mut app = app_with_apply_details(clean_fs(), shell); @@ -61,10 +60,45 @@ async fn apply_success_sets_success_popup_and_resets_apply_action() { "applied successfully", "Kernel Tree: '/kernel'", "Applied branch: 'patchset-", + "Current branch: 'patchset-", ], ); } +#[tokio::test] +async fn apply_success_switches_back_when_stay_disabled() { + let (shell, calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + ]); + let mut app = app_with_details( + clean_fs(), + shell, + lore_handle_with_persistence(), + apply_details_state(), + apply_config_stay_disabled(), + ); + + app.consolidate_patchset_actions().await.unwrap(); + + assert_apply_action(&app, false); + assert_info_popup_contains( + app.state.popup.as_ref(), + "Patchset Apply Success", + &["Current branch: 'feature'"], + ); + let calls = calls.lock().unwrap(); + assert_eq!( + command(&["git", "-C", KERNEL_TREE_PATH, "switch", "feature"]), + calls[6] + ); +} + #[tokio::test] async fn apply_failure_sets_failure_popup_and_resets_apply_action() { let (shell, calls) = shell_with_outputs(vec![ @@ -161,6 +195,7 @@ fn app_with_apply_details(fs: MockFileSystemTrait, shell: MockShellTrait) -> App shell, lore_handle_with_persistence(), apply_details_state(), + apply_config(), ) } @@ -170,6 +205,7 @@ fn app_with_reviewed_reply_details(shell: MockShellTrait, lore_api: LoreApiHandl shell, lore_api, reviewed_reply_details_state(), + apply_config(), ) } @@ -178,9 +214,10 @@ fn app_with_details( shell: MockShellTrait, lore_api: LoreApiHandle, details: PatchsetDetailsState, + config: ConfigSnapshot, ) -> App { let mut app = App::new( - apply_config(), + config, dummy_config_handle(), BootstrapLoreData { mailing_lists: vec![sample_mailing_list()], @@ -258,6 +295,8 @@ fn reviewed_reply_lore_handle(saved_reviewed: SharedReviewedState) -> LoreApiHan LoreApiHandle::new(tx) } +// `stay_on_applied_branch` is deliberately absent so the tests exercise the +// serde default (true) that existing config files inherit. fn apply_config() -> ConfigSnapshot { serde_json::from_value::(serde_json::json!({ "kernel_trees": { @@ -274,6 +313,23 @@ fn apply_config() -> ConfigSnapshot { .to_snapshot() } +fn apply_config_stay_disabled() -> ConfigSnapshot { + serde_json::from_value::(serde_json::json!({ + "kernel_trees": { + "linux": { + "path": KERNEL_TREE_PATH, + "branch": BASE_BRANCH + } + }, + "target_kernel_tree": "linux", + "git_am_options": "--signoff --3way", + "git_am_branch_prefix": "patchset-", + "stay_on_applied_branch": false + })) + .expect("test config should deserialize") + .to_snapshot() +} + fn clean_fs() -> MockFileSystemTrait { let mut fs = MockFileSystemTrait::new(); fs.expect_is_dir() diff --git a/src/app/mod.rs b/src/app/mod.rs index 39edf715..887d3130 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -414,7 +414,7 @@ impl App { &self.services.lore_api, ); let popup = match action_service.apply_patchset(&request, &self.state.config) { - Ok(msg) => popup::AppPopup::info("Patchset Apply Success", msg), + Ok(applied) => popup::AppPopup::info("Patchset Apply Success", applied.message), Err(msg) => popup::AppPopup::info("Patchset Apply Fail", msg), }; From 96539547ba13ca1cc4f80fccf7ef83c631eecfa1 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 13 Aug 2026 17:19:45 -0300 Subject: [PATCH 3/7] feat(kw): add KwHistoryStore with atomic apply-history persistence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit introduces the kw module's apply-history store. Each successful git am is recorded as user state — message id, tree, and branch snapshot — in kw_apply_history.json, using the same atomic write pattern as the other JSON repositories. A missing file reads as empty; a corrupt file errors instead of being silently overwritten. This commit is part of the kw integration's step 2. Signed-off-by: lorenzoberts --- src/kw/history.rs | 260 ++++++++++++++++++++++++++++++++++++++++++++++ src/kw/mod.rs | 4 + src/main.rs | 1 + 3 files changed, 265 insertions(+) create mode 100644 src/kw/history.rs create mode 100644 src/kw/mod.rs diff --git a/src/kw/history.rs b/src/kw/history.rs new file mode 100644 index 00000000..d5445242 --- /dev/null +++ b/src/kw/history.rs @@ -0,0 +1,260 @@ +//! User-local history of patchset applies (and, in later steps, kw builds), +//! stored as JSON under the configured `data_dir`. +//! +//! Apply records feed kw build/deploy readiness and the KwOps branch prefill, +//! so they are user state — not a cache — and are never refreshed from lore. + +use mockall::automock; +use serde::{Deserialize, Serialize}; +use serde_json::{from_reader, to_writer_pretty}; + +use std::{collections::HashMap, io, path::Path, sync::Arc}; + +use crate::infrastructure::file_system::{FileSystemError, FileSystemTrait}; + +// No production caller exists until the apply hook wires the store into the +// app; kept per the CachePolicy precedent (src/lore/application/cache.rs). +#[allow(dead_code)] +pub const APPLY_HISTORY_FILENAME: &str = "kw_apply_history.json"; + +/// One recorded `git am` application of a lore patchset to a kernel tree. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct KwApplyRecord { + pub message_id: String, + pub kernel_tree_id: String, + /// Snapshot of `KernelTree.path` when the record was written, so later + /// readiness checks can detect the tree being repointed or moved. + pub tree_path: String, + pub applied_branch: String, + pub base_branch: String, + /// RFC3339 timestamp. + pub applied_at: String, +} + +// The trait's production caller is the apply hook (and, later, KwActor); the +// read side serves readiness/prefill in later steps. Kept per the CachePolicy +// precedent (src/lore/application/cache.rs). +#[allow(dead_code)] +#[automock] +pub trait KwHistoryStore: Send + Sync { + /// Inserts or replaces the apply record keyed by `record.message_id`. + fn record_apply(&self, record: KwApplyRecord) -> Result<(), FileSystemError>; + + /// Returns the apply record for `message_id`, or `None` if it was never + /// recorded. A missing history file is a normal state, not an error. + fn apply_record(&self, message_id: &str) -> Result, FileSystemError>; +} + +pub struct FileKwHistoryStore { + fs: Arc, + apply_history_path: String, +} + +impl FileKwHistoryStore { + // No production caller exists until the apply hook wires the store into + // the app; kept per the CachePolicy precedent. + #[allow(dead_code)] + pub fn new(fs: Arc, apply_history_path: String) -> Self { + FileKwHistoryStore { + fs, + apply_history_path, + } + } + + fn load_apply_records(&self) -> Result, FileSystemError> { + let path = Path::new(&self.apply_history_path); + if !self.fs.is_file(path) { + return Ok(HashMap::new()); + } + let reader = self.fs.open_bufreader(path)?; + // A corrupt file is an error rather than an empty map: history must + // never be silently clobbered by the next write. + from_reader(reader) + .map_err(io::Error::from) + .map_err(FileSystemError::from) + } + + /// Mirrors `FileLorePersistence::atomic_write_json` + /// (src/lore/infrastructure/persistence.rs). + fn atomic_write_json( + &self, + value: &T, + path: &str, + ) -> Result<(), FileSystemError> { + if let Some(parent) = Path::new(path).parent() { + self.fs.create_dir_all(parent)?; + } + + let tmp_path = format!("{path}.tmp"); + { + let writer = self.fs.create_writer(Path::new(&tmp_path))?; + to_writer_pretty(writer, value).map_err(io::Error::from)?; + } + self.fs.rename(Path::new(&tmp_path), Path::new(path))?; + Ok(()) + } +} + +impl KwHistoryStore for FileKwHistoryStore { + fn record_apply(&self, record: KwApplyRecord) -> Result<(), FileSystemError> { + let mut records = self.load_apply_records()?; + records.insert(record.message_id.clone(), record); + self.atomic_write_json(&records, &self.apply_history_path) + } + + fn apply_record(&self, message_id: &str) -> Result, FileSystemError> { + Ok(self.load_apply_records()?.remove(message_id)) + } +} + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::PathBuf; + use std::sync::atomic::{AtomicU64, Ordering}; + + use crate::infrastructure::file_system::OsFileSystem; + + use super::*; + + static TEST_SEQ: AtomicU64 = AtomicU64::new(0); + + fn tmp_dir(test_name: &str) -> PathBuf { + let n = TEST_SEQ.fetch_add(1, Ordering::SeqCst); + let dir = std::env::temp_dir().join(format!( + "patch-hub-kw-history-{}-{}-{}", + test_name, + std::process::id(), + n + )); + fs::create_dir_all(&dir).unwrap(); + dir + } + + fn store_at(dir: &Path) -> FileKwHistoryStore { + FileKwHistoryStore::new( + Arc::new(OsFileSystem), + dir.join(APPLY_HISTORY_FILENAME) + .to_str() + .unwrap() + .to_string(), + ) + } + + fn record(message_id: &str, branch: &str) -> KwApplyRecord { + KwApplyRecord { + message_id: message_id.to_string(), + kernel_tree_id: "mainline".to_string(), + tree_path: "/home/user/linux".to_string(), + applied_branch: branch.to_string(), + base_branch: "master".to_string(), + applied_at: "2026-08-01T17:30:00Z".to_string(), + } + } + + #[test] + fn record_and_read_round_trip() { + let dir = tmp_dir("round-trip"); + let store = store_at(&dir); + + store + .record_apply(record("msg-1", "patchset-2026-08-01-17-30-00")) + .unwrap(); + store + .record_apply(record("msg-2", "patchset-2026-08-02-10-00-00")) + .unwrap(); + + assert_eq!( + Some(record("msg-1", "patchset-2026-08-01-17-30-00")), + store.apply_record("msg-1").unwrap() + ); + assert_eq!( + Some(record("msg-2", "patchset-2026-08-02-10-00-00")), + store.apply_record("msg-2").unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn record_with_same_message_id_overwrites() { + let dir = tmp_dir("overwrite"); + let store = store_at(&dir); + + store.record_apply(record("msg-1", "patchset-old")).unwrap(); + store.record_apply(record("msg-1", "patchset-new")).unwrap(); + + assert_eq!( + Some(record("msg-1", "patchset-new")), + store.apply_record("msg-1").unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn missing_history_file_reads_as_empty() { + let dir = tmp_dir("missing"); + let store = store_at(&dir); + + assert_eq!(None, store.apply_record("msg-1").unwrap()); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn record_creates_parent_directories() { + let dir = tmp_dir("parents"); + let store = FileKwHistoryStore::new( + Arc::new(OsFileSystem), + dir.join("nested") + .join("deeper") + .join(APPLY_HISTORY_FILENAME) + .to_str() + .unwrap() + .to_string(), + ); + + store.record_apply(record("msg-1", "patchset-x")).unwrap(); + + assert_eq!( + Some(record("msg-1", "patchset-x")), + store.apply_record("msg-1").unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn corrupt_history_file_errors_instead_of_clobbering() { + let dir = tmp_dir("corrupt"); + let store = store_at(&dir); + fs::write(dir.join(APPLY_HISTORY_FILENAME), b"not json").unwrap(); + + assert!(store.apply_record("msg-1").is_err()); + assert!(store.record_apply(record("msg-1", "patchset-x")).is_err()); + // The corrupt file is left untouched for the user to inspect. + assert_eq!( + "not json", + fs::read_to_string(dir.join(APPLY_HISTORY_FILENAME)).unwrap() + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn atomic_write_leaves_no_tmp_file() { + let dir = tmp_dir("atomic"); + let store = store_at(&dir); + + store.record_apply(record("msg-1", "patchset-x")).unwrap(); + + let tmp_left = fs::read_dir(&dir).unwrap().any(|e| { + e.ok() + .is_some_and(|x| x.file_name().to_string_lossy().ends_with(".tmp")) + }); + assert!(!tmp_left, "atomic write should rename away .tmp"); + + fs::remove_dir_all(&dir).unwrap(); + } +} diff --git a/src/kw/mod.rs b/src/kw/mod.rs new file mode 100644 index 00000000..4d881716 --- /dev/null +++ b/src/kw/mod.rs @@ -0,0 +1,4 @@ +//! kw integration: persistence and (in later steps) the actor orchestrating +//! `kw build` / `kw deploy` jobs. + +pub mod history; diff --git a/src/main.rs b/src/main.rs index 211efb67..5977be91 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,6 +3,7 @@ mod cli; mod config; mod infrastructure; mod input; +mod kw; mod lore; mod macros; mod render; From d81b017ccc19ffe4a93431b2c72fe92e8a401ada Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 13 Aug 2026 17:27:19 -0300 Subject: [PATCH 4/7] feat(app): record kw apply history on successful apply This commit writes an apply record after a successful git am so the applied patchset, tree, and branch are durable across sessions. A history-write failure does not turn a successful apply into a reported failure: the success popup stays and the error is traced. The store is injected as a shared instance through AppServices. This commit is part of the kw integration's step 2. Signed-off-by: lorenzoberts --- src/app/actions/apply.rs | 3 - src/app/actor.rs | 3 + .../integration_tests/helpers/app_harness.rs | 4 + src/app/integration_tests/patchset_actions.rs | 117 +++++++++++++++++- src/app/mod.rs | 58 ++++++++- src/kw/history.rs | 13 +- src/main.rs | 6 + 7 files changed, 188 insertions(+), 16 deletions(-) diff --git a/src/app/actions/apply.rs b/src/app/actions/apply.rs index 382459fe..bca38493 100644 --- a/src/app/actions/apply.rs +++ b/src/app/actions/apply.rs @@ -18,9 +18,6 @@ pub(crate) struct ApplyPatchsetRequest { #[derive(Debug)] pub(crate) struct AppliedPatchset { pub message: String, - // Consumed by the apply-history record write (kw integration); no - // production reader exists until that wiring lands. - #[allow(dead_code)] pub applied_branch: String, } diff --git a/src/app/actor.rs b/src/app/actor.rs index 3e44bb90..497666a9 100644 --- a/src/app/actor.rs +++ b/src/app/actor.rs @@ -168,6 +168,7 @@ mod tests { config::{ConfigHandle, ConfigState}, infrastructure::{file_system::MockFileSystemTrait, shell::MockShellTrait}, input::{event::InputEvent, handle::InputHandle, messages::InputMessage}, + kw::history::MockKwHistoryStore, lore::{ application::{ actor::LoreApiActor, cache::CacheTtl, handle::LoreApiHandle, service::LoreService, @@ -232,6 +233,7 @@ mod tests { shell: Box::new(MockShellTrait::new()), fs: Box::new(MockFileSystemTrait::new()), config: dummy_config_handle(), + kw_history: Arc::new(MockKwHistoryStore::new()), }, } } @@ -333,6 +335,7 @@ mod tests { Box::new(MockShellTrait::new()), lore_api.clone(), render.clone(), + Arc::new(MockKwHistoryStore::new()), ) .expect("App::new must succeed"); diff --git a/src/app/integration_tests/helpers/app_harness.rs b/src/app/integration_tests/helpers/app_harness.rs index 24f1dffb..ba7531fa 100644 --- a/src/app/integration_tests/helpers/app_harness.rs +++ b/src/app/integration_tests/helpers/app_harness.rs @@ -1,9 +1,12 @@ +use std::sync::Arc; + use tokio::sync::mpsc; use crate::{ app::App, config::{ConfigHandle, ConfigState}, infrastructure::{file_system::MockFileSystemTrait, shell::MockShellTrait}, + kw::history::MockKwHistoryStore, lore::application::{cache::BootstrapLoreData, handle::LoreApiHandle}, render::handle::RenderHandle, terminal::{handle::TerminalHandle, messages::TerminalMessage}, @@ -52,6 +55,7 @@ pub(crate) fn app_with_bootstrap_and_handles( Box::new(MockShellTrait::new()), lore_api, render, + Arc::new(MockKwHistoryStore::new()), ) .expect("minimal app should build") } diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs index 5739489e..6de2c3e6 100644 --- a/src/app/integration_tests/patchset_actions.rs +++ b/src/app/integration_tests/patchset_actions.rs @@ -16,9 +16,10 @@ use crate::{ }, config::{ConfigSnapshot, ConfigState}, infrastructure::{ - file_system::MockFileSystemTrait, + file_system::{FileSystemError, MockFileSystemTrait}, shell::{MockShellTrait, ShellCommand, ShellOutput}, }, + kw::history::MockKwHistoryStore, lore::application::{ cache::BootstrapLoreData, handle::LoreApiHandle, messages::LoreApiMessage, }, @@ -82,6 +83,7 @@ async fn apply_success_switches_back_when_stay_disabled() { lore_handle_with_persistence(), apply_details_state(), apply_config_stay_disabled(), + history_store_allowing_writes(), ); app.consolidate_patchset_actions().await.unwrap(); @@ -128,6 +130,109 @@ async fn apply_failure_sets_failure_popup_and_resets_apply_action() { ); } +#[tokio::test] +async fn apply_success_records_apply_history() { + let (shell, _calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + ]); + let mut kw_history = MockKwHistoryStore::new(); + kw_history + .expect_record_apply() + .times(1) + .withf(|record| { + record.message_id == "http://lore.kernel.org/test-list/1234-1-foo@bar.example" + && record.kernel_tree_id == "linux" + && record.tree_path == KERNEL_TREE_PATH + && record.applied_branch.starts_with("patchset-") + && record.base_branch == BASE_BRANCH + && !record.applied_at.is_empty() + }) + .returning(|_| Ok(())); + let mut app = app_with_details( + clean_fs(), + shell, + lore_handle_with_persistence(), + apply_details_state(), + apply_config(), + kw_history, + ); + + app.consolidate_patchset_actions().await.unwrap(); + + assert_info_popup_contains( + app.state.popup.as_ref(), + "Patchset Apply Success", + &["applied successfully"], + ); +} + +#[tokio::test] +async fn apply_success_with_history_write_failure_keeps_success_popup() { + let (shell, _calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "", true), + ]); + let mut kw_history = MockKwHistoryStore::new(); + kw_history + .expect_record_apply() + .times(1) + .returning(|_| Err(FileSystemError::IoError(std::io::Error::other("disk full")))); + let mut app = app_with_details( + clean_fs(), + shell, + lore_handle_with_persistence(), + apply_details_state(), + apply_config(), + kw_history, + ); + + 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", "was not recorded in the kw history"], + ); +} + +#[tokio::test] +async fn apply_failure_does_not_record_history() { + let (shell, _calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "apply failed", false), + output("", "", true), + output("", "", true), + ]); + let mut kw_history = MockKwHistoryStore::new(); + kw_history.expect_record_apply().times(0); + let mut app = app_with_details( + clean_fs(), + shell, + lore_handle_with_persistence(), + apply_details_state(), + apply_config(), + kw_history, + ); + + app.consolidate_patchset_actions().await.unwrap(); + + assert_info_popup_contains(app.state.popup.as_ref(), "Patchset Apply Fail", &[]); +} + #[tokio::test] async fn reviewed_reply_success_records_persists_and_resets_reply_action() { let saved_reviewed = Arc::new(Mutex::new(None)); @@ -196,6 +301,7 @@ fn app_with_apply_details(fs: MockFileSystemTrait, shell: MockShellTrait) -> App lore_handle_with_persistence(), apply_details_state(), apply_config(), + history_store_allowing_writes(), ) } @@ -206,6 +312,7 @@ fn app_with_reviewed_reply_details(shell: MockShellTrait, lore_api: LoreApiHandl lore_api, reviewed_reply_details_state(), apply_config(), + MockKwHistoryStore::new(), ) } @@ -215,6 +322,7 @@ fn app_with_details( lore_api: LoreApiHandle, details: PatchsetDetailsState, config: ConfigSnapshot, + kw_history: MockKwHistoryStore, ) -> App { let mut app = App::new( config, @@ -228,6 +336,7 @@ fn app_with_details( Box::new(shell), lore_api, dummy_render_handle(), + Arc::new(kw_history), ) .expect("app should build"); @@ -297,6 +406,12 @@ fn reviewed_reply_lore_handle(saved_reviewed: SharedReviewedState) -> LoreApiHan // `stay_on_applied_branch` is deliberately absent so the tests exercise the // serde default (true) that existing config files inherit. +fn history_store_allowing_writes() -> MockKwHistoryStore { + let mut store = MockKwHistoryStore::new(); + store.expect_record_apply().returning(|_| Ok(())); + store +} + fn apply_config() -> ConfigSnapshot { serde_json::from_value::(serde_json::json!({ "kernel_trees": { diff --git a/src/app/mod.rs b/src/app/mod.rs index 887d3130..1944fcba 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -32,15 +32,22 @@ use color_eyre::{ }; use tracing::{debug, event, info, warn, Level}; +use std::sync::Arc; + +use chrono::{SecondsFormat, Utc}; + use crate::{ app::actions::{ - apply::ApplyPatchsetRequest, reviewed_reply::ReviewedReplyRequest, PatchsetActionService, + apply::{AppliedPatchset, ApplyPatchsetRequest}, + reviewed_reply::ReviewedReplyRequest, + PatchsetActionService, }, config::{ConfigHandle, ConfigSnapshot}, infrastructure::{ file_system::FileSystemTrait, monitoring::logging::garbage_collector::collect_garbage, shell::ShellTrait, }, + kw::history::{KwApplyRecord, KwHistoryStore}, lore::{ application::{ cache::{BootstrapLoreData, CacheMode}, @@ -69,6 +76,8 @@ pub struct AppServices { pub shell: Box, pub fs: Box, pub config: ConfigHandle, + /// Shared with KwActor once it exists (the actor adopts the same store). + pub kw_history: Arc, } /// Result type signalling whether a patchset was successfully loaded. @@ -100,6 +109,7 @@ impl App { shell: Box, lore_api: LoreApiHandle, render: RenderHandle, + kw_history: Arc, ) -> Result { event!(Level::INFO, "patch-hub started"); collect_garbage(&config); @@ -136,6 +146,7 @@ impl App { shell, fs, config: config_handle, + kw_history, }, }) } @@ -414,7 +425,24 @@ impl App { &self.services.lore_api, ); let popup = match action_service.apply_patchset(&request, &self.state.config) { - Ok(applied) => popup::AppPopup::info("Patchset Apply Success", applied.message), + Ok(applied) => { + let record = kw_apply_record(details, &self.state.config, &applied); + match self.services.kw_history.record_apply(record) { + Ok(()) => popup::AppPopup::info("Patchset Apply Success", applied.message), + // The git apply itself succeeded; a history-write + // failure must not turn it into a reported failure. + Err(e) => { + warn!(error = %e, "failed to record kw apply history"); + popup::AppPopup::info( + "Patchset Apply Success", + format!( + "{}\n\nWarning: the apply was not recorded in the kw history: {e}", + applied.message + ), + ) + } + } + } Err(msg) => popup::AppPopup::info("Patchset Apply Fail", msg), }; @@ -492,6 +520,32 @@ fn apply_patchset_request(details: &PatchsetDetailsState) -> ApplyPatchsetReques } } +fn kw_apply_record( + details: &PatchsetDetailsState, + config: &ConfigSnapshot, + applied: &AppliedPatchset, +) -> KwApplyRecord { + // Both were already validated by the apply itself, which cannot have + // succeeded without a resolvable target kernel tree. + let kernel_tree_id = config + .target_kernel_tree() + .as_ref() + .expect("invariant: a successful apply implies a target kernel tree was set") + .clone(); + let kernel_tree = config + .get_kernel_tree(&kernel_tree_id) + .expect("invariant: a successful apply implies the target kernel tree exists"); + + KwApplyRecord { + message_id: details.representative_patch.message_id().href.clone(), + kernel_tree_id, + tree_path: kernel_tree.path().clone(), + applied_branch: applied.applied_branch.clone(), + base_branch: kernel_tree.branch().clone(), + applied_at: Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true), + } +} + #[cfg(test)] mod tests { use std::collections::{HashMap, HashSet}; diff --git a/src/kw/history.rs b/src/kw/history.rs index d5445242..da993702 100644 --- a/src/kw/history.rs +++ b/src/kw/history.rs @@ -12,9 +12,6 @@ use std::{collections::HashMap, io, path::Path, sync::Arc}; use crate::infrastructure::file_system::{FileSystemError, FileSystemTrait}; -// No production caller exists until the apply hook wires the store into the -// app; kept per the CachePolicy precedent (src/lore/application/cache.rs). -#[allow(dead_code)] pub const APPLY_HISTORY_FILENAME: &str = "kw_apply_history.json"; /// One recorded `git am` application of a lore patchset to a kernel tree. @@ -31,10 +28,6 @@ pub struct KwApplyRecord { pub applied_at: String, } -// The trait's production caller is the apply hook (and, later, KwActor); the -// read side serves readiness/prefill in later steps. Kept per the CachePolicy -// precedent (src/lore/application/cache.rs). -#[allow(dead_code)] #[automock] pub trait KwHistoryStore: Send + Sync { /// Inserts or replaces the apply record keyed by `record.message_id`. @@ -42,6 +35,9 @@ pub trait KwHistoryStore: Send + Sync { /// Returns the apply record for `message_id`, or `None` if it was never /// recorded. A missing history file is a normal state, not an error. + // Read by the kw readiness checks in a later step; kept per the + // CachePolicy precedent (src/lore/application/cache.rs). + #[allow(dead_code)] fn apply_record(&self, message_id: &str) -> Result, FileSystemError>; } @@ -51,9 +47,6 @@ pub struct FileKwHistoryStore { } impl FileKwHistoryStore { - // No production caller exists until the apply hook wires the store into - // the app; kept per the CachePolicy precedent. - #[allow(dead_code)] pub fn new(fs: Arc, apply_history_path: String) -> Self { FileKwHistoryStore { fs, diff --git a/src/main.rs b/src/main.rs index 5977be91..8accbadb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -25,6 +25,7 @@ use infrastructure::{ terminal::init, }; use input::{actor::InputActor, event::InputEvent}; +use kw::history::{FileKwHistoryStore, KwHistoryStore, APPLY_HISTORY_FILENAME}; use lore::{ application::{actor::LoreApiActor, cache::CacheTtl, service::LoreService}, infrastructure::{ @@ -95,6 +96,10 @@ async fn main() -> Result<()> { config.patchsets_cache_dir().to_string(), )); let parser = Arc::new(MboxPatchsetParser::new(fs_arc.clone())); + let kw_history: Arc = Arc::new(FileKwHistoryStore::new( + fs_arc.clone(), + format!("{}/{}", config.data_dir(), APPLY_HISTORY_FILENAME), + )); let render = RenderActor::spawn(Box::new(ShellRenderService::new(shell_arc.clone()))); @@ -126,6 +131,7 @@ async fn main() -> Result<()> { Box::new(OsShell), lore_api.clone(), render.clone(), + kw_history.clone(), )?; let (app_input_tx, app_input_rx) = mpsc::channel::(64); let input_handle = InputActor::spawn(terminal_handle.clone(), app.input_context()); From 6c503e8d257259194f162b67b2814e63c0ce4a8d Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Fri, 14 Aug 2026 20:53:44 -0300 Subject: [PATCH 5/7] fix(kw): nest apply history by tree and harden record writes This commit keys apply records by message id and kernel tree so the same patchset applied to several trees keeps one record per tree. Store errors name the history file with an actionable popup, and an unresolvable target tree warns instead of panicking the TUI. This commit is part of the kw integration's step 2. Signed-off-by: lorenzoberts --- src/app/integration_tests/patchset_actions.rs | 7 +- src/app/mod.rs | 55 +++---- src/config/service.rs | 3 + src/config/tests.rs | 2 +- src/kw/history.rs | 140 ++++++++++++++---- test_samples/app/config/config.json | 27 ---- 6 files changed, 147 insertions(+), 87 deletions(-) delete mode 100644 test_samples/app/config/config.json diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs index 6de2c3e6..1eadbc7a 100644 --- a/src/app/integration_tests/patchset_actions.rs +++ b/src/app/integration_tests/patchset_actions.rs @@ -201,7 +201,12 @@ async fn apply_success_with_history_write_failure_keeps_success_popup() { assert_info_popup_contains( app.state.popup.as_ref(), "Patchset Apply Success", - &["applied successfully", "was not recorded in the kw history"], + &[ + "applied successfully", + "was not recorded in the kw history", + "disk full", + "inspect or delete that file", + ], ); } diff --git a/src/app/mod.rs b/src/app/mod.rs index 1944fcba..58b418c1 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -426,22 +426,31 @@ impl App { ); let popup = match action_service.apply_patchset(&request, &self.state.config) { Ok(applied) => { - let record = kw_apply_record(details, &self.state.config, &applied); - match self.services.kw_history.record_apply(record) { - Ok(()) => popup::AppPopup::info("Patchset Apply Success", applied.message), + 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 + ) + } // The git apply itself succeeded; a history-write // failure must not turn it into a reported failure. - Err(e) => { - warn!(error = %e, "failed to record kw apply history"); - popup::AppPopup::info( - "Patchset Apply Success", + Some(record) => match self.services.kw_history.record_apply(record) { + 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}", + "{}\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), }; @@ -524,26 +533,18 @@ fn kw_apply_record( details: &PatchsetDetailsState, config: &ConfigSnapshot, applied: &AppliedPatchset, -) -> KwApplyRecord { - // Both were already validated by the apply itself, which cannot have - // succeeded without a resolvable target kernel tree. - let kernel_tree_id = config - .target_kernel_tree() - .as_ref() - .expect("invariant: a successful apply implies a target kernel tree was set") - .clone(); - let kernel_tree = config - .get_kernel_tree(&kernel_tree_id) - .expect("invariant: a successful apply implies the target kernel tree exists"); - - KwApplyRecord { +) -> Option { + let kernel_tree_id = config.target_kernel_tree().as_ref()?; + let kernel_tree = config.get_kernel_tree(kernel_tree_id)?; + + Some(KwApplyRecord { message_id: details.representative_patch.message_id().href.clone(), - kernel_tree_id, + kernel_tree_id: kernel_tree_id.clone(), tree_path: kernel_tree.path().clone(), applied_branch: applied.applied_branch.clone(), base_branch: kernel_tree.branch().clone(), applied_at: Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true), - } + }) } #[cfg(test)] diff --git a/src/config/service.rs b/src/config/service.rs index 331049fe..bad6ab78 100644 --- a/src/config/service.rs +++ b/src/config/service.rs @@ -135,6 +135,9 @@ pub(crate) fn validate_update( let stay_on_applied_branch = match &draft.stay_on_applied_branch { None => None, + Some(s) if s.trim().is_empty() => { + return Err(ConfigError::InvalidStayOnAppliedBranch(s.clone())); + } Some(s) => Some( s.trim() .parse::() diff --git a/src/config/tests.rs b/src/config/tests.rs index 34c0e717..d60eb826 100644 --- a/src/config/tests.rs +++ b/src/config/tests.rs @@ -63,7 +63,7 @@ fn default_env() -> (MockEnvTrait, PathBuf) { (mock, home) } -/// Same logical content as `test_samples/app/config/config.json`, but paths under `root` so +/// Fully-populated config file content, with paths under `root` so /// `ensure_directories` stays inside a writable temp tree. After bootstrap, `normalize_derived_paths` /// overwrites patchset/data paths from `cache_dir` and `data_dir` only (explicit per-field paths /// in JSON are not preserved). diff --git a/src/kw/history.rs b/src/kw/history.rs index da993702..dc7ebf1b 100644 --- a/src/kw/history.rs +++ b/src/kw/history.rs @@ -28,17 +28,27 @@ pub struct KwApplyRecord { pub applied_at: String, } +/// message id → kernel tree id → record: applying the same patchset to +/// several trees keeps one record per tree. +type ApplyRecords = HashMap>; + #[automock] pub trait KwHistoryStore: Send + Sync { - /// Inserts or replaces the apply record keyed by `record.message_id`. + /// Inserts or replaces the apply record for the record's + /// `(message_id, kernel_tree_id)` pair. fn record_apply(&self, record: KwApplyRecord) -> Result<(), FileSystemError>; - /// Returns the apply record for `message_id`, or `None` if it was never - /// recorded. A missing history file is a normal state, not an error. + /// Returns the apply record for the `(message_id, kernel_tree_id)` pair, + /// or `None` if it was never recorded. A missing history file is a normal + /// state, not an error. // Read by the kw readiness checks in a later step; kept per the // CachePolicy precedent (src/lore/application/cache.rs). #[allow(dead_code)] - fn apply_record(&self, message_id: &str) -> Result, FileSystemError>; + fn apply_record( + &self, + message_id: &str, + kernel_tree_id: &str, + ) -> Result, FileSystemError>; } pub struct FileKwHistoryStore { @@ -54,7 +64,7 @@ impl FileKwHistoryStore { } } - fn load_apply_records(&self) -> Result, FileSystemError> { + fn load_apply_records(&self) -> Result { let path = Path::new(&self.apply_history_path); if !self.fs.is_file(path) { return Ok(HashMap::new()); @@ -67,6 +77,15 @@ impl FileKwHistoryStore { .map_err(FileSystemError::from) } + fn store_apply_record(&self, record: KwApplyRecord) -> Result<(), FileSystemError> { + let mut records = self.load_apply_records()?; + records + .entry(record.message_id.clone()) + .or_default() + .insert(record.kernel_tree_id.clone(), record); + self.atomic_write_json(&records, &self.apply_history_path) + } + /// Mirrors `FileLorePersistence::atomic_write_json` /// (src/lore/infrastructure/persistence.rs). fn atomic_write_json( @@ -86,17 +105,36 @@ impl FileKwHistoryStore { self.fs.rename(Path::new(&tmp_path), Path::new(path))?; Ok(()) } + + /// Makes store errors self-describing so the apply hook's warning popup + /// can point the user at the file to inspect or delete. + fn error_with_path(&self, error: FileSystemError) -> FileSystemError { + FileSystemError::IoError(io::Error::other(format!( + "{}: {error}", + self.apply_history_path + ))) + } } impl KwHistoryStore for FileKwHistoryStore { fn record_apply(&self, record: KwApplyRecord) -> Result<(), FileSystemError> { - let mut records = self.load_apply_records()?; - records.insert(record.message_id.clone(), record); - self.atomic_write_json(&records, &self.apply_history_path) + self.store_apply_record(record) + .map_err(|e| self.error_with_path(e)) } - fn apply_record(&self, message_id: &str) -> Result, FileSystemError> { - Ok(self.load_apply_records()?.remove(message_id)) + fn apply_record( + &self, + message_id: &str, + kernel_tree_id: &str, + ) -> Result, FileSystemError> { + self.load_apply_records() + .map(|records| { + records + .get(message_id) + .and_then(|by_tree| by_tree.get(kernel_tree_id)) + .cloned() + }) + .map_err(|e| self.error_with_path(e)) } } @@ -120,6 +158,9 @@ mod tests { std::process::id(), n )); + // A leftover from a failed previous run (pid reuse + counter reset) + // must not poison this one. + let _ = fs::remove_dir_all(&dir); fs::create_dir_all(&dir).unwrap(); dir } @@ -134,11 +175,11 @@ mod tests { ) } - fn record(message_id: &str, branch: &str) -> KwApplyRecord { + fn record(message_id: &str, kernel_tree_id: &str, branch: &str) -> KwApplyRecord { KwApplyRecord { message_id: message_id.to_string(), - kernel_tree_id: "mainline".to_string(), - tree_path: "/home/user/linux".to_string(), + kernel_tree_id: kernel_tree_id.to_string(), + tree_path: format!("/home/user/{kernel_tree_id}"), applied_branch: branch.to_string(), base_branch: "master".to_string(), applied_at: "2026-08-01T17:30:00Z".to_string(), @@ -151,46 +192,75 @@ mod tests { let store = store_at(&dir); store - .record_apply(record("msg-1", "patchset-2026-08-01-17-30-00")) + .record_apply(record("msg-1", "mainline", "patchset-2026-08-01-17-30-00")) .unwrap(); store - .record_apply(record("msg-2", "patchset-2026-08-02-10-00-00")) + .record_apply(record("msg-2", "mainline", "patchset-2026-08-02-10-00-00")) .unwrap(); assert_eq!( - Some(record("msg-1", "patchset-2026-08-01-17-30-00")), - store.apply_record("msg-1").unwrap() + Some(record("msg-1", "mainline", "patchset-2026-08-01-17-30-00")), + store.apply_record("msg-1", "mainline").unwrap() ); assert_eq!( - Some(record("msg-2", "patchset-2026-08-02-10-00-00")), - store.apply_record("msg-2").unwrap() + Some(record("msg-2", "mainline", "patchset-2026-08-02-10-00-00")), + store.apply_record("msg-2", "mainline").unwrap() ); fs::remove_dir_all(&dir).unwrap(); } #[test] - fn record_with_same_message_id_overwrites() { + fn record_with_same_message_id_and_tree_overwrites() { let dir = tmp_dir("overwrite"); let store = store_at(&dir); - store.record_apply(record("msg-1", "patchset-old")).unwrap(); - store.record_apply(record("msg-1", "patchset-new")).unwrap(); + store + .record_apply(record("msg-1", "mainline", "patchset-old")) + .unwrap(); + store + .record_apply(record("msg-1", "mainline", "patchset-new")) + .unwrap(); assert_eq!( - Some(record("msg-1", "patchset-new")), - store.apply_record("msg-1").unwrap() + Some(record("msg-1", "mainline", "patchset-new")), + store.apply_record("msg-1", "mainline").unwrap() ); fs::remove_dir_all(&dir).unwrap(); } + #[test] + fn same_message_id_applied_to_multiple_trees_coexists() { + let dir = tmp_dir("multi-tree"); + let store = store_at(&dir); + + store + .record_apply(record("msg-1", "mainline", "patchset-mainline")) + .unwrap(); + store + .record_apply(record("msg-1", "stable", "patchset-stable")) + .unwrap(); + + assert_eq!( + Some(record("msg-1", "mainline", "patchset-mainline")), + store.apply_record("msg-1", "mainline").unwrap() + ); + assert_eq!( + Some(record("msg-1", "stable", "patchset-stable")), + store.apply_record("msg-1", "stable").unwrap() + ); + assert_eq!(None, store.apply_record("msg-1", "amd-gfx").unwrap()); + + fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn missing_history_file_reads_as_empty() { let dir = tmp_dir("missing"); let store = store_at(&dir); - assert_eq!(None, store.apply_record("msg-1").unwrap()); + assert_eq!(None, store.apply_record("msg-1", "mainline").unwrap()); fs::remove_dir_all(&dir).unwrap(); } @@ -208,11 +278,13 @@ mod tests { .to_string(), ); - store.record_apply(record("msg-1", "patchset-x")).unwrap(); + store + .record_apply(record("msg-1", "mainline", "patchset-x")) + .unwrap(); assert_eq!( - Some(record("msg-1", "patchset-x")), - store.apply_record("msg-1").unwrap() + Some(record("msg-1", "mainline", "patchset-x")), + store.apply_record("msg-1", "mainline").unwrap() ); fs::remove_dir_all(&dir).unwrap(); @@ -224,8 +296,12 @@ mod tests { let store = store_at(&dir); fs::write(dir.join(APPLY_HISTORY_FILENAME), b"not json").unwrap(); - assert!(store.apply_record("msg-1").is_err()); - assert!(store.record_apply(record("msg-1", "patchset-x")).is_err()); + let err = store.apply_record("msg-1", "mainline").unwrap_err(); + // Errors name the file so the warning popup can point at it. + assert!(err.to_string().contains(APPLY_HISTORY_FILENAME)); + assert!(store + .record_apply(record("msg-1", "mainline", "patchset-x")) + .is_err()); // The corrupt file is left untouched for the user to inspect. assert_eq!( "not json", @@ -240,7 +316,9 @@ mod tests { let dir = tmp_dir("atomic"); let store = store_at(&dir); - store.record_apply(record("msg-1", "patchset-x")).unwrap(); + store + .record_apply(record("msg-1", "mainline", "patchset-x")) + .unwrap(); let tmp_left = fs::read_dir(&dir).unwrap().any(|e| { e.ok() diff --git a/test_samples/app/config/config.json b/test_samples/app/config/config.json deleted file mode 100644 index 05e2b5fe..00000000 --- a/test_samples/app/config/config.json +++ /dev/null @@ -1,27 +0,0 @@ -{ - "page_size": 1234, - "patchsets_cache_dir": "/cachedir/path", - "bookmarked_patchsets_path": "/bookmarked/patchsets/path", - "mailing_lists_path": "/mailing/lists/path", - "reviewed_patchsets_path": "/reviewed/patchsets/path", - "logs_path":"/logs/path", - "git_send_email_options": "--long-option value -s -h -o -r -t", - "cache_dir": "/cache_dir", - "data_dir": "/data_dir", - "patch_renderer": "default", - "cover_renderer": "default", - "max_log_age": 42, - "kernel_trees": { - "linux": { - "path": "/home/user/linux", - "branch": "master" - }, - "amd-gfx": { - "path": "/home/user/amd-gfx", - "branch": "amd-staging-drm-next" - } - }, - "target_kernel_tree": "linux", - "git_am_options": "--foo-bar foobar -s -n -o -r -l -a -x", - "git_am_branch_prefix": "really-creative-prefix-" -} From 3e2f1ed60bc281c29ba7c2f459f0c1cc4cdc773b Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Thu, 20 Aug 2026 09:51:28 -0300 Subject: [PATCH 6/7] refactor(infrastructure): share atomic JSON writes via JsonUtils This commit lifts the tmp-then-rename JSON write used by lore, config, and kw history into a shared file-system helper. Serialization is uniformly pretty-printed; serde failures on config save now surface as filesystem errors, and the unused ConfigError::Save variant is removed. This commit completes the kw integration's step 2. Signed-off-by: lorenzoberts --- src/config/errors.rs | 2 -- src/config/repository.rs | 28 ++++--------------- src/infrastructure/file_system/json.rs | 38 ++++++++++++++++++++++++++ src/infrastructure/file_system/mod.rs | 2 ++ src/kw/history.rs | 26 ++---------------- src/lore/infrastructure/persistence.rs | 30 ++++---------------- 6 files changed, 54 insertions(+), 72 deletions(-) create mode 100644 src/infrastructure/file_system/json.rs diff --git a/src/config/errors.rs b/src/config/errors.rs index 8a95f358..39c698b2 100644 --- a/src/config/errors.rs +++ b/src/config/errors.rs @@ -6,8 +6,6 @@ use crate::infrastructure::file_system::FileSystemError; pub enum ConfigError { #[error("config actor unavailable: {0}")] ActorUnavailable(String), - #[error("failed to save config: {0}")] - Save(String), #[error("invalid page size: {0}")] InvalidPageSize(String), #[error("invalid directory: {0}")] diff --git a/src/config/repository.rs b/src/config/repository.rs index 67c9a378..ae9e3e2e 100644 --- a/src/config/repository.rs +++ b/src/config/repository.rs @@ -1,11 +1,10 @@ -use std::path::Path; - -use serde_json::to_writer_pretty; - use crate::config::errors::ConfigError; use crate::config::state::ConfigState; use crate::config::DEFAULT_CONFIG_PATH_SUFFIX; -use crate::infrastructure::{env::EnvTrait, file_system::FileSystemTrait}; +use crate::infrastructure::{ + env::EnvTrait, + file_system::{FileSystemTrait, JsonUtils}, +}; pub trait ConfigRepository: Send + Sync { fn save(&self, state: &ConfigState) -> Result<(), ConfigError>; @@ -46,24 +45,7 @@ impl JsonConfigRepository { impl ConfigRepository for JsonConfigRepository { fn save(&self, state: &ConfigState) -> Result<(), ConfigError> { - let config_path = Path::new(&self.config_path); - if let Some(parent_dir) = Path::parent(config_path) { - self.fs - .create_dir_all(parent_dir) - .map_err(ConfigError::from)?; - } - - let tmp_filename = format!("{}.tmp", config_path.display()); - { - let tmp_file = self - .fs - .create_writer(Path::new(&tmp_filename)) - .map_err(ConfigError::from)?; - to_writer_pretty(tmp_file, state).map_err(|e| ConfigError::Save(e.to_string()))?; - } - self.fs - .rename(Path::new(&tmp_filename), config_path) - .map_err(ConfigError::from)?; + JsonUtils::atomic_write_json(&self.fs, state, &self.config_path)?; Ok(()) } } diff --git a/src/infrastructure/file_system/json.rs b/src/infrastructure/file_system/json.rs new file mode 100644 index 00000000..2181c0aa --- /dev/null +++ b/src/infrastructure/file_system/json.rs @@ -0,0 +1,38 @@ +//! JSON persistence helpers over [`FileSystemTrait`]. + +use serde::Serialize; +use serde_json::to_writer_pretty; + +use std::{io, path::Path}; + +use crate::infrastructure::file_system::{FileSystemError, FileSystemTrait}; + +/// Shared JSON persistence operations. Stateless namespace for methods used +/// by the JSON-backed stores (config repository, lore persistence, kw +/// history); grows as more shared JSON behavior appears. +pub struct JsonUtils; + +impl JsonUtils { + /// Atomically writes `value` as pretty-printed JSON to `path`: the content + /// goes to `.tmp` first and is then renamed over `path`, so a crash + /// mid-write cannot leave a truncated file behind. Parent directories are + /// created as needed. + pub fn atomic_write_json( + fs: &dyn FileSystemTrait, + value: &T, + path: &str, + ) -> Result<(), FileSystemError> { + let path = Path::new(path); + if let Some(parent) = path.parent() { + fs.create_dir_all(parent)?; + } + + let tmp_path = format!("{}.tmp", path.display()); + { + let writer = fs.create_writer(Path::new(&tmp_path))?; + to_writer_pretty(writer, value).map_err(io::Error::from)?; + } + fs.rename(Path::new(&tmp_path), path)?; + Ok(()) + } +} diff --git a/src/infrastructure/file_system/mod.rs b/src/infrastructure/file_system/mod.rs index cec7ea13..48ede4e4 100644 --- a/src/infrastructure/file_system/mod.rs +++ b/src/infrastructure/file_system/mod.rs @@ -1,5 +1,7 @@ +mod json; mod r#trait; +pub use json::JsonUtils; pub use r#trait::{FileSystemError, FileSystemTrait}; #[cfg(test)] diff --git a/src/kw/history.rs b/src/kw/history.rs index dc7ebf1b..e5030974 100644 --- a/src/kw/history.rs +++ b/src/kw/history.rs @@ -6,11 +6,11 @@ use mockall::automock; use serde::{Deserialize, Serialize}; -use serde_json::{from_reader, to_writer_pretty}; +use serde_json::from_reader; use std::{collections::HashMap, io, path::Path, sync::Arc}; -use crate::infrastructure::file_system::{FileSystemError, FileSystemTrait}; +use crate::infrastructure::file_system::{FileSystemError, FileSystemTrait, JsonUtils}; pub const APPLY_HISTORY_FILENAME: &str = "kw_apply_history.json"; @@ -83,27 +83,7 @@ impl FileKwHistoryStore { .entry(record.message_id.clone()) .or_default() .insert(record.kernel_tree_id.clone(), record); - self.atomic_write_json(&records, &self.apply_history_path) - } - - /// Mirrors `FileLorePersistence::atomic_write_json` - /// (src/lore/infrastructure/persistence.rs). - fn atomic_write_json( - &self, - value: &T, - path: &str, - ) -> Result<(), FileSystemError> { - if let Some(parent) = Path::new(path).parent() { - self.fs.create_dir_all(parent)?; - } - - let tmp_path = format!("{path}.tmp"); - { - let writer = self.fs.create_writer(Path::new(&tmp_path))?; - to_writer_pretty(writer, value).map_err(io::Error::from)?; - } - self.fs.rename(Path::new(&tmp_path), Path::new(path))?; - Ok(()) + JsonUtils::atomic_write_json(&*self.fs, &records, &self.apply_history_path) } /// Makes store errors self-describing so the apply hook's warning popup diff --git a/src/lore/infrastructure/persistence.rs b/src/lore/infrastructure/persistence.rs index b59f2c0f..87420c91 100644 --- a/src/lore/infrastructure/persistence.rs +++ b/src/lore/infrastructure/persistence.rs @@ -1,6 +1,6 @@ use mockall::automock; -use serde::{de::DeserializeOwned, Serialize}; -use serde_json::{from_reader, to_writer}; +use serde::de::DeserializeOwned; +use serde_json::from_reader; use std::{ collections::{HashMap, HashSet}, @@ -10,7 +10,7 @@ use std::{ }; use crate::{ - infrastructure::file_system::{FileSystemError, FileSystemTrait}, + infrastructure::file_system::{FileSystemError, FileSystemTrait, JsonUtils}, lore::domain::{mailing_list::MailingList, patch::Patch}, }; @@ -60,24 +60,6 @@ impl FileLorePersistence { } } - fn atomic_write_json( - &self, - value: &T, - path: &str, - ) -> Result<(), FileSystemError> { - if let Some(parent) = Path::new(path).parent() { - self.fs.create_dir_all(parent)?; - } - - let tmp_path = format!("{path}.tmp"); - { - let writer = self.fs.create_writer(Path::new(&tmp_path))?; - to_writer(writer, value).map_err(io::Error::from)?; - } - self.fs.rename(Path::new(&tmp_path), Path::new(path))?; - Ok(()) - } - fn read_json(&self, path: &str) -> Result { let reader = self.fs.open_bufreader(Path::new(path))?; from_reader(reader) @@ -92,7 +74,7 @@ impl MailingListsCacheStore for FileLorePersistence { } fn save_available_lists(&self, lists: &[MailingList]) -> Result<(), FileSystemError> { - self.atomic_write_json(lists, &self.mailing_lists_path) + JsonUtils::atomic_write_json(&*self.fs, lists, &self.mailing_lists_path) } } @@ -102,7 +84,7 @@ impl UserLoreStateStore for FileLorePersistence { } fn save_bookmarked_patchsets(&self, patchsets: &[Patch]) -> Result<(), FileSystemError> { - self.atomic_write_json(patchsets, &self.bookmarked_path) + JsonUtils::atomic_write_json(&*self.fs, patchsets, &self.bookmarked_path) } fn load_reviewed_patchsets(&self) -> Result>, FileSystemError> { @@ -113,7 +95,7 @@ impl UserLoreStateStore for FileLorePersistence { &self, reviewed: &HashMap>, ) -> Result<(), FileSystemError> { - self.atomic_write_json(reviewed, &self.reviewed_path) + JsonUtils::atomic_write_json(&*self.fs, reviewed, &self.reviewed_path) } } From 720501a5ef1f8e07300f27bd7981c0db760880d3 Mon Sep 17 00:00:00 2001 From: lorenzoberts Date: Mon, 24 Aug 2026 14:20:00 -0300 Subject: [PATCH 7/7] docs(kw): describe apply history as what this change actually stores This commit drops comments that point at later steps, KwOps, or readiness probes that are not in this change. The dead_code allowance on apply_record stays; the comment around it does not. This commit is part of the kw integration's step 2. Signed-off-by: lorenzoberts Co-authored-by: Cursor --- src/infrastructure/file_system/json.rs | 2 +- src/kw/history.rs | 12 ++++-------- src/kw/mod.rs | 3 +-- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/infrastructure/file_system/json.rs b/src/infrastructure/file_system/json.rs index 2181c0aa..f7c46d13 100644 --- a/src/infrastructure/file_system/json.rs +++ b/src/infrastructure/file_system/json.rs @@ -9,7 +9,7 @@ use crate::infrastructure::file_system::{FileSystemError, FileSystemTrait}; /// Shared JSON persistence operations. Stateless namespace for methods used /// by the JSON-backed stores (config repository, lore persistence, kw -/// history); grows as more shared JSON behavior appears. +/// history). pub struct JsonUtils; impl JsonUtils { diff --git a/src/kw/history.rs b/src/kw/history.rs index e5030974..a823c295 100644 --- a/src/kw/history.rs +++ b/src/kw/history.rs @@ -1,8 +1,7 @@ -//! User-local history of patchset applies (and, in later steps, kw builds), -//! stored as JSON under the configured `data_dir`. +//! User-local history of patchset applies, stored as JSON under the +//! configured `data_dir`. //! -//! Apply records feed kw build/deploy readiness and the KwOps branch prefill, -//! so they are user state — not a cache — and are never refreshed from lore. +//! Records are user state — not a cache — and are never refreshed from lore. use mockall::automock; use serde::{Deserialize, Serialize}; @@ -19,8 +18,7 @@ pub const APPLY_HISTORY_FILENAME: &str = "kw_apply_history.json"; pub struct KwApplyRecord { pub message_id: String, pub kernel_tree_id: String, - /// Snapshot of `KernelTree.path` when the record was written, so later - /// readiness checks can detect the tree being repointed or moved. + /// Snapshot of `KernelTree.path` when the record was written. pub tree_path: String, pub applied_branch: String, pub base_branch: String, @@ -41,8 +39,6 @@ pub trait KwHistoryStore: Send + Sync { /// Returns the apply record for the `(message_id, kernel_tree_id)` pair, /// or `None` if it was never recorded. A missing history file is a normal /// state, not an error. - // Read by the kw readiness checks in a later step; kept per the - // CachePolicy precedent (src/lore/application/cache.rs). #[allow(dead_code)] fn apply_record( &self, diff --git a/src/kw/mod.rs b/src/kw/mod.rs index 4d881716..68c49c5e 100644 --- a/src/kw/mod.rs +++ b/src/kw/mod.rs @@ -1,4 +1,3 @@ -//! kw integration: persistence and (in later steps) the actor orchestrating -//! `kw build` / `kw deploy` jobs. +//! kw integration: persistence for apply history. pub mod history;