Align package metadata extras and Python classifiers#575
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f940a02031
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dgenio
left a comment
There was a problem hiding this comment.
Review: Align package metadata extras and Python classifiers (#473)
Nicely scoped change — the classifier-drift guard is well-built and well-tested, and every acceptance criterion in #473 is met except the CI is currently red on a gating step.
Blocker — regenerate the committed LLM docs
README.md is a source document for the generated llms-full.txt (it's first in the DOC list in scripts/gen_llms.py), and the install-extras edits change content that is mirrored into that artifact, which was not regenerated.
- Verified locally:
python scripts/gen_llms.py --check→ exit 1,llms drift detected in: llms-full.txt. - The committed
llms-full.txtstill contains the removed| `contextweaver[cli]` | Deprecated no-op alias… |row and lacks the newtokenizersrow + the install-contract paragraph. make llms-checkis a gating CI step (perdocs/agent-context/workflows.md), so the PR showsmergeable_state: blocked.
Fix: run make llms and commit the regenerated llms.txt / llms-full.txt. This is the same point the automated Codex review raised inline on README.md. Once that artifact is committed, this is an easy approve.
What's solid (verified)
- Classifiers ↔ CI matrix in sync:
read_pyproject_python_classifiersandread_ci_python_versionsboth return[3.10, 3.11, 3.12, 3.13];check_readme_version.pypasses._version_keysorts numerically, so3.10correctly orders after3.9. - Drift guard detects both missing and extra versions; new tests cover both directions, the in-sync case, and
[project]-table scoping. - Extra removals (
cli/ann/graph) are clean — no dangling references in code or docs, and[all]was updated. (Thehnswlibmention inrouting/router.pyis an unrelated comment on the embeddings late-import path, not the removed[ann]extra.) [cli]removal is policy-compliant: deprecated since v0.5, far beyond the one-minor-release window indocs/stability.md.
Non-blocking note
_CI_PYTHON_MATRIX_RE matches the first python-version: [...] list in ci.yml. That's correct today (the gating test matrix is first), but it would silently validate against the wrong list if a job with a different matrix were ever added above it. Consider anchoring to the test job's matrix or asserting a single match. Non-blocking.
Decision: REQUEST_CHANGES
The implementation is correct and complete against #473; the only thing standing between this and approval is the regenerated llms-full.txt/llms.txt. Run make llms, commit, and ping for re-review.
Generated by Claude Code
dgenio
left a comment
There was a problem hiding this comment.
Re-review: Align package metadata extras and Python classifiers (#473)
The prior blocker is resolved. Verified on 4a3b0a3:
python scripts/gen_llms.py --check→ llms.txt and llms-full.txt are up to date (the regeneration in commit4a3b0a3).llms-full.txtnow carries thetokenizersrow + install-contract paragraph and drops the[cli]row.python scripts/check_readme_version.py→ in sync (0.14.1); classifiers3.10–3.13match the CI matrix (ci.yml:26) exactly.pytest tests/test_check_readme_version.py→ 10 passed (4 new tests cover both drift directions, the in-sync case, and[project]-table scoping).hnswlib/networkxfully removed frompyproject.toml;[all]correctly dropsann/graph.
All #473 acceptance criteria are met and the approach (extending the existing check_readme_version.py gate, per the issue's suggestion §5) is clean and idiomatic.
Decision: APPROVE
Two non-blocking should-fix items + one merge prerequisite below.
Should-fix (minor) — removed extras still referenced internally
The [cli]/[ann]/[graph] extras are gone from the install contract and all user-facing surfaces (README, llms, CHANGELOG, workflows), but two internal references were left behind. No runtime impact; worth a quick sweep. (Posted here rather than inline because these files aren't part of this PR's diff.)
src/contextweaver/__main__.py:26— the module docstring still says "the legacy[cli]extra is kept as an empty alias for one cycle", which now contradicts this PR (the extra was removed). Suggest: state that[cli]is removed (Typer/Rich remain core)..github/copilot-instructions.md:49— lists[cli],[ann],[graph]among optional extras; all three are removed. Suggest dropping them from the example list to keep the house-rules doc in sync withpyproject.toml.
Merge prerequisite (not a code issue) — base drift
The branch is ~17 commits behind main with real conflicts in CHANGELOG.md, docs/agent-context/workflows.md, and llms-full.txt (mergeable_state: dirty). After rebasing, re-run make llms — main has since moved llms-full.txt — and confirm make llms-check passes before merge.
Non-blocking note (carried from prior review)
_CI_PYTHON_MATRIX_RE matches the first python-version: [...] list in ci.yml. Correct today (the gating test matrix is first; the ci.yml:150 scalar "3.10" has no brackets and is skipped), but it would silently bind to the wrong list if a bracketed matrix were ever added above it. Optional hardening: anchor to the test job's matrix.
Generated by Claude Code
…extras-473 Signed-off-by: wuyangfan <yangfan.wu@succaiss.com> # Conflicts: # CHANGELOG.md # docs/agent-context/workflows.md # llms-full.txt
|
Addressed the merge prerequisite in 1607bd8. What changed:
Validation:
|
|
Verified
My approval stands. Only nit: Generated by Claude Code |
Closes #473
Summary
[cli]extra and reserved[ann]/[graph]extras that installed dependencies without enabling code pathsreadme-version-checkto detect classifier/matrix driftNotes
mainis version0.14.1, so the checker and docs were aligned to the live repository state rather than the older0.14.0text in the issue.Verification
uvx --from ruff ruff format --check scripts/check_readme_version.py tests/test_check_readme_version.pyuvx --from ruff ruff check scripts/check_readme_version.py tests/test_check_readme_version.py/Users/wuyangfan/.local/bin/python3.11 scripts/check_readme_version.pytmpdir=$(mktemp -d); ln -s /Users/wuyangfan/.local/bin/python3.11 "$tmpdir/python"; PATH="$tmpdir:$PATH" make readme-version-checkuvx --with . --with pytest --with pytest-asyncio python -m pytest tests/test_check_readme_version.py -qgit diff --check