feat(update): stage auto-updates instead of installing mid-session - #212
Merged
Conversation
On Windows the silent startup auto-updater launched the Inno Setup installer inline with /CLOSEAPPLICATIONS, letting its Restart Manager force-close the running pythinker.exe and kill the active session. Redesign the update lifecycle around a typed UpdateIntent (CHECK / STAGE_FOR_RESTART / INSTALL / INSTALL_AND_EXIT) so background callers are type-unable to request an in-session install: - Background startup updates download, sha256-verify, and stage the Windows installer with an atomically written manifest; they can no longer spawn installers, run package-manager upgrades, or raise SystemExit (contained in _run_silent_update_job; the done-callback stays as defense in depth). - A pre-session bootstrap in the CLI entry applies a verified staged update before any config/session/runtime construction and fails closed (discard + continue) on any invalid or stale stage; apply re-verifies the digest and version and guards against a concurrently superseded manifest. - In-shell /update stages on Windows (restart-to-apply notice); the standalone `pythinker update` CLI keeps its install-and-exit behavior as an explicit foreground operation. - config auto_update becomes a policy enum off|notify|download| apply_on_exit (default download) with legacy bool compatibility (true->download, false->notify) including PYTHINKER_AUTO_UPDATE; PYTHINKER_CLI_NO_AUTO_UPDATE stays the highest-precedence kill switch. /update auto, the settings panel, and pythinker info are mode-aware (info JSON auto_update_config is now a string). - The post-install smoke check is skipped for the Windows staged path: it would run the old executable and falsely certify the stage, which is instead digest-verified at staging and again at apply.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAuto-update configuration now uses ChangesAuto-update policy and interaction surfaces
Intent-based update execution
Windows staging and startup application
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/pythinker_code/ui/shell/update_orchestrator.py`:
- Around line 410-419: Update the failure branch following
run_post_install_smoke_check in the update orchestration flow so a failed smoke
check records and returns FAILED (or the established verification-failed result)
instead of retaining successful reported_result and final_state values. Keep
staging unpromoted, and update the corresponding test expectation to assert the
failure result.
In `@src/pythinker_code/ui/shell/update.py`:
- Around line 1281-1342: Update apply_windows_staged_update_now to atomically
rename/claim the staged manifest before reading or validating it, returning
False when another startup has already claimed it. Ensure all validation, launch
failure cleanup, and post-launch cleanup operate only on the claimed manifest
and its installer directory, so a newer published manifest is never deleted; use
an identity-specific claim rather than comparing versions alone. Add tests
covering duplicate concurrent applies and failure while a newer stage supersedes
the claimed one.
In `@tests/ui_and_conv/test_shell_update.py`:
- Around line 32-37: Add assertions to both fake_do_update mocks in
tests/ui_and_conv/test_shell_update.py at lines 32-37 and 861-865: require
intent to be update.UpdateIntent.INSTALL_AND_EXIT at the first site and
update.UpdateIntent.INSTALL at the second, while preserving the existing
print_output and call-recording checks.
In `@tests/ui/test_update_staging.py`:
- Around line 103-123: Update test_windows_install_and_exit_launches_installer
to mock the detached process-spawn boundary rather than _run_native_installer,
then assert that _maybe_run_native_update with UpdateIntent.INSTALL_AND_EXIT
raises SystemExit with code 0 while still verifying the installer was launched.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e790eed1-e1d5-4751-88f6-a585a0ccd0aa
⛔ Files ignored due to path filters (1)
tasks/todo.mdis excluded by!tasks/**
📒 Files selected for processing (20)
CHANGELOG.mdsrc/pythinker_code/cli/__init__.pysrc/pythinker_code/cli/info.pysrc/pythinker_code/cli/update.pysrc/pythinker_code/config.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/selectors/settings.pysrc/pythinker_code/ui/shell/slash.pysrc/pythinker_code/ui/shell/update.pysrc/pythinker_code/ui/shell/update_orchestrator.pysrc/pythinker_code/update_policy.pytests/cli/test_info.pytests/core/test_config.pytests/ui/test_update_staging.pytests/ui_and_conv/test_native_update_parity.pytests/ui_and_conv/test_settings_selector.pytests/ui_and_conv/test_shell_update.pytests/ui_and_conv/test_silent_auto_update.pytests/ui_and_conv/test_update_auto_slash.pytests/ui_and_conv/test_update_orchestrator.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Address code review findings on the Windows staged-update path: - Atomically claim the staged-update manifest (os.rename) before validating and applying it, so two Pythinker processes racing to apply the same stage (concurrent shell launches, or concurrent apply_on_exit sessions) can no longer both pass validation and spawn duplicate installers. The loser of the claim simply has nothing to apply. - test_shell_update.py: assert the exact UpdateIntent forwarded to do_update in the pre-start-prompt and native-installer-marker dispatch tests, so a future regression that routes the wrong intent fails loudly instead of silently passing. - test_update_staging.py: mock the detached-spawn boundary instead of _run_native_installer in the install-and-exit test, and assert the real SystemExit(0) — the mock was bypassing the exact behavior the test exists to protect. Add regression coverage for the concurrent claim race and for a failed claim not touching a manifest staged by another process in the meantime.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pythinker_code/ui/shell/update.py (1)
1251-1253: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRecover manifests stranded after claiming.
Once Line 1324 renames the canonical manifest, a process crash or the read failure at Lines 1251-1253 leaves only
.claimed-{pid}. Subsequent startups report no staged update, and PID reuse can make that stale destination block another claim on Windows.Use collision-resistant claim identities and recover/requeue orphaned claims without overwriting a newer canonical manifest.
As per coding guidelines, “Handle invalid, malformed, unauthorized, expired, timed-out, duplicated, concurrent, partial, cancelled, and retry-exhausted cases explicitly; never silently ignore unexpected states.”
Also applies to: 1310-1330
🤖 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/pythinker_code/ui/shell/update.py` around lines 1251 - 1253, Update the manifest claim/recovery flow around the staged-manifest read handling and the rename operation near the claim logic to use collision-resistant claim identities instead of PID-only names. On startup and after read failures, detect orphaned .claimed-* manifests, validate them, and requeue them to the canonical manifest only when no newer canonical manifest exists; handle malformed, unauthorized, expired, concurrent, and conflicting states explicitly with logging, never overwriting a newer manifest or silently ignoring unexpected states.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 `@tests/ui/test_update_staging.py`:
- Around line 329-344: Make both regression tests genuinely concurrent: in
tests/ui/test_update_staging.py lines 329-344, coordinate two callers so they
reach the claim operation simultaneously, then assert only one succeeds and one
installer is spawned; in tests/ui/test_update_staging.py lines 347-355, publish
a newer canonical stage after the original claim but before validation fails,
and assert the newer stage remains intact. Use the existing test helpers and
update staging symbols without changing the intended assertions beyond
exercising these race windows.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/update.py`:
- Around line 1251-1253: Update the manifest claim/recovery flow around the
staged-manifest read handling and the rename operation near the claim logic to
use collision-resistant claim identities instead of PID-only names. On startup
and after read failures, detect orphaned .claimed-* manifests, validate them,
and requeue them to the canonical manifest only when no newer canonical manifest
exists; handle malformed, unauthorized, expired, concurrent, and conflicting
states explicitly with logging, never overwriting a newer manifest or silently
ignoring unexpected states.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cdbfdaed-ba3d-4787-99e7-ae1cb8fcdf2e
📒 Files selected for processing (3)
src/pythinker_code/ui/shell/update.pytests/ui/test_update_staging.pytests/ui_and_conv/test_shell_update.py
The concurrent-apply and supersession-cleanup regression tests added in 70a4cc6 only called apply_windows_staged_update_now() sequentially or without a claim-time failure hook, so a future non-atomic check-then-rename regression could pass them undetected. - test_apply_now_concurrent_callers_only_one_wins now races two real threads through the claim via a threading.Barrier immediately before the call, instead of calling the function twice in sequence. - New test_apply_now_stale_claim_failure_preserves_concurrently_staged_newer_manifest publishes a newer canonical manifest from inside the digest-check hook (the failure mode that runs after a successful claim), and asserts it survives the claimed manifest's discard.
Contributor
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/ui/test_update_staging.py`:
- Around line 352-366: Bound synchronization in the test around the racer
function and thread joins: give barrier.wait a timeout and assert or otherwise
fail clearly if it expires, then join each worker with a timeout and verify no
thread remains alive. Preserve result collection while ensuring stalled workers
or deadlocks fail the test instead of hanging the 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1a0e1d5-6241-42a6-a228-ded22682d5dc
📒 Files selected for processing (1)
tests/ui/test_update_staging.py
Add a timeout to the barrier and thread joins in test_apply_now_concurrent_callers_only_one_wins, and use daemon threads, so a stalled worker or deadlock regression fails the test loudly instead of hanging the suite.
Contributor
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/ui/test_update_staging.py`:
- Around line 201-207: Update the test setup around read_claimed_update_only so
it no longer wraps read_windows_staged_update to assert a manifest argument.
Remove this private reader-signature coupling and assert the observable startup
apply outcome directly, leaving claimed-manifest behavior to the dedicated
concurrency tests.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20043e9e-4c27-4785-be5a-f02e14a148fd
📒 Files selected for processing (6)
src/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/update.pysrc/pythinker_code/ui/shell/update_orchestrator.pytests/ui/test_update_staging.pytests/ui_and_conv/test_silent_auto_update.pytests/ui_and_conv/test_update_orchestrator.py
…ED result Replace inferring verification failure from shared status-file state with an explicit UpdateResult.VERIFICATION_FAILED returned by the orchestrator, so the shell surfaces the toast from the job result and the CLI exits non-zero.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/ui_and_conv/test_update_orchestrator.py (2)
165-180: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winIsolate staged Windows state in the smoke-check test.
The orchestrator checks
_pending_windows_staged_update()before running the mocked smoke check. On Windows, a leftover manifest can bypass this test’s failure path. Patch that helper to returnNoneor isolate the staging directory.🤖 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/ui_and_conv/test_update_orchestrator.py` around lines 165 - 180, Update test_update_job_reports_failure_when_post_install_smoke_check_fails to isolate the staged Windows state by monkeypatching orchestrator._pending_windows_staged_update to return None, alongside the existing do_update and run_post_install_smoke_check patches, so leftover manifests cannot bypass the failure path.Source: Path instructions
170-173: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover intent forwarding in both test doubles.
Both fakes accept the new intent parameter without validating it, so incorrect intent propagation can go undetected.
tests/ui_and_conv/test_update_orchestrator.py#L170-L173: assertintent is update.UpdateIntent.INSTALL.tests/ui_and_conv/test_update_orchestrator.py#L321-L323: assert the expected native-update intent.🤖 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/ui_and_conv/test_update_orchestrator.py` around lines 170 - 173, Update both test doubles, fake_do_update at tests/ui_and_conv/test_update_orchestrator.py:170-173 and the native-update fake at tests/ui_and_conv/test_update_orchestrator.py:321-323, to assert the received intent matches the expected value: update.UpdateIntent.INSTALL for the first and the expected native-update intent for the second.Sources: Coding guidelines, Path instructions
tests/ui_and_conv/test_silent_auto_update.py (1)
342-350: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winExercise a nonzero
SystemExit.The test raises
SystemExit(0), which does not cover the failure exit code that could otherwise terminate the session. RaiseSystemExit(1)or parameterize both codes while retaining theNoneand error-log assertions.As per path instructions, tests must cover failure cases.
🤖 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/ui_and_conv/test_silent_auto_update.py` around lines 342 - 350, Update the exiting_job fixture used by shell._run_silent_update_job to raise a nonzero SystemExit, such as SystemExit(1), while preserving the assertions that the result is None and an exit-related error is logged; alternatively parameterize the test to cover both zero and nonzero exit codes.Source: Path instructions
🤖 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/ui_and_conv/test_silent_auto_update.py`:
- Around line 60-63: Update tests/ui_and_conv/test_silent_auto_update.py lines
60-63 to stop mocking the internal read_update_status function; instead provide
conflicting shared-status data and assert the observable behavior from the
returned silent auto-update result. At lines 83-83, remove or contradict the
matching status fixture so VERIFICATION_FAILED independently drives the expected
toast, ensuring both cases use the returned update result as the test oracle.
---
Outside diff comments:
In `@tests/ui_and_conv/test_silent_auto_update.py`:
- Around line 342-350: Update the exiting_job fixture used by
shell._run_silent_update_job to raise a nonzero SystemExit, such as
SystemExit(1), while preserving the assertions that the result is None and an
exit-related error is logged; alternatively parameterize the test to cover both
zero and nonzero exit codes.
In `@tests/ui_and_conv/test_update_orchestrator.py`:
- Around line 165-180: Update
test_update_job_reports_failure_when_post_install_smoke_check_fails to isolate
the staged Windows state by monkeypatching
orchestrator._pending_windows_staged_update to return None, alongside the
existing do_update and run_post_install_smoke_check patches, so leftover
manifests cannot bypass the failure path.
- Around line 170-173: Update both test doubles, fake_do_update at
tests/ui_and_conv/test_update_orchestrator.py:170-173 and the native-update fake
at tests/ui_and_conv/test_update_orchestrator.py:321-323, to assert the received
intent matches the expected value: update.UpdateIntent.INSTALL for the first and
the expected native-update intent for the second.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b57af41-a4ff-4ad3-a9f7-44dea4170940
📒 Files selected for processing (7)
src/pythinker_code/cli/update.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/update.pysrc/pythinker_code/ui/shell/update_orchestrator.pytests/cli/test_update_cli.pytests/ui_and_conv/test_silent_auto_update.pytests/ui_and_conv/test_update_orchestrator.py
…tice The post-install smoke check ran sys.executable for non-native installs; on Homebrew that exercises the still-running old keg, so it certified the pre-upgrade version right after an upgrade. Target the brew opt-linked launcher instead and assert the reported version matches the update target, reporting VERIFICATION_FAILED on mismatch. Derive the restart-to-apply notice from the recorded update-job status instead of the dismissal-filtered update cache, so dismissing a version's install prompt no longer hides the restart notice once that version is installed; ignore stale UPDATED statuses for versions not newer than the running one.
Drop the private reader-signature wrapper in the startup staged-apply test (claim behavior is covered by the dedicated concurrency tests) and replace status-read mocks in the silent auto-update tests with conflicting or absent status records, so the returned update result is the test oracle.
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.
Problem
On Windows, the silent startup auto-updater (
source="startup-auto") launched the Inno Setup installer inline, mid-session (ui/shell/update.py→_run_native_installerwith/SILENT /CLOSEAPPLICATIONS /PID=<pid>+sys.exit(0)). The shell swallows the SystemExit, the installer's 15sWaitForLauncherExittimes out, and Restart Manager force-closes the runningpythinker.exe— killing the user's active session mid-work. macOS/Linux had a stage-and-promote-at-exit path; Windows had none.Design
Invariant: no automatic update path may launch an installer, run a package-manager upgrade, or exit the process while a session is live.
UpdateIntent(CHECK | STAGE_FOR_RESTART | INSTALL | INSTALL_AND_EXIT) replaces thecheck_onlybool throughrun_update_job/do_update; background callers can only check or download-and-stage.os.replace); interrupted downloads never become ready; digest mismatch discards./updatestages on Windows with the existing restart-to-apply notice; standalonepythinker updatekeeps install-and-exit as an explicit foreground op.auto_updateconfig becomes a policy enumoff | notify | download (default) | apply_on_exit, with legacy bool compatibility (true→download,false→notify) incl.PYTHINKER_AUTO_UPDATE;PYTHINKER_CLI_NO_AUTO_UPDATEstays the hard kill switch./update auto, the settings panel, andpythinker infoare mode-aware._run_silent_update_job(on py3.14 a task-raised SystemExit kills the loop before any callback — proven empirically; the done-callback remains defense in depth).Verification
make check-pythinker-codegreen (ruff + format + pyright + ty).make test-pythinker-code: all update-related tests green incl. 18 new tests intests/ui/test_update_staging.py(manifest lifecycle/atomicity, intent gating, no-Popen/no-SystemExit regression, bootstrap fail-closed, supersession guard, staged-path smoke skip). One unrelated pre-existing flake:tests/e2e/test_shell_pty_e2e.py::test_shell_cancel_running_command_kills_process_and_recoversfails identically on cleanmainon this machine (PTY timing) and passed in an earlier full run./updatenot armingapply_on_exit, legacy-off doc contradiction, staged-path smoke false-certification) — all fixed with regression tests.Follow-up (out of scope)
packages/windows-installer/installer.isstuning (WaitForLauncherExit/CloseApplicationssemantics for the deliberate-shutdown spawn).Session.acquire_ownershipis an fcntl no-op on Windows (session.py);/CLOSEAPPLICATIONScan affect other live Pythinker processes.Compatibility notes
pythinker info --json:auto_update_configchanged from bool to the mode string./update auto offnow selects the fully-off policy; the old "no silent install, keep notices" behavior is the explicitnotifymode (and is what legacyauto_update = falseconfigs migrate to).Summary by CodeRabbit
/update autoto select and display the active policy mode.pythinker infoto report the configured policy mode.