Repository navigation
fix(deps): declare greenlet where the async SQLAlchemy engine is used (#17432) - #17433
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 23 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBackend and AI-stack requirements now cap SQLAlchemy below 2.1. Backend requirements declare greenlet, storage CI pins it, and a repository test checks greenlet declarations for services that use SQLAlchemy's async engine. ChangesAsync SQLAlchemy dependency declarations
Priority: ⬆️ High Severity of issue fixed: High Merge Risk: 🟡 Moderate · up to The dependency declarations address the reported missing package, but the new guard can miss an invalid declaration or reject a synchronous service. Correct its detection rules before relying on it for future changes. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The dependency changes address a reported container startup failure and do not appear to expand access or privileges. The remaining risk is whether each deployed environment installs the declared versions as intended. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The PR satisfies the coding objective in [ Resolution Add an automated SLM container-start check that exercises the startup path and reports the missing module. Keep the declaration guard as regression coverage. Record the smoke-test and Full details: Out of Scope Changes checkExplanation The greenlet declarations, the declaration guard, and the storage mirror support [ Full details: Docstring CoverageExplanation Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 1 files. (4 skipped: 4 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 6e842bff-a789-4e6c-86c1-720c80174391
📒 Files selected for processing (3)
autobot-backend/requirements.txtautobot-slm-backend/requirements.txtrepo_tests/async_sqlalchemy_declares_greenlet_17432_test.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
…#17432) Every docker-smoke-test run since 2026-09-24 ~20:10 failed, on every branch including main: Container autobot-slm Error ModuleNotFoundError: No module named 'greenlet' greenlet appeared in no requirements file in the tree. SQLAlchemy needs it for AsyncSession / create_async_engine, and it had been arriving transitively: SQLAlchemy declares it under an `[asyncio]` extra AND, in some releases, under a `platform_machine == "x86_64" or ...` marker. Verified against the installed metadata -- 2.0.49 carries both forms. So whether it arrives depends on which release a `>=2.0.54` floor resolves to and on the machine resolving it. Neither is a property this repo controls, and both can change with no commit here. That is exactly what happened. Declared explicitly in both backends rather than via the extra. An extra is still an inheritance, and the point is to stop inheriting a runtime requirement this code makes directly. Scoped by evidence rather than blanket. Four requirements files declare SQLAlchemy: autobot-backend/requirements.txt 278 async-engine files -> declared autobot-slm-backend/requirements.txt 83 async-engine files -> declared ai-stack/requirements-ai.txt 0 async-engine files -> untouched requirements-ci/storage.txt no source tree -> recorded autobot-backend had the identical latent gap and only the SLM container happened to fail first. The ai-stack image never opens an async engine, so adding greenlet there would be noise. requirements-ci/storage.txt declares sqlalchemy and aiosqlite and would need it if async-engine tests run against it, but nothing has been observed failing there and this is the PR that has to unblock a red base -- so it is an explicit, reasoned exemption in the new guard, not an omission. New guard: a requirements file that declares SQLAlchemy, whose own source tree uses AsyncSession or create_async_engine, must declare greenlet with a floor. Mutation-checked -- removing the line reddens it naming autobot-slm-backend/requirements.txt. Two defects found in my own scan while writing it, both by its own tests: the glob was `requirements*.txt`, which cannot see `requirements-ci/storage.txt` because that file is named `storage.txt`; the exemption-staleness test caught it, reading as "the file is gone" rather than "the scan is narrow". And the exemption list is checked against the tree so an entry cannot outlive the file it excuses. NOT established, and deliberately not closed on: whether `ERROR: Migration execution failed: relation "agents" does not exist` clears with this. The reading is that without greenlet the async migration machinery cannot run, so the table is never created -- but that ordering is unproven, and if it survives this fix it is a second defect that gets its own issue rather than widening this one. This PR's own smoke-test run is the measurement. Refs #17432
ca5a41e to
503d69a
Compare
PR #17395 was CONFLICTING with 0/11 required contexts and only 4 checks present -- CI had never dispatched, so there was no red to root-cause. The conflict was holding it. One conflicted file, `.secrets.baseline`, which is a hub file every branch touches. Resolved by CONSTRUCTION rather than by merging text, because a textual merge of 1,900 JSON entries is unreviewable and silently lossy: main 301 files 1402 entries (en.json only, 53 entries) branch 312 files 1916 entries (11 slm locale files, 556 entries) merged 311 files 1905 entries Built from MAIN's baseline, replacing only the `autobot-slm-frontend/src/locales/` keys with this branch's. That keeps every entry any other branch has audited since this one opened, and takes this branch's locale entries -- which already have the 22 `Private Key` specimens removed and carry no `line_number`. The arithmetic is checkable rather than asserted: 1402 - 53 (main's en.json, superseded) + 556 (this branch's eleven) = 1905, and 301 - 1 + 11 = 311 files. Verified on the result, not inferred: 0 entries carry a `line_number` (#16353), 0 are unaudited, 0 `Private Key` remain in the locale files. `FLOOR = 1_885` still holds -- 1905 - 1885 = 20, inside the 100 headroom -- so no floor change rides this merge. `useCaptchaStatus.ts` comes through as main's: `9a1de040cf` (openVnc via `getServiceUrl('vnc')`) is intact and no `VITE_LOCALHOST`/`VITE_VNC_PORT` returns. Checked because reviving an older shape across a merge is exactly how that fix would have been reversed. repo_tests/no_tracked_key_material_test.py repo_tests/secrets_baseline_has_no_line_numbers_16353_test.py 22 passed the two i18n guards + conftest imports + glob declarations 81 passed lift_locale_translations.py --check clean Note on `smoke-test`: it is red on main right now for a missing `greenlet` in the SLM image (#17433, 58's). A red there on this run is the base, not this branch. THE MERGE ALSO PUT A FILE ONE LINE OVER THE CEILING. `repo_tests/glob_declared_reads_15900_test.py` came out of the auto-merge at 601 lines against the 600 limit -- main appended a declaration and so did this branch, and neither was over on its own. Split rather than exempted, per the rule. The record itself (`GLOB_DECLARED_UNCOVERED`, 195 lines) moves to `repo_tests/_glob_declared_uncovered.py` and the guard imports it: 601 -> 405 lines, with the registry free to grow on its own. The seam is data from assertions, not an arbitrary cut, and it fixes something beyond the line count: this is a registry EVERY branch appends to, so appends now land in a small data file instead of colliding inside a long test. This merge conflicted on exactly that shape in `.secrets.baseline`. The one-line form of the entry that tipped it over does not fit either -- 122 characters against a 120 limit -- so shortening was not available. repo_tests/glob_declared_reads_15900_test.py 30 passed + one_repo_root_spelling_15925_test.py 37 passed scripts/check_python_file_size.py exit 0
A second check fails on this, outside
|
Correction: this PR already covers the non-container path. Disregard my warning.I wrote that "a fix that adds This PR adds So What still stands, and is the only part worth keeping: the two checks fail from one cause, and What does not stand: the implication that the fix's shape was too narrow. It is not. Apologies for the noise, particularly to anyone who was about to add a criterion for a gap that |
|
Two questions raised in review, both answered with the measurement rather than the argument. Does this fix reach the non-container path?A report suggested The first half is right — it does import - name: Install SLM backend dependencies
run: |
python -m pip install --upgrade pip setuptools wheel
python -m pip install -r autobot-slm-backend/requirements.txt --prefer-binaryThe job installs from exactly the file this PR adds Checked the current state rather than inferring it — that check on this PR is Why a direct
|
…it (#17432) code-quality red on the requirements-ci drift audit, one package: 1 package(s) declared in production but missing from requirements-ci.txt / requirements-ci/*.txt, and not recorded in repo_tests/requirements_ci_drift_baseline.txt: greenlet (greenlet>=3.1.0) Two things were wrong, and the second is the one worth recording. The mechanical error: I recorded the exemption in my own guard's list with my reasoning attached, and this gate keeps a different list -- repo_tests/requirements_ci_drift_baseline.txt -- which does not read mine. The judgement was written down somewhere that could not enforce it. The substantive error: the judgement itself was weak. I wrote "nothing has been observed failing there" as grounds for not declaring greenlet in requirements-ci/storage.txt. That is precisely the reasoning this PR exists to reject. A test that skips, or never opens an async engine, observes nothing either way -- and that invisibility is what hid the missing dependency for a day across every branch. "Not observed failing" and "does not need it" are different claims, and only the second justifies an omission. That file declares sqlalchemy AND aiosqlite, so a test opening an async engine against it fails exactly as the SLM container did. Mirrored: greenlet==3.3.2, matching the exact-pin convention of that file, with the reasoning inline. Drift audit now clean over 106 production packages. Also removes the now-obsolete entry from the new guard's exemption list. The mechanism stays -- empty dict plus the staleness test -- so a future exemption must carry a reason and cannot outlive its file. And replaces it with a STATED limitation rather than a silent one: a requirements file with no sibling Python tree reaches the async-usage check and finds nothing, so it passes without being examined. That is a pass by absence of evidence. Closing it means teaching the guard which suites run against which requirements set, which nothing in the tree records today. Said out loud because the rule this guard enforces is that exact distinction. Refs #17432
…17445) The greenlet fix unmasked a second defect rather than causing one. On main those tests died at collection (17 errors, no greenlet); with greenlet present collection succeeded and 123 ran, 27 failing inside SQLAlchemy's own cache-key machinery: TypeError: 'InternalTraversal' object is not callable sqlalchemy/sql/_cache_key_cy.py:196 in BaseHasCacheKey._gen_cache_key Traced rather than inferred. Two separate venvs, each internally consistent: 1055 $VENV/bin/python -m pip install -r requirements-ci.txt -> 2.0.54 1991 rm -rf "$SLM_VENV"; python -m venv "$SLM_VENV" 2002 $SLM_VENV/bin/python -m pip install -r autobot-slm-backend/requirements.txt -> 2.1.0 frames: .venv-python-suite-slm/lib/python3.14/site-packages/sqlalchemy/... So there is no mixed install and no cross-file conflict: the failing tests run in the SLM venv, which took 2.1.0 because `sqlalchemy>=2.0.54` had no upper bound. `requirements-ci/storage.txt`'s `==2.0.54` is a different environment and is healthy on the same commit -- which is what makes 2.0.54 known-good rather than merely previous. The traceback also confirms the greenlet fix works: it passes straight through `concurrency.py:199 greenlet_spawn`, so greenlet is present and the failure is downstream inside 2.1.0. Capped at <2.1 in all three service requirements files. The break is proven only in the SLM venv, but autobot-backend carries the identical unbounded floor over 278 async-engine files and is protected today only by CI installing it from requirements-ci's pin -- its own Docker build would take 2.1.0. Same reasoning as the greenlet declaration: fix the class, not the site that happened to fail first. This is a DEFERRAL and the comment says so. It restores the version CI already pins rather than choosing a new state, and converts an uncontrolled minor bump into a recorded one. Migrating to 2.1.x is separate work and is emphatically not a base-unblocking change. Not capping was not an option worth weighing against migrating: it is a choice to leave the base red with six PRs behind it. That is why this is on #17433 rather than posted as a decision. repo_tests/requirements_pin_parity_test.py and the new greenlet guard: 19 passed. Drift audit clean over 106 production packages. Refs #17445
…elf (#17432) Review found the guard had no negative case. The parametrised test iterates the real tree, the tree now declares greenlet everywhere, so it passed trivially -- and would have passed identically if the detector were broken. A local mutation showed the detector worked on the day it was written; nothing in the suite would catch it breaking later. Correct finding, and it is the third instance of this shape in this repo today, each time caught by someone other than the author: a glob of requirements*.txt blind to requirements-ci/storage.txt an exclusion keyed on __file__.name that stopped covering a moved record an async-usage check that passed a file it never examined A detector with no contrast pair is the same family as no detector at all: it reports clean either way. Two synthetic fixtures under tmp_path -- one async service declaring SQLAlchemy without greenlet that must be flagged, one with greenlet that must not be. Both go through _violates(), the same single predicate the real-tree test uses, so the fixtures prove the detector rather than a re-statement of its rule. A contrast test that re-implements the logic agrees with itself and catches nothing, which is #17407's failure in another guard on the same day. Mutation-checked in both directions, which is what a pair is for: detector blind, greenlet always 'declared' -> negative fixture fails detector over-eager, nothing counts -> positive fixture + real tree fail async-usage check always False -> negative fixture fails 8 passed on the real tree. Refs #17432
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: c683942f-258c-4dfc-9656-77d61c03d222
📒 Files selected for processing (5)
autobot-backend/requirements.txtautobot-infrastructure/shared/docker/ai-stack/requirements-ai.txtautobot-slm-backend/requirements.txtrepo_tests/async_sqlalchemy_declares_greenlet_17432_test.pyrequirements-ci/storage.txt
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Three review findings, all verified against the code before fixing, all real. (1) FALSE NEGATIVE. _ASYNC_MARKERS omitted async_engine_from_config, which is the async-engine entry point autobot-backend/migrations/env.py:18,99 uses -- and it uses no other marker in the set. A service reaching the async engine only that way passed this guard without declaring greenlet. Added, along with async_sessionmaker, AsyncEngine and AsyncConnection. (2) FALSE NEGATIVE, and the one that would have let the original outage through. The floor regex accepted `greenlet>=3.1.0; python_version < "3.0"` -- a declaration that can never install on the 3.14 runtime, counted as satisfying the rule. Verified against the old pattern before replacing it. The rule is now unconditional declaration: a floor and NO environment marker. Conditionality is the defect -- greenlet went missing because SQLAlchemy declared it behind an extra and a platform_machine marker, so a marker here rebuilds exactly that. Judging a marker needs a PEP 508 evaluation against the target environment, which this guard does not know, so it refuses markers rather than guessing which are benign. (3) FALSE POSITIVE. `any(marker in text ...)` over the raw file meant a comment or docstring saying "AsyncSession" made a purely synchronous service require greenlet. Now identifier-based: ast alias, Name and Attribute nodes, so prose produces no match. Substring pre-filter first, because parsing every module is what took a sibling guard past 120 seconds. An unparseable file falls back to the substring answer rather than reporting "no async usage", which would be a pass by failure to read. Over-strict is not the safe direction: a guard that cries wolf gets narrowed by whoever it inconveniences, and the narrowing is where real coverage is lost. A fixture per finding, and one of them exposed a fourth problem in my own test design. test_every_async_marker_is_detected is parametrised over _ASYNC_MARKERS, so dropping an entry drops its own test case -- the mutation left 15 tests passing. A set that supplies its own test cases cannot notice becoming smaller. Fixed with an anchor OUTSIDE the set: migrations/env.py references exactly one async name and no other marker, so it binds the entry the review found missing. Remove it and that test fails where the parametrised one cannot. Mutation-checked, 3 of 3 now caught, each by its own test: drop async_engine_from_config -> test_the_marker_set_still_detects_the_real_migrations_module accept a marker-carrying line -> test_a_greenlet_declaration_behind_an_environment_marker_is_refused back to raw substring -> test_naming_the_async_engine_in_prose_is_not_using_it 17 passed. Refs #17432
|
Three review findings on the fixture commit, all verified against the code before fixing, all real. Fixed in (1) (2) The floor regex accepted a runtime-excluding marker — the finding that matters most. Verified against the old pattern before replacing it: A guard in that state would have let the original outage through. The rule is now a floor and no environment marker. Conditionality is the defect here — greenlet went missing because SQLAlchemy declared it behind an extra and a (3) Raw substring over the whole file — false positive. A comment or docstring saying "AsyncSession" made a purely synchronous service require greenlet. Now identifier-based via Over-strict was not the safe direction to leave it: a guard that cries wolf gets narrowed by whoever it inconveniences, and the narrowing is where the real coverage goes. A fixture per finding — and one exposed a fourth problem, in my own test design
Fixed with an anchor outside the set: That is the same shape as the finding that started this thread — a detector that reports clean either way — one level up, in the test that was supposed to prove the detector. Worth naming because parametrising over the list under test looks like thorough coverage. Mutation results — 3 of 3 caught, each by its own test17 passed. Threads 0, behind 0. |
…ote (#15877) python-suite shard 7/12: 1 failed, 2901 passed. test_no_new_comment_cites_a_line_number async_sqlalchemy_declares_greenlet_17432_test.py cites autobot-backend/migrations/env.py:18,99 The comment I added for the async_engine_from_config finding cited line numbers. Replaced with the symbol: `run_async_migrations` in `autobot-backend/migrations/env.py` reaches the async engine through async_engine_from_config and through no other name in the marker set. Checked the rest of the branch rather than only the line the guard named -- `git diff origin/main...HEAD` shows exactly one such citation and this was it. The coincidence is worth recording because it is the same defect twice in one PR, at two levels. #15877 exists because a line-number citation fails SILENTLY into whatever later occupies the line -- the reference drifts and nothing says so. That is the identical shape as the guard this PR is about: greenlet arrived through a conditional upstream declaration, the condition changed, and nothing said so. I fixed the second while introducing the first, in the same file, in the comment explaining the fix. #15877 caught it loudly, which is what separates it from the class it guards against. 22 passed across both guards. Refs #15877, #17432
Base went green when #17433 merged the greenlet declaration, so this picks it up for a first real CI pass. Clean merge -- `git merge-tree` reported 0 conflicts before running it, unlike the previous merge which had to reconstruct `.secrets.baseline`. Merged rather than rebased, deliberately. The goal is a run against a base that builds; a rebase would replay twelve commits including two merges, and the baseline-touching ones conflict individually -- which is what forced a merge last time after the rebase hit the same file on four separate commits. A merge reaches the same base with zero conflicts and preserves the construction-based baseline resolution that a replay would redo commit by commit.
…17091) Picks up #17433's greenlet declaration so this PR gets a run against a base that builds. Its three reds all predate that fix and are the base's, not this branch's -- verified by timing rather than assumed: verify-generated-types-slm started 2026-09-24T20:48:03Z smoke-test started 2026-09-24T20:30:33Z hardened-smoke-test started 2026-09-24T20:26:02Z greenlet fix on main 2026-09-25T05:53Z Nine hours earlier, and all three are SLM-app-build checks sharing the cause already traced on verify-generated-types-slm: the SQLAlchemy asyncio engine needs greenlet, which appeared in no requirements file. Nothing in this branch touches the SLM backend -- it is agents, coordination and the budget gate.
…et the floor check reach a venv it is not running in (#17448, #17449) (#17502) * fix(deps): check that both declaration planes resolve the same, and let the floor check reach a venv it is not running in (#17448, #17449) Two halves of one surface. A guard asserting the planes agree is useless if it can only read one of them. #17448 -- the planes may both declare a package and still install different releases. #14551's guard, in the same module, checks PRESENCE and says so in its own docstring: "What this guard does NOT do: merge the two dependency sets." A package declared on both planes as ==2.0.54 and >=2.0.54 passes it cleanly, and that is how the SLM test venv took SQLAlchemy 2.1.0 while CI's own venv stayed on 2.0.54 -- 2.1 dropped greenlet as a hard dependency, nothing declared it directly, and every SLM test module failed at import (#17433). The two migration gates reached the same 2.1.0 from the same cause a day later (#17499). Two point fixes, one untouched mechanism. Measured: 78 packages appear on both planes and 52 can resolve differently. That enumeration is the deliverable, not the SQLAlchemy fix. The comparison is PER SERVICE FILE, deliberately. production_requirement_names flattens both backends with last-wins, and they are allowed to disagree: websockets is capped <16 in autobot-backend because langgraph-sdk needs it, and floored >=17.1 in autobot-slm-backend, which has had its own venv since #16394, each documented at its own site. Comparing a merged plane would report that deliberate split as a conflict -- training people to ignore the guard -- while still missing real divergence, because last-wins had already discarded one of the two constraints before the comparison ran. Two unbounded constraints do NOT diverge: >=2.0.52 and >=2.0.54 look different and install the same thing, because both take whatever published last. Divergence needs at least one side pinned or capped. A string comparison would have been wrong in both directions. #17449 -- check_dependency_floors.py could only ever inspect the interpreter running it: importlib.metadata.version(), no --venv, no --host. So it was pointed at local interpreters and CI and never at a deployed service venv, the only environment serving traffic. It now takes --venv (a venv directory or an interpreter path) and queries that interpreter over a script importing nothing from this repository. installed_versions() is the whole seam; every comparison downstream is unchanged, and the local path still binds with ONE argument so existing callers and test stubs are untouched. Using it immediately found a hole in the instrument. A deployed venv carried TWO dist-info directories for one distribution -- #15063's append-only venv, where pip install -r upgrades but never removes. With two of them importlib.metadata.version() answers nothing, the old code stored None, shortfalls() treated None as absent, and absence is not a shortfall by default. So that package's floor was silently never checked and the environment reported fully satisfied: "nothing found" and "did not look" returning the same green, inside the module built to tell them apart. There are now three states -- readable, absent, and installed-but-unreadable -- and the third always reports, regardless of --require-present, because it means the check could not answer. The report also names the environment it read. It previously said "the interpreter running this check (python 3.10.12)" while examining a 3.14.6 venv, attributing a deployed environment's numbers to the local box. Two sessions retracted conclusions from exactly that confusion on one CI log within an hour; a tool that prints it is worse than a log that merely permits it. A deployed report also no longer offers CI-parity advice, which would tell an operator to fix production by running a local parity script. Retraction carried from the issues: I reported the deployed SLM venv as sitting at 2.0.52, below its declared floor. It is at 2.0.54 and satisfies the constraint. I read sqlalchemy.__version__ and reported it as if it were importlib.metadata.version() -- two instruments, two questions. Whether an update ran between my two readings or the first resolved stale metadata is NOT established, and stays that way rather than being guessed. Not delivered here, and stated rather than implied: running the venv check on the builtin update path. That is deployment wiring and goes through the updater, never an ad-hoc script, so it stays on #17449 as its own criterion. Mutations verified: an unbaselined divergent pair, a stale baseline entry, a missing interpreter, a directory with no interpreter, and unreadable-vs-absent each behave as specified. 22 tests on the drift guard, 84 on the floor checker. Refs #17448 Refs #17449 * fix(deps): compare constraint semantics, and stop the detector claiming more than it can know (#17448, #17449) Seven review findings on #17502, all correct. The first changes what the number means. `can_resolve_differently` compared operator TEXT and was read as comparing meaning -- the failure this PR catalogues everywhere else, committed in its own instrument. Review found both directions: ">=1,!=2.1" vs ">=1" -> False, but they select different releases if 2.1 is newest. A false negative, and a missed pair here is silent. "==1.0" vs "==1.0.0" -> True, though version matching treats them as equal. Noise, which is how a guard gets ignored. The deeper problem was the NAME. No offline check can know which releases two specifiers select, because that depends on the set of versions that exist. The carve-out that came with the name was wrong for the same reason: it treated two purely lower-bound specifiers as equivalent because "both take whatever published last" -- true only while a release exists above both floors. With only 2.0.53 published, >=2.0.52 installs and >=2.0.54 does not resolve at all. An unstated assumption about the version universe was doing load-bearing work. So it is now `constraints_differ`, and the guard asserts the narrower, fully checkable property: the two planes DECLARE the same constraint. Whether a divergent pair installs different releases today is a question about PyPI and is deliberately not modelled. The count moves 52 -> 53 and, more importantly, stops being a floor reported as a measurement. `packaging` decides both cases correctly and is imported lazily, not at module scope: it is declared in no requirements file here, and this PR's own issue asks that a directly-imported dependency be declared directly rather than relied on transitively. Hard-importing it would commit the defect the PR describes. Without it the fallback is a plain textual difference, which over-reports equivalent spellings and never under-reports -- the only acceptable direction for a guard whose misses are invisible. The other six: - The constraint baseline may now reach zero. Asserting it non-empty punished whoever finishes the enumeration: fixing the last pair means deleting its entry, and the assertion then failed while the audit was clean. The ceiling stays; the floor is gone. - Production files are checked per file, so one healthy file cannot mask an absent one behind the merged set. Absence only -- `_write_tree` empties the SLM file deliberately to isolate a backend-only case, and emptiness is covered by a real-tree test instead, where it would actually be a defect. The original merged-empty check is kept beside it rather than replaced; dropping it reintroduced the hole test_audit_fails_on_zero_production_packages holds. - `duplicate_metadata_names()` is hoisted. It scans every installed distribution, and calling it per wanted name made one audit do N+1 full scans. - `subprocess.TimeoutExpired` and a wrong-shaped payload now raise `InterpreterUnreadable`. TimeoutExpired is not an OSError, so a hung probe ended the run with a traceback instead of the documented FATAL and exit 2. Same argument as the third state one layer in: a probe that did not finish is "could not look", never "nothing is installed". - The deployed report keeps its truncation notice. The environment branch returned before it, so a --venv report silently dropped entries without saying it had. - `Declaration` is built by keyword in the test. Its field order is (source, name, operator, required) and the positional call assigned the wrong values to every field. 120 tests. Refs #17448 Refs #17449 * fix(deps): declare the OpenTelemetry core in the CI plane at the production pin (#17557, #17448) `python-suite shard 1/12` fails on eight unrelated PRs, none of which touched a requirements file. It is not a test failure: `check_dependency_floors.py --strict` exits 1 about 24 seconds before the first test runs. Root requirements.txt:51-58 pins opentelemetry at ==1.44.0. The CI plane -- requirements-ci.txt and every requirements-ci/*.txt it includes -- declared opentelemetry NOWHERE. Undeclared is not absent: chromadb, langsmith and redis pull api, sdk, exporter-otlp-proto-grpc and proto in transitively, so CI resolved them unconstrained and took 1.45.0 when it shipped, against a production pin of 1.44.0. The strict check then compared the two planes and failed. Pinned in one plane and silent in the other is the pairing that guarantees divergence the moment upstream publishes, and it surfaces on whichever branch next builds a cold cache -- never the branch that caused it. Declared at the production pin rather than bumping production to 1.45.0. A bump chases upstream, breaks again on 1.46.0, and would move the protobuf cap that requirements.txt:10-14 records opentelemetry-proto==1.44.0 as one of the two direct requirers of -- reasoning someone did explicitly. Verified safe to pin rather than pushed and hoped: every requirer states a LOWER bound only -- chromadb >=1.2.0, langsmith >=1.30.0, redis >=1.39.1, and the otel packages pin each other at ==1.44.0 -- so ==1.44.0 satisfies all of them and pip has nothing to backtrack. Only the four CI actually resolves are declared. exporter-otlp, exporter-otlp-proto-http and propagator-b3 stay production-only and stay in the drift allowlist: nothing in CI installs them, so declaring them would ADD packages rather than constrain existing ones. Those four leave repo_tests/requirements_ci_drift_baseline.txt, which is the allowed direction -- the allowlist only shrinks, and an entry CI now declares is stale by that file's own second rule. The comment kept the old reason and records why it stopped applying: "no collected test reaches the import" was true and irrelevant, because a transitive install is unaffected by whether a test imports it. The 4-of-10 split is install-presence, not resolver backtracking: installed_versions() catches PackageNotFoundError and continues, so the three packages absent from CI are skipped rather than compared. Verified against the local environment, which holds exactly those four plus two transitive otel packages and none of the other three. Scope held to the unblock: eight PRs are red on this and a smaller diff reviews faster. The two structural gaps this exposed -- installed_versions reporting a clean sweep over packages it never compared, and compute_constraint_drift being blind to a package declared in one plane only -- are filed, not folded in here. Pin lives in requirements-ci/storage.txt beside chromadb==1.5.9, not in monitoring.txt: chromadb is the only otel-dragging package in the CI plane, so the constraint belongs where whoever bumps it will see it. chromadb 1.5.9's own floors were read from its metadata (>=1.2.0 on api, sdk and exporter-otlp-proto-grpc) rather than assumed, after checking that the installed distribution really is the 1.5.9 the CI plane declares.
1 participant
mainis red and six PRs are blocked behind it. This is the one fix.Thinking Path
Every
docker-smoke-testrun since ~20:10 on 2026-09-24 failed, on every branch includingmain:greenletappears in no requirements file in the tree. SQLAlchemy needs it forAsyncSession/create_async_engine, and it had been arriving transitively. Verified against installed metadata rather than assumed — SQLAlchemy 2.0.49 declares it twice:So whether it arrives depends on which release a
>=2.0.54floor resolves to and on the machine doing the resolving. Neither is a property this repo controls, and both can change with no commit here. Nothing in the tree recorded the requirement, so nothing failed when it stopped being met — until a container would not start.Declared explicitly, not via the
[asyncio]extra. An extra is still an inheritance, and the point is to stop inheriting a runtime requirement this code makes directly.What Changed
Scoped by evidence rather than added everywhere SQLAlchemy appears:
autobot-backend/requirements.txtautobot-slm-backend/requirements.txtai-stack/requirements-ai.txtrequirements-ci/storage.txtautobot-backendhad the identical latent gap — only the SLM container happened to fail first. Fixing one site would have left a 278-file service one resolver change from the same outage.requirements-ci/storage.txtdeclaressqlalchemyandaiosqliteand would need greenlet if async-engine tests run against it. Nothing has been observed failing there, and this is the PR that has to unblock a red base, so it is an explicit exemption carrying its reason in the new guard rather than a silent omission or a blind edit.New guard: a requirements file declaring SQLAlchemy, whose own source tree uses
AsyncSessionorcreate_async_engine, must declare greenlet with a floor.Verification
Mutation — removing the declaration reddens the guard by file name:
Two defects the guard found in itself
The glob was too narrow.
requirements*.txtcannot seerequirements-ci/storage.txt, because that file is namedstorage.txt. The exemption-staleness test caught it — and the way it surfaced is the interesting part: it read as "the exempted file is gone from the tree" rather than "the scan cannot see it". Same shape as the failures this repo has been collecting in #15826: an instrument reporting absence when it had not looked.The exemption list is checked against the tree, so an entry cannot outlive the file it excuses and quietly become a hole.
Not established — deliberately not closed on
Whether
ERROR: Migration execution failed: relation "agents" does not existclears with this fix. The reading is that without greenlet the async migration machinery cannot run, so the table is never created — but that ordering is unproven. If it survives this fix it is a second defect and gets its own issue rather than widening #17432. This PR's ownsmoke-testrun is the measurement, not my inference.Also outstanding on the issue: re-running the six blocked PRs (#17375, #17380, #17394, #17408, #17426, #17429) to confirm the required context goes green rather than assuming the fix propagates.
Model Used
Claude Opus 5 (1M context)
Closes #17432
Summary by CodeRabbit