Skip to content

Fix KeyError in cmd_compress: stats dict key mismatch - #588

Closed
helhindi wants to merge 1 commit into
MemPalace:mainfrom
helhindi:fix/compress-stats-key-mismatch
Closed

Fix KeyError in cmd_compress: stats dict key mismatch#588
helhindi wants to merge 1 commit into
MemPalace:mainfrom
helhindi:fix/compress-stats-key-mismatch

Conversation

@helhindi

Copy link
Copy Markdown

Summary

  • mempalace compress crashes with KeyError: 'compressed_chars' because cmd_compress in cli.py references stale dictionary keys that no longer match what Dialect.compression_stats() returns
  • Updated 6 key references in cli.py and 2 test mock dictionaries in test_cli.py to use the correct keys: summary_chars, original_tokens_est, summary_tokens_est, size_ratio

Key mapping

cli.py used (old) dialect.py returns (correct)
compressed_chars summary_chars
original_tokens original_tokens_est
compressed_tokens summary_tokens_est
ratio size_ratio

Repro

$ mempalace compress
KeyError: 'compressed_chars'

Test plan

  • All 6 compress-related tests pass (pytest tests/test_cli.py -k compress)
  • Verified mempalace compress --dry-run works against a real palace with 21K+ drawers

🤖 Generated with Claude Code

…ression_stats()

Dialect.compression_stats() was refactored to use semantic key names
(summary_chars, original_tokens_est, summary_tokens_est, size_ratio)
but cmd_compress still referenced the old names (compressed_chars,
original_tokens, compressed_tokens, ratio), causing a KeyError crash
on every invocation.

Updated cli.py and test mocks to use the correct key names.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@web3guru888 web3guru888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix is correct. I verified against dialect.pycompression_stats() has returned summary_chars, original_tokens_est, summary_tokens_est, and size_ratio since the 39c14be refactor. The old key names in cli.py were never updated, so this was a guaranteed crash on any mempalace compress invocation.

One non-blocking observation: line 374 in the patch stores to comp_meta["original_tokens"] (the ChromaDB metadata field name) while reading from stats["original_tokens_est"] (the dialect key). The asymmetry is intentional — the stored metadata key predates the _est suffix — but it's a potential source of confusion for future contributors. A brief comment (# stored key name predates _est suffix) would help.

The test mock updates are exactly right. No new tests needed since this is a key-name-only fix with coverage already in place.

LGTM — ship it.

@web3guru888 web3guru888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix — the key mismatch is mechanical (cli.py references stale keys that no longer match what compression_stats() returns) and the PR addresses all 4 mismatches consistently across both cli.py and the test mocks.

The key mapping table in the PR description is exactly what this kind of rename fix needs — makes the diff reviewable in 30 seconds.

One note: #562 includes the same compress key fix (along with 14 other items). If #562 lands first, this PR would conflict. Either way one of them fixes it — just flagging for maintainer coordination.

LGTM.

@web3guru888 web3guru888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct fix. The key mismatch is easy to miss because compression_stats() returns a dict that looks structurally similar to what cmd_compress expects — same rough shape, slightly different key names, no type error until the wrong key is accessed.

The test mock updates are exactly right: mock dicts should mirror what the real method returns, not what the caller expects.

One small observation: ratiosize_ratio is the only key rename that changed semantics slightly (ratio implies compression ratio; size_ratio is more precise). The others are just naming convention (_chars, _est suffix). Worth a brief comment in the mock explaining what size_ratio means (chars-before / chars-after) so future test authors don't confuse it with token ratio or compression speed.

Otherwise clean — the fix is minimal and targeted.

@web3guru888

Copy link
Copy Markdown

Clean fix. The key mapping table in the issue description is exactly what a reviewer needs — makes it unambiguous that the rename is one-to-one and the semantics are preserved.

Two things worth noting for context:

  1. PR feat: batch writes, concurrent mining, MCP tools, hooks, export, search improvements #562 contains a very similar fix in the same commit (3dd85b66 — "cherry-pick upstream bug fixes — cosine distance, WAL rotation, compress keys"). If feat: batch writes, concurrent mining, MCP tools, hooks, export, search improvements #562 merges first, this PR will conflict on cli.py. Worth coordinating or rebasing against feat: batch writes, concurrent mining, MCP tools, hooks, export, search improvements #562.

  2. The test mock updates (changing expected keys in test_cli.py) are the most important part — without those, the bug could regress silently if someone refactors compression_stats() again. The test now anchors the interface contract.

LGTM otherwise. The cmd_compress KeyError is a real crash that makes the compress feature completely unusable in current builds.


[MemPalace-AGI integration — 540+ discoveries, 5,251 KG triples]

@jphein

jphein commented Apr 11, 2026

Copy link
Copy Markdown
Collaborator

Already fixed in #562 (commit 3dd85b6) — same key alignment: summary_chars, original_tokens_est, summary_tokens_est, size_ratio. Our fix also updates the test mocks to match.

@bensig

bensig commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Closing in favor of #569, which has the identical cli.py fix plus stronger test coverage (adds the "note" key to mocks and a new integration test verifying key parity with Dialect.compression_stats()). Thanks for the contribution!

@bensig bensig closed this Apr 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants