Repository navigation
Python: Reject out-of-range Cosmos memory search parameters - #9318
Merged
Eduard van Valkenburg (eavanvalkenburg) merged 2 commits intoOct 15, 2026
Conversation
Grapette.L (Lesereingrape)
requested review from
SergeyMenshykh,
Tao Chen (TaoChenOSU),
Eduard van Valkenburg (eavanvalkenburg),
Giles Odigwe (giles17),
Jose Alvarez (jpalvarezl),
Evan Mattson (moonbox3),
Roger Barreto (rogerbarreto),
sophia-ramsey and
westey (westey-m)
as code owners
October 12, 2026 16:08
Grapette.L (Lesereingrape)
temporarily deployed
to
github-app-auth
October 12, 2026 16:08 — with
GitHub Actions
Inactive
Grapette.L (Lesereingrape)
temporarily deployed
to
github-app-auth
October 12, 2026 16:08 — with
GitHub Actions
Inactive
Grapette.L (Lesereingrape)
temporarily deployed
to
github-app-auth
October 12, 2026 16:13 — with
GitHub Actions
Inactive
Member
|
/review |
Contributor
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (2 commit(s)): 7000128a8dae, dc367b1acaba
Model: gpt-5.6-sol
Overview
The PR moves validation of the documented retrieval bounds to provider construction, before client creation or retrieval, and preserves valid defaults and inclusive confidence boundaries. Focused unit tests cover non-positive top_k, out-of-range and non-finite confidence values, and accepted boundary values. No Critical, High, or Medium defect introduced by this change was established.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
Eduard van Valkenburg (eavanvalkenburg)
approved these changes
Oct 15, 2026
This branch was previously deployed
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
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.
Motivation & Context
CosmosMemoryContextProviderdocumentedmin_confidenceas a 0.0-1.0 score (_context_provider.py:130, andREADME.md:325/:347) and then assigned both retrieval knobs straight toselfwithout checking either one (:159-160). Both are forwarded verbatim into the toolkit on every turn (:393,:395,:400), and against the realazure-cosmos-agent-memory0.3.0b2 each out-of-range value fails silently at retrieval instead of at the call site that has the mistake:top_k <= 0: the toolkit validates it deep in the query builder (store/_search_helpers.py:32-38, reached fromaio/store/memory_store.py:861) and raisesValidationError("top_k must be a positive integer"). The provider's ownexcept(:404) turns that into one warning line, so the agent keeps running with zero memories injected — permanently, with a log line as the only trace.min_confidence > 1.0:_utils.py:512-513buildsAND c.confidence >= @min_confidencewith the value as written. Measured with1.5, the predicate can never be satisfied, so every retrieval is empty and nothing is logged at all.min_confidence < 0: the samemin_confidence > 0guard skips the confidence predicate entirely (measured with-0.5), so the filter the caller configured silently does nothing.Reported with measurements in #9317.
Description & Review Guide
python/packages/azure-cosmos-memory/agent_framework_azure_cosmos_memory/_context_provider.py(8 lines added) and three tests (19 lines added):__init__immediately before the two assignments they protect;Raises:block in the class docstring names the new contract next to the existingSettingNotFoundErrorentry.The messages follow the repo's existing convention for exactly this shape of parameter:
"temperature must be between 0.0 and 2.0"/"top_p must be between 0.0 and 1.0"/"max_tokens must be greater than 0"(core/agent_framework/_types.py:4863,:4868,:4872), and the retrieval-shape knobs inazure-ai-search/_vector_store.py:789andmongodb/_vector_store.py:1067.The tests are placed inside
TestInit, right aftertest_init_default_values, which already asserts the defaults this change must not disturb.What is the impact of these changes? Nothing moves for valid input: the documented defaults (
top_k=5,min_confidence=0.7), every in-range value, and both inclusive boundaries (top_k=1,min_confidence=1.0) behave exactly as before — the third test pins that. What changes is the previously-silent group: the constructor now raisesValueErrorfor values that would otherwise degrade retrieval later, which localises the mistake to the line that set it. Callers who were passingmin_confidence=90ortop_k=0do see a new error; that is the point, since those configurations already returned no memories.What do you want reviewers to focus on? Two judgement calls I made and would like a steer on:
0.0and1.0.min_confidence=0.0is accepted even though the toolkit'smin_confidence > 0guard means it produces no predicate — that matches the documented meaning of "no filtering", so rejecting it would be stricter than the contract.1.0is likewise kept (exactly-matching memories still qualify).top_kandmin_confidenceare plain public attributes, so a caller who assignsprovider.top_k = 0after construction still bypasses this and still lands in the swallowed-error path. I kept the change minimal rather than converting them to read-only properties or re-checking inside the retrieval method, but if the team would rather close that door too, say which shape you prefer and I will follow up on this branch.Local verification, on
mainat726dcff8:main(Failed: DID NOT RAISE ValueError,tests/test_context_provider.py:138and:144):2 failed, 1 passed, 53 deselected. The third test already passes and is kept to pin the inclusive boundaries.tests/test_context_provider.pypasses in full: 56 passed (the 53 that pass onmain, plus the 3 new).azure-cosmos-agent-memory0.3.0b2 rather than assumed:top_literal(0)andtop_literal(-1)both raiseValidationError, and_build_memory_query_builder(memory_types=["fact"], min_confidence=1.5)emitsWHERE c.type IN (@memory_type_0) AND c.confidence >= @min_confidencewith bind value1.5, whilemin_confidence=-0.5emits no confidence predicate.ruff==0.16.10, run from the package directory:ruff check --no-fix .→ "All checks passed!",ruff format --check .→ "10 files already formatted".pyright1.1.414 with the repo's own config: 0 errors on the changed source module (it is the sole source-code checker here). Running it over the whole package reports 3 errors, all of them"FoundryChatClient" is unknown import symbolinsamples/*.py; none of those files is touched by this PR, and they come from my isolated venv resolvingagent-framework-core1.21.0 from the PyPI wheel instead of the workspace source.mypy2.4.0 with the repo's[tool.mypy]config: "Success: no issues found in 1 source file" on the changed test file.Related Issue
Fixes #9317
I checked for competing open PRs before filing: of the 189 currently open PRs in this repository, none touches
python/packages/azure-cosmos-memory/. The two that look closest on a title search are #8875 (Cosmos DB checkpoints, a different package:python/packages/azure-cosmos/) and #9063 (a_context_provider.pyinpython/packages/redis/). The last commit to touch this provider file is723d8caf(#9223), so these hunks are untouched on currentmain.AI Assistance
AI assistance details: the guards, the docstring line, the three tests and this description were drafted by the Qoder coding assistant working from the repro written up in #9317, then run locally through the checks listed above (pinned
ruff==0.16.10,pyright1.1.414,mypy2.4.0 and the package's own test module on726dcff8, before and after the source change). The diff is 27 added lines across two files so it is quick to audit. The checkbox above is left for the human submitter to tick, since accepting responsibility is theirs to state.Contribution Checklist
breaking changelabel requested: the documented defaults, all in-range values and both inclusive boundaries behave exactly as before; only values outside the documented range — which already failed to retrieve anything, silently — now raise at construction. Callers passing such a value do see a new error, which is the point of the fix and is noted under "What is the impact of these changes?" above.