Repository navigation
fix(sdk/python): make memory exists() detect a missing key - #1106
ZeroPoint95 merged 3 commits into
Conversation
MemoryClient.exists() called get() and reported True whenever it did not raise. get() answers the control plane's deliberate 404 for an absent key with the caller's default instead of raising, so exists() returned True for every key as long as the backend was reachable, and the scoped, global and MemoryInterface surfaces inherited that answer. Agent-Field#966 documented the behavior and left the implementation fix for its own PR. Decide presence against a private sentinel passed as get()'s default: a stored value -- including a falsy one such as None, 0 or "" -- means True, a 404 means False, and a transport or backend failure still means False. get()'s default-on-404 contract is unchanged. This matches the TypeScript sibling, which decides presence from the returned value rather than from "did not raise". The four docstrings Agent-Field#966 rewrote now state the real behavior again. Refs Agent-Field#966
…mory Agent-Field#966 recorded the exists() limitation in five places: the four docstrings in memory.py and the exists bullet of the Agent.memory "Memory Operations" API list. The previous commit corrected the four; this completes the set so the advertised API list no longer contradicts the implementation it describes, and drops the get(key, sentinel) workaround that is no longer needed. Refs Agent-Field#966
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Performance
✓ No regressions detected |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
ZeroPoint95
left a comment
There was a problem hiding this comment.
@remote-controlled-man, this addresses the bug documented in #966 cleanly. The sentinel distinguishes missing keys from stored falsy values across the scoped and global APIs, including checks through the real HTTP client boundary. Thanks for fixing it; no changes requested from this review.
Summary
MemoryClient.exists()answered "is this key stored?" by callingget()and returningTrueunless it raised. Butget()turns the control plane's deliberate 404 for a missing key into the caller'sdefault:So nothing ever raised for an absent key, and
exists()returnedTruefor every key as long as the memory backend was reachable.ScopedMemoryClient.exists,GlobalMemoryClient.existsandMemoryInterface.exists— i.e.app.memory.exists— all delegate to that one implementation, so all four public surfaces gave the same wrong answer, and existence-gated logic (cache warm-up, "have I stored this already?", idempotent writes) silently took the wrong branch.#966 documented exactly this — item 7, "
exists()cannot detect a missing key" — rewrote the four docstrings to describe the limitation and point callers at aget(key, sentinel)workaround, and noted that "the existing unit tests miss this because they monkeypatchget()to raise on a miss, which the realget()never does". It then left the fix out on purpose: "This is a documentation change only — the implementation fix belongs in its own PR." This is that PR.Presence is now decided against a private sentinel passed as
get()'s default:True, including a falsy one (None,0,""), which anis not Noneshortcut would get wrong;False;False, so the existing error-handling tests keep passing unchanged.get()'s documented default-on-404 contract is untouched, and the docstrings state the real behavior again instead of the limitation plus workaround — the fourexists()docstrings inmemory.pyand theexistsbullet of theAgent.memory"Memory Operations" API list inagent.py, which #966 had rewritten in the same pass. This also brings Python in line with the TypeScript SDK, which decides presence from the returned value rather than from "did it throw":The Go SDK exposes no
Exists, so no third surface is affected.Type of change
Test plan
cd sdk/python && python -m pytest tests/test_memory_client_core.py -k exists -rf→ 2 failed, 2 passed, both failuresassert True is Falseagainst a stubbed 404 (test_exists_is_false_when_the_control_plane_returns_404and, through the scoped surface,test_scoped_exists_is_false_for_a_missing_key). The stub drives_async_request, notget()— that is the gap docs: correct memory scope docs, the app.memory API list, and the exists() promise #966 identified in the existing tests.tests/test_*memory*.pyfiles → 114 passed, 2 deselected.null→True) so this cannot later regress intois not None.ruff check .→All checks passed!with the CI-pinnedruff==0.15.22;mypy --config-file mypy.ini agentfield/→Success: no issues found in 83 source fileswithmypy==1.18.2. Python 3.12.7 on Windows. The new lines areruff format-clean; the one remaining format proposal in the test file is pre-existing onmain(it appears at@@ -282there) and was left alone rather than mixed into a focused fix.memory.py=2dda43d47f059456640d4386bce40621fa90e686) and re-running the four affected files, which gives the identical 10 deterministic failures with identical node IDs (7 ×test_log_writerCRLF/pipe-timing, 2 ×test_harness_packagingGBKUnicodeDecodeErrorfrompathlib.read_text, 1 ×test_tool_calling.py::TestExecuteToolCallLoop::test_single_tool_call_then_response); the twelfth node istest_litellm_observability's known cold-cache flake and passed on this run.tests/test_agent_mesh.pyis excluded because it does not complete on this Windows host — a pre-existing hang, unrelated to this change. Left to CI onubuntu-latest../scripts/test-all.shand./scripts/coverage-summary.shwere not run locally: they also build the Go control plane, the web UI and the desktop app, and the coverage summary needs a Docker engine, which is unavailable here. This PR changes onlysdk/python/**, which with the workflow file itself is the complete path filter of.github/workflows/sdk-python.yml.control-plane/internal/handlers/memory.go(GetMemoryHandler, scope walk at:273, 404 at:288) and matches what docs: correct memory scope docs, the app.memory API list, and the exists() promise #966 measured against a running server.Test coverage
Falseon the client, 404 →Falsethrough a scoped client (asserting the scope still travels), and a stored falsy value →True.coverage-baseline.json— not applicable; no code was removed../scripts/coverage-summary.shmeasures all five surfaces and needs a Docker engine.Unlike most SDK modules,
agentfield.memoryis inside the nine-module--covwhitelist insdk/python/pyproject.toml's pytestaddopts, so the gate does measure this change and the new lines are exercised by the three tests above.pyproject.tomlandcoverage-baseline.jsonare untouched.Checklist
docs/CONTRIBUTING.mdanddocs/DEVELOPMENT.md.Refs #966, with no closing keyword: docs: correct memory scope docs, the app.memory API list, and the exists() promise #966 is a merged docs PR rather than an issue, and this implements only the fix it deferred.Related issues / PRs
Agent.memorydocstring insdk/python/agentfield/agent.py; fix(python-sdk): use SDK-wide truthy/falsy vocabulary for remaining strict env flags #1090 edits that file's env-flag parsing. The regions are far apart (docstring at ~1641 versus the flag helpers), so whichever merges second should rebase cleanly, and I kept the docstring edit to the minimum needed to stop it contradictingmemory.py.agent_server.py/logger.py(fix(python-sdk): use SDK-wide truthy/falsy vocabulary for remaining strict env flags #1090), the prompt-template surface (feat: configurable PromptTemplates layer across Python/TS/Go SDKs (#229) #1092), the harness CLI (fix(sdk): stop harness retries triggered by prompt text in timeout errors #1104), andmultimodal_response.py/test_usage_transport.py(fix(sdk/python): report a provider-confirmed zero cost instead of unknown #1105).