Use the Windows installer for Warp Agent CLI autoupdates - #14477
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR changes Warp Agent CLI autoupdates on Windows to download, verify, and execute the signed Inno installer instead of unpacking ZIP payloads in Rust, while leaving Unix archive updates mostly unchanged.
Concerns
- The Windows silent installer invocation does not pass the background-update flags that keep Inno from treating this as a normal install, so it can prompt/close the running process or skip update-specific handling.
- The Authenticode verification only checks that the file chains to any trusted code-signing publisher and disables revocation checking, which is too weak for a silent autoupdate executable.
Security
- The downloaded installer and installed payload executables should be verified against Warp's expected publisher/certificate identity with revocation checks enabled before execution/acceptance.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
58ace2d to
eabae0d
Compare
f1491fa to
ba1452d
Compare
ba1452d to
7d04937
Compare
7d04937 to
a0beaf4
Compare
9f15233 to
c39e9c5
Compare
acarl005
left a comment
There was a problem hiding this comment.
I think a wholistic pass over the generated unit tests is needed as there is a lot of things being tested that don't fall under the purview of unit tests.
| } | ||
|
|
||
| #[test] | ||
| fn windows_installer_arguments_preserve_custom_root() { |
There was a problem hiding this comment.
I would delete this windows_installer_arguments_preserve_custom_root test (and just inline windows_installer_args)
There was a problem hiding this comment.
I could simplify these. tests!
There was a problem hiding this comment.
Removed the argument test and the windows_installer_args helper. The installer arguments are now inlined at the execution call site in 9bd38f0.
| } | ||
|
|
||
| #[cfg(windows)] | ||
| fn verify_authenticode_signer(path: &Path) -> Result<String> { |
There was a problem hiding this comment.
I don't think signature verification is necessary. The GUI is already performing unattended upgrades on auto-update. We rely on the HTTPS/TLS security to ensure that the file we download is legitimate. This is redundant. I'd remove this whole procedure.
There was a problem hiding this comment.
Removed the entire WinTrust/Authenticode verification procedure and the resulting windows crate dependency in 9bd38f0. The updater now relies on the existing HTTPS/TLS download path, matching the GUI autoupdate model.
| "/update=1".into(), | ||
| "/NOCLOSEAPPLICATIONS".into(), | ||
| install_dir, | ||
| "/SKIP_PATH_UPDATE=1".into(), |
There was a problem hiding this comment.
Oops. I missed this detail when reviewing #14475 but with Inno Setup scripts it's conventional to use all uppercase for built-in params and lowercase for custom params. That's why it's /update=1. Same goes for param:WARP_BIN_DIR and any others in script/windows/tui-installer.iss
| "/SKIP_PATH_UPDATE=1".into(), | |
| "/skip_path_update=1".into(), |
There was a problem hiding this comment.
Ah I didn't know that
There was a problem hiding this comment.
Updated in 9bd38f0: this is now /skip_path_update=1, and all custom parameters in tui-installer.iss and its integration test (warp_bin_dir, skip_path_update, and allow_downgrade) are lowercase.
| fs::copy(&signed_executable, version_dir.join("vcruntime140.dll")).unwrap(); | ||
| fs::copy(&signed_executable, version_dir.join("vcruntime140_1.dll")).unwrap(); | ||
| fs::copy(&signed_executable, version_dir.join("msvcp140.dll")).unwrap(); | ||
| } |
There was a problem hiding this comment.
This smells like out-of-scope for unit testing.
There was a problem hiding this comment.
Fully removed in c0f66f0. The Rust completeness checks and unit fixtures no longer enumerate or create the Windows packaged payload files; shared updater tests now create only the binary and resources/. Installer payload completeness remains covered by the Inno installer integration test.
| .chain(std::iter::once(0)) | ||
| .collect::<Vec<_>>(); | ||
| let result = unsafe { | ||
| MoveFileExW( |
There was a problem hiding this comment.
We're dropping down to the low-level Win32 API to move a file with MoveFileExW instead of calling std::fs::rename?? It looks like it's for the purpose of passing the MOVEFILE_WRITE_THROUGH flag. That does add an extra level of guarantee that the file is flushed to disk... but this function is guarded with cfg(test) and I really doubt that such esoteric stuff is needed for a unit test. This smells like it's very out-of-scope for unit tests.
There was a problem hiding this comment.
Removed the test-only Windows point_current_at/MoveFileExW implementation and the atomic activation unit test in 9bd38f0. Cross-platform tests now write the Windows text pointer directly as fixture setup.
| if let Some(system_root) = std::env::var_os("SystemRoot").map(PathBuf::from) { | ||
| candidates.push(system_root.join("System32/WindowsPowerShell/v1.0/powershell.exe")); |
There was a problem hiding this comment.
hardcoded separators are an anti-pattern. see here:
warp/app/src/terminal/available_shells.rs
Lines 730 to 735 in cd45ebb
There was a problem hiding this comment.
Removed this signed-executable discovery fixture in 9bd38f0, including the hardcoded-separator paths. The simplified Windows fixture setup now constructs paths exclusively with Path::join.
9bd38f0 to
c0f66f0
Compare
Co-Authored-By: Warp <agent@warp.dev>
c0f66f0 to
16434a6
Compare

Description
Make Windows Warp Agent CLI background updates download and execute the same signed Inno installer used for initial installation.
Windows no longer parses or installs ZIP payloads in Rust. The updater verifies the installer with
WinVerifyTrust, invokes it silently against the detected managed root, verifies the completed version and active pointer, and then performs conservative lease-aware cleanup. Unix tarball updates, polling, status, telemetry, custom-root detection, and live-version leases are unchanged.Artifact publication remains intentionally disabled. This replaces reference PR #14446 without modifying it.
Depends on #14476 and warpdotdev/warp-server#13497.
Implementation plan: https://staging.warp.dev/drive/notebook/BI2TyxneCNDssiTUVIF0iK
Linked Issue
ready-to-specorready-to-implement.Testing
./script/formatRepository-prescribed split Clippy checks
cargo test -p warp_tui --lib autoupdate --features release_bundle,standalone,crash_reporting— 17 passedWindows tests retain Authenticode rejection, installed-payload validation, custom-root arguments, current/previous behavior, rollback retention, and live-version garbage collection
Native Windows and cross-repository release validation will be linked after the remote branches are exercised
I have manually tested my changes locally with
./script/runAgent Mode
CHANGELOG-NONE
Co-Authored-By: Warp agent@warp.dev