Add updater download and verification (#444, #445) - #733
Merged
Conversation
Wire downloadAndInstall through ArtifactResolver, a resumable worker-thread downloader with retry/cancel, and SHA-256 + minisign verification before ReadyToInstall. Enables the dialog download button and auto-download path. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughImplements the full auto-updater download-verify-install pipeline: platform-aware artifact resolution via ChangesAuto-updater download, verify, and install pipeline
Sequence Diagram(s)sequenceDiagram
participant User
participant UpdaterDialog as UpdaterDialog (QML)
participant Controller as UpdaterController
participant Worker as UpdaterWorker
participant Network as QNetworkAccessManager
participant Verifier as UpdateVerifier
rect rgba(100, 149, 237, 0.5)
Note over Controller,Worker: Update detected
Controller->>Controller: beginDownloadIfNeeded(false)
Controller->>Worker: downloadUpdate(DownloadRequest)
end
rect rgba(144, 238, 144, 0.5)
Note over Worker,Network: Resumable artifact download
Worker->>Network: GET artifact (Range header)
Network-->>Worker: progress / data chunks
Worker-->>Controller: downloadProgress(received, total)
Controller-->>UpdaterDialog: progressChanged
Worker->>Network: downloadUrlBlocking(signature)
Worker->>Network: downloadUrlBlocking(SHA256SUMS)
Worker-->>Controller: downloadFinished(DownloadOutcome)
end
rect rgba(255, 200, 100, 0.5)
Note over Controller,Verifier: Verification stage
Controller->>Worker: verifyDownload(VerifyRequest)
Worker->>Verifier: verifyDownloadedArtifact(artifact, sig, manifest, name)
Verifier-->>Worker: Outcome
Worker-->>Controller: verifyFinished(VerifyOutcome)
Controller->>Controller: state = ReadyToInstall
Controller-->>UpdaterDialog: stateChanged
end
User->>UpdaterDialog: sees "Ready to install" screen
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 143050f5ab
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/updater/UpdaterWorker.cpp (1)
110-344: ⚡ Quick winAdd breadcrumbs for download/verify I/O operations in this worker path.
This new flow introduces significant user-visible I/O stages (artifact download, sidecar download, verification) without breadcrumb instrumentation in this file. Add
SentryReporter::addBreadcrumb()at start/retry/success/failure points usingfile.importfor downloads and a consistent category for verify stage events.As per coding guidelines, “Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb() with categories: 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tools, 'file.import'/'file.export' for I/O.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/updater/UpdaterWorker.cpp` around lines 110 - 344, Add Sentry breadcrumbs to instrument the download and verification workflow in UpdaterWorker. Insert SentryReporter::addBreadcrumb() calls with category "file.import" at the following points: at the start of downloadUpdate() to mark the beginning of the artifact download, at the beginning of startArtifactDownloadAttempt() to track each retry attempt with the attempt number, in onDownloadReplyFinished() to record successful artifact download completion and again before downloading each sidecar file (signature and sha256Sums), and in verifyDownload() at the start and upon completion to track the verification stage. Use descriptive messages that include relevant context like attempt numbers and file names to aid debugging.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmake/Libsodium.cmake`:
- Around line 16-30: The source-build fallback path in this CMake file uses a
make invocation with the -j flag that lacks a numeric argument, which is
incompatible with BSD make (the default on macOS). Locate the make command
invocation (referenced as being around line 67) in the source-build fallback
section and modify it to specify a concrete number of parallel jobs or use a
CMake-compatible approach that works across both GNU make and BSD make. Ensure
the parallel build flag is portable by providing an explicit job count or using
a CMake function that handles this portability concern.
In `@docs/AUTO_UPDATER_DESIGN.md`:
- Around line 45-47: The documentation contains contradictory statements about
platform support for MinisignVerify. Line 45 states the feature is "Linux-only"
while line 46 indicates that MinisignVerify with QTMESH_MINISIGN_VERIFY=1
supports both Linux and macOS via libsodium. Resolve this contradiction by
choosing one consistent statement across both lines. Either revise line 45 to
accurately reflect that Linux and macOS are supported, or revise line 46 to
clarify the actual platform scope. Ensure the final wording clearly states which
platforms are currently supported under QTMESH_MINISIGN_VERIFY.
In `@src/updater/ArtifactResolver.cpp`:
- Around line 79-80: The error message string at lines 79-80 in the
ArtifactResolver.cpp file currently mentions only AppImage and .tar.gz as
accepted portable Linux package formats, but the resolver pattern actually
accepts .tar.xz as well. Update the error message string to include .tar.xz in
the list of supported extensions alongside AppImage and .tar.gz to ensure users
are provided accurate information about what package formats are accepted.
- Around line 58-60: The install-flavor validation logic in ArtifactResolver.cpp
has an inverted condition that allows package-managed installs to bypass the
error check. The current condition at the if statement rejects only when the
flavor is NOT package-manager-managed, NOT portable, and NOT unknown, which
means package-managed flavors pass through without triggering the error. Since
the error message states "In-app download is only supported for portable
installs", you need to invert the logic by removing the
InstallFlavor::isPackageManagerManaged() check and keeping only the checks that
ensure the flavor is either Portable or Unknown; if it's any other flavor type
(especially package-managed), the error should be triggered.
In `@src/updater/MinisignVerify.h`:
- Around line 14-15: The documentation comment in MinisignVerify.h incorrectly
states that only Linux links libsodium when QTMESH_MINISIGN_VERIFY is set, but
this PR enables the backend on macOS as well. Update the comment at lines 14-15
to reflect that both Linux and macOS support the minisign verify backend with
libsodium, either by explicitly listing both platforms or by using
platform-agnostic language such as "supported platforms" instead of specifically
mentioning only Linux.
In `@src/updater/UpdaterController.cpp`:
- Around line 435-492: Both handleDownloadFinished and handleVerifyFinished
methods need to validate that the outcome is still relevant for the current
state before processing it. Add a check at the beginning of each method to
ensure the current state matches the expected state for that operation. In
handleDownloadFinished, verify the state is in an active download state before
processing the outcome. In handleVerifyFinished, verify the state is in the
Verifying state before processing the outcome. If the state has changed (e.g.,
due to cancellation), return early without updating the state or error message.
This prevents stale signals from resurrecting canceled operations.
- Around line 367-369: In the downloadAndInstall() method, change the breadcrumb
category passed to SentryReporter::addBreadcrumb() from "updater.download.start"
to "ui.action" on line 367, since this action is triggered by a user button
click and should follow the coding guideline of using the "ui.action" category
for toolbar and button-triggered user-facing actions.
- Around line 403-408: The QDir().mkpath(stagingRoot) call does not check its
return value, which is a boolean indicating success or failure. Capture the
return value from mkpath() and add an explicit check: if it returns false, log
an appropriate error message and return early from the function or throw an
exception to prevent the workflow from continuing with a non-existent staging
directory. This will ensure failures are caught immediately with proper error
context rather than failing later in the update process.
In `@src/updater/UpdaterWorker.cpp`:
- Around line 177-203: The downloadUrlBlocking() method creates a local
QNetworkReply pointer but never assigns it to the m_activeReply member variable,
preventing cancelActiveRequest() from actually canceling the blocking download
since it returns early when m_activeReply is null. To fix this, assign the reply
returned from m_network->get(httpRequest) to m_activeReply immediately after
creation, then clear it back to nullptr after the event loop completes (after
loop.exec()). Additionally, ensure the propagation of the cancelled state
through finishDownloadWithError() calls is consistent by passing the appropriate
cancellation flag to maintain proper controller state handling.
- Around line 256-278: The code does not handle the case where a server ignores
a Range header request during resume operations and responds with 200 OK (full
body) instead of 206 Partial Content, causing data corruption when the full
payload is appended to an existing partial file. Reorder the logic so the 416
status check (for Range Not Satisfiable) executes before the generic httpOk
error check. Additionally, add a new condition that checks if httpStatus equals
200 while append is true (indicating the server ignored the Range header), and
in that case, remove the partial file at m_downloadRequest.artifactPartPath and
restart the download attempt from the beginning using
startArtifactDownloadAttempt. This prevents blindly appending full responses to
partial files.
---
Nitpick comments:
In `@src/updater/UpdaterWorker.cpp`:
- Around line 110-344: Add Sentry breadcrumbs to instrument the download and
verification workflow in UpdaterWorker. Insert SentryReporter::addBreadcrumb()
calls with category "file.import" at the following points: at the start of
downloadUpdate() to mark the beginning of the artifact download, at the
beginning of startArtifactDownloadAttempt() to track each retry attempt with the
attempt number, in onDownloadReplyFinished() to record successful artifact
download completion and again before downloading each sidecar file (signature
and sha256Sums), and in verifyDownload() at the start and upon completion to
track the verification stage. Use descriptive messages that include relevant
context like attempt numbers and file names to aid debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09aa5e2f-aebf-4610-a552-90f2e88b5b27
📒 Files selected for processing (17)
cmake/Libsodium.cmakedocs/AUTO_UPDATER_DESIGN.mdqml/UpdaterDialog.qmlsrc/CMakeLists.txtsrc/updater/ArtifactResolver.cppsrc/updater/ArtifactResolver.hsrc/updater/ArtifactResolver_test.cppsrc/updater/MinisignVerify.cppsrc/updater/MinisignVerify.hsrc/updater/MinisignVerify_test.cppsrc/updater/UpdateVerifier.cppsrc/updater/UpdateVerifier.hsrc/updater/UpdateVerifier_test.cppsrc/updater/UpdaterController.cppsrc/updater/UpdaterController.hsrc/updater/UpdaterWorker.cppsrc/updater/UpdaterWorker.h
Link ArtifactResolver/UpdateVerifier/MinisignVerify into MaterialEditorQML test targets, stream artifact bytes to .part during download, track sidecar replies for cancellation, fix portable-only flavor gate, and use portable make -jN for macOS libsodium builds. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/CMakeLists.txt`:
- Around line 109-111: The updater component test implementation sources are
included in TEST_SRC_FILES at lines 109-111 (ArtifactResolver.cpp,
UpdateVerifier.cpp, MinisignVerify.cpp), but the corresponding test executables
are not registered in the CMake build system. Add create_test_executable()
function calls for each of the three updater test files: ArtifactResolver_test,
UpdateVerifier_test, and MinisignVerify_test, following the same pattern used
for existing test registrations like CloudAccountMenuButton_test and
ProjectPackager_test, to ensure these tests compile and execute as part of the
test suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba0637e8-cf3e-41ad-9c91-a2e16cea01a4
📒 Files selected for processing (7)
cmake/Libsodium.cmakedocs/AUTO_UPDATER_DESIGN.mdsrc/updater/ArtifactResolver.cppsrc/updater/ArtifactResolver_test.cppsrc/updater/UpdaterWorker.cppsrc/updater/UpdaterWorker.htests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/AUTO_UPDATER_DESIGN.md
- cmake/Libsodium.cmake
- src/updater/ArtifactResolver.cpp
- src/updater/ArtifactResolver_test.cpp
- src/updater/UpdaterWorker.h
- src/updater/UpdaterWorker.cpp
Comment on lines
+109
to
+111
| ${CMAKE_CURRENT_SOURCE_DIR}/../src/updater/ArtifactResolver.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/../src/updater/UpdateVerifier.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/../src/updater/MinisignVerify.cpp |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if updater test files exist and search for their registration in CMake
echo "=== Checking for updater test files ==="
fd -t f 'ArtifactResolver_test\.cpp|UpdateVerifier_test\.cpp|MinisignVerify_test\.cpp' src/
echo ""
echo "=== Searching for test file registration in CMake ==="
rg -n -C3 'ArtifactResolver_test|UpdateVerifier_test|MinisignVerify_test' -g 'CMakeLists.txt' -g '*.cmake'
echo ""
echo "=== Checking if tests are included in MaterialEditorQML_test.cpp ==="
if [ -f src/MaterialEditorQML_test.cpp ]; then
rg -n '`#include`.*test\.cpp|ArtifactResolver.*Test|UpdateVerifier.*Test|MinisignVerify.*Test' src/MaterialEditorQML_test.cpp
else
echo "MaterialEditorQML_test.cpp not found in src/"
fiRepository: fernandotonon/QtMeshEditor
Length of output: 345
🏁 Script executed:
# Check the full context of the tests/CMakeLists.txt file
head -150 tests/CMakeLists.txtRepository: fernandotonon/QtMeshEditor
Length of output: 9284
🏁 Script executed:
# Look for CTest configuration or test discovery patterns
rg -n 'add_test|enable_testing|gtest|TEST|SOURCES' tests/CMakeLists.txt | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 1513
🏁 Script executed:
# Check if there's a pattern that auto-includes test files
rg -n 'aux_source_directory|file.*GLOB.*test|foreach.*test' tests/ -g 'CMakeLists.txt'Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Verify the actual structure of test executables defined in CMakeLists.txt
rg -n 'add_executable.*test' tests/CMakeLists.txt -A 5Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Find where MaterialEditorQML_test.cpp is located or if it exists
fd -t f 'MaterialEditorQML_test\.cpp|.*_test\.cpp' . | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 829
🏁 Script executed:
# Check what test executables are actually being created
sed -n '440,500p' tests/CMakeLists.txtRepository: fernandotonon/QtMeshEditor
Length of output: 2304
🏁 Script executed:
# Look for add_executable definitions to see test structure
rg -n 'add_executable' tests/CMakeLists.txt -A 10 | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 887
🏁 Script executed:
# Check if the updater test files have actual test code
head -50 src/updater/ArtifactResolver_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1550
🏁 Script executed:
# Find where test executables are being created - search for create_test_executable calls
tail -200 tests/CMakeLists.txt | head -100Repository: fernandotonon/QtMeshEditor
Length of output: 3299
🏁 Script executed:
# Search for all create_test_executable calls related to updater tests
rg -n 'create_test_executable.*updater|ArtifactResolver_test|UpdateVerifier_test|MinisignVerify_test' tests/CMakeLists.txtRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Get the total line count of CMakeLists.txt to understand file structure
wc -l tests/CMakeLists.txtRepository: fernandotonon/QtMeshEditor
Length of output: 95
🏁 Script executed:
# Find all create_test_executable calls in the entire file
rg -n 'create_test_executable' tests/CMakeLists.txt | tail -50Repository: fernandotonon/QtMeshEditor
Length of output: 604
🏁 Script executed:
# Check if the updater test files are `#included` in any of the registered test files
rg -n 'include.*ArtifactResolver_test|include.*UpdateVerifier_test|include.*MinisignVerify_test' src/ tests/Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Show the registered test executables to confirm
sed -n '509,542p' tests/CMakeLists.txtRepository: fernandotonon/QtMeshEditor
Length of output: 1483
🏁 Script executed:
# Check if MaterialEditorQML_test.cpp includes the updater test files
rg -n 'ArtifactResolver|UpdateVerifier|MinisignVerify' src/MaterialEditorQML_test.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Show what's in MaterialEditorQML_test.cpp - first 100 lines
head -100 src/MaterialEditorQML_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3176
🏁 Script executed:
# Check if there are any `#include` directives for test files in MaterialEditorQML_test.cpp
grep -n '`#include`.*_test\.cpp' src/MaterialEditorQML_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there are any other create_test_executable calls further down in the file
tail -100 tests/CMakeLists.txt | grep -n 'create_test_executable'Repository: fernandotonon/QtMeshEditor
Length of output: 445
🏁 Script executed:
# Confirm: list ALL test executables being created
grep -n 'create_test_executable' tests/CMakeLists.txtRepository: fernandotonon/QtMeshEditor
Length of output: 604
Register the updater component test files as test executables.
The updater test files exist (ArtifactResolver_test.cpp, UpdateVerifier_test.cpp, MinisignVerify_test.cpp) but are not registered in the CMake build system. The implementation sources are correctly included in TEST_SRC_FILES at lines 109-111, but the corresponding test files must be registered as separate test executables using create_test_executable() calls (as done for CloudAccountMenuButton_test and ProjectPackager_test), or included in an existing comprehensive test executable.
Currently, only these test executables are registered:
- MaterialEditorQML_test
- MaterialEditorQML_qml_test
- MaterialEditorQML_perf_test
- CloudAccountMenuButton_test
- ProjectPackager_test
Add registration for the three updater test files to ensure they compile and run as part of the test suite.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/CMakeLists.txt` around lines 109 - 111, The updater component test
implementation sources are included in TEST_SRC_FILES at lines 109-111
(ArtifactResolver.cpp, UpdateVerifier.cpp, MinisignVerify.cpp), but the
corresponding test executables are not registered in the CMake build system. Add
create_test_executable() function calls for each of the three updater test
files: ArtifactResolver_test, UpdateVerifier_test, and MinisignVerify_test,
following the same pattern used for existing test registrations like
CloudAccountMenuButton_test and ProjectPackager_test, to ensure these tests
compile and execute as part of the test suite.
|
This was referenced Jun 18, 2026
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
1 participant
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
.partstaging under AppData).ArtifactResolver(platform-specific release asset pick) andUpdateVerifier(SHA-256 manifest + minisign signature).UpdaterController::downloadAndInstall(), auto-download when enabled, and dialog UI throughReadyToInstall(install step remains Update: macOS installer — replace .app bundle atomically #446–448).Test plan
UnitTests --gtest_filter="ArtifactResolver*:UpdateVerifier*:MinisignVerify*:UpdaterController*"passes locally on Linuxunit-tests-linuxgreenbuild-windows/build-macosgreen (Windows verify backend still stubbed — fail-closed at signature step)Made with Cursor
Summary by CodeRabbit