fix: address 6 open issues (#64, #150-#154) and bump to v3.3.0 - #157
Merged
Conversation
check_dead_modules computed reachability from every import it found, but never checked that the imported module actually exists — five benchmark and script files kept importing the SQLite backend removed in 3.0.0, and the guard reported "No unreachable modules." while all five would fail to run. It now resolves every root's imports against the tree and reports the ones that don't. stress_at_scale.py measured only the removed backend and is deleted; the other four (rehearsal_coverage_overhead, benchmark_mem0_vs_nm, benchmark_cognee_vs_nm, e2e_gemini_recall) now require a live SURREALDB_URL instead of silently falling back to SQLite.
…write (#151) smem_drift always reported "clean" on the only shipped backend: its two inputs (tag co-occurrence, session summaries) raised on SurrealDB, were caught, and turned into empty results indistinguishable from a real analysis that found nothing. Those two things were never implemented on SurrealDB and never had been -- even before the SQLite backend's removal, temporal/activation drift detection had no working data source. The merge action never merged tags either; it only flipped a status field. Removed entirely: engine/drift_detection.py, mcp/drift_handler.py, the smem_drift MCP tool and its schema, the DETECT_DRIFT consolidation strategy, and the tag-cooccurrence write in the encode hot path. TagNormalizer.detect_drift needs no storage and already backs smem doctor's tag-drift warnings -- it remains and covers the same need. A second, unrelated swallowed failure in the same review: ReflexPipeline periodically tried to persist a session summary via a storage method that was removed with the same backend and never existed on SurrealDB. That write path is removed too; session tracking itself (record_query, get_top_topics) is untouched. BREAKING: the smem_drift MCP tool is gone. Docs/tool count regenerated (57 tools, was 58).
…ain (#152) hub_status and hub_devices take brain_id from the URL path and called storage.set_brain(brain_id) on the process-wide shared storage instance to answer their own lookup. Background maintenance loops (consolidation, decay) read storage.brain_id off that same shared instance on every tick, so a read-only GET for one brain could redirect the next scheduled pass onto a brain the operator never switched to. Both endpoints now read through storage_for_scope, a scoped-storage helper moved to server/dependencies.py from reasoning_training.py (the same pattern already protected its read endpoints with) -- it reuses the shared instance when it is already bound to the requested brain, or opens an isolated one otherwise, without ever mutating the shared instance's brain as a side effect of a read.
Every existing job either mocked the database or skipped whenever SURREALDB_URL was unset, so the only production backend never ran in CI -- the exact gap that let a query returning plausible garbage (#143's SELECT VALUE bug) reach main undetected by its own new test, which had been skipping since the day it was added. The job starts the official image via a plain `docker run` step: GitHub Actions' `services:` only accepts `options:` (docker-create flags), never a container command, and the image's ENTRYPOINT has no default CMD -- `services:` cannot pass the `start ...` arguments SurrealDB requires. Waits on `/surreal isready`, then runs pytest sequentially (no -n auto: parallel workers sharing one connection produce transaction-conflict errors that are about contention, not correctness). Verified end-to-end against a throwaway container matching this recipe: full suite passes in ~2m12s with zero flakiness.
- Dashboard tool-stats: get_tool_stats took no `days` argument, so switching the 7/30/90-day range filter changed the daily chart but not the summary above it -- three ranges rendered a byte-identical summary. Now respects the same window as the daily series. - get_tool_stats / get_tool_stats_by_period existed only on the SurrealDB mixin, reachable through `# type: ignore[attr-defined]` with no declaration on NeuralStorage and no in-memory implementation -- AttributeError on any other backend. Declared on the interface with an inert default and a real in-memory implementation. - store.py's shared query helper is typed to return row dicts, which understates what a `SELECT VALUE` query actually returns: the selected field for one row can itself be an array, indistinguishable by shape alone from several scalar rows -- the mechanism behind #143's fixed bug. The one live SELECT VALUE call site (get_connected_neuron_ids) now goes through a separate, honestly-typed _query_values instead. - /health's schema_version moved from the 2.x SQLite constant (40) to the active backend's own version (9 on SurrealDB) with no line in the migration guide explaining it -- monitoring that parses the field could read it as a regression. Documented in Troubleshooting; the same table edit also drops a "drift clusters" carried-state row #151 made stale. - The Settings brain-files panel built and returned a path for every brain regardless of whether a file existed there -- a SurrealDB-only brain has none, so the panel showed a plausible path to nothing. size_bytes was already correctly 0; path is now omitted too when the file does not exist. - Renamed ensure_aiosqlite_or_exit_cli to ensure_sqlite_or_exit_cli: it has only ever checked the stdlib sqlite3 module, never aiosqlite, and the name misled anyone reading the CLI startup path. Internal-only.
) A contributor's audit rejected the obvious fix -- removing ma/na/co/sa from STOP_WORDS_PL -- because N/A and S.A. don't actually collide: punctuation fragments them below the minimum word length before the stop-word check ever runs, so removing the words would trade real Polish function-word noise for a niche acronym gain. Only the bare uppercase forms (MA, NA, CO, SA) collide. A token that is ALL-CAPS in the source now survives even when its lowercased form is a stop word, but only when the token was uppercase in the original text (str.isupper(), which already rejects a sentence-initial "Ma"/"Na") -- every other token is tokenized and filtered exactly as before. Gated by how much of the whole text is uppercase: fully-capitalized text (a shouted note, a heading) has no acronym to rescue, only every word incidentally capitalized, and must not have its stop-word filtering disabled wholesale.
MINOR: mostly bug fixes plus one narrow, disclosed breaking removal (the never-functional smem_drift tool, #151) -- same class of change v3.2.0 shipped as MINOR rather than MAJOR.
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.
Summary
Addresses all 6 open issues, verified independently against current
mainbefore any fix (several claims in the original reports needed re-checking against code that had moved since they were filed).ma/na/co/sacollide with acronymsMA/NA/CO/SA. Preserves ALL-CAPS tokens that collide with a stop word, gated by a caps-ratio guard so fully-uppercase text (a shouted note, a heading) doesn't disable stop-word filtering wholesale.check_dead_modulespassed silently while five files imported a module removed in3.0.0. Guard now validates import targets against the tree; the five files are fixed or deleted.smem_driftalways reported "clean" — its inputs were never implemented on SurrealDB and swallowed their own failures. Removed entirely (BREAKING — see below), along with a second unrelated swallowed session-summary write.reasoning_training.py's existing pattern).SELECT VALUEtyping, doc gaps, dashboard path fabrication, a misnamed helper). The seventh (anAGENTS.mdinternal contradiction) is intentionally not touched here — that file says not to edit it without asking first.Breaking change
smem_driftMCP tool is removed (#151). It never returned a trustworthy result on the SurrealDB backend — every call path swallowed its own failure and reported a hardcoded "clean".TagNormalizer.detect_drift(already backingsmem doctor's tag-drift warnings) covers the same practical need without needing storage.Test plan
ruff check/ruff format --check— cleanmypy src/— clean (2 pre-existing, unrelated missing-stub errors)check_dead_modules.py --strict— clean_query_values, tool-stats days filter, brain-files path omission)smem brain list→ onlydefault)