Repository navigation
fix(security): UUID-only ids; filter expressions accepted, camel-ai>=0.2.60, reranker fallback - #2
Merged
Conversation
The goodmem SDK (0.1.35) interpolates ids into request paths raw and httpx
resolves dot segments before sending, so an id could redirect a request to
another resource. Every GoodMem id is a UUID; anything else is now refused
with GoodMemIdError (a ValueError, like GoodMemUploadError), naming the
argument, before any request is made.
One validator, camel_goodmem/_ids.py require_uuid(value, field), is called at
the top of every id-taking method: get_memory (+content), goodmem_get_space,
update_space, delete_space, list_memories, delete_memory, and create_space's
embedder_id (a body id, checked for consistency). Configured space_ids and
reranker_id are checked at construction and again at every use, since both
are public attributes. Tool signatures declare the ids with the same UUID
pattern, so the CAMEL and MCP tool schemas tell the model; FastMCP also
enforces it. Upper-case UUIDs are accepted and sent lower-case.
Measured against a local server recording every request line, real SDK and
httpx, payloads ../spaces/<U>, a/../../spaces/<U>, %2e%2e/spaces/<U>,
..%2Fspaces%2F<U>, <U>/../../spaces/<U>, "", " <U>", <U>?x=1, <U>#frag,
<U>\n:
Before (0.2.0):
- delete_memory("../spaces/<U>") sent DELETE /v1/spaces/<U> and returned
{"success": True, "memoryId": "../spaces/<U>"}; the same through the
CAMEL FunctionTool and the MCP tool.
- delete_space("<U>/../../spaces/<U>") -> DELETE /v1/spaces/<U>, success.
- get_memory / goodmem_get_space / update_space / list_memories sent
GET|PUT /v1/spaces/<U>[/memories] for the traversals; %2e%2e and ..%2F
were sent verbatim; list_memories("<U>#frag") hit GET /v1/spaces/<U>;
list_memories("") silently listed the configured space.
- Malformed space_ids / reranker_id were sent as-is, and list_memories()
put the configured space id in a URL path.
- New suite on the base code: 336 failed, 9 passed (the 9 are the
valid-UUID controls).
After: every payload at every entry point raises GoodMemIdError and the
server records zero requests; a valid UUID still reaches exactly
DELETE /v1/memories/<U> etc.
Tests: tests/test_goodmem_ids.py (345, added to CI's offline step); fake
ids in tests/test_goodmem_toolkit.py ("space-1", "m-1", "emb-voyage",
"rr-1", ...) replaced with UUIDs. Offline 414 passed on 3.10-3.13; live 29
skipped without credentials; ruff, ruff format, mypy clean. Version 0.2.1;
pydantic>=2 declared (already required by camel-ai and goodmem); mcp<2 pin
kept.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… check broke Follow-up to 9bb49fd, from an adversarial review. No external id reached a URL path unchecked; these are the three problems the review found. 1. A live test was left sending a non-UUID id. TestLiveSpaces:: test_a_rejected_create_carries_the_servers_message called create_space(..., "not-a-uuid") and expected the server's 400. The id check now refuses it with GoodMemIdError (a ValueError, not a GoodMemError) before any request, so the test failed as soon as the live suite ran with credentials. CI never runs it without them. Before: run offline against the recorder, it raised GoodMemIdError and sent nothing. After: it sends a well-formed UUID that names no embedder (00000000-0000-7000-8000-000000000000), expects any 4xx carrying the server's own body and naming the embedder, and cleans up if a space was created. Against the recorder: GET /v1/spaces, POST /v1/spaces -> 404, passes. A new live test, test_a_malformed_embedder_id_is_refused_before_ it_is_sent, asserts that "not-a-uuid" raises GoodMemIdError and that no space is created. 2. reranker_id="" meant "no reranker" in 0.2.0 and is now refused at construction, which was not called out. It is still refused ("" is one of the payloads the check must refuse), but the error now ends "To search without a reranker, pass reranker_id=None or leave it out ...". The README's ids section and its 0.2.1 table name the change and the os.getenv(...) or None idiom, and the constructor docstring says so. 3. Hardening: require_uuid returned value.lower() for a str and str(value) for a uuid.UUID. A subclass could pass the check and still change what was sent. It now copies a str into a plain str with str.__str__, checks that exact text and returns it lower-cased. A uuid.UUID is judged by the text its str() produces. Before, with _SwappingStr(<real UUID>), a str whose lower/__str__/__format__ return "../spaces/<V>": delete_space sent DELETE /v1/spaces/<V> and returned {"success": True, "spaceId": "../spaces/<V>"}. delete_memory sent DELETE /v1/spaces/<V>. get_memory, goodmem_get_space and update_space sent GET or PUT /v1/spaces/<V>. list_memories(v), and list_memories() with space_ids set later, sent GET /v1/spaces/<V>/memories. A uuid.UUID subclass whose __str__ returns the traversal did the same at all seven entry points. After: the str subclass reaches exactly DELETE /v1/memories/<real>, etc., and the UUID subclass is refused with GoodMemIdError with nothing sent. Tests (tests/test_goodmem_ids.py, 345 -> 380): - 7 swapping-str and 7 swapping-UUID cases through the real SDK and httpx - plain-str return checks - empty reranker_id hint, at construction and when set later - a README guard - the two live TestLiveSpaces tests run against the recorder, which now answers an unknown embedder with 404 - an AST check that no live test passes a literal id the validator refuses, outside pytest.raises(GoodMemIdError), plus 7 self-tests for that check Run against 9bb49fd: 24 failed, 356 passed. The 11 new tests that pass there are controls: the check's self-tests, the plain-str inputs and the None-reranker body. Offline 449 passed on Python 3.10, 3.11, 3.12 and 3.13. Live: 30 skipped without credentials and the CI gate holds. ruff check, ruff format --check and mypy are clean. The key and TLS gates are clean. python -m build and twine check pass, and the wheel imports in a clean venv. Version stays 0.2.1: 9bb49fd already bumped it, and 0.2.1 is neither tagged nor on PyPI (released: 0.1.0 and 0.2.0). This commit completes that release. The mcp<2 pin is kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and retriever
The README's "Metadata filters" section built an expression with
camel_goodmem.filters, but GoodMemToolkit took metadata_filter only as a dict
(dict(metadata_filter) then from_mapping), and GoodMemRetriever took no filter
at all. compare / one_of / not_equals / any_of could not be applied through
this package.
Measured against a local server recording every request, real SDK + httpx:
Before:
- GoodMemToolkit(..., metadata_filter=filters.all_of(equals, compare,
one_of)) raised ValueError "dictionary update sequence element #0 has
length 1; 2 is required" at construction.
- GoodMemRetriever(tk, metadata_filter=...) raised TypeError (unexpected
keyword argument).
- README blocks: 3 of 6 failed as written (the filters block and the
retriever block used space_ids=["..."], refused by the UUID check; the
uploads block had no import -> NameError).
- New TestFilterExpressions: 9 failed, 2 passed (the two dict/empty controls).
After:
- metadata_filter is dict | str on both. A dict is the AND of equalities it
always was; a str is a filters-built expression sent verbatim; "" means
no filter; any other type, or a dict value with no safe form (None, list,
dict), raises GoodMemFilterError at construction instead of on the first
search. The retriever's filter is ANDed with the toolkit's, so it can
narrow the toolkit's scope, never widen it. The model still sees only
query/top_k.
- The recorded request carries
(CAST(val('$.tenant') AS TEXT) = 'acme') AND (... >= 2026) AND
(... IN ('note', 'doc')) as spaceKeys[].filter.
- README filters section rewritten with a runnable toolkit + retriever
example; all 6 README python blocks run against the local server.
- TestFilterExpressions 11 passed; offline suite 460 passed; ruff, ruff
format, mypy clean.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r_id outcome_from_events labelled every hit by configuration (reranked=bool(reranker_id)). When the configured reranker fails, GoodMem reports RERANKING_FAILED (and NOT_FOUND naming the reranker) and still returns the vector-stage hits, scored as negative distances. Measured with the captured live stream retrieve_degraded_hits.ndjson (NOT_FOUND + RERANKING_FAILED + one hit, raw -0.5845972) served by a local server to the real SDK, reranker_id set: Before: - goodmem_search: scoreKind "reranker", score -0.5846 (un-negated). - min_score=0.0: 0 of 1 hits returned, UserWarning "min_score=0.0 removed all 1 reranked result(s)" -- Q4a violated, a server-returned hit discarded. - GoodMemRetriever.query(similarity_threshold=0.0): the hit was dropped and only the "No results were returned" row came back. - New tests: 6 failed (one of them the "unrelated status keeps reranker scores" control, which failed only because outcome.reranked did not exist). After: - RetrievalOutcome.reranked is decided once the stream is complete (a RERANKING_FAILED may follow the hits): requested AND no RERANKING_FAILED AND no NOT_FOUND naming the reranker. Hits are scoreKind "vector", score 0.5846; min_score is applied only when outcome.reranked, so the hit is kept with no warning; partial stays true with both statuses. - An unrelated status (e.g. an UNKNOWN code) leaves reranker scores as-is. - README Scores section documents the fallback; 0.2.1 changes table row. - Offline 466 passed; ruff, ruff format, mypy clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The declared floors camel-ai>=0.2.0 and pydantic>=2 were not true.
Measured on Python 3.10, each camel-ai pinned, goodmem 0.1.35, mcp<2:
Before:
- camel-ai 0.2.0, 0.2.10: import camel_goodmem -> ModuleNotFoundError
camel.logger.
- camel-ai 0.2.20, 0.2.59: ModuleNotFoundError PIL (camel-ai's own
undeclared import).
- camel-ai 0.2.60, 0.2.70, 0.2.72-0.2.78: import works, but camel-ai's
BaseToolkit with_timeout wrapper runs each method in a thread and drops
its exception, so the caller gets IndexError: list index out of range.
delete_memory("../spaces/<id>") raised IndexError, not GoodMemIdError;
offline suite 245 failed, 221 passed. 0.2.79 re-raises (diffed
camel/utils/commons.py 0.2.78 -> 0.2.79); 0.2.79, 0.2.80, 0.2.85, 0.2.90:
all pass.
- pydantic: with camel-ai 0.2.79 the resolver's lowest is 2.10.6, where on
Python 3.10 camel-ai's FunctionTool raises "TypeError: issubclass() arg 1
must be a class" for every method annotated -> dict[str, Any], so
get_tools() fails: 375 failed, 74 passed. 2.11.0: 449 passed.
- The floors job below, run locally (its run: blocks extracted and executed
on Python 3.10) against the old pyproject: installs camel-ai 0.2.0,
pydantic 2.0, mcp 0.9.1; fails at "Package imports at the floors".
After:
- camel-ai>=0.2.79, pydantic>=2.11; mcp<2 kept.
- New CI job `floors` (Python 3.10): uv pip install --resolution
lowest-direct -e ., asserts the installed camel-ai/goodmem/pydantic equal
the declared floors (catches an unreachable floor: pydantic>=2 resolves
to 2.10.6 and fails the check) and mcp<2, imports the package, runs the
offline suite. Locally: camel-ai 0.2.79, goodmem 0.1.35, pydantic 2.11.0,
mcp 1.3.0; import ok; 466 passed. With camel-ai>=0.2.60 it fails
(245 failed). Every run: block in ci.yml passes bash -n.
- README: floors stated under Install; the "what CI runs" block includes
the floor commands (executed as written); toolkit test count 69 -> 86
(collected); 0.2.1 changes row. Version stays 0.2.1 (the security commit
bumped it); no CHANGELOG file exists, the README's "Changes in 0.2.1"
table is the changelog.
Note: camel-ai 0.2.79 pins tiktoken<0.8, which has no Python 3.13 wheel, so
the floor is only installable on 3.10-3.12; on 3.13 the resolver picks a
newer camel-ai, which the main test matrix covers.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Local commits from the 2026-09-25 review, each with a regression test that fails before and passes after; full offline suite and CI gates green locally. Ids that reach a URL path must now be canonical UUIDs, checked before any request (the SDK sends
../spaces/<id>as a request to a different endpoint).Commits:
🤖 Generated with Claude Code