Round 12 review, finding 2 — filed as P2, and the interesting part is
that its author retracted it to P3 once we had measurements, while the
remedy it originally proposed would have been a fail-open.
The finding was that our validator rejects nesting `spa-json-dump -s`
accepts, and the suggested fix was a recursive sub-iterator walk to
match the dump tool. Both halves rest on the dump tool being the
reference. It is not. Nothing reads `PIPEWIRE_PROPS` or `PIPEWIRE_ALSA`
with `spa-json-dump`; `pw_properties_update_string` does, in the client
process.
Measured live on this host, against the real ALSA plugin:
depth 513 dump accept plugin accept ours accept
depth 514 dump accept plugin accept ours REJECT
depth 515 dump accept plugin REJECT ours reject
depth 1000 dump accept plugin REJECT ours reject
At 515 the plugin discards the whole object: the node came back as
`alsa_playback.aplay` with no properties at all. So matching the dump
tool would have made us splice carriers into values the consumer throws
away wholesale — losing both, which is the echo this feature exists to
prevent. Over-rejecting costs a routing preference; over-accepting costs
a carrier. Those are not the same price.
What was genuinely wrong is narrower: we sat exactly one level below the
consumer. `pw_properties_update_string` calls `spa_json_container_len`
on a container value, which enters one more sub-iterator before its flat
walk, and that single level is the entire discrepancy. Doing the same
puts the boundaries on the same number.
Codex reached the same three numbers independently by calling
`pw_properties_update_string_checked(NULL, ...)` directly, having
disassembled both call sites; I measured through the live plugin. Two
methods, one table.
The dump differential stays, but it is now labelled a *grammar* oracle
with a warning not to add deep values — it would fail by design. The
acceptance oracle is the new boundary test.
Mutation-verified: removing the container step fails the 514 assertion.
622 -> 623 lib tests, fmt clean, clippy clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 12 review, finding 1 — a measured fail-open, and the third
distinct door into the same failure.
A trailing comment is valid SPA-JSON and *ends the document*
(`case __COMMENT: return 0` in spa/utils/json-core.h), so an object may
close before the last `}` in the string. `merge_pipewire_props` located
the closing brace with `rfind('}')`, which is a byte scan and not a
parse, so for
{ "target.object" = "my-sink" } # trailing }
it selected the comment's brace and spliced both ownership carriers
*into the comment*. The re-validation did not catch it, because the
result parses perfectly well — as `{ target.object = "my-sink" }`, with
neither carrier present. Confirmed against `spa-json-dump -s`.
That is an untagged node, so no taint root, so echo — exactly what
rounds 10 and 11 each closed by a different route. Latent rather than
live: pixelpass's evaluate() is still audit-only, so today it corrupts
an audit classification and becomes a leak when phase 6 consumes
eligibility.
The whole thesis of round 11 was "do not re-implement someone else's
grammar". The scanner went, but this brace hunt stayed behind in the
caller, which is the same defect wearing different clothes.
So spa_object now reports the object's own closer, taken from libspa:
closing a container at depth 0 writes the brace's position back to the
parent iterator, and spa_json_enter made `outer` that parent. Read
before the trailing check, which advances past it.
Also:
- whatever followed the object is preserved, so a user's trailing
comment survives instead of being silently deleted;
- the output check now asks whether the object closes where we put our
brace, not merely whether the string parses. A parse-only check is
what this finding defeated.
Mutation-verified: restoring `rfind` fails the new test, and dropping
the tail fails it on the deleted comment. Honest note in the code —
mutation cannot distinguish the closer comparison or the is-object
test; both are labelled belt-and-braces rather than presented as
tested.
621 -> 622 lib tests, fmt clean, clippy clean, and the ignored
spa-json-dump differential still agrees.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 11 review, findings 2, 3 and 4.
The round-10 fix replaced a brace check with a hand-written scanner. That was
the wrong shape: a second implementation of someone else's grammar drifts in
both directions at once, and measured against `spa-json-dump -s` on this host
it did.
It ACCEPTED `{ "foo" = { garbage } }` (only brackets were balanced, contents
never validated), `{ "a" = "\é" }`, `{ "a" = é }` and `{ "a" = foo\bar }`.
Merging into those put an invalid pair before our carriers, so the daemon
stops at it and drops both -- recreating the exact fail-open the round-10 fix
existed to close. Its own test even pinned `"\é"` as a valid token.
It REJECTED `{ target.object, "my-sink" }`, `{ key == "value" }` and
CR-terminated comments, all valid -- so a user with one of those in their
environment silently lost their routing policy to an overwrite. That half
affects a running Linux user.
Now libspa's own parser validates, and the merge splices into the validated
text instead of re-emitting parsed pairs. Splicing preserves the user's bytes
exactly, which also answers the review's point that re-quoting a bare key can
invent a different one (`foo\bar` -> a string with a \b escape). Three
measured properties make the splice safe -- the last `}` is the object's, a
validated object's brace is never mid-comment, and commas are pure separators
-- and the result is validated again before it is returned.
Mutation testing then deleted the rest: every pairing and recursion check I
had written turned out to be redundant, because spa_json_next already errors
on `{ garbage }` and on nested garbage, and skips containers rather than
descending. ~60 lines of my own grammar logic removed. What remains is gated
by a new differential test against `spa-json-dump -s` over a 27-value corpus
-- the check whose absence caused this round. It found a real disagreement on
its first run (a bare document, which we reject by design, not by accident).
One mutation HUNG rather than failed: dropping the `length < 0` check makes
libspa report the same error without advancing, spinning forever. Kept, now
labelled load-bearing for termination, with a token-count bound beside it.
Finding 4: the ordering test took the first textual match of `fn main`, so a
raw-string decoy above the real function satisfied it while the real one
spawned a thread first. Now requires each of the three anchors to be unique.
Mutation-verified with the review's own decoy.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verification round on the round-10 review fixes.
Adds the property the whole of finding 3 is about, stated directly: over
20,000 deterministic inputs built from the exact characters that break
SPA-JSON (braces, brackets, quotes, separators, comment marks, escapes,
newlines, multi-byte characters), the merge always emits both carriers in an
object it can read back. Either outcome — parse and rebuild, or overwrite —
has to end that way, and now nothing can quietly change which.
Also replaces two byte-index steps with character-boundary steps. Both were
correct on the ASCII input they actually see, but `index + 1` after a
reverse find would have split a multi-byte character and panicked the slice.
scan_token gains multi-byte cases for the same reason.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 10 review, finding 6. The cross-repo contract still documented carrier
1 as "any value other than false/0 is truthy" after R10-4 made pixelpass
match it exactly. A future producer following the fixture could emit "true"
and silently lose the carrier.
Committed byte-identical with pixelpass's copy in the same session, as the
file's own rules require.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 10 review, findings 2 and 5.
Finding 2 — R10-2's rationale for tagging local playlist audio was factually
wrong. It claimed a local track is "already being broadcast to peers on the
same keypress", but shared listening is opt-in: music_broadcast defaults to
false, play_music_index starts local playback unconditionally, and
broadcast_track returns immediately when can_broadcast_music is false. So a
default-config playlist is not already broadcast.
The tag stays, now as an explicit policy with the real reason: the carriers
reach rodio through PIPEWIRE_ALSA, which is process-wide, and clip_player
and music_player are two ClipPlayer instances in one process — no value of
that variable can tag one and not the other. Exempting the playlist means
giving it a separately taggable stream, which is a large change for a case
with a one-step workaround (play it in any other app). Tagging is not
optional for received clips and peer music, which are the far end's own
audio.
Finding 5 — the ordering test proved only "before run_gui", which a
thread::spawn inserted above the tag still satisfies while making the
set_var a data race. It now requires the tag to be the first executable
statement in main: attributes, `unsafe` and block punctuation are stripped,
and any residue fails. Mutation-verified against a spawn, an unrelated
statement, and the call deleted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round 10 review, finding 3. The merge's shape check was the outer braces
only, so an inherited `PIPEWIRE_ALSA='{ garbage }'` was spliced into rather
than overwritten, producing an object the daemon does not accept.
Measured live 2026-07-25, and the failure is worse than a rejection: with
PIPEWIRE_ALSA set to the old merge's output, a real aplay node came up as
node.name=alsa_playback.aplay, no peerspeak.owned, and a junk property
`garbage = "peerspeak.owned"` — the lenient parser ate our key as their
value and stopped. Both ownership carriers lost on a live
Stream/Output/Audio node, which is an echo.
So: parse the inherited object and REBUILD it with our pairs last, rather
than splicing before the closing brace. Rebuilding is what makes the result
independent of the input's formatting — a value ending in a `#` comment
would otherwise swallow everything appended after it.
The three values the new merge emits were verified against the live daemon
(user props preserved, both carriers present) and are pinned byte-for-byte.
scan_token is gated on its own postcondition: at the object level an
unterminated string is also caught by "the object never closed", so the two
implementations only disagree at the seam.
Also parameterizes the malformed-value warning, which always named
PIPEWIRE_PROPS even when PIPEWIRE_ALSA was the malformed one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verification round on round 10's own fixes, not on the next layer.
R10-5 preserved a user's PULSE_PROP and PIPEWIRE_PROPS but
tag_this_process_alsa_audio still clobbered their PIPEWIRE_ALSA, which is
the same kind of routing policy and deserves the same treatment. Both it
and tag_child now merge.
MEASURED, rather than assumed, because "our pairs go last so they win"
was load-bearing for the whole merge design and was never checked:
PIPEWIRE_PROPS='{ "node.name"="theirs_first", "media.role"="music",
"node.name"="ours_last" }' on pw-play
-> node.name=ours_last, media.role preserved.
The PULSE_PROP equivalent on paplay -> the same.
So last-wins holds on both grammars: a user who already sets node.name
cannot silently untag us, and their other keys survive.
That also makes tag_child's ALSA carrier merge from the inherited value
safely: in production main has already put this process's `clip` tag
there, and the child's own role now overrides it by coming last. The
existing row could not see this — the test binary never runs main, so it
only ever exercised the merge-into-nothing case. Added a row that drives
the real shape directly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
ClipPlayer opens a rodio default sink, which on Linux reaches the graph
through PipeWire's ALSA plugin. It was untagged through all of phase 1,
and it is a real echo path: B broadcasts music, A tunes in, A shares
their desktop, B hears their own track played back at them. Confirmed
live as `alsa_playback.peerspeak-...` with no ownership properties.
rodio exposes no way to set PipeWire node properties, so the carrier is
PIPEWIRE_ALSA, set once at the top of main while still single-threaded.
Measured, with PIPEWIRE_PROPS and PULSE_PROP unset, to establish that
setting it process-wide is safe:
- aplay (ALSA plugin) -> both carriers land. Confirms the mechanism.
- pw-play (native) -> untouched. Our own call-playback and capture
streams are native, so they keep their own
explicit tagging and are unaffected.
- arecord (ALSA capture)-> IS tagged, on a Stream/Input/Audio. Not
surgical in the role dimension; harmless only
because R10-1 honours the carriers on
producers alone. This is why R10-1 lands first.
Local playlist tracks are tagged too, not just inbound peer audio. A
local track is already broadcast to peers over the call on the same
keypress, so sharing it again through the screen share would send the far
end two copies at differing latency. That is a defect, not a feature.
Codex phase-1 review F1. Round 10, R10-2.
New live exit-gate row drives the real ClipPlayer; mutation-verified
(drop the tag -> no node within 5s). The wiring guard is mutation-
verified too, and its first version was WRONG: it searched raw source and
passed against a main with the call deleted, because the comment above it
named the function. It strips comments now.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Producer half of the same fix (Codex phase-1 review, finding 3, P2).
This side collected fixture lines into a map, so a duplicated key
silently took the last value while pixelpass took the first — both
repos green on different contracts.
Mutation-verified in both repos with a duplicated `prop_value`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment said aplay ignores PULSE_PROP/PIPEWIRE_PROPS. Measured:
it reaches the graph through PipeWire's ALSA plugin and carries both
carriers exactly like pw-play and paplay. Comment only.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Zero behaviour change. This is what makes the screenshare exclusion
engine able to see us at all (plan §5.1, impl plan §3): pixelpass must
refuse to fan out our own playback, and until now it had no way to
recognise it.
Two carriers, matched by pixelpass as a union — `peerspeak.owned=1`
and a `node.name` prefix `peerspeak_owned_<role>_<pid>`. Round 8 added
the second after the phase-5 audit found a node property is invisible
to the PipeWire registry `global` event and recoverable only by
binding the node; the prefix is announced directly. A union is also
the fail-closed direction: a missed tag leaks call audio into a share,
a spurious one only over-excludes.
Three tagging sites, all three verified live on this host:
- native call playback → props on the stream dict
- screenshare mpv/VLC → PULSE_PROP + PIPEWIRE_PROPS on the child
- notification chimes → same, on pw-play/paplay
The literals are a cross-repo wire contract, so they appear once here
as named constants and are pinned in a fixture committed byte-identical
in both repos (tests/fixtures/ownership-tag-contract.txt). The contract
test is black-box: it builds a real child `Command` and reads back the
environment it would carry, rather than testing our own formatter.
Three live `#[ignore]`d exit-gate tests drive the real call sites and
poll `pw-dump` for the resulting node — the plan requires the tag be
shown landing on a live node, not just in the env. All three
mutation-verified (drop either carrier, or the role, and the matching
gate fails).
Measured while verifying: mpv, VLC, pw-play and paplay all honour
`node.name` from those env vars. The native stream set neither
`application.name` nor a description, so a mixer fell back to
`node.name` — which the tag turns into an internal identifier. Added
an explicit `node.description = "PeerSpeak"` there, which keeps the
plan's rule (the prefix must not reach `node.description`) while
preserving its intent: mixers stay readable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 3r landed §6.7 and the phase-5 audit was re-run against it immediately.
It found a second measured defect within minutes: a real hardware sink
carrying `unresolved-ancestry` permanently, from one link observed while its
output node was still unbound during enumeration. Round 8 made that
systematic rather than rare, because every node is now withheld until its
bind resolves.
New §6.8: sticky taint is a claim about history, and uncertainty is not
history. Retiring by reason code would not be enough — an unresolved node
propagates `TaintedUpstream`, which is indistinguishable from real
contamination once recorded — so the split is by provenance: the engine runs
its fixpoint twice, and only the evidence-only pass may feed sticky state.
Decisions are unchanged and still fail closed.
Also recorded in §6.8, both from Codex's round-9 review and both pre-existing:
hardware playback-to-capture paths ("Stereo Mix") defeat the `session_device`
classifier in a way the driver denylist cannot detect — a real echo path
needing a design call — and the 2 s readiness budget has no calibration
argument beyond one measurement on one idle desktop.
Impl plan: phase 3r marked built and merged with its gate results, including
the extra Device-side live gate and why row 1 alone could not cover it.
The phase-5 dry-run gate failed on its first live run: the engine built to
v3.4 could not see its own primary taint root (echo, AEC off) while excluding
every stream on the machine (silence). One cause — the PipeWire registry
`global` event carries only a filtered subset of an object's properties, and
eight the design depends on are never announced.
Design doc (v3.4 → v3.5):
- NEW §6.7 — the observation boundary. The global is an index, not a source of
truth: bind every Node and Device, `info` props are the sole source, live
prop tracking, one readiness obligation per unbound node, fail closed.
Four user design calls recorded.
- §5.1 — a second, registry-visible tag carrier (`node.name` prefix) alongside
`peerspeak.owned`, so the primary root does not rest on one mechanism.
- §6.4 — node/device props are not an optimisation to skip, they are
unavailable from the global; the round-6 Link lesson was right and applied
to exactly one object type.
- §6.1.0, §6.1.4 — the two corrections the impl plan owed v3.5: a
time-dependent "hazard is LIVE" claim, and an unreachable nominated test
case (twice over).
- §9.1 measured facts, §12 rig discipline (pw-dump binds; the registry does
not), §14 readiness.
Impl plan:
- NEW phase 3r with a four-part exit gate, the first the direct inverse of the
finding. Ports deliberately not bound in v1, with a revisit trigger.
- Phase 1 pins the second carrier literal as a cross-repo contract.
- Phase 5 marked GATE FAILED; matrix and O5 re-run after 3r and 1.
- Risk register: the over-exclusion row fired and worked; new row for the
observation boundary class.
Architecture is unchanged and vindicated: fed correct properties, the engine
decided correctly in every fixture. The §5.1 exact-partition requirement is
what caught this — every exclusion was defensible and the eligible half was
empty.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Impl plan §5's required results file. Phase 6 does not start.
F1 (fatal): the PipeWire registry `global` event delivers only a filtered
subset of node properties, and eight of the properties the phase-3 adapter
reads are not among them — peerspeak.owned, pulse.module.id, node.link-group,
application.process.id, node.passthrough, device.api, factory.name,
alsa.driver_name (plus port.exclusive on Ports). They are silently absent, so
the primary taint root never fires, the AEC identity can never validate, and
session_device is universally false. Measured on PipeWire 1.6.8 /
WirePlumber 0.5.15, with the full announced key set for all five object types
recorded. Links and Clients are unaffected; pulse-PID derivation works.
F2: with F1 in force no node has a strong owner key, so any tainted capture
stream is an unbounded tainted reader and phase 2's fail-closed backstop
excludes every Stream/Output/Audio on the machine. Fail-closed, so silence
rather than echo — but entirely non-functional, and non-functional in a way an
exclusion-only checklist would have scored as passing. The eligible half of
the §5.1 partition is what caught it, exactly as the plan argued it would.
The fix direction is measured and recorded: binding each Node and reading its
info props recovers every missing property, which is the pattern phase 3
already built for Links. factory.id is not a shortcut — it resolves to
"adapter", not api.alsa.pcm.sink.
O5 is closed with ~4 orders of magnitude of headroom: 308 graph events in
6.5s under churn, every recompute under 50us (max 15us), busy fraction 0.0004.
Caveat recorded — measured on the degraded graph, and the F1 fix adds
per-node bind I/O this run did not measure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Turns the converged v3.4 design into ordered phases with falsifiable exit
gates. Three adversarial review rounds with Codex (gpt-5.6-sol, xhigh);
findings adjudicated rather than accepted wholesale, with reachability
verified against source on both sides.
Structural decisions:
- Phase 0d closes BOTH unsafe paths into the capture (source string and
capture-sink inputs) before any machinery that could take them exists.
- Phase 5 dry-run audit mode is a hard gate: the taint engine runs against
the live graph, creating no links, asserting exact eligible/excluded
partitions with reason codes.
- Link manager is deliberately last among the pixelpass components.
Two measured corrections owed back to v3.4 (plan §11): §6.1.0's "hazard is
LIVE right now" has already flipped and must not be gated on, and §6.1.4
nominates an unreachable test case (as did my first replacement for it).
Design approval only. No code, nothing approved for merge.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Checked ~/.config/peerspeak/config.json: output_device and input_device are
both pinned to the Arctis, so echo_cancel::enable always passes sink_master
explicitly and the AEC binds to real hardware regardless of Sunshine owning
the default sink. Not live for this user.
Kept as a low-priority general defect: on "system default", the master args
are omitted (echo_cancel.rs:89-94) and module-echo-cancel binds to whatever
the default is, which on a box like this one is a null sink. Hardening would
be to resolve and validate the default before load. Own task, not this
feature.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round 7. Codex ratifies: v3.3 is ready to become the implementation plan.
Seven rounds, every blocker fixed or refuted with evidence. Design approval
only — nothing approved for merge, no code written.
Two subtle catches from the ratification round, both applied:
- The owner-key union had a wording trap that would have preserved the exact
bug it was written to fix. "Resolves" must mean "yields a MATCH between the
two legs", not "first property present on the node" — client.id IS present
on both gst-launch legs but differs, so a first-present implementation stops
at key 3, sees a mismatch, concludes "different owners" and leaks. Now
specified as try-in-order-until-equal, with a dedicated test.
- Sticky taint must be lifetime-aware, not keyed on raw ids. client.id, node
ids, module indices, link-groups and PIDs all recycle on this stack, so a
bare key would hand an unrelated future app permanent inherited taint.
Stored against live owner components, cleared only when all members vanish.
Also added: how pixelpass learns the pipewire-pulse PID itself (consistent
pipewire.sec.pid across Pulse clients, validated against /proc/<pid>/comm),
with the failure modes in both directions — safe only because unresolved
ancestry is fail-closed, which is the invariant the section rests on.
NEW LIVE FINDING (§6.1.0), the strongest reachability evidence yet and one
Codex's sandbox could not have seen: the user's CURRENT DEFAULT SINK is
sink-sunshine-stereo, a support.null-audio-sink. Every hardware sink is
SUSPENDED; the only RUNNING sink is Sunshine's virtual one, with Firefox
playing into it and sunshine reading its monitor. The hazardous forwarder
topology is live in the default audio path full time, with no EasyEffects
involved. It also means the rejected hardware-sink-only shortcut would have
captured NOTHING on this machine. Flagged separately, explicitly UNVERIFIED:
what module-echo-cancel binds to when the default sink is an app-owned null
sink.
§12 expanded with a graph-engine test surface (node-local tests cannot catch
C2/C3-class defects). §14 rewritten: convergence table, agreed v1 scope, and
what is deliberately out.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round 6. Codex disagreed with two of my three round-5 claims and was right
about both; I had independently refuted one of them with a sharper test.
C2 REFUTED (by my own measurement): client.id is NOT an owner bridge. One
gst-launch process doing capture+playback produced TWO client objects (209
input, 210 output), no link-group, same application.process.id 20172. So
client.id bridges a *connection*, not an owner, and GStreamer — the same
framework pixelpass uses — splits them by default. Replaced with a
conservative union, strongest first: node.link-group, owned pulse.module.id,
client.id, node application.process.id, else fail closed.
With a trap Codex did not flag: application.process.id is pipewire-pulse's
PID for module-created streams, so bridging on it would fuse every Pulse
module's legs into one owner and mass-exclude tunnel/RTP/loopback audio the
user may legitimately want shared. Never bridge on that key when it equals
the pipewire-pulse PID; keys 1-2 already cover those precisely. PID thus
returns to the design in the CORRELATION role while remaining unusable in
the IDENTITY role — and in that role a wrong answer fails closed.
C3 CONCEDED: taint must be STICKY. Current-topology taint forgets buffered
audio — an app that reads a tainted monitor, buffers, then closes its input
leg would be relinked while still emitting peerspeak audio from the buffer,
and no graph event marks the drain. Taint now persists per owner until its
nodes disappear. Added §6.1.4 quantifying the arrival-side window (~10.6-21.3
ms quantum plus scheduling) and noting it is zero when taint roots already
exist, which is the common case.
C1 SUSTAINED with Codex's caveat: node-granular traversal is free for the
monitor boundary, but over-taints Audio/Duplex nodes. Fail-closed, accepted
for v1, documented as a known contradiction of the "Firefox with a mic stays
shareable" promise on duplex devices.
S1: endpoint props demoted to an optimization; bind-LinkInfo fallback is the
correctness path. S2: readiness epoch + revalidate before each link creation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round 5. Codex and I converged independently on the same conclusion — a
Link-only ancestry walk does not catch the leak — it from the crate/header/
WirePlumber sources, me from the live graph. Its sandbox could not reach the
daemon (pw-dump: Operation not permitted), so the measurements are mine.
Reproduced the EasyEffects topology with module-null-sink + module-loopback
(same shape, no EasyEffects needed). Result: there is NO Link object between
a forwarder's input leg and its output leg. Walking upstream from the leaking
node over Links alone finds no inbound links at all — a dead end that reads
as "clean". The legs are related only by shared node.link-group / client.id /
pulse.module.id.
So the signal graph needs three edge types:
1. Link edges — measured: registry Links carry all four endpoint props.
2. Sink-monitor — measured FREE at node granularity: the monitor connection
IS a real Link whose output node is the sink itself. Codex held that this
must be modelled explicitly; that is true only for a port-granular walk.
Taint walks at node granularity, links are created per port.
3. Owner bridge — node.link-group when present, else client.id (measured
shared across the forwarder's legs, distinct per app). Only modules set
link-group, so client.id is what covers ordinary apps.
New §6.1.1: bridge taint must be CONDITIONAL on the input leg being tainted.
"Client has both legs ⇒ exclude" would exclude every app using a microphone.
Firefox in a Meet call stays shareable; Firefox sharing desktop audio does not.
Also: §6.5 rejects the cheap "hardware-sink-only" predicate with a measurement
— the forwarder's output leg links directly to alsa_output, so the shortcut
passes the leak and excludes the innocent app, backwards on both halves.
§6.3 barrier corrected: core sync/done is a previous-work roundtrip, not graph
quiescence. §6.4 adds crate version, endpoint fast path + bind fallback, and
full-recompute cost. §5.2 correction 5 rewritten: application.process.id lives
on the Node and is the app's own PID; pipewire.sec.pid lives on the Client and
is pipewire-pulse's for every Pulse client. That resolves four rounds of
contradictory PID claims.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round 4 (review-2026-07-21-design-v3-round4.md) returned 5 findings, 3 of
them blocking. All claims re-verified against source before acceptance.
Biggest correction: eligibility is a GRAPH property, not a node property.
Exclusion does not propagate downstream — a filter-chain/loopback/combine-sink
re-emits the mix as a fresh untagged Stream/Output/Audio that passes both the
peerspeak.owned and pulse.module.id checks, re-injecting the whole call into
the share. Reachability confirmed: easyeffects IS installed on this machine
(it merely wasn't running during the fan-out spike, which is why the spike
missed it). §6 rewritten around transitive upstream reachability, tracking
Node/Port/Link globals, with a registry sync barrier and revalidation
immediately before each link creation.
Also applied:
- §5.3 is now a bounded validation state machine, not a one-shot check.
wait_for_nodes only waits for the virtual source/sink, never the playback
hazard leg, and pixelpass capture spawns lazily on first viewer, so the
one-shot check raced in both directions. Revocation redefined as loss of
the module identity, not transient absence of one leg.
- §7.2: reordering ActiveSession fields is NOT sufficient — kill_on_drop
sends SIGKILL without waiting, so AEC can still unload while pixelpass
lives. Fix is explicit shutdown().await at both channel-close breaks,
field order as defence in depth, plus a fake-resource ordering test.
- §5.1 relabelled implementation sites; none of them tag anything today.
- Stop Share citation corrected to :699/:3480.
- D1-D7 resolved; readiness section added.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
v1/v2 described a move-based design that Option C superseded on 2026-07-20,
and the AEC playback-leg identity gate has since passed. Roughly two thirds
of v2 documented problems Option C does not have, so this is a rewrite rather
than a patch (v1/v2 remain at 88ad5a0 / 10203e1).
Folds in: the four AEC gate results, the five corrections that constrain them
(observed correlation not a contract; exact-equality only; index/link-group
reuse and node-id recycling; group prefix = hazard detection not ownership;
application.process.id == pipewire-pulse for module-created streams), the
verified implicit-drop ordering defect in ActiveSession, fail-closed
validation/revocation, the IPC shape, and the split-out prerequisites.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ran the direct-link spike on the live graph (PipeWire 1.6.8,
WirePlumber 0.5.15). Fan-out carries full-level audio for paplay, mpv
and VLC while the application keeps its existing speaker link;
WirePlumber does not reap foreign links across default-sink switch,
suspend/resume or 100s steady state; and non-lingering links are
destroyed automatically when their owning connection is SIGKILLed.
The decisive result is that destroying the capture sink mid-share left
the application playing to its speakers undisturbed, so capture-side
failure degrades to "not captured" rather than breaking the user's
audio. That is the property the move-based design had to work hard to
approximate.
Records what the spike does not prove: fidelity beyond signal presence,
daemon restart, quantum perturbation, and exclusive/passthrough streams.
The capture null sink is still pactl-owned, so Stop Share continues to
leak a module every time and the graceful-stop work is still owed.
Eligibility becomes a broad guarded selector rather than a narrow
allowlist, since copying no longer risks disturbing the source.
Option A and its attendant cleanup, restore and output-switch machinery
are retained for the record but are no longer the plan. A v3 rewrite is
owed once the AEC playback-leg identity is settled.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rewrite after Codex's adversarial review of v1 found four release
blockers, all independently verified against source.
v1's premise was wrong: whole-desktop capture bypasses Routing::start
entirely (pipeline.rs:121), so this needs a new capture mode rather than
an inverted predicate.
v2 replaces PID-based identity with ownership by inherited tag, and makes
the router an allowlist so unrecognized infrastructure is left alone
rather than optimistically moved. Graceful stop becomes a prerequisite:
Stop Share is currently SIGKILL, so cleanup never runs on the normal path.
Records live measurements taken 2026-07-20: PULSE_PROP tagging reaches
the graph for paplay, mpv and VLC, and application.process.id is the
client's own PID, not pipewire-pulse's — correcting a claim both the
review and v1 relied on.
Adds Option C (fan out a second owned link instead of moving streams),
which deletes most of the cleanup, latency and multi-host problems the
move-based design has to solve. Not yet implemented; gated on a
feasibility spike.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-20 02:54:53 -04:00
11 changed files with 4219 additions and 6 deletions
| 6 | Link manager + status events, driven through the real host path | pixelpass | **yes — first mutation** | link-manager matrix (plan §6.1) + live dynamic matrix |
| 7 | **Public mode selector** + capability advertisement | pixelpass | no | old-peerspeak/new-pixelpass golden test |
| 8 | peerspeak integration (mode flag, argv, picker, UI, status) | peerspeak | no | new/new argv golden (mode **and**`--aec`); new-peerspeak/old-pixelpass golden; causal status delivery |
| 9 | Rig upgrade + field matrix | both | yes | 🚦 **SHIP GATE** — plan §7 |
**Landing DAG** (development may be concurrent; *landing* order may not):
v3.4 §6.3 and §6.4. Replaces (not extends) the existing router, which watches Node and Metadata
adds, forwards raw removals, and binds no graph (`src/host/audio.rs:523-585`).
> ⚠️ **Built and merged, then superseded in part by "Phase 3 revision (round 8)" below.** This
> section's node-property requirements assume the registry `global` event carries them. It does
> not (v3.5 §6.7). Everything here about removals, the readiness epoch, PID derivation and the
> Link path is unaffected and still holds.
- Node, Port, Link **and Client** globals; adds **and removes**.
- Link endpoint props from the global are the **optimisation**; the bind-`LinkInfoRef` fallback
is the correctness path.
- Readiness = `core.sync()`/`done`**plus** no outstanding required observations, fail-closed
timeout. Log which condition released the epoch.
- pipewire-pulse PID derivation (v3.4 §6.1.2): consistent `pipewire.sec.pid` across Pulse
clients, validated against `/proc/<pid>/comm`.
**Exit gate — six parts.** A one-time `pw-dump` diff passes while Port observation is absent,
removals are ignored, the fallback is dead code, and readiness releases early:
| gate | proves |
| --- | --- |
| adapter tests: add **and remove** of all four object types | removal handling exists |
| forced-absent Link endpoint props | the bind fallback is live, not decorative |
| unresolved-observer timeout test | readiness fails closed |
| readiness does not release with an observation outstanding | the epoch means something |
| **PID derivation matrix** (round-2): consistent valid PID · inconsistent PIDs · missing client property · `/proc` entry missing · `comm` mismatch · PID reuse — **every failure makes owner-bridge key 4 unusable** | the pure engine can be correct on a wrong context; this is where the context is built |
| **live**: create and destroy a controlled node/link topology; diff Nodes, **Ports**, Links and Clients before/during/after | the adapter tracks a *changing* graph, not a static one |
### Phase 3 revision (round 8) — bind every Node and Device ✅ BUILT AND MERGED 2026-07-25
v3.5 §6.7. Phase 3 shipped reading node properties off the registry `global` event, where
**eight of them are never announced**. This is the fix. Scope is the observer only — phases 2
and 4 are unaffected, and the phase-5 audit machinery is already correct.
> **🟢 Done.** Pure core + adapter, split-seam with mutual review as in phase 3 (mine and
> Codex's respectively, each reviewing the other). All four gate rows below pass, the live
> row on this host. Codex's review of the core found no certain P1; two findings taken and
> mutation-verified (a `device_props` ambiguity test that checked for one live *Device*
> rather than one live *global*, and `device.api` corroborating by presence). Two findings
> left open as design items, both pre-existing — hardware playback-to-capture paths and the
> readiness-budget calibration, both recorded in design §6.8.
>
> **Added beyond the spec: a second live gate for the Device-side path.** Row 1's
> `session_device` assertion is satisfied by a union, and WirePlumber 0.5.15 copies
> `device.api`/`alsa.driver_name` onto ALSA nodes on this host — so row 1 passes through the
> node fallback and would keep passing if the Device bind delivered nothing at all, leaving
> §6.7 decision 4 ungated on the development machine. Verified by mutation: breaking the
> Device-side driver read fails the new test while row 1 still passes.
**Requirements.**
1.**Bind every `Node` global**, unconditionally, no `media.class` filter. Retain the proxy
and its `info` listener in that global's slot in the existing per-id FIFO
(`LiveGlobal.bound_link` generalises to a bound-proxy slot).
⚠️ The phase-3 review's finding 3 — record the id and apply the add as **one** step, so
the proxy FIFO stays lockstep with the model's `live_ids` — now applies on the **hottest**
path in the observer. A recycled Node id must not pop another generation's proxy.
2.**The global is an index; `info` is the source of truth.** Read from the global only what
must exist before the bind resolves: `object.serial` (identity), the object's id, and
`device.id`/`node.id` linkage. **Every** taint-relevant property — including `node.name`
and `media.class`, so there is exactly one source — comes from the bound `info` props.
3.**A node with no `info` yet is WITHHELD from the snapshot and is a readiness obligation**
(`pending_nodes`, beside `withheld` and `pending_links`). `graph_ready` false while any is
outstanding; the existing bounded deadline makes an unresolvable bind sticky-`TimedOut`,
fail closed. No provisional-ownership admission, ever (v3.4 §6.1.3).
4.**Track props for the node's lifetime.** On a later `info` with `PROPS` in `change_mask`,
re-read, re-classify, and apply a `NodePropsUpdated` event.
⚠️ **Suppression rule:** a prop update may be dropped **only** when the resulting
`Projection` is identical to the current one. Anything looser breaks phase 4's
no-coalescing contract; anything stricter (emitting on every `info`, including
state-only changes) inflates the O5 event rate with non-events.
5.**Bind every `Device` global** and read `device.api`**and**`alsa.driver_name` from its
`info` props — authoritative, and the phase-3 review's owed fix (on PipeWire ≥ 1.2.6 with
WirePlumber < 0.5.13 the driver name is not copied to the node, and the fail-closed
absent-driver rule would over-exclude real cards). `factory.name` exists only on the node.
`classify` takes both sides; node values are the fallback, Device values win.
6.**Ports are NOT bound in v1 — an explicit accepted limitation.**`port.exclusive` is the
only port property missing from the global, and it guards a *mutation* (don't fan out into
an exclusive port), not echo: an exclusive port rejects the second link, so phase 6 sees a
clean link-create failure it must handle correctly anyway. Binding ~21 more objects at
rest to pre-empt an error that surfaces safely is not worth the obligation surface in v1.
**Revisit trigger:** any phase-6 link-matrix row where an exclusive-port link failure is
not cleanly recoverable. (`node.passthrough`, the *other* half of that §6.2 row, is a node
property and **is** recovered by this revision.)
**Exit gate — four parts.** The first is the direct inverse of the F1 finding.
| gate | proves |
| --- | --- |
| **live prop recovery**: a `module-null-sink` tagged `peerspeak.owned=true` plus a `module-loopback` reading its monitor — assert the projection carries `peerspeak.owned`, `pulse.module.id`, `node.link-group`**and**`factory.name`/`device.api`/`alsa.driver_name` on a real ALSA node | the eight properties actually arrive — F1 cannot recur silently |
| **pure-model prop-update matrix**: props-changed → re-classified; identical props → suppressed; a `session_device`-relevant change flips classification. (A *live* prop mutation has no reliable CLI trigger — the pure test is the gate, a live sighting is opportunistic) | the lifetime-tracking path exists and its suppression rule is exact |
| **readiness with node binds**: no projection reports `graph_ready` while a node bind is outstanding; an `info` that never arrives ends in sticky `TimedOut` | withholding and fail-closed timeout still hold with the new obligation class |
| **recycled Node id under churn**: repeated add/remove of the same id; no proxy leak, no cross-generation misattribution | the FIFO lockstep rule survives being moved to the hot path |
**Then re-run the whole phase-5 §5.1 matrix and re-measure O5** with bind I/O included — the
existing numbers were taken on the degraded graph and inherit nothing.
### Phase 4 — AEC identity validation state machine, read-only
| 1 | `module-null-sink` + `module-loopback` forwarder (the v3.4 §6.1 measured shape) | output leg, reason = **owner bridge**, naming the key — *not* a Link walk | same forwarder shape with **no** tainted input |
| 1b | *opportunistic, non-gating:* Sunshine's null-sink topology while it is routing desktop audio | its forwarder leg, if a re-emitting leg exists | — |
| 2 | `gst-launch pulsesrc ! pulsesink` split clients, input **explicitly rooted on a tainted monitor** | output leg via key 4 | the same process reading an **untainted** source |
| 3 | **two** Pulse modules; **one** tainted input | the tainted module's output only | **the other module's output must be ELIGIBLE** — this is what makes wrong pipewire-pulse-PID fusion observable |
| 4 | peerspeak native call playback | that node, reason = tag | — |
| 5 | peerspeak-spawned **mpv** (watched share) | that node, reason = tag | mpv launched by hand |
| 6 | peerspeak **notification** sound | that node, reason = tag | — |
| 7 | a **second** pixelpass host's capture sink, **plus a controlled forwarder reading that sink's monitor** | the forwarder's **named output serial** (cycle prevention, v3.4 §6.2) | — |
SIGKILL, last viewer, pipewire-pulse restart, PipeWire daemon restart, `--repair`.
4. EasyEffects running for the whole share.
5. Output-device switch mid-share via the real `Ctrl+Meta+F` / `Ctrl+Meta+S` scripts.
6. Two concurrent hosts; notification mid-share; app that starts playing after the share.
7.**Sample-rate / channel / passthrough behaviour on real sinks, and CPU cost** (restored).
⚠️ **Mid-share taint-root arrival — v3.4 §6.1.4 names an unreachable case, and so did my first
replacement.** v3.4 says "AEC-load-mid-share is the case to test": unreachable, because there is
exactly one `echo_cancel::enable` site at session join (`core/mod.rs:1850`), the guard moves
into `ActiveSession` at `:2729`, and `StartScreenShare` rejects `active_session == None` at
`:3397-3404` ("Join a call before sharing your screen") — so the AEC always predates the share.
My proposed replacement, "a peer joining creates their playback node," is **also wrong**:
peerspeak starts **one mixed playback stream** at session construction (the sole core
`start_playback`, `core/mod.rs:1900`), and `PeerJoined` (`:2396-2409`) only admits and connects
the sender. No per-peer node is ever created.
The reachable newly-created mid-share taint roots are: **a notification sound played mid-share**
(`notify.rs:265-272`), and **starting to view another share mid-share**, which spawns a tagged
mpv/VLC (`screenshare/mod.rs:768-775`). Those are the transition-window field tests. Owned-AEC
mid-share load stays a **synthetic** test until a second `enable` site or hot reload arms it.
---
## 8. Open questions — final status
| # | Question | Status |
| --- | --- | --- |
| O1 | Is 0c a true blocker? | **CLOSED — yes, no demotion path.** The *echo* argument for demoting it is sound and irrelevant: D6 settled it. Balloon ⇒ round 8. |
| O2 | Ship the dry-run mode? | **CLOSED — keep**, env-gated, stable reason codes + epoch + serial, stderr or defined JSON event so `--output json` stays clean. |
| O3 | Naming | **SPLIT.** The `peerspeak.owned` wire literal is pinned in plan §3 now (a contract, not product wording). Public mode/picker wording remains the **user's call**, blocking at the start of Phase 7 only. |
| O4 | Sticky state: engine or observer? | **CLOSED — pure engine.** Observer supplies lifetime-bearing membership/removal facts; the engine decides. |
| O5 | Is full recompute really fine? | **CLOSED — measure it in Phase 5**: duration distribution, maximum, and queueing, not a recompute count. |
| O6 | Can the default-monitor fallback be made structurally impossible? | **CLOSED — yes, but it took two closures, not one.** Source path *and* sink-input path, both in 0d. Placement 0d rather than Phase 6 was my divergence; Codex agreed in round 2 with the added constraint `0c → 0d`. |
| O7 | Does over-exclusion need its own gate? | **CLOSED — subsumed.** Codex correctly narrowed my premise: an exclude-everything build already fails the eligible controls in six Phase 5 rows *provided they are asserted*. Fix is the exact-partition requirement (plan §5.1) plus link-matrix row 9's positive capture assertion. |
| O8 | Runtime assertion for "no source switch on failure"? | **CLOSED — 0d's enum is insufficient.** It stops a `DesktopExcluding` value containing `DefaultMonitor`, not a failure handler swapping the whole plan for `LegacyDesktop`. Release-mode integration test per failure transition (link-matrix row 8); `debug_assert!` in addition, but it is not the gate. |
---
## 9. Risk register
| Risk | Where it bites | Mitigation |
| --- | --- | --- |
| Engine correct, **source** wrong | full echo, engine bypassed | 0d path 1 — typed capture plan |
| Engine correct, **sink inputs** poisoned | full echo, no source switch anywhere | 0d path 2 — bare sink type + conflict rejection + graph assertion |
| Taint engine subtly wrong about real PipeWire | Phase 6 leaks the call into the share | Phase 5 exact-partition gate with negative controls |
| Unsafe link exists briefly, then is cleaned up | a real leak that "eventual cleanup" tests score as a pass | link-matrix row 1: zero unsafe `create_link` calls |
| Link manager built before the engine is validated | same, discovered in front of a viewer | strict DAG; plan §0's stated temptation |
| Tag literal mismatch across repos | v3.4 §5.1 silently does nothing, *quietly* | literal pinned in plan §3; cross-repo black-box test; consumption gated in Phase 5 rows 4–6 |
| Cross-repo skew | share hard-fails on spawn | pixelpass-first; a golden test in **each** direction; capability bound to resolved path |
| Fail-closed with no explanation | user switches back to unsafe whole-desktop audio | causal status-delivery test, Phase 6 → Phase 8 |
| Over-exclusion ships as "working" | mode captures silence, all gates pass | exact partitions (plan §5.1) + link-matrix row 9 — **🟢 FIRED 2026-07-25 and worked**: the build *was* the exclude-everything degenerate case, and the empty eligible half is what exposed it |
| **A property the engine reads is silently absent at the observation boundary** | engine correct, context permanently `None`; fails in *both* directions at once (F1: no taint root ⇒ echo; F2: no owner key ⇒ exclude everything) | **v3.5 §6.7 — never read node/device props off a registry global.** Phase 3r's live prop-recovery gate asserts each one arrives. General form: `pw-dump` is a **bound** view; the registry is not, and the difference is silent |
| Wrong pipewire-pulse PID | mass over-exclusion from a correct engine on a wrong context | Phase 3 PID-derivation matrix |
| 0c balloons | prerequisites eat the schedule | reopen D6 as round 8 — no silent waiver |
| "It works on my box" | the only box is this box | two-machine field test is the ship gate |
# Pinned by peerspeak docs/screenshare-audio-exclusion-impl-plan.md §3 and
# docs/screenshare-audio-exclusion-plan.md §5.1 (v3.5). Changing a value here
# is a cross-repo breaking change: both repos must land in the same session,
# and the phase 5 matrix must be re-run.
#
# Two carriers, matched as a UNION — a node is peerspeak-owned if EITHER
# matches. Round 8 added the second because a property is invisible to the
# PipeWire registry `global` event and readable only via a node bind, so the
# primary taint root must not rest on one observation mechanism alone.
# Carrier 1 — a node property, matched EXACTLY: `prop_value` below is the
# ONLY spelling the consumer reads as owned. A producer emitting "true", "yes"
# or "" is NOT owned on this carrier, and only carrier 2 would still catch it.
#
# ⚠️ This wording is load-bearing and it CHANGED in round 10. The consumer
# used to accept any value other than "false"/"0", on the theory that leniency
# over-excludes and is therefore safe. It is not: leniency buys false-positive
# exclusion, and it let any process suppress a rival application's audio from
# the share with a property it did not even have to spell right. Fail-closed
# on this feature is about ANCESTRY — an unresolvable graph is not eligible —
# not about parsing.
prop_key=peerspeak.owned
prop_value=1
# Carrier 2 — a `node.name` prefix, announced by the registry without a bind.
# `node.description` is deliberately NOT touched, so mixers still show "mpv".
# Only the prefix is matched; the rest of the name is for diagnostics.
node_name_prefix=peerspeak_owned_
node_name_format=peerspeak_owned_<role>_<pid>
node_name_example=peerspeak_owned_mpv_31284
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
molluskandClaude Opus 5