Skip to content

feat(cli): show version update notice#602

Open
eric-tramel wants to merge 8 commits intomainfrom
codex/issue-598-version-update-notice
Open

feat(cli): show version update notice#602
eric-tramel wants to merge 8 commits intomainfrom
codex/issue-598-version-update-notice

Conversation

@eric-tramel
Copy link
Copy Markdown
Contributor

@eric-tramel eric-tramel commented May 4, 2026

📋 Summary

Adds a PyPI-backed update notice to data-designer --version while preserving the installed version as the first output line. The notice is intentionally opportunistic: scripted/non-TTY output, local development versions, network failures, malformed PyPI data, and cache failures all skip the CTA instead of disrupting version output.

🔗 Related Issue

Fixes #598

🔄 Changes

  • Added CLI version update detection against PyPI with fail-closed behavior, DATA_DESIGNER_DISABLE_VERSION_CHECK, and DATA_DESIGNER_VERSION_CHECK_PRERELEASES.
  • Rendered the optional update notice as a compact Rich panel after the plain installed version line.
  • Skipped update-notice lookup for non-TTY stdout so data-designer --version remains script-friendly.
  • Skipped update notices for local/dev installed versions that should not compare against public PyPI releases.
  • Hardened PyPI release parsing for yanked releases, malformed release file records, malformed payloads, and invalid version strings.
  • Added package-specific, schema-versioned cache reads and atomic cache writes.
  • Selected uv tool upgrade data-designer by default, uv add --upgrade data-designer for project environments, and pipx upgrade data-designer for pipx installs.
  • Tightened upgrade-command path detection so project venvs under paths containing uv/tools are not misclassified as uv-tool installs.
  • Addressed review feedback by simplifying the install-path suffix helper and moving test seams to public/injected helpers instead of private monkeypatches.
  • Added packaging as a direct runtime dependency for PEP 440 version comparison.
  • Added tests for newer/current versions, lookup failure, non-TTY output, invalid/local installed versions, opt-out, cache hit/expiry/schema mismatch, malformed PyPI data, prerelease handling, and upgrade command selection.

Usage Examples

Local development checkout output, captured from this branch:

$ uv run --package data-designer data-designer --version
0.5.10.dev7+fc0365ca

Opted-out version check output, captured from this branch:

$ DATA_DESIGNER_DISABLE_VERSION_CHECK=1 uv run --package data-designer data-designer --version
0.5.10.dev7+fc0365ca

Interactive update-available rendering, captured from the CLI --version path with the installed version set to 0.5.10 and latest version set to 0.5.11 for deterministic output:

$ data-designer --version
0.5.10
╭─ 🚀 Update available ───────────────────────╮
│ New Data Designer version available: 0.5.11 │
│ Upgrade with: uv tool upgrade data-designer │
╰─────────────────────────────────────────────╯

🧪 Testing

  • make test passes (not run; interface package suite run instead)
  • Unit tests added/updated
  • E2E tests added/updated (N/A - no E2E surface changed)
  • make check-interface
  • make test-interface (682 passed)
  • uv run --package data-designer pytest packages/data-designer/tests/cli/test_main.py packages/data-designer/tests/cli/test_version_notice.py (30 passed)
  • uv run --package data-designer data-designer --version
  • DATA_DESIGNER_DISABLE_VERSION_CHECK=1 uv run --package data-designer data-designer --version
  • Deterministic CLI --version update-available rendering run

✅ Checklist

  • Follows commit message conventions
  • Commits are signed off (DCO)
  • Architecture docs updated (N/A - no architecture docs impacted)

Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
@eric-tramel eric-tramel changed the title [codex] feat(cli): show version update notice feat(cli): show version update notice May 4, 2026
@eric-tramel eric-tramel marked this pull request as ready for review May 4, 2026 17:57
@eric-tramel eric-tramel requested a review from a team as a code owner May 4, 2026 17:57
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

PR #602 Review — feat(cli): show version update notice

Summary

Adds a PyPI-backed update notice that renders after the installed version line on data-designer --version. Includes a 0.75s-timeout HTTPS fetch to the PyPI JSON API, a 6h on-disk cache under DATA_DESIGNER_HOME, prerelease filtering, a DATA_DESIGNER_DISABLE_VERSION_CHECK opt-out, and a DATA_DESIGNER_VERSION_CHECK_PRERELEASES override. Selects between uv tool upgrade data-designer and uv add --upgrade data-designer based on sys.prefix / environment variables. Adds packaging>=25,<27 as a direct runtime dep and a new version_notice module plus unit tests.

Fixes #598.

Findings

Correctness

  • get_update_notice always prints version first_version_callback echoes the installed version before importing the notice modules, so the core --version contract is preserved even if the notice path fails. Good.
  • Yanked-release heuristic edge case (version_notice.py:139-142): _is_yanked_release returns True when release_files is [] (no files) — reasonable, but it returns False when the list contains any non-dict entry (because isinstance(..., dict) and ... short-circuits to False, causing all(...) to be False). Non-dict entries shouldn't occur in practice, but the inversion is subtle; consider any(is_valid_and_not_yanked) framing instead.
  • Dev / local-version installs trigger spurious notices: a user with e.g. 0.6.1.dev0+gabc1234 has is_prerelease == True, so the check enables prereleases and then compares the local version to PyPI's latest prerelease. Version("0.6.1.dev0+abc") < Version("0.6.1rc1") is true, so editable developers will get nagged to upgrade their own working tree. Minor, but worth a one-line skip for versions with local segments:
    if installed.local is not None:
        return None
  • Upgrade-command heuristic is uv-only: users who installed via pipx install data-designer or pip install --user data-designer will see uv tool upgrade data-designer or uv add --upgrade data-designer, both of which won't work for them. If cross-installer support matters, consider a pipx prefix check (".local/pipx/venvs" in parts) or a generic fallback like pip install -U data-designer. Acceptable given the uv-centric positioning, but noting it.
  • Cache key lacks a schema version: cache_data stores checked_at/include_prereleases/latest_version but no schema_version or package_name. If the key semantics ever change, stale caches will be silently misread. Low risk right now; a one-field bump field is cheap insurance.
  • Cache file write is non-atomic (_write_cached_version): write_text on the final path means a concurrent data-designer --version could read a half-written file. _read_cached_version swallows json.JSONDecodeError so it's safe, but a tmp-file+rename would avoid the wasted network roundtrip. Optional.

Performance / UX

  • 0.75s synchronous block on first invocation (and every 6h thereafter) before --version returns. The installed version is printed first, so the user sees output immediately, but the process doesn't exit for up to 750ms. Consider a shorter default (e.g. 0.5s) or an env-var override for CI environments that see thousands of --version calls.
  • No stdout/TTY gating: the notice prints via Rich even when stdout is piped (e.g. data-designer --version | cat). Version-checkers in scripts will get a Rich panel in their output on upgrade-available days. A sys.stdout.isatty() or not sys.stdout.isatty() skip in _version_callback would make this safer for scripting.
  • Fail-closed is appropriate: all failure modes (timeout, HTTP error, invalid JSON, InvalidVersion) return None and print nothing. Good.

Style / conventions

  • Absolute imports, from __future__ import annotations, full type annotations — all match the style guide.
  • Deferred imports inside _version_callback keep the fast path clean. Matches the lazy_heavy_imports posture in AGENTS.md.
  • Private helpers are underscore-prefixed and module-local; public surface is just UpdateNotice, get_update_notice, select_upgrade_command.
  • Mapping[str, str] and Callable[[], float] injection for environ / now make the tests clean without monkeypatching globals.

Tests

  • Coverage of core branches is thorough: newer version, current version, fetch failure, opt-out, fresh cache, prerelease filtering (both opt-in paths), and upgrade-command selection.
  • Gaps to consider:
    • No test for yanked-release filtering (the _is_yanked_release path).
    • No test for an invalid installed_version (e.g., "garbage") — the early InvalidVersion return is untested.
    • No test for cache-expired → refetch (opposite of the fresh-cache test).
    • No test for the PyPI payload being malformed at the top level (releases missing / not a dict).
    • test_app_version_prints_installed_data_designer_version only asserts result.output == "0.6.0\n"; stable against regressions, but the "version-first ordering" invariant isn't explicitly tested with a non-None notice in cli/main.py's own tests (covered in test_version_notice.py separately).

Security

  • Uses HTTPS to pypi.org with Accept: application/json and a named User-Agent; no credentials sent. Fine.
  • No shell expansion / subprocess — upgrade command is a suggestion string only, never executed. Good.
  • Cache file written under DATA_DESIGNER_HOME (user home by default). No symlink follow attacks beyond what pathlib.write_text already has.

Verdict

Approve with minor comments. The implementation is clean, well-tested, and defensive in the right places (fail-closed, opt-out, version-first echo, deferred imports). The issues above are mostly polish:

  • Skip notice for local-version dev installs (recommended).
  • Skip notice when stdout is not a TTY (recommended — avoids breaking scripts parsing --version).
  • Reconsider the uv-only upgrade suggestion if pipx/pip users are in scope.
  • Add tests for yanked releases, invalid installed versions, and cache expiry.
  • Consider a schema-version field in the cache payload.

None of these block merging.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 4, 2026

Greptile Summary

This PR adds an opportunistic PyPI-backed update notice to data-designer --version. The installed version is always printed first; the update panel is only shown on interactive TTYs and skipped silently on any error, non-TTY stream, local/dev version string, or opt-out env var.

  • version_notice.py (new): implements PyPI fetch, a 6-hour schema-versioned atomic cache, PEP 440 version comparison via packaging, and upgrade-command selection that correctly distinguishes uv-tool installs, pipx envs, and project venvs using a suffix-adjacency heuristic.
  • main.py: splits the old single-line version callback into a two-phase flow — print installed version, then optionally fetch and render the update notice wrapped in a Rich panel.
  • Tests: 30 tests covering the notice path, cache hit/expiry/schema-mismatch, malformed PyPI payloads, prerelease opt-in, all upgrade-command branches, and non-TTY / lookup-failure / opt-out skip paths.

Confidence Score: 5/5

Safe to merge — the update notice is fully opportunistic and never disrupts the version output line that scripts depend on.

All error paths (network failure, malformed payload, invalid version string, cache I/O error, non-TTY) return None and leave the installed version as the sole output. The _path_ends_with_segments suffix helper correctly resolves the upgrade-command classification concern from the previous review round, and the test suite covers every meaningful branch including the adjacency edge cases.

No files require special attention.

Important Files Changed

Filename Overview
packages/data-designer/src/data_designer/cli/version_notice.py New module implementing PyPI version lookup, schema-versioned atomic cache, PEP 440 comparison, and upgrade-command heuristic — all with fail-closed semantics. Logic and edge cases are correct.
packages/data-designer/src/data_designer/cli/main.py Version callback split into TTY-gated two-phase flow; lazy imports keep startup fast; generic Exception catch around the notice fetch is intentional and correct for opportunistic behavior.
packages/data-designer/src/data_designer/cli/ui.py Adds print_update_notice using Rich Text.assemble and Panel.fit with existing Nord theme colors — consistent with surrounding UI helpers.
packages/data-designer/tests/cli/test_version_notice.py Comprehensive new test file covering 20+ scenarios including cache hit/expiry/schema mismatch, malformed payloads, prerelease opt-in, all upgrade-command branches, and opt-out paths.
packages/data-designer/tests/cli/test_main.py Existing version test updated and four new integration-level tests added for the notice flow; patches target the correct module namespaces.
packages/data-designer/pyproject.toml Adds packaging>=25,<27 as a direct runtime dependency; lock file updated successfully, confirming the constraint resolves.

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as data-designer CLI
    participant PyPI as PyPI JSON API
    participant Cache as version-check.json

    User->>CLI: data-designer --version
    CLI->>CLI: importlib.metadata.version()
    CLI->>User: print installed version (always)
    CLI->>CLI: should_show_update_notice() → isatty()?
    alt non-TTY stdout
        CLI->>CLI: raise typer.Exit()
    else TTY stdout
        CLI->>Cache: read cached version
        alt cache hit (fresh, schema match)
            Cache-->>CLI: latest_version
        else cache miss / expired
            CLI->>PyPI: GET /pypi/data-designer/json (0.75s timeout)
            alt network / parse error
                PyPI-->>CLI: exception → None
            else success
                PyPI-->>CLI: releases payload
                CLI->>CLI: filter yanked, parse versions
                CLI->>Cache: atomic write (pid.tmp → rename)
            end
        end
        CLI->>CLI: latest > installed?
        alt update available
            CLI->>CLI: select_upgrade_command()
            CLI->>User: Rich panel with latest version + upgrade command
        end
        CLI->>CLI: raise typer.Exit()
    end
Loading

Reviews (7): Last reviewed commit: "Merge branch 'main' into codex/issue-598..." | Re-trigger Greptile

Comment thread packages/data-designer/src/data_designer/cli/version_notice.py Outdated
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
@nabinchha
Copy link
Copy Markdown
Contributor

Thanks for putting this together, @eric-tramel — really nice attention to fail-closed behavior and cache hygiene throughout.

Summary

Adds an opportunistic --version update notice that queries PyPI, caches the result for 6 hours, and renders a Rich panel only when stdout is a TTY. The implementation matches the stated intent in the PR description, including local/dev-version skipping, prerelease opt-in, and install-method-aware upgrade commands.

Findings

Warnings — Worth addressing

packages/data-designer/src/data_designer/cli/version_notice.py:101-105_has_direct_child_path can be both simpler and clearer

  • What: The body iterates range(len(parts) - 1) but the predicate index + 2 == len(parts) - 1 only ever holds for index == len(parts) - 3, so the loop is effectively a single comparison on the path suffix. The current name (_has_direct_child_path) also doesn't reflect what's actually being checked: "the path ends in …/{parent}/{child}/<one segment>".
  • Why: Today's form forces a reader to mentally unfold any(...) plus a suffix-position predicate to recover a one-line check. That obscures the heuristic and makes it easier to break later (e.g. someone "fixing" the loop bound, or extending it to a different suffix shape, would have to re-derive the invariant).
  • Suggestion: Collapse to a direct suffix check, and rename to something that names the shape it detects (e.g. _path_ends_with_segments):
    def _path_ends_with_segments(parts: tuple[str, ...], parent: str, child: str) -> bool:
        """Return True when ``parts`` ends in ``…/{parent}/{child}/<one segment>``."""
        return len(parts) >= 3 and parts[-3] == parent and parts[-2] == child

packages/data-designer/tests/cli/test_version_notice.py and test_main.py — Tests reach into private API

  • What: The tests patch and call several _-prefixed helpers directly: monkeypatch.setattr(version_notice, "_fetch_latest_version", ...) is used in ~10 tests, version_notice._latest_version_from_pypi_payload(...) is called in 4 tests, and test_main.py patches data_designer.cli.main._should_show_update_notice.
  • Why: STYLEGUIDE.md is explicit on this: "Prefer public over private for testability: Use public functions (no _ prefix) for helpers that benefit from direct testing." Reaching into private API also couples the tests to module structure — a future refactor that renames or inlines one of these helpers will break tests despite no behavior change.
  • Suggestion: Promote the seams the tests actually need to public surface. Two concrete options that fit the existing dependency-injection style of get_update_notice:
    • Add a fetch_latest_version: Callable[..., str | None] | None = None parameter to get_update_notice (alongside the existing now, cache_dir, environ, python_prefix) so tests inject a fake fetcher rather than monkeypatching a private symbol.
    • Rename _latest_version_from_pypi_payload to latest_version_from_pypi_payload (it's already a pure function with no I/O — a natural public helper).
    • For _should_show_update_notice, either rename to should_show_update_notice or inject the stream/decision as an argument to _version_callback's helper.

packages/data-designer/tests/cli/test_version_notice.py:241-247 — Suffix logic for uv tool lacks a direct unit test

  • What: test_select_upgrade_command_treats_project_venv_under_uv_tools_as_project is matched by the earlier prefix.name == ".venv" branch in select_upgrade_command, so it never actually exercises the _has_direct_child_path("uv", "tools") adjacency-with-suffix check. The uv tool happy-path test only covers the canonical …/uv/tools/data-designer layout.
  • Why: The whole point of the recent "tighten upgrade command detection" commit is that uv and tools appearing non-consecutively, or with extra segments after tools, must not be misclassified. Without a test that hits the suffix branch with a non-matching path, that protection is silently regression-prone.
  • Suggestion: Add at least one test where python_prefix contains uv and tools non-consecutively (e.g. /Users/me/uv/code/tools/pkg) and one where …/uv/tools/<name>/<extra> has an extra trailing segment, asserting both fall through to _UV_TOOL_UPGRADE_COMMAND only via the final fallback rather than via the heuristic. Same idea would be worth doing for the pipx/venvs pair.

Suggestions — Take it or leave it

packages/data-designer/src/data_designer/cli/main.py:41-45 — Broad except Exception around the lookup

  • What: The fail-closed try/except Exception in _version_callback is intentional and well-commented, but the project style guide says we should avoid catching Exception without re-raising, and get_update_notice already wraps the expected I/O failure modes (HTTPError, URLError, TimeoutError, OSError, JSONDecodeError).
  • Why: A narrower catch documents the surfaces we actually expect to fail (network, JSON, filesystem) and would still surface a real programming bug instead of silently swallowing it on every invocation.
  • Suggestion: Tighten to something like except (OSError, ValueError, RuntimeError): — happy to leave Exception if you prefer the absolute "version output never breaks" guarantee, but if so a one-line comment pointing to the style-guide deviation would help future readers.

packages/data-designer/src/data_designer/cli/main.py:20-22 — TTY check still fires in many CI environments

  • What: _should_show_update_notice only checks stream.isatty(), which is true in plenty of CI runners (GitHub Actions allocates a pty for many steps, GitLab CI with tty: true, Docker -t, etc.).
  • Why: We could end up rendering the panel — and reaching out to PyPI — in CI runs where the user explicitly didn't ask for it.
  • Suggestion: Consider also short-circuiting when common CI env vars are set (CI, GITHUB_ACTIONS, BUILDKITE, JENKINS_URL, etc.). If you'd rather keep it minimal, the DATA_DESIGNER_DISABLE_VERSION_CHECK opt-out is sufficient — just worth noting that CI users will need to be told.

packages/data-designer/src/data_designer/cli/main.py:17 and version_notice.py:21_PACKAGE_NAME duplicated

  • What: _PACKAGE_NAME = "data-designer" is defined in both modules.
  • Why: Minor DRY nit — the two are coupled (cache package_name field, importlib.metadata.version call, and PyPI URL all need to agree).
  • Suggestion: Lift the constant to one module (probably version_notice.py) and import from main, so a future rename touches one line.

architecture/cli.md — No mention of the new --version side effect

  • What: The architecture doc enumerates entry-point behavior on startup but doesn't mention the new opportunistic PyPI lookup or the env vars (DATA_DESIGNER_DISABLE_VERSION_CHECK, DATA_DESIGNER_VERSION_CHECK_PRERELEASES).
  • Why: The PR description marks architecture docs as "N/A", which is reasonable for the layering, but this is a new CLI-visible behavior with documented opt-outs that contributors will want to know about.
  • Suggestion: A short subsection (or even a bullet under "Entry Point") describing the version check, its opt-out, and the cache location. Could also be a follow-up — not blocking.

What Looks Good

  • The fail-closed design is consistent end to end: invalid installed versions, malformed PyPI payloads, yanked-only releases, network errors, malformed cache, schema-version skew, and write failures all degrade silently to "no notice." The narrow exception list at the network boundary (HTTPError, URLError, TimeoutError, OSError, json.JSONDecodeError) is exactly the right size.
  • Cache design is well thought through — schema versioning, package-name keying, prerelease-aware caching, atomic temp-file writes with cleanup on failure, and a 6-hour TTL all make sense and are individually tested.
  • Test coverage is genuinely thorough: happy path, current-version no-op, lookup failure, invalid/local installed versions, opt-out, fresh cache hit, expired cache, schema mismatch, prerelease handling (both via installed prerelease and env flag), and per-environment upgrade-command selection. Easy to extend.
  • Lazy-importing version_notice and ui.print_update_notice inside _version_callback keeps the cost of the new feature off the path that doesn't need it, and _is_version_request correctly skips the bootstrap for the fast path.

Verdict

Needs changes — the private-API test coupling and the test gap around the suffix heuristic are worth addressing before merge; the _has_direct_child_path simplification is small and pairs naturally with the test fix. None of the Suggestions are blocking.

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.

feat(cli): show update notice when a newer Data Designer version is available

2 participants