Skip to content

Fix version resolution and detect install conflicts - #334

Open
ump45nose wants to merge 3 commits into
ARPAHLS:mainfrom
ump45nose:fix/333-version-resolver-install-conflicts
Open

ump45nose wants to merge 3 commits into
ARPAHLS:mainfrom
ump45nose:fix/333-version-resolver-install-conflicts

Conversation

@ump45nose

Copy link
Copy Markdown

Description

Fixes #333 — the CLI prints vNone / skillware None when a maintainer mixes editable and PyPI installs on the same Python and pip leaves duplicate/orphan skillware distribution metadata.

The root cause is _package_version_str() / get_installed_version() treating corrupt metadata as a valid "None" string and the fallback returning importlib.metadata.version("skillware") verbatim. This PR hardens both, adds conflict detection, and surfaces it via skillware doctor --install plus a startup advisory.

Changes

  • skillware/version_policy.py
    • get_installed_version() now treats "", "dev", and literal "None" as invalid (returns None).
    • Added detect_install_conflicts() — flags duplicate distributions, missing Metadata-Version, unparseable versions, orphan .dist-info dirs (no METADATA/RECORD), and editable+wheel coexistence on one interpreter.
    • Added InstallConflict dataclass with code, summary, and Windows/Unix fix commands.
  • skillware/cli.py
    • _package_version_str() never returns None or empty — final fallback is "dev".
    • Added emit_install_conflict_advisory() — one dim stderr line on CLI startup when conflicts are detected.
    • Added skillware doctor --install — reports install version + health, lists each conflict with copy-paste Windows/Unix fix commands. Exit 0 = healthy, 1 = conflicts.
  • CONTRIBUTING.md — new "Editable vs PyPI on the same Python" subsection documenting one-mode-per-interpreter, uninstall-before-switching, and orphan cleanup.
  • CHANGELOG.md — entries under [Unreleased].
  • docs/usage/cli.md — documents skillware doctor --install.

Type of Change

  • Bug Fix — incorrect runtime or framework behavior
  • CLI — skillware/cli.py, docs/usage/cli.md

Checklist (all PRs)

  • Linked GitHub issue (Fixes #333)
  • Scope matches the issue — no unrelated refactors
  • python -m black --check . passes locally
  • flake8 . passes locally
  • pytest tests/ passes for affected modules (tests/test_cli.py, tests/test_version_policy.py — 87 passed)
  • CHANGELOG.md updated under [Unreleased] (user-visible behavior change)
  • No skills/ files changed (skill checklist omitted)

Testing

  • python -m black --check . → all files unchanged.
  • python -m flake8 . → clean.
  • python -m pytest tests/test_version_policy.py tests/test_cli.py -q → 87 passed.
  • Real editable install on this machine is detected correctly: skillware doctor --install reports the editable+wheel mix and duplicate distribution that actually exist in the local venv (exit 1), and the startup advisory prints when conflicts exist.

Note: the full pytest tests/ / pytest skills/ suites require the [dev,all] extras (google-genai, playwright, PIL, etc.) that are not installed in this local venv; CI installs them via pip install -e ".[dev,all]" and remains authoritative.

AI assistance

This contribution was drafted with AI assistance. Per the repository's Agent Code of Conduct, no AI tool or agent is added to Co-authored-by: trailers.

Related Issues

@rosspeili

Copy link
Copy Markdown
Contributor

Thanks @ump45nose, good fix approach.

Please comment on #333 and claim the issue first (GFI, unclaimed duplicates clutter the backlog).

Before merge:

  1. Rebase on current main (CHANGELOG conflict with feat(creative): add deck_builder skill for deterministic PPTX assembly (#276) #331/feat(cli): add theme subcommand and align help topics with context/ch… #335).
  2. skillware config show, install-health block per [Bug]: CLI shows vNone / skillware None when editable and PyPI installs overlap on the same Python #333.
  3. scripts/dev_install.ps1 + scripts/dev_install.sh.
  4. Unit tests for detect_install_conflicts() (orphan, duplicate, editable+wheel), not just CLI mocks.
  5. test_cli_main, assert install advisory runs once on startup.
  6. Help ripples: HELP_GROUPS, --help doctor examples, cli.md version-check note; align Windows editable fix with .[dev,all].
  7. SKILLWARE_NO_VERSION_CHECK, install advisory respects opt-out or document why not.
  8. PR description maps each [Bug]: CLI shows vNone / skillware None when editable and PyPI installs overlap on the same Python #333 acceptance criterion → file/test.

Re-run pytest tests/ after rebase, CI is authoritative. Overall good chunk of work, thanks for transparency with AI usage. We generally highly encourage AI use, as long as you take ownership of commits <3

@ump45nose
ump45nose force-pushed the fix/333-version-resolver-install-conflicts branch from 7d362b2 to da757de Compare September 4, 2026 15:08
@rosspeili

Copy link
Copy Markdown
Contributor

Thanks for the rebase onto main, CHANGELOG is clean now and the core #333 fix still looks good (vNone hardening, doctor --install, startup advisory). Targeted tests pass on my side too.

Still required before merge which were unchanged from prior review:

  1. skillware config show, install-health block (version + conflict summary + pointer to doctor --install).
  2. scripts/dev_install.ps1 + scripts/dev_install.sh, orphan cleanup + pip install -e ".[dev,all]".
  3. Unit tests for detect_install_conflicts() in tests/test_version_policy.py, orphan dist-info, duplicate registrations, editable+wheel (not only CLI mocks).
  4. test_cli_main, assert emit_install_conflict_advisory() runs once on startup (parity with upgrade advisory).
  5. Help ripples, HELP_GROUPS, brief --help doctor examples, cli.md version-advisory section note for install conflicts.
  6. SKILLWARE_NO_VERSION_CHECK, install advisory respects opt-out or document why not.
  7. Fix-command consistency, Windows editable path should use -e ".[dev,all]" (matches CONTRIBUTING + Unix fix).
  8. Comment on [Bug]: CLI shows vNone / skillware None when editable and PyPI installs overlap on the same Python #333 and claim the issue if you haven't already. This is a must if you want this PR to be considered for merge.

Please map each #333 acceptance criterion → file/test in the PR description when you push the next update. <3

@rosspeili

Copy link
Copy Markdown
Contributor

Hey @ump45nose let me know if you need help with this, or I can push a maintainer wrap up so we can merge this? thanks.

- 合并 main 并保留版本解析兜底、安装状态输出与 opt-out 行为
- 增加开发安装脚本、单元覆盖及 CLI 文档
@ump45nose
ump45nose force-pushed the fix/333-version-resolver-install-conflicts branch from da757de to 943bc55 Compare September 16, 2026 10:29
@rosspeili

Copy link
Copy Markdown
Contributor

Hey @ump45nose please try to rebase again, or I can do before merge regardless of state when you push, but we need to comment and claim #333, this is a requirement from prior review, docs/usage/api_keys.md, extend the SKILLWARE_NO_VERSION_CHECK note to cover install-conflict advisories too (cli.md already documents both; keep them aligned), re run full ci equivalent check on the rebased branch if needed or see if ci is green.

  • Optional tests for missing-metadata-version and unparseable-version conflict codes (logic exists; duplicate/orphan/editable-wheel are already covered).

Once these are in, I will merge, and rebase if needed <3

…tall-conflicts

Upstream has since landed its own install-health implementation
(`assess_install_health` / `InstallHealth` / `format_install_health_report`),
which covers every case this branch set out to fix — duplicate
registrations, orphan dist-info, editable/wheel mixes and unparseable
versions — with a richer report. This branch's `detect_install_conflicts`
API is therefore retired rather than kept alongside it.

All conflicts take upstream's side. `tests/test_cli.py` keeps only the two
assertions upstream lacks (the shared advisory opt-out suppressing
install-conflict output, and `config show` surfacing the install section),
ported onto the new API; the five it duplicates are dropped, since
`tests/test_version_policy.py` already covers them.

No test that passed before fails now.
Refs ARPAHLS#333. Clarify the existing shared advisory switch in the environment-variable reference.
@ump45nose

Copy link
Copy Markdown
Author

Addressed the remaining documentation point in a548689: docs/usage/api_keys.md now explicitly says SKILLWARE_NO_VERSION_CHECK=1 suppresses both version and install-conflict advisories, with a changelog entry. The existing opt-out regression test is included in validation.

Local validation: 135 tests passed across test_version_policy.py, test_cli.py, test_registry_docs.py, and test_skill_docs.py; Black and Flake8 passed on the version-policy/CLI test paths; git diff --check passed. This was focused validation, not the complete optional-dependency suite. Hosted CI on the new head still needs to run.

The follow-up was AI-assisted. No implementation or package-version changes were made in this update.

This branch has not been deployed

No deployments
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.

[Bug]: CLI shows vNone / skillware None when editable and PyPI installs overlap on the same Python

2 participants