Skip to content

Python: Reject out-of-range Cosmos memory search parameters - #9318

Merged
Eduard van Valkenburg (eavanvalkenburg) merged 2 commits into
microsoft:mainfrom
Lesereingrape:fix/cosmos-memory-search-params-unvalidated
Oct 15, 2026
Merged

Eduard van Valkenburg (eavanvalkenburg) merged 2 commits into
microsoft:mainfrom
Lesereingrape:fix/cosmos-memory-search-params-unvalidated

Conversation

@Lesereingrape

Copy link
Copy Markdown
Contributor

Motivation & Context

CosmosMemoryContextProvider documented min_confidence as a 0.0-1.0 score (_context_provider.py:130, and README.md:325 / :347) and then assigned both retrieval knobs straight to self without checking either one (:159-160). Both are forwarded verbatim into the toolkit on every turn (:393, :395, :400), and against the real azure-cosmos-agent-memory 0.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 from aio/store/memory_store.py:861) and raises ValidationError("top_k must be a positive integer"). The provider's own except (: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-513 builds AND c.confidence >= @min_confidence with the value as written. Measured with 1.5, the predicate can never be satisfied, so every retrieval is empty and nothing is logged at all.
  • min_confidence < 0: the same min_confidence > 0 guard 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

  • What are the major changes? Two small edits in python/packages/azure-cosmos-memory/agent_framework_azure_cosmos_memory/_context_provider.py (8 lines added) and three tests (19 lines added):
if top_k <= 0:
    raise ValueError("top_k must be greater than 0")
if not 0.0 <= min_confidence <= 1.0:
    raise ValueError("min_confidence must be between 0.0 and 1.0")
  1. the guards sit in __init__ immediately before the two assignments they protect;
  2. the Raises: block in the class docstring names the new contract next to the existing SettingNotFoundError entry.

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 in azure-ai-search/_vector_store.py:789 and mongodb/_vector_store.py:1067.

The tests are placed inside TestInit, right after test_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 raises ValueError for values that would otherwise degrade retrieval later, which localises the mistake to the line that set it. Callers who were passing min_confidence=90 or top_k=0 do 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:

    1. Bounds are inclusive at 0.0 and 1.0. min_confidence=0.0 is accepted even though the toolkit's min_confidence > 0 guard means it produces no predicate — that matches the documented meaning of "no filtering", so rejecting it would be stricter than the contract. 1.0 is likewise kept (exactly-matching memories still qualify).
    2. The guard is constructor-only. top_k and min_confidence are plain public attributes, so a caller who assigns provider.top_k = 0 after 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 main at 726dcff8:

  • Before the source change, the two rejection tests fail on main (Failed: DID NOT RAISE ValueError, tests/test_context_provider.py:138 and :144): 2 failed, 1 passed, 53 deselected. The third test already passes and is kept to pin the inclusive boundaries.
  • After it, tests/test_context_provider.py passes in full: 56 passed (the 53 that pass on main, plus the 3 new).
  • The toolkit side was measured against the real azure-cosmos-agent-memory 0.3.0b2 rather than assumed: top_literal(0) and top_literal(-1) both raise ValidationError, and _build_memory_query_builder(memory_types=["fact"], min_confidence=1.5) emits WHERE c.type IN (@memory_type_0) AND c.confidence >= @min_confidence with bind value 1.5, while min_confidence=-0.5 emits no confidence predicate.
  • The repo's pinned linter/formatter, ruff==0.16.10, run from the package directory: ruff check --no-fix . → "All checks passed!", ruff format --check . → "10 files already formatted".
  • pyright 1.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 symbol in samples/*.py; none of those files is touched by this PR, and they come from my isolated venv resolving agent-framework-core 1.21.0 from the PyPI wheel instead of the workspace source.
  • mypy 2.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.py in python/packages/redis/). The last commit to touch this provider file is 723d8caf (#9223), so these hunks are untouched on current main.

AI Assistance

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

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, pyright 1.1.414, mypy 2.4.0 and the package's own test module on 726dcff8, 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

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. No breaking change label 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Oct 12, 2026
@eavanvalkenburg

Copy link
Copy Markdown
Member

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Merged via the queue into microsoft:main with commit f34d6d6 Oct 15, 2026
45 checks passed

This branch was previously deployed

1 inactive deployment
github-app-auth — dc367b1a Deployed Oct 12, 2026 by Lesereingrape via add_label #25071
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: Out-of-range Cosmos memory search parameters silently disable retrieval

3 participants