Fix various small issues
This commit is contained in:
NeilClack
@@ -0,0 +1,166 @@
|
||||
# KioskConfig Review Report
|
||||
|
||||
Verification of `kioskconfig` (target: `radiantwavekiosk-dev`) against the app source in `radiantwave-rebuild/` (flake input `radiantwave-dev` → `radiantwave-cpp.git?ref=dev`).
|
||||
|
||||
Severity legend: **Critical** = blocks boot/launch · **High** = wrong behavior in production · **Medium** = wrong behavior in a variant · **Low** = hygiene.
|
||||
|
||||
---
|
||||
|
||||
## Critical
|
||||
|
||||
### 1. Kiosk user is missing the `seat` group — cage/app can't get a seat
|
||||
|
||||
**Location:** `configuration.nix:111`
|
||||
|
||||
```nix
|
||||
extraGroups = ["audio" "video" "input" "networkmanager" "wheel"]; # missing "seat"
|
||||
```
|
||||
|
||||
**Why it breaks:** NixOS `services.seatd` owns `/run/seatd.sock` as `root:seat` (`nixos/modules/services/desktops/seatd.nix`, default `group = "seat"`). wlroots uses libseat, which connects to the seatd socket. Without `seat` group membership `libseat_open_seat` fails, and there is no logind fallback: getty autologin creates a `tty` (not `graphical`) session, which the logind backend rejects. The NixOS seatd test (`nixos/tests/seatd.nix`) explicitly adds the user to `seat`.
|
||||
|
||||
**Result:** `exec cage -- radiantwave` exits immediately — the app never appears on screen.
|
||||
|
||||
- [X] **Fix:** add `"seat"` to `extraGroups`.
|
||||
|
||||
**Also affected:** `radiantwave-rebuild/nixos/vm.nix:85` has the same omission.
|
||||
|
||||
---
|
||||
|
||||
## High
|
||||
|
||||
### 2. Healthcheck can never match → 10-minute rollback loop
|
||||
|
||||
**Location:** `configuration.nix:247`
|
||||
|
||||
```nix
|
||||
if ${pkgs.procps}/bin/pgrep -x radiantwave > /dev/null; then
|
||||
```
|
||||
|
||||
**Why it breaks:** The installed `radiantwave` is a `makeWrapper` script — `nix/package.nix:132-134` renames the real binary to `.radiantwave-unwrapped`, and the wrapper ends with `exec .radiantwave-unwrapped`. The kernel sets `comm` from the exec'd file, truncated to 15 chars: `.radiantwave-unw`. `pgrep -x radiantwave` (exact match) never matches.
|
||||
|
||||
**Result:** Every autoUpgrade (`dates = "*:00/10"` → every 10 min) is followed ~90s later by `nixos-rebuild switch --rollback`, so the system oscillates forward/backward forever and upgrades never stick.
|
||||
|
||||
- [x] **Fix:** match on the actual process, e.g. `pgrep -f '/radiantwave'` or `pgrep -x .radiantwave-unw`, or use a different liveness check.
|
||||
|
||||
**Related design flaw:** after `switch`, the running app is still the *old* binary (the login session persists; `loginShellInit` doesn't re-run), so even a correct healthcheck can't validate the new build — only a reboot can.
|
||||
|
||||
### 3. `radiantwavekiosk-commercial-dev` deploys production code
|
||||
|
||||
**Location:** `flake.nix:142-148`
|
||||
|
||||
```nix
|
||||
radiantwavekiosk-commercial-dev = mkKioskSystem {
|
||||
radiantwave = radiantwave; # should be radiantwave-dev
|
||||
...
|
||||
upgradeFlakeRef = "...KioskConfig.git?ref=dev#radiantwavekiosk-commercial"; # should be ...-dev
|
||||
```
|
||||
|
||||
**Result:** a "dev" commercial kiosk runs the production binary and auto-upgrades to the production config. Same bug class as finding #5's root cause: hand-wired per-variant wiring.
|
||||
|
||||
- [X] **Fix:** `radiantwave = radiantwave-dev;` and `#radiantwavekiosk-commercial-dev`.
|
||||
|
||||
### 4. Backup + tmpfiles use the wrong data directory
|
||||
|
||||
**Locations:**
|
||||
- `configuration.nix:210` — tmpfiles: `d /home/kiosk/.local/share/radiantwave 0770 kiosk kiosk -`
|
||||
- `configuration.nix:223-224` — backup: `BACKUP_DIR`/`SOURCE_DIR` under `.local/share/radiantwave`
|
||||
- `configuration.nix:74-76` — `cleanRadiantFiles`: `rm -rf /home/kiosk/.local/share/radiantwave/audio`
|
||||
|
||||
**Why it's wrong:** the app uses `~/.local/radiantwave/` (verified: `config_manager.cpp:36`, `database_manager.cpp:36`, `logger.cpp:70`, `audio_downloader.cpp:22`, `scroller_page.cpp:766`). `~/.local/share/radiantwave` is the wrong path.
|
||||
|
||||
**Result:**
|
||||
- Backup rsyncs an empty/nonexistent dir every upgrade → pre-upgrade backup is useless.
|
||||
- `cleanRadiantFiles` is a silent no-op (audio never actually cleaned).
|
||||
- The app creates its own dirs via `create_directories`, so the wrong-path tmpfiles rule is harmless functionally — but wrong.
|
||||
|
||||
- [X] **Fix:** replace `share/radiantwave` with `radiantwave` in all three places.
|
||||
|
||||
**Caution:** clarify `cleanRadiantFiles` intent — if the path were "fixed" naively it would wipe downloaded audio on every activation.
|
||||
|
||||
---
|
||||
|
||||
## Medium
|
||||
|
||||
### 5. Android ISO installs the wrong config
|
||||
|
||||
**Locations:** `flake.nix:215-236`, `iso-config.nix:8-12`
|
||||
|
||||
`android-iso` reuses `./iso-config.nix`, which hardcodes:
|
||||
|
||||
```nix
|
||||
targetSystem = self.nixosConfigurations.radiantwavekiosk.config.system.build.toplevel;
|
||||
```
|
||||
|
||||
`nixos-install --system` installs the plain (non-rotated) `radiantwavekiosk` closure even though the ISO ships `radiantwavekiosk-android` via `isoImage.storeContents`. The rotated variant never gets installed.
|
||||
|
||||
- [ ] **Fix:** parameterize the ISO config (a TODO already exists in `commercial-iso-config.nix`) or add an android-specific ISO config.
|
||||
|
||||
---
|
||||
|
||||
## Low
|
||||
|
||||
### 6. `-d -s` launch flags are dead
|
||||
|
||||
**Locations:** `rotated_display.nix:7`, `configuration.nix:145`
|
||||
|
||||
Both invoke `radiantwave -d -s`, but `main.cpp:17` (`SDL_AppInit(void **appstate, int argc, char *argv[])`) never parses `argc/argv` — there is no getopt/argv handling anywhere in the source. The flags are silently ignored.
|
||||
|
||||
- [ ] **Fix:** implement the flags in the app's CLI parsing, or drop them from the launch commands.
|
||||
|
||||
### 7. Secrets committed
|
||||
|
||||
**Locations:** `configuration.nix:64`, `configuration.nix:113`
|
||||
|
||||
- Gitea `nix.settings.access-tokens` token in plaintext.
|
||||
- Plaintext kiosk password (`Fuckp@ssw0rds`); the user is in `wheel` and has SSH key + NetworkManager access, so the password effectively grants root.
|
||||
|
||||
- [ ] **Fix:** rotate both; the repo already has `.sops.yaml` / age encryption for secrets.
|
||||
|
||||
### 8. CI trigger mismatch (AGENTS.md stale)
|
||||
|
||||
**Location:** `.gitea/workflows/build-dev.yml:1-6`
|
||||
|
||||
The workflow triggers on `push` to `dev` + `workflow_dispatch`, but AGENTS.md documents a `repository_dispatch` (`radiantwave-dev-built`) trigger that isn't in the workflow. If the app repo dispatches that event, CI won't fire. Also note the workflow lives in `.gitea/workflows/` (AGENTS.md says `.forgejo/workflows/`).
|
||||
|
||||
- [X] **Fix:** Ignore this; Gitea does not yet support repository_dispatch
|
||||
|
||||
### 9. Dead `enableAutoUpgrade` parameter
|
||||
|
||||
**Location:** `flake.nix:56`
|
||||
|
||||
`mkKioskSystem` accepts `enableAutoUpgrade ? true` but never uses it; all variants auto-upgrade unconditionally. If the intent was to disable auto-upgrade on dev systems, it doesn't work.
|
||||
|
||||
- [X] **Fix:** use `enableAutoUpgrade` to gate `system.autoUpgrade.enable`, or remove the dead parameter.
|
||||
|
||||
### 10. Dead files
|
||||
|
||||
- `hardware/x86_64-nvme-uefi.nix` — not referenced by `flake.nix`.
|
||||
- `vm-disk-config.nix` — not referenced by `flake.nix`.
|
||||
|
||||
- [ ] **Fix:** delete `hardware/x86_64-nvme-uefi.nix` and `vm-disk-config.nix`, or wire them into `flake.nix`.
|
||||
|
||||
---
|
||||
|
||||
## Verified not-bugs
|
||||
|
||||
- **Unprivileged `ping` works.** NixOS sets `net.ipv4.ping_group_range = mkDefault "0 2147483647"` (`nixos/modules/tasks/network-interfaces.nix:1786-1787`), so the 10s `loginShellInit` network wait works as the kiosk user. iputils built without setcap is fine.
|
||||
- **Plymouth theme is correct in both phases.** The flake's sed rewrites `/usr/` → store path, and the NixOS module's `plymouth-initrd-themes` runCommand rewrites initrd store paths again (`sed -i "s,${builtins.storeDir}/.*/share/plymouth/themes,$out,"`), so `ImageDir`/`ScriptFile` resolve in both initrd and runtime.
|
||||
- **`sudo -n plymouth quit --wait` works.** The `security.sudo.extraRules` NOPASSWD rule (`configuration.nix:46-56`) matches; NixOS default sudoers `secure_path` includes `/run/current-system/sw/bin`.
|
||||
- **`wlr-randr` parsing in `rotated_display.nix:5` is correct.** Output header is `DP-1 "…"` — the first field is the output name, so `grep -m1 '^\S' | awk '{print $1}'` yields the right name.
|
||||
- **`radiantwave-dev` exposes the needed outputs.** `nixosModules.default` and `packages.default` exist for `x86_64-linux` and `aarch64-linux` (RPi dev OK).
|
||||
- **Disk configs match target hardware.** `disk-config.nix` → `/dev/nvme0n1` (NUC), `commercial-disk-config.nix` → `/dev/sda`.
|
||||
- **Rollback churn won't kill the session.** `seatd` and `plymouth-quit` set `restartIfChanged = false`.
|
||||
|
||||
---
|
||||
|
||||
## Priority fix order
|
||||
|
||||
- [ ] 1. Add `"seat"` to kiosk `extraGroups` (`configuration.nix:111`). **Unblocks the app.**
|
||||
- [ ] 2. Fix the healthcheck matcher (`configuration.nix:247`) and wrong data dirs (`configuration.nix:74-76, 210, 224`).
|
||||
- [ ] 3. Wire `radiantwavekiosk-commercial-dev` to `radiantwave-dev` + `-dev` upgrade ref (`flake.nix:142-148`).
|
||||
- [ ] 4. Parameterize the ISO config so `android-iso` installs `radiantwavekiosk-android`.
|
||||
- [ ] 5. Rotate committed secrets.
|
||||
|
||||
## Open item
|
||||
|
||||
Whether the seat-group issue is field-confirmed. If a deployed NUC currently boots into the app, finding #1 isn't biting and the mechanism needs re-checking (e.g. different seatd group config or a graphical session somewhere).
|
||||
Reference in New Issue
Block a user