Repository navigation
Conversation
Automated AI review
Outcome: At the pinned head, the prompt never appears. The click command is rejected by The toast is never sent:
|
| Step | State file | Toast |
|---|---|---|
| Plug in, unauthorized | uid recorded | shown |
| Unplug | pruned: printed, uid kept |
none |
| Replug, still unauthorized | uid kept | none |
| Authorized outside the menu | pruned: printed, uid kept |
none |
Impact: After one toast, a device stays silent until it is authorized through this menu. Skip, a dismissed toast, direct boltctl, unplug, replug and reboot all leave it silenced, because the state lives in ~/.local/state. This contradicts the PR description and the code comment.
Suggested change: Add unset 'notified[$known_id]' inside the prune loop. With both fixes, the synthetic run behaved as described. If "once per session" is intended, $XDG_RUNTIME_DIR would match the comment at line 54.
The new units are not packaged, which breaks first-run enablement
User units reach /usr/lib/systemd/user through explicit install lines in the omarchy-pkgs pkgbuilds/omarchy-settings/PKGBUILD, and test/shell.d/config-test.sh (package_defaults) tracks them. None of the three omarchy-thunderbolt-notify units appears in either place.
enable-user-units.sh: systemctl --user enable --now bt-agent ... omarchy-crash-watch <3 missing units>
-> systemd resolves every name before linking any (src/shared/install.c)
-> "Unit ... does not exist", exit 1, no links created, --now phase never reached
-> none of the listed units is enabled or started on a new install
An offline systemctl --root check on a scratch tree reproduced this: one missing name, exit 1, zero links. The control without the missing name linked every unit.
Impact: If the PR ships without packaging, new installs lose bt-agent, omarchy-sleep-lock, omarchy-migrate-notify, omarchy-fcitx5, omarchy-crash-watch and the rest of that list. On existing installs, the migration's fallback writes dangling .wants links.
Suggested change: Add install lines for the three units in omarchy-pkgs, and add them to package_defaults in config-test.sh. The crash-watch and fcitx5 changes did the same.
The path unit does not fire when a device connects
Entries under /sys/bus/thunderbolt/devices are created by bus_add_device() → sysfs_create_link() → kernfs_add_one(), which emits no inotify create event. The only kernfs fsnotify call is FS_MODIFY from kernfs_notify(), and systemd's PathChanged mask does not include IN_MODIFY (src/core/path.c). The directory always exists because the module loads from the initramfs. This is based on kernel and systemd source only, not observed on hardware.
Impact: The 30 s timer is the only hot-plug trigger, so detection takes up to about 35 s rather than happening at connect. The comment in the .path unit overstates this.
Suggested change: Either drop the .path unit and describe the timer accurately, or use a real event source. One option is a trigger-only udev rule setting ENV{SYSTEMD_USER_WANTS}, which authorizes nothing. Another is a watcher on boltctl monitor.
The migration starts only the service in a live session
migrations/1789816652.sh enables all three units but starts only omarchy-thunderbolt-notify.service. That runs one scan and exits, and the timer and path stay inactive until the targets are next activated, normally at the next login.
Impact: After omarchy update in a graphical session, a device plugged in later in that session is not detected.
Suggested change: Also start the .timer, and the .path if it is kept.
Smaller points
- Secure-mode success is reported as failure. When bolt authorizes a stored device with its key, the kernel reports
authorizedas2.bin/omarchy-thunderboltaccepts only1, so it prints "Failed to authorize" and skips the confirmation toast. Accepting any non-zero value avoids this. - Devices without
device_nameare invisible. The kernel hidesdevice_namewhen the device's DROM lacks a name or fails to parse. Both scripts require that attribute, so such a device gets no toast, and the menu says nothing is waiting, although bolt can still authorize it by numeric ID. This is uncommon on certified hardware. A fallback to the numeric vendor/device IDs, or to the unique id, would cover it. - "Enrolled devices" counts connected devices. Piped
boltctl listmarks every connected device with*whether or not it is stored, and marks disconnected enrolled devices witho. Rename the label, or count stored entries. - Polling on every install. The timer has no condition of its own, so it fires every 30 s on machines without Thunderbolt and for users who set the opt-out file, and each run adds info-level journal lines.
ConditionPathExistsGlob=/sys/bus/thunderbolt/devices/domain*plus the toggle condition on the timer would keep it inert there. - Rebase. The branch conflicts with
quattroininstall/user/first-run/enable-user-units.sh, wherequattroaddedowed.serviceand a hook line. Keep both sides. After the rebase, the migration timestamp is older than threequattromigrations.
Review information
Test scope: Source review of the PR against bolt 0.9.11, systemd 262, Linux kernel Thunderbolt/kernfs sources (v6.17 and v7.2.6), gum 2.0.1 and current omarchy-pkgs. Sandboxed runs executed the unmodified PR scripts, the notification helper and the migration against a synthetic sysfs tree, with boltctl, gum, busctl and systemctl stubbed. A separate offline systemctl --root check covered enable semantics. No Thunderbolt hardware, live bolt daemon, user systemd session or notification daemon was used, and the full ./test/cli and ./test/shell suites were not run. Path-unit and polling behavior are from source only.
AI process: Opus 5.5 Medium coordination and synthesis, independent Opus 5.5 Xhigh and GPT 6 Sol Xhigh technical assessments with targeted cross-checks of single-sourced claims, Opus 5.5 Medium editorial check.
Opt out: To stop receiving these reviews, reply to this comment saying so.
A Thunderbolt device connecting unauthorized keeps its USB ports silently dead while power and displays keep working, so the failure is invisible: Omarchy ships bolt, but nothing ever surfaces a waiting device. Show one critical toast per waiting device whose click opens the authorization menu, with a path unit for instant trigger and a timer backstop. Authorization stays explicit and per-device.
- Pass --exec words separately so omarchy-notification-send accepts the click command (single quoted word was rejected; toast never fired) - Actually unset pruned ids in the state file so a replugged device notifies again instead of staying silenced - Accept any non-zero authorized value: bolt's stored-key path reports 2 and was reported as a failure - Fall back to unique_id when the kernel hides device_name, so nameless devices still get a toast - Rename the misleading 'Enrolled devices' label to 'Connected devices' - Add Thunderbolt + opt-out conditions to the timer so machines without a controller do not log every 30s - Correct the path-unit comment: kernfs does not reliably deliver create events; the 30s timer is the guaranteed trigger - Start timer and path (not just the oneshot service) in the migration when a graphical session is live
44d3486 to
2ae0d79
Compare
|
Rebased onto Fixed:
Testing: 11-check regression suite (toast on connect, no duplicates, prune-on-unplug, re-notify on replug, authorized=2 handling, nameless devices, quiet exit on non-Thunderbolt) — all passing after the rebase. Still open (needs upstream/omarchy-pkgs):
|
Automated AI review
Outcome: Follow-up on the earlier review and the author's reply, checked at
The author's Without packaging, first-run no longer completesThe three units are still missing from the
At the rebased base the failure goes further than the earlier review described: Impact: On a new install built without the packaging change, none of the first-run user units are enabled, the theme-set hook is not installed, and first-run repeats at every login. Suggested change: Add New: the timer condition can disable detection for the whole sessionThe timer now has
This is based on kernel, systemd and bolt sources and docs only. It was not observed on hardware, and how many machines behave this way was not measured. Impact: On affected machines, a device connected after login gets no prompt for the rest of the session. The path-unit comment says "the 30s backstop timer is what guarantees detection", which does not hold there. Suggested change: Remove the Smaller points
Review informationTest scope: Source review of AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check. Opt out: To stop receiving these reviews, reply to this comment saying so. |
- Remove ConditionPathExistsGlob=domain* from the timer: systemd only evaluates conditions at unit start and a skipped timer is not retried, and domain* only exists while the controller's PCI device is bound, so on firmware-powered-down controllers detection went silent for the whole session. Keep only the opt-out condition; /sys/bus/thunderbolt exists whenever the module is loaded, so there is no per-fire cost concern on machines without hardware. - Pass '--' before device names in every gum style call: DROM names are unfiltered and a leading-dash name was parsed as an option (exit 80, blocking later devices; --help printed gum's help and, under set -e, skipped the success toast after a successful authorization). - Fall back to unique_id when device_name exists but is empty: the -r test passes on an empty attribute and the toast showed a blank name. - Correct the path unit comment: kernfs emits no fsnotify create event, so the unit does not fire on connect on current kernels; the 30s timer is the guaranteed trigger. - Reword the check script's state comment: the state intentionally persists across sessions (avoid re-nagging about the same dock).
|
All points addressed at Timer condition (removed). Dropped Dash-leading names (fixed). Every Empty Path unit comment (corrected). Now states plainly that kernfs emits no fsnotify create event, so the unit does not fire on connect on current kernels and the 30s timer is the guaranteed trigger. State-lifetime comment (reworded). Kept Packaging: agreed it's a ship-blocker for new installs and it lives in omarchy-pkgs — I'll open that PR (three run_suite.sh: removed from the branch in the next commit (it's my local harness; happy to contribute a proper #11874 double-prompt: acknowledged — will coordinate with that PR's author on the shared Suite result: 23/23 passing. |
The config test asserts every file the omarchy-settings PKGBUILD installs exists in the source tree. The units are now installed by omacom/omarchy-pkgs#713, so list them here to keep both halves of the packaging contract checked.
|
Packaging PR is up: omacom/omarchy-pkgs#713 — installs the three units in both |
Automated AI review
Outcome: Follow-up on the second review and author reply, at
First-run fails if this lands before omarchy-pkgs#713Dev builds track Suggested change: Merge omarchy-pkgs#713 first or together; say so in the PR description. Reproducer (fails at omarchy-pkgs master, passes with #713)Run from an omarchy checkout at
#!/bin/bash
# Run from an omarchy checkout at 6caba24c. $1: an omarchy-pkgs checkout.
# Builds omarchy-settings with that checkout's package(), then enables the
# first-run unit list offline against the packaged user units.
set -u
pkgs=${1:?usage: first-run-offline.sh /path/to/omarchy-pkgs}
work=$(mktemp -d)
mkdir -p "$work/src" "$work/pkg" "$work/bin"
ln -s "$PWD" "$work/src/omarchy"
# Icon thumbnails are irrelevant here; stand in for ImageMagick.
printf '#!/bin/bash\nout=${!#}; out=${out#PNG32:}; mkdir -p "$(dirname "$out")"; : >"$out"\n' >"$work/bin/magick"
chmod +x "$work/bin/magick"
(
set -e
export CARCH=x86_64 srcdir="$work/src" pkgdir="$work/pkg" PATH="$work/bin:$PATH"
source "$pkgs/pkgbuilds/omarchy-settings/PKGBUILD"
package
) >/dev/null
echo "package() exit: $?"
ls "$work/pkg/usr/lib/systemd/user" | grep thunderbolt || echo "no Thunderbolt units packaged"
# owed.service ships in the owe package, not omarchy-settings; stand in for it.
printf '[Service]\nExecStart=/bin/true\n[Install]\nWantedBy=default.target\n' >"$work/pkg/usr/lib/systemd/user/owed.service"
units=$(sed -n '/enable --now/,/^$/p' install/user/first-run/enable-user-units.sh | grep -o '[a-z0-9-]*\.\(service\|path\|timer\)')
systemctl --root="$work/pkg" --global enable $units >/dev/null 2>"$work/err"
echo "systemctl enable exit: $?"
grep -v -e '^Created symlink' -e 'is added as a dependency' "$work/err"
echo "links created: $(find "$work/pkg/etc/systemd/user" -type l 2>/dev/null | wc -l)"omarchy-pkgs omarchy-pkgs#713 head ( systemd 261.2 ( DetailsMerge order: the dev lane repins the Still present
Related open PR #13312 (source-only): it clears the click action of toasts restored after a shell restart. The Thunderbolt toast is critical, so if it is still on screen it is restored, and a click then only dismisses it. Because the device stays in the notified state, no fresh toast follows while it stays connected. Keeping the state in Corrections to the second review
Verified (sandbox; real gum 2.0.0
Reproducer: #11874 conflictIn an omarchy clone with both PR heads fetched ( Control, the same #11874 head against Both PRs replace the same Review informationTest scope: Source and sandbox tests at AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check. Opt out: To stop receiving these reviews, reply to this comment saying so. |
The notified-state file persists in ~/.local/state and is only pruned when a device leaves or is authorized, so the pending list is not scoped to the current session. The old comment wrongly said "this session" and contradicted the pruning block below it.
|
Updated in response to the last review:
On the overlap with #11874: it edits the same |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if [[ -f $state_file ]]; then | ||
| while IFS= read -r known_id; do | ||
| [[ -n $known_id && $known_id != $unique_id ]] || continue | ||
| printf '%s\n' "$known_id" | ||
| done <"$state_file" >"$state_file.tmp" | ||
| mv "$state_file.tmp" "$state_file" |
There was a problem hiding this comment.
omarchy-thunderbolt-check and omarchy-thunderbolt both rewrite notified without a shared lock. A scan can save an old copy after the menu removes an authorized device, restoring its ID. If that device reconnects before another scan sees it authorized or absent, its next prompt is suppressed.
Two open menus also share notified.tmp, so one can overwrite the other's changes or fail at mv. Use one shared lock for both read-and-write paths and a separate temporary file for each writer.
The scanner (omarchy-thunderbolt-check) and the reviewer (omarchy-thunderbolt) both rewrite the notified-state file, so a scan's write could restore an id a menu had just removed for an authorized device, and two open menus shared one notified.tmp. Serialize the read-modify-write on a shared flock and merge the on-disk set at write time; the menu removes through a unique mktemp file. A device can also be unplugged mid-scan; the [[ -r ]] checks did not protect the read that followed, so a failed read aborted the scan under set -e or closed the menu before other devices were reviewed. Read every attribute tolerantly and skip a device that left. The sysfs root honours OMARCHY_THUNDERBOLT_DEVICES_PATH, matching the other hw-* overrides, so behavior can be tested without hardware.
Adds shell coverage with a synthetic sysfs tree and stubbed boltctl, gum and notification helpers: detection, duplicate suppression, prune on unplug, replug, nameless/empty device names, retry after a failed toast, state merge, and the menu's Skip/cancel/authorize/enroll/failure paths.
|
Addressed the three Greptile findings at feae654:
Verified: the new suite (17 checks) passes. |
3 participants
What
When a Thunderbolt device connects unauthorized (BIOS security level
user/secure), its USB ports stay silently dead — power delivery and displays keep working, so the failure is invisible. Omarchy shipsbolt(#1894) but nothing ever surfaces a waiting device. This adds a prompt: one critical toast per waiting device, whose click opens a floating terminal running agummenu — Authorize for this session / Authorize permanently (enroll) / Skip — drivingboltctl.Proposal discussion: #12508.
Merge order (required): this PR ships its systemd user units through omarchy-pkgs, so it must merge together with, or after, omarchy-pkgs#713. Without those units,
systemctl --user enable --nowin first-run fails and repeats on every login until the package ships them.Why explicit-only
#934's blanket auto-authorize was rejected over the DMA exposure. This keeps that line: no udev rules, no auto-authorization, and nothing runs
boltctlwithout a human choosing it in the menu.How
bin/omarchy-thunderbolt-check(hidden): scans/sys/bus/thunderbolt/devices, notifies once per waiting device — state file records only successful notifications so a failed handover retries; ids are pruned once a device leaves or becomes authorized. Notification state persists across sessions (~/.local/state), so a device toasted once stays quiet on later logins until it disconnects or is authorized — intentional, it avoids re-nagging about the same dock every session. Mirrorsomarchy-migrate-notify'somarchy-notification-waitrace guard, click-handover, and tty fallback.bin/omarchy-thunderbolt: the reviewer. Verifies the device reports authorized in sysfs (authorized != 0, not just1) after the action before claiming success; drops the device from the notification state so a future unauthorized event starts fresh.default/systemd/user/omarchy-thunderbolt-notify.{service,path,timer}: a oneshot gated onWAYLAND_DISPLAYplus an opt-out toggle file (~/.local/state/omarchy/toggles/thunderbolt-auth-off), a 30s backstop timer, and aPathChangedunit kept as a best-effort instant trigger — on current kernels kernfs emits no fsnotify create event, so connect detection is guaranteed by the timer, not the path watch.install/user/first-run/enable-user-units.sh, plus a migration that enables the units on existing installs and falls back to writing the wants symlink directly when no live user manager is available (e.g. an update from a TTY).Relationship to #11874
#11874 (
acrogenesis) adds a broader USB/Thunderbolt approval system and edits the sameenable-user-units.shunit list, so the two will conflict textually at merge and both would prompt for the same Thunderbolt device. They don't need to ship together, but whichever lands second needs a rebase, and the double-prompt overlap should be settled before both land.Tested
./test/cligreen on currentquattro(router group + command metadata); style perAGENTS.md;systemd-analyze verifyclean.test/shell.d/thunderbolt-test.sh(new): detection, duplicate suppression, prune on unplug, replug, nameless/empty device names, retry after a failed toast, state merge, and the menu's Skip/cancel/authorize/enroll/failure paths, against a synthetic sysfs tree with stubbedboltctl/gum/notification helpers.$HOME: idempotent, correct symlink fallbacks.boltctl forget→ replug → toast → click → menu → enroll → dock USB restored.