Repository navigation
feat(update): route install through the recovery journal gate (W3-02e) - #411
Conversation
install_update_when_idle now opens the update journal, re-observes the session freeze, write block and drain, and runs update.install only inside RecoveryDriver after begin_install persisted. Missing drain or backup evidence never reaches the installer. Also: keep expected/actual SHA in the swapped-installer error and correct the VerifiedImage hold comment. Test-First: src-tauri/src/delivery_recovery/install.rs::delivery_recovery::install::tests::production_install_never_reaches_the_installer_without_evidence Test-First: src-tauri/src/delivery_recovery/install.rs::delivery_recovery::install::tests::production_install_persists_installing_then_runs_the_installer_once Test-First: src-tauri/src/delivery_recovery/installer.rs::delivery_recovery::installer::tests::swapped_installer_error_names_expected_and_actual_digest Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
No-Test: allow(dead_code) attribute only, no behavior change 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: f97a2ee93f
ℹ️ 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".
| fn install(&mut self, _: &FileIdentity, _: &StagedManifestIdentity) -> Fx<()> { | ||
| (self.0.take().ok_or("installer already consumed")?)() |
There was a problem hiding this comment.
When a BackupVerified journal was prepared for candidate A but the supplied updater resource represents candidate B—for example, after the update feed changes—the driver passes A's identities here, but this implementation discards both and invokes the unbound closure, which installs B's downloaded bytes. The journal then records Installing and performs later validation/recovery using A's identity even though B was installed; compare the updater version and downloaded-byte digest with candidate and staged_manifest before advancing the journal.
Useful? React with 👍 / 👎.
| .install_when_idle(|| update.install(&bytes).map_err(|error| error.to_string())) | ||
| delivery_recovery::install_through_journal( | ||
| &app.state::<PtyManager>(), | ||
| app.state::<Store>().is_maintenance_active(), |
There was a problem hiding this comment.
When leave_maintenance runs concurrently after this boolean is sampled, it can roll back the store's BEGIN IMMEDIATE while installation remains authorized. install_in_maintenance cannot close this race because it checks only the PTY flag, and leave_database_maintenance releases the store before clearing that flag; it can also release the store after the installer has begun. This permits database writes during an update despite the required live write block, so installation and leaving maintenance need shared serialization or an owned maintenance guard held for the entire installer effect.
Useful? React with 👍 / 👎.
| match journal.next_action() { | ||
| RecoveryAction::InstallCandidate { .. } => Ok(()), |
There was a problem hiding this comment.
When a journal reaches BackupVerified and the process then restarts or leaves maintenance before installing, later writes can change projecta.db before maintenance is entered again. This branch accepts that old journal solely from its phase plus current boolean/PTY state; it never compares the currently quiesced database path and digest, or a maintenance generation, with the stored drain and backup evidence. Installation can therefore proceed with a stale backup, and recovery may discard writes accepted after that backup was created; bind the evidence to the current maintenance interval and re-observe the live database identity before accepting it.
Useful? React with 👍 / 👎.
| if !store_write_block { | ||
| return Err("write block is not active; refusing to install".into()); |
There was a problem hiding this comment.
These new refusal paths return ad hoc text such as write block is not active rather than the shared ERR_REFUSED prefix required by docs/architecture-rules.md rule 5. Callers that classify named refusals by the repository-wide prefix will treat every missing-evidence failure from this gate as an unclassified transport error, so all refusal branches in require_install_evidence should use the shared vocabulary.
AGENTS.md reference: AGENTS.md:L306-L309
Useful? React with 👍 / 👎.
| match journal.next_action() { | ||
| RecoveryAction::InstallCandidate { .. } => Ok(()), |
There was a problem hiding this comment.
When the snapshot recorded by VerifiedBackup is deleted, truncated, replaced, or aliases the live database after the journal entered BackupVerified, this phase-only check still authorizes the destructive install. The persisted record proves what was observed earlier but does not ensure that recoverable bytes still exist now, so an installation failure can leave the recovery action pointing at a missing or corrupt snapshot. Reopen the recorded snapshot, ensure it is distinct from the live database, and verify its current digest while entering Installing.
Useful? React with 👍 / 👎.
|
Queued — the merge queue status continues in this comment ↓. |
Cuarroc
added
the
review-ok
|
@Mergifyio queue |
Cuarroc
added
do-not-merge
|
@Mergifyio dequeue |
Merge Queue Status
This pull request spent 35 seconds in the queue, with no time running CI. ReasonPull request #411 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 ↓. |
☑️ The pull request is not queued |
Cuarroc
removed
dequeued
do-not-merge
Cuarroc
added
the
review-ok
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 13 minutes 41 seconds in the queue, including 13 minutes 21 seconds running CI. Required conditions to merge
|
Der Update-Befehl startet den Installer jetzt nur noch über das Wiederherstellungs-Journal. Fehlt der Nachweis für geleerte Sitzungen, gesperrte Schreibzugriffe oder ein geprüftes Backup, kommt der Installer nie zum Zug. Das echte Herunterladen und Installieren kann nur am PC geprüft werden.
Report
What changed
delivery_recovery/install.rs(new):require_install_evidencere-observes write block, session freeze, empty registry and journal phaseBackupVerified;install_through_journalrunsupdate.installonly insideRecoveryDriver::step(afterbegin_installpersistedInstalling), once.pty.rs:install_in_maintenance(requires the maintenance freeze, shares the latch withinstall_when_idle),is_maintenance_active.install_when_idleis now test-only (allow(dead_code)).main.rsinstall_update_when_idle: opensupdate-recovery.json, checks evidence before the download, installs via the gate.delivery_recovery/installer.rs:121now states the real lifetime (until the installer exits). F2RejectionReason::Swapped/HashMismatchcarry expected and actual SHA, which reach therun_installererror (new test).src-tauri/src/installer.rsdoes not exist; the adapter isdelivery_recovery/installer.rs.Evidence (base origin/main a4a1b95; head f97a2ee; diff 281+/17-)
bash scripts/ci/gates.sh lane prepush-> all gates green (rust-suite, clippy, fe-test, red-first-plan), log /tmp/w3-02e-prepush.log; push hook passed;git ls-remote= f97a2ee.bash scripts/ci/red-first.sh --plan-> 0 (3 specs).bash scripts/ci/gates.sh lane prepush-> exit 1 attypecheckbecausenode_moduleswas absent in the newly created worktree; no candidate test failed.npm ci:bash scripts/ci/gates.sh --from typecheck lane prepush-> exit 0 (typecheck, lint, 485 frontend tests, 585 HQ/script tests, clippy, 1,838 Rust tests; 17 skipped).production_install_never_reaches_the_installer_without_evidence,production_install_persists_installing_then_runs_the_installer_once,swapped_installer_error_names_expected_and_actual_digest.Open point (behavior): nothing yet produces the journal with drain proof and backup (
update-recovery.json), so until that producer lands (follow-up package) the command refuses with a named reason. That is the intended fail-closed result of this cut, but updates in a shipped build stay blocked until then.Architecture drift: The review found that the new
InstallOnlyadapter discards the existingRecoveryEffects::install(candidate, staged_manifest)identity contract instead of using it. This is recorded as C1 below and remains open.NICHT ABGEDECKT
#[cfg(windows)]incl.ProcessLauncher, the F1 comment's code); merge-queue Windows lane.Review disposition
Round 1, candidate
f97a2ee93f4ee56d041875876899b91c6052c802. Author family: Claude. Reviewers: Codex (this review session; the served model identifier is not exposed as runtime evidence) and Ollama Cloudglm-5.2:cloud(reported modelglm-5.2, transport exit 0, 2026-10-04T23:59:07Z). FINDEN and GEGENPRÜFEN were performed againstorigin/main...HEAD; no style-only findings are recorded.src-tauri/src/delivery_recovery/install.rs:73, existing contractsrc-tauri/src/delivery_recovery/driver.rs:77-86: deferred to release blocker W3-02h.InstallOnly::installdiscards bothcandidateandstaged_manifest;main.rs:1213then installs the separately downloaded updater bytes. Therefore a journal for candidate A can authorize the signed updater object for candidate B. Signature verification proves publisher authenticity, while the journal CAS proves only the phase; neither binds the installed bytes/version tosigned_artifact_sha256andcandidate_version. Countercheck: no later comparison exists, and the journal's validation/promotion path assumes the ignored identity. Decision: open, not safely fixable in this package → orchestrator decision 05.10. (advisor Fable 5.1): deferred, resolved for this package as release-blocker follow-up W3-02h — the candidate is already 298/300 changed lines; binding version and bytes to the journal requires a red regression test plus code and must be split or the package recut.src-tauri/src/main.rs:1209-1213, conflicting pathsrc-tauri/src/main.rs:1153-1158: deferred to release blocker W3-02i. The install gate receivesStore::is_maintenance_active()as a boolean snapshot. Concurrentleave_maintenancecan roll back the SQLite write lock after that snapshot and before/duringupdate.install; the PTY latch serializes only the session registry and does not hold the Store maintenance lock. Countercheck: the post-download live check still has the same check/use window, and no shared command gate or Store lease spans the installer. Decision: open, not safely fixable in this package → orchestrator decision 05.10. (advisor Fable 5.1): deferred, resolved for this package as release-blocker follow-up W3-02i — a shared maintenance lease/gate and a red concurrency regression test exceed the remaining two-line package budget and touch a safety seam.src-tauri/src/pty.rs:693-696: rejected. This requested a direct test of the defensive(true, false)branch but showed no incorrect behavior. The production precheck rejects the ordinary path; the second check under the PTY registry lock is deliberately the race-closing defense. C2 records the separate Store race.src-tauri/src/main.rs:1186-1203: rejected. The pre-download journal check is an early refusal, not the authorization point.install_through_journalchecks again andDurableJournal::compare_and_writerefuses a stale revision before invoking the effect;leave_maintenancedoes not mutate the journal.src-tauri/src/delivery_recovery/install.rs:58: rejected.RecoveryErrorhas a completefmt::Displayimplementation atsrc-tauri/src/delivery_recovery.rs:947-975;Effect(String)preserves the adapter reason. The claimed possible missing/minimal display path does not exist.No code was changed during review, so no delta review was run. A second round is required only after fixes for C1/C2 and must review only that fix delta.
Reviewer verdict (round 1) was BLOCKIERT: C1 binds no installed candidate identity to the journal, and C2 allows the Store write block to be released concurrently with installation.
Orchestrator decision (05.10.)
Merged with C1/C2 deferred, on advice of Fable 5.1 (GPT-6 Astra unavailable: Codex weekly limit). Reason: origin/main installs with no journal, no backup evidence and no store write block (
install_update_when_idle, main.rs ~:1182-1200), so this gate is strictly fail-closed relative to main; C1/C2 are incompleteness of the new gate, not regressions. Known consequence: until a journal producer exists, in-app self-update always refuses. Release blockers before the v1.5.0-beta tag, serial in lane mn: W3-02g journal producer, W3-02h (= C1, bind digest/version), W3-02i (= C2, maintenance lease).🤖 Generated with Claude Code