Repository navigation
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) - #17502
Merged
Conversation
…et 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
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 46 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 ignored due to path filters (2)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe dependency-floor checker can audit a selected Python environment and distinguish unreadable installed versions from absent distributions. The CI requirements checker now detects constraint drift between CI and production requirements and validates the detected pairs against a baseline. ChangesDependency floor audit
CI constraint-drift audit
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to The new constraint-drift guard can pass silently when one service's requirements file is empty or missing. It can also miss some divergent constraints. Its test fails when the last divergent pair is fixed. A hung target interpreter crashes the floor check with a traceback instead of reporting a clean error. Address these before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new checks strengthen dependency auditing. The reviewed CI callers do not expose interpreter selection to untrusted input, and no new production entry point or privilege increase was identified. Coverage of deployed use and the new exception baseline remains incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Contributor
✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 383edfaa-fb49-4821-91f5-c6958589d9fd
⛔ Files ignored due to path filters (1)
repo_tests/requirements_constraint_drift_baseline.txtis excluded by!**/*_baseline.txt
📒 Files selected for processing (5)
pipeline-scripts/check_dependency_floors.pypipeline-scripts/check_dependency_floors_test.pyrepo_tests/dependabot_requirements_coverage_test.pyrepo_tests/requirements_ci_drift_test.pytools/lint/check_requirements_ci_drift.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
4 tasks
…ng 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
mrveiss
commented
Sep 26, 2026
mrveiss
left a comment
mrveiss
left a comment
Owner
Author
There was a problem hiding this comment.
Second reader, asked to confirm two things rather than re-approve the sweep. Both hold. Verdict: land it. One Minor and one nit, neither blocking.
| Sev | Where | Finding |
|---|---|---|
| Minor | tools/lint/check_requirements_ci_drift.py:79 |
The new baseline is not in GUARD_INPUT_PATHS, so the reach test cannot verify its trigger |
| Nit | pipeline-scripts/check_dependency_floors.py resolve_interpreter |
bin/python3 is not a candidate |
Confirmed: the corrected detector catches the class, not the instance
The rename is the substance. constraints_differ asserts "the two planes declare the same constraint", which is fully checkable from the tree, where can_resolve_differently claimed something no offline check can know. The false negative followed from the name promising resolution while the body compared text, and the tests now pin the class rather than the one pair:
">=1,!=2.1"vs">=1"— the instance review found.">=2.0.52"vs">=2.0.54"— the removed carve-out, and the more interesting half. "Both take whatever published last" was an unstated assumption about the version universe doing load-bearing work; with only 2.0.53 published, one installs and the other does not resolve at all."==2.0.54"vs">=2.0.54"— the shape that actually took SQLAlchemy 2.1.0 into one venv."==1.0"vs"==1.0.0"→ not a difference, so the normalisation is asserted in the no-false-positive direction too."not-a-specifier"vs">=1"→ True. Failing to parse must not read as agreement, which is the property that makes every remaining unknown safe.
The lazy packaging import is the right call for the reason given — hard-importing an undeclared dependency inside the PR that asks for direct declaration would commit the defect it describes — and the ImportError fallback over-reports rather than under-reports. One consequence worth naming, since it is the one direction that bites a baselined guard: over-reporting is the safe default for a detector but a red build for a ratchet, so an environment without packaging would fail against a baseline generated with it. packaging ships wherever pip does, so this is a note rather than a finding — but it is the reason I would not later "simplify" that fallback to return False.
Confirmed: the baseline can reach empty
test_the_baseline_never_covers_every_compared_pair keeps the ceiling (len(baseline) < compared) and deliberately drops the floor, with the reasoning I would want in the docstring: "fixing the last divergent pair means deleting its entry, and the assertion then fails while audit_constraint_drift reports a perfectly clean tree". A ratchet that cannot reach zero punishes whoever finishes the work.
The path is clean end to end, not just at the assertion: load_constraint_baseline returns set() for a missing file, audit_constraint_drift with empty drift and empty baseline produces neither new nor stale, and compared > 0 guards the ceiling so an empty comparison cannot pass as clean. The stale branch is what walks the last entry out — "this list only shrinks — delete these lines".
test_both_production_files_are_populated_on_the_real_tree is the subtler addition and I would keep it: unreadable_production_files flags absence only, so a production file that exists and declares nothing was the one hazard the merged set could hide, and a synthetic fixture cannot cover it because _write_tree empties that file on purpose.
Minor — the second baseline is outside the declared contract
GUARD_INPUT_PATHS documents itself as "Every repo-relative path/glob this checker reads", and _ALLOWLIST_FILE is in it. _CONSTRAINT_BASELINE_FILE is not — the diff adds it at :715 and uses it three times, and the tuple at :79 is untouched.
Not a live hole: .github/filters/code-quality-paths.yml covers repo_tests/** (:57) and **/requirements*.txt (:55), and the new baseline matches both, so the required check does fire on a change to it today. The defect is that check_code_quality_guard_reach.py verifies declared paths, so this one is verified by nothing. repo_tests/** is exactly the kind of broad glob someone narrows to cut CI time, and the day that happens this guard silently stops re-running when its own baseline changes — a baseline entry could then be added to excuse a divergence with no run confirming the pair is still divergent. One name in the tuple closes it, and the asymmetry with its sibling is the tell.
Nit — resolve_interpreter
(target / "bin" / "python", target / "Scripts" / "python.exe") covers python -m venv and Windows. A prefix assembled by hand or by conda can carry only bin/python3 / bin/python3.12, and the failure is a clear InterpreterUnreadable rather than a silent pass, so this is a convenience, not a correctness issue. Adding bin/python3 would cost one tuple entry.
What I checked and found correct
- The probe cannot be injected. Package names go to the target interpreter over stdin as JSON, not interpolated into the
-csource, so the safety does not depend on_NAME_REholding. List-formsubprocess.run, no shell. - Every failure mode raises instead of returning empty, consistently: not-a-file,
OSError, non-zero exit, unparseable stdout, and valid JSON of the wrong shape. TheTimeoutExpiredclause earns its comment — it is not anOSError, so it previously escaped as a traceback instead of the documented FATAL, and a probe that did not finish is could not look, never nothing installed. - The two absences stay distinct inside the probe too:
PackageNotFoundErroromits the name (genuinely not installed) while any other exception writes'', which the caller maps toUNREADABLE. A broken metadata read and a missing package do not collapse — which is the same defect #15063 hit with duplicate*.dist-info, anddupcarries that across the boundary rather than re-deriving it locally. - The comparison is per service file, not a merged plane, with the
websocketssplit (<16for langgraph-sdk vs>=17.1) as the worked reason. A flattened plane would report a deliberate split as a conflict and miss the real thing, because last-wins had already discarded one constraint before the comparison ran.test_the_comparison_is_per_service_file_not_a_merged_planeasserts the file keys, so that cannot regress to a merge. constraint_ofstrips extras and markers, withgreenlet→""pinned — an unconstrained declaration is a real state, not an error.
No worktree and nothing executed: this is a read of the diff plus GUARD_INPUT_PATHS, the code-quality path filter, and parse_requirements on main (which does follow -r with cycle protection, so the CI plane's -r includes are genuinely covered).
This was referenced Sep 26, 2026
…uction 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.
… into issue-17448-requirements-planes
Owner
Author
|
Review by the one session that had not touched this PR. Verdict: sound, and I would approve it — GitHub refuses ( Checked against the tree and the job state, not the description. The otel fix is verified by measurement, not argumentI proposed an acceptance criterion on #17557 — a fix must be shown to change what CI installs, not only what it declares — because a pin in repo-root
It also closes the residual risk empirically: The comment explaining why four and not seven is the right call: the three absent ones would ADD packages rather than constrain existing ones, and The detector change fixes the class, not the instance
Leaving
What I did not check
One follow-up, not blocking#17558 records that chromadb's otel requirement is unbounded, so agreement with On the approval gate
Per the standing rule, the approval ruleset is for external contributors; an owner merge needs green checks, no unresolved review threads, and not behind base — not an APPROVED review. This PR currently has 0 failing, 0 unresolved threads and |
This was referenced Sep 26, 2026
mrveiss
added a commit
that referenced
this pull request
Sep 27, 2026
…ion bound the CI plane cannot keep (#17558) (#17610) * fix(deps): a floor report states its role, its scope, and what it could not compare (#17558) Three readers misread a dependency-floor block in one day -- twice as a build failure it was not, once as a clean environment it was not. Every reading was of correct data whose ROLE and SCOPE were unstated. That is one property, not three bugs, and it is fixed once here. WHAT A NUMBER NOW CARRIES. `audit()` returns a `FloorAudit` instead of `(shortfalls, declarations_read)`, because the second element was printed as though it were the number of comparisons and it is not. A package declared but not installed is dropped by `installed_versions` -- compared against nothing -- so "128 declarations checked, all satisfied" could be, and was, true of an environment holding 86 of them. Measured on one box: 128 declared, 42 never compared. A pass now reports comparisons and names what it could not compare. ROLE. Every block says whether it is a `[GATE]` or `report only (informational; nothing here fails this run)`. repo_tests/dependency_floor_banner.py is the report half and printed a RED "environment is BELOW the declared dependency floors" header in the same shape as the gating check; it now says what it is and prints yellow. It cannot fail a run and never could. SCOPE. The declaration roots are printed. The same deployed venv reports `22 of 216 NOT satisfied` against every root and `94 ... all satisfied` against the one that builds it -- both true, answering different questions, and neither line said which. With `--venv` against the union the report carries an ADVISORY saying so and naming the fix, because a shortfall from a plane that does not build the environment is a false positive, not a finding. `--all` lists the not-compared names too: a flag promising every shortfall while hiding the packages no shortfall could be computed for is the same omission one level down. SIZE. The change took check_dependency_floors.py to 639 lines against a ceiling of 600, so the reporting half moves to pipeline-scripts/dependency_floor_report.py (550 and 147). The ceiling is not raised and the seam is real: nothing in the new module measures anything and it imports nothing from the checker, so the dependency runs one way -- required, because the banner loads the checker with spec_from_file_location, where a sibling import would not resolve. `FloorAudit` carries `roots_are_the_union` rather than re-deriving it from `DECLARATION_ROOTS` for the same reason. `_print_report` is extracted from `main` for the #620 length gate, which caught it at 66 body lines against a threshold of 65. Verified against a deployed venv as well as fixtures. Four mutations each fail a test: counting declarations instead of comparisons, dropping the roots line, labelling every block a gate, and hiding the not-installed count. 117 tests pass across the checker, the strict gate and the ci-drift suites. Items 2 and 4 of #17558 -- one-plane-only drift detection, and the sweep for CI-plane packages with unbounded transitive deps the root plane pins -- are not in this commit. * feat(deps): detect a production upper bound the CI plane cannot keep (#17558, #17557) Items 2 and 4. `check_requirements_ci_drift.py` compares packages declared in BOTH planes, so #17557 was invisible to it: `opentelemetry-api` was pinned in one plane and merely INSTALLED in the other, dragged unbounded by `chromadb==1.5.9`. No pair to compare, so the check was correctly silent while the planes diverged, and the coincidence ended when 1.45.0 shipped -- eight unrelated PRs red, none of which had touched a requirements file. The new check enumerates the shape instead of waiting for it: transitive closure of the CI plane, intersected with the names the production plane bounds ABOVE, minus what the CI plane declares. TWO REFINEMENTS, BOTH LOAD-BEARING. Either alone gives a wrong answer. A bare `>=` floor is safe under an unbounded CI resolve, because newest satisfies it -- so the trigger is an upper bound the CI plane does not restate. Filtering on `==` alone finds one of three candidates here. A requirer that bounds the package above ITSELF makes the agreement structural. `langchain` requires `langgraph<1.3.0` where production says `<2.0.0`: stricter, so the resolution cannot escape. Reporting those is the false-positive noise that makes a check ignored, which is #17558's own subject. THE CONTRAST PAIR IS FREE. The four OpenTelemetry packages are silent now and would have fired before #17502 declared them. The check reproduces #17557 when the fix is absent and says nothing when it is present. IT FOUND ONE THING THE MEASUREMENT THAT DESIGNED IT GOT WRONG. That sweep recorded only the FIRST requirer per package and reported `protobuf` as structurally enforced. `protobuf`'s first-reached requirer caps it at `<8.0`; its fourth, `onnxruntime`, requires `protobuf>=4.25.8` with no upper bound at all. Production caps `<8.0.0`, so CI takes 8.0 the day it ships. A first-match sweep reports a property of its iteration order as a property of the world; the closure docstring says it records every requirer and names this as the reason. So BASELINE is two entries. `protobuf` is filed as #17582 rather than left as a footnote -- the fix is #17502's applied again, but it moves what CI installs, and that cap was already got wrong once and re-derived under #15070 with `onnxruntime` absent from the analysis. It deserves its own review. A SEPARATE MODULE, deliberately, against the same-file instinct: the drift checker is purely static and reads requirement files only. This must read installed distributions to know what the CI plane drags in, and folding a metadata-dependent question into a static guard would make the static one environment-dependent -- the conflation #17558 exists to remove. It imports the drift checker's parsers, so the planes are still parsed in one place, and it names the interpreter its closure came from. rule is stated as data. `GUARD_INPUT_PATHS` is declared and the checker is registered with the reach meta-guard, which #17542 exists because this one's sibling was not. 19 tests, none of which read the real environment: the closure is injected so the rule is stated as data. REGISTERED WITH THE REACH META-GUARD, and that registration found a defect in the meta-guard. `GUARD_INPUT_PATHS` alone does nothing -- #17542 exists because this checker's sibling declared it and was never added to `_GUARDED_CHECKERS`. Adding it made `check_code_quality_guard_reach.py` fail with `AttributeError: 'NoneType' object has no attribute '__dict__'`: its `_load_module` never registered the module in `sys.modules`, and `@dataclass` resolves annotations through `sys.modules[cls.__module__]`. So that loader could not read ANY guarded checker defining a dataclass, and the error named neither the cause nor the file. `repo_tests/dependency_floor_banner.py` documents the same trap for the same reason; this is that fix one guard over, plus a `sys.modules.pop` on failure so a half-executed module is never left registered for the next load to reuse. The reach audit now covers 19 guarded paths, up from 15. THE GUARD COMMITTED ITS OWN DEFECT AND THE PRE-PUSH HOOK CAUGHT IT. The first version keyed BASELINE on package names alone, so an interpreter lacking a requirer saw no pair and the bidirectional contract fired a confident "torch is no longer an unenforced pair". The hook runs a leaner interpreter than this was written against (#16715): torchmetrics absent, verdict inverted. The contract was right about its data; the data was the problem -- *did not look* rendered as *nothing found*, inside the change built to stop that. Two fixes. BASELINE now maps each package to the unbounded requirer that makes it a pair, so the check asks "is that requirer installed here?" before calling an entry stale. And evaluability is per ENTRY, not per plane: a coverage threshold was the first attempt and the wrong instrument, because the interpreter that broke it held most of the CI plane and still lacked the one requirer that mattered. audit() returns (problems, notes, pairs). A pair this interpreter cannot judge is neither a failure nor a pass: gating it would block every push from a leaner environment, dropping it would be the silence this guard exists to prevent. Same gate-versus-report split as the floor reporter in the commit below. Verified in both: the full venv reports 2 known pairs and 0 not evaluated, exit 0; python3.10 reports 1 known pair, 1 not evaluated with the requirer named, exit 0. * fix(guards): re-pin two test-module reach floors past their allowance (#17558) Both floors sat 201 over an allowance of 200 on the merge of this branch with main, so `reach_declarations_test.py` failed twice for the same reason from two angles. Accumulation, and this branch is the module that crossed the line. The allowance absorbed 200 test modules added across every session since the last pin; this branch adds exactly one, `repo_tests/ci_plane_unenforced_pins_17558_test.py`, and 201 - 1 = 200 is precisely the allowance. Without it the gap passes. So the crossing is shared and the last step is mine, which is what the allowance is for. Re-pinned to the measured live populations -- 2540 -> 2741 and 2527 -> 2728 -- rather than widening `growth`. That tolerance is what let two hundred modules land without a re-pin; raising it makes the next crossing later and larger, which is the opposite of what a ratchet is for. Not reproducible before merging main: this branch was 4 commits behind and measured 196, under the allowance. CI evaluates the merge, so the numbers only matched after `git merge origin/main` -- 2741 and 2728, the exact figures CI reported. Worth stating because "cannot reproduce locally" was the first reading and it was an artefact of the branch's base, not a flake. Main had not re-pinned: `origin/main` still declared 2540 and 2527 when checked, so this was nobody's housekeeping yet. * fix(deps): four review findings — count declarations, exclude absences, parse markers (#17558) All four are the branch's own family: a value accurate about something other than the question asked. COUNT DECLARATIONS, NOT NAMES. `absent` is de-duplicated, so `len(declarations) - len(absent)` credited the extra declarations of an uninstalled package as compared. A package declared in two service files, or reached twice through `-r`, made the pass line report "N of M compared" with the wrong N -- this issue's own defect, one layer inside the fix for it. ABSENCES ARE NOT COMPARISONS. With `--require-present` an absent declaration becomes an ABSENT shortfall while `compared` excludes it, so the headline could read "12 of 5". Absences already have their own line; they no longer sit in the numerator of a comparison count. A MARKER IS NOT A SPECIFIER. `bounds_above` scanned the whole requirement suffix, so `widget>=1.0; python_version < "3.10"` looked capped because of the `<` in its MARKER -- turning an unbounded requirer into a structurally bounded one and HIDING a real pair. A false negative in the direction that matters for a guard whose job is finding pairs nothing bounds. `specifier_of` parses and returns only the specifier; an unparseable requirement drops everything after `;` rather than scanning it, under-claiming a bound instead of inventing one. `extra=="x"` IS AN EXTRA. The optional-dependency filter was a substring check for `"extra =="`, which missed valid metadata written without spaces; a retained optional requirement invents a pair nothing installs. `is_optional` now evaluates the marker with an empty extra. `packaging` added to the code-quality install step: that job installs its tools explicitly and never the application's dependencies. Both checkers degrade rather than crash without it, and degrading silently is what this branch removes. The additions took check_dependency_floors_test.py to 614 against the 600 ceiling, so the render-facing tests move to pipeline-scripts/dependency_floor_report_test.py (542 and 119). The ceiling is not raised and the seam follows the module split they test. Six tests added; four mutations each fail one -- counting per unique name, returning ABSENT shortfalls to the numerator, scanning the whole suffix, and restoring the substring extras check. * fix(tests): the new report test leaked its module into every later test (#17558) `dependency_floor_report_test.py` installed `check_dependency_floors` in `sys.modules` so `exec_module` could run, and never removed it. The key is needed only WHILE the module body executes -- a dataclass defined in it resolves its own module by name. Left installed, a `pipeline-scripts/` module sits in the `sys.modules` of every test that runs afterwards. The leak guard caught it: LEAK: pipeline-scripts/dependency_floor_report_test.py leaves 1 sys.modules key(s) installed outside pipeline-scripts/ Install and remove in the same try/finally, which is what the guard's own message asks for. The `checker` reference the tests use stays valid; only the global registry entry goes. Worth noting where this came from: the file is new in the previous commit, and that commit was written and never pushed. The suite it belongs to reported "112 passed" while exiting non-zero, because the leak guard reports after the summary line -- reading the count and not the exit code would have shipped it. Refs #17558 * fix(deps): unreadable metadata is neither compared nor below floor (#17558) Two review findings on 211d3a7, both the defect this change exists to remove, found inside the change itself. UNREADABLE IS NOT A COMPARISON. `UNREADABLE` is a VALUE in `installed`, not an absence, so `name in installed` counted an installed-but-unreadable package as compared when nothing had been compared at all. `compared` now requires the installed version to be readable. UNREADABLE IS NOT BELOW FLOOR EITHER. `shortfalls()` emits `Shortfall(d, UNREADABLE)`, and the headline numerator excluded only ABSENT -- so an unverifiable version was reported as "below its declared floor", a verdict no measurement supports. It is now excluded from that numerator and reported as unverified on its own line. *Did not measure* had been rendering as *measured and found wanting*, which is the sentence this PR is about. THE ABSENCE COUNT NAMED THE WRONG UNIT. `not_installed` is de-duplicated by package name, so two files declaring one absent package rendered as "1 declared but NOT INSTALLED" while two declarations went uncompared. The line now carries both units, because the complement of `compared` must be countable in the same unit as `compared` or the two numbers cannot be read together. Three tests added, three mutations each fail one. One existing test asserted the old wording and is updated with the reason recorded rather than silently repointed. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: t <t@example.invalid>
This was referenced Oct 4, 2026
mrveiss
added a commit
that referenced
this pull request
Oct 4, 2026
…every dropped watch-folder event, and the #17502 revert fixture (#17822, #17547, #17558) #17822 finding 1 -- `repo_tests/secret_access_check_is_wired_16982_test.py`'s `_references_symbol` was `if _SYMBOL not in source`, raw text over the whole file. The owner's mutation on #16982 (2026-09-29): remove the real call and leave the docstring that describes it, and the guard asserting a security rule is wired stays green with the rule gone. Measured on that exact mutation -- old finder GREEN, new finder RED. The finder now walks `Name`, `Attribute`, `alias` and `getattr`-family string arguments, with a nine-case contrast pair (four prose shapes that must not match, five code shapes that must). #17547 AC3 -- `services/kb_folder_events.py` bound `logger` and made zero `logger.` calls, against 30 in the module it was split out of. All four early returns in `_handle_change` dropped a watch-folder event with no trace, so "the KB ignored my file" and "watchdog never fired" read the same. Each now names the folder, path, change type and reason; the failed cross-thread hand-off (#15636) is a warning where the three routine drops are debug. A structural test fails if a fifth silent return is ever added. #17558 Gap 2 AC2 -- only the inverse assertion existed, which a check that reports nothing ever satisfies. Reverting #17502 against the real requirement files reported three of #17557's four packages: `opentelemetry-proto` was silent because both its requirers say `==1.44.0`, which reads as a cap. It is not one -- those requirers are themselves unpinned once the fix is reverted, so the cap travels with them. `floating_packages` resolves that to a fixed point. Blast radius measured: the tree's reported set is `['protobuf']` before and after, and the fixture now reports all four. Closes #17822 Closes #17547 Closes #17558
mrveiss
added a commit
that referenced
this pull request
Oct 4, 2026
…every dropped watch-folder event, and the #17502 revert fixture (#17822, #17547, #17558) Closes #17547 Closes #17558 Closes #17822 Also carries the first consolidation under the canonical-utilities programme (#13916): - tools/lint/_comment_syntax.py is now the one comment stripper; 11 private copies migrated and an AST ratchet pins the remaining 33 so the count can only fall. Four of those were behaviour fixes, not moves: a `#` inside a string had been truncating real lines at their own data. - tools/lint/_scan_helpers.configure_logging replaces THIRTEEN byte-identical private copies, with a guard that fails when a checker re-declares a helper _scan_helpers owns. Refs #17941, #13916, #17970
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Two halves of one surface, in one PR because a guard asserting the two planes agree is useless if it can only read one of them.
#17448. A drift guard already exists —
check_requirements_ci_drift.py(#14551), wired in CI with a shrink-only allowlist. It checks presence, and says so in its own docstring: "What this guard does NOT do: merge the two dependency sets." So a package declared on both planes as==2.0.54and>=2.0.54passes it cleanly. That is exactly how the SLM test venv took SQLAlchemy 2.1.0 while CI's own venv stayed on 2.0.54 — 2.1 droppedgreenletas 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). Three witnesses, three point fixes, one untouched mechanism.Measured: 78 packages appear on both planes, 52 can resolve differently. The enumeration is the deliverable; SQLAlchemy is just what surfaced first.
Two things a naive version gets wrong, both found by measuring rather than reasoning:
production_requirement_namesflattens both backends with last-wins, and they are allowed to disagree —websocketsis capped<16in autobot-backend because langgraph-sdk needs it, and floored>=17.1in autobot-slm-backend, which has had its own venv since ci: run SLM backend tests in their own venv, so CI stops forcing the backend's websockets<16 cap onto the SLM #16394, each documented at its own site. A merged comparison reports that deliberate split as a conflict — training people to ignore the guard — and misses real divergence, because last-wins discarded one constraint before the comparison ran.>=2.0.52and>=2.0.54look different and install the same thing: both take whatever published last. Divergence needs one side pinned or capped. A string comparison would have been wrong in both directions.#17449.
check_dependency_floors.pycould only 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.What Changed
--venvon the floor checker, taking a venv directory or an interpreter path and querying it over a script that imports 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 the 73 existing tests and their stubs were untouched rather than edited to fit the addition.Using it immediately found a hole in the instrument, which is the more valuable half. A deployed venv carries two
dist-infodirectories for one distribution — #15063's append-only venv, wherepip install -rupgrades but never removes. With two of themimportlib.metadata.version()answers nothing; the old code storedNone;shortfalls()treatedNoneas absent; absence is not a shortfall by default. That floor was silently never checked while the environment reported fully satisfied — nothing found and did not look returning the same green, inside the module built to distinguish them.Three states now: readable, absent, and installed-but-unreadable. 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. 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 no longer offers CI-parity advice either, which would tell an operator to fix production by running a local parity script.
Verification
Five mutations, each verified: an unbaselined divergent pair, a stale baseline entry, a missing interpreter, a directory with no interpreter, and unreadable-vs-absent. 106 tests (22 drift + 84 floors), including the 73 pre-existing ones unchanged.
A 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 wereimportlib.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.Stated limits, so nothing here reads wider than it is:
62041b574cwas. Those two workflows would have been reported clean, correctly and uselessly. The 52 pairs are the measured half; enumerating install sites that consult no constraint file is a different detector and is recorded on bug(ci): the CI venv and the service venv pin the same package differently and resolve to different releases #17448 rather than attempted here.dist-infocondition is filed separately as bug(deps): duplicate dist-info in a deployed venv makes importlib.metadata answer nothing, and every consumer reads that as 'not installed' #17501, because the floor checker is one consumer of a malformed environment, not the only one — and I measuredversion()'s behaviour, notdistribution()'s, which is stated there rather than assumed.Refs, notCloses: both issues keep criteria this PR does not deliver.Model Used
Claude Opus 5 (1M context)
Refs #17448
Refs #17449
Summary by CodeRabbit