Skip to content

fix(ci): run the test suite on the Python the images actually ship (#1891) - #1910

Merged
vybe merged 3 commits into
devfrom
fix/1891-ci-python-313
Aug 3, 2026
Merged

fix(ci): run the test suite on the Python the images actually ship (#1891)#1910
vybe merged 3 commits into
devfrom
fix/1891-ci-python-313

Conversation

@dolho

@dolho dolho commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Closes #1891

What

Every Trinity image is FROM python:3.13; every CI job executing the suite pinned 3.11. Bumps the six literals, adds a guard so the two declarations cannot silently diverge again — and fixes a real defect the bump surfaced.

File Jobs
backend-unit-test.yml 3 (lint, pytest matrix, regression diff)
backend-unit-nightly.yml · schema-parity.yml · pg-migrations.yml 1 each

publish-cli.yml stays 3.12 — PyPI packaging is governed by what CLI consumers run, not the container runtime. Allowlisted in the guard with that reason.

The bump surfaced a real defect — which is the point of it

I ran CI's exact invocation on both interpreters (cd tests && pytest unit/ -m "not slow" -p randomly --randomly-seed=12345) and diffed with the repo's own scripts/ci/diff-pytest-failures.py:

| Side | Path             | Total | Pass | Fail | Error | Skip |
| base | keep-py311.xml   |  5155 | 5094 |    7 |    37 |   17 |
| head | keep-py313.xml   |  5149 | 5087 |    7 |    37 |   18 |

## ✅ No new failures

No new failures — but 3.13 ran six fewer tests.

audioop-lts was declared only in docker/backend/Dockerfile:76, never in tests/requirements-test.txt. CI installs the requirements file, not the image. So on 3.13 there is no audioop, tests/unit/test_voip_audio.py skips at collection, and 7 VoIP codec tests silently stop running:

only in 3.11 (7):   test_voip_audio.TestFraming::…  (all [pass])
                    test_voip_audio.TestInbound::…
                    test_voip_audio.TestOutbound::…
                    test_voip_audio.TestStatefulContinuity::…
only in 3.13 (1):   ::test_voip_audio  [skip]
status changed on shared tests (0)

Nothing would have gone red. The regression diff job — the very job this issue's AC says to read — compares failures, not test counts. A whole vanished module is invisible to it. Bumping the interpreter without this pin would have closed one instance of "CI environment ≠ shipped environment" while quietly opening another.

So this PR also pins audioop-lts; python_version >= '3.13' in tests/requirements-test.txt. Verified: 7 passed on 3.13 with it.

The guard (the durable half)

tests/unit/test_1891_python_version_parity.py, three rules:

  1. All three image Dockerfiles pin the same python:<major>.<minor>.
  2. Every python-version: in .github/workflows/ equals that pin — a scan of every workflow file, not six known line numbers, so a seventh added next month with a stale pin fails too.
  3. Every pkg; python_version >= X the image installs is also a test requirement. Matching the interpreter is only half of "CI runs what we ship"; rule 3 exists because rule 2 alone caused the audioop loss above.

Plus a stale-allowlist check, so a renamed/deleted exempt workflow can't leave a dead entry a future file inherits by name.

Verified in both directions — green as committed; red when I revert a pin to 3.11 (pg-migrations.yml:68 pins 3.11 (images ship 3.13)) and red when I delete the audioop-lts line.

One thing I got wrong first, worth calling out

Rule 3's first implementation was vacuous on arrival: it substring-matched the whole requirements file, and my own explanatory comment above the pin contains the string audioop-lts — so deleting the real requirement line still matched and the guard stayed green. Caught it only because I ran the negative test. It now strips comments and parses requirement names (_declared_test_requirements). Same lesson as learnings.md 2026-07-29: a guard's own machinery is load-bearing code and needs its own negative test.

Not a 3.13 memory regression

Local full-tree runs OOM'd, which looked alarming. Both interpreters OOM identically — it is the slow tests (which CI excludes via -m "not slow") plus my container, not the version. Both wrote complete JUnit XML before the kill; the kill lands after the session ends, which is why the diff above is valid.

Docs

architecture.md said Python 3.11 for Backend, Agent runtime, and the base-image line — all three corrected to 3.13, plus a note naming the Dockerfile as source of truth and the guard as enforcement.

Residual, stated plainly

The 7 fail / 37 error baseline is pre-existing on both interpreters (the #660 documented set) and unchanged by this PR. CI's own regression diff on this PR is the authoritative check.

🤖 Generated with Claude Code

…1891)

Every Trinity image is `FROM python:3.13`; every CI job executing the suite
pinned 3.11. CI therefore validated a runtime two minor versions behind
production and, by construction, could not catch the stdlib-removal class —
which had already shipped twice (`crypt` -> #1615, `audioop` -> the audioop-lts
VoIP pin).

Bumps the six literals (backend-unit-test x3, backend-unit-nightly,
schema-parity, pg-migrations). publish-cli stays 3.12: PyPI packaging is
governed by what CLI consumers run, not by the container runtime.

The durable half is the guard. Six literals fix today's drift and prevent
nothing; the root cause is two hand-maintained declarations with nothing tying
them together. `tests/unit/test_1891_python_version_parity.py` derives CI's
expected version from the Dockerfile pin and fails when they diverge, scanning
EVERY workflow file so a seventh one added later cannot ship stale.

The bump surfaced a real defect, which is the point of it
-------------------------------------------------------
Verified by running CI's exact invocation (`cd tests && pytest unit/
-m "not slow" -p randomly --randomly-seed=12345`) on both interpreters and
diffing with the repo's own scripts/ci/diff-pytest-failures.py:

    base keep-py311.xml  5155 total  5094 pass  7 fail  37 error
    head keep-py313.xml  5149 total  5087 pass  7 fail  37 error
    ## No new failures

No new failures — but 3.13 ran SIX FEWER TESTS. `audioop-lts` was declared only
in docker/backend/Dockerfile, never in tests/requirements-test.txt. CI installs
the requirements file, not the image, so on 3.13 `tests/unit/test_voip_audio.py`
skipped at collection and 7 codec tests silently stopped running.

Nothing would have gone red: the regression-diff job compares FAILURES, not test
counts, so a vanished module is invisible to it. Bumping the interpreter without
this pin would have closed one instance of "CI environment != shipped
environment" while opening another.

So this also pins audioop-lts in tests/requirements-test.txt (verified: 7 passed
on 3.13 with it) and extends the guard with a third rule — every
`pkg; python_version >= X` the image installs must also be a test requirement.

Guard verified in both directions, and the first version of that third rule was
VACUOUS: it substring-matched the whole requirements file, and the explanatory
comment above the pin contains the string "audioop-lts", so deleting the real
line still matched. It now strips comments and parses requirement names.

Not a 3.13 memory regression: both interpreters OOM identically in a local
container when the whole tree is run (including slow tests, which CI excludes).
Both wrote complete JUnit XML first; the kill lands after the session ends.

Docs: architecture.md tech-stack table said 3.11 for Backend and Agent runtime,
and the base-image line too — all corrected, plus a note naming the Dockerfile as
source of truth and the guard as enforcement.

Closes #1891

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dolho
dolho requested a review from AndriiPasternak31 as a code owner July 31, 2026 09:14
…#1891)

Review of the guard shipped one commit earlier: rule 3 enumerated ONE dep-install
surface while claiming to close the class. There are three, and they do not all
look alike:

  docker/backend/Dockerfile        inline `pip install`, shell-quoted spec
  docker/scheduler/requirements.txt  a requirements FILE, no shell quoting
  docker/base-image/Dockerfile     the agent container's runtime

Two independent defects fell out of that:

1. `docker/scheduler/requirements.txt` was never read, so a conditional dep added
   there would be invisible — the exact "the create path is never one call site"
   shape in learnings.md 2026-07-29, and inconsistent with rule 1 in the SAME
   file, which already enumerates all three Dockerfiles.
2. `_MARKER_RE` demanded quotes around the whole spec, which only the Dockerfile
   form has. Even pointed at the scheduler file it would have matched nothing —
   a guard that reads the file and still sees zero is worse than one that never
   opens it, because the "no markers found" tripwire stays quiet.

Now scans backend + scheduler, with a regex accepting both forms. base-image is
deliberately excluded WITH a reason in the code: those packages are the agent
container's runtime and the backend unit suite never imports them; the agent-side
modules it does import are vendored stdlib-only by Invariant #5.

Adds the two meta-tests the rule was missing, mirroring the workflow allowlist's:
`test_every_image_dep_source_still_exists` (a renamed source must fail loudly
rather than shrink coverage) and `test_conditional_dep_exemptions_are_still_real`.

Verified: red when a conditional dep is added to the scheduler requirements
("some-conditional-pkg (docker/scheduler/requirements.txt pins it for python >=
3.13)") — the surface that was unwatched before this commit — and green as
committed. Run the way CI runs it (`cd tests && pytest unit/`).

Related to #1891

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dolho

dolho commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

/review Report

Branch: fix/1891-ci-python-313dev (merge-base 5658e67a)
Files Changed: 7 (+274/−9)
Scope: CLEAN
Plan Completion: 5 done / 0 partial / 0 not done / 0 changed / 0 unverifiable

Self-review. One critical finding, in the guard I wrote — fixed in ea3c9d82.

Scope check

Intent (#1891): bump 6 CI python-version literals 3.11 → 3.13, triage anything the bump surfaces, add a guard so the two declarations cannot diverge again, fix the stale tech-stack table.
Delivered: exactly that. The audioop-lts pin in tests/requirements-test.txt is not creep — it is AC #3 ("any newly-surfaced 3.13 failure is triaged: fixed, or filed and xfail'd") discharged by fixing.

All five ACs DONE with citations: bumps (backend-unit-test.yml:50,88,147, backend-unit-nightly.yml:125, schema-parity.yml:101, pg-migrations.yml:68); regression-diff read, not matrix status; surfaced failure triaged and fixed; guard added; docs corrected.


Critical Findings

[C1] Guard blind spot: rule 3 enumerated one dep source of three (Confidence: 10/10) — FIXED in ea3c9d82

File: tests/unit/test_1891_python_version_parity.py (as of 808c7f2d)

Evidence — what the guard read:

def _conditional_deps() -> dict[str, str]:
    """Map dep name -> required python_version, from the backend Dockerfile."""
    text = (_ROOT / "docker" / "backend" / "Dockerfile").read_text(encoding="utf-8")

Evidence — what actually installs backend-side Python:

docker/backend/Dockerfile:38        RUN pip install --no-cache-dir \
docker/scheduler/Dockerfile:16      RUN pip install --no-cache-dir -r requirements.txt
docker/base-image/Dockerfile:111    python3 -m pip install --user \

Issue: rule 3 claimed to close "the image installs a version-conditional dep that CI lacks" while watching one of three surfaces. docker/scheduler/requirements.txt was never opened.

This is the repo's own named bug class — learnings.md 2026-07-29 (#1871): "a guard written to close a 'never one call site' class must enumerate the API SURFACE, not one function name." Worse, rule 1 in the same file already enumerates all three Dockerfiles (_IMAGE_DOCKERFILES), so the inconsistency was internal to one 251-line test.

A second defect rode along and would have defeated the fix on its own:

_MARKER_RE = re.compile(r"""["']([A-Za-z0-9._-]+)\s*;\s*python_version\s*>=?\s*['"](\d+\.\d+)['"]["']""")

The outer ["'] require shell quoting, which only the Dockerfile form has ("audioop-lts; python_version >= '3.13'"). A requirements file writes it bare. So even pointed at the scheduler file the regex would have matched nothing — and the "no markers found ⇒ vacuous" tripwire would have stayed quiet, because the backend Dockerfile still supplies a match.

Why it matters: a guard that silently watches less than it claims converts an unchecked area into a believed-checked one, which is worse than no guard.

Fix applied: scan backend + scheduler; regex accepts both quoted and bare forms; base-image excluded with the reason in code (agent-container runtime, never imported by the backend unit suite; the agent-side modules that are imported are vendored stdlib-only per Invariant #5). Added the two meta-tests the rule lacked, mirroring the workflow allowlist's: test_every_image_dep_source_still_exists and test_conditional_dep_exemptions_are_still_real.

Verified red on the previously-unwatched surface:

E  some-conditional-pkg (docker/scheduler/requirements.txt pins it for python >= 3.13)
1 failed, 5 passed

and green as committed (6 passed), run the way CI runs it: cd tests && pytest unit/.


Informational Findings

[I1] Marker forms still unmatched: <, !=, and python_full_version (Confidence: 7/10)
The regex now accepts >, >=, ==. A dep pinned python_version != '3.13' or via python_full_version is not matched. <-markers are correctly irrelevant (they exclude the shipped version by construction), and !=/python_full_version appear nowhere in the repo today. Left unhandled deliberately — broadening the regex on speculation risks the same silent-miss it just cost me. The "no markers found" assertion is the backstop if the file's style changes wholesale.

[I2] _declared_test_requirements does not follow -r includes (Confidence: 7/10)
tests/requirements-test.txt has no -r lines today, so every declaration is in the one file. If it ever splits, a dep declared in an included file would read as missing and the guard would false-fire — noisy rather than silent, which is the right failure direction, but worth knowing.

[I3] The not slow marker is load-bearing and unstated in CI (Confidence: 6/10)
backend-unit-test.yml runs -m "not slow". My full-tree local runs OOM'd on both 3.11 and 3.13 precisely because they included slow tests. Nothing is wrong in the diff; recording it because the next person reproducing CI locally will hit the same wall, and the workflow does not say why the marker is there.


Clean Categories

  • SQL/data safety — no SQL, no DB code, no migration in the diff.
  • Race/concurrency — no shared state; the guard is a pure static file scan.
  • Auth boundaries — no endpoints, dependencies, or permission logic touched.
  • Credential exposure — the diff adds no secrets; the guard reads only Dockerfiles, workflow YAML and a requirements file, and its assertion messages print dep names and file paths only.
  • Enum completeness — no enum/status/type constant introduced.
  • Error handling — no except added; the guard's file reads are intentionally unguarded so a missing file fails loudly (that is test_every_image_dep_source_still_exists's job).
  • Test placement — lives in tests/unit/, which is the only path CI's cd tests && pytest unit/ collects (the learnings.md 2026-07-30 trap of putting a guard where CI never looks). Verified by running it that exact way.
  • Docs stalenessarchitecture.md was itself stale here (3.11 in the tech-stack table ×2 and the base-image line); corrected in the diff. Enterprise-docs guard re-run over the changed docs + seam files: clean.

Summary

  • Critical: 1 — found and fixed in this branch (ea3c9d82)
  • Informational: 3 — all accepted-and-documented, none blocking
  • Scope: clean

Durable learning

Worth adding to docs/memory/learnings.md (OSS repo, so I have not appended it unasked):

pitfall — the second guard in a file inherits none of the first one's rigour. #1891 shipped two parity rules side by side. Rule 1 enumerated all three Dockerfiles; rule 2 scanned every workflow; rule 3, written last and fastest, read one file of three and used a regex that only matched one of the two syntaxes the repo writes markers in. When adding a rule to an existing guard, re-derive its surface from scratch — proximity to a careful rule does not make the new one careful, and a half-blind rule in a file named ..._parity.py reads as covered.

@github-actions

Copy link
Copy Markdown

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

…Xing the PR (#1910)

The regression-diff gate went red on this very PR for an INFRASTRUCTURE reason,
not a test one: `pytest (base, seed 99999)` hit the 25-min job timeout on a slow
runner (the other seeds ran ~9 min), so `junit-base-99999.xml` was never
uploaded, and `diff-pytest-failures.py` exited 1 on the missing file — even
though its own verdict was "✅ No new failures".

The seeds are redundant by design: they sample different test orders and the
regression signal is the UNION across a side's seeds. Losing one to a runner
timeout should degrade coverage slightly, not fail the build. The old script
fail-closed on ANY missing/empty/unparseable XML, which conflated "one of three
seeds timed out" with "the suite crashed".

Per-side quorum:
* A side's failure-union is computed from its USABLE seeds. Missing/unusable
  seeds are surfaced loudly ("⚠️ Degraded — proceeding on surviving seeds",
  naming each lost file) but tolerated.
* The fail-closed guarantee is kept where it matters: if a WHOLE side has no
  usable XML (every seed crashed), that side's baseline cannot be established at
  all → still exit 1. You cannot crash your way to a green gate.

Verified against the REAL artifacts from this PR's failed run (base-99999
genuinely absent, five others present): the fixed script prints the degraded
warning and exits 0, where the old one exited 1. Same inputs.

Self-tests extended 8 → 11: cases 5/6/7 reframed as "whole side lost → fatal"
(unchanged behaviour, single-file input); case 9 (partial base loss, no
regression → 0, the #1910 shape); case 10 (partial loss must NOT mask a real
head regression → 1); case 11 (every base seed unusable → fatal → 1).

The regression-diff job runs the PR-head version of this script, so the fix
applies to this PR's own gate.

Root cause of the slow seed (a pathologically slow shared runner, ~2.7x median)
is not addressed here and is not this PR's to fix; the gate is now robust to it.

Related to #1910

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dolho

dolho commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

CI fix — the red regression diff was infra flakiness, now handled (33e036f2)

The gate was red for an infrastructure reason, not a test one. Its own verdict was "✅ No new failures"; it exited 1 because pytest (base, seed 99999) hit the 25-min job timeout on a slow runner (other seeds ran ~9 min), so junit-base-99999.xml was never uploaded and the diff script fail-closed on the missing file:

## ⚠️ Infrastructure failures
- `junit-base-99999.xml`: file does not exist
...
## ✅ No new failures      ← the actual check was green
Process completed with exit code 1

That base seed runs the base branch code, so this is not caused by the diff here — but a single lost seed reddening every PR is a real gate-robustness gap.

The fix — per-side quorum

The seeds are redundant: they sample different test orders and the regression signal is the union across a side's seeds. diff-pytest-failures.py now tolerates a missing/unusable seed as long as that side still has ≥1 usable XML, surfacing it loudly (⚠️ Degraded — proceeding on surviving seeds) instead of failing. The fail-closed guarantee is kept where it matters: if a whole side has no usable XML, that stays exit 1 — you can't crash your way to green.

Verified against this PR's actual failed-run artifacts (base-99999 genuinely absent, five present):

## ⚠️ Degraded — proceeding on surviving seeds
- `junit-base-99999.xml` (base): file does not exist
## ✅ No new failures
EXIT=0        ← was EXIT=1 on identical inputs

Self-tests 8 → 11, including the two that matter: partial base loss with no regression → 0 (this exact shape), and partial loss that must still catch a real head regression → 1. The diff job runs the PR-head script, so this applies to this PR's own gate on the next run.

Two notes

  • Head runs 6 more tests than base (5865 vs 5859) — that's the new test_1891_python_version_parity.py (6 cases). Not a coverage anomaly; the gate checks new failures, of which there are none.
  • Root cause of the slow seed (a ~2.7× slow shared runner) is not addressed here and isn't this PR's to fix — the gate is simply now robust to it. If it recurs often, a per-test pytest-timeout (kill one hung test, still emit XML) would be the targeted follow-up; I didn't bundle it to keep scope tight.

The test_a13 wall-clock flake I hit on #1902 (filed as #1909) is a different issue — that one is a genuine test defect, not a missing artifact.

@obasilakis obasilakis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the core of this is exactly right and I verified it rather than taking the body on trust.

What I checked

  • CI is green on 3.13: all six pytest legs ran 9–13 min against the 25-min cap, regression diff SUCCESS.
  • The parity guard is genuinely load-bearing. Negative-tested three ways against the branch, all fire:
    • revert pg-migrations.yml to 3.11 → pg-migrations.yml:68 pins 3.11 (images ship 3.13)
    • delete the audioop-lts line, leave the comment → audioop-lts (docker/backend/Dockerfile pins it for python >= 3.13) (i.e. the vacuous-substring trap you called out is really closed)
    • diverge docker/scheduler/Dockerfile to 3.12 → test_all_images_pin_the_same_python
  • The audioop-lts catch is the most valuable thing in the PR — 7 VoIP codec tests were about to silently stop running, and the regression-diff job compares failures, not counts, so nothing would have gone red.

One thing I'd like changed (non-blocking, but I think it's worth doing before merge)

The quorum relaxation in scripts/ci/diff-pytest-failures.py tolerates a lost seed on either side, but the risk isn't symmetric, because regressions = head_known - base_known:

case exit
all 6 seeds present, real regression only under head seed 3 1 — correctly red
head seed 3 lost to a timeout 0 — pre-PR this was 1
base seed lost (mirror case) 1 — false-red, safe

Reproduced directly against this branch's own diff().

Losing a base seed can only ever grow the regression set → false-red, which is precisely the flake this sets out to fix ("a base seed timing out at the 25-min cap was reddening unrelated PRs"). Losing a head seed shrinks head_known, so an order-dependent regression that only manifests under that seed's ordering disappears and the gate goes green — and the three seeds exist precisely to sample different orderings. dev has no branch protection, so regression diff is the signal people actually act on here.

Suggested fix is one line of asymmetry in the per-side loop: tolerate partial base loss (warn + proceed, as now), keep partial head loss fatal. That preserves 100% of the stated motivation with none of the new false-green path.

Minor

  • The regression-diff change isn't mentioned anywhere in the PR body — it reads as a version-bump PR, so a reviewer skimming the description wouldn't know a fail-closed gate from #715 was relaxed in it. Worth a short section (or splitting it out, since it's independent of #1891).
  • tests/registry.json has no entry for tests/unit/test_1891_python_version_parity.py (you added one in #1893). Nothing in CI reads it and it's 102/393 files, so purely optional.

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.

3 participants