Repository navigation
W3-02d: installer adapter (exit/UAC/sharing classification, no retry) - #348
Conversation
Runs exactly the verified staged installer bytes once through a launcher trait, classifies exit code, UAC refusal and sharing violation, and turns anything unproven into an error so the persisted Installing phase never leads to a second launch. Windows glue is a minimal proc::command launcher. Test-First: src-tauri/src/delivery_recovery/installer.rs::delivery_recovery::installer::tests::failed_or_unclear_start_keeps_installing_and_never_launches_twice Test-First: src-tauri/src/delivery_recovery/installer.rs::delivery_recovery::installer::tests::installer_swapped_after_verification_is_never_launched Test-First: src-tauri/src/delivery_recovery/installer.rs::delivery_recovery::installer::tests::exit_uac_and_sharing_codes_are_classified Test-First: src-tauri/src/delivery_recovery/installer.rs::delivery_recovery::installer::tests::launcher_receives_exactly_the_verified_installer_bytes Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb68a36574
ℹ️ 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".
| .and_then(|mut file| file.write_all(installer).and_then(|()| file.sync_all())); | ||
| if let Err(error) = written { | ||
| return LaunchResult::StartFailed(error.raw_os_error()); | ||
| } | ||
| match crate::proc::command(&self.target).status() { |
There was a problem hiding this comment.
Wenn ein anderer Prozess in das Zielverzeichnis schreiben kann, wird der Dateihandle nach sync_all geschlossen und die EXE anschließend erneut nur über ihren Pfad geöffnet. In diesem Fenster kann etwa ein gleichberechtigter Worker die geprüfte Kopie ersetzen oder verändern, sodass der privilegierte Recovery-Helper fremde, nie verifizierte Bytes ausführt. Halte die Datei deshalb mit einer Share-Mode-Sperre gegen Schreiben/Löschen bis nach der Prozesserzeugung offen und sichere zusätzlich das Zielverzeichnis beziehungsweise die ACL ab.
Useful? React with 👍 / 👎.
| if let Err(error) = written { | ||
| return LaunchResult::StartFailed(error.raw_os_error()); | ||
| } | ||
| match crate::proc::command(&self.target).status() { |
There was a problem hiding this comment.
Für das von ProjectA erzeugte NSIS-Artefakt startet dieser Aufruf die EXE ohne Argumente, obwohl tauri.conf.json den passiven Update-Modus konfiguriert. Die offizielle Tauri-Updater-Implementierung übergibt dafür /P, zwingend /UPDATE sowie gegebenenfalls /R /ARGS und beendet danach den alten Prozess. Ohne diese Parameter läuft der normale Installerpfad statt des vorgesehenen passiven Update-/Relaunch-Protokolls; der Launcher muss die äquivalenten NSIS-Argumente und die Beendigung des alten Prozesses sicherstellen.
Useful? React with 👍 / 👎.
| match crate::proc::command(&self.target).status() { | ||
| Ok(status) => status | ||
| .code() | ||
| .map_or(LaunchResult::NoExitCode, LaunchResult::Exited), | ||
| Err(error) => LaunchResult::StartFailed(error.raw_os_error()), |
There was a problem hiding this comment.
Wenn der Installer tatsächlich erhöhte Rechte verlangt und der Helper nicht bereits erhöht läuft, verwendet proc::command(...).status() den CreateProcess-Pfad: Dieser zeigt keinen UAC-Dialog, sondern liefert ERROR_ELEVATION_REQUIRED (740), das classify anschließend fälschlich als vom Nutzer abgelehnte Erhöhung meldet. Microsoft beschreibt, dass dafür ShellExecute beziehungsweise runas nötig ist, um den Dialog auszulösen (Microsoft Learn). In diesem Szenario kann der Nutzer die Installation derzeit gar nicht genehmigen.
Useful? React with 👍 / 👎.
| let outcome = staging | ||
| .install(|payload| classify(launcher.launch(&payload.installer))) |
There was a problem hiding this comment.
Bei einem echten Update liest VerifiedStaging::install Manifest, Installer, Programmdatei und die vollständige Datenbank nacheinander in vier Vec<u8> ein, obwohl diese Closure ausschließlich payload.installer verwendet; die übrigen großen Puffer bleiben zudem erhalten, solange der blockierende Prozesslauncher auf den Installer wartet. Eine gewachsene ProjectA-Datenbank kann dadurch den Recovery-Helper gerade während der Wartung aus dem Speicher drängen und das Journal in Installing zurücklassen. Prüfe die ungenutzten Artefakte streamend und behalte für diesen Adapter nur die Installer-Bytes im Speicher.
Useful? React with 👍 / 👎.
|
Queued — the merge queue status continues in this comment ↓. |
No-Test: append-only review handoff only
No-Test: generated HQ snapshot only
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 3 minutes 41 seconds in the queue, with no time running CI. ReasonThe pull request conflicts with the base branch HintYou should update or rebase your pull request. If you want to requeue this pull request, you can post a Requeued — the merge queue status continues in this comment ↓. |
No-Test: generated HQ snapshot only
|
@Mergifyio queue |
Pass the updater's passive NSIS arguments (/P /UPDATE), name Windows error 740 as ElevationRequired instead of a UAC refusal, and keep the written installer held via VerifiedImage until the process has started. Test-First: src-tauri/src/delivery_recovery/installer.rs::delivery_recovery::installer::tests::launcher_receives_the_updater_passive_nsis_arguments Test-First: src-tauri/src/delivery_recovery/installer.rs::delivery_recovery::installer::tests::elevation_required_is_named_and_not_reported_as_a_refusal Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
No-Test: generated HQ snapshot, no runtime code Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Coordinator disposition (round 2 complete, tier A with two vendors): Codex round 1 R-B1 (high) and R-B2 (medium) fixed in 8c963f7, Codex delta review DELTA-OK at 0182206; GLM 5.2 approve with conditions, no high finding. GLM F1-F4 (medium/low) accepted as follow-ups, not blocking: F1 comment wording (handle held until exit), F2 keep digest-mismatch detail, F3 confirmed by the Windows merge-queue lane, F4 ACTIVITY.md order (moot after CI-CONFLICT-01 #370). Removing do-not-merge. |
Merge Queue Status
This pull request spent 4 minutes 40 seconds in the queue, with no time running CI. ReasonPull request #348 has been dequeued Queue conditions are not satisfied:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Requeued — the merge queue status continues in this comment ↓. |
No-Test: generated HQ snapshot refreshed by merge hook
|
@Mergifyio queue |
No-Test: activity handoff entry only
No-Test: generated HQ snapshot refreshed by merge hook
|
@Mergifyio queue |
☑️ Command
|
Merge Queue Status
This pull request spent 15 minutes 17 seconds in the queue, including 13 minutes 33 seconds running CI. Required conditions to merge
|
Dieses Paket baut den Teil des Windows-Updaters, der den Installer genau einmal startet und das Ergebnis einordnet.
Der Branch wurde mit dem aktuellen
mainabgeglichen; die Installer-Logik blieb dabei unverändert.Die abschließende Linux-Prüfung ist grün, während die Windows-Ausführung weiterhin in der Merge-Queue offen ist.
Report
What changed
src-tauri/src/delivery_recovery/installer.rs: verified staging bytes are launched exactly once, exit/UAC/sharing results are classified, and unclear failures never trigger a retry.git merge origin/maincompleted with exit 0 and no conflict hunks; the merge added only current-main changes outside the installer adapter. The generated HQ snapshot was refreshed as required by the merge hook.Evidence
npm run dev:start-check-> exit 0 (12.2 GB free, one Cargo build at check time).bash scripts/ci/gates.sh lane precommit-> exit 0.bash scripts/ci/gates.sh lane prepush-> exit 0 (all 12 gates green; 80 frontend files / 475 tests passed; 1,761 Rust tests passed, 7 skipped).bash scripts/ci/red-first.sh --plan-> exit 0 (count=4,specs=4).git push -u origin claude/w3-02d-installer-adapter-> exit 0; the push hook also completed the prepush lane successfully.git ls-remote origin refs/heads/claude/w3-02d-installer-adapter-> exit 0; remote headf724e8f5f93ad7e4bff0dbd0f7b25c75d69e3343matches local HEAD.node scripts/ci/architecture-drift.mjs check-> exit 0 (no new findings; no allowlist entry added).Architecture drift
No new architecture-drift finding was reported after the merge. No allowlist entry was needed.
NICHT ABGEDECKT
#[cfg(windows)]ProcessLauncher; the Windows lane runs in the merge queue.Review disposition
Review: ausstehend (Stufe A), orchestrator-owned.
The previously recorded independent review remains applicable to the installer commit. Its open findings are unchanged: R-B1 (high, updater/UAC launch contract) and R-B2 (medium, held-image/path replacement contract). They are not guessed into this conflict-resolution package and remain open.
Queue disposition:
@Mergifyio queuerequested because the conflict resolution changed no code logic. The open review findings remain visible to the orchestrator.Open point: resolve R-B1 and R-B2 with Windows contract/race tests and the PC drill.
Reviewer 2: GLM 5.2 (observed model: glm-5.2, requested glm-5.2:cloud), candidate
0182206b8b24135ea64258257d66bff2ba48aa41src-tauri/src/delivery_recovery/installer.rs:102:heldremains alive throughCommand::status()until process exit, while the comment says it is held until process start. Disposition proposal: follow-up documentation fix; behavior is conservative and not a correctness bug.src-tauri/src/delivery_recovery/installer.rs:98: aVerifiedImage::openfailure becomesStartFailed(None)/Unclear, losing the digest-mismatch OS detail. Disposition proposal: follow-up diagnostic improvement; no-retry safety contract remains intact.src-tauri/src/delivery_recovery/installer.rs:83: the Windows-onlycrate::proc::commandpath could not be verified from the diff alone. Disposition proposal: follow-up confirmation in the Windows merge-queue lane; no confirmed defect from this review..pa/ACTIVITY.md:33: the new 20:36 entry appears before a later 19:37 entry, violating chronological append order. Disposition proposal: follow-up ordering correction; no runtime impact.Verdict: approve with conditions; no HIGH finding. Windows compilation and the listed follow-ups remain open.
Review fixes (round 1, Review B findings)
8c963f7:installer.rsnow launches the NSIS installer with/P /UPDATE, the argument settauri-plugin-updater2.13.0 builds forinstallMode: "passive"(src/updater.rsupdater_parameters,src/config.rsnsis_args; contract intauri.conf.jsonplugins.updater.windows.installMode). No/R//ARGSrelaunch (recovery validates the candidate itself). Error 740 is now its own outcomeElevationRequired(not "UAC refused"; only 1223 maps toUacRefused). The trait takes the args, soFakeProcessrecords them. Follow-up (not invented here): the updater usesShellExecuteW, which elevates via the installer manifest; this launcher uses CreateProcess, so an installer demanding elevation ends in 740. If recovery must elevate, a canonical elevation helper is needed (none exists in the repo).FILE_SHARE_READonly, then held through the existingprojecta_capture::windows_image::VerifiedImage::open(path, sha256)(re-hashed against the verified digest) until the process has started.launcher_receives_the_updater_passive_nsis_arguments,elevation_required_is_named_and_not_reported_as_a_refusal;exit_uac_and_sharing_codes_are_classifiedupdated for 740.bash scripts/ci/gates.sh lane prepushat final HEAD0182206(includes merge of origin/maind6c287d): exit 0;red-first.sh --planexit 0;cargo test delivery_recovery::installer6 passed. Fix diff insrc-tauri: +64/-10.#[cfg(windows)]ProcessLauncher(share modes,VerifiedImagehold, real 740) does not compile/run on Linux; the Windows lane runs in the merge queue. No real installer run.Round 2 (delta, Codex, 0182206)
8c963f7):run_installernow passes["/P", "/UPDATE"]to the launcher, andlauncher_receives_the_updater_passive_nsis_argumentsobserves the complete ordered argument vector. Windows error 740 now maps toElevationRequired, while only 1223 maps toUacRefused;elevation_required_is_named_and_not_reported_as_a_refusalobserves the user-facing distinction. Locked-source check:tauri-plugin-updater2.13.0 returns/Pfor passive mode insrc/config.rs:41-48and appends/UPDATEinsrc/updater.rs:982-990. Precision: those are exactly the two arguments only when restart-after-install is disabled and no extra installer arguments exist; the crate default enables restart and would additionally append/R /ARGS ...(src/updater.rs:228,993-1009). Recovery intentionally omits relaunch arguments because candidate validation/relaunch is owned by the recovery flow.8c963f7): after writing and syncing,ProcessLauncherreopens and re-hashes the installer throughVerifiedImage::open, whose live handle permits read sharing only and therefore denies write/delete sharing. Theheldvalue remains in scope acrossCommand::status()and is dropped only afterward, so it is held through process creation (and, more conservatively, until process exit).bash scripts/ci/red-first.sh --plan-> exit 0 (count=6,specs=6);CARGO_TARGET_DIR=<build-slot> CARGO_BUILD_JOBS=1 cargo test --manifest-path src-tauri/Cargo.toml delivery_recovery::installer -- --nocapture-> exit 0 (6 passed).#[cfg(windows)]launcher, native sharing behavior, real UAC interaction, or a real NSIS installer. The Windows lane runs in the merge queue; the real installer/UAC behavior remains for the PC drill.8c963f7; no additional code change was reviewed beyond the fix delta.Conflict resolution update
git merge origin/main-> exit 0; merge commite6e5c9cresolved PR W3-02d: installer adapter (exit/UAC/sharing classification, no retry) #348 against the currentorigin/main.bash scripts/ci/gates.sh lane prepush-> exit 0 after the merge.git push -u origin claude/w3-02d-installer-adapter-> exit 0;git ls-remoteconfirms the remote head ise6e5c9c9db7de806da00b431364fdf202a8bf96d.Queue disposition:
@Mergifyio queuerequested; no installer runtime logic changed during conflict resolution.Final handoff commit:
dab9f1drecords the required activity note; its precommit and prepush gates both exited 0.Final remote verification:
git ls-remote origin refs/heads/claude/w3-02d-installer-adapterreportsdab9f1d952dd25eba09a816134e26b902abda6c3.Final current-main merge:
git merge origin/main-> exit 0; merge commita0672d2included the main update that arrived during the first push.Generated HQ snapshot commit:
0bc4519; finalprepushand push-hook both exited 0.Final remote verification reports
0bc4519cdf29c1dae97ef37811cd31a072f5952a.Queue disposition:
@Mergifyio queuerequested after the current-main merge; installer runtime logic remained unchanged.