fix: Use uv instead of pip in Makefile - #109
Conversation
Align with agent-session-analytics by using `uv sync` instead of `.venv/bin/pip install`. The pip-based venv target breaks on machines where uv manages environments. Closes #108 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewSummaryReplaces Issues FoundCriticalNone ImportantNone Suggestions
VerdictREQUEST_CHANGES - One minor suggestion about the now-redundant Automated review by Claude Code |
CI was using pip which resolves latest fastmcp, but a newer version broke the .fn attribute used in tests. Using uv sync with the lockfile ensures CI uses the same pinned dependencies as local development. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewSummaryReplaces Issues FoundCriticalNone ImportantNone Suggestions
VerdictREQUEST_CHANGES - One minor suggestion about the now-redundant Automated review by Claude Code |
The lockfile pins fastmcp to 2.14.4 — without it, CI resolves fastmcp 3.x which broke the .fn attribute API used in tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewSummaryReplaces Issues FoundCriticalNone Important
Suggestions
Previously Addressed (Filtered)None — no "Feedback Addressed" comments found from prior rounds. VerdictREQUEST_CHANGES - One important issue (stale pip-based error messages in shell scripts undermine the fix this PR provides) and two suggestions. Automated review by Claude Code |
- Update error messages in install scripts to suggest `uv sync` instead of the old `python3 -m venv && pip install` commands - Use `uv run` in Makefile fmt/lint/test targets for consistency with CI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code ReviewSummaryReplaces Issues FoundCriticalNone ImportantNone Suggestions
VerdictREQUEST_CHANGES - One suggestion about the now-redundant Automated review by Claude Code |
Feedback AddressedImplemented
Skipped
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove `source .venv/bin/activate` from dev.sh, use `uv run` instead - Add `--python` to `uv sync` in CI test matrix to guarantee each matrix entry actually tests the specified Python version Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Summary
Clean migration from pip to uv across the Makefile, CI workflows, shell scripts, and docs. All previously raised feedback has been addressed: shell script error messages now reference uv sync, Makefile targets use uv run consistently with CI, and the venv target is intentionally retained per project conventions.
Previously Addressed (Filtered)
- [Important] Stale pip error messages in shell scripts — Implemented ✓
- [Suggestion]
uv runconsistency in Makefile — Implemented ✓ - [Suggestion] Redundant
venvtarget — Skipped (intentionally kept, matches agent-session-analytics conventions) ✓
Verdict
APPROVE - All previous feedback addressed. No new issues found. The changes are straightforward, correct, and well-scoped.
Automated review by Claude Code
No reviewer feedbackclaude-review APPROVED on latest commit. All previously raised items were addressed in prior rounds. No new issues found. |
Summary
.venv/bin/pip installwithuv syncinvenv,dev,install-server, andinstall-clienttargetsvenvas prerequisite frominstall-serverandinstall-client(uv sync handles it)Closes #108
Test plan
make checkpasses (299 tests)make install-client REMOTE_URL=...works on a fresh clonemake install-serverworks on the server machine🤖 Generated with Claude Code