fix(update): stop reporting an update that never installed - #46
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesUpdate installation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UpdatePreflight
participant verifyInstalledVersion
participant NativeExecutable
participant InstallState
UpdatePreflight->>verifyInstalledVersion: verify expected version
verifyInstalledVersion->>NativeExecutable: run --version
NativeExecutable-->>verifyInstalledVersion: version output or probe error
verifyInstalledVersion-->>UpdatePreflight: verification result
UpdatePreflight->>InstallState: persist outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/pythinker-code/src/cli/update/preflight.ts`:
- Around line 603-607: Update installUpdate to return the successful
InstallVerification, then have the manual-install success path in
runUpdatePreflight persist verification.unverified alongside lastSuccess.
Preserve the existing failure behavior and add a regression test covering a
successful manual install with an unverified verification result.
In `@apps/pythinker-web/public/install.ps1`:
- Line 498: Update the installation progress flow around Write-MachineProgress
and Print-Done so the archive move continues reporting state=downloading; remove
the premature state=done emission and emit state=done only after Print-Done
succeeds, following checksum validation, extraction, executable replacement, and
PATH configuration.
🪄 Autofix
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: CHILL
Plan: Pro
Run ID: a11cd1f1-b3b7-460f-a6d4-ae38c6eb5f02
📒 Files selected for processing (16)
.changeset/doctor-native-install.md.changeset/update-verify-before-reporting-success.md.changeset/windows-installer-progress.md.changeset/windows-package-manager-updates.mdapps/pythinker-code/src/cli/sub/doctor.tsapps/pythinker-code/src/cli/update/install-state.tsapps/pythinker-code/src/cli/update/preflight.tsapps/pythinker-code/src/cli/update/source.tsapps/pythinker-code/src/cli/update/types.tsapps/pythinker-code/src/cli/update/verify-install.tsapps/pythinker-code/src/cli/version.tsapps/pythinker-code/test/cli/doctor.test.tsapps/pythinker-code/test/cli/update/preflight.test.tsapps/pythinker-code/test/cli/update/source.test.tsapps/pythinker-code/test/cli/update/verify-install.test.tsapps/pythinker-web/public/install.ps1
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/pythinker-code/src/cli/update/preflight.ts (1)
912-918: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease the lock when verification rejects.
finishawaitsverifyInstalledVersionbefore entering thetry/finallythat callslock.release(). If verification rejects, the finalizer exits without releasing the lock and can block updates for up to six hours. Move verification into the protected block or catch all verifier failures.🤖 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 `@apps/pythinker-code/src/cli/update/preflight.ts` around lines 912 - 918, Move the verifyInstalledVersion call and related verification handling into the try/finally protected by lock.release() in finish, or otherwise catch verifier failures, ensuring every rejection releases the lock while preserving the existing installed success condition.
🤖 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 `@apps/pythinker-code/src/cli/sub/upgrade.ts`:
- Line 51: Update handleUpgrade to inspect the InstallVerification returned by
UpgradeDeps.installUpdate and route any result with ok false through the
existing failure path using verification.reason, rather than recording success
and returning 0. Preserve the current successful-result behavior; alternatively,
narrow the dependency contract only if failed results are intentionally
impossible.
In `@apps/pythinker-code/test/cli/upgrade.test.ts`:
- Around line 223-231: Update the createDeps fixture in the upgrade test to
provide the existing darwin native manifest so handleUpgrade reaches
installUpdate; retain the unverified result and state-write assertions, and add
an assertion that installUpdate was called to verify the intended execution
path.
- Around line 226-230: Update handleUpgrade to branch on a resolved
InstallVerification result with ok: false, route that mismatch through
lastFailure, and preserve its reason instead of writing lastSuccess or emitting
upgrade_command_succeeded. Add a test in upgrade.test.ts using installUpdate to
return a failed verification and assert the failure state retains the declared
reason.
- Around line 235-239: Update the assertion for lastSuccess.unverified in the
upgrade test to compare against the complete expected fixture string rather than
using expect.stringContaining('ETIMEDOUT'). Preserve the full persisted
manual-install reason, including the executable path and explanatory details, so
the test fails if the recorded message is truncated.
---
Outside diff comments:
In `@apps/pythinker-code/src/cli/update/preflight.ts`:
- Around line 912-918: Move the verifyInstalledVersion call and related
verification handling into the try/finally protected by lock.release() in
finish, or otherwise catch verifier failures, ensuring every rejection releases
the lock while preserving the existing installed success condition.
🪄 Autofix
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: CHILL
Plan: Pro
Run ID: 77d39710-a527-43ea-b374-9cd206f7c5aa
📒 Files selected for processing (3)
apps/pythinker-code/src/cli/sub/upgrade.tsapps/pythinker-code/src/cli/update/preflight.tsapps/pythinker-code/test/cli/upgrade.test.ts
1 participant
Related Issue
No issue — reported directly on Windows. The problem is described below.
Problem
A Windows user on v0.12.0 saw
Updating to v0.13.1 / Installing in the background, then↑ v0.13.1 restart to applyunder the prompt. Restarting the terminal still gave v0.12.0, and theupdate log recorded the same install as succeeded several times over.
where.exe pythinkerput%LOCALAPPDATA%\Programs\Pythinker\pythinker.exefirst, and the published0.13.1 Windows binary is correct (its sha256 matches the channel manifest and it reports 0.13.1), so
the executable that ran was the one the installer targets and the advertised version was real. What
was wrong is that nothing ever checked. Investigating it found four defects:
lastSuccesswhen the installer exits 0. No step asks whether the binary that runs next is the target version,
so an installer that exits 0 without replacing anything advertises "restart to apply" forever, on
every launch.
doctorcrashes on every native install. It reports the package root, and a packaged binaryships no
package.json, so the command died withError: Could not locate package.json near …— exactly when a user needs it most. The samelookup sits on the launch path in install-source detection.
npm.cmd,pnpm.cmdandyarn.cmdarespawned directly, which Node ≥18.20/20.12 refuses (CVE-2024-27980) with
EINVAL. The same callfails in the npm-prefix lookup, so those installs also classify as
unsupported.install.ps1emits no progress.install.shwrites machine-readableprogress:lines onstderr and the parent renders them; the PowerShell installer wrote none, so the footer's
downloading state was unreachable on Windows and an update in flight looked identical to a wedged
one — the defect fix(update): make the update flow report the truth and stop wedging #38 set out to close, still open on one platform.
What changed
A success now means the new version runs. After an installer exits 0, the version is verified
against the artifact the installer replaced, and a mismatch is recorded as a failure carrying the
reason (
… still reports 0.12.0 (expected 0.13.1)) instead of a success. Onlynativeinstalls areverified, by probing
process.execPath --version: an npm global reinstall rewrites the directorythis process was loaded from, so nothing readable there proves what the next launch runs, and a
wrong answer would park a healthy version.
Verification fails open — a probe that times out (an antivirus scan on a fresh unsigned exe is
the realistic case), cannot run, or prints no version records the success anyway, with a note saying
why it is unproven.
doctorprints that note next to the recorded outcome, so the next report of"it says updated but it did not" is answerable in one command.
Windows package-manager shims run through the command interpreter.
cmd.exe /d /s /c npm.cmd …,spelled out as argv rather than
shell: true, so the exact command line is visible in the source andasserted in tests instead of being assembled by Node's string joining. Same fix in the npm-prefix
lookup that classifies the install source.
install.ps1speaks the progress protocol, mirroringinstall.sh:state=waitingwhile releaseassets are not up yet,
state=downloadingwith percent and byte counts (one line per second atmost),
state=done, and a singlestate=failedafter the last retry — not between attempts, whichwould drop the footer out of its downloading state and back into a failure it is about to recover
from.
doctorsurvives a native install, reporting the package root only when there is one, and thelaunch-path source detection classifies an unresolvable layout as
unsupportedrather than throwing.It also now prints the last recorded update success, which is what would have shown the original
problem immediately.
One scope decision worth flagging: the interactive
Updated … to Xmessage still prints unchangedwhen a native probe could not run. The mismatch case — the actual lie — throws and is reported as a
failure on both foreground paths; the unproven case only loses a line in a flow the user is watching,
and it is recorded in the install state either way.
Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update. The user-facing docs describe thecommands, not
doctor's runtime lines, and the update behaviour is unchanged when an installreally works.
Summary by CodeRabbit
Bug Fixes
New Features