From 716290c3bfcb114f744ab5db17921c192e2a014a Mon Sep 17 00:00:00 2001 From: atagen Date: Thu, 21 May 2026 21:30:06 +1000 Subject: [PATCH] fix(Q5): drop retain pre-pass in apply_route_overrides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex audit of 4c39ecd flagged that the `retain` step would silently prune any base-profile rule whose single-field shape coincided with the overlay's emit pattern. `materialize` is already stateless — it reserialises the base profile fresh from `pick_base` on every call, so overlay rules can't accumulate across consecutive `set_route` calls. The retain bought nothing and risked dropping user-authored rules; prepending alone makes the overlay win first-match iteration. Adds a regression test that loads a user profile carrying a rule whose shape matches what the overlay emits, then sets an override for the same app and asserts the user rule survives alongside the prepended overlay rules. --- crates/headroom-core/src/profile_store.rs | 93 ++++++++++++++--------- 1 file changed, 56 insertions(+), 37 deletions(-) diff --git a/crates/headroom-core/src/profile_store.rs b/crates/headroom-core/src/profile_store.rs index d8b5b7e..2b6d0c1 100644 --- a/crates/headroom-core/src/profile_store.rs +++ b/crates/headroom-core/src/profile_store.rs @@ -637,9 +637,8 @@ fn materialize_skipping( } fn apply_route_overrides(profile: &mut Profile, overrides: &BTreeMap) { - // Drop any existing single-app user rule matching an override, then - // prepend two rules per app so the match catches whichever - // identity field the stream actually advertises. + // Prepend two rules per overlay entry so the match catches + // whichever identity field the stream actually advertises. // // Why two rules and not one with both fields set? The matcher // ANDs across non-empty fields, so a rule with both @@ -655,10 +654,18 @@ fn apply_route_overrides(profile: &mut Profile, overrides: &BTreeMap = overrides.keys().collect(); - profile - .rules - .retain(|r| !is_single_app_rule_for_any(r, &override_apps)); + // + // No retain pre-pass: `materialize` is stateless (it + // serializes the base profile fresh from `pick_base` each + // call), so overlay rules can't accumulate across consecutive + // `set_route` calls. A retain pre-pass would only deduplicate + // rules whose *base profile* TOML coincidentally has the same + // shape — silently removing a user-authored rule that was + // never an overlay artefact. The prepended order means + // overlay rules win first-match iteration over any genuinely + // duplicate base-profile rule anyway, so no correctness gain; + // dropping the retain closes the data-loss surface Codex + // flagged in its audit of the route.set match-by-name change. let mut new_rules: Vec = Vec::with_capacity(overrides.len() * 2); for (app, route) in overrides { new_rules.push(RouteRule { @@ -680,36 +687,6 @@ fn apply_route_overrides(profile: &mut Profile, overrides: &BTreeMap, -) -> bool { - let m = &rule.match_; - if !m.portal_app_id.is_empty() || !m.media_role.is_empty() { - return false; - } - // process_binary-only single-app rule - if m.process_binary.len() == 1 - && apps.contains(&m.process_binary[0]) - && m.application_name.is_empty() - { - return true; - } - // application_name-only single-app rule (added 2026-05-21 so - // CLI tools that don't advertise `process.binary` match too) - if m.application_name.len() == 1 - && apps.contains(&m.application_name[0]) - && m.process_binary.is_empty() - { - return true; - } - false -} - // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- @@ -1030,6 +1007,48 @@ mod tests { assert!(residual.is_empty(), "leftover override rules: {residual:#?}"); } + #[test] + fn user_rule_with_overlay_shape_survives_set_route_for_same_app() { + // Regression for Codex audit Q5: an earlier retain pre-pass in + // `apply_route_overrides` would silently drop any base-profile + // rule whose single-field shape coincided with the overlay's + // emit pattern. The fix is to delete the retain entirely — + // prepending already makes the overlay win first-match + // iteration, and removing the retain closes the data-loss + // surface. This test pins the surviving-rule behaviour so a + // future refactor can't quietly reintroduce the prune. + let (paths, _g) = tmp_paths(); + fs::write( + paths.config_dir.join("profiles/custom.toml"), + r#" +name = "custom" +description = "user custom" +default_route = { route = "processed" } +[[rules]] +match = { process_binary = ["obs"] } +route = "processed" +"#, + ) + .unwrap(); + let mut s = ProfileStore::load(&paths).unwrap(); + s.use_profile("custom").unwrap(); + // Sanity: user rule is loaded once. + assert_eq!(s.effective().rules.len(), 1); + + s.set_route("obs", Route::Bypass).unwrap(); + + let rules = &s.effective().rules; + // Two overlay rules (process_binary + application_name) plus + // the preserved user rule. + assert_eq!(rules.len(), 3, "rules: {rules:#?}"); + assert_eq!(rules[0].route, Route::Bypass); + assert_eq!(rules[0].match_.process_binary, vec!["obs".to_string()]); + assert_eq!(rules[1].route, Route::Bypass); + assert_eq!(rules[1].match_.application_name, vec!["obs".to_string()]); + assert_eq!(rules[2].route, Route::Processed); + assert_eq!(rules[2].match_.process_binary, vec!["obs".to_string()]); + } + #[test] fn set_route_updates_existing_override() { let (paths, _g) = tmp_paths();