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 Summary

This PR adds a PyPI-backed version update notice to data-designer --version, rendered as a Rich panel only on interactive TTY output. The implementation is carefully hardened against all failure modes (network errors, malformed data, yanked releases, Python incompatibility, local dev versions) and includes atomic cache writes, schema-versioned cache invalidation, and environment-aware upgrade command selection.

• version_notice.py introduces the full version check pipeline: PyPI fetch with a 0.75 s timeout, a 6-hour TTL cache (keyed by schema version, package name, Python version, and prerelease flag), packaging-backed PEP 440 comparison, and per-installer upgrade command selection via a _path_ends_with_segments tail-matching heuristic.
• main.py and ui.py wire the notice into the --version callback, guarded behind a TTY check and a broad exception catch so version output remains script-safe.
• test_version_notice.py adds 26 focused unit tests covering the major code paths, edge cases, and environment detection scenarios.

Important Files Changed

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

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.

All actionable feedback from Andre, Nabin, Greptile, and the agentic CI review pass has been addressed in this PR.

Summary of the cleanup:

• Hardened version lookup behavior so failures remain fail-closed and do not disrupt the installed-version output.
• Added TTY gating, local/dev-version skipping, cache schema/package/Python-version guards, atomic cache writes, and PyPI payload hardening.
• Added pip-aware upgrade guidance while preserving uv-tool, uv project, and pipx-specific commands.
• Filtered PyPI release candidates by requires_python compatibility.
• Reduced private test coupling by adding public/injected seams where appropriate.
• Added coverage for cache expiry/schema mismatch, yanked/malformed releases, invalid/local versions, prerelease handling, urlopen success/failure, and upgrade-command detection edge cases.

Validation is green locally and on the required GitHub checks. The broader GitHub matrix was green when last checked; Greptile was the only external check still pending at that point.

Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance.