fix(update): refresh brew tap before upgrade and verify result - #87
Conversation
`pythinker update` on a Homebrew install ran `brew upgrade pythinker-code`
against the locally-cloned tap. A stale clone pins the old formula, so the
upgrade silently no-ops ("0.37.0 already installed") while the updater — which
only checked the subprocess exit code — still printed "Updated successfully!".
- Run `brew update --quiet` to refresh the tap before `brew upgrade`, so a
stale clone can no longer pin the old version. Best-effort: a failed refresh
does not block the upgrade attempt.
- After a brew upgrade exits 0, re-resolve the installed version via
`brew list --versions` (the running process's own importlib.metadata cannot
observe an in-place upgrade) and report a clear failure instead of success
when the version did not advance to the target.
Both paths are Homebrew-gated; uv/pip/pipx upgrades are unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds Homebrew updater correctness checks. A new helper extracts the installed version from ChangesHomebrew Updater Correctness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
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 `@src/pythinker_code/ui/shell/update.py`:
- Around line 661-669: The subprocess.run call that invokes ["brew", "list",
"--versions", "pythinker-code"] uses an excessive timeout and omits an explicit
check flag; change the call in update.py that assigns to result (the
subprocess.run(...) call using get_clean_env()) to use timeout=5 and add
check=False so non-zero returncodes are handled via result.returncode instead of
raising an exception.
🪄 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 Plus
Run ID: 70f4c53f-06fe-49f7-a131-3bc9c7b5be5c
📒 Files selected for processing (3)
CHANGELOG.mdsrc/pythinker_code/ui/shell/update.pytests/ui_and_conv/test_shell_update.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
`_installed_homebrew_version` used a 60s timeout for the local `brew list --versions` query; the file's other local package-state queries (dpkg-query, rpm) use 5s. A local DB read never needs 60s, and a shorter timeout fails fast if brew hangs. Addresses CodeRabbit review.
Problem
On a Homebrew install,
pythinker updateprints a contradictory result:The update both fails to install and falsely reports success.
Root cause
Two independent issues, confirmed empirically (local tap HEAD was one commit behind the upstream
homebrew-pythinkertap; an explicitbrew updateflipped the local formula0.37.0 → 0.38.0and the upgrade then worked):brew upgrade <formula>resolves against the locally-cloned tap formula. A Homebrew tap only advances onbrew update; the updater never ran it, so a stale clone pinned0.37.0and the upgrade no-op'd. (brew upgrade's implicit auto-update refreshed only the core/cask JSON API, not the third-party git tap.)do_updatetreatedreturncode == 0as success. A no-opbrew upgradeexits0, so it printed "Updated successfully!" without ever checking that the installed version advanced.It's compounded by the existing readiness gate reading the remote formula (sees
0.38.0✓) whilebrew upgradereads the stale local tap — two different sources of truth.Changes (
src/pythinker_code/ui/shell/update.py, Homebrew-gated)brew update --quietbeforebrew upgradeso a stale clone can't pin the old version. Best-effort — a failed refresh logs a warning and still attempts the upgrade (the version check below catches a no-op).--quietavoids dumping the host's full outdated-formula list._installed_homebrew_version()re-resolves the installed version viabrew list --versionsafter a0exit (the running process's ownimportlib.metadatacan't observe an in-place upgrade). Only prints "Updated successfully!" when it reached the target; otherwise returnsFAILEDwith a clear, actionable message.Scope
Both paths are Homebrew-only. The uv/pip/pipx paths still trust exit code
0— PyPI is authoritative with no stale local clone, so a false-positive is far less likely. Deliberate boundary, not an oversight.Testing
tests/ui_and_conv/test_shell_update.py(TDD, RED verified first): refresh-ordering, no-op→FAILED, refresh-failure-is-non-fatal, version parsing (max-of-multiple-kegs / non-zero exit →None).ruff check,ruff format --check, and the type checker are clean.brew update→brew upgradeadvanced0.37.0 → 0.38.0). The patcheddo_updatewas not exercised against a live stale-tap scenario because the local tap is now current.Summary by CodeRabbit