From 4c39ecd5d20010cb14f9b9227b86869a626de252 Mon Sep 17 00:00:00 2001 From: atagen Date: Thu, 21 May 2026 21:12:23 +1000 Subject: [PATCH] fix: route.set matches application_name too, not just process_binary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `route.set ` overlay used to emit a single `RouteRule` keyed on `process_binary`. The matcher ANDs across non-empty fields, so a stream that didn't advertise `application.process.binary` would miss the rule even though its `application.name` was a perfect match. pw-cat is the canonical hit — it sets `application.name = "pw-cat"` and `node.name = "pw-cat"` but leaves `process.binary` unset entirely. The same goes for several Electron and Flatpak wrappers where the wrapping process eats the binary name. `apply_route_overrides` now emits TWO rules per override, one keyed on each identity field, with the same route. PipeWire iterates rules in order and returns on first match, so the effect is an OR across `process_binary` and `application_name` for the single override — exactly the "match by whatever name the stream advertises" intent of the CLI verb. Why two rules and not "loosen the matcher to OR these two fields": the matcher's AND-across-fields is load-bearing for profile-author rules like `{process_binary: ["firefox"], media_role: ["voice"]}` (match firefox-with-voice-role only). Loosening the matcher would silently break those. Two single-field rules with the same route preserve the original semantics and add zero risk. `is_single_app_rule_for_any` (the retain pre-pass that drops old override rules before re-emitting) extends to recognise the application_name-only variant too, so re-setting or unsetting an override leaves no residual rules. Tests - `profile_store::tests::set_route_emits_both_process_binary_and_application_name_rules` asserts both variants exist after `set_route`. - `profile_store::tests::set_route_then_unset_leaves_no_residual_rules` catches the matching retain-pre-pass regression that would have leaked rules on unset. - `routing::tests::application_name_only_rule_matches_stream_with_no_process_binary` proves a stream with `application.name = "pw-cat"` and no `process.binary` actually matches the application_name-keyed rule path. 194 tests pass (was 191; +3 for the new coverage); clippy clean. Live verification Daemon up, pw-cat → headroom-processed (default rule). `headroom route set pw-cat bypass`: pw-cat's link snaps to `Mbox:playback_FL` within one drain tick (~50 ms); status reports `route: bypass`. Layer A tap survives the transition intact. `headroom route unset pw-cat`: snaps back to `headroom-processed:playback_FL`. Both transitions are audibly clean against the F2 audio-gap mitigation from `5c769a1`. --- crates/headroom-core/src/profile_store.rs | 112 +++++++++++++++++++--- crates/headroom-core/src/routing.rs | 34 +++++++ 2 files changed, 135 insertions(+), 11 deletions(-) diff --git a/crates/headroom-core/src/profile_store.rs b/crates/headroom-core/src/profile_store.rs index df3c509..d8b5b7e 100644 --- a/crates/headroom-core/src/profile_store.rs +++ b/crates/headroom-core/src/profile_store.rs @@ -638,34 +638,76 @@ fn materialize_skipping( fn apply_route_overrides(profile: &mut Profile, overrides: &BTreeMap) { // Drop any existing single-app user rule matching an override, then - // prepend the overrides as one rule per app at the top of the list. + // prepend two rules per app 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 + // `process_binary` *and* `application_name` populated would + // only match a stream that has *both* properties set to the + // same string. Many CLI tools (pw-cat being the canonical + // case, plus various Electron / Flatpak wrappers) only set + // `application.name` and leave `application.process.binary` + // unset — they'd miss the AND-shape rule despite the user's + // clear intent. + // + // Two single-field rules with the same route effectively form + // an OR across the identity fields. PipeWire iterates rules in + // order and returns on first match, so emitting both is cheap + // (constant per override) and correct in either case. let override_apps: std::collections::HashSet<&String> = overrides.keys().collect(); profile .rules .retain(|r| !is_single_app_rule_for_any(r, &override_apps)); - let mut new_rules: Vec = overrides - .iter() - .map(|(app, route)| RouteRule { + let mut new_rules: Vec = Vec::with_capacity(overrides.len() * 2); + for (app, route) in overrides { + new_rules.push(RouteRule { match_: RouteRuleMatch { process_binary: vec![app.clone()], ..Default::default() }, route: *route, - }) - .collect(); + }); + new_rules.push(RouteRule { + match_: RouteRuleMatch { + application_name: vec![app.clone()], + ..Default::default() + }, + route: *route, + }); + } new_rules.extend(std::mem::take(&mut profile.rules)); profile.rules = new_rules; } +/// True iff `rule` is one of the single-app override rules +/// generated by [`apply_route_overrides`] for *any* of `apps`. +/// Used by the retain pre-pass so re-applying an overlay doesn't +/// double-stack the per-app rules from a prior materialise. fn is_single_app_rule_for_any( rule: &RouteRule, apps: &std::collections::HashSet<&String>, ) -> bool { - rule.match_.process_binary.len() == 1 - && apps.contains(&rule.match_.process_binary[0]) - && rule.match_.application_name.is_empty() - && rule.match_.portal_app_id.is_empty() - && rule.match_.media_role.is_empty() + 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 } // --------------------------------------------------------------------------- @@ -940,6 +982,54 @@ mod tests { assert_eq!(rule.route, Route::Bypass); } + #[test] + fn set_route_emits_both_process_binary_and_application_name_rules() { + // The route.set CLI verb accepts a single app identifier + // but streams can advertise themselves via either + // `application.process.binary` or `application.name` + // (or neither — those go through default_route). The + // overlay materialises BOTH single-field rules so each + // possible identity field is covered. + let (paths, _g) = tmp_paths(); + let mut s = ProfileStore::load(&paths).unwrap(); + s.set_route("pw-cat", Route::Bypass).unwrap(); + let rules = &s.effective().rules; + let proc_rule = rules + .iter() + .find(|r| r.match_.process_binary == vec!["pw-cat".to_string()]) + .expect("process_binary rule"); + assert_eq!(proc_rule.route, Route::Bypass); + assert!(proc_rule.match_.application_name.is_empty()); + let name_rule = rules + .iter() + .find(|r| r.match_.application_name == vec!["pw-cat".to_string()]) + .expect("application_name rule"); + assert_eq!(name_rule.route, Route::Bypass); + assert!(name_rule.match_.process_binary.is_empty()); + } + + #[test] + fn set_route_then_unset_leaves_no_residual_rules() { + // Both the process_binary and application_name variants + // of a single-app override must clear on unset; otherwise + // a re-add would stack rules and the matcher would carry + // dead entries indefinitely. + let (paths, _g) = tmp_paths(); + let mut s = ProfileStore::load(&paths).unwrap(); + s.set_route("pw-cat", Route::Bypass).unwrap(); + s.unset_route("pw-cat").unwrap(); + let residual: Vec<_> = s + .effective() + .rules + .iter() + .filter(|r| { + r.match_.process_binary == vec!["pw-cat".to_string()] + || r.match_.application_name == vec!["pw-cat".to_string()] + }) + .collect(); + assert!(residual.is_empty(), "leftover override rules: {residual:#?}"); + } + #[test] fn set_route_updates_existing_override() { let (paths, _g) = tmp_paths(); diff --git a/crates/headroom-core/src/routing.rs b/crates/headroom-core/src/routing.rs index a5956aa..99e0be3 100644 --- a/crates/headroom-core/src/routing.rs +++ b/crates/headroom-core/src/routing.rs @@ -185,6 +185,40 @@ mod tests { } } + #[test] + fn application_name_only_rule_matches_stream_with_no_process_binary() { + // The shape `route set` emits when expanded into an + // `application_name`-keyed override. Verifies that a + // stream missing `application.process.binary` (typical + // of pw-cat, many CLI tools, some Flatpak wrappers) is + // still matched by the user's intent. + use headroom_ipc::{RouteRule, RouteRuleMatch}; + let mut profile = Profile::default_v0(); + // Override at the top of the rule list. + profile.rules.insert( + 0, + RouteRule { + match_: RouteRuleMatch { + application_name: vec!["pw-cat".into()], + ..Default::default() + }, + route: Route::Bypass, + }, + ); + // Stream advertises only application.name = "pw-cat". + let info = PwNodeInfo { + node_id: 9, + media_class: Some("Stream/Output/Audio".into()), + application_process_binary: None, + application_name: Some("pw-cat".into()), + ..Default::default() + }; + assert_eq!( + evaluate(&info, &profile, false), + RoutingDecision::Route(Route::Bypass) + ); + } + #[test] fn matches_bypass_rule_for_known_music_player() { let info = playback("mpv");