fix(tests): repair main test suite — daemon fast-path cascade + entry-point pollution - #209
Conversation
…-point pollution
The August REST fast-path landings (`_daemon_search_fast`,
`_daemon_status_fast`) introduced shape and routing assumptions that the
existing test fixtures didn't model. Fixtures only mocked POST/MCP
envelopes; new fast paths issue GET to `/search/fast` and `/status/fast`,
so the same mocks crashed on `req.data.decode()` (data is None on GET) or
returned an envelope shape that `_daemon_search_fast` then iterated as a
bare list.
Fixes:
- `_daemon_search_fast` now accepts both `{"results": [...]}` and bare
list shapes, defensively renames per-hit fields.
- Test fixtures (test_cli_search_output, test_cli_stats, test_cli_daemon,
test_cli_json) dispatch on `req.data is None` to serve REST GETs with a
bare payload or HTTP 404 fall-through.
- `sources.registry.reset_discovery()` lets fixtures re-discover
entry-point adapters after a previous test's `unregister()` flipped the
`_discovered` cache. Without it, `available_adapters()` permanently
returned `[]` for later tests.
- README tool-count tests skip markdown table rows so competitor counts
in comparison tables stop triggering spurious failures.
- README version badge bumped 3.3.5 → 3.3.6 to match version.py.
3193 passed, 35 skipped locally (full suite). The 4 remaining local
failures are environmental (developer's `~/.mempalace/config.json`
leaking past `patch.dict({}, clear=True)` and one chroma lock flake) and
do not reproduce in CI.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…to MCP Follow-up to the previous commit. Same shape as the search_output/stats fixture pattern, but for tests that exercise the routing-to-daemon path in cmd_search and cmd_status. With the REST fast-path landing first, these fixtures crashed on `req.data.decode()` (data is None on GET). - test_cli_daemon.py: `_rest_fastpath_404(req)` helper raises HTTPError on GET so cmd_search falls through to the MCP POST envelope the test is actually verifying. - test_cli_json.py: patches `_call_daemon_rest` alongside `_call_daemon_tool` so the cmd_status/cmd_search tests see no fast path at all and exercise the MCP envelope/error paths intended. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses regressions in the test suite caused by the introduction of REST fast-path endpoints. It resolves issues where test fixtures were expecting specific POST envelopes, fixes parsing logic for daemon responses, and improves test isolation by adding a mechanism to reset entry-point discovery. Additionally, it includes minor maintenance to the README versioning and tool-count verification logic. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CLI daemon search to support both dictionary and list response formats, introduces a reset_discovery utility to safely re-scan entry points in tests, and updates test fixtures to correctly handle REST fast-path GET requests alongside MCP POST requests. The review feedback suggests enhancing robustness by defensively handling non-numeric or missing values when parsing the search result rank and when extracting the limit query parameter in test mocks.
| if "rank" in hit: | ||
| hit["bm25_score"] = round(hit.pop("rank"), 3) |
There was a problem hiding this comment.
If the 'rank' key in hit has a value of None or is not a numeric type (e.g., a string), calling round() directly on it will raise a TypeError. To ensure robustness against unexpected daemon responses, we should defensively convert the value to a float and handle potential conversion errors safely.
| if "rank" in hit: | |
| hit["bm25_score"] = round(hit.pop("rank"), 3) | |
| if "rank" in hit: | |
| rank_val = hit.pop("rank") | |
| try: | |
| hit["bm25_score"] = round(float(rank_val), 3) if rank_val is not None else 0.0 | |
| except (ValueError, TypeError): | |
| hit["bm25_score"] = 0.0 |
References
- PEP 8 recommends defensive programming and robust exception handling to prevent unexpected runtime crashes. (link)
| from urllib.parse import urlparse, parse_qs | ||
|
|
||
| qs = parse_qs(urlparse(req.full_url).query) | ||
| captured["arguments"] = {"limit": int(qs["limit"][0])} |
There was a problem hiding this comment.
Accessing qs['limit'][0] directly can raise a KeyError or IndexError if the 'limit' query parameter is missing or empty in the URL. Using .get() with a default fallback is safer and prevents potential test crashes.
| captured["arguments"] = {"limit": int(qs["limit"][0])} | |
| limit_list = qs.get("limit") | |
| limit_val = int(limit_list[0]) if limit_list else 5 | |
| captured["arguments"] = {"limit": limit_val} |
| from urllib.parse import urlparse, parse_qs | ||
|
|
||
| qs = parse_qs(urlparse(req.full_url).query) | ||
| captured["arguments"] = {"limit": int(qs["limit"][0])} |
There was a problem hiding this comment.
Accessing qs['limit'][0] directly can raise a KeyError or IndexError if the 'limit' query parameter is missing or empty in the URL. Using .get() with a default fallback is safer and prevents potential test crashes.
| captured["arguments"] = {"limit": int(qs["limit"][0])} | |
| limit_list = qs.get("limit") | |
| limit_val = int(limit_list[0]) if limit_list else 5 | |
| captured["arguments"] = {"limit": limit_val} |
After rebasing onto d2ef152 (PR #209), check-docs flagged two drifts — both downstream of #209's content, picked up here so #208's CI goes green on its own: - website/reference/python-api/sources/registry.md: documents the new reset_discovery() function added in #209's entry-point-pollution fix. - website/public/llms-full.txt: one-line shield bump from #209's README version-badge update (3.3.5 -> 3.3.6 to match version.py). Re-ran `scripts/render-api-docs.py` (81 files, 1 changed) and `scripts/render-llms-full.py` (73417 bytes, 1 line changed). No mempalace/ source touched — pure rendered-artifact sync.
…nPool (#208) * perf(kg-extract): migrate KG triple worker to psycopg3 AsyncConnectionPool Replaces psycopg2 + asyncio.Lock with psycopg_pool.AsyncConnectionPool on the KG triple-extraction hot path. Each coroutine now claims its own connection so writes can overlap with the N LLM calls feeding them; the previous single-shared-connection design serialised every postgres write after PR #206 raised LLM throughput ~10x. Worker changes (mempalace/kg_triple_worker.py): - _SyncConnPool retains its name but wraps psycopg_pool.AsyncConnectionPool internally; `configure=` callback runs LOAD 'age' + SET search_path once per fresh connection (was per-cursor before). - _KGHandle.add_triple is async; no asyncio.Lock anywhere. - Pool sized min=max(4, max_concurrency // 2), max=max_concurrency + 2 so steady-state idle capacity is modest but bursts can keep all LLM writers concurrent with the queue claim loop. - Async variants of _claim_batch, _fetch_drawer_text, _mark_completed, _mark_error, _seed_backfill live alongside the sync versions which the CLI --status flag still uses. Driver swap elsewhere is mechanical (`import psycopg as psycopg2`) so the existing test monkeypatch surface keeps working: - backends/postgres.py, knowledge_graph_age.py: _load_psycopg2 still returns the driver+sql modules (now psycopg3) under the legacy name. - searcher.py, migrate_to_postgres.py, cli.py, kg_writethrough.py: aliased imports + error-message updates. Tests: - tests/test_kg_triple_worker.py fakes upgraded to dual-mode sync/async via _NoopAwaitable + _SyncResult helpers — same fakes back both the AsyncConnectionPool hot path and the kept-sync CLI status path. - _FakeKG.add_triple is async with the worker's predicate-positional shape; kg_factory test seam takes the pool, not the dsn. - All 14 kg_triple_worker tests pass; full suite: 3157 passed (up from 3151), 38 pre-existing failures unrelated to psycopg (CLI search output, daemon routing, readme parity, source-adapter entry-point ordering). pyproject.toml: psycopg lines only — psycopg[binary]>=3.2,<4 and psycopg-pool>=3.2,<4 in both [postgres] and [kg-extract] extras. References: #206 (LLM-blob ReDoS fix that made the lock binding). * fix(ci): update test-postgres workflow to psycopg3 + apply ruff format CI on PR #208 failed two checks: 1. `ruff format --check` flagged three migration-touched files (mempalace/cli.py, mempalace/kg_triple_worker.py, mempalace/migrate_to_postgres.py). All three are cosmetic line-wrap diffs from string literals that exceeded the line limit after the driver-rename swap. Ran `ruff format` to fix. 2. .github/workflows/ci.yml's test-postgres job had an inline `python - <<'PY' ... import os, psycopg2 ...` step that creates the pgvector extension before pytest runs. Since psycopg2-binary is no longer in [postgres], this failed with ModuleNotFoundError. Swapped to `import os, psycopg` + the matching `psycopg.connect(...)`. Sync API is identical (autocommit attribute + cursor().execute()) so no other changes needed. Real connection is still exercised — the step continues to create the vector extension against the live pgvector/pgvector:pg16 service container. * fix(ci): swap psycopg2 alias in postgres backend test + regen API docs Two CI failures on PR #208: 1. test-postgres: tests/test_backends_postgres.py:129 still did a literal `import psycopg2`. With psycopg2 removed from [postgres], the import raises ModuleNotFoundError before any assertion runs. Swap to `import psycopg as psycopg2` — the rest of the test body (connect, autocommit, cursor, execute, fetchone) is psycopg3-compatible. 2. check-docs: website/reference/python-api/{kg_triple_worker,kg_writethrough}.md were stale after the worker rewrite (714a930) and the writethrough docstring update. Regenerated via scripts/render-api-docs.py. Worker unit tests still 14/14 green. The other test files that import psycopg2 (test_migrate_to_postgres.py, test_palace_graph.py) are not exercised by PR #208's test-postgres job — keeping this commit scoped to the actually-failing surface. * docs(ci): regen API docs + llms-full.txt after rebase After rebasing onto d2ef152 (PR #209), check-docs flagged two drifts — both downstream of #209's content, picked up here so #208's CI goes green on its own: - website/reference/python-api/sources/registry.md: documents the new reset_discovery() function added in #209's entry-point-pollution fix. - website/public/llms-full.txt: one-line shield bump from #209's README version-badge update (3.3.5 -> 3.3.6 to match version.py). Re-ran `scripts/render-api-docs.py` (81 files, 1 changed) and `scripts/render-llms-full.py` (73417 bytes, 1 line changed). No mempalace/ source touched — pure rendered-artifact sync.
Summary
Main has been red on the
Testsworkflow since the REST fast-pathlanded for
_daemon_search_fast/_daemon_status_fast. The new GETpaths to
/search/fastand/status/fastviolated two test invariants:req.data is None, soreq.data.decode()crashed withAttributeError: 'NoneType' object has no attribute 'decode'._daemon_search_fastiterated the daemon's response as a barelist. The daemon returns
{"results": [...], "warnings": []}, soiterating yielded string keys and
.pop()crashed.Plus two unrelated test-suite issues surfaced at the same time:
unregister()flipped a cachethat suppressed re-discovery for later tests).
comparison tables.
Changes
mempalace/cli.py:1457—_daemon_search_fastaccepts both{"results": [...]}and bare list shapes; defensively renames hitfields.
mempalace/sources/registry.py— newreset_discovery()to lettests re-trigger entry-point scan.
mempalace/sources/__init__.py— exportsreset_discovery.test_cli_search_output,test_cli_stats,test_cli_daemon,test_cli_json) dispatch onreq.data is Noneorpatch
_call_daemon_restso REST fast-paths fall through to the MCPenvelope path the tests actually verify.
test_cli_source.py/test_sources.pycallreset_discovery()intheir isolation fixtures.
test_readme_claims.py— tool-count regex skips markdown table rows.Test plan
pytestfull suite: 3193 passed, 35 skipped locallyenv leaks from developer's
~/.mempalace/config.jsonand onechroma collection lock flake) do not reproduce
clean main
🤖 Generated with Claude Code