Full project audit + fixes for all High/Medium findings#6
Merged
Conversation
Consolidated audit of Pop.Core, the Windows/macOS/Linux hosts, the release pipeline, and repo hygiene: 1 critical, 9 high, ~20 medium findings with file:line references, verified-sound areas, test coverage gaps, and prioritized recommendations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhoT77dxUk7AmRRUX3XFvV
…Ctrl vertical flick - JsonSettingsStore.LoadAsync now catches IO/access errors (not just JsonException) and backs up corrupt settings to *.corrupt before falling back to defaults; SaveAsync is serialized behind a gate to stop concurrent writers racing File.Replace/File.Move. - AppSettings.Normalized() clamps velocity/dominance/glide to safe ranges; applied on load so a hand-edited settings.json cannot defeat the dominance gate or produce negative animation durations. - DiagnosticsLogFormatter builds its field map with last-writer-wins so keys that collide after 48-char truncation no longer throw on the drag hot path. - SnapDecider rejects a Ctrl vertical flick that has no monitor above/below instead of snapping a horizontal half by landing X. - WindowAnimator clamps the short-duration branch to non-negative. - DragSession.ResetDragOrigin clears stale release state; documented the non-thread-safe contract on DragSession and IDragTracker. - Added tests for corrupt-file backup, settings clamping, colliding diagnostic keys, and the vertical-flick rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhoT77dxUk7AmRRUX3XFvV
…ce, safety nets - app.manifest declares PerMonitorV2 dpiAwareness so coordinate math is correct on mixed-DPI multi-monitor setups (the csproj ApplicationHighDpiMode property had no effect on this WPF app). - Win32WindowMover repositions via SetWindowPos with SWP_ASYNCWINDOWPOS instead of MoveWindow, so a move never blocks on a hung target window's message loop — including the in-drag restore that runs on the low-level mouse hook thread, which the OS would otherwise time out and silently unhook. - Added a Local\ named mutex single-instance guard in Main so a manual launch plus the Run-key entry no longer install two hooks/tray icons. - Global DispatcherUnhandledException / AppDomain / unobserved-task handlers fail soft and append to host-errors.log instead of crashing the tray-only process. - UpdateService.Dispose is idempotent and waits for the background loop; in-flight checks catch ObjectDisposedException; OnUpdateStateChanged guards a null Application.Current / shutting-down dispatcher; the Check-For-Updates handler swallows cancel/dispose races. - SetWindowsHookEx marshals the Win32 error into the failure message. - IStartupRegistration now returns success and can read back OS state; WindowsStartupRegistration implements both and no longer throws, and a failed registration write reverts the setting with a user error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhoT77dxUk7AmRRUX3XFvV
…OT barriers, AX monitor - ScreenCoordinator no longer double-flips: CGEvent locations and AX window rects are already top-left global coordinates and now pass through unflipped; only NSScreen (Cocoa, bottom-left) frames are flipped, pivoting on the primary screen height. This fixes inverted title-bar hit-testing, wrong snap Y placement, and inverted vertical velocity that mis-targeted stacked monitors. - GlobalDragTracker re-enables the CGEvent tap on tapDisabledByTimeout / tapDisabledByUserInput instead of going permanently deaf after one slow AX inspection. - The URLSession download delegate moves the temp archive to a stable path synchronously before the main-actor hop, so prepareUpdate no longer races URLSession deleting the file (updates were failing). - All six [UnmanagedCallersOnly] bridge exports wrap their body so a managed exception returns a safe default instead of fail-fasting the NativeAOT process. - PopRuntimeController polls Accessibility state so a mid-session revocation stops tracking and updates the menu instead of showing a stale 'Granted'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhoT77dxUk7AmRRUX3XFvV
…ave crash - X11PollingDragTracker wraps its loop body so an exception recovers instead of faulting the task and silently ending all snapping. - X11DisplayConnection installs a process-wide XSetErrorHandler that swallows async protocol errors (BadWindow from a window closed mid-drag) instead of letting Xlib exit(); Dispose is now idempotent and exposes IsDisposed. - X11WindowInspector resolves per-monitor geometry via Xinerama when more than one head is present (falling back to whole-display and to a single head, so single-monitor behaviour is unchanged and a missing libXinerama degrades gracefully). - X11WindowMover checks IsDisposed under the display lock so a move in flight during shutdown can't send on a freed Display. - LinuxPopHost guards _snapRestoreStates with a lock (poll thread vs async completion) and persists settings before adopting them in memory. - SettingsWindow preserves contract fields it doesn't expose (e.g. LaunchAtStartup) via 'with', and its async-void Save handler catches exceptions instead of crashing the app. - KWinWaylandIntegration handles a missing gdbus binary per its allowFailure contract instead of throwing at spawn time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhoT77dxUk7AmRRUX3XFvV
…ion, pinned tool - ci.yml declares least-privilege 'contents: read' and now builds Pop.App.Linux on the ubuntu leg, so Linux host breakage is caught at PR time instead of only during post-release packaging. - release.yml gains a concurrency group so two quick version bumps can't race the update-feed push and release-asset create/delete. - Both release asset-cleanup steps now preserve Pop-linux-x64-* assets, so re-running a release for an existing tag no longer deletes the published Linux downloads. - package-linux-release.sh pins appimagetool to an immutable release tag instead of the moving 'continuous' tag and verifies its checksum when APPIMAGETOOL_SHA256 is provided (warns otherwise). - Fixed the AppRun launcher so a failed non-terminal start reports the real exit code instead of always 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhoT77dxUk7AmRRUX3XFvV
- WindowsStartupRegistration needs an explicit 'using System.IO;' — the WPF SDK removes System.IO from implicit usings, unlike the plain SDK projects where the same catch-filter pattern compiles. - Suppress WFAC010 in the csproj with a comment: the analyzer suggests ApplicationHighDpiMode, but that only works via the WinForms ApplicationConfiguration bootstrap this WPF app never calls; the manifest dpiAwareness element is what WPF honors. - Use Assert.Single per xUnit2013 in the new diagnostics test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhoT77dxUk7AmRRUX3XFvV
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b270de3d6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| APPIMAGETOOL_TAG="${APPIMAGETOOL_TAG:-13}" | ||
| curl --fail --location --retry 3 --retry-delay 2 \ | ||
| "https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage" \ | ||
| "https://github.com/AppImage/AppImageKit/releases/download/${APPIMAGETOOL_TAG}/appimagetool-x86_64.AppImage" \ |
There was a problem hiding this comment.
The Release Linux workflow invokes this script on a fresh Ubuntu runner, so this download path is taken whenever the cached tool is absent. With the default APPIMAGETOOL_TAG=13, this URL is 404 because the AppImageKit tag 13 release publishes obsolete-appimagetool-x86_64.AppImage, not appimagetool-x86_64.AppImage (checked https://github.com/AppImage/AppImageKit/releases/tag/13); curl --help all also confirms --fail fails on HTTP errors, so set -e aborts packaging before the AppImage output is produced.
Useful? React with 👍 / 👎.
Review feedback: AppImageKit is deprecated and its tag-13 release assets were renamed with an 'obsolete-' prefix, so the previously pinned URL can 404 on a fresh runner. Try the renamed asset first, the pre-rename name second, and the maintained AppImage/appimagetool repo's continuous build as a last resort, logging which source was used. APPIMAGETOOL_URL forces a specific source and APPIMAGETOOL_SHA256 still enforces integrity of whatever is downloaded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhoT77dxUk7AmRRUX3XFvV
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba66c004b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| notify "Pop is running. Launch output is logged to $log_path." | ||
|
|
||
| if "$POP_BIN" "$@" >> "$log_path" 2>&1; then | ||
| "$POP_BIN" "$@" >> "$log_path" 2>&1 |
There was a problem hiding this comment.
When the AppImage is launched without a terminal and Pop exits non-zero, the generated AppRun has set -euo pipefail, so the shell aborts immediately at this command instead of reaching exit_code=$? and the "could not start" notification below. That leaves desktop launches showing the earlier "Pop is running" notification with no failure notification or log pointer even though startup failed.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A full audit of the project (report committed as
docs/AUDIT-2026-07-17.md), followed by fixes for every High and Medium finding, one commit per area:*.corrupt), value clamping viaAppSettings.Normalized(), serialized saves, diagnostics formatter no longer throws on colliding truncated keys, Ctrl vertical-flick no longer produces a horizontal snap, threading contracts documented. New tests for each behavior.PerMonitorV2DPI awareness via app.manifest (the csproj property had no effect on WPF); window moves viaSetWindowPos+SWP_ASYNCWINDOWPOSso a hung target app can't freeze system mouse input and get the LL hook silently removed; single-instance mutex; global exception handlers logging tohost-errors.log;UpdateService.Disposerace fixed;IStartupRegistrationnow reports success/read-back.ScreenCoordinatorno longer double-flips coordinates (CGEvent locations and AX rects are already top-left global; only NSScreen frames are flipped, pivoting on the primary screen height). This fixes inverted title-bar hit-testing, snapped windows landing ~55 px low, and inverted vertical velocity. Also: event tap re-enables aftertapDisabledByTimeout/ByUserInput; the update download is moved to a stable path synchronously before URLSession deletes its temp file; all six NativeAOT exports get exception barriers; Accessibility revocation is detected at runtime.XSetErrorHandler(a window closed mid-drag can no longerexit()the process); multi-monitor support via Xinerama (gated: single-head setups keep the exact previous path; missing libXinerama degrades gracefully); restore-state dictionary race fixed; save-path crash fixed; shutdown use-after-free on the X Display fixed; missinggdbushandled; non-UI settings fields preserved on save.permissions: contents: readon CI, a Linux build leg (Linux host breakage is now caught at PR time), release concurrency group, Linux assets preserved on release re-runs, appimagetool pinned to an immutable tag with optional sha256 verification, AppRun exit-code bug fixed.Not addressed (deliberately)
Verification
Changes were written via careful static review — the authoring environment could not run .NET or Swift toolchains, so CI on this PR is the compile/test verification across ubuntu, windows, and macos legs. The macOS coordinate fix should additionally get a manual smoke test on real hardware (drag a window by its title bar, flick left/right, confirm it lands flush with the menu-bar edge) before the next release.
🤖 Generated with Claude Code
https://claude.ai/code/session_01RhoT77dxUk7AmRRUX3XFvV
Generated by Claude Code