feat(cli): nudge raven upgrade in the tui status bar when behind - #220
Conversation
0xKT
left a comment
There was a problem hiding this comment.
Review of ce56583, merged onto main 16719d0: clean merge, full unit suite green locally (4621 passed). Note the green checks on this PR predate #224, which switched CI to the full unit suite, so a rebase and re-run is needed regardless.
Items are ID severity. Each is independent; reply with the ID when addressed.
B1 blocking: unit tests now hit the GitHub API and write the real $HOME
- Where:
raven/cli/tui_commands.py:997-999,tests/test_cli_tui_commands.py:505 - What:
test_tui_announces_log_path_only_on_abnormal_exit(6 cases) mocksfind_node, so control flow reachesmaybe_refresh_async()with nothing stubbing it. Under a throwaway HOME those cases write$HOME/.raven/update_check.jsonwith a real fetchedlatest_version. All failures are swallowed, so the test stays green either way. Since #224 the CI unit job runs the full suite. - Fix: autouse fixture redirecting
update_notice._CACHE_PATHtotmp_path, mirroringtests/conftest.py:30-52formodel_catalog_cache._CACHE_PATH. That also isolatestests/test_tui_rpc_session.py, whosesession_create({})reads the real cache today. - Do not: patch
tui_commands.maybe_refresh_async. The import attui_commands.py:997is function-local, so there is no module attribute to replace.
B2 blocking: a valid-JSON, non-object cache file raises instead of being swallowed
- Where:
raven/cli/update_notice.py:37-40, raising at:77and:93 - What:
_read_cachereturns whateverjson.loadsyields with no dict check. A cache holding"0.2.0",[1]or42reachescache.get(...)and raisesAttributeErrorinsidetui()(raven tuifails to launch) and inside_default_session_info(session.createfails). Both reproduced. Reaching it takes a hand-edited file, which the "To see it locally" snippet invites, and the module contract is that the hint never breaks startup. - Fix:
isinstance(parsed, dict)guard in_read_cache.
S1 should-fix: the nudge points at a command that always fails for editable and pip installs
- Where:
raven/cli/update_notice.py:96-100vsraven/cli/upgrade_commands.py:440-449 - What:
raven upgraderaises for editable installs and non-uv-tool installs and exits 1, butupdate_notice()only compares versions, so those users get a permanent warn-colored nudge to a failing command. - Fix: gate the notice on
upgrade_commands._is_uv_tool_install()(:356), wrapped in try/except since_uv_tool_targetraisesUpgradeErroron a malformed receipt.
S2 should-fix: the failure path never backs off, and its comment says otherwise
- Where:
raven/cli/update_notice.py:56-65 - What:
_refreshwriteschecked_atonly on success, so while offline, rate-limited, or whenever the latest release is a draft or prerelease (_parse_release_payloadraises), every launch spawns a thread and refetches. The comment at:64claims "try again next TTL". - Fix: write
checked_aton failure too, keeping the previouslatest_version.
S3 should-fix: the cache path bypasses the cache-dir helper
- Where:
raven/cli/update_notice.py:24 - What: hardcodes
Path.home() / ".raven", so a non-default instance set viaset_config_path()writes back into the default home. - Fix:
paths.get_cache_dir()(raven/config/paths.py:37), the designated disposable-cache dir, with precedent atraven/token_wise/model_catalog_cache.py:39. It also removes the manualmkdir.
N1 minor: duplicate version parser
update_notice.py:27,30 reimplements upgrade_commands.py:21,178. The looser semantics (prefix match, return None) look deliberate, but the module already imports _fetch_latest_release from that file, so one shared lenient parser keeps a single source of truth.
N2 minor: dead and missing demo fixtures
ui-tui/src/demo/gallery.tsx:102-103 still seeds update_behind: 2 into the SessionPanel demo, dead now that the banner block is gone, while the StatusRule demos and stubGatewayFixtures.ts:32 never exercise the new nudge state.
N3 minor: no opt-out for the launch-time network call
The fetch sends the raven version in the User-Agent (upgrade_commands.py:234-239). An env-var escape hatch is conventional for update checks.
N4 minor: update_behind is a flag in a count-shaped field
Nothing on main ever populated it (stubGatewayFixtures.ts:32 is null), so renaming it to a boolean update_available is free now and not later.
Verified fine, no action: the status bar stays on one line at narrow widths (checked down to cols=44, where Yoga shrinks and truncates both slots), and the RPC server is only constructed inside the raven tui process, so the refresher and the reader always share a process.
The TUI already read update_behind / update_command out of the session init bundle, but nothing ever filled those fields, so the hint never appeared. Add raven/cli/update_notice.py: a cached check against the GitHub releases API. The live call is too slow for the session-create path, so `raven tui` refreshes ~/.raven/update_check.json in a daemon thread at most once a day and the gateway only reads that cache while building the info bundle. Every failure path is swallowed -- an update hint must never break startup. The cost is that a freshly published release surfaces on the next launch. Show it in the status bar's right slot, replacing the cwd/branch label, in the warn color. The old banner block is removed: the banner is the first thing on screen and an upgrade hint does not deserve that weight. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Review follow-ups on the startup nudge. Tests no longer touch the network or the real home: an autouse fixture redirects the cache path and sets RAVEN_NO_UPDATE_CHECK for the suite. Redirecting the path alone leaves the cache absent, which is exactly the state that spawns the fetch, so six tui cases fetched a release on every run since CI went to the full suite. A cache file holding valid JSON that is not an object no longer raises out of `raven tui` or session.create -- _read_cache rejects non-dicts. The nudge is gated on an upgradable install, so editable and non-uv-tool users stop being pointed at a command that always exits 1. A failed refresh now stamps checked_at too, so offline, rate-limited and draft-release launches back off for a full TTL instead of refetching every time. The cache moves to paths.get_cache_dir(), resolved per call so set_config_path() is honoured, and the duplicate version parser delegates to upgrade_commands. Also adds a RAVEN_NO_UPDATE_CHECK opt-out, renames update_behind to the boolean update_available it always was, and gives the demo gallery a StatusRule nudge state in place of the dead SessionPanel seed. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
ce56583 to
1c581e6
Compare
|
All nine items addressed, rebased onto B1 fixed -- unit tests no longer fetch or write the real home
The redirect alone is not enough. Under a temp path the cache is absent, which is exactly the state that spawns the fetch thread, so the six
B2 fixed -- non-object cache no longer raises
S1 fixed -- no nudge when
|
|
Re-reviewed 1c581e6 on the merged state. B2, S1, S2, S3, N1, N2, N3, N4 all verified fixed -- S1 checked against an editable checkout ( B1 is not closed yet, and there are two new minors. B1 still open blocking: the suite still fetches and still writes the real home
N5 minor: the refresh path is not gated the way the notice is
N6 minor: delegation tightened the version grammar
|
The non-object-cache test called maybe_refresh_async() bare. A non-object payload reads back as "no cache", which is the state that spawns the refresh thread, so each parametrized case fetched a release and wrote the result after monkeypatch had already restored the cache path -- landing on the real home rather than tmp_path. Running the file alone hid it: the process exits before the request completes. Stub the thread, since the assertion only needs "does not raise". Gate maybe_refresh_async on an upgradable install too. Editable and pip installs cannot act on the result, so they were paying for a fetch every TTL for a hint update_notice() already suppresses. Normalise a prerelease version to the release it builds on before the strict parser sees it. Delegating to upgrade_commands._version_key brought fullmatch semantics with it, which silently dropped the hint for anyone on an rc or dev build. The suffix is discarded rather than ordered, so an rc of the latest release compares equal to it and is not nagged. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
|
B1 closed
Reproduced the leak first, without touching the real cache, by standing in a second temp file for Then verified the fix the way you found it: deleted N5 done -- the refresh is gated like the notice
N6 -- deliberate call: normalise, do not orderYou are right that delegation tightened the grammar and that this deserves a decision rather than a side effect. Decision: a prerelease is reduced to the release it builds on before the single strict parser sees it, so The suffix is discarded rather than ordered: VerificationRan in a separate worktree with its own environment, since another change is in flight on this checkout. Note that a plain Still not verified, for the recordThe positive path -- a real uv-tool install with a newer release actually rendering the hint -- has only been exercised with Also worth noting: nothing asserts that |
0xKT
left a comment
There was a problem hiding this comment.
Verified 0f071fb on the merged state. All eleven items closed, each checked rather than taken on report:
- B1: full unit suite under a throwaway HOME,
4634 passed, and afterwards the cache directory does not exist at all. The previous revision left$HOME/.raven/cache/update_check.jsonthere with a real fetched version. - N5 doubles as a second line of defence: CI installs raven editable, so
_upgrade_command_works()is false and the refresh never spawns. - N6: swept the normaliser.
0.2.0rc1/0.1.9.dev1/0.1.9+local/v0.2.0all reduce correctly,garbage/1.2/.1.2all read as unparseable. Dropping the suffix instead of ordering it is the right call. - S1 checked against an editable checkout, N3 with
RAVEN_NO_UPDATE_CHECK=yes, and theupdate_availablefield name matches on both sides of the seam.
Approving. Three coverage gaps stay open by agreement; the maintainer is doing a test-organisation pass separately and these fold into it.
-
Nothing asserts the session bundle carries
update_available/update_command.tests/test_tui_rpc_session.pyis untouched by this PR, so deleting the two lines atsession.py:159or misspelling the key leaves the whole suite green and the TUI showing nothing, which is exactly the pre-PR state this change exists to fix. That seam has broken once already. -
Nothing asserts
raven tuistill callsmaybe_refresh_async(). One correction on the reasoning: a function-local import resolves the attribute on the source module at call time, somonkeypatch.setattr(update_notice, "maybe_refresh_async", ...)does observe the call. Verified with a throwaway test against the realtui_appinvocation; no production restructuring needed. -
The positive path has never run on a real uv-tool install, as you noted. Worth one manual look at some point, since every dev checkout is editable and S1 correctly suppresses the hint there, so nobody stumbles onto it by accident.
Summary
The TUI already read
update_behind/update_commandout of the session initbundle, but nothing on the Python side ever filled those fields, so the hint
never appeared. This wires up the missing half and moves where it shows.
The check
raven/cli/update_notice.pycompares the running version against a smallcache in
~/.raven/update_check.json:raven tuicallsmaybe_refresh_async()once per launch. It spawns a daemonthread only when the cache is missing or older than 24h, so a normal launch
touches the network at most once a day and never blocks startup.
(
_default_session_info). Reading is pure and fast; no network on thesession-create path, which is why the check is cached rather than live.
network error, rate limit. An update hint must never break startup.
The tradeoff is staleness by one launch: the first launch after a release
lands refreshes the cache, and the hint shows on the next launch.
Where it shows
The status bar's right slot, in the warn color, replacing the cwd/branch label
while an update is available. No extra line, no layout shift.
The old banner block in
branding.tsxis removed. The banner is the firstthing on screen at startup and an upgrade nudge does not deserve that weight.
update_behindis now a flag rather than a commit count, so the text isversion-agnostic (
Update available - run raven upgrade) instead of the oldN commits behind.Type
Verification
New tests cover the cache read/write and refresh throttle
(
tests/test_cli_update_notice.py), the status-bar slot swap(
ui-tui/src/__tests__/statusRule.test.tsx), and the banner no longerrendering the block (
branding.test.tsx).Also exercised the five edge cases by hand: absent cache, newer cached
release, cache equal to current, unparseable version string, and a corrupt
cache file. Only the second one returns a notice; the rest return
None.The lint warning reported on
appChrome.tsx(react-hooks/exhaustive-deps,line 275) is pre-existing on
mainand untouched by this change.Risk
The GitHub releases fetch reuses
upgrade_commands._fetch_latest_release, sono new network surface or credential handling. The cache file holds a version
string and a timestamp only.
Wire-compatible:
update_behind/update_commandwere already optionalfields in the info bundle, and a client that never receives them behaves as
before. Rollback is reverting this commit; the feature degrades to its current
state, which is showing nothing.
To see it locally:
Related Issues
N/A