fix: deduplicate session validation logic and harden EmbeddingProvider#154
Conversation
|
Hi @johnnichev! I've resolved the merge conflicts with the latest main branch, preserved the validation changes from both sides, and reran the session-related test suite locally. |
|
lovely! thanks for pushing more code to selectools, I'll review right now :) |
|
Thanks for picking this up, the module-level extraction is the right call and Redis and Mongo look good. Two things before it can go in: The dedup is only half done. Supabase and DynamoDB still define the old Also the four embeddings files each picked up a stray blank line before Drop those and it's good to merge. |
vaishnavidesai09
force-pushed
the
fix-140-session-validation-deduplication
branch
from
dca6d18 to
bb36ea7
Compare
|
Hi @johnnichev, I've addressed the review feedback:
Thanks for the review! |
|
I removed the dead Supabase and DynamoDB validation helpers and updated the Supabase validation tests to use the shared module-level validation helpers. I also reran the full session-related test suite locally (7459 passed, 215 skipped). CI is now passing on the updated branch. |
johnnichev
left a comment
johnnichev
left a comment
There was a problem hiding this comment.
The module-level extraction is the right call and the dead Supabase/DynamoDB helpers are gone now, so this closes #140 cleanly. Scoping the branch down to just the session-validation cleanup was the right move too. Merging. Thanks for the contribution! I'll do a small follow-up pass on the test polish myself.
…ation tests (#155) Adds a `lint` job to CI (ruff check + ruff format --check on src/ + tests/), pinned to ruff 0.15.8 to match the ruff-pre-commit rev. Until now nothing linted the whole tree: the pre-commit hook only sees changed files and CI had no ruff step, so lint debt accumulated invisibly. `build` now needs lint. Enabling that gate required clearing the pre-existing debt in tests/ (src/ was already clean): - 411 F401 unused-imports: removed the dead ones; the 45 intentional availability probes (`from google.genai import types` / `import numpy` inside try/except ImportError or skipif guards) get an explicit `# noqa: F401` since deleting them would break skip logic. - 92 F841 unused-variables: dropped the dead bindings while preserving the side-effecting calls (e.g. `result = agent.run(...)` -> `agent.run(...)`). Also hardens the four Supabase namespace-validation tests carried in by #154: they had been re-pointed at the module-level `_validate_namespace` in isolation, leaving an orphaned `store` local (4x F841) and exercising no store path. Routed them back through `store._key("sid", namespace=...)` so the validator's wiring into _key is actually asserted, which also removes the duplicate/unsorted import. Full suite: 7469 passed, 205 skipped, coverage 91.01% (gate 90%). Claude-Session: https://claude.ai/code/session_01HQhq1HKYipPa6rjQAzffcg
2 participants
Summary
This PR centralizes session identifier validation logic into shared module-level helper functions and adds a default
_dimensionattribute to the embedding provider base class.Changes
_validate_session_id()helper._validate_namespace()helper for consistent namespace validation._dimension: int = 0attribute toEmbeddingProviderto provide a safe base implementation for subclasses.Validation
Verified session-related functionality by running:
pytest tests/test_sessions_edge_cases.py -q pytest tests/test_sessions* -qResults:
tests/test_sessions_edge_cases.py: 15 passed