fix: align cmd_compress dict keys with compression_stats() return values#569
Conversation
web3guru888
left a comment
web3guru888
left a comment
There was a problem hiding this comment.
Straightforward crash fix —
No palace found at /root/.mempalace/palace
Run: mempalace init
The key mapping in the PR description is clean and verifiable: , , , — all match the return values.
One small suggestion: add a test that calls against a mock palace with known content and asserts the stats keys are present. Right now there's no CI coverage catching this class of mismatch, which is presumably how it survived until now.
Clean fix otherwise.
…compress keys - Default new collections to cosine distance (hnsw:space=cosine) instead of L2 — fixes negative similarity scores (MemPalace#568) - Filter unexpected MCP tool args before dispatch — prevents TypeError crash from extra params like top_k (MemPalace#572) - Rotate WAL at 10 MB with one backup to prevent unbounded growth (MemPalace#573) - Fix cmd_compress KeyError: align dict keys with compression_stats() return values (MemPalace#569) - Fix spellcheck _load_known_names: read from "people" key, use dict keys as canonical names (MemPalace#570) - Repair command uses cosine distance for rebuilt collection Co-Authored-By: Claude Opus 4.6 <[email protected]>
web3guru888
left a comment
web3guru888
left a comment
There was a problem hiding this comment.
Clear bug fix — KeyError crash on every mempalace compress run is a regression that would affect anyone using the compress command.
The key mapping is correct: verified against compression_stats() return values. Test mocks updated to match the real return shape. Clean, targeted fix.
LGTM.
[MemPalace-AGI integration — 215 tests, 710 KG entities]
|
Incorporated into #562 — aligned all dict key references in |
web3guru888
left a comment
web3guru888
left a comment
There was a problem hiding this comment.
Correct, narrow fix. LGTM.
[MemPalace-AGI integration — 215 tests, 710 KG entities]
bensig
left a comment
bensig
left a comment
There was a problem hiding this comment.
Two issues found in review:
-
The
Total:summary line (post-loop accumulator incmd_compress) uses the renamed keys but neithertest_cmd_compress_dry_runnortest_cmd_compress_stores_resultsasserts on that output. If someone reverts just the accumulator line, no test catches it. Please addassert "Total:" in captured.out(or assert token counts) to at least one of those tests. -
test_compression_stats_keys_match_dialecttestsDialect.compression_stats()directly but lives intests/test_cli.py. It should be intests/test_dialect.py— a failure here would point to the wrong file and gives false confidence thatcmd_compressintegration is covered.
The key renames themselves are correct and verified against dialect.py. These are test coverage gaps, not bugs in the fix.
|
Both addressed in 6f615a6:
All 59 tests pass. |
web3guru888
left a comment
web3guru888
left a comment
There was a problem hiding this comment.
@arnoldwender — both of @bensig's concerns addressed in 6f615a6:
Total:assertion added to bothtest_cmd_compress_dry_runandtest_cmd_compress_stores_results✓test_compression_stats_keys_match_dialectmoved totests/test_dialect.py✓
LGTM.
[MemPalace-AGI integration — 540+ discoveries, 5,251 KG triples]
…ues (MemPalace#569) * fix: align cmd_compress dict keys with compression_stats() return values * test: align compress test mocks with actual compression_stats() keys * fix: address review — add Total: assertion, move stats key test to test_dialect.py
4 participants
Summary
cmd_compressincli.pyto use the correct dict keys returned byDialect.compression_stats()Problem
cmd_compressaccessesstats["compressed_chars"],stats["original_tokens"],stats["compressed_tokens"], andstats["ratio"]— butcompression_stats()returns"summary_chars","original_tokens_est","summary_tokens_est", and"size_ratio". This causes aKeyErrorcrash every timemempalace compressis run.Changes
compression_stats())compressed_charssummary_charsoriginal_tokensoriginal_tokens_estcompressed_tokenssummary_tokens_estratiosize_ratiooriginal_charswas already correct and unchanged.Test plan
mempalace compress --dry-run— should display stats without crashingmempalace compress— should store compressed drawers with correct metadata