Repository navigation
build: pin the full dependency tree with uv instead of requirements.txt - #88
Conversation
requirements.txt pinned four packages. The environment actually resolves to 40: pydantic, mcp, httpx, httpcore, jsonschema, cryptography, starlette, sse-starlette, attrs, certifi and the rest arrived unpinned and were free to move between image rebuilds. That directly contradicted the file's own comment, which said the SDK loop is the harness of this system and an unpinned harness lets a rebuild silently change agent behaviour. The intent was right; pinning only the direct dependencies could not deliver it. uv.lock does. The drift is not hypothetical: syncing an existing developer environment against the new lockfile moved uvicorn from 0.46.0 to 0.52.1. Notes on the specific choices: - `package = false`: there is no importable distribution to build, and the image runs `python -m orchestrator.run_all` against the copied source tree. Without it uv tries to build a wheel and fails. - The image installs with `--no-dev`. `dev` is a default group in uv, so without that flag pytest (and later ruff and mypy) would ship to production. Verified both ways: with --no-dev the interpreter cannot import pytest while the runtime dependencies are present; without it, pytest is there for CI. - The image puts /app/.venv on PATH rather than using `uv run`, which re-checks the environment per call and can pull the dev group back in. - `version` in pyproject.toml is a placeholder. release-please runs with release-type "simple", which does not rewrite version strings in source files, so a realistic value would rot one release after being written; .release-please-manifest.json stays the single source of truth. - uv itself is version-pinned in both the image and CI: an unpinned installer is the same class of drift the lockfile exists to prevent. Also updates the three places that referenced the deleted file. One of them is the implementer sub-agent prompt for this repository, which already told the agent to add dependencies to pyproject.toml and not to hand-edit requirements*.txt — advice that pointed at a file which did not exist until now. It now describes the lockfile workflow and drops `pip install -e .`, which does not apply to a non-installable project.
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 2m 5s —— View job Review complete
Verdict: Approved — This is a clean, well-reasoned build/tooling change (no application logic touched): Verified:
One non-blocking nit posted inline:
No P1/P2 findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| RUN pip install --no-cache-dir -r requirements.txt | ||
| # uv, pinned by version like everything else here: an unpinned installer is | ||
| # the same class of drift the lockfile exists to prevent. | ||
| COPY --from=ghcr.io/astral-sh/uv:0.11.11 /uv /usr/local/bin/uv |
There was a problem hiding this comment.
P3 — uv itself is pinned by registry tag (ghcr.io/astral-sh/uv:0.11.11), not by digest. Docker tags aren't guaranteed-immutable the way uv.lock hashes are, so this is the one remaining link in the "exact pins" chain that a registry-side retag could still move. Given the PR's own stated rationale (an unpinned installer is the same class of drift the lockfile prevents), pinning with @sha256:... alongside the tag would close that gap. Not blocking — astral-sh version tags are practically immutable — just noting the inconsistency with the PR's stated philosophy.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b790cd025a
ℹ️ 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".
| # No --no-dev here, unlike the Dockerfile: CI wants the dev group, | ||
| # which is where pytest lives. --frozen fails if the lockfile is stale | ||
| # rather than silently re-resolving. | ||
| run: uv sync --frozen |
There was a problem hiding this comment.
When a contributor changes pyproject.toml without regenerating uv.lock, this step can pass while testing the old dependency tree. uv sync --help defines --frozen as “Sync without updating the uv.lock file,” while --locked asserts that the lockfile remains unchanged; reproducing this with the PyYAML pin changed made the frozen command exit successfully and retain 6.0.3. The Dockerfile repeats the same flag, so a dependency-only update could be silently ignored in both CI and the production image; use --locked where stale-lock detection is required.
Useful? React with 👍 / 👎.
|
|
||
| ```bash | ||
| pip install -r requirements.txt | ||
| uv sync # installs from uv.lock, including the dev group (pytest) |
There was a problem hiding this comment.
On a fresh checkout where the dependencies are not also installed globally, uv sync creates .venv but does not activate it or alter the current shell's PATH. The subsequent bare python -m orchestrator... commands therefore use the system interpreter and fail while importing dependencies such as anyio or claude_agent_sdk; activate .venv here or prefix those commands with uv run.
Useful? React with 👍 / 👎.
|
|
||
| ```bash | ||
| pip install -r requirements.txt | ||
| uv sync |
There was a problem hiding this comment.
For contributors who only have the listed Python and Docker prerequisites, this first setup command fails with uv: command not found because uv is a separate tool and is not installed by Python. Add a pinned uv installation step or list uv explicitly in the prerequisites so the new local-development workflow is usable from a clean machine.
Useful? React with 👍 / 👎.
Review finding (P3). The PR argues that an unpinned installer is the same class of drift the lockfile prevents, then pinned uv by registry tag — and a tag can be moved after the fact in a way uv.lock hashes cannot. That was the one remaining soft link in the chain. Verified the digest resolves to an OCI image index covering linux/amd64 and linux/arm64, so this does not tie the build to a single architecture, and that a stage copying from it builds and runs uv.
|
P3 addressed: uv is now pinned by digest ( The finding was right and it landed on the PR's own argument — claiming an unpinned installer is the same class of drift the lockfile prevents, while leaving the installer on a movable tag, was inconsistent. Checked before pinning that the digest resolves to an OCI image index covering |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 3m 10s —— View job Review complete
Verdict: Changes requested — The digest-pin commit ( While reviewing the full diff again, I independently verified Codex's findings and agree with two of them:
No issues found with |
| # No --no-dev here, unlike the Dockerfile: CI wants the dev group, | ||
| # which is where pytest lives. --frozen fails if the lockfile is stale | ||
| # rather than silently re-resolving. | ||
| run: uv sync --frozen |
There was a problem hiding this comment.
P2 — --frozen doesn't catch a pyproject.toml/uv.lock mismatch; --locked is the flag that does.
Per uv sync --help, --frozen means "sync without updating uv.lock" — it installs whatever the lockfile already pins and does not check that it's still consistent with pyproject.toml. --locked is the one that asserts the lock is up to date and errors otherwise. The comment above this line ("--frozen fails if the lockfile is stale rather than silently re-resolving") describes --locked's behavior, not --frozen's — --frozen only fails if uv.lock is entirely missing.
Concretely: if a contributor bumps a version bound in pyproject.toml (e.g. PyYAML) without running uv lock, this step installs the old pinned version from the stale lock and both the test job and the Docker smoke build pass — silently testing/shipping a tree that doesn't match what's declared in pyproject.toml. That's exactly the class of drift this whole PR exists to close.
Since this tests job is the merge gate and the Dockerfile build reuses the same checked-out lockfile, switching just this line to uv sync --locked would catch the drift before merge without needing to touch the Dockerfile.
|
|
||
| ```bash | ||
| pip install -r requirements.txt | ||
| uv sync # installs from uv.lock, including the dev group (pytest) |
There was a problem hiding this comment.
P2 — the commands below this (python -m orchestrator.run_service_agent, run_mentor, run_all, run_issue_investigator) assume the locked venv is active, but uv sync alone doesn't put .venv on PATH or activate it. On a fresh checkout without these packages installed globally, the bare python -m ... invocations will fail importing anyio/claude_agent_sdk/etc., using the system interpreter instead of .venv.
Prefix these with uv run (matching how CI invokes pytest at .github/workflows/pr-validation.yml:68) or add an explicit source .venv/bin/activate step, so the documented quick-start actually works as written.
|
|
||
| ```bash | ||
| pip install -r requirements.txt | ||
| uv sync |
There was a problem hiding this comment.
P3 — Prerequisites (above, unchanged) lists Python 3.12+ and Docker but not uv, even though this is now the first setup command. A contributor following this doc top-to-bottom on a clean machine hits uv: command not found with no pointer to fix it. Worth a one-liner in Prerequisites (e.g. link to https://docs.astral.sh/uv/getting-started/installation/), ideally noting the pinned version (0.11.11) used by CI/the image.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 2 P2, 1 P3. uv sync --frozen in the tests job doesn't verify uv.lock is still in sync with pyproject.toml (that's --locked's job), so a pyproject-only dependency edit could pass CI silently on a stale lock — the exact drift this PR sets out to close. README's post-uv sync commands run bare python -m ... without activating .venv or using uv run, so they'll fail on a fresh checkout. P3: CONTRIBUTING's prerequisites don't mention uv itself. The digest-pinned uv base image (this commit) looks correct.
| # No --no-dev here, unlike the Dockerfile: CI wants the dev group, | ||
| # which is where pytest lives. --frozen fails if the lockfile is stale | ||
| # rather than silently re-resolving. | ||
| run: uv sync --frozen |
There was a problem hiding this comment.
P2 — --frozen doesn't catch a pyproject.toml/uv.lock mismatch; --locked is the flag that does.
Per uv sync --help, --frozen means "sync without updating uv.lock" — it installs whatever the lockfile already pins and does not check that it's still consistent with pyproject.toml. --locked is the one that asserts the lock is up to date and errors otherwise. The comment above this line ("--frozen fails if the lockfile is stale rather than silently re-resolving") describes --locked's behavior, not --frozen's — --frozen only fails if uv.lock is entirely missing.
Concretely: if a contributor bumps a version bound in pyproject.toml (e.g. PyYAML) without running uv lock, this step installs the old pinned version from the stale lock and both the test job and the Docker smoke build pass — silently testing/shipping a tree that doesn't match what's declared in pyproject.toml. That's exactly the class of drift this whole PR exists to close.
Since this tests job is the merge gate and the Dockerfile build reuses the same checked-out lockfile, switching just this line to uv sync --locked would catch the drift before merge without needing to touch the Dockerfile.
|
|
||
| ```bash | ||
| pip install -r requirements.txt | ||
| uv sync # installs from uv.lock, including the dev group (pytest) |
There was a problem hiding this comment.
P2 — the commands below this (python -m orchestrator.run_service_agent, run_mentor, run_all, run_issue_investigator) assume the locked venv is active, but uv sync alone doesn't put .venv on PATH or activate it. On a fresh checkout without these packages installed globally, the bare python -m ... invocations will fail importing anyio/claude_agent_sdk/etc., using the system interpreter instead of .venv.
Prefix these with uv run (matching how CI invokes pytest at .github/workflows/pr-validation.yml:68) or add an explicit source .venv/bin/activate step, so the documented quick-start actually works as written.
…ands Three review findings, two of them flagged independently by Claude and Codex — same conclusion from two different reviewers is a strong signal. **--frozen does not catch a stale lockfile; --locked does (P2, both bots).** My own comment in this PR claimed --frozen "fails if the lockfile is stale rather than silently re-resolving" — that description is actually --locked's behavior. Verified empirically: bumped a version bound in pyproject.toml without regenerating uv.lock, then ran both flags against the stale lock. `uv sync --frozen` exited 0 and installed the old pinned version anyway; `uv sync --locked` exited 1 with "The lockfile at uv.lock needs to be updated". So a contributor who edits pyproject.toml and forgets to run uv lock would have sailed through both the tests job and the Docker build, silently testing and shipping a tree that doesn't match what's declared — exactly the class of drift this PR exists to close. Switched CI and the Dockerfile to --locked; re-verified the real lockfile still passes and the 234 tests still run. **README quick-start commands fail on a fresh checkout (P2, both bots).** `uv sync` creates .venv but does not put it on PATH or activate it, so the bare `python -m orchestrator...` commands below it would use the system interpreter and fail importing anyio/claude_agent_sdk on a machine without those installed globally. Prefixed each with `uv run`, matching how CI invokes pytest. **uv missing from Prerequisites (P3, Claude).** It's the first command in Local Development now; added it to Prerequisites with the pinned version CI and the image use.
|
All three findings addressed — thanks, and worth noting Claude and Codex independently flagged the same two P2s, which is a good signal both were real.
README quick-start (P2, both bots) — fixed. uv missing from Prerequisites (P3) — fixed. Added with the pinned version. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 1m 8s —— View job Review complete
Verdict: Approved — Commit
Confirmed No P1/P2 findings, no P3 nits this round. |
There was a problem hiding this comment.
No P1/P2 findings (0 P3). All three prior findings resolved: CI and Dockerfile now use --locked (verified: --frozen was confirmed empirically to silently accept a stale lock, --locked correctly rejects it), README's quick-start commands are prefixed with uv run, and CONTRIBUTING lists uv as a prerequisite. pyproject.toml/uv.lock unchanged since the prior approved review. Good to merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bab544606
ℹ️ 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".
| - New Python dependency? Add it to `dependencies` in `pyproject.toml` | ||
| (or to the `dev` group if it is tooling), then run `uv lock` and | ||
| commit `uv.lock` in the same change. Verify with `uv sync`. Never | ||
| hand-edit `uv.lock`, and note the project is not installable |
There was a problem hiding this comment.
When the implementer adds a dependency in a fresh clone, uv sync creates the project environment but does not activate it—the updated README.md:56-57 explicitly notes this—while this prompt's sanity checks at lines 65-68 still invoke bare python and pytest. Those checks can therefore use the system interpreter and fail to import the new dependency, or test against stale globally installed packages; instruct the agent to activate .venv or use uv run for every sanity command.
Useful? React with 👍 / 👎.
What
Replaces
requirements.txtwithpyproject.toml+uv.lock, and switchesboth the image and CI to
uv.Why
requirements.txtpinned 4 packages. The environment actually resolves to40 —
pydantic,mcp,httpx,httpcore,jsonschema,cryptography,starlette,sse-starlette,attrs,certifiand the rest arrived unpinnedand were free to move between image rebuilds.
That contradicted the file's own comment:
The intent was right. Pinning only the direct dependencies could not deliver
it, because the harness is the whole tree.
uv.lockdoes.The drift is not hypothetical. Syncing an existing developer environment
against the new lockfile moved
uvicornfrom 0.46.0 to 0.52.1 — a packagenobody ever chose, arriving transitively, at a version nothing recorded.
Worth being explicit about what this is not for: the speed argument is
weak here. The Dockerfile is dominated by
apt-get install nodejs npm git ghand a global npm install, not by installing four Python packages. Expect no
meaningful build-time change from this PR. The reason to do it is
reproducibility.
Notes on specific choices
package = false— there is no importable distribution to build, and theimage runs
python -m orchestrator.run_allagainst the copied source tree.Without this, uv tries to build a wheel and fails.
--no-dev.devis a default group in uv, sowithout that flag pytest — and later ruff and mypy — would ship to
production. Verified in both directions (see below).
/app/.venvonPATHinstead of usinguv run, whichre-checks the environment on each call and can pull the dev group back in.
It also makes
pythonmean the locked interpreter for anything the agentsrun through their Bash tool.
versionis a placeholder. release-please runs with release-typesimple, which maintains the manifest and changelog but does not rewriteversion strings in source files. A realistic value here would rot one
release after being written, so
.release-please-manifest.jsonstays thesingle source of truth.
the same class of drift the lockfile exists to prevent.
Documentation kept in step
Three places referenced the deleted file. The third is worth calling out:
agents/mctl-agents/.claude/agents/implementer.md— the implementersub-agent's own prompt for this repository — already said
That instruction pointed at a file which did not exist until this PR. It
now describes the lockfile workflow and drops
pip install -e ., which doesnot apply to a non-installable project.
Related
#42 ("fix: add upper bounds to Python dependencies") is superseded by this
PR and is also stale. It proposes
claude-agent-sdk>=0.1.68,<0.2, butmainhas since moved to the exact pin
==0.2.110, so its bound now conflictswith what is on main. Its stated goal — "so the harness can't silently break
between builds" — is what a lockfile actually achieves, since upper bounds
still leave the transitive tree free. Suggest closing it; not doing so here,
as it is not mine to close.
Verification
uv lockresolves cleanly — 40 packages,requires-python = "==3.12.*"uv sync --frozenthenuv run --frozen pytest -q→ 234 passeduv sync --frozen --no-dev→ interpreter cannot import pytest, whileclaude_agent_sdk,anyio,dotenvandyamlall import fineuv sync --frozen(no flag) → pytest present, as CI needsrequirements.txtanywhere in the repo