eaea188e05ff8ffa755f80fdb2fd4f22ecc76073
8
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  molluskandClaude Opus 5
|
6dd9b2d25a |
repair: say "once per (pid, attribution)", because that is what it is now
Codex's non-blocking round-7 nit. The comment and test name still claimed liveness is asked once per pid, which stopped being true when one pid became two questions. No behaviour change; the wording was the last thing pointing at the old model. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
molluskandClaude Opus 5
|
58bc8e99e8 |
repair: one pid can carry two attributions, and they are two questions
Round 6, one blocking P2 — in code I wrote in the round-5 fix, not in the token design, which the review passed again. The verdict cache was keyed on pid alone. A pid can legitimately be claimed by a tokened module *and* an untagged one at the same time — a host crashes, an older build restarts and reuses the number — and those are not the same question: one is answerable directly from the token, the other only if the degradation signals allow it. Collapsing them let whichever module sorted last decide both, so under `--repair-legacy-untagged` with a degraded probe an untagged winner made safely attributable debris `Unknown`, and a tokened winner planned the untagged debris as dead (the execution recheck happened to stop the destruction, which is luck, not design). Verdicts are now cached per `(pid, Attribution)` and each fingerprint is filtered by its own, never by looking its pid up in `dead_pids`. Those three pid sets are documented as reporting-only, since a pid can now honestly appear in two of them. Mutation-verified: restoring the `dead_pids.contains(pid)` filter fails the new test, which runs both module-id orders because the bug was order-dependent, and both polarities — the second asserts that a *live* tokened owner does not lose its module because an untagged claim on the same pid looked dead. Also, the degraded-probe warning had become false (P3): it announced "refusing to unload anything" while the tokened path can now legitimately unload, which in the exact container-recovery case the token was added for would print a categorical refusal and then destroy state. It is now scoped to what it actually means — modules *without* a token will be left alone. 256 tests, clippy clean, fmt clean, field gates green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
molluskandClaude Opus 5
|
104a95a6d7 |
repair: let the token beat the namespace guesswork, and ask liveness first
Round 5. Two blocking P2s, both in the execution shell rather than the token design, plus an overstated claim of mine. **The degradation signals were defeating the token in exactly the case it exists for.** A host crashing inside a container leaves a token that matches this machine, boot and pid namespace perfectly — but the probe saw a container marker or a multi-entry `NSpid` and answered `Unknown` for everything, so token-qualified repair did nothing precisely where it had just become safe. Those signals are guesswork about whether a bare pid is meaningful, and for an attributed pid that is no longer a guess. Liveness is now asked with the module's attribution in hand: a tokened pid goes straight to `kill`, while an untagged one still has to get past the signals, because there they are the only protection left. **Liveness now runs before the fresh snapshot, not after it.** The natural order — verify the module, check liveness, unload — leaves the dangerous window open: after `kill` returns ESRCH this process can be descheduled while the planned module vanishes, a new host inherits both the pid and the module index, and its differently-nonced arguments occupy that index. Nothing re-read those arguments, so the reused index would have been unloaded. Asking liveness first means the post-liveness fingerprint check catches that replacement, leaving only the irreducible snapshot-to-unload interval. **The nonce claim was overstated and is now true.** A token was minted once per `Routing` session and reused for every subsequent reload, making it a host-session nonce rather than a per-load one. It is now minted inside `load_module`, mixing a bumped counter with the clock, so two loads by the same pid really do render different arguments — which, combined with the reordering above, is what lets a fingerprint tell a module from its replacement at the same index. ⚠️ **The first version of the attribution fix had no gate, and the mutation said so.** Swapping `of_attributed` back to `of` passed all 254 tests, because on an ordinary desktop the probe is not degraded and the two paths agree, while the planner tests use a fake liveness closure that never touches the probe at all. The new test constructs a *deliberately degraded* probe, which is the only state where the distinction is observable. Both mutants — routing a tokened pid through `of`, and re-applying the degradation gate inside `of_attributed` — now fail it. Codex's answers to the questions I raised, recorded because they close them: omitting the pid from the token loses nothing, since the canonical argument already binds the token to exactly one pid; namespace inode reuse is real but only after the old namespace is destroyed, so its host is necessarily gone and no live owner is endangered; and refusing foreign tokens even under `--repair-legacy-untagged` is the right line, because the flag speaks to missing evidence rather than wrong evidence. No finding against the two-hole template derivation. 255 tests, clippy clean, fmt clean. All four live field gates re-run green: tokened cleaned, foreign refused with and without the flag, untagged refused then cleaned on request, A/B orphan removal byte-identical elsewhere, reference gate still firing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
molluskandClaude Opus 5
|
d9ef38c1c5 |
repair: a pid is not an owner — modules carry a machine/boot/namespace token
Closes the last blocking finding: a pid is only a number, and the same number is a different process in a different pid namespace. Repair running inside a container that can reach the host's Pulse socket saw a live host's modules, asked about that pid in its own namespace, was told nothing existed, and unloaded a running host's audio. No negative signal closes that — `NSpid == 1` does not prove the initial namespace, since its leftmost value is relative to whichever procfs was mounted. So the module now carries the answer with it. Every module a host loads gets `pixelpass.owner=<version>-<machine>-<boot>-<pid_ns>-<nonce>`, and repair only asks about a pid when all three identities match its own. Anything else is reported and left alone, and its pid is never even looked up — asking is the bug, because the answer would be meaningless. **Untagged modules are refused by default.** Everything loaded before tokens existed is unattributable, so `--repair` now lists those and does nothing, with `--repair-legacy-untagged` to opt into the old pid-only heuristic after seeing the candidates. That is a deliberate loss of reach: the failure being optimised against is a false-positive destructive repair, and leaving an old orphan behind is recoverable where destroying live routing is not. A foreign token is refused even with the flag, since the flag speaks to missing evidence, not wrong evidence. The vehicle was verified on the live server before anything was built on it: all three shapes accept a property-list argument (`sink_properties`, `sink_input_properties`, `source_output_properties`), the recorded argument comes back byte-identical — so exact-form matching still holds — and the property really lands on the resulting sink, sink-input and source-output. **Audit gate passed, with the variable isolated.** The token rides on real graph objects that phases 2/3 observe, so the partition had to be re-measured. Running the same fixture with and without tokens gives an identical partition: 2 eligible (FFXIV, Chromium), 2 excluded with the same `tainted-owner-bridge` reason, and the same six-entry taint set. Everything that differs from the empty-graph baseline is the fixture's own doing — a local-monitor loopback genuinely bridges our owned sink into the real default sink — and none of it is the property's. Attributing that to the token without the untokened control would have been the mistake. A side benefit: the per-load nonce narrows the ABA window I previously documented as unclosable. Two loads by the same pid no longer render byte-identical arguments, so a fingerprint taken from one no longer matches the other. Six new tests, three mutation-verified gates: `can_judge` always true, `pid_ns` dropped from the comparison, and untagged treated as judgeable regardless of policy — each killed by its own test. ⚠️ The third "survived" on first run because my mutation script's indentation did not match and the edit silently did nothing; the re-run asserts the file actually changed. A mutation that was never applied proves the same amount as no mutation at all. Field-verified live, three fixtures for one dead pid in one run: tokened with this machine's identity is cleaned, tokened with a foreign pid namespace is left alone and reported (and the legacy flag does not override it), and untagged is refused then cleaned only when asked. The two older field fixtures were tokenised too — without that the A/B test would have failed and the reference-gate test would have passed for the wrong reason, which is a vacuous gate in the harness rather than the code. 253 tests, clippy clean, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
molluskandClaude Opus 5
|
45464b4af5 |
repair: read and unload Pulse modules through libpulse, not pactl text
Third review round found three more blocking P2s, and they shared one root cause: `pactl`'s text output cannot carry the guarantees repair was claiming. All three are closed by talking to the protocol instead. New dependency taken with the user's explicit sign-off after vetting. **Record boundaries were unprovable.** `pactl list short modules` prints a module's argument raw into a tab/newline-delimited format with no escaping. A *genuine* module whose argument contains a newline renders a first line that reads byte-exactly like one of our canonical forms, with the remainder dropped as an unparseable continuation — no forged index, so the duplicate-index check could not see it. Repair would have classified and unloaded a module it never saw in full. **Field-confirmed on the live server**, because this needed no adversary: loading a loopback whose argument is canonical-then-newline-then-`remix=false` (a real loopback option) produces exactly that listing. A tab in the same position is worse: it hid a sink reference from the gate that protects a still-referenced sink. **Index and argument could be mis-paired.** The listing carrying exact arguments (`-f json`) carries no index at all on pactl 17; the one carrying the index cannot carry the argument faithfully. Correlating them by position — which the previous commit did — is unsound whenever module names repeat: another client loading one module and unloading another between the two calls leaves counts and names aligned while every argument has shifted by one, so a foreign module inherits a canonical fingerprint. The name check cannot see it and the retry never fires. **Locality was a guess.** `PULSE_SERVER` is a fallback *list*, so `unix:/missing tcp:remote:4713` passes any "starts with unix:" test and then connects to another machine, where local pids mean nothing and a live remote host's modules look dead. A remote server can also be selected by client config with the variable unset entirely. New `repair/introspect.rs` owns one verified-local connection: `pa_module_info` gives index, name and exact argument in a single record, `pa_context_is_local()` answers locality about the connection actually established, and unloading goes back through that same connection so listing and destruction cannot disagree about which server they mean. It holds no policy beyond refusing the wrong server; every decision stays in the pure planner. ⚠️ **The field test caught a real bug that no unit test could have.** The first version did its work correctly and then aborted on the way out: Assertion '!e->dead' failed at ../pulseaudio/src/pulse/mainloop.c:207, function mainloop_io_free(). Aborting. SIGABRT, core dumped, exit 134 — a fully successful repair reporting failure to its caller. Cause: Rust drops fields in declaration order and the context's teardown frees IO events living in the mainloop, which I had declared first. **This is exactly the invariant phase 0b exists for, met again one layer down.** Fixed, and then hardened past the fix: `Drop` explicitly takes and destroys the context before the mainloop, so the ordering no longer depends on where the fields are written. Liveness keeps its `NSpid`/container checks but the claim is corrected: `NSpid > 1` means "definitely nested", while `NSpid == 1` is NOT proof of the initial namespace — its leftmost value is relative to the procfs that was mounted, so a nested namespace with its own `/proc` reports one entry legitimately. These are negative signals that fail closed, not a proof of trustworthy pids. Closing that properly needs modules to carry an owner token (machine/boot plus pid-namespace identity), which changes what pixelpass writes into the graph and how far back `--repair` can clean up: recorded as a design decision, not guessed at. libpulse-binding 2.30.1 vetted before use: MIT/Apache-2.0, 5.5M downloads, 3 new crates total, build script does nothing but probe pkg-config, no network or subprocess use anywhere in the sources, and all three historical RustSec advisories (2018-0020, 2018-0021, 2019-0038) were fixed by 2.6.0. The reasoning is recorded in Cargo.toml beside the dependency. 247 tests, clippy clean, fmt clean apart from the pre-existing taint/tests.rs:2683. The text parser's tests are gone with the parser; the liveness probe keeps its own, and the live field gates — A/B orphan removal, the reference/unrecognised fixture, and the newline fixture — all pass with exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
molluskandClaude Opus 5
|
01c582427b |
repair: one atomic listing, namespace-aware liveness, renderer as authority
Second review round. Two blocking P2s and two P3s, all applied.
**The two-listing correlation was unsound, so it is gone** (P2). Pairing the short
listing's indices with the JSON listing's exact arguments by position breaks with
repeated module names: if another client loads one module and unloads another
between the two calls, the counts and names still line up while the arguments have
shifted by one — and a *foreign* module inherits a canonical fingerprint. The name
check cannot see it and the retry never fires, because correlation "succeeded".
Observations now come from a single `pactl list short modules` invocation, so every
`(index, name, argument)` comes from one server response and cannot be
mis-assembled. The two costs of that format are handled rather than hoped away:
- A tab inside an argument is invisible here, so the row is marked
`args_complete: false`. `classify` refuses such a row outright — a truncation
could otherwise coincide with a canonical form — while the reference gate can
still see that it names a sink.
- A crafted argument containing a newline can fabricate a row, but it only does
damage if it claims a *real* module's index, which makes that index appear twice.
A duplicated index now refuses the whole run.
libpulse introspection (`pa_module_info` carries index, name and argument in one
record) remains the exact route. It is a new dependency plus a mainloop in a
one-shot CLI path, so it is recorded as the upgrade rather than taken unilaterally.
**Liveness was still converting invisible-but-alive into dead** (P2). A
`/proc/self` preflight proves nothing: inside a pid namespace — a container, a
distrobox — `self` is visible while every process in the parent namespace is not,
and `hidepid` has the same shape. Repair there can reach the host's Pulse socket,
see a live host's modules, call its pid dead and unload a running host's audio.
So the probe now asks for positive confidence instead: `NSpid` in
`/proc/self/status` reports this process's pid in every namespace it appears in, so
more than one entry means our pid numbers are not the outer namespace's and every
verdict becomes `Unknown`. A kernel that does not report `NSpid`, a container
marker, and a non-local `PULSE_SERVER` all fail closed the same way. Liveness
itself is `kill(pid, 0)` via the existing `nix` dep, where `EPERM` proves
existence; pid 0 and pids past `i32::MAX` are never asked, since `kill(0, …)`
would signal our own process group.
**The renderer is the authority, not the derived template** (P3). `classify` now
re-renders the pid it extracted and demands byte equality, so the template is only
a pre-filter. `Shape` also owns the module *name* now, and `host/audio.rs` loads
through `Shape::{module_name, render_args}` — previously the "cannot drift" claim
covered only arguments while the names were still written out at both ends. The
sentinel assertion is unconditional (`assert!`), so a future shape that repeats the
pid cannot slip through a release build.
**A test I wrongly called unclosable** (P3). I argued no non-vacuous case could
prove the module name is part of the identity, because the name determines which
argument grammar can match. That was wrong: the grammars are not disjoint —
`module-echo-cancel sink_name=pixelpass_capture_9` is byte-identical to the
canonical null-sink argument, which the suite already constructs. Same index, same
arguments, different name, and `still_matches` must say no.
252 tests (+1 net; the correlation tests were replaced by parser and probe tests),
clippy clean, fmt clean apart from the pre-existing taint/tests.rs:2683. Both live
field gates re-run against the rewritten observation path: the A/B orphan test
still removes exactly the two orphans with the module table otherwise identical,
and the reference/unrecognised fixture still leaves both modules alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
molluskandClaude Opus 5
|
9145b2a726 |
repair: exact-form matching, tri-state liveness, and a reference-gated sink
Codex's review of the repair planner returned changes-requested: no P1s, four reachable P2s plus a P3, all in the observation and execution layers rather than in the discovery fix itself. All applied. **Only the canonical forms are ours** (P2, plan.rs). `classify` recognised any loopback with one pixelpass-looking endpoint, so a third party's `module-loopback source=some_mic sink=pixelpass_capture_4242` was ours to unload once that pid died — and a `sink=` token nested inside a quoted `sink_input_properties` value could be mistaken for a top-level argument. The whole recorded argument string must now equal what pixelpass itself would have written. The matcher's templates are **generated from the loader's own renderers**, not written out beside them: hard-coding `latency_msec=20` in a matcher means a loader change silently blinds repair to every module the new build loads, which is the fail-closed-and-silent failure this project has been bitten by three times. `host/audio.rs` now loads through those same renderers, so the two cannot drift. Blindness is also reported rather than assumed impossible — `unrecognised_pixelpass_modules` finds modules that name our sinks but match no canonical form, and `--repair` says so loudly. Measured on the live server before relying on it (pactl 17.0): arguments come back byte-for-byte as passed, joined with single spaces, with `@DEFAULT_SINK@` NOT resolved. Both facts are load-bearing for exact matching and both have a test. **Ordering is not a licence either** (P2, mod.rs). The plan put loopbacks before the sink, but an unload can fail or be skipped and a loopback can appear after planning, so the executor could still destroy a sink that something was attached to. The sink unload is now gated on `sink_still_referenced` against the fresh snapshot — any other module naming that sink blocks it, ours or not, because the question is what would break, not who owns it. **Undecidable is not dead** (P2, mod.rs). `Path::exists()` maps permission errors, a missing `/proc` and a foreign pid namespace all to `false`, which here read as "dead, go ahead and unload". Liveness is now `Alive | Dead | Unknown` via `try_exists()` behind a `/proc/self/stat` preflight, and `Unknown` is treated exactly like alive and reported separately. **The short listing cannot carry a fingerprint** (P2, mod.rs). Its arguments are tab-delimited text that a module argument may itself contain, and a continuation line beginning with a digit could fabricate a row. Observations now come from two listings: the short one for the module index, and `pactl -f json list modules` for the exact argument. Codex proposed JSON alone; on pactl 17 its records carry `"index": null`, so it cannot be used on its own — verified, hence the correlation. The pairing is positional and *checked* (same count, same name at every position, else refuse), which is also what makes a fabricated row harmless instead of exploitable: it has no JSON counterpart, so the sequences misalign. Normalisation is gone (P3). Within one invocation every snapshot comes from the same server, so re-rendering does not happen, and normalising only made genuinely different arguments compare equal. The residual ABA window — planned module vanishes, byte-identical one takes its index — cannot be closed through an unload API whose only argument is an index; that is now said plainly in the fingerprint's own doc comment rather than implied away. Five vacuity gaps Codex found in the tests, closed: a raw-pactl-output-to-plan test (the planner suite survived a parser that dropped every argument), the liveness-once test now uses two pids with per-pid counters, non-canonical and nested-quoted arguments have their own cases, and the reference gate has one. Field-verified on the live graph, both new rules: the A/B orphan test still removes exactly the two orphans with the module table otherwise byte-identical, and a fixture of a dead pid's legacy sink plus a non-canonical loopback naming it leaves both alone and reports why. 251 tests (+9), clippy clean, fmt clean apart from the pre-existing taint/tests.rs:2683. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
molluskandClaude Opus 5
|
919d5bdef2 |
repair: find the orphans a connection-owned sink leaves behind
Phase 0c's first landed piece. `--repair` does not get smaller when the capture sink becomes connection-owned — it goes blind, which is the opposite of the assumption I started from and the finding that reordered this phase. Today repair learns a dead host's pid ONLY from `module-null-sink sink_name=pixelpass_capture_<pid>`, and matches loopbacks only if their pid is already in that set. After 0c the sink is a native node that removes itself with its owning connection, so a hard-killed host leaves two Pulse loopbacks and *no null-sink module to learn the pid from*. The set stays empty, nothing matches, and the orphans are invisible forever. Candidate pids are now derived independently from all three module shapes. Split into a pure planner (`repair/plan.rs`) and an I/O shell, because every interesting property here is a decision — which pid is dead, which module is whose, in what order to unload — and none of them need PipeWire to exercise. The rule that is new, and that the old code could not express: **a plan is not a licence.** Pulse module indices are reused verbatim, so an id planned against one module can name a different live module by the time the unload runs; a pid recheck alone does not catch that. Every action now carries a full fingerprint (id, module name, normalized args, derived pid, shape) which is re-verified against a FRESH snapshot immediately before each unload, with liveness rechecked last, closest to the destruction. Anything that does not match exactly is skipped and said out loud — never unloaded on the strength of a stale plan. Ordering is carried by `Shape`'s declaration order rather than by two separate passes, so loopbacks unload before the sink they reference by construction. Liveness is asked once per pid, not once per module: a flapping answer must not be able to half-repair a host, which is the one outcome worse than doing nothing. Field-tested against a real post-0c orphan, not just mocked: a connection-owned sink created via `pw-cli create-node adapter` (module-null-sink count: 0, so the old discovery provably could not see it), both loopback shapes loaded against it, then SIGKILL of the owning connection. The sink vanished on its own, both loopbacks survived, `--repair` removed exactly those two, and the module table was otherwise byte-identical before and after — the collateral-damage half of the two-host safety property. Mutation-verified, five mutants, each killed by its own gate: null-sink-only discovery (the 0c blindness itself), id-ordered unloads instead of shape-ordered, a fingerprint that compares only the index (exactly one), no liveness filter, and un-normalized args (exactly one). Still owed: the live two-host gate (two real hosts, kill one, prove the other's graph and modules are untouched) cannot run until the native sink exists, so it is deferred to 0c's combined exit gate. This lands with test + single-host field proof only, which Codex agreed is an acceptable phase dependency rather than an objection to landing repair first. 242 tests (+11), clippy clean, fmt clean apart from the known pre-existing `taint/tests.rs:2683` — pixelpass is never `cargo fmt`ed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |