Skip to content

Fix: unbreak the permanently-failing update-constraints pre-commit hook - #56

Open
AmaadMartin wants to merge 6 commits into
mainfrom
fix/update-constraints-precommit-hook
Open

Fix: unbreak the permanently-failing update-constraints pre-commit hook#56
AmaadMartin wants to merge 6 commits into
mainfrom
fix/update-constraints-precommit-hook

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Problem: Every pull request fails the Pre-commit Linter job regardless of what it
changes, and the same failure reproduces locally for anyone who follows CONTRIBUTING.md
and runs pre-commit run --all-files. Three independent defects each keep it red:

  1. The hook always fires and its inputs do not exist. pre-commit/action runs
    pre-commit run --all-files, so the hook's files: ^(pyproject\.toml|constraints-.*\.txt)$
    regex is matched against the whole repo listing — pyproject.toml always matches. No
    constraints-*.txt file has ever been committed (git log --diff-filter=A -- 'constraints*.txt'
    is empty), so diff -u "$TARGET_FILE" "$NEW_FILE" runs against a nonexistent path, the
    script writes the file, and exits 1.

  2. The hook runs in update mode, which is non-deterministic by design. The entry:
    omits --check, so EXCLUDE_NEWER_DATE=$(date -d "4 days ago" ...) is recomputed on
    every run and baked into the file header. Committing the files alone would fix CI for at
    most one day. Only --check is date-stable — it re-reads the date out of the committed
    header.

  3. Root cause: the script was structurally non-idempotent, so even --check could never
    pass.
    It passed the previously committed file to uv as --constraint, and uv then
    records that file as a resolution source, injecting an annotation into the # via block
    of every constrained package:

     a2a-sdk==1.1.2
    -    # via google-adk (pyproject.toml)
    +    # via
    +    #   -c constraints-3.10.txt.stable.tmp
    +    #   google-adk (pyproject.toml)

    The committed file is produced without that annotation, so the diff is non-empty forever.
    Verified on this branch: generating all five files, then running the pre-fix script's
    --check against them immediately, reports all five OUT OF DATE and exits 1.

Separately, the lint job has no astral-sh/setup-uv step (unlike type-check and
unit-test), so uv is not guaranteed to be on PATH there at all — the reported
"Resolution failed even without constraints" was a missing tool, not a broken dependency
graph. All five interpreters resolve cleanly; no dependency versions are changed here.

Solution:

  1. scripts/update_constraints.sh — make it converge. Seed the candidate output file
    with a copy of the committed pins instead of passing them via --constraint. uv reads an
    existing output file as version preferences, which gives the same pin stability
    (anthropic stays at its committed 0.120.0 rather than floating) without polluting the
    annotations. That also makes the two-attempt "retry without constraints" ladder dead code
    — preferences can never make resolution fail — so it is deleted along with its misleading
    error message. Net -12 lines.
  2. .pre-commit-config.yaml — remove the update-constraints hook. pre-commit is a
    fast, offline formatting gate; a five-way network dependency resolution that needs uv,
    takes minutes, and mutates files does not belong in it — least of all under
    --all-files, where it fires on every unrelated PR. Deliberately not stages: [manual]
    and not a SKIP= in CI: both leave a hook nobody runs and hide the drift check rather
    than relocating it.
  3. .github/workflows/constraints-check.yml (new) — relocate the drift check to a job
    that installs uv, runs --check (report, don't rewrite), and only triggers on
    pyproject.toml, constraints-*.txt, or scripts/update_constraints.sh. Action SHAs are
    the ones already pinned in continuous-integration.yml; permissions: contents: read and
    timeout-minutes match the existing style.
  4. Commit the five constraints-3.10.txtconstraints-3.14.txt files that commit
    75c773ed intended to publish. Generated by the script, never hand-typed — --check
    parses --exclude-newer back out of the header the script reconstructs, so a hand-written
    uv invocation would silently break it.
  5. pyproject.toml [tool.codespell] skip must exempt them: codespell flags astroid
    (a real pylint dependency) as a misspelling of asteroid, 6 occurrences, exit 65.
    Without this the linter simply turns red again under a different hook. Deliberately not
    ignore-words-list = astroid, which would weaken spell-checking of real prose repo-wide.
    The existing skip rationale ("generated or data files, not prose we own") already covers
    them.
  6. README.md pointed curl at the GitHub blob page, which serves HTML rather than
    the file; switched to raw.githubusercontent.com so the documented flow actually works.

Collision check (per contribution process). gh pr list --repo AmaadMartin/adk-python --state open --limit 100 was reviewed before any code was written. Two open PRs touch this
area and neither lands this change:

This PR is not stacked on either: both adopt the manual-stage approach that this change
supersedes (a hook nobody runs still leaves the drift unchecked), and #49's branch carries
unrelated churn that must not enter this diff. It branches from current main. If #54 lands
first, the conflict is confined to the single .pre-commit-config.yaml hunk.

Why one PR and not a stack: the hand-written change is ~200 lines across 6 files; the
remaining ~7,250 lines are one inert, machine-generated artifact commit. The commits are
atomic and separately reviewable, and splitting would create broken intermediate states —
removing the hook before the files exist, or committing files the fixed script has not yet
been able to validate.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

New: tests/unittests/test_constraints_files.py (17 cases), modelled on the existing
tests/unittests/test_release_dependencies.py repo-hygiene guard and reusing its
_find_pyproject() upward walk rather than inventing a second root resolver. No network, no
subprocess, no mocks; runs in 0.07s.

$ pytest tests/unittests/test_constraints_files.py -q
17 passed

Every new test was proven able to fail by mutating the exact thing it pins:

Mutation Test killed Failure message
Delete constraints-3.10.txt test_constraints_file_exists_for_every_supported_version[3.10] constraints-3.10.txt is missing. README.md documents downloading it…
Strip --exclude-newer <date> from constraints-3.11.txt line 2 test_constraints_header_is_script_generated[3.11] line 2 is not the command ./scripts/update_constraints.sh reconstructs, so --check cannot parse its --exclude-newer date back out
Inject # -c constraints-3.12.txt.stable.tmp test_constraints_files_have_no_annotation_leakage[3.12] references a temporary constraint file: ['# -c constraints-3.12.txt.stable.tmp']
Drop "3.14" from PYTHON_VERSIONS in the script test_script_versions_match_python_classifiers scripts/update_constraints.sh and the pyproject.toml Python classifiers disagree on the supported interpreters
Re-add the update-constraints hook to .pre-commit-config.yaml test_precommit_does_not_run_constraints_script The constraints updater is back in .pre-commit-config.yaml…

Source-level mutation (the acceptance proof). With the pre-fix script from main and
these exact committed files, --check still fails — which is the bug, and confirms the shell
fix is load-bearing rather than incidental to committing the files:

$ git show main:scripts/update_constraints.sh > /tmp/mut/scripts/update_constraints.sh
$ cd /tmp/mut && ./scripts/update_constraints.sh --check
❌ constraints-3.10.txt is OUT OF DATE!   (… ×5)   exit 1
#    -c constraints-3.10.txt.stable.tmp   <- the leaked annotation, in every "# via" block

Coverage. Measured with --cov-branch over the new module. On Python 3.10 (the oldest
interpreter in the CI matrix): 96% line, 48 statements, 1 uncovered. The single uncovered
line is the pytest.skip(...) body of the not-a-source-checkout guard, which cannot fire in a
source checkout. It is not dead code and was verified manually — copying the module into a
tree that has pyproject.toml but no scripts/ produces
SKIPPED [1] … Not a full source checkout: scripts/update_constraints.sh is absent. The
tomllib/tomli import fallback is uncovered on 3.12 (92% there) but covered on 3.10, so
both branches execute across the matrix; it is the same compat shim
test_release_dependencies.py already uses and is required by the 3.10 job.

Integration — the real tooling, no mocks. Every command below was run on this branch:

# Command Result
1 bash -n scripts/update_constraints.sh exit 0
2 ./scripts/update_constraints.sh (files absent) exit 1, five files written — expected, update mode exits 1 when it writes
3 ./scripts/update_constraints.sh --check exit 0, five ✅ — this is the acceptance test; it exits 1 on main
4 ./scripts/update_constraints.sh --check (again) exit 0 — idempotent
5 ./scripts/update_constraints.sh (files present) exit 0, five ✅ — nothing rewritten
6 pre-commit run --all-files exit 0 — all 12 hooks pass, including codespell

Artifact sanity: each file starts with exactly two # lines, line 2 ends with
-o constraints-<ver>.txt, no ^#\s+-c .*\.tmp$ lines, no trailing whitespace, no CRLF, LF
line endings with a single trailing newline (so end-of-file-fixer and trailing-whitespace
leave them alone). No constraints-*.tmp files are left behind.

CI on this PR, against the base-commit baseline. The base commit 6bab08f has its own
red CI run, so here is the honest before/after rather than a bare "green":

Job On base commit 6bab08f On this PR
Pre-commit Linter failure success (1m5s) — this is the bug being fixed
Constraints Up To Date n/a (workflow is new) success (16s) — real uv, real PyPI, path filter matched
Mypy Check (3.10–3.13) success success
A2A v0.3 Tests (3.10–3.14) success success
Unit Tests (3.10–3.14) failure failure — identical, pre-existing

The Unit Tests failure is the same single test on both:
tests/unittests/cli/utils/test_cli_tools_click.py::test_telemetry_cli_commands, which fails
on all five interpreters at the base commit too. It is a Click >= 8.2 exit-code regression in
the adk telemetry CLI — unrelated to this change, already the subject of separate open PRs,
and deliberately not fixed here to avoid colliding with them.

The per-interpreter pass counts confirm nothing else moved and that the new tests really ran
in CI: 9202 → 9219, 9204 → 9221 (×3), 9211 → 9228+17 passed on every interpreter,
exactly the 17 cases in test_constraints_files.py, with 1 failed unchanged.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

  1. The user-facing promise in README.md — the published artifact is installable. Ran
    against a real Python 3.10 environment and PyPI:

    uv venv /tmp/adk-c --python 3.10
    /tmp/adk-c/bin/pip install --dry-run google-adk -c constraints-3.10.txt   # exit 0

    Resolves with no conflict. Also confirm the download line itself now works:
    curl -o constraints-3.10.txt https://raw.githubusercontent.com/google/adk-python/main/constraints-3.10.txt
    returns the file rather than an HTML page (the blob URL it replaces does not).

  2. The lint gate is green from a clean checkout — the bug reproduction from main:

    pre-commit run --all-files          # exit 1 on main, exit 0 here
    ./scripts/update_constraints.sh --check   # exit 1 on main, exit 0 here (run it twice)
  3. The new workflow's path filter. Constraints Up To Date should appear on this PR
    (it touches pyproject.toml, constraints-*.txt, and scripts/update_constraints.sh) and
    must not appear on a PR that only touches src/**/*.py.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 6 commits August 1, 2026 07:00
`--check` could never pass, even on files generated seconds earlier. The
committed file was handed to uv as `--constraint`, so uv recorded it as a
resolution source and injected a `# -c constraints-<ver>.txt.stable.tmp`
line into the `# via` block of every constrained package. The committed
files carry no such annotations, so the diff was non-empty forever.

Seed the candidate with a copy of the committed file instead: uv reads an
existing output file as version *preferences*, which keeps pins just as
stable without polluting the annotations.

That also makes the two-attempt "retry without constraints" ladder dead
code -- preferences can never make resolution fail -- so it is removed
along with its misleading "Resolution failed even without constraints"
message.
README documents `pip install google-adk -c constraints-3.10.txt`, but no
constraints file has ever been committed, so the documented flow 404s and
`./scripts/update_constraints.sh --check` reports all five as missing.

Generated with ./scripts/update_constraints.sh (never by hand: `--check`
parses the `--exclude-newer` date back out of the header the script
reconstructs).

codespell has to skip them: it flags `astroid`, a real pylint dependency,
as a misspelling of `asteroid`, which would turn the lint gate red again
under a different hook. The existing skip rationale -- generated files,
not prose we own -- already covers them.
The CI lint job runs `pre-commit run --all-files`, and the hook's `files:`
regex matches `pyproject.toml`, so the constraints updater fired on every
pull request regardless of what it touched. That job installs no uv and the
script resolves five dependency graphs against PyPI, so the gate was both
slow and permanently red -- pre-commit is meant to be a fast, offline
formatting pass.

Relocate the check to its own workflow that installs uv and only triggers
when its actual inputs change, and run it in `--check` mode so it reports
drift instead of rewriting files.
The README pointed `curl` at the GitHub blob page, which serves an HTML
document rather than the constraints file, so the downloaded file could
never be passed to `pip -c`.
Pins the invariants whose absence kept the lint gate red: a constraints
file exists for every interpreter the script generates, its header is the
one the script reconstructs (so `--check` can read the `--exclude-newer`
date back out), no `-c ...tmp` annotation leaked in from the old
`--constraint` handling, and pre-commit no longer shells out to the
network-dependent update script.
Rebasing onto main picked up langgraph>=1.0.10,<2, langgraph-checkpoint
>=4.1.1,<5 and google-genai>=2.12.1, which the previously generated files
predate: they pin langgraph==0.4.7, so `uv pip install -c constraints-<ver>.txt`
resolves to nothing and `./scripts/update_constraints.sh --check` reports
permanent drift.

Regenerated with the script, not hand-edited. langgraph moves 0.4.7 -> 1.2.10.
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.

1 participant