Skip to content

fix(*): resolve the latest release without the github api quota - #299

Merged
0xKT merged 3 commits into
mainfrom
fix/upgrade_release_discovery_quota
Aug 13, 2026
Merged

fix(*): resolve the latest release without the github api quota#299
0xKT merged 3 commits into
mainfrom
fix/upgrade_release_discovery_quota

Conversation

@0xKT

@0xKT 0xKT commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Summary

Version discovery only ever called api.github.com, whose unauthenticated bucket is 60 requests per hour keyed on the source IP. A shared egress exhausts it for everyone behind that address, and both raven upgrade and the one-line installers then fail to resolve a version.

The release page carries no API quota, so its redirect now backs the API up: github.com/EverMind-AI/Raven/releases/latest names the latest stable tag, and the wheel URL is derived from the same shape the payload validator already enforced (now a single _release_wheel_url() used by both paths, so the rule cannot drift). The derived wheel is confirmed with a HEAD request, which replaces the "exactly one wheel asset" check the API payload provided.

Only transport, status and decoding failures fall back. No payload-level failure is routed around, because the release page cannot re-check what the payload carries - above all the draft and prerelease flags, where falling back would install exactly what the API rejected; a test asserts the release page is not requested in that case.

The same quota is also spent by Raven itself: update_notice._refresh() runs on every launch behind a 24h TTL and called the API-first path, so every install behind a shared egress spent one unauthenticated request per day. That is the traffic that empties the bucket. The notice needs a version string and nothing else, so it now calls fetch_latest_version(), which stops after the redirect - no API request, no wheel HEAD.

The error reporting was wrong in a second, independent way: Check your network and try again; if the problem persists, rerun the official installer. was appended to every failure, so a spent quota was reported as a network problem, and unrelated errors got the same sentence down to Editable Raven installations cannot be upgraded automatically. Pull the source checkout and rebuild Raven.. Check your network (note the double period). Release-lookup failures now raise ReleaseLookupError and report GitHub's quota facts plus the reset time, taken from the response that httpx keeps on the exception and that nothing read before. Network advice appears only when both paths failed at the transport layer, the installer hint stays on local-installation errors, and the double period is gone.

install.sh and install.ps1 get the same fallback and the same URL shape. install.sh also stops silencing the failure: the pipeline's exit status made set -eu blind to it, so an exhausted quota produced no diagnostic at all.

Deliberately out of scope: reading GITHUB_TOKEN / GH_TOKEN. It only helps callers that already have a token and does nothing for the users who hit this, so the error message does not advise setting one either.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

Reproduced without spending the real quota: a local HTTP server replays GitHub's spent-quota 403 (same body, same five x-ratelimit-* headers) and only the API endpoint is pointed at it, so every fallback below reached real GitHub.

  • uv run pytest tests/test_cli_upgrade_commands.py tests/test_cli_update_notice.py -q -> 138 passed.
  • make test-python -> 6146 passed, 33 skipped, 13 deselected (measured at the head of this branch, not carried over from an earlier commit).
  • Against real GitHub: fetch_latest_version() returns the current version in a single request, with no call to api.github.com.
  • install.sh's validation block, extracted by line range and run under sh, rejects .., 1..2, 1.2, 1.2.3.4, 0.01.2, a.b.c, an rc tag and an off-host redirect, and accepts 0.1.11 - matching the CLI and install.ps1.
  • uv run --extra dev ruff check raven/cli/upgrade_commands.py tests/test_cli_upgrade_commands.py -> All checks passed.
  • make lint-python -> All checks passed, 7 files already formatted.
  • sh -n install.sh -> clean.
  • Pre-fix code (loaded from origin/main) against the replayed 403 reproduces Client error '403 Forbidden' ... Check your network and try again; post-fix code resolves 0.1.11 through the release page and continues past version discovery.
  • Real CLI entry point with the API replaying 403: Raven 0.1.11 is up to date.; with both paths down: rate limit exhausted (unauthenticated requests share 60 per hour per IP), resetting at 22:24:19, with no network or installer advice.
  • install.sh resolution block (extracted by line range, not retyped) with the API replaying 403: falls back to the correct wheel URL; with both paths down it exits 1 with the new message and the curl error is now visible.
  • install.ps1 resolution functions under PowerShell 7.6.4: real lookup, 403 fallback and both-paths-down all behave as intended; a forced exception branch (403 carrying a Location header, the shape Windows PowerShell produces for an unfollowed redirect) still reads the tag.

Not verified: Windows PowerShell 5.1 itself, where $_.Exception.Response.Headers is a WebHeaderCollection and the code falls through to GetValues("Location"). That read is wrapped in try/catch and degrades to the explicit Fail message instead of today's unhandled exception, so the worst case is strictly better than current behaviour.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Risk

User-visible behaviour changes: raven upgrade and the installers now succeed in cases where they previously failed, and failure messages are reworded. The API remains the primary path for raven upgrade, so its happy path is unchanged. The background update notice no longer uses the API at all; it runs in a daemon thread whose failures are swallowed, so it is covered by tests rather than by a visible error. The two fallback requests use a 5s timeout, keeping the worst-case wait near what it was with one 10s request. The upgrade path is self-sealing (a user who cannot upgrade cannot receive the fix), which is why the API keeps its versioned contract and full payload validation instead of switching to the release page outright.

Security: the fallback derives the wheel URL from a hard-coded shape rather than trusting the redirect target, so a redirect can only influence the version string, which must match a strict three-part pattern. No credentials are read or sent.

Rollback: revert the commit. The installers keep their RAVEN_WHEEL_URL override in either direction.

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Related Issues

Fixes #298

Version discovery hit only api.github.com, whose unauthenticated bucket is 60
requests per hour keyed on the source IP, so a shared egress exhausts it for
everyone behind that address and both `raven upgrade` and the one-line
installers stop resolving a version. The release page carries no API quota, so
its redirect now backs the API up: it names the latest stable tag, and the
wheel URL is derived from the same shape the payload validator already enforces.

Only transport and status failures fall back. A payload that parsed as draft or
prerelease is never routed around, because the release page cannot re-check
those flags.

Report the failure honestly too. A spent quota reported "Check your network and
try again", a sentence appended unconditionally to every error, down to
"Editable Raven installations cannot be upgraded". Release-lookup failures now
raise ReleaseLookupError and report the quota facts with the reset time, the
network advice appears only when both paths failed at the transport layer, and
the installer hint stays on local-installation errors.

Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this change does

Version discovery keeps the GitHub API as its primary path and adds a quota-free fallback: the release page redirect names the tag, the wheel URL is derived from a hard-coded shape, and a HEAD confirms the wheel exists. Failure reporting is split so that a spent quota is no longer reported as a network problem, and the unconditional "Check your network" sentence (which produced ... rebuild Raven.. Check your network) is gone. install.sh and install.ps1 get the same fallback.

The direction is right and the security surface is held: the redirect can only influence a version string that has to pass a strict three-part pattern, and the download prefix is hard-coded.

What I checked and found sound

I pulled the branch into a worktree and probed the behaviour rather than reading only, so the following are verified, not assumed:

Suspicion Result
Does the fallback route around the prerelease guard? No. github.com/OWNER/REPO/releases/latest has the same non-draft / non-prerelease semantics as the API, so the fallback is in fact equally safe. The conservative stance in the PR is stricter than it needs to be, not weaker.
Can the redirect steer the wheel URL? No. has_redirect_location + startswith(RELEASE_TAG_PREFIX) + _VERSION_RE. I fed ../../evil, v1.2.3.4 and v01.2.3 through a mock transport: all rejected, and no HEAD is sent on rejection.
The owned client sets follow_redirects=True; is the release-page GET followed anyway? No. The explicit follow_redirects=False wins; the mock transport observed exactly one GET to the release page.
warn / Write-Warn are called from inside catch blocks. Do they exist? Yes, install.sh:25 and install.ps1:27. A missing helper there would fail a second time inside the handler.
Can ReleaseLookupError escape the background update check and break raven tui startup? No. update_notice._refresh() catches bare Exception.
Does the path work against real GitHub? Yes. releases/latest -> 302 to tag/v0.1.11; HEAD on the derived wheel follows to release-assets.githubusercontent.com and returns 200.
Test claim Reproduced: tests/test_cli_upgrade_commands.py + tests/test_cli_update_notice.py -> 129 passed.

Findings

Ten inline comments below. Confidence is marked on each; all of the Python-side ones were reproduced against the branch with a mock transport, and the install.sh one was reproduced by running the two case blocks in sh.

  1. raven/cli/update_notice.py:114-116 (high, root cause, no diff line to anchor to). The daily background version check calls _fetch_latest_release(), which is API-first. Every install behind a shared egress therefore spends an API request per day, plus one per raven upgrade. That is very likely what empties the 60/hour bucket in the first place. This PR makes the exhaustion survivable but does not slow it down. The notice only needs a version number: it needs neither the payload nor the wheel, so it could go straight to the release page and leave the API budget to the command that actually installs something. I would open this as a separate issue rather than widen this PR, but it is the actual fix for #298. Anchored on _fetch_latest_release since update_notice.py is not in this diff.
  2. API returns 200 with a non-JSON body: no fallback, and the message regresses to the shape this PR set out to remove. See inline on _fetch_latest_release_via_api.
  3. The fallback condition is broader than the PR describes. The description says draft/prerelease is never routed around; the code never routes around any payload-level failure. See inline on _resolve_latest_release.
  4. A 404 on the derived wheel is reported as release page: HTTP 404, when the release page returned a correct 302. See inline on the HEAD call.
  5. install.sh version validation has drifted from the Python and PowerShell shape checks, despite the description saying the rule cannot drift. Not exploitable; I checked. See the two inline comments on install.sh.
  6. Four smaller ones inline: timeout budget, secondary rate limit, rstrip('.'), missing User-Agent on the HEAD.

One coverage note with no line to anchor to: the install.sh and install.ps1 changes have no automated coverage at all. The repo has no installer tests today, so this is not a regression introduced here, but both platforms' fallback logic is now guarded only by manual verification.

Not verified

  • Windows PowerShell 5.1, as the PR already states. The WebHeaderCollection / GetValues("Location") path is wrapped in try/catch and degrades to Fail, so the worst case is better than today, but nobody has run it.
  • PowerShell 7's exact behaviour for -MaximumRedirection 0. Both branches are written, so it does not matter much.
  • The long-term stability of HEAD against release-assets.githubusercontent.com signed URLs. It works today and is now load-bearing for the fallback.
  • The claim in finding 1 that the background check is the dominant quota consumer is inferred from the code (one request per install per 24h TTL), not from telemetry.

Suggested handling

Findings 2 and 4 are small and land squarely on this PR's own subject (honest failure reporting); worth fixing in this round. Finding 3 needs either a code change or a description change so the two agree. Finding 1 deserves its own issue. Finding 5 and the small ones can follow.

Leaving this as a comment rather than a request-changes; the merge call is yours.

raise ReleaseLookupError(message) from web_error


def _fetch_latest_release(client: httpx.Client | None = None) -> ReleaseInfo:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High - root cause is upstream of this function. (Anchored here because raven/cli/update_notice.py is not in this diff.)

raven/cli/update_notice.py:114-116 calls this function from a daemon thread that raven tui starts on every launch (maybe_refresh_async, _REFRESH_TTL_SECONDS = 24 * 60 * 60). Because _fetch_latest_release is API-first, the sequence is:

  1. User launches raven tui.
  2. maybe_refresh_async() sees the cache is older than 24h and spawns _refresh().
  3. _refresh() calls _fetch_latest_release() -> one request to api.github.com.
  4. Repeat for every install behind the same egress IP, every day, on top of every raven upgrade.

That is the traffic that empties the 60/hour unauthenticated bucket. This PR makes exhaustion survivable but does not reduce the rate at which it happens.

Suggested direction: the update notice only needs a version string. It needs neither the release payload nor a wheel URL, so it does not need the API and does not need the HEAD either. Add a redirect-only helper (the first half of _fetch_latest_release_via_redirect, stopping before the wheel derivation) and have _refresh() call that, reserving _fetch_latest_release for raven upgrade. That leaves the API budget for the path that actually installs something, and drops Raven's own contribution to the shared-IP exhaustion to near zero.

I would not widen this PR to cover it. Worth a follow-up issue linked to #298.

Confidence: high on the mechanism (read from the code); the claim that this is the dominant consumer is inferred, not measured.

Comment thread raven/cli/upgrade_commands.py Outdated
}
response = client.get(LATEST_RELEASE_API, headers=headers)
response.raise_for_status()
return _parse_release_payload(response.json())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium - the fallback does not cover a 200 with a non-JSON body, and the resulting message is the exact shape this PR removes elsewhere.

response.json() raises json.JSONDecodeError, a ValueError. _resolve_latest_release catches only httpx.HTTPError, so this escapes the fallback entirely, and then lands in the generic handler in upgrade() (which does catch ValueError).

Reproduced on this branch with a mock transport returning 200 with body <html>blocked by proxy</html>:

JSONDecodeError: Expecting value: line 1 column 1 (char 0)
requests: ['GET https://api.github.com/repos/EverMind-AI/Raven/releases/latest']

No request to the release page. What the user sees:

Unable to upgrade Raven: Expecting value: line 1 column 1 (char 0). If the problem persists, rerun the official installer.

A raw JSON-parser message, plus advice to reinstall a healthy local installation - the same class of misreporting this PR fixes for the quota case. And a proxy or captive portal returning HTML with a 200 is precisely a remote failure the release-page fallback would recover from.

tests/test_cli_upgrade_commands.py:350 (test_fetch_latest_release_propagates_invalid_json) pins this with pytest.raises(ValueError), so it reads as deliberate rather than missed. If it is deliberate, it is worth a sentence in the PR description saying why a malformed body should be treated differently from a malformed status.

Suggested fix: wrap the decode so it becomes a fallback-eligible failure:

response.raise_for_status()
try:
    payload = response.json()
except ValueError as exc:
    raise httpx.DecodingError("GitHub API returned a non-JSON body", request=response.request) from exc
return _parse_release_payload(payload)

httpx.DecodingError is an httpx.HTTPError, so _resolve_latest_release picks it up and falls back; _github_failure_detail renders it via str(error). Then update the test to assert the fallback is taken.

Confidence: high, reproduced.

def _resolve_latest_release(client: httpx.Client) -> ReleaseInfo:
try:
return _fetch_latest_release_via_api(client)
except httpx.HTTPError as error:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium - the implemented rule is broader than the documented one.

The PR description and the comment just below say a payload that parsed as draft or prerelease is never routed around. What the code actually does is never route around any _parse_release_payload failure, because all of them are ReleaseLookupError and only httpx.HTTPError is caught here.

That sweeps in three failures that have nothing to do with the draft/prerelease argument:

  • Malformed GitHub release payload
  • Expected exactly one release wheel named ...
  • Untrusted Raven release wheel URL: ...

Reproduced with a mock returning a well-formed but assetless payload (the window where a release is published and its assets are still uploading):

ReleaseLookupError: Expected exactly one release wheel named raven-0.1.4-py3-none-any.whl
requests: ['GET https://api.github.com/repos/EverMind-AI/Raven/releases/latest']

No fallback. Yet the fallback path independently confirms the wheel with its own HEAD, so routing around a payload-integrity failure is safe in a way that routing around a stability flag is not.

Suggested fix - pick one:

  1. Split the exception so intent and code agree: keep a non-recoverable type for the stability check only (e.g. UnstableReleaseError(ReleaseLookupError) raised at the if draft or prerelease branch), and catch (httpx.HTTPError, ReleaseLookupError) here while letting UnstableReleaseError propagate. The comment then describes exactly what the code does.
  2. Or keep the current conservative behaviour and reword the description and the comment to say "any payload-level failure", so a reviewer is not told a narrower rule than the one being merged.

I lean towards 1: the payload-integrity cases are pure remote-data problems, and today they hard-fail an upgrade that the release page could have completed.

Confidence: high, reproduced.

Comment thread raven/cli/upgrade_commands.py Outdated

version = ".".join(str(part) for part in _version_key(location[len(RELEASE_TAG_PREFIX) :]))
wheel_url = _release_wheel_url(version)
client.head(wheel_url, follow_redirects=True).raise_for_status()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium - a wheel that 404s is blamed on the release page, which answered correctly. Plus a small inconsistency on the same line.

Failure sequence, reproduced with a mock (API 403 quota-spent, release page 302 to tag/v0.1.4, wheel HEAD -> 404):

  1. client.get(LATEST_RELEASE_WEB) returns 302 with a valid tag Location. The release page did its job.
  2. _version_key parses 0.1.4. Still fine.
  3. client.head(wheel_url).raise_for_status() raises HTTPStatusError 404.
  4. _resolve_latest_release catches it as web_error and renders release page: HTTP 404 Not Found.

Actual output:

could not resolve the latest Raven release (GitHub API: GitHub rate limit exhausted (...), resetting at 20:23:47; release page: HTTP 404 Not Found)

The release page returned 302, not 404. In a PR whose thesis is that failures should be reported for what they are, this is worth fixing. test_fetch_latest_release_rejects_release_page_without_a_wheel currently asserts the incorrect wording, so it needs updating with the fix.

Suggested fix: distinguish the two remote failures at the point they happen:

try:
    client.head(wheel_url, headers=headers, follow_redirects=True).raise_for_status()
except httpx.HTTPError as exc:
    raise ReleaseLookupError(
        f"release {version} has no wheel at the expected URL ({_github_failure_detail(exc)})"
    ) from exc

Also on this line (low): the HEAD is the only one of the three requests that does not carry the headers dict built two lines above, so it goes out with httpx's default python-httpx/x.y User-Agent while the other two identify as raven/<version>. Passing headers=headers (as in the snippet) fixes both at once.

Confidence: high, reproduced.

Comment thread install.sh Outdated
warn "GitHub API returned no release wheel; falling back to the release page."
tag="$(curl -fsS -o /dev/null -w '%{redirect_url}' \
"https://github.com/EverMind-AI/Raven/releases/latest")" || tag=""
version="${tag##*/}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium - the redirect target is not validated at all here, unlike the other two implementations.

${tag##*/} takes the last path segment of whatever %{redirect_url} produced, with no check on scheme, host, or path prefix. Compare:

  • upgrade_commands._fetch_latest_release_via_redirect requires location.startswith("https://github.com/EverMind-AI/Raven/releases/tag/").
  • install.ps1 Resolve-RavenLatestVersion matches the whole URL against ^https://github\.com/EverMind-AI/Raven/releases/tag/v([0-9]+\.[0-9]+\.[0-9]+)$.
  • install.sh checks nothing about the URL.

The PR description says the wheel URL is derived from a single shared shape "so the rule cannot drift". That holds inside upgrade_commands.py; it does not hold across the three implementations, and install.sh is the one that drifted.

To be clear about severity: I checked, and this is not exploitable. The download URL's scheme, host and path prefix are hard-coded on line 212, and both interpolations are prefixed (v${version} and raven-${version}-), so no bare .. segment can be produced and curl cannot be steered to another host. The practical cost is a worse failure mode: instead of cleanly reporting "could not resolve a version", the script builds a URL that is guaranteed to 404 and reports a download failure instead.

Suggested fix, mirroring the PowerShell version:

case "$tag" in
  https://github.com/EverMind-AI/Raven/releases/tag/v*) ;;
  *) tag="" ;;
esac
version="${tag##*/}"
version="${version#v}"

Confidence: high, read directly from the three implementations.

Comment thread install.sh Outdated
*.*.*) ;;
*) version="" ;;
esac
case "$version" in

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium (same family as the previous comment) - these two case blocks accept version strings that both other implementations reject.

I extracted the two blocks verbatim and ran them under sh:

Input install.sh Python _VERSION_RE / install.ps1 regex
0.1.11 accepted accepted
1.2.3.4 accepted rejected
0.01.2 accepted rejected (leading zero)
1..2 accepted rejected
.. accepted rejected
... accepted rejected
v1.2.3, 1.2, ``, a.b.c rejected rejected

*.*.* matches .. because each * can match the empty string, and *[!0-9.]* only excludes characters outside [0-9.] - it says nothing about how many dots there are or where they sit.

As noted on line 202, none of these produce a traversal or an off-host fetch; they produce a wheel URL that 404s. The cost is a misleading error message at download time instead of a correct one at resolution time.

Suggested fix - an explicit three-field numeric check, still POSIX sh:

valid_version=""
IFS=. read -r v_major v_minor v_patch v_extra <<EOF
$version
EOF
if [ -z "$v_extra" ]; then
  valid_version="$version"
  for field in "$v_major" "$v_minor" "$v_patch"; do
    case "$field" in
      ""|*[!0-9]*) valid_version="" ;;
    esac
  done
fi
version="$valid_version"

(Or keep the case style and simply require each of the three fields to be non-empty and all-digits; the point is that the current pattern cannot express "exactly three non-empty numeric fields".)

Confidence: high, reproduced by running the extracted blocks.

Comment thread raven/cli/upgrade_commands.py Outdated
response.raise_for_status()
return _parse_release_payload(response.json())
return _resolve_latest_release(client)
with httpx.Client(timeout=10.0, follow_redirects=True) as owned_client:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low - the worst-case wait for raven upgrade tripled, and the Risk section does not mention it.

timeout=10.0 is per request. The resolution path is now up to three sequential requests: API GET, release-page GET, wheel HEAD. With GitHub unreachable, raven upgrade blocks for up to ~30s before printing anything, where it previously failed at ~10s.

The background update_notice._refresh() caller is a daemon thread, so it does not block anything there - this only affects the foreground command.

Suggested fix: either give the fallback path a shorter timeout (client.get(..., timeout=5.0), which httpx accepts per request), or note the new worst case in the Risk section so it is a decision rather than a side effect. A shorter fallback timeout seems reasonable given the fallback exists to make a failing command succeed, not to keep it hanging.

Confidence: high on the arithmetic; I did not measure a real 30s failure.

Comment thread raven/cli/upgrade_commands.py Outdated
def _github_failure_detail(error: Exception) -> str:
if isinstance(error, httpx.HTTPStatusError):
response = error.response
if response.status_code in (403, 429) and response.headers.get("x-ratelimit-remaining") == "0":

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low - GitHub's secondary rate limit does not match this condition and degrades to a bare status line.

The primary limit sets x-ratelimit-remaining: 0, which this handles well. The secondary (abuse) rate limit returns 403 or 429 with a retry-after header while x-ratelimit-remaining may still be non-zero. Reproduced with a mock returning 403 + retry-after: 60:

GitHub API: HTTP 403 Forbidden

The retry-after value, which is the one actionable fact in that response, is dropped. Installer-style traffic bursts are a common trigger for the secondary limit.

Suggested fix: add a branch before the fallthrough:

retry_after = response.headers.get("retry-after")
if retry_after:
    return f"GitHub asked us to retry in {retry_after}s (HTTP {response.status_code})"

Confidence: high on the code path (reproduced); medium on how often GitHub picks 403 vs 429 for it.

Comment thread raven/cli/upgrade_commands.py Outdated

def _sentence(error: Exception) -> str:
"""Terminate the message with exactly one period; some already carry theirs."""
return f"{str(error).rstrip('.')}."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low - rstrip strips a character set, not a suffix.

str.rstrip('.') removes every trailing ., not just one. Verified on this branch:

_sentence('ends in ellipsis...') -> 'ends in ellipsis.'
_sentence('already ends.')       -> 'already ends.'
_sentence('no punctuation')      -> 'no punctuation.'

No message in the codebase currently ends in an ellipsis, so this is latent rather than live, but the docstring says "exactly one period" and the code implements "collapse all trailing periods to one".

Suggested fix: return f"{str(error).removesuffix('.')}."

Confidence: high, reproduced.

Comment thread raven/cli/upgrade_commands.py Outdated
if not response.has_redirect_location or not location.startswith(RELEASE_TAG_PREFIX):
raise ReleaseLookupError(f"HTTP {response.status_code} without a release tag redirect")

version = ".".join(str(part) for part in _version_key(location[len(RELEASE_TAG_PREFIX) :]))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low (cosmetic) - a bare tag prefix yields a message with a dangling colon.

If the redirect Location is exactly https://github.com/EverMind-AI/Raven/releases/tag/ (prefix present, nothing after it), the startswith guard passes, the slice is empty, and _version_key("") raises with an empty value. Reproduced:

could not resolve the latest Raven release (GitHub API: HTTP 503 Service Unavailable; release page: Unsupported Raven version:)

Suggested fix: either require a non-empty remainder in the guard on the line above, or have _version_key quote the value (f"Unsupported Raven version: {value!r}"), which also makes whitespace-only values visible in the other call sites.

Confidence: high, reproduced. Very low impact - GitHub does not produce this shape today.

Review on #299 found that the daily update check is what drains the bucket this
branch was making survivable: update_notice._refresh() called the API-first
_fetch_latest_release(), so every install behind a shared egress spent one
unauthenticated request per day, on top of every upgrade. The notice needs a
version string and nothing else, so it now calls fetch_latest_version(), which
reads the release page redirect and sends neither the API request nor the wheel
HEAD.

Nine smaller findings from the same review:

- a 200 with a non-JSON body raises httpx.DecodingError, so a proxy or captive
  portal reaches the fallback instead of surfacing a raw parser message that
  also advised reinstalling a healthy installation
- a missing wheel is reported against the wheel rather than against the release
  page that answered correctly, and the HEAD now carries the raven User-Agent
  like the other two requests
- install.sh validates the redirect target and accepts exactly three numeric
  fields with no leading zeros, matching the CLI and install.ps1; the previous
  pattern accepted `..`, `1.2.3.4` and `0.01.2`
- the secondary rate limit reports its retry-after instead of a bare status line
- the fallback requests use a 5s timeout, so adding a third request does not
  triple the worst-case wait
- _sentence strips one trailing period instead of the whole run, keeping an
  ellipsis intact, and _version_key quotes the value it rejected
- the fallback comment now states the rule the code implements: no payload-level
  failure is routed around, not only draft or prerelease

Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
@0xKT

0xKT commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks - this was a substantive review. All ten findings hold up; I reproduced each one against the branch before acting on it. Nine are fixed in 0818bcf, and one suggested fix is declined with a reason below.

# Finding Disposition
1 Daily update check spends API budget Fixed, and folded into this PR rather than deferred
2 200 with a non-JSON body escapes the fallback Fixed as suggested
3 Implemented rule broader than the documented one Fixed by option 2 (wording); option 1 declined, see below
4 Missing wheel blamed on the release page; HEAD lacks the User-Agent Both fixed as suggested
5a install.sh does not validate the redirect target Fixed as suggested
5b install.sh version pattern accepts .., 1.2.3.4, 0.01.2 Fixed; all twelve inputs from your table now match the CLI and install.ps1
6 Worst-case wait tripled Fixed: the two fallback requests use a 5s timeout
7 Secondary rate limit degrades to a bare status line Fixed as suggested
8 rstrip('.') strips a set, not a suffix Fixed with removesuffix, plus a parametrised test covering the ellipsis case
9 Dangling colon on an empty tag remainder Fixed with !r in _version_key

On finding 1, you were right that it is the actual consumer, and right that it is upstream of the diff - but I folded it in rather than opening a separate issue. It is the same sentence as the PR title (both are "resolve the latest release without spending the API quota"), #298 already records the mechanism, and leaving it out would close #298 while the bucket still drains daily for everyone sharing the egress, gh and CI included. update_notice._refresh() now calls a new fetch_latest_version(), which stops after the redirect: no API request, no wheel HEAD. A test asserts that touching _fetch_latest_release from that path fails.

On finding 3, I took your option 2 and declined option 1. The argument for UnstableReleaseError rests on the assetless window recovering through the fallback, and it does not: if the assets are still uploading, the fallback derives the wheel URL and its own HEAD returns 404, so the upgrade fails anyway, two requests later. That leaves an extra exception type earning only the API-glitch case where the payload is broken but the asset exists. The comment and the description now say what the code does - no payload-level failure is routed around - and name draft/prerelease as the case that must stay that way.

On the coverage note: still true, and I am not fixing it here. The install.sh block was verified by extracting the real lines by line range and running your twelve inputs through them; install.ps1 was verified under PowerShell 7.6.4, including a forced exception branch (a 403 carrying a Location header, which is the shape Windows PowerShell produces for an unfollowed redirect). Windows PowerShell 5.1 itself remains unverified, as the description states.

Local verification after the fixes: tests/test_cli_upgrade_commands.py + tests/test_cli_update_notice.py -> 137 passed, make test-python -> 6137 passed, 33 skipped, make lint-python clean, sh -n install.sh clean, and against real GitHub fetch_latest_version() returns 0.1.11 in one request.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-verified 0818bcf against the same probes as the first pass, plus new ones for each fix. Nine of nine confirmed fixed. Your pushback on finding 3 is correct and I withdraw option 1. One new low finding inline, and one note about the reported test state that is not a defect in this PR.

Fixes verified

Each row was reproduced on the branch, not read off the diff.

# Verified behaviour
1 fetch_latest_version(client) issues exactly GET https://github.com/EverMind-AI/Raven/releases/latest and returns 0.1.11. No API request, no wheel HEAD. test_fetch_latest_version_stays_off_the_api_and_sends_no_head asserts the full request list by equality, so it pins the absence, not just the presence.
2 200 + <html>blocked by proxy</html> now resolves through the fallback to 0.1.4.
2b I checked the load-bearing claim in your comment: httpx.DecodingError.__mro__ on httpx 0.28.1 is DecodingError -> RequestError -> HTTPError, so isinstance(err, httpx.TransportError) is False. Confirmed end to end: non-JSON body + release page ConnectError reports (GitHub API: GitHub API returned a non-JSON body; release page: no route to host) with no "check your network".
3 A prerelease: true payload still raises Latest Raven release is not stable after exactly one request. The fallback is not entered.
4 release 0.1.4 has no wheel at the expected URL (HTTP 404 Not Found) - the release page is no longer blamed. All three requests now carry ua=raven/0.1.11.
5a/5b Extracted lines 202-223 by line range and ran them under sh, bash and dash. Accepts 0.1.11, 0.0.0, 10.20.30, 1.2.3, 0.1.4. Rejects all twelve of my inputs plus https://evil.example.com/..., https://github.com/Other-Org/Raven/..., a non-tag redirect and an empty redirect.
6 Observed per-request timeouts: API 10.0, release page 5.0, wheel HEAD 5.0.
7 GitHub asked us to retry in 60s (HTTP 403).
8 'ends in ellipsis...' -> 'ends in ellipsis...', 'no punctuation' -> 'no punctuation.'.
9 Bare tag prefix now reports HTTP 302 without a release tag redirect; no dangling colon.

Two details worth calling out because they are easy to get wrong and you got them right:

  • In the install.sh field loop, version is assigned inside the loop body while version also feeds the for word list. The list is expanded once before the first iteration, so mutating it mid-loop does not truncate the remaining fields. Verified: 0.01.2 is rejected on the second field, and 1..2 on the second field, without either short-circuiting the third.
  • 0[0-9]* as a shell pattern requires at least two characters, so a bare 0 field is still accepted. That is what makes 0.1.11 and 0.0.0 pass while 0.01.2 fails, and it matches _VERSION_RE's (0|[1-9][0-9]*) exactly. I checked this specifically because the same expression read as a regex would have rejected every 0.x.y release.

Finding 3: you are right, option 1 withdrawn

Your argument holds. I re-ran both halves: the assetless payload fails at Expected exactly one release wheel, and if it were routed to the fallback the derived wheel's HEAD returns 404, so the upgrade fails anyway two requests later. UnstableReleaseError would therefore only earn the case where the payload is broken and the asset exists, which does not justify a second exception type. Option 2 was the right call and the comment now says what the code does.

One residual, stated for the record rather than as a request: the case that stays uncovered is a GitHub payload-shape change, which would hard-fail raven upgrade for every user at once, and the upgrade path is self-sealing by your own Risk section - a user who cannot upgrade cannot receive the fix. That was equally true before this PR, so it is not a regression, and the fallback is not the natural place to solve it.

New finding

One inline comment: the update notice's only request is given _FALLBACK_TIMEOUT, and the _REQUEST_TIMEOUT in fetch_latest_version is consequently unreachable. Low severity.

On the reported test state - not a defect in this PR

make test-python -> 6137 passed, 33 skipped does not reproduce here. What I get:

  • at 0818bcf: 1 failed, 6144 passed, 33 skipped, 13 deselected
  • at main (687da67): 1 failed, 6130 passed, 30 skipped, 13 deselected

The failure is tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare, identical on both sides, so it is not caused by this PR. It is a pre-existing intra-file ordering bug, which I bisected: the test passes alone under both pytest and pytest --all-extras, and fails when tests/test_cli_theme.py::test_onboard_panels_render_without_missing_style[dark] runs before it in the same session. It asserts a hard-coded accent ANSI sequence (1;38;2;251;226;63), so the polluting test appears to leave theme state behind.

Two things follow. First, 6137 is the count from 5fccd72, before the eight tests this commit adds - the branch total is now 6145, so the number was not re-measured after the fixes. Second, the suite is not green on main either, which is worth someone's attention independently of this PR; happy to open it separately with the bisect above.

Everything else in your verification list reproduces: tests/test_cli_upgrade_commands.py + tests/test_cli_update_notice.py -> 137 passed, make lint-python -> All checks passed! / 7 files already formatted, sh -n install.sh clean, and against real GitHub releases/latest -> 302 to tag/v0.1.11 with the derived wheel HEAD returning 200 through release-assets.githubusercontent.com.

No objection to merging from my side once the timeout nit is either fixed or consciously dropped.

Comment thread raven/cli/upgrade_commands.py Outdated
LATEST_RELEASE_WEB,
headers={"User-Agent": _user_agent()},
follow_redirects=False,
timeout=_FALLBACK_TIMEOUT,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low - the update notice's primary request is given the fallback timeout, and fetch_latest_version's own _REQUEST_TIMEOUT is unreachable.

This override lives in _fetch_latest_version_via_redirect, which now has two callers with opposite needs:

  1. _fetch_latest_release_via_redirect - here 5s is exactly right: it is the second of three sequential requests and must not add a full timeout to a failing raven upgrade. That is what the constant's comment says.
  2. fetch_latest_version - here it is the only request. The rationale does not apply, and httpx.Client(timeout=_REQUEST_TIMEOUT, ...) on the line below is dead: the per-request override always wins.

Observed on the branch by recording request.extensions["timeout"]:

_fetch_latest_release:  GET api.github.com  timeout=10.0
                        GET github.com      timeout=5.0
                        HEAD github.com     timeout=5.0
fetch_latest_version:   GET github.com      timeout=5.0   <- expected 10.0

Effect is small but real: the daily notice refresh runs in a daemon thread with a 24h TTL and nothing waiting on it, so it can afford the full 10s. At 5s, a slow link makes it give up and then back off for a full day - the least-hurried caller gets the most impatient timeout, and _write_cache(keep, ...) stamps checked_at so it will not retry before tomorrow.

Suggested fix - let the caller decide instead of the callee:

def _fetch_latest_version_via_redirect(client: httpx.Client, *, timeout: float = _REQUEST_TIMEOUT) -> str:
    response = client.get(
        LATEST_RELEASE_WEB,
        headers={"User-Agent": _user_agent()},
        follow_redirects=False,
        timeout=timeout,
    )
    ...

and pass timeout=_FALLBACK_TIMEOUT from _fetch_latest_release_via_redirect only (alongside the HEAD, which is correctly on the fallback budget already). fetch_latest_version then gets the 10s its own client already asks for, and _REQUEST_TIMEOUT stops being decoration.

Alternatively, if you would rather keep the signature, drop timeout=_REQUEST_TIMEOUT from fetch_latest_version's client so the code does not claim a budget it never uses - but I think the notice genuinely wants the longer one.

Confidence: high, reproduced.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. No blocker: all ten findings from the first pass are fixed and re-verified against 0818bcf, the one remaining item is a low-severity timeout nit that does not affect correctness, and every CI check is green.

Correction to my previous comment, which was incomplete. I said the suite is not green on main either, and gave two local runs ending 1 failed. That is what happens on my machine, but I did not identify why CI disagrees, and the reason changes the reading:

TERM=dumb            -> 45 passed
TERM=xterm-256color  -> 1 failed, 44 passed
TERM= (empty)        -> 1 failed, 44 passed

The unit job sets TERM: dumb (for an unrelated typer/rich colour reason documented in ci.yml), which is why CI is green and my local make test-python is red. So tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare is not a failure anyone should attribute to this PR or to main as CI sees it - it is a test-isolation bug that only surfaces under a real terminal, i.e. exactly when a developer runs the suite locally. Bisected: it passes alone, and fails when test_onboard_panels_render_without_missing_style[dark] runs before it. Worth its own issue, not this PR's problem, and I withdraw the implication that the branch was merged over a red suite.

The stale-count observation stands on arithmetic rather than environment: 6137 was the total at 5fccd72, and this commit adds eight tests (129 -> 137 in the two touched files), so the branch total is 6145. Minor, and it does not change the disposition.

Verification summary for this approval (all reproduced on the branch, not read off the diff): nine fixes confirmed behaviourally; httpx.DecodingError confirmed to be an HTTPError and not a TransportError, which is what keeps a non-JSON body from reading as "offline"; install.sh's new shape check exercised under sh, bash and dash across 21 inputs including off-host and off-repo redirects; 137 passed on the two touched test files; make lint-python clean; sh -n install.sh clean; and the real-GitHub path resolving 0.1.11 through the release page with the derived wheel returning 200.

The one open nit (the update notice's only request getting _FALLBACK_TIMEOUT while fetch_latest_version's _REQUEST_TIMEOUT is unreachable) is yours to take or drop - it is not worth another round trip.

gloryfromca
gloryfromca previously approved these changes Aug 12, 2026
Review on #299: _fetch_latest_version_via_redirect hard-coded the fallback
budget, so its two callers could not disagree. The update notice makes that
request and nothing else, from a daemon thread with a 24h TTL and nobody waiting
on it, yet it got the impatient 5s while the _REQUEST_TIMEOUT its own client asks
for was unreachable -- a per-request override always wins. A slow link therefore
made the notice give up and then back off for a full day.

The timeout is a keyword argument now: the upgrade fallback passes 5s, because it
is the second and third of three sequential requests, and the notice keeps 10s.
A test pins all four request budgets by equality.

Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
@0xKT

0xKT commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Timeout finding accepted and fixed in 9731790. And you are right about the test count: mine was stale, not measured.

The timeout. Confirmed by construction before changing anything: a per-request override always wins over the client's, so fetch_latest_version's _REQUEST_TIMEOUT was decoration, and the least-hurried caller got the most impatient budget - with _write_cache(keep, ...) stamping checked_at, one slow link cost a full day of no notice. Fixed the way you suggested, with the timeout as a keyword argument: the upgrade fallback passes _FALLBACK_TIMEOUT because it is the second and third of three sequential requests, and the notice keeps _REQUEST_TIMEOUT. test_request_timeouts_follow_the_caller_not_the_helper pins all four budgets by equality, so a future caller cannot silently inherit the wrong one.

The test count. 6137 came from 5fccd72 and I reused it in the description after adding tests instead of re-running. That is my error, and the number is now measured at the head of this branch rather than carried:

make test-python -> 6146 passed, 33 skipped, 13 deselected, 0 failed  (243s)
uv run pytest tests/test_cli_upgrade_commands.py tests/test_cli_update_notice.py -q -> 138 passed

6146 reconciles with your 6145 total at 0818bcf plus the one test this commit adds. The description is updated.

Your failure does not reproduce here. tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare passes for me in four configurations:

Configuration Result
make test-python (full suite) 6146 passed, 0 failed
uv run pytest tests/test_cli_theme.py -q 45 passed
Your exact order: test_onboard_panels_render_without_missing_style then test_bold_accent_renders_styled_not_bare 3 passed
Same file with HOME pointed at an empty directory 45 passed

So I cannot confirm the intra-file ordering bug from here, and I am not going to chase a failure I cannot see. Two things make an environment difference plausible: that test asserts a hard-coded accent ANSI sequence, and a worktree can carry different theme state than the checkout I ran in. If it reproduces for you consistently, please do open it separately with the bisect - your report is the only evidence either way, and it should not be attached to this PR.

On the residual you recorded for finding 3 - a GitHub payload-shape change hard-failing every user at once - agreed on all three points: real, not a regression, and not the fallback's job. Worth its own issue if it ever stops being hypothetical.

@0xKT
0xKT merged commit 1cb604a into main Aug 13, 2026
12 checks passed
@0xKT
0xKT deleted the fix/upgrade_release_discovery_quota branch August 13, 2026 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: version discovery fails once the anonymous github api quota is spent

2 participants