audio/ownership: merge inherited player env vars instead of clobbering

PULSE_PROP and PIPEWIRE_PROPS can legitimately carry a user's own routing
policy — media.role, a target sink — and replacing them changes where the
user's audio goes as a side effect of a tagging mechanism that is
supposed to be behaviourally invisible.

PULSE_PROP is space-separated key=value, so merging is appending;
PIPEWIRE_PROPS is a SPA-JSON object, so it is an insert before the
closing brace. Our pairs go last in both, so they win a duplicate key —
without that, a user with node.name already set would silently untag us.
A value that does not match the expected shape is logged and overwritten:
a half-merged string that fails to parse would drop the tag silently,
which is worse than losing a routing preference. No full SPA-JSON parser,
which would be over-engineering for a case with no live consumer
(measured: neither variable is set anywhere in this user's env or config).

Also sets PIPEWIRE_ALSA on the child, with the child's own role. A player
configured for ALSA output is reached by neither of the other two
variables, so this closes a real gap rather than only a cosmetic one —
and without it such a child would inherit this process's `clip` tag from
tag_this_process_alsa_audio and report the wrong role in the audit.

Corrects a stale doc comment on OWNED_PROP_VALUE that still claimed
pixelpass accepts any truthy value; R10-4 made the match exact. Codex's
F5 was reasoned partly from a stale comment of mine, so these are worth
fixing on sight.

Codex phase-1 review F4. Round 10, R10-5.
8 new rows; 5 mutations verified (clobber PULSE_PROP, our pairs first,
naive object concat, doubled trailing comma, drop the ALSA carrier).
All 4 live ownership gates re-run green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

src/audio/ownership.rs

@@ -44,8 +44,14 @@ use std::process::Command;
 /// after binding the node — see the module docs.
 pub const OWNED_PROP_KEY: &str = "peerspeak.owned";
 
-/// Carrier 1, value. pixelpass treats anything other than `false`/`0` as
+/// Carrier 1, value.
-/// truthy (fail-closed), but we always emit exactly this.
+///
+/// ⚠️ **pixelpass matches this EXACTLY** (its round-10 R10-4). An earlier
+/// version of this comment said it accepted anything other than `false`/`0`;
+/// that leniency was removed, because it let any process suppress a rival
+/// app's audio from the share with a property it did not have to spell right.
+/// Emitting any other value here now means the node is not owned on carrier 1
+/// — only carrier 2's `node.name` prefix would still catch it.
 pub const OWNED_PROP_VALUE: &str = "1";
 
 /// Carrier 2. The `node.name` prefix pixelpass matches; the remainder of the
@@ -161,18 +167,112 @@ fn pipewire_props_value(node_name: &str) -> String {
 /// Tag a child process that will play audio, so its resulting node carries
 /// both ownership carriers.
 ///
-/// Both environment variables are set unconditionally, because we cannot know
+/// All three environment variables are set unconditionally, because we cannot
-/// from here which audio API the child will pick (mpv chooses at runtime, and
+/// know from here which audio API the child will pick: mpv chooses at runtime,
-/// VLC and `paplay` differ from `pw-play`). Setting the one the child ignores
+/// VLC and `paplay` differ from `pw-play`, and either may be configured for
-/// costs nothing.
+/// ALSA output. Setting the ones the child ignores costs nothing.
 ///
-/// Any inherited value of either variable is **replaced**, not appended: the
+/// An inherited value is **merged** rather than replaced (round 10, R10-5).
-/// tag is a correctness mechanism, and a half-merged property string that
+/// Both variables can legitimately carry a user's own routing policy —
-/// fails to parse would drop it silently.
+/// `media.role`, a target sink — and clobbering it changes where the user's
+/// audio goes as a side effect of a tagging mechanism that is supposed to be
+/// behaviourally invisible. Our pairs go **last**, so they win any duplicate
+/// key. An inherited value that does not match the expected shape is logged
+/// and overwritten: a half-merged string that fails to parse would drop the
+/// tag silently, which is the one outcome worse than losing the user's
+/// routing preference.
+///
+/// ⚠️ Reachability, measured 2026-07-25: neither variable is set anywhere in
+/// this user's environment or configuration, so this is a correctness
+/// property with no live consumer today.
 pub fn tag_child(command: &mut Command, role: &str) {
     let node_name = owned_node_name(role);
-    command.env(PULSE_PROP_ENV, pulse_prop_value(&node_name));
+    command.env(
-    command.env(PIPEWIRE_PROPS_ENV, pipewire_props_value(&node_name));
+        PULSE_PROP_ENV,
+        merge_pulse_prop(inherited(PULSE_PROP_ENV).as_deref(), &node_name),
+    );
+    let pipewire_props = merge_pipewire_props(inherited(PIPEWIRE_PROPS_ENV).as_deref(), &node_name);
+    // The ALSA carrier takes the same SPA-JSON grammar and is set here too, so
+    // a child configured for ALSA output — which neither of the other two
+    // variables reaches — is tagged with *its own* role rather than inheriting
+    // this process's `clip` tag from `tag_this_process_alsa_audio`.
+    command.env(PIPEWIRE_ALSA_ENV, &pipewire_props);
+    command.env(PIPEWIRE_PROPS_ENV, pipewire_props);
+}
+
+/// This process's value for `key`, which is what a child would inherit.
+/// Non-UTF-8 reads as absent — we cannot merge into bytes we cannot parse, and
+/// the overwrite path is where that belongs.
+fn inherited(key: &str) -> Option<String> {
+    match std::env::var(key) {
+        Ok(value) if !value.trim().is_empty() => Some(value),
+        Ok(_) => None,
+        Err(std::env::VarError::NotPresent) => None,
+        Err(std::env::VarError::NotUnicode(_)) => {
+            crate::log_msg(&format!(
+                "ownership: inherited {key} is not valid UTF-8; overwriting it with the ownership tag"
+            ));
+            None
+        }
+    }
+}
+
+/// `PULSE_PROP` is whitespace-separated `key=value`, so merging is appending.
+///
+/// The shape check is every token containing `=`. A token without one is not
+/// something we can append to safely — PulseAudio's parser would take the
+/// whole string as malformed and we would lose the tag with it.
+fn merge_pulse_prop(inherited: Option<&str>, node_name: &str) -> String {
+    let ours = pulse_prop_value(node_name);
+    let Some(existing) = inherited else {
+        return ours;
+    };
+    if existing.split_whitespace().all(|pair| pair.contains('=')) {
+        format!("{} {ours}", existing.trim())
+    } else {
+        crate::log_msg(&format!(
+            "ownership: inherited {PULSE_PROP_ENV} is not space-separated key=value \
+             ({existing:?}); overwriting it with the ownership tag"
+        ));
+        ours
+    }
+}
+
+/// `PIPEWIRE_PROPS` is a SPA-JSON object, so merging is inserting our pairs
+/// before the closing brace.
+///
+/// The shape check is the outer braces. Anything else — a bare `key = value`
+/// list, a truncated object — is left alone and overwritten rather than
+/// guessed at; a full SPA-JSON parser is not worth writing for a case with no
+/// live consumer.
+fn merge_pipewire_props(inherited: Option<&str>, node_name: &str) -> String {
+    let ours = pipewire_props_value(node_name);
+    let Some(existing) = inherited else {
+        return ours;
+    };
+    let trimmed = existing.trim();
+    let inner = trimmed
+        .strip_prefix('{')
+        .and_then(|rest| rest.strip_suffix('}'))
+        .map(str::trim);
+    match inner {
+        // An empty object carries nothing to preserve.
+        Some("") => ours,
+        Some(inner) => {
+            let separator = if inner.ends_with(',') { "" } else { "," };
+            format!(
+                "{{ {inner}{separator} \"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\", \
+                 \"node.name\" = \"{node_name}\" }}"
+            )
+        }
+        None => {
+            crate::log_msg(&format!(
+                "ownership: inherited {PIPEWIRE_PROPS_ENV} is not a SPA-JSON object \
+                 ({existing:?}); overwriting it with the ownership tag"
+            ));
+            ours
+        }
+    }
 }
 
 /// Tag the audio this process plays **through ALSA** — in practice everything
@@ -499,6 +599,130 @@ mod tests {
         );
     }
 
+    /// R10-5: a user's inherited `PULSE_PROP` survives, and our pairs go last
+    /// so they win any duplicate key.
+    ///
+    /// Tested on the merge functions rather than through `tag_child`, because
+    /// `tag_child` reads the *process* environment and a test that set it
+    /// would race every other test in the binary.
+    #[test]
+    fn pulse_prop_merges_rather_than_clobbering() {
+        let name = owned_node_name("mpv");
+        let merged = merge_pulse_prop(Some("media.role=music target.object=my-sink"), &name);
+
+        assert!(
+            merged.contains("media.role=music") && merged.contains("target.object=my-sink"),
+            "the user's routing policy survives: {merged}"
+        );
+        assert!(merged.contains(&format!("{OWNED_PROP_KEY}={OWNED_PROP_VALUE}")));
+        assert!(merged.contains(&format!("node.name={name}")));
+
+        // Still parses as whitespace-separated key=value, with ours last.
+        let pairs: Vec<(&str, &str)> = merged
+            .split_whitespace()
+            .map(|pair| pair.split_once('=').expect("key=value"))
+            .collect();
+        assert_eq!(pairs.len(), 4, "{merged}");
+        assert_eq!(pairs[pairs.len() - 1].0, "node.name");
+    }
+
+    /// Ours must win a key the inherited value already sets, or a user with
+    /// `node.name` in their environment would silently untag us.
+    #[test]
+    fn our_pulse_prop_pairs_win_a_duplicate_key() {
+        let name = owned_node_name("mpv");
+        let merged = merge_pulse_prop(Some("node.name=theirs"), &name);
+        let last_name = merged
+            .split_whitespace()
+            .filter_map(|pair| pair.split_once('='))
+            .rfind(|(key, _)| *key == "node.name")
+            .expect("a node.name pair");
+        assert_eq!(last_name.1, name, "ours must come last: {merged}");
+    }
+
+    #[test]
+    fn a_malformed_pulse_prop_is_overwritten_not_half_merged() {
+        let name = owned_node_name("mpv");
+        // No `=` in the token: not something we can safely append to.
+        let merged = merge_pulse_prop(Some("garbage-without-equals"), &name);
+        assert_eq!(merged, pulse_prop_value(&name));
+        assert!(!merged.contains("garbage"));
+    }
+
+    #[test]
+    fn pipewire_props_merges_inside_the_spa_json_object() {
+        let name = owned_node_name("vlc");
+        let merged = merge_pipewire_props(Some("{ \"media.role\" = \"music\" }"), &name);
+
+        assert!(merged.starts_with('{') && merged.ends_with('}'), "{merged}");
+        assert!(merged.contains("\"media.role\" = \"music\""), "{merged}");
+        assert!(merged.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\"")));
+        assert!(merged.contains(&format!("\"node.name\" = \"{name}\"")));
+        // Exactly one object, not two concatenated.
+        assert_eq!(merged.matches('{').count(), 1, "{merged}");
+        assert_eq!(merged.matches('}').count(), 1, "{merged}");
+    }
+
+    /// A trailing comma in the inherited object must not become a double
+    /// comma, which SPA-JSON would reject — taking our tag down with it.
+    #[test]
+    fn pipewire_props_merge_does_not_double_a_trailing_comma() {
+        let name = owned_node_name("vlc");
+        let merged = merge_pipewire_props(Some("{ \"media.role\" = \"music\", }"), &name);
+        assert!(!merged.contains(",,"), "{merged}");
+        assert!(!merged.contains(", ,"), "{merged}");
+        assert!(merged.contains("\"media.role\" = \"music\""), "{merged}");
+    }
+
+    #[test]
+    fn an_empty_or_malformed_pipewire_props_yields_our_value_alone() {
+        let name = owned_node_name("vlc");
+        assert_eq!(
+            merge_pipewire_props(Some("{ }"), &name),
+            pipewire_props_value(&name)
+        );
+        // Not an object: overwritten rather than guessed at.
+        assert_eq!(
+            merge_pipewire_props(Some("media.role = music"), &name),
+            pipewire_props_value(&name)
+        );
+        assert_eq!(
+            merge_pipewire_props(None, &name),
+            pipewire_props_value(&name)
+        );
+    }
+
+    /// A spawned player configured for ALSA output is reached by neither
+    /// `PULSE_PROP` nor `PIPEWIRE_PROPS`. `tag_child` therefore sets the ALSA
+    /// carrier too — with the *child's* role, so it does not inherit this
+    /// process's `clip` tag from `tag_this_process_alsa_audio`.
+    #[test]
+    fn tagged_child_command_also_carries_the_alsa_carrier_with_its_own_role() {
+        let mut command = Command::new("mpv");
+        tag_child(&mut command, "mpv");
+
+        let env: BTreeMap<String, String> = command
+            .get_envs()
+            .map(|(key, value)| {
+                (
+                    key.to_string_lossy().into_owned(),
+                    value
+                        .expect("tagging sets values, never removals")
+                        .to_string_lossy()
+                        .into_owned(),
+                )
+            })
+            .collect();
+
+        let alsa = env.get(PIPEWIRE_ALSA_ENV).expect("PIPEWIRE_ALSA set");
+        assert!(
+            alsa.contains(&format!("\"node.name\" = \"{}\"", owned_node_name("mpv"))),
+            "the child's own role, not this process's clip role: {alsa}"
+        );
+        assert!(!alsa.contains(&format!("_{CLIP_ROLE}_")), "{alsa}");
+        assert!(alsa.contains(&format!("\"{OWNED_PROP_KEY}\" = \"{OWNED_PROP_VALUE}\"")));
+    }
+
     #[test]
     fn every_generated_name_matches_the_prefix_pixelpass_looks_for() {
         for role in ["call", "mpv", "vlc", "notify"] {