Skip to content

Wave-40 C11: user-initiated update check - #328

Merged
KooshaPari merged 3 commits into
mainfrom
feat/sl-w40-update-check
Jul 19, 2026
Merged

Wave-40 C11: user-initiated update check#328
KooshaPari merged 3 commits into
mainfrom
feat/sl-w40-update-check

Conversation

@KooshaPari

Copy link
Copy Markdown
Owner

Summary

  • Add sl-daemon check-update to compare the installed version against the latest GitHub Release tag (check-only; no download or install per ADR 0001).
  • Document the manual update posture in docs/ops/update-check.md with ADR/distribution cross-links.
  • Add hermetic SelfCheck (scripts/update-check-check.ps1, tests/update_check.rs) plus soft (update-check-soft.yml) and blocking (update-check-hard.yml) CI smoke.

Test plan

  • pwsh ./scripts/update-check-check.ps1 -SelfCheck
  • cargo test update_check in crates/sl-daemon
  • cargo test --test check_update in crates/sl-daemon
  • cargo test --test update_check at repo root
  • CI: update check hard workflow on PR

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@KooshaPari, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0383d3ff-6494-477c-9fde-ffb7eee49706

📥 Commits

Reviewing files that changed from the base of the PR and between 2cee1dc and c64ed09.

📒 Files selected for processing (12)
  • .github/workflows/update-check-hard.yml
  • .github/workflows/update-check-soft.yml
  • CHANGELOG.md
  • crates/sl-daemon/src/main.rs
  • crates/sl-daemon/src/update_check.rs
  • crates/sl-daemon/tests/check_update.rs
  • docs/adr/0001-desktop-companion-scope.md
  • docs/ops/distribution.md
  • docs/ops/update-check.md
  • llms.txt
  • scripts/update-check-check.ps1
  • tests/update_check.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/sl-w40-update-check
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch feat/sl-w40-update-check

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Risk: medium. Cursor Bugbot did not run on this PR; human review is required because the change adds user-facing update-check behavior and GitHub API network calls, which exceeds the low-risk approval threshold.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a user-initiated release update check command (sl-daemon check-update) that compares the installed version against the latest GitHub Release tag. The feedback highlights several critical improvements: enhancing the SemVer comparison to properly handle pre-release suffixes, supporting uppercase 'V' prefixes in release tags, and avoiding hardcoded repository URLs in the status output when a custom repository is queried. Additionally, the reviewer recommends configuring a network timeout on the HTTP client to prevent CLI hangs and adding support for GitHub authentication tokens to mitigate API rate-limiting issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread crates/sl-daemon/src/update_check.rs Outdated
Comment on lines +54 to +56
pub fn version_cmp(a: &str, b: &str) -> Ordering {
parse_semver(a).cmp(&parse_semver(b))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current version_cmp implementation completely ignores pre-release suffixes (e.g., -beta.1) by splitting on - and only comparing the numeric core. This causes stable releases and pre-releases of the same version core to be treated as equal (e.g., 0.1.0-beta.1 is treated as equal to 0.1.0). As a result, users on pre-releases will not be notified of stable updates, and pre-releases won't be ordered correctly. We should implement a robust comparison that handles pre-release tags according to SemVer rules.

pub fn version_cmp(a: &str, b: &str) -> Ordering {
    let a_semver = parse_semver(a);
    let b_semver = parse_semver(b);
    match a_semver.cmp(&b_semver) {
        Ordering::Equal => {
            let a_pre = normalize_tag(a).split_once('-').map(|(_, pre)| pre);
            let b_pre = normalize_tag(b).split_once('-').map(|(_, pre)| pre);
            match (a_pre, b_pre) {
                (None, None) => Ordering::Equal,
                (None, Some(_)) => Ordering::Greater,
                (Some(_), None) => Ordering::Less,
                (Some(ap), Some(bp)) => compare_prerelease(ap, bp),
            }
        }
        ord => ord,
    }
}

fn compare_prerelease(ap: &str, bp: &str) -> Ordering {
    let mut ap_parts = ap.split('.');
    let mut bp_parts = bp.split('.');
    loop {
        match (ap_parts.next(), bp_parts.next()) {
            (None, None) => return Ordering::Equal,
            (None, Some(_)) => return Ordering::Less,
            (Some(_), None) => return Ordering::Greater,
            (Some(a_seg), Some(b_seg)) => {
                let ord = match (a_seg.parse::<u64>(), b_seg.parse::<u64>()) {
                    (Ok(an), Ok(bn)) => an.cmp(&bn),
                    _ => a_seg.cmp(b_seg),
                };
                if ord != Ordering::Equal {
                    return ord;
                }
            }
        }
    }
}

Comment thread crates/sl-daemon/src/update_check.rs Outdated
Comment on lines +98 to +111
pub fn format_status(status: &UpdateStatus) -> String {
match status {
UpdateStatus::UpToDate { installed, latest } => {
format!("sl-daemon {installed} is up to date (latest release: {latest})")
}
UpdateStatus::UpdateAvailable { installed, latest } => {
format!(
"update available: sl-daemon {installed} → {latest}\n\
Download from https://github.com/{DEFAULT_REPO}/releases/tag/{latest}\n\
Verify SHA256SUMS (and Sigstore bundle when present) before replacing binaries."
)
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The format_status function hardcodes DEFAULT_REPO in the printed download URL. If a user specifies a custom repository using the --repo flag, the printed update message will still point to the default repository. We should pass the queried repo to format_status to ensure the correct URL is displayed.

Suggested change
pub fn format_status(status: &UpdateStatus) -> String {
match status {
UpdateStatus::UpToDate { installed, latest } => {
format!("sl-daemon {installed} is up to date (latest release: {latest})")
}
UpdateStatus::UpdateAvailable { installed, latest } => {
format!(
"update available: sl-daemon {installed} → {latest}\n\
Download from https://github.com/{DEFAULT_REPO}/releases/tag/{latest}\n\
Verify SHA256SUMS (and Sigstore bundle when present) before replacing binaries."
)
}
}
}
pub fn format_status(status: &UpdateStatus, repo: &str) -> String {
match status {
UpdateStatus::UpToDate { installed, latest } => {
format!("sl-daemon {installed} is up to date (latest release: {latest})")
}
UpdateStatus::UpdateAvailable { installed, latest } => {
format!(
"update available: sl-daemon {installed} → {latest}\n\
Download from https://github.com/{repo}/releases/tag/{latest}\n\
Verify SHA256SUMS (and Sigstore bundle when present) before replacing binaries."
)
}
}
}

let payload = serde_json::to_string_pretty(&status).unwrap_or_default();
println!("{payload}");
} else {
println!("{}", update_check::format_status(&status));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Update the call to format_status to pass the repo parameter so that custom repositories specified via --repo are correctly reflected in the printed download URL.

        println!("{}", update_check::format_status(&status, repo));

let latest_tag = match latest_override {
Some(tag) => tag,
None => {
let client = reqwest::Client::new();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The reqwest::Client is constructed without a timeout. If the network is slow or GitHub is unresponsive, the CLI command will hang indefinitely. Configuring a reasonable timeout (e.g., 10 seconds) is a crucial best practice for CLI tools.

            let client = reqwest::Client::builder()
                .timeout(std::time::Duration::from_secs(10))
                .build()
                .unwrap_or_default();

Comment thread crates/sl-daemon/src/update_check.rs Outdated
Comment on lines +39 to +41
pub fn normalize_tag(tag: &str) -> &str {
tag.trim().strip_prefix('v').unwrap_or(tag.trim())
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The normalize_tag function only strips lowercase 'v'. If a release tag starts with uppercase 'V', it won't be stripped, leading to parsing failures. We should support both cases.

pub fn normalize_tag(tag: &str) -> &str {
    let trimmed = tag.trim();
    trimmed
        .strip_prefix('v')
        .or_else(|| trimmed.strip_prefix('V'))
        .unwrap_or(trimmed)
}

Comment thread crates/sl-daemon/src/update_check.rs Outdated
Comment on lines +75 to +81
let resp = client
.get(&url)
.header("Accept", "application/vnd.github+json")
.header("X-GitHub-Api-Version", "2022-11-28")
.header("User-Agent", "sl-daemon-check-update")
.send()
.await?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Unauthenticated GitHub API requests are heavily rate-limited (60 requests per hour per IP). If users or CI workflows run check-update frequently, they will easily hit this limit. We should check for GITHUB_TOKEN or GH_TOKEN environment variables and attach them as a Bearer token if present to increase the rate limit to 5000 requests per hour.

    let mut req = client
        .get(&url)
        .header("Accept", "application/vnd.github+json")
        .header("X-GitHub-Api-Version", "2022-11-28")
        .header("User-Agent", "sl-daemon-check-update");

    if let Some(token) = std::env::var("GITHUB_TOKEN").ok().or_else(|| std::env::var("GH_TOKEN").ok()) {
        if !token.trim().is_empty() {
            req = req.header("Authorization", format!("Bearer {}", token.trim()));
        }
    }

    let resp = req.send().await?;

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale comment

Risk: medium. Not approving: user-facing update-check behavior with GitHub API network calls exceeds the low-risk approval threshold. Cursor Bugbot was not present on this PR; human review is required.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

std::env::var("SL_CHECK_UPDATE_LATEST").ok().filter(|value| !value.trim().is_empty())
});
let latest_tag = match latest_override {
Some(tag) => tag,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SUGGESTION: --latest "" (empty override) is not filtered, unlike the SL_CHECK_UPDATE_LATEST env-var branch at line 815.

When --latest is an empty/whitespace string, Some(tag) is used verbatim, so compare_versions(installed, "") parses latest as (0,0,0) and always reports UpToDate — silently hiding a real update. Filter the override for emptiness/whitespace the same way the env var does, e.g. chain .filter(|v| !v.trim().is_empty()) on the map so an empty --latest falls through to the API (or errors explicitly).

Suggested change
Some(tag) => tag,
Some(tag) => tag,

client: &reqwest::Client,
repo: &str,
) -> Result<String, UpdateCheckError> {
let url = format!("{GITHUB_API_BASE}/repos/{repo}/releases/latest");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SUGGESTION: The repo argument is interpolated into the request URL with no format validation.

If --repo is missing the owner/name slash (e.g. SessionLedger) or contains spaces/control characters, this builds a malformed path (/repos/SessionLedger/releases/latest) that returns HTTP 404 and a generic Parse error, which is confusing for operators. Validating that repo matches ^[^/]+/[^/]+$ (and trimming) before formatting the URL would surface a clear usage error. This is user-local, so severity is low, but it improves CLI robustness.

Suggested change
let url = format!("{GITHUB_API_BASE}/repos/{repo}/releases/latest");
let url = format!("{GITHUB_API_BASE}/repos/{repo}/releases/latest");

@kilo-code-bot

kilo-code-bot Bot commented Jul 18, 2026

Copy link
Copy Markdown

Code Review Summary

Status: 8 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 8
Issue Details (click to expand)

SUGGESTION

File Line Issue
crates/sl-daemon/src/update_check.rs 54 version_cmp ignores pre-release suffix (-beta.1); a beta of the same version core is treated as equal to stable, so users on prereleases aren't notified of stable updates
crates/sl-daemon/src/update_check.rs 40 normalize_tag only strips lowercase v; uppercase V prefixes aren't normalized
crates/sl-daemon/src/update_check.rs 103 format_status hardcodes DEFAULT_REPO in the download URL; --repo override isn't reflected
crates/sl-daemon/src/main.rs 820 reqwest::Client::new() has no timeout; slow/unresponsive GitHub hangs the CLI indefinitely
crates/sl-daemon/src/update_check.rs 74 Unauthenticated GitHub API is rate-limited (60/hr); no GITHUB_TOKEN/GH_TOKEN bearer attach
crates/sl-daemon/src/main.rs 833 format_status(&status) call doesn't pass the queried repo (matches the hardcoded-URL fix above)
crates/sl-daemon/src/main.rs 818 --latest "" bypasses the empty/whitespace filter applied to the SL_CHECK_UPDATE_LATEST env var, silently reporting UpToDate and hiding a real update
crates/sl-daemon/src/update_check.rs 74 repo is interpolated into the API URL with no owner/name format validation; a malformed --repo yields a confusing HTTP 404 Parse error
Files Reviewed (13 files)
  • .github/workflows/update-check-hard.yml - new blocking CI
  • .github/workflows/update-check-soft.yml - new soft CI
  • CHANGELOG.md - entry added
  • crates/sl-daemon/src/main.rs - run_check_update wiring
  • crates/sl-daemon/src/update_check.rs - new module
  • crates/sl-daemon/tests/check_update.rs - new CLI smoke test
  • docs/adr/0001-desktop-companion-scope.md - C11 L111 cross-ref
  • docs/ops/distribution.md - update-check section
  • docs/ops/update-check.md - new ops doc
  • llms.txt - doc index entries
  • scripts/update-check-check.ps1 - new SelfCheck script
  • tests/update_check.rs - new hermetic SelfCheck wrapper

Fix these issues in Kilo Cloud

Previous Review Summary (commit 55bd403)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 55bd403)

Status: 8 Issues Found (6 pre-existing from prior review + 2 new) | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 8

The PR adds a clean, hermetic, well-tested sl-daemon check-update command (offline --latest / SL_CHECK_UPDATE_LATEST override, CI SelfCheck, docs per ADR 0001). The core logic is sound and tests pass. However, several robustness gaps remain from the prior Gemini review and two new edge cases were found. None are compile/clippy blockers, but the prerelease ordering and timeout issues are worth fixing before merge.

Issue Details (click to expand)

SUGGESTION (prior review — still open, active comments)

File Line Issue
crates/sl-daemon/src/update_check.rs 54 version_cmp ignores pre-release suffix (-beta.1); a beta of the same version core is treated as equal to stable, so users on prereleases aren't notified of stable updates
crates/sl-daemon/src/update_check.rs 40 normalize_tag only strips lowercase v; uppercase V prefixes aren't normalized
crates/sl-daemon/src/update_check.rs 103 format_status hardcodes DEFAULT_REPO in the download URL; --repo override isn't reflected
crates/sl-daemon/src/main.rs 820 reqwest::Client::new() has no timeout; slow/unresponsive GitHub hangs the CLI indefinitely
crates/sl-daemon/src/update_check.rs 74 Unauthenticated GitHub API is rate-limited (60/hr); no GITHUB_TOKEN/GH_TOKEN bearer attach
crates/sl-daemon/src/main.rs 833 format_status(&status) call doesn't pass the queried repo (matches the hardcoded-URL fix above)

SUGGESTION (new this review)

File Line Issue
crates/sl-daemon/src/main.rs 818 --latest "" bypasses the empty/whitespace filter applied to the SL_CHECK_UPDATE_LATEST env var, silently reporting UpToDate and hiding a real update
crates/sl-daemon/src/update_check.rs 74 repo is interpolated into the API URL with no owner/name format validation; a malformed --repo yields a confusing HTTP 404 Parse error
Files Reviewed (12 files)
  • crates/sl-daemon/src/main.rs - changed (run_check_update wiring)
  • crates/sl-daemon/src/update_check.rs - new module
  • crates/sl-daemon/tests/check_update.rs - new CLI smoke test
  • tests/update_check.rs - new hermetic SelfCheck wrapper
  • scripts/update-check-check.ps1 - new SelfCheck script
  • .github/workflows/update-check-hard.yml - new blocking CI
  • .github/workflows/update-check-soft.yml - new soft CI
  • docs/ops/update-check.md - new ops doc
  • docs/adr/0001-desktop-companion-scope.md - C11 L111 cross-ref
  • docs/ops/distribution.md - update-check section
  • llms.txt - doc index entries
  • CHANGELOG.md - entry added

Fix these issues in Kilo Cloud


Reviewed by step-3.7-flash · Input: 103.1K · Output: 17.9K · Cached: 472.1K

KooshaPari and others added 2 commits July 18, 2026 04:50
Add sl-daemon check-update to compare installed version against GitHub latest
release without downloading or installing. Document ADR 0001 manual-update
posture with SelfCheck anchors and soft/blocking CI smoke.
Co-authored-by: Cursor <cursoragent@cursor.com>
@KooshaPari
KooshaPari force-pushed the feat/sl-w40-update-check branch from 55bd403 to 91b517d Compare July 18, 2026 11:51

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Risk: medium. Not approving: user-facing update-check behavior with GitHub API network calls exceeds the low-risk approval threshold. Cursor Bugbot was not present on this PR; human review is required.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

@KooshaPari
KooshaPari merged commit 58eed14 into main Jul 19, 2026
71 of 72 checks passed
KooshaPari added a commit that referenced this pull request Jul 19, 2026
Conservative +2 (394→396/402): L111 update-check (#328), L112 signing-hard (#326). Held L79/L40/L7 at pillar max.

Co-authored-by: Cursor <cursoragent@cursor.com>
@KooshaPari
KooshaPari deleted the feat/sl-w40-update-check branch August 12, 2026 09:00
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.

1 participant