fix(gl): point doctor's version check at Gitlawb/node, not the frozen Gitlawb/releases repo#198
fix(gl): point doctor's version check at Gitlawb/node, not the frozen Gitlawb/releases repo#198euxaristia wants to merge 4 commits into
Conversation
… Gitlawb/releases repo check_version queried https://api.github.com/repos/gitlawb/releases/releases/latest. Gitlawb/releases is a separate repo that stopped receiving tags after v0.3.8 (2026-03-30). Releases have shipped directly on Gitlawb/node since v0.4.0, via release-please, and install.sh already treats Gitlawb/node as the canonical release source. The old query target meant gl doctor's version check could never again detect a real update. Points the check at Gitlawb/node instead, matching install.sh's own default, and makes the GitHub API base injectable so the fix is covered by a test against a mocked server rather than the real GitHub API. Closes Gitlawb#197
github-actions
Bot
added
the
needs-tests
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
|
Warning Review limit reached
Next review available in: 59 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. 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. 📝 WalkthroughWalkthroughThe ChangesDoctor version check
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/gl/src/doctor.rs (1)
336-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider checking the HTTP status before parsing the response body.
If GitHub rate-limits the IP, it returns a 403 status with a JSON error payload. The current code parses the error JSON, finds
tag_namemissing, and defaults to(could not parse latest tag). Explicitly checkingresp.status().is_success()before parsing would allow for a more accurate diagnostic message when the API rate-limits the request or fails.♻️ Proposed refactor
let resp = match client .get(format!( "{github_api_base}/repos/Gitlawb/node/releases/latest" )) .send() .await { Ok(r) => r, Err(_) => return Check::pass("version", format!("v{current} (offline — could not check)")), }; + + if !resp.status().is_success() { + return Check::pass("version", format!("v{current} (API returned HTTP {})", resp.status())); + } let body: serde_json::Value = match resp.json().await {🤖 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 `@crates/gl/src/doctor.rs` around lines 336 - 338, Update the GitHub release request flow in the surrounding doctor logic to check resp.status().is_success() before parsing the response body. For non-success responses, return or report a diagnostic that includes the HTTP failure status, while preserving the existing tag parsing behavior for successful responses.
🤖 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 `@crates/gl/src/doctor.rs`:
- Around line 441-454: Strengthen test_check_version_up_to_date by asserting the
returned check’s expected detail/message in addition to CheckState::Ok. Use the
existing detail field or accessor on the check and verify it contains the
up-to-date version information, ensuring the mocked successful response was
actually processed.
---
Nitpick comments:
In `@crates/gl/src/doctor.rs`:
- Around line 336-338: Update the GitHub release request flow in the surrounding
doctor logic to check resp.status().is_success() before parsing the response
body. For non-success responses, return or report a diagnostic that includes the
HTTP failure status, while preserving the existing tag parsing behavior for
successful responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Re: the needs-tests note — this PR does add tests, in the same commit and file. `test_check_version_queries_gitlawb_node_releases` and `test_check_version_up_to_date` are both in `crates/gl/src/doctor.rs`'s existing `mod tests` block, covering the warn and up-to-date branches of `check_version` against a mocked GitHub API. The triage workflow's test-detection regex (`.github/workflows/pr-triage.yml:73-75`) only matches `#[test]` or `#[cfg(test)` literally. Both new tests use `#[tokio::test]`, since `check_version` is async, which doesn't match either alternative. That looks like a gap in the regex rather than anything about this PR: it would false-flag any PR whose only new tests are async (`#[tokio::test]`, `#[async_std::test]`, etc.), which given this codebase already uses `#[tokio::test]` throughout is probably worth a follow-up fix on its own. |
…gthen up-to-date test Addresses CodeRabbit review on Gitlawb#198: - check_version now checks resp.status().is_success() before parsing the body, so a GitHub rate-limit or other API error surfaces as its actual HTTP status instead of a generic "could not parse latest tag". - test_check_version_up_to_date now also asserts the detail message, so the test can't silently pass on an unmatched mock request (mockito's fallback for an unhandled request is a 501, which check_version also maps to Ok).
|
Pushed a fixup addressing both CodeRabbit comments:
`cargo test -p gl doctor`, `cargo fmt --check`, and `cargo clippy -D warnings` all still clean. |
beardthelion
added
crate:gl
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Verified by execution: reverting just the production URL (leaving the mocks) turns both new tests red, so they're genuinely pinned to the fix, and the retarget to Gitlawb/node matches the GITLAWB_RELEASE_REPO default in install.sh/install.ps1. The core change is correct. One thing to add before merge.
Findings
-
[P2] Add a test for the new non-2xx branch
crates/gl/src/doctor.rs:346
Theif !resp.status().is_success()guard is the branch the fixup was written for (the 403 rate-limit case), but it has no test. Disabling the whole block leaves all 9 tests green, so nothing currently drives the error response class, only the 200 path. This is the adversarial gap that matters on a diff whose second half exists specifically to handle non-2xx.A
with_status(403)mock closes it (I ran this against the branch; it passes):#[tokio::test] async fn test_check_version_http_error() { let mut server = mockito::Server::new_async().await; let _m = server .mock("GET", "/repos/Gitlawb/node/releases/latest") .with_status(403) .create_async() .await; let check = check_version("0.1.0", &server.url()).await; assert!(matches!(check.state, CheckState::Ok)); assert!(check.detail.contains("403")); }
Assert both the
Okstate andcontains("403")— a state-only check would still pass if a regression dropped the status into the JSON-parse path instead.
The retarget itself is sound and the two added tests are load-bearing, so this is the only blocker. The is_newer numeric-only parse (pre-release / 4-part tags) is pre-existing and unchanged here, out of scope for this PR.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Remove the extra blank line at EOF
crates/gl/src/doctor.rs:478
The added terminal blank line makescargo fmt --all -- --checkfail; the repository'sfmt + clippyworkflow runs that command as a required formatting gate. The current head therefore cannot pass CI until this line is removed.
cargo fmt --all -- --check fails on the extra terminal blank line, and the fmt + clippy workflow runs that check as a required gate.
|
Pushed a fixup addressing jatmn's finding: removed the trailing blank line at EOF in doctor.rs. cargo fmt --all -- --check now passes clean, and all 10 doctor tests still pass. |
Dismissing my stale CHANGES_REQUESTED — it was on 67d52be, before the contributor added the non-2xx test and removed the EOF blank line. Re-reviewed the current head; approving separately.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Both blockers are closed. test_check_version_http_error is load-bearing: I removed the if !resp.status().is_success() block and it goes red (panic at the 403-detail assertion) while the other 9 doctor tests stay green, then restored to 10/10. That pins the exact non-2xx branch the fixup was written for. The EOF blank line is gone and fmt + clippy is green.
The rest still holds from my last pass: the two URL tests remain load-bearing (reverting the retarget turns both red), and the retarget itself matches the canonical release source (install.sh and install.ps1 both default GITLAWB_RELEASE_REPO to Gitlawb/node). I traced the full response-class matrix on the current head, offline / 3xx / 4xx / 5xx / 2xx-non-JSON / 2xx-without-tag / 2xx-newer / 2xx-up-to-date, and every branch returns a truthful detail string with the status guard correctly ahead of the JSON parse. The injected base URL is test-only; the sole production caller passes the const. LGTM.
One non-blocking note for later, not this PR: now that the URL actually resolves, is_newer's numeric-only parse (splitn(3, '.')) starts seeing real tags, so a pre-release or 4-part tag (v1.2.3-rc1, v1.2.3.4) would compare imperfectly. It degrades gracefully (no panic), it's pre-existing, and it's out of scope here, worth a follow-up if release tags ever take that shape.
@kevincodex1 approved and ready to merge.
3 participants
Summary
check_versionincrates/gl/src/doctor.rsqueriesgitlawb/releases/releases/latest.Gitlawb/releasesis a separate repo that stopped receiving tags after v0.3.8 (2026-03-30). Releases have shipped directly onGitlawb/nodesince v0.4.0, via release-please, andinstall.shalready treatsGitlawb/nodeas the canonical release source (REPO="${GITLAWB_RELEASE_REPO:-Gitlawb/node}"). The old query target meant the version check could never again detect a real update once past v0.3.8.Closes #197
What changed
check_versionnow queriesGitlawb/node's releases endpoint instead ofgitlawb/releases.github_api_base), so the fix can be covered by a test against a mocked server instead of hitting the real GitHub API.How a reviewer can verify
Kind of change
Summary by CodeRabbit
gl doctorversion checks by retrieving release information from the correct project endpoint.