Skip to content

fix: release 3.7.0 polish — WAL reopen, search errors, hash stamp - #2130

Merged
igorls merged 1 commit into
developfrom
fix/release-3.7.0-polish
Aug 2, 2026
Merged

fix: release 3.7.0 polish — WAL reopen, search errors, hash stamp#2130
igorls merged 1 commit into
developfrom
fix/release-3.7.0-polish

Conversation

@igorls

@igorls igorls commented Aug 2, 2026

Copy link
Copy Markdown
Member

Summary

Polish before Milla reviews #2129 (develop → main for 3.7.0).

From bot review on #2129

  1. sqlite_exact: retire immutable RO cache only when both -wal and -shm exist (Copilot). Partial pairs no longer force a reconnect that raises.
  2. palace.get_collection: retry without options when third-party backends raise TypeError on the kwarg (Copilot).
  3. convo_miner content_hash: stamp only on chunk_index == 0 so multi-conversation bundle hashes are not O(N²)-duplicated (Copilot + Codex P2).

Windows CI flake

test_hybrid_search_enrichment_isolates_chunks... hit KeyError: 'results' when search returned an error envelope without the key. Search error paths now always include results: [], and the test asserts the full envelope.

Deferred (not blocking 3.7.0)

  • Codex P1: re-mine alias ownership when every conversation matches another source — real design work; follow-up issue preferred over last-minute rewrite.

Local validation

uv run ruff check .          # clean on touched files
uv run pytest tests/ --ignore=tests/benchmarks
# 3498 passed, 31 skipped

Test plan

  • Full local suite
  • New partial-WAL sidecar regression
  • Search error envelope includes results: []

Address actionable #2129 bot feedback and the Windows closet KeyError:

- Reopen immutable sqlite_exact readers only when both WAL sidecars exist
  (partial pair keeps the clean snapshot instead of failing the reconnect).
- Retry get_collection without options when plugin backends reject the kwarg.
- Stamp multi-conversation content_hash only on chunk 0 to avoid O(N²) meta.
- Always include results: [] on search error envelopes so callers never KeyError.
- Clearer hybrid-search assertions in the closet isolation test.
@igorls
igorls requested a review from milla-jovovich as a code owner August 2, 2026 10:43
Copilot AI review requested due to automatic review settings August 2, 2026 10:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Polish changes for the 3.7.0 release branch to make read-only SQLiteExact behavior more robust around WAL sidecars, harden search result “error envelope” shape for callers, and reduce unnecessary content_hash duplication during conversation mining.

Changes:

  • Adjust SQLiteExactBackend immutable reader reopening to retire the cache only when both -wal and -shm sidecars exist, and add a regression test for partial sidecar state.
  • Standardize search error returns to always include results: [], and strengthen tests to assert the full envelope to avoid CI flakes.
  • Stamp content_hash only on chunk_index == 0 to avoid O(N²) duplication for bundled conversations.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
mempalace/backends/sqlite_exact.py Reopen immutable RO cache only when WAL sidecar pair is complete.
tests/test_sqlite_exact_backend.py Adds regression coverage for partial WAL sidecar state and reopens-on-writer behavior.
mempalace/searcher.py Introduces _search_error_result() and routes error paths through it to guarantee results: [].
tests/test_searcher.py Asserts search error envelopes include results: [].
tests/test_closets.py Improves hybrid-search assertions to surface full search envelopes on failure.
mempalace/palace.py Adds a retry path for plugin backends that reject the options kwarg.
mempalace/convo_miner.py Limits content_hash stamping to chunk 0 to avoid per-chunk duplication.
Suppressed comments (1)

mempalace/palace.py:267

  • Same issue in the legacy-call fallback: checking only for the substring "options" in the nested TypeError is too broad and can cause an unintended retry without options. Restrict this to the specific unexpected-kwarg message for options.
                collection = backend_obj.get_collection(palace_path, **legacy_kwargs)
            except TypeError as nested:
                if backend_options is None or "options" not in str(nested):
                    raise

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread mempalace/palace.py
# Plugin backends may still use the pre-options signature. Drop
# ``options`` first so read_only degrades gracefully instead of
# hard-failing TypeError on third-party entry points.
if backend_options is not None and "options" in msg:

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee03d65a52

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

# reconnect that would raise on the incomplete set.
wal_exists, shm_exists = self._wal_sidecar_state(db_path)
if wal_exists or shm_exists:
if wal_exists and shm_exists:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reject incomplete WAL sets instead of freezing recall

When an incomplete sidecar set is persistent rather than a transient writer startup—for example, a restored palace containing the database and a valid -wal but no -shm—this condition keeps returning the immutable base-database snapshot for as long as the cached reader is used. Committed rows may exist only in the WAL, and _connect_read_only explicitly rejects this same state because it cannot read it safely, so silently reusing the handle can omit memories instead of surfacing the actionable BackendError.

AGENTS.md reference: AGENTS.md:L9-L9

Useful? React with 👍 / 👎.

Comment thread mempalace/palace.py
Comment on lines +242 to +243
if backend_options is not None and "options" in msg:
preferred_kwargs.pop("options", None)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Restrict fallback to unexpected options keyword errors

When read_only=True and a backend accepts options but raises an internal TypeError whose message happens to mention that word, such as TypeError("invalid options value"), this branch misclassifies the real backend failure as signature incompatibility, invokes initialization again, and silently drops the read-only option. This can hide the original error, duplicate side effects, and return a writable collection despite the caller's request; the fallback should match the specific unexpected-keyword signature error rather than any message containing options.

Useful? React with 👍 / 👎.

@igorls
igorls merged commit a337caf into develop Aug 2, 2026
9 checks passed
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.

2 participants