fix(update): stage native updates on exit + move notice below input - #172
Merged
Conversation
Two separate update-flow bugs reported together.
1. The "Updated → vX. Restart to apply." notice rendered inside the prompt
input box (between the input line and the footer separator). It now renders
below the separator rule, underneath the box. Helper renamed
`_prepend_update_notice` → `_append_update_notice` and both toolbar call
sites place it after the separator.
2. Logging into ChatGPT after a silent auto-update crashed with
`zlib.error: incorrect header check`. Root cause: the silent updater's
`_install_native_archive` did `os.replace` over `sys.executable`, overwriting
the running PyInstaller onefile bundle in place. This build reads its Python
archive lazily from the exe path, so the first not-yet-loaded import after the
swap (llm.py `openai_codex` branch) read a stale archive and died.
The native update now stages the new binary beside the running exe
(`.{exe}.staged`) and promotes it via `os.replace` at process exit (atexit),
so it goes live on the next launch — matching the restart notice. The smoke
check validates the staged binary; a binary that fails smoke is discarded,
never promoted. A boundary guard converts any residual post-update archive
corruption into a clean "restart to apply" message instead of a fatal
traceback.
Verified: make check-pythinker-code (ruff + pyright + ty) green; full test
suite 6388 passed, 7 skipped, 1 xfailed.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughNative self-updates now stage the downloaded binary beside the running executable and promote it via ChangesNative update staging and corruption guard
Update notice repositioning below prompt separator
Sequence Diagram(s)N/A — the changes compose existing APIs (staging, promotion, orchestration, CLI error handling) with no novel multi-component flows that would benefit from visualization beyond the checkpoints already shown in the hidden stack. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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)
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: 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_orchestrator.py (1)
377-389:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSmoke-check failure is still reported as
UPDATED.When smoke-check fails, the staged binary is discarded, but
reported_resultandfinal_statestay
derived fromUpdateResult.UPDATED. That can persist a success status and return success even though no
update will be applied.Suggested fix
reported_result = result final_state = _result_state(result) message = result.name.replace("_", " ").lower() if result is UpdateResult.UPDATED and not check_only: smoke_ok, smoke_message = run_post_install_smoke_check() append_update_log(smoke_message) if smoke_ok: message = smoke_message _write_last_success(job_id=job_id, message=message) _finalize_native_staging(promote=True) else: message = f"{SMOKE_CHECK_FAILED_PREFIX}{smoke_message}" # Never promote a staged binary that can't even print --version. _finalize_native_staging(promote=False) + reported_result = UpdateResult.FAILED + final_state = UpdateJobState.FAILEDAs per coding guidelines, “Never return success … after a required internal step failed.”
🤖 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_orchestrator.py` around lines 377 - 389, When the smoke check fails (smoke_ok is False in the else block), the final_state variable still reflects UpdateResult.UPDATED (success) because it was set before the smoke check ran. Update the final_state variable in the else block where smoke_ok is False to reflect a failure state instead of keeping it as the success state derived from the original UpdateResult.UPDATED result. This ensures the function returns the correct failure status when a required internal step (the smoke check) fails.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 `@src/pythinker_code/cli/__init__.py`:
- Around line 360-363: The corruption classifier in the block checking for
zlib.error is too broad and will match any zlib.error instance, but the
documented signature for post-update corruption is specifically "zlib.error:
incorrect header check". Narrow the condition that checks if cls.__module__ ==
"zlib" and cls.__name__ == "error" to also validate that the error message
contains "incorrect header check". This ensures only the known bundle corruption
case is classified as such, preventing misclassification of other zlib
decompression failures.
---
Outside diff comments:
In `@src/pythinker_code/ui/shell/update_orchestrator.py`:
- Around line 377-389: When the smoke check fails (smoke_ok is False in the else
block), the final_state variable still reflects UpdateResult.UPDATED (success)
because it was set before the smoke check ran. Update the final_state variable
in the else block where smoke_ok is False to reflect a failure state instead of
keeping it as the success state derived from the original UpdateResult.UPDATED
result. This ensures the function returns the correct failure status when a
required internal step (the smoke check) fails.
🪄 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: d5bd1748-551c-4230-aa3f-084fd3df3636
📒 Files selected for processing (10)
CHANGELOG.mdsrc/pythinker_code/cli/__init__.pysrc/pythinker_code/ui/shell/__init__.pysrc/pythinker_code/ui/shell/prompt.pysrc/pythinker_code/ui/shell/update.pysrc/pythinker_code/ui/shell/update_orchestrator.pytests/cli/test_post_update_corruption_guard.pytests/ui/test_update_native.pytests/ui_and_conv/test_prompt_tips.pytests/ui_and_conv/test_update_orchestrator.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…p flag - _is_post_update_bundle_corruption now also requires the documented "incorrect header check" message, so an unrelated zlib decompression failure on a frozen build isn't misclassified as a stale bundle and masked behind a restart-only message (CodeRabbit). - Drop the _staged_promotion_registered module global; register_staged_native _promotion now registers unconditionally and relies on the already-idempotent atexit handler (no-ops once the staged file is promoted), removing the unused- global finding (github-code-quality). - Tests updated/added: unrelated-zlib-message rejection; handler idempotency on double-run; registration registers the atexit handler.
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
Fixes two separate, user-reported update-flow bugs.
1. Update notice rendered inside the input box → now below it
The persistent "Updated → vX. Restart to apply." line was prepended above the footer separator rule, so it sat between the
❯input line and the box's bottom border — visually inside the input area. It now renders after the separator, underneath the box._prepend_update_notice→_append_update_notice; both toolbar call sites (card+pythinkerstyles) place it after the separator.2.
zlib.error: incorrect header checkafter ChatGPT loginRoot cause (confirmed from the crash trace): the silent auto-updater's
_install_native_archivedidos.replace(...)oversys.executable, overwriting the running PyInstaller onefile bundle in place. This build reads its Python module archive lazily from the exe path, so the first not-yet-loaded import after the swap —llm.py:358, theopenai_codexlogin branch importingopenai_responses— read a stale archive and crashed fatally.Fix: the native update now stages the new binary beside the running exe (
.{exe}.staged) and promotes it viaos.replaceat process exit (atexit), so it goes live on the next launch — matching the "Restart to apply" notice. The post-install smoke check now validates the staged binary; a binary that fails smoke is discarded, never promoted. A narrow boundary guard (_is_post_update_bundle_corruption) converts any residual archive/zlib crash on a frozen build into a clean "restart to apply" message instead of a fatal traceback.Scope: limited to the native-archive path (the reported bug). Linux-package and pip in-place paths are unchanged.
Known residual (documented, not silent)
Promotion at
atexitleaves two far-smaller windows than the mid-session crash it replaces: aSIGKILLskipsatexit(promotion self-heals on the next clean exit), and a first-time lazy import during the rest of interpreter shutdown could read stale bytes. Fully closing both needs a detached post-exit swapper (the deferred seamless-relaunch design); noted in code.Testing
make check-pythinker-code(ruff + pyright + ty) — greentestssuite — 6388 passed, 7 skipped, 1 xfailedHonest status: root cause confirmed from the trace and verified by unit-tested mechanics; not yet reproduced end-to-end on a packaged native build (requires a live release + update).
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements