From e8a4f54c07159b205b456f96344c9602212f148f Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 9 Aug 2026 10:39:41 +0200 Subject: [PATCH] fix(pf-vdisplay): the capability-hint test asserted the environment, not the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `silent_without_capabilities` called the real `capability_denial_hint()` and asserted it returns "", on the strength of a doc comment that read "The test process has no capabilities." That is true on a dev box and false in CI, where the runner container is root with a full permitted set. main went red on 0f79587d with: left: " — NOTE: this process carries capabilities (CapPrm=0x000001ffffffffff) …" right: "" Nothing was wrong: the hint fired correctly, on a process that really did hold every capability. The test was reading the ambient environment and calling it a property of the code. `permitted_caps_from_status` had already been split out for exactly this reason — "so that shape is testable without a capability-carrying process to point at" — but only the PARSE half. The message half still went to /proc/self/status. This finishes the split: `capability_denial_hint_for(Option)` holds the formatting and takes the mask, `capability_denial_hint()` reads /proc and delegates. Both keep their callers, so neither is dead code. Also adds `names_the_mask_and_the_repair_when_capped`. Without it the silent case passes just as well against a function that returns "" unconditionally — which is the failure mode this repo has been bitten by before, and the reason every decode fix carries a counterfactual. No behaviour change: the three error paths call the same function and get the same string. ⚠ Verification is CI. `kwin.rs` is `#[cfg(target_os = "linux")]`, so it does not compile on the macOS host this was written from; `cargo fmt --all --check` is clean and a Linux container check was attempted but the stock rust image has no cmake for audiopus_sys, so it never reached the test. ci.yml going green on main is the proof — and unlike the case it replaces, this test now fails or passes for reasons that have nothing to do with the machine running it. Does not touch the v0.26.0 tag: ci.yml runs on `push: branches: [main]` and `pull_request` only, and no tag leg runs cargo test. --- crates/pf-vdisplay/src/vdisplay/linux/kwin.rs | 37 ++++++++++++++++++- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs b/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs index bf7c3d99..9816bbc0 100644 --- a/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs +++ b/crates/pf-vdisplay/src/vdisplay/linux/kwin.rs @@ -1093,6 +1093,19 @@ fn capability_denial_hint() -> String { let permitted = std::fs::read_to_string("/proc/self/status") .ok() .and_then(|status| permitted_caps_from_status(&status)); + capability_denial_hint_for(permitted) +} + +/// The message half of [`capability_denial_hint`], split from the `/proc/self/status` read so it is +/// testable against a *given* mask instead of whatever the test process happens to hold. +/// +/// That distinction is not academic: the first version of this asserted the empty case by calling +/// the real thing and trusting the test process to be uncapped. That holds on a dev box and is +/// false in CI, where the runner container is root with a full permitted set +/// (`CapPrm=0x000001ffffffffff`) — so the hint fired, correctly, and the test failed on a machine +/// where nothing was wrong. A check whose answer depends on the ambient environment tests the +/// environment, not the code. +fn capability_denial_hint_for(permitted: Option) -> String { match permitted { Some(caps) if caps != 0 => format!( " — NOTE: this process carries capabilities (CapPrm={caps:#018x}), which is enough on \ @@ -1135,10 +1148,30 @@ mod capability_hint_tests { /// A capability-free host must not append the hint — the message it decorates is also printed /// on genuinely missing `.desktop` files, and a spurious "you have capabilities" line would - /// send the reader chasing a setcap that was never there. The test process has no capabilities. + /// send the reader chasing a setcap that was never there. + /// + /// Driven off an explicit mask rather than the test process's own: see + /// [`capability_denial_hint_for`] for why calling the real reader here fails in CI. #[test] fn silent_without_capabilities() { - assert_eq!(capability_denial_hint(), ""); + assert_eq!( + capability_denial_hint_for(permitted_caps_from_status(CLEAN)), + "" + ); + // Absent or unparseable field: also silent, never a panic and never a spurious hint. + assert_eq!(capability_denial_hint_for(None), ""); + } + + /// ...and the case that matters actually speaks, naming the mask and the repair. Without this + /// the test above passes just as well against a function that returns `""` unconditionally. + #[test] + fn names_the_mask_and_the_repair_when_capped() { + let hint = capability_denial_hint_for(permitted_caps_from_status(CAPPED)); + assert!( + hint.contains("0x0000000000800000"), + "names the mask: {hint}" + ); + assert!(hint.contains("setcap -r"), "names the repair: {hint}"); } }