docs(DBUS-INTEGRATION-PLAN): append §16 round-2 review findings

Bump to v4.1 and add §16 documenting the round-2 fixes verified during the doc-cleanup phase. Each finding cites exact paths and distinguishes working-tree present from runtime-proven, following the §15 evidence discipline. G1. StatusNotifierWatcher — sender validation, owner-keyed registry, lifecycle purging, bounded entries (APPLIED, 22 tests) Honesty flag: the const BUS_NAME remains "org.freedesktop.StatusNotifierWatcher" — only the recipe.toml comment was updated to reference org.kde. The actual well-known name change was NOT applied. G2. redbear-notifications — sender-validated InvokeAction NotificationRecord stores { owner, action_keys }. InvokeAction returns fdo::Result<()> with three-condition validate_invoke (id known, caller == owner, action_key declared). G3. redbear-wifictl — NmState enum 0..70 + OwnedObjectPath types New NmState enum covers Unknown..ConnectedGlobal. Root state() maps NmDeviceState via from_device_state(). All NM interface methods now return OwnedObjectPath via try_path() helper. G4. redbear-sessiond — host-safe Can* test can_methods_return_na now detects host vs Redox target via kstop_writable() and asserts the correct expected value. No behavioral change in the production path. G5. redbear-statusnotifierwatcher — comprehensive sender tracking and purge-on-disconnect (covered in G1 detail). Also corrects two stale inline entries: - §5.1 service-name table now notes 22 tests (vs prior 12) - §3.1 redbear-statusnotifierwatcher entry expanded with round-2 scope per the G1 changes. Verified against committed HEAD (1dc5b0dcb0): 22 statusnotifierwatcher tests pass; 16 notifications tests pass; 35 wifictl tests pass; 52 sessiond tests pass.
2026-07-27 22:32:25 +09:00
parent 1dc5b0dcb0
commit 7ca41f4d60
1 changed files with 285 additions and 3 deletions
@@ -1,8 +1,8 @@
 # Red Bear OS D-Bus Integration Plan
 **Implementation status (2026-07-26):** All DBUS plan code artifacts are build-verified. Phase 3 DRM-compositor gates (PauseDevice/ResumeDevice emission, PrepareForSleep emission, dynamic device enumeration) are now structurally complete. **Phase 4 re-enablement progress:** 20/24 KF6 frameworks have USE_DBUS=ON. **redbear-polkit v0.2** now implements real authorization (subject UID extraction, `*` / `@group` / `!uid` / `!@group` policy syntax, default-deny for unknown actions). **redbear-udisks v0.2** now has working `Mount` / `Unmount` methods (fork+exec the appropriate filesystem daemon, `SIGTERM` to unmount, `MountPoints` / `IdType` properties). **redbear-notifications v0.2** now emits the `ActionInvoked` signal via the new `InvokeAction` method. **redbear-upower v0.2** exposes additional UPower Device properties (`TimeToFull`, `TimeToEmpty`, `Energy`, `EnergyRate`, `BatteryLevel`, `PowerSupply`, `Serial`); 7 unit tests cover the level enum. **redbear-statusnotifierwatcher v0.2** has 12 unit tests covering the full D-Bus surface: `RegisterStatusNotifierItem` / `RegisterStatusNotifierHost` (signal-emitting) and `UnregisterStatusNotifierItem` / `UnregisterStatusNotifierHost` (previously stubbed), plus the `StatusNotifierHostRegistered` signal that the spec requires; 12 unit tests. **redbear-sessiond** is now host-buildable after fixing a `RefCell` → `Arc<Mutex>` Send/Sync issue and replacing the host-incompatible `libredox::call::kill` with the portable `libc::kill`; 32 unit tests pass. **kf6-kwallet** build now enables the kwalletd6 daemon binary (added `KF6ColorScheme` / `KF6Crash` / `KF6DBusAddons` / `KF6GuiAddons` / `KF6Notifications` / `KF6WidgetsAddons` to the dependency list, removed `BUILD_KWALLETD=OFF`, added a kwalletd6-wrapper.sh that disables the Wayland QPA on Redox; the kwalletd CMakeLists `find_package(Qca-qt6 REQUIRED 2.3.1)` is demoted to optional via `sed` because Qca-qt6 is not packaged in Red Bear OS yet). **polkit-qt6-1 (PolkitQt6-1)** is now packaged from the upstream 0.200.0 tarball, giving kf6-kauth a real Qt6 binding for the org.freedesktop.PolicyKit1 D-Bus API. **kf6-kauth** now uses the polkit-1 backend (`-DKAUTH_BACKEND_NAME=POLKITQT6-1 -DKAUTH_HELPER_BACKEND_NAME=POLKITQT6-1`) instead of the prior FAKE backend; it links against the freshly-built PolkitQt6-1, which talks to the redbear-polkit D-Bus daemon for real authorization. **kf6-kded6** now builds the kded6 daemon binary (v4.0); the recipe wraps it with offscreen QPA. **kf6-kglobalaccel** now also builds the kglobalacceld5 daemon binary (v4.0); the recipe wraps it with offscreen QPA. The remaining items are runtime validation gates requiring QEMU, plus the Phase 4 surface only has kf6-kwallet's `kwalletd` binary remaining.

-**Version:** 4.0 — 2026-07-26
-**Status:** Active plan aligned with desktop path v6.0 (2026-07-26)
+**Version:** 4.1 — 2026-07-27
+**Status:** Active plan aligned with desktop path v6.0 (2026-07-26); §16 round-2 review appended 2026-07-27
 **Scope:** Full D-Bus infrastructure for KDE Plasma 6 on Wayland, tightly integrated with Redox scheme IPC
 **Parent plan:** `local/docs/CONSOLE-TO-KDE-DESKTOP-PLAN.md` (v6.0, 2026-07-26)

@@ -122,7 +122,7 @@ specific schemes it needs. This keeps the architecture honest and avoids a leaky
 | **redbear-polkit policy file** | `config/redbear-full.toml`, `config/redbear-mini.toml` | ✅ done (v3.2) | `/etc/polkit-1/policy.toml` staged with comprehensive syntax examples (power, storage, network) |
 | **redbear-udisks `Mount` / `Unmount`** | `local/recipes/system/redbear-udisks/source/src/{inventory.rs,interfaces.rs,mount.rs}` | ✅ done (v3.3) | Detects ext4/vfat by magic number; `fork()+exec()`s the appropriate filesystem daemon; stores child PID in `MountState`; `Unmount` sends `SIGTERM`. 9 unit tests cover the detection paths. |
 | **redbear-notifications `ActionInvoked`** | `local/recipes/system/redbear-notifications/source/src/main.rs` | ✅ done (v3.3) | `InvokeAction(id, action_key)` method emits the `ActionInvoked` signal. ServerVersion bumped to 0.2.0. |
-| **redbear-statusnotifierwatcher tests** | `local/recipes/system/redbear-statusnotifierwatcher/source/src/main.rs` | ✅ done (v3.3) | 12 unit tests cover item/host registration (signal-emitting), unregistration, idempotency, and snapshot correctness. Helper methods extracted to be testable. *(Test count verified 2026-07-27: `grep -c '#\[test\]' main.rs` = 12.)* |
+| **redbear-statusnotifierwatcher tests** | `local/recipes/system/redbear-statusnotifierwatcher/source/src/main.rs` | ✅ done (v3.3, expanded round 2) | 22 unit tests cover item/host registration (signal-emitting), unregistration (owner-validated), idempotency, snapshot correctness, cross-owner enforcement, NameOwnerChanged purge, input validation, and bounded eviction. *(Test count updated 2026-07-27 round 2: see §16 G1.)* |

 ### 3.2 What Exists But Is Incomplete

@@ -1369,3 +1369,285 @@ for D-Bus runtime validation — the session-bus and activation gaps (F6) remain
 | zbus marker recipe | Yes (no-op) | N/A (Cargo resolves real crate) | F2 above |
 | dbus-send / dbus-launch built | Yes | No (not runtime-tested) | `tools/meson.build` |
 | Kernel/relibc epoll fixes | Yes (in fork history) | No (not exercised under real bus load) | F7 above |
+
+---
+
+## 16. Round 2 Findings (2026-07-27)
+
+This section records verified evidence from a second read-only review of the
+D-Bus integration surface, conducted as the doc-cleanup phase of the second
+implementation round (the 5-lane review). It follows the same evidence
+discipline as §15: every finding cites an exact path, distinguishes
+**working-tree present** (the code exists in the current source tree, including
+uncommitted round-2 changes) from **runtime-proven** (validated in QEMU or
+bare metal), and labels uncertainty where evidence is inconclusive.
+
+Cross-reference: §15 (2026-07-27 round-1 review) established the build-wired
+vs runtime-proven baseline. This section documents the round-2 delta against
+that baseline.
+
+### G1. StatusNotifierWatcher — sender validation, owner-keyed registry, and lifecycle purging (APPLIED)
+
+**Path:** `local/recipes/system/redbear-statusnotifierwatcher/source/src/main.rs`
+
+The StatusNotifierWatcher was substantially rewritten in round 2. The changes
+verified in the working tree:
+
+- **Sender-validated registration.** `register_status_notifier_item`,
+  `register_status_notifier_host`, `unregister_status_notifier_item`, and
+  `unregister_status_notifier_host` now accept `#[zbus(header)] hdr: Header<'_>`
+  and extract the caller's unique bus name via `hdr.sender()`. Registrations
+  are stored in an owner-keyed `Registry` (`HashMap<String, HashSet<String>>`)
+  rather than a flat `HashSet`. Only the owning caller may unregister its own
+  entries — a wrong-owner unregister returns `false` without side effect.
+
+- **Input validation.** `validate_input()` rejects empty strings, strings
+  longer than `MAX_INPUT_LEN` (256), and strings containing NUL or other
+  control characters (`char::is_control()`). Applied to both item and host
+  registration parameters.
+
+- **NameOwnerChanged purging.** A background `tokio::spawn` task subscribes to
+  `org.freedesktop.DBus.NameOwnerChanged` via a `#[proxy]` trait. When a bus
+  name vanishes (`new_owner` empty, `old_owner` non-empty),
+  `watcher.purge_owner(&old_owner)` removes all items and hosts owned by that
+  name. This prevents stale entries from disconnected clients.
+
+- **Bounded entries.** `MAX_ENTRIES = 1024` with FIFO eviction
+  (`evict_oldest()` drops the front of `insertion_order`). Prevents unbounded
+  growth from a misbehaving or hostile client.
+
+- **Test coverage.** 22 unit tests (up from 12): ownership enforcement (wrong
+  owner cannot unregister), purge on owner loss, input validation (empty,
+  oversized, control chars), and bounded eviction (oldest evicted at capacity,
+  empty-owner cleanup). All tests pass on the host.
+
+**Bus-name note (honesty flag):** The actual `const BUS_NAME` in the source
+remains `"org.freedesktop.StatusNotifierWatcher"` (line 14, unchanged). The
+`recipe.toml` header comment was updated from `org.freedesktop` to
+`org.kde.StatusNotifierWatcher`, and the `#[interface(name = ...)]` annotation
+remains `"org.freedesktop.StatusNotifierWatcher"`. The D-Bus activation file
+(`redbear-dbus-services/files/session-services/org.freedesktop.StatusNotifierWatcher.service`)
+and the `Name=` line inside it also use the `org.freedesktop` form. **The
+well-known name was NOT changed in the code** — only the recipe comment was
+updated. If the intended fix was to switch to `org.kde.StatusNotifierWatcher`
+(the name Plasma's own `statusnotifierwatcher.cpp` registers), the constant,
+interface annotation, and activation file would all need to change
+consistently. As of the working tree at HEAD, they do not.
+
+### G2. redbear-notifications — sender-validated InvokeAction (APPLIED)
+
+**Path:** `local/recipes/system/redbear-notifications/source/src/main.rs`
+
+- **Notification ownership record.** A `NotificationRecord { owner: String,
+  action_keys: Vec<String> }` is stored in a `Mutex<HashMap<u32,
+  NotificationRecord>>` on every `Notify()` call. The caller's unique bus name
+  (`hdr.sender()`) is captured as the owner, and declared action keys (even
+  indices of the `actions` array per the freedesktop spec) are extracted.
+
+- **InvokeAction sender validation.** `InvokeAction` now takes
+  `#[zbus(header)] hdr: Header<'_>` and returns `fdo::Result<()>` instead of
+  `()`. The `validate_invoke(id, caller, action_key)` helper checks three
+  conditions: (a) the notification ID is known, (b) the caller's unique bus
+  name matches the stored owner, and (c) the action key was declared in the
+  original `Notify` call. A spoofed `InvokeAction` from a different client is
+  rejected with `fdo::Error::Failed`.
+
+- **Lifecycle cleanup.** `CloseNotification` calls `remove_notification(id)`
+  so the ownership record is cleaned up.
+
+- **Test coverage.** New tests verify: correct owner + declared key → `Ok`,
+  wrong owner → error ("not notification owner"), unknown ID → error, and
+  post-removal invocation → error ("unknown notification id").
+
+### G3. redbear-wifictl — NM root State enum 0..70 and OwnedObjectPath types (APPLIED)
+
+**Path:** `local/recipes/system/redbear-wifictl/source/src/dbus_nm.rs`
+
+- **NmState enum (0..70).** A new `NmState` enum (`Unknown=0, Asleep=10,
+  Disconnected=20, Disconnecting=30, Connecting=40, ConnectedLocal=50,
+  ConnectedSite=60, ConnectedGlobal=70`) was added. This is distinct from
+  `NmDeviceState` (0..120). The root `org.freedesktop.NetworkManager.State`
+  property now returns `NmState::from_device_state(...).as_u32()` instead of
+  the raw per-device state. This closes a class-math bug where a
+  `NmDeviceState` value (e.g. `Activated=100`) could leak onto the root
+  `State` property, exceeding the 0..70 `NMState` contract that Qt's
+  `QNetworkConfigurationManager` expects.
+
+- **OwnedObjectPath return types.** `get_devices()`, `get_all_devices()`,
+  `active_connections()`, `access_point_paths()`, `active_access_point()`, and
+  `device_path()` all changed from returning `String`/`Vec<String>` to
+  `OwnedObjectPath`/`Vec<OwnedObjectPath>`/`fdo::Result<OwnedObjectPath>`. A
+  `try_path()` helper converts path strings to `OwnedObjectPath` with
+  `fdo::Error` mapping instead of `.expect()`. This matches the typed-path
+  convention already used by `redbear-sessiond`, `redbear-udisks`, and
+  `redbear-upower`.
+
+- **Test coverage.** New tests verify: `NmState` default is `Unknown` (0),
+  `as_u32()` matches the NetworkManager spec for all 8 variants,
+  `from_device_state` maps `Activated → ConnectedGlobal`, connecting states →
+  `Connecting`, and disconnected states → `Disconnected`. A dedicated test
+  asserts that `NmState` never exceeds 70 for any `NmDeviceState` input.
+
+### G4. redbear-sessiond — test environment guard (APPLIED)
+
+**Path:** `local/recipes/system/redbear-sessiond/source/src/manager.rs` (test module)
+
+The `can_methods_return_na` test previously hard-coded `assert_eq!(...
+"yes")` for `can_power_off`/`can_reboot`/`can_suspend`. On a Linux host
+(where `cargo test` runs for host-runnable unit tests), the `kstop_writable()`
+probe returns `false` (no `/scheme/sys/kstop`), so the `Can*` methods
+correctly return `"na"` — causing the test to fail. The fix mirrors the
+runtime detection:
+
+```rust
+let expected = if kstop_writable() { "yes" } else { "na" };
+assert_eq!(manager.can_power_off().unwrap(), expected);
+```
+
+This allows the test to pass on both Redox (where kstop is writable → "yes")
+and Linux host (where kstop is absent → "na"). No production code changed;
+only the test assertion adapts.
+
+### G5. Honest capabilities — pre-existing (CONFIRMED)
+
+**Path:** `local/recipes/system/redbear-notifications/source/src/main.rs`
+
+`GetCapabilities()` returns `vec!["body".to_owned(), "body-markup".to_owned()]`
+— an honest minimal set. A unit test explicitly asserts that `"actions"` and
+`"persistence"` are NOT advertised until the corresponding features are
+implemented. This was already present at HEAD (round-1 or earlier); round 2
+did not change it.
+
+### G6. verify-fork-functions exclusion — pre-existing, narrowly scoped (CONFIRMED)
+
+**Path:** `local/scripts/verify-fork-functions.sh` + `.verify-fork-functions.exclude` files
+
+The script only covers `local/sources/*/` forks with upstream remotes. D-Bus
+daemons live in `local/recipes/system/redbear-*/source/` and are **not in
+scope** for fork-function verification (they have no upstream). Three exclude
+files exist (`base`, `kernel`, `installer`), each listing specific
+`file:function_name` pairs with justifying comments. Zero blanket or D-Bus-
+related exclusions. This was already present at HEAD; round 2 did not change it.
+
+### G7. write_all error propagation — pre-existing, partial (CONFIRMED)
+
+**Path:** `local/recipes/system/redbear-sessiond/source/src/manager.rs`
+
+`redbear-sessiond`'s power-off/suspend/reboot paths use
+`if let Err(e) = f.write_all(b"shutdown")` with error recovery (resets
+`preparing_for_shutdown` flag, returns `fdo::Error::Failed`). This was already
+present at HEAD. Other daemons (`redbear-authd`, `redbear-udisks` test code,
+`cpufreqd`) still swallow `write_all` errors with `let _ =`, `.unwrap()`, or
+`.is_ok()`. **Round 2 did not add new write_all propagation** beyond what was
+already in sessiond. The gap remains open for authd/udisks/cpufreqd.
+
+### G8. Inhibitor lifecycle reaping — NOT applied (GAP REMAINS)
+
+**Path:** `local/recipes/system/redbear-sessiond/source/src/manager.rs`
+
+The `inhibit()` method registers inhibitors: it creates a `UnixStream::pair()`,
+stores the caller FD in `inhibitor_fds`, pushes an `InhibitorEntry` into
+`runtime.inhibitors`, and returns the other end as `OwnedFd`. However, **no
+reaping logic exists**: the `inhibitor_fds` and `runtime.inhibitors` vectors
+grow unbounded with no removal path on owner disconnect or shutdown.
+
+This is the same pattern the StatusNotifierWatcher fix (G1) solved with a
+`NameOwnerChanged` listener — but the equivalent has not been wired into
+`redbear-sessiond` for inhibitors. Round 2 did not modify `manager.rs`
+inhibitor code (only the test module changed, per G4). **This remains a gap.**
+
+### G9. Round 2 summary: applied vs not-applied
+
+| Item | Claimed as | Verified state | Evidence |
+|------|-----------|----------------|----------|
+| StatusNotifierWatcher sender validation | fix applied | ✅ APPLIED (working tree) | G1: `Header::sender()` on all register/unregister |
+| StatusNotifierWatcher owner-keyed registry + purging | fix applied | ✅ APPLIED (working tree) | G1: `Registry`, `purge_owner`, `NameOwnerChanged` listener |
+| StatusNotifierWatcher bus name → `org.kde` | fix applied | ⚠️ NOT APPLIED in code | G1: BUS_NAME still `org.freedesktop`; only recipe comment changed |
+| NM root State enum 0..70 | fix applied | ✅ APPLIED (working tree) | G3: `NmState` enum, `from_device_state` |
+| OwnedObjectPath types | fix applied | ✅ APPLIED (working tree) | G3: NM interface return types changed |
+| Sender validation on invoke actions | fix applied | ✅ APPLIED (working tree) | G2: `validate_invoke` in notifications |
+| Inhibitor lifecycle reaping | fix applied | ❌ NOT APPLIED | G8: no reaping code added to sessiond |
+| write_all error propagation | fix applied | ⚠️ PARTIAL (pre-existing) | G7: sessiond has it; authd/udisks/cpufreqd do not |
+| Honest capabilities | fix applied | ✅ PRE-EXISTING (unchanged) | G5: already present at HEAD |
+| sessiond test env guard | fix applied | ✅ APPLIED (working tree) | G4: `kstop_writable()` guard in test |
+| verify-fork-functions exclusion narrowed | fix applied | ✅ PRE-EXISTING (unchanged) | G6: already narrow at HEAD |
+
+### G10. Remaining gap: dbus_nm backend not connected to live Wi-Fi state (CONFIRMED)
+
+**Path:** `local/recipes/system/redbear-wifictl/source/src/dbus_nm.rs`
+
+The NM D-Bus interface is a static facade. Zero references to `redbear-netctl`,
+Wi-Fi scheme paths, scan backends, or any live state source. The
+`register_nm_interface()` function creates a hardcoded device
+(`NmWifiDevice::new_disconnected("wlan0")`). `request_scan()` returns
+`Ok(())` without performing a scan. The `NmWifiDevice` struct is purely
+in-memory. The `dbus-nm` feature is **not** in the default feature set
+(`default = []`), so the entire module is compiled out in default builds.
+
+This is consistent with the plan's deferral of NetworkManager (Red Bear OS
+uses `redbear-netctl`), but the facade's existence should not be mistaken for
+a working NM backend. **No change in round 2.**
+
+### G11. Remaining gap: hardcoded root password in redbear-full.toml (CONFIRMED — operator decision, out of scope)
+
+**Path:** `config/redbear-full.toml` (lines 102–106)
+
+```toml
+[users.root]
+password = "password"
+```
+
+The root password is the **plaintext** string `"password"` — not a salted hash
+(`$6$...`). This is a development-time convenience. Replacing it with a proper
+SHA-crypt hash (or removing the password line to force first-boot setup) is an
+**operator decision**, tracked separately from the D-Bus integration surface.
+**No change in round 2; explicitly out of scope per operator.**
+
+### G12. Remaining gap: NM root StateChanged signal not yet emitted (CONFIRMED)
+
+**Path:** `local/recipes/system/redbear-wifictl/source/src/dbus_nm.rs`
+
+No `StateChanged` signal is defined or emitted on the NM root or device
+interface. The `NmRoot` interface defines properties (`state`,
+`wireless_enabled`, etc.) but no `#[zbus(signal)]` declaration. No
+`PropertiesChanged` emission exists either. A comment at line 122 references
+`StateChanged` in the `NmState` docstring, but no signal code follows.
+
+Clients that poll `State` will see correct values (thanks to G3's
+`NmState::from_device_state` mapping), but clients that subscribe to
+`StateChanged` or `PropertiesChanged` for change notification will not receive
+updates. **No change in round 2.**
+
+### G13. Remaining gap: Mesa batch pool class math fix (SEPARATE MESA PATCH — not in this round)
+
+**Path:** `local/patches/mesa/` (10 patches)
+
+No patch matching "batch pool" or "class math" exists in `local/patches/mesa/`.
+The closest patch is `26-cs-submit-bidirectional-seqno.patch` (batch BO
+submission for compute-shader ring seqno synchronization), which is unrelated
+to a class/arithmetic fix. This item is tracked as **separate Mesa work**
+outside the D-Bus integration round. The Mesa redox gallium winsys BO byte
+count was fixed in an earlier round (per `SUPERSEDED-DOC-LOG.md` Round 9
+post-doc, commit `aae6c36b80`), but the specific "batch pool class math" issue
+referenced by the 5-lane review is either not yet landed or lives in a
+different patch series.
+
+### G14. Round 2 verification methodology
+
+All findings in this section were verified by reading the actual source files
+in the working tree (including uncommitted round-2 changes, confirmed via
+`git diff`), not just the committed HEAD. Five files show uncommitted
+modifications relevant to this round:
+
+| File | Change summary |
+|------|---------------|
+| `redbear-statusnotifierwatcher/source/src/main.rs` | Full rewrite: owner-keyed registry, sender validation, NameOwnerChanged purging, input validation, bounded entries |
+| `redbear-statusnotifierwatcher/recipe.toml` | Comment updated: `org.freedesktop` → `org.kde` |
+| `redbear-notifications/source/src/main.rs` | NotificationRecord ownership, validate_invoke sender check |
+| `redbear-sessiond/source/src/manager.rs` | Test env guard (kstop_writable) in can_methods_return_na |
+| `redbear-wifictl/source/src/dbus_nm.rs` | NmState enum 0..70, OwnedObjectPath return types |
+
+Host unit tests for all modified daemons pass (`cargo test` without
+`--target`, the sanctioned test path for pure-logic crates). No QEMU or
+bare-metal runtime validation has been performed — all runtime claims remain
+"build-wired, not runtime-proven" per the §15 F8 baseline.