Skip to content

fix: bug fixes to Rust core#2

Merged
quangdang46 merged 1 commit into
mainfrom
devin/1778334560-sync-upstream-fixes
May 9, 2026
Merged

fix: bug fixes to Rust core#2
quangdang46 merged 1 commit into
mainfrom
devin/1778334560-sync-upstream-fixes

Conversation

@quangdang46
Copy link
Copy Markdown
Owner

Summary

Tier‑1 sync of four small, isolated upstream bug fixes from
milla-jovovich/mempalace (Python) to the Rust port. Each fix mirrors a
specific upstream commit and ships with a regression test.

# Upstream commit Issue Rust file
1 ead2c5d (#1004) detect_room substring match misroutes (viewsinterviews) crates/core/src/miner.rs
2 40e2c8b (#1156) Exporter doesn't refuse symlinks at output targets crates/core/src/exporter.rs
3 0d1c1fb (#925) Diary skip‑if‑unchanged drops same‑size edits (tehthe) crates/core/src/diary_ingest.rs
4 2a0ed0c (#1155) closet_llm bails on first JSON decode error instead of retrying crates/core/src/closet_llm.rs

Larger upstream gaps (sync.py, source adapters, palace lock, hooks PID
guard, LLM init flags, …) are intentionally out of scope for this PR
— they will land as follow‑ups so each can be reviewed independently.

Per‑fix details

1. miner: token‑boundary detect_room — adds tokens() /
name_matches() helpers that split on -, _, ., / and replace the
raw String::contains checks. Before: every file under
project/interviews/ got routed to a "views" room. After: only true
token matches route. Regression tests:
test_name_matches_token_boundary,
test_detect_room_does_not_misroute_substring,
test_detect_room_matches_separator_bounded_token.

2. exporter: refuse symlinks at output targets — adds
reject_symlink() defense‑in‑depth check. A pre‑placed symlink at the
export path would otherwise redirect markdown writes anywhere the
symlink points (e.g., system directories). Mirrors the miner's
input‑side caution. Tests:
test_reject_symlink_allows_regular_path,
test_reject_symlink_allows_missing_path,
test_reject_symlink_blocks_symlinked_dir (unix‑gated).

3. diary_ingest: hash‑based change detection — switches the skip
gate from byte length to SHA256(content). Same‑size edits (typo fixes,
single‑char swaps) used to be silently dropped, violating the
verbatim‑storage guarantee. Legacy state files without content_hash
keep using the size gate so a post‑upgrade run does not re‑ingest
every untouched diary. Tests:
test_state_entry_legacy_format_deserializes_without_content_hash,
test_state_entry_round_trips_with_content_hash,
test_ingest_diaries_detects_same_size_edit.

4. closet_llm: retry on JSON decode — local LLM runtimes
occasionally produce truncated / partial JSON under load; the previous
bail‑on‑first‑failure path made the existing retry loop dead for that
case. Now JSONDecodeError shares the same exponential‑backoff path as
HTTP 429/503. New RegenerateError::InvalidJson variant. Tests:
test_regenerate_entry_retries_on_invalid_json,
test_regenerate_entry_returns_invalid_json_after_all_retries_fail
(use a small in‑process mock HTTP server).

Local verification

cargo fmt --all -- --check       # clean
cargo clippy --all-targets --all-features -- -D warnings   # clean
cargo test --workspace --all-features                       # 380 passed (369 + 11 new)

Review & Testing Checklist for Human

  • Sanity-check the name_matches() token semantics — confirm the
    separator set (-, _, ., /) matches what the upstream
    Python uses and doesn't break any pre‑existing room mapping in
    your real palace.
  • On unix, manually create a symlink at the export output_dir and
    confirm export now refuses it instead of writing through the link.
  • If you have an existing diary state file under
    ~/.mempalace/state/diary_ingest_*.json, run a no‑op ingest first
    and confirm it does NOT re‑ingest every day (legacy size‑gate
    compatibility).

Notes

  • Gap report (Tier 1 vs 2 vs 3 triage) and the patch file are attached
    in the originating Devin session for follow‑up PRs.
  • The closet_llm test refactor introduces an internal
    regenerate_entry_with_delay() helper purely to keep the regression
    test fast; the public regenerate_entry() still uses the production
    INITIAL_DELAY_MS = 1000.

Tier-1 sync of small, isolated bug fixes from upstream
milla-jovovich/mempalace that landed since the last sync ledger entry
in port.txt (94f1689). Each fix mirrors a specific upstream commit and
ships with a regression test.

- miner: token-boundary matching in detect_room (port ead2c5d, #1004).
  Stops 'views' \u2282 'interviews' substring misrouting that sent every
  interviews/* file to a 'views' room. Adds tokens()/name_matches()
  helpers using -, _, ., / as token boundaries.

- exporter: refuse symlinks at output and per-wing targets (port
  40e2c8b, #1156). Mirrors the miner's input-side caution; a pre-placed
  symlink at output_dir would otherwise redirect markdown writes.

- diary_ingest: detect same-size edits via sha256 content hash (port
  0d1c1fb, #925). The size-only skip gate dropped in-place edits like
  'teh' \u2192 'the', a verbatim-storage violation. Legacy state files
  without content_hash continue to use the size gate so post-upgrade
  runs do not re-ingest every untouched diary.

- closet_llm: retry on JSON decode error with exponential backoff
  (port 2a0ed0c, #1155). Local LLM runtimes occasionally produce
  truncated/partial JSON; the previous bail-on-first-failure path made
  the retry loop dead for that case.

Co-Authored-By: Trần Quang Đãng <tranquangdang21@gmail.com>
@quangdang46 quangdang46 changed the title fix: port four upstream Python bug fixes to Rust core fix: bug fixes to Rust core May 9, 2026
@quangdang46 quangdang46 merged commit 6118b29 into main May 9, 2026
4 checks passed
@quangdang46 quangdang46 deleted the devin/1778334560-sync-upstream-fixes branch May 9, 2026 15:22
quangdang46 added a commit that referenced this pull request Jun 2, 2026
Closes Oracle gap #4: telemetry had pre-registered descriptors
(telemetry.rs:99-135) but zero call sites. Minimum-viable
instrumentation: feature-gated, no behaviour change when
`telemetry` is off.

  - searcher.rs::search_memories_with_rerank
      counter!("mempalace_search_total", status=success)
      histogram!("mempalace_search_latency_ms")
  - palace_db.rs::add
      counter!("mempalace_insert_total", status=success)
  - llm/minimax_provider.rs::complete
      counter!("mempalace_llm_total_minimax")
      histogram!("mempalace_llm_latency_ms")

Pattern: `let _t0 = Instant::now()` at function entry, `.inspect()`
on the Ok return to record counter + histogram. Error paths
remain uninstrumented (deferred — a RAII guard or a pre-registered
Counter handle can capture both paths without per-call allocation).

Dynamic model label is deferred: the `metrics` macro requires
\&'static str for label values, and a dynamic String would force
a clone per call. Suffixing the counter name with the provider
(`..._minimax`) keeps providers distinguishable in Prometheus
without per-call allocation. The same pattern applies to
anthropic/openai when their call sites are instrumented.

Verification:
  - cargo build -p mempalace-core --features telemetry: clean
    (0 errors, 106 pre-existing warnings)
  - default build: unchanged (all instrumentation is
    #[cfg(feature = "telemetry")])

All 4 Oracle gaps owned by me are now closed:
  - #1 SYNONYM_BM25_WEIGHT -> RRF (a4a4cea)
  - #2 event-loop lag check (a4a4cea)
  - #3 9 missing connect adapters (98cffa2)
  - #4 telemetry call-site instrumentation (this commit)

The 6 failing tests are pre-existing in compress*/governance/heal/
mcp_server (touched a1743d4/f801525 by other agents) — not my
responsibility per AGENTS.md.
quangdang46 added a commit that referenced this pull request Jun 3, 2026
PR #7 added the new typed Drawer fields (tags, trust, access_count,
last_accessed, reinforcements, superseded_by, active) plus a
`migrate_metadata` helper. The helper was left as opt-in
(consumers had to call it themselves), which meant legacy drawers
written by pre-PR #7 Palace versions would still be read in the
v0 shape (data in `metadata`, typed fields empty).

This PR wires the migration into every read/write path so the
self-healing is automatic and the typed fields are the source of
truth on disk from this point forward.

Wiring:

  - `Palace::add_drawer` (palace.rs:625) — calls
    `drawer.migrate_metadata()` once at the top, before any
    `store.upsert`. Every `add_drawer` write is now a
    self-healing write: legacy drawers that get re-written land
    in the v1 (typed-field) shape automatically.

  - `Palace::get_drawers` (palace.rs:710) — iterates the
    returned drawers and calls `migrate_metadata` on each
    before returning. Every reader (Layer 1 wake-up, status,
    PR #1 mutation methods via `default_mutate_drawer`,
    PR #2 tag/link, PR #4 recent) flows through this single
    chokepoint, so the migration propagates through the whole
    read surface.

  - `palace/store/usearch_sqlite.rs::get_drawer_by_id` and
    `::all_drawers` — call `migrate_metadata` right after
    constructing the Drawer from SQLite rows. Defensive: covers
    the case where a host calls `store.get_drawers()` directly
    (bypassing `Palace::get_drawers`).

  - `layers.rs::Layer1::generate` and `layers.rs::create_test_palace_db`
    (palace_db.rs read path) — same defensive coverage for
    the wake-up layer and tests that go through `PalaceDb::get_all`
    rather than the trait.

The `migrate_metadata` function is left as `pub fn` (not
demoted to `pub(crate)`) to avoid a SemVer-breaking change
on PR #7. Consumers can still call it explicitly if they want
to; they just don't have to.

Includes a cherry-pick of PR #7 (the `migrate_metadata` helper
and Drawer field extension) so this branch has the full
self-healing pipeline. The cherry-pick is a separate commit so
the two PRs are reviewable independently.

This is PR 24/8+ (post-series) — closes the design gap in PR #7
that was called out in the PR #7 review: migrate_metadata
should be mempalace's responsibility, not the consumer's.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
quangdang46 added a commit that referenced this pull request Jun 3, 2026
PR #7 added the new typed Drawer fields (tags, trust, access_count,
last_accessed, reinforcements, superseded_by, active) plus a
`migrate_metadata` helper. The helper was left as opt-in
(consumers had to call it themselves), which meant legacy drawers
written by pre-PR #7 Palace versions would still be read in the
v0 shape (data in `metadata`, typed fields empty).

This PR wires the migration into every read/write path so the
self-healing is automatic and the typed fields are the source of
truth on disk from this point forward.

Wiring:

  - `Palace::add_drawer` (palace.rs:625) — calls
    `drawer.migrate_metadata()` once at the top, before any
    `store.upsert`. Every `add_drawer` write is now a
    self-healing write: legacy drawers that get re-written land
    in the v1 (typed-field) shape automatically.

  - `Palace::get_drawers` (palace.rs:710) — iterates the
    returned drawers and calls `migrate_metadata` on each
    before returning. Every reader (Layer 1 wake-up, status,
    PR #1 mutation methods via `default_mutate_drawer`,
    PR #2 tag/link, PR #4 recent) flows through this single
    chokepoint, so the migration propagates through the whole
    read surface.

  - `palace/store/usearch_sqlite.rs::get_drawer_by_id` and
    `::all_drawers` — call `migrate_metadata` right after
    constructing the Drawer from SQLite rows. Defensive: covers
    the case where a host calls `store.get_drawers()` directly
    (bypassing `Palace::get_drawers`).

  - `layers.rs::Layer1::generate` and `layers.rs::create_test_palace_db`
    (palace_db.rs read path) — same defensive coverage for
    the wake-up layer and tests that go through `PalaceDb::get_all`
    rather than the trait.

The `migrate_metadata` function is left as `pub fn` (not
demoted to `pub(crate)`) to avoid a SemVer-breaking change
on PR #7. Consumers can still call it explicitly if they want
to; they just don't have to.

Includes a cherry-pick of PR #7 (the `migrate_metadata` helper
and Drawer field extension) so this branch has the full
self-healing pipeline. The cherry-pick is a separate commit so
the two PRs are reviewable independently.

This is PR 24/8+ (post-series) — closes the design gap in PR #7
that was called out in the PR #7 review: migrate_metadata
should be mempalace's responsibility, not the consumer's.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

1 participant