fix(Q5): drop retain pre-pass in apply_route_overrides

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.
This commit is contained in:
atagen 2026-05-21 21:30:06 +10:00
parent 4c39ecd5d2
commit 716290c3bf

View file

@ -637,9 +637,8 @@ fn materialize_skipping(
}
fn apply_route_overrides(profile: &mut Profile, overrides: &BTreeMap<String, Route>) {
// 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<String, Rou
// 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));
//
// 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<RouteRule> = 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<String, Rou
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 {
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();