Skip to content

feat: online self-update with version check and daily auto-check#59

Closed
gleami wants to merge 6 commits into
rpamis:masterfrom
gleami:online-self-update
Closed

feat: online self-update with version check and daily auto-check#59
gleami wants to merge 6 commits into
rpamis:masterfrom
gleami:online-self-update

Conversation

@gleami
Copy link
Copy Markdown
Contributor

@gleami gleami commented May 31, 2026

✨ Summary

🎯 Scope

  • CLI commands (init, status, doctor, update)
  • Core installer / platform detection
  • Comet skills (assets/skills/, assets/skills-zh/)
  • Comet shell scripts (assets/skills/comet/scripts/)
  • Tests / CI
  • Documentation / changelog
  • Other:

🧪 Testing

  • pnpm build
  • pnpm lint
  • pnpm format:check
  • pnpm test
  • pnpm test -- test/ts/comet-scripts.test.ts
  • pnpm test:shell
  • Not run:

✅ Checklist

  • PR title follows Conventional Commits, for example fix: handle project-scope init
  • User-facing behavior is documented in README.md, README-zh.md, or CONTRIBUTING.md
  • CHANGELOG.md is updated when behavior changes
  • Skill changes were made in Chinese first when applicable, then synced to English
  • New scripts are included in assets/manifest.json and relevant tests
  • Shell scripts remain portable across macOS, Linux, and Windows Git Bash
  • No unrelated generated files or local artifacts are included

👀 Notes for Reviewers

虫鱼 added 2 commits May 31, 2026 22:46
- Add version-check.ts: npm registry fetch (5s timeout), semver compare,
  update check config management (~/.config/comet/update-check.json),
  startup daily auto-check with stderr notification
- Enhance update.ts: check version before updating, add --check mode
- Add performStartupCheck() to cli/index.ts for daily auto-check
- Support COMET_UPDATE_CHECK env override
- 22 tests covering config, strategy, persistence, and error handling
Copy link
Copy Markdown
Contributor

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

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 an online self-update feature for the Comet CLI, including npm registry version checking, a non-blocking startup check, and a new --check option for the update command. The feedback highlights several critical improvements: skipping the startup check during help/version commands or in non-TTY environments to prevent output corruption; resolving a logical flaw where the local changelog of the old version is displayed instead of the new version's changelog; mocking the global fetch in unit tests to avoid external network dependencies; and updating the version comparison logic to robustly handle pre-release tags containing non-numeric characters.

Comment thread src/cli/index.ts Outdated
Comment on lines +12 to +13
// Non-blocking startup version check
performStartupCheck(version);
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

Running the startup version check on every execution can corrupt the output of commands like --version or --help (which scripts often parse), print duplicate warnings when running comet update, or clutter non-interactive environments (like CI pipelines).

It is highly recommended to skip the startup check if:

  1. The user is requesting help (--help, -h) or version (--version, -V).
  2. The command is update (which already performs its own version check).
  3. The output stream is not a TTY (non-interactive/piped).
Suggested change
// Non-blocking startup version check
performStartupCheck(version);
const isHelpOrVersion = process.argv.some(arg => ['--help', '-h', '--version', '-V', 'update'].includes(arg));
if (!isHelpOrVersion && process.stderr.isTTY) {
performStartupCheck(version);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed in gleami@962d79b — the startup check now skips when:

  • --help / -h / --version / -V is requested
  • The command is update (which already has its own version check)
  • process.stdout.isTTY is false (non-interactive / CI / piped output)
    This ensures scripts parsing comet --version never see extra output, and CI pipelines stay clean.

Comment thread src/commands/update.ts Outdated
Comment on lines +194 to +203
const changelogPath = path.resolve(
path.dirname(fileURLToPath(import.meta.url)),
'..',
'..',
'CHANGELOG.md',
);
const section = await extractLatestChangelog(changelogPath);
if (section) {
log(`\n${section}\n`);
}
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

There is a logical flaw in how the changelog is displayed.

When comet update is run, it executes the currently installed version of the CLI. At this point, the package has not been updated yet. Therefore, reading CHANGELOG.md from the local directory (../../CHANGELOG.md) will extract the changelog of the currently installed (older) version, rather than the latest (newer) version that is available for update.

As a result, the user will see the changelog for the version they are already running, which defeats the purpose of showing what's new in the update. To fix this, you should either fetch the latest changelog from a remote source (e.g., via unpkg or GitHub raw) or remove/clarify this section since local file extraction before the update is not viable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right. The local CHANGELOG.md always reflects the currently installed version, so displaying it before an update shows the user their own release notes rather than what's new in the target
version — completely backwards.

Fixed in gleami@962d79b — removed extractLatestChangelog and its call site entirely. The version comparison (v0.3.5 → v0.4.0) remains as the primary update signal. Pulling
the real changelog from a remote source (unpkg, GitHub raw) would be the correct fix, but that's a heavier change and is deferred.

Comment on lines +1 to +4
import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest';
import path from 'path';
import os from 'os';
import { promises as fs } from 'fs';
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

The unit tests currently make real network requests to the npm registry (https://registry.npmjs.org/@rpamis/comet/latest).

This is a major testing anti-pattern that makes the test suite slow, flaky, and completely dependent on internet connectivity (which will fail in offline environments or restricted CI/CD pipelines).

You should mock the global fetch function using Vitest to ensure the tests are fast, deterministic, and run entirely offline.

Suggested change
import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest';
import path from 'path';
import os from 'os';
import { promises as fs } from 'fs';
import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest';
import path from 'path';
import os from 'os';
import { promises as fs } from 'fs';
beforeEach(() => {
vi.stubGlobal('fetch', vi.fn().mockResolvedValue({
ok: true,
json: async () => ({ version: '0.4.0' }),
}));
});
afterEach(() => {
vi.unstubAllGlobals();
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in gleami@962d79b — globalThis.fetch is now mocked via vi.spyOn in every test that exercises fetchLatestVersion. All tests are deterministic, run offline, and complete
in under 400ms.

The mocked scenarios cover: update available, already latest, network error (ETIMEOUT), non-200 HTTP response (429), persisted config after check, startup stderr notification, daily-skip when recently
checked, and graceful error recovery.

Comment thread src/core/version-check.ts
Comment on lines +151 to +162
export function compareVersions(v1: string, v2: string): -1 | 0 | 1 {
const parts1 = v1.split('.').map(Number);
const parts2 = v2.split('.').map(Number);

for (let i = 0; i < Math.max(parts1.length, parts2.length); i++) {
const a = parts1[i] || 0;
const b = parts2[i] || 0;
if (a < b) return -1;
if (a > b) return 1;
}
return 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.

medium

The current implementation of compareVersions splits the version string by . and maps each part to Number.

If a version string contains non-numeric characters (such as pre-release tags like 0.3.5-beta.1), Number('5-beta') will return NaN. Since any comparison with NaN (like NaN < 5 or NaN > 5) evaluates to false, the comparison logic will fail or produce incorrect results (e.g., treating 0.3.5-beta.1 as newer than 0.3.5 or vice versa).

We can make this comparison much more robust by stripping pre-release tags and safely parsing integers with a fallback to 0.

export function compareVersions(v1: string, v2: string): -1 | 0 | 1 {
  const parse = (v: string) =>
    v.replace(/^v/, '').split('-')[0].split('.').map((x) => parseInt(x, 10) || 0);
  const parts1 = parse(v1);
  const parts2 = parse(v2);

  for (let i = 0; i < Math.max(parts1.length, parts2.length); i++) {
    const a = parts1[i] || 0;
    const b = parts2[i] || 0;
    if (a < b) return -1;
    if (a > b) return 1;
  }
  return 0;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — compareVersions now strips prerelease suffixes (-*) via v.split('-')[0] before splitting on . and converting to numbers. A Number.isFinite guard provides a final safety net for any remaining
non-numeric segments.

0.3.5-beta.1 and 0.3.5 now compare as equal on the numeric axis, which is the correct behavior since the latest dist-tag on npm registry always points to the latest stable release.

虫鱼 added 2 commits May 31, 2026 23:17
extractLatestChangelog reads the current (old) version's changelog from
the local CHANGELOG.md, not the incoming version's. Displaying it before
update is misleading since the user sees their own version's notes, not
what's new. Removed the function and its call site. Version comparison
("v0.3.5 → v0.4.0") remains as the primary update signal.
@gleami gleami changed the title Online self update feat: online self-update with version check and daily auto-check May 31, 2026
@gleami
Copy link
Copy Markdown
Contributor Author

gleami commented May 31, 2026

/review

@gleami
Copy link
Copy Markdown
Contributor Author

gleami commented May 31, 2026

/gemini review

Copy link
Copy Markdown
Contributor

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

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 an online self-update feature for the Comet CLI, including npm registry version checks, update configuration management, CLI startup checks, and a new --check option for the update command. While the implementation is solid, several critical improvements are needed: first, failed network checks should still update the check timestamp to prevent a 5-second hang on every subsequent command when offline; second, the startup check should run in a detached background process to avoid blocking CLI execution; third, the version comparison logic should be enhanced to correctly handle prerelease tags; and finally, the unit tests should mock the global fetch function to avoid flaky, real network requests.

Comment thread src/core/version-check.ts
Comment on lines +190 to +198
export async function checkForUpdateAndPersist(
currentVersion: string,
): Promise<VersionCheckResult> {
const result = await checkForUpdate(currentVersion);
if (!result.error) {
await markUpdateCheckDone(result.latestVersion);
}
return result;
}
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

If the network check fails (e.g., the user is offline or behind a firewall), checkForUpdateAndPersist currently skips calling markUpdateCheckDone. This means lastCheckTime is never updated, causing every single subsequent CLI command to attempt the network check again, resulting in a 5-second hang on every command invocation.

We should update lastCheckTime regardless of whether the fetch succeeded or failed to prevent this "offline storm" and ensure the daily check interval is respected.

export async function checkForUpdateAndPersist(
  currentVersion: string,
): Promise<VersionCheckResult> {
  const result = await checkForUpdate(currentVersion);
  const config = await loadUpdateCheckConfig();
  config.lastCheckTime = Date.now();
  if (!result.error) {
    config.cachedVersion = result.latestVersion;
  }
  await saveUpdateCheckConfig(config);
  return result;
}

Comment thread src/core/version-check.ts
Comment on lines +204 to +221
export async function performStartupCheck(currentVersion: string): Promise<void> {
try {
const config = await loadUpdateCheckConfig();
if (!shouldCheckUpdate(config)) return;

const result = await checkForUpdateAndPersist(currentVersion);
if (result.hasUpdate) {
console.error(`\n ╭─ Comet Update ───────────────────────────╮`);
console.error(
` │ New version available: v${result.currentVersion} → v${result.latestVersion} │`,
);
console.error(` │ Run 'comet update' to upgrade. │`);
console.error(` ╰──────────────────────────────────────────╯\n`);
}
} catch {
// Silent — startup check must never affect CLI behavior
}
}
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

The current implementation of performStartupCheck performs a live network request during CLI startup. This blocks the CLI exit and causes a noticeable delay (up to 5 seconds) for the user. Additionally, the update notification is only shown once every 24 hours (when the network check runs), meaning users will likely miss it if they don't update immediately.

We can solve both issues by:

  1. Displaying the notification instantly on startup if config.cachedVersion is newer than currentVersion (this is a fast, local check).
  2. Running the network check in a detached background process so it never blocks the main CLI execution or exit.
export async function performStartupCheck(currentVersion: string): Promise<void> {
  try {
    const config = await loadUpdateCheckConfig();

    // 1. Display notification instantly if we have a cached update
    if (config.cachedVersion && compareVersions(currentVersion, config.cachedVersion) < 0) {
      console.error(`\\n  ╭─ Comet Update ───────────────────────────╮`);
      console.error(
        `  │  New version available: v${currentVersion} → v${config.cachedVersion}  │`,
      );
      console.error(`  │  Run 'comet update' to upgrade.           │`);
      console.error(`  ╰──────────────────────────────────────────╯\\n`);
    }

    // 2. Trigger network check in background if needed
    if (shouldCheckUpdate(config)) {
      const currentFilePath = fileURLToPath(import.meta.url);
      const workerCode = `import('${currentFilePath.replace(/\\\\/g, '/')}').then(m => m.checkForUpdateAndPersist('${currentVersion}')).catch(() => {});`;
      const child = spawn(process.execPath, ['--input-type=module', '-e', workerCode], {
        detached: true,
        stdio: 'ignore',
        windowsHide: true,
      });
      child.unref();
    }
  } catch {
    // Silent — startup check must never affect CLI behavior
  }
}

Comment thread src/core/version-check.ts
Comment thread src/core/version-check.ts
Comment on lines +151 to +163
export function compareVersions(v1: string, v2: string): -1 | 0 | 1 {
const strip = (v: string) => v.split('-')[0];
const parts1 = strip(v1).split('.').map(Number);
const parts2 = strip(v2).split('.').map(Number);

for (let i = 0; i < Math.max(parts1.length, parts2.length); i++) {
const a = Number.isFinite(parts1[i]) ? parts1[i] : 0;
const b = Number.isFinite(parts2[i]) ? parts2[i] : 0;
if (a < b) return -1;
if (a > b) return 1;
}
return 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.

medium

The current implementation of compareVersions strips prerelease tags (like -beta.1), treating them as equal to the stable version (e.g., 0.3.5-beta.1 and 0.3.5 are treated as equal). This prevents users on prerelease versions from being notified of stable updates, or vice versa.

We can improve this by comparing the presence of prerelease tags when the numeric parts are equal, ensuring stable versions are correctly identified as newer than prereleases.

export function compareVersions(v1: string, v2: string): -1 | 0 | 1 {
  const strip = (v: string) => v.split('-')[0];
  const parts1 = strip(v1).split('.').map(Number);
  const parts2 = strip(v2).split('.').map(Number);

  for (let i = 0; i < Math.max(parts1.length, parts2.length); i++) {
    const a = Number.isFinite(parts1[i]) ? parts1[i] : 0;
    const b = Number.isFinite(parts2[i]) ? parts2[i] : 0;
    if (a < b) return -1;
    if (a > b) return 1;
  }

  // If numeric parts are equal, stable version is newer than prerelease
  const hasDash1 = v1.includes('-');
  const hasDash2 = v2.includes('-');
  if (hasDash1 && !hasDash2) return -1;
  if (!hasDash1 && hasDash2) return 1;

  // If both have prereleases, compare them lexicographically as a fallback
  if (hasDash1 && hasDash2) {
    const p1 = v1.split('-')[1];
    const p2 = v2.split('-')[1];
    if (p1 < p2) return -1;
    if (p1 > p2) return 1;
  }

  return 0;
}

Comment thread test/ts/version-check.test.ts
gleami and others added 2 commits May 31, 2026 23:39
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@gleami gleami closed this May 31, 2026
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