Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Both review findings are fixed in a33b78a. 1. Privileged helper resolved via PATH ( Confirmed as a real local privilege escalation, not a theoretical one. Fixed by removing PATH from that decision entirely. Verified, not just reasoned about. I compiled the pre-fix and post-fix
2. Wallpaper grid not refreshed after install ( Correct — a newly installed pack drops a Build/test on arm64 (GTK4/libadwaita/Vala): @codex review |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mirkobrombin
left a comment
mirkobrombin
left a comment
There was a problem hiding this comment.
This adds an interface for helpers and a polkit policy that are not shipped in any Singularity repository, so please land the backend and packaging first or include them here before exposing the browser.
|
Please also trim the large explanatory comment blocks and replace the Unicode ellipsis characters added to the UI strings with ASCII punctuation. |
perlowja
changed the title
|
Addressed via path B in a23edb4. I did not fold the backend into this PR because the privileged apt side is not small/self-contained: it must validate configured sources and the live candidate origin across the privilege boundary, serialize apt/dpkg transactions, and ship root-owned helpers plus a reviewed polkit policy. There is no analogous apt helper in the Singularity org to reuse. Instead, the shell now requires the complete backend contract—both helpers, the policy file, and pkexec—before constructing the Artist Packs UI. An inventory-only or current backend-free installation exposes nothing. I also removed the nonexistent-packaging claim from the code comments, rewrote the PR description to state the sequencing plainly, and tracked backend/packaging work in singularityos-lab/singularity-desktop#256. Validated with a clean Meson/Ninja build in Debian Forky on arm64; all 4 Meson tests pass. |
3cb140c to
c841db1
Compare
Combine the Artist Pack browser with the integrated wallpaper and OCS feature line. Assisted-by: Codex:gpt-5 AI-Scope: Merged PR singularityos-lab#28 into the integrated wallpaper branch and reconciled the Meson source list to retain both feature lines.
mirkobrombin
left a comment
mirkobrombin
left a comment
There was a problem hiding this comment.
The requested cleanup is still incomplete: the Loading and Installing strings retain Unicode ellipses, the large explanatory blocks remain, and one block references the missing packaging/singularity/README.md file.
|
Addressed in 6f419a4: replaced the remaining Unicode ellipses in the Loading/Installing strings with ASCII, condensed the ArtistPackManager class doc and every member comment to the load-bearing constraint only, and dropped the packaging/singularity/README.md reference (the reasoning it pointed at is already stated inline). Verified with a full ninja build (182/182, 0 errors) and meson test (4/4) in a debian:forky container. |
|
@mirkobrombin please take another look. |
6f419a4 to
9cff18c
Compare
Combine the Artist Pack browser with the integrated wallpaper and OCS feature line. Assisted-by: Codex:gpt-5 AI-Scope: Merged PR singularityos-lab#28 into the integrated wallpaper branch and reconciled the Meson source list to retain both feature lines.
|
Thanks for the cleanup. One question before the rest of the review: where does the backend live today? An earlier comment mentions Could you share it, ideally as a PR linked to singularityos-lab/singularity-desktop#256, so I can look at both halves together? |
… backend Adds an Artist Packs group to the Desktop settings page: lists curated wallpaper packs (ncz-wallpapers-* debs) available or already installed via apt, with an Install button per pack. Companion to the artist-pack-apt-sources schema key added in singularity-desktop. ArtistPackManager (src/core/artist_pack_manager.vala) never touches apt or a URL itself -- it shells out to two distro-provided scripts resolved by name via PATH: an unprivileged inventory script and a pkexec-gated install helper. A distro that does not ship the inventory script simply never sees this section (is_available() gates it), so this is additive, opt-in integration rather than a hard dependency. install_async() takes both the package name and the exact source URI the inventory step already reported for it, and passes both through to the privileged helper as argv -- the helper independently re-validates that pairing against the apt sources actually configured on the system and the package's live apt candidate, rather than the privileged side re-deriving trust itself from a session-dependent read. (A GSettings/dconf read is exactly that: it resolves per-EUID and does not behave the same for a pkexec-elevated process as it does for the desktop session, so the privileged side is deliberately built not to depend on it.) Uses only Singularity.Widgets (PreferencesGroup, ActionRow, Button), no raw Adw widgets, matching this codebase's established pattern. Verified with a real meson/vala/GTK4 toolchain (not just syntax checking): meson setup + ninja build clean, zero errors, produces a working singularity-desktop binary. Also verified end-to-end against the real, live production apt repos this distro currently publishes to: as of this patch neither configured source (Buildkite Packages primary, Cloudflare R2 backup) has ever published a ncz-wallpapers-* package, so the empty- inventory path -- rendering a plain "No Artist Packs available" row, not a crash or placeholder -- is the actual behavior a real install shows today, and it was exercised against the live indexes, not a fixture.
install_async() resolved ncz-wallpaper-pack-install with Environment.find_program_in_path() and then handed the resolved path to pkexec as the program to execute as root. Any directory the desktop user can write to that sits earlier in the process's PATH (~/.local/bin and ~/bin are user-writable and commonly precede /usr/local/bin) was therefore enough to have an arbitrary binary elevated - a local privilege escalation. Both helpers now come from compiled-in absolute paths matching where post-install/49-artist-pack-browser.sh installs them and the org.freedesktop.policykit.exec.path annotation in the dev.sinty.desktop.artist-pack-install polkit action, so nothing the environment controls can influence which file is elevated. pkexec itself is resolved the same way, from a fixed list. Presence is checked with FileUtils.test() instead of a PATH search. Also refresh the wallpaper grid after a pack installs. The callback only relabelled the button; the grid's pack directories come from collection_dirs(), which is re-read only by populate_grid(), and settings pages are cached - so a newly installed pack stayed invisible until the user navigated away and back. Assisted-by: Claude Code:claude-opus-5 AI-Scope: Applied the PATH-resolution security fix and the post-install grid refresh, and ran the build, tests and PATH-hijack verification.
…installs Clicking the header Refresh button while an install was in flight cleared the group and rebuilt the row from an inventory fetched before the apt transaction finished, so the pack still read as not installed and the row came back with an enabled Install button. Two consequences: a second concurrent install could be started for the same package, and the completion callback then relabelled captured_btn -- by that point the detached old button -- leaving the visible row stuck on "Install" indefinitely. Track in-flight packages in artist_packs_installing and let a rebuild trust that over the stale inventory answer, so the row keeps rendering a disabled "Installing…". The install itself moves to start_artist_pack_install(), which also records the refresh generation it started under: if that moved, the button it holds is detached, so it repopulates the inventory instead of relabelling a widget nobody can see. The set doubles as the guard against a second install of a package already running. Verified against a harness replicating both state machines (pre-fix at a33b78a and this patch) on one timeline -- install at t=0 completing at t=300ms, refresh at t=100ms, inventory landing stale at t=150ms: PRE-FIX, no second click : row stuck at label='Install' sensitive=true while the DETACHED button was relabelled 'Installed' PRE-FIX, second click : install_calls=2 -- two apt transactions, one pack POST-FIX, same timeline : label='Installing…' at t=200, 'Installed' at t=600, install_calls=1 Assisted-by: Claude Code:claude-opus-5 AI-Scope: Authored the in-flight tracking and the generation-aware completion path, and built the pre/post-fix timing harness used to confirm both symptoms.
…d README reference
Addresses Mirko's review: the Loading/Installing strings still used Unicode
ellipses, the class-level and field comments in ArtistPackManager stayed
long-form after the prior cleanup pass, and one paragraph pointed at
packaging/singularity/README.md, which does not exist in this repository.
- desktop_page.vala: replaced the three remaining Unicode ellipsis
characters ("Loading...", "Installing...") with ASCII, and trimmed two
verbose inline comments in the Artist Packs section down to the
non-obvious invariant each one was actually documenting.
- artist_pack_manager.vala: condensed the class doc comment and every
member comment to the load-bearing constraint only (fixed PATH-free
helper paths and why, the GSettings/dconf trust-boundary split, the
install-path validation split between this class and the privileged
helper). Dropped the dangling README reference entirely rather than
pointing it at a real file, since the reasoning it referenced is already
stated inline in the same paragraph.
Verified: full ninja build (182/182 targets, 0 errors) and meson test
(4/4 pass) in a debian:forky podman container on ULTRA.
Assisted-by: Claude Code:claude-sonnet-5
AI-scope: identified and fixed every flagged item from Mirko's review
(remaining ellipses, oversized comment blocks, the broken doc reference)
and verified the build/test result quoted above.
Mirko (PR singularityos-lab#28 review, on f72af09): the Artist Pack browser gated on a helper interface and polkit policy that weren't shipped in any Singularity repository, so is_available() could never be true for anyone building this from source. Ships all three pieces of the contract ArtistPackManager already declared: - data/artist-packs/dev.sinty.desktop.artist-pack-install.policy - data/artist-packs/singularity-artist-pack-inventory (unprivileged) - data/artist-packs/singularity-artist-pack-install (pkexec target) Renamed from the ncz-wallpaper-pack-{inventory,install} names in the original commit -- those are NCZ-OS's own branding, out of place in a generic Singularity repository (rule 2 in the contribution guidelines). Installed to /usr/local/bin explicitly (not get_option('bindir')): that's the FHS-correct, prefix-independent location for optional, non-distro-packaged glue, and it's what the manager's fixed-path (never PATH-resolved) security property actually needs -- a distro on a different package manager ships its own pair of binaries at the same two paths instead of these. Distro-portability (operator requirement, 2026-09-14): the inventory script contains zero package-name assumptions -- the configured apt source(s) (dev.sinty.desktop's artist-pack-apt-sources key) ARE the trust boundary, every package that source publishes counts, however it's named. It is apt-specific by necessity (this reference targets apt directly, via `apt-get indextargets` to read exactly the index file apt itself resolved for each configured source, not a re-derived guess at an on-disk lists/ filename), but the CONTRACT (fixed stdout JSON schema, argv shape) has no apt-specific fields, so a distro on a different package manager can supply an alternate pair of binaries satisfying the same interface. Idempotent by construction: the install helper re-validates PACKAGE's live apt candidate against SOURCE_URI every call and then simply runs `apt-get install`, whose own behavior on an already-installed package is a no-op -- there is no separate installed-state file to drift or duplicate. Verified: both scripts pass `dash -n` (POSIX sh, not just bash-lenient syntax). Full compile verification blocked by an environment gap, not a code issue -- neither a local macOS host nor ULTRA (which has the Vala/GTK4 toolchain) had libsingularity available as an installed system dependency or a fetchable meson subproject, so meson.build's `dependency('singularity-1.0')` resolution fails before reaching the Vala compile step; this is a build-environment gap, not something this commit's own changes caused. Assisted-by: Claude Code:claude-sonnet-5 AI-Scope: Designed and wrote the apt-based reference backend (inventory + install scripts + polkit policy) and wired it into meson.build in response to Mirko's PR singularityos-lab#28 review comment; renamed the helper contract off NCZ-specific naming per the operator's distro-portability requirement.
9cff18c to
2196b51
Compare
|
Addressed both threads on this branch (now at 2196b51):
Also rebased onto current origin/main (this branch predated #25 and #27; no real conflicts beyond the two already-merged UI changes both branches independently touched). Assisted-by: Claude Code:claude-sonnet-5 |
2 participants
Summary
Adds the shell-side Artist Pack browser UI and its typed JSON interface, but keeps the feature unavailable unless the distro has installed the complete backend contract:
/usr/local/bin/ncz-wallpaper-pack-inventory/usr/local/bin/ncz-wallpaper-pack-install/usr/share/polkit-1/actions/dev.sinty.desktop.artist-pack-install.policypkexecThe UI is not constructed when any part is missing, so current Singularity installations do not expose a browser or an Install action that cannot work.
Scope and sequencing
This PR deliberately does not implement the privileged apt backend. That backend is security-sensitive packaging work: it must validate configured apt sources and the live candidate origin across the privilege boundary, serialize apt/dpkg transactions, and install root-owned helpers plus a polkit policy. It deserves its own implementation and review rather than being folded into this shell UI change.
Backend and packaging follow-up: singularityos-lab/singularity-desktop#256.
Once that issue lands, the existing availability gate will enable the browser without another shell change. Until then this remains dormant.
Implementation
ArtistPackManagerparses the inventory helper JSON into typed objects and invokes the fixed, absolute install-helper path through a fixedpkexecpath. The Desktop page handles loading, empty and error states, prevents duplicate installs, and refreshes the wallpaper collection after installation.No apt repository is hard-coded in the shell. Source selection and validation remain backend policy.
Test plan
pkexecbefore the UI is constructed.Compatibility
Additive and dormant by default. Builds without the separately packaged backend have no visible behavior change.
AI assistance: disclosed