Skip to content

fix: port upstream symlink logging and MCP unknown-param error (#1462, #1512)#7

Merged
quangdang46 merged 2 commits into
mainfrom
devin/1779012349-sync-upstream-mempalace
May 17, 2026
Merged

fix: port upstream symlink logging and MCP unknown-param error (#1462, #1512)#7
quangdang46 merged 2 commits into
mainfrom
devin/1779012349-sync-upstream-mempalace

Conversation

@quangdang46
Copy link
Copy Markdown
Owner

@quangdang46 quangdang46 commented May 17, 2026

Syncs the Rust port with upstream milla-jovovich/mempalace develop bb31396 (2026-05-15). Diffed 68319dc..bb31396 (11 non-merge commits touching mempalace/) and ported the two highest-priority upstream fixes. Chunk-size configuration plumbing (#1024 / #1410 / #1519 + 342cfa6 / bea7cb1 follow-ups) is feature-flag work and is deferred to a follow-up sync.

Ported

#1462 — log skipped symlinks during mining

Upstream commits: becf561 (fix), d7d9604 (polish). Issue: milla-jovovich/mempalace#1462.

scan_project and scan_convos used to silently skip symlinks, so a directory full of symlinked files surfaced only as a baffling Files: 0 line with no indication why. Now emits a SKIP: <relative-path> (symlink) diagnostic to stderr (stdout stays clean for callers parsing the Files: / Drawers filed: markers). The polished path uses the scan-root-relative POSIX-style path so nested and dangling symlinks remain unambiguous on every platform.

End-to-end testing this PR surfaced a bug in the initial port (commit c6d01cf): the new SKIP diagnostic was dead code. walkdir under follow_links(false) reports symlinks-to-files with file_type().is_file() == false, and an earlier if !entry.file_type().is_file() { continue; } filter dropped them before the diagnostic branch could fire. The original 5 regression tests only asserted the symlink was absent from the result set — which the prior filter already guaranteed — so they passed against dead code. Fixed in commit 6fd459d:

  • Changed the early filter to if !ft.is_file() && !ft.is_symlink() { continue; } so symlinks reach the diagnostic branch.
  • Refactored both scan functions to delegate to internal scan_project_with_log / scan_convos_with_log helpers that accept an arbitrary &mut impl Write, so unit tests can capture the writer buffer and assert the diagnostic actually fires.
  • Use entry.file_type().is_symlink() (walkdir's cached lstat — no extra syscall) instead of path.is_symlink().
  • In scan_convos, reorder so the extension filter runs before the symlink log, matching upstream Python ordering (a .png symlink is silently dropped at the extension gate rather than logged).
  • Strengthened all 5 #[cfg(unix)] regression tests to assert the exact SKIP: <rel-path> (symlink) line in the writer buffer (would-have caught the dead-code bug).
  • Added 2 negative-control tests: test_scan_project_emits_no_skip_lines_when_no_symlinks_present and test_scan_convos_does_not_log_extension_filtered_symlinks.

Files:

  • crates/core/src/miner.rs::scan_project (wrapper) + scan_project_with_log (impl)
  • crates/core/src/convo_miner.rs::scan_convos (wrapper) + scan_convos_with_log (impl)

#1512 — structured -32602 for unknown parameter name at MCP dispatch

Upstream commit: 51c03a0. Issue: milla-jovovich/mempalace#1512.

A typo like text= instead of content= used to be silently dropped (serde ignores unknown fields by default), so the cause surfaced only indirectly as a later Missing required 'X'. The dispatch layer now returns a JSON-RPC -32602 naming the offending kwarg — symmetric with the missing-required path.

  • crates/core/src/mcp_server.rs::validate_known_params, wired into invoke_with_wal.
  • Tool-name → allowed-property lookup is cached once via OnceLock from make_tools()'s JSON schema so the schema stays the single source of truth.
  • Gated by TOOLS_ACCEPTING_EXTRAS — currently mempalace_add_drawer, whose Input uses #[serde(flatten)] custom_metadata so callers can keep supplying arbitrary string-valued metadata keys (mirrors Python's accepts_var_keyword gate on **kwargs handlers).
  • wait_for_previous is excluded as an internal transport kwarg (no tool schema, stripped before dispatch elsewhere).
  • Regression tests:
    • mcp_server::tests::test_unknown_param_returns_invalid_params_for_wrong_kwarg_name
    • mcp_server::tests::test_two_unknown_params_list_both_names
    • mcp_server::tests::test_wait_for_previous_not_flagged_as_unknown
    • mcp_server::tests::test_add_drawer_accepts_unknown_custom_metadata_keys

Remaining gaps after this sync

Carried forward unchanged: gitignore-aware drawer prune, LLM-assisted init / CLI flags, palace-graph cache invalidation, hook/doc/i18n sync, broader test parity for upstream-added modules.

New deferred work (not ported here):

  1. Configurable chunk sizes (1cc2876 feat (#1024), b1d75b3 / fd63703 follow-ups (#1410, #1519), bea7cb1 / 342cfa6 validation fixes). Upstream wires --chunk-size / --chunk-overlap through CLI → config → miner / convo_miner. Touches non-trivial config plumbing and is not yet user-requested in the Rust port.
  2. CHANGELOG / docs polish around the same chunk-size feature (68dc1bf).

port.txt is updated with the new bb31396 reference state.

Validation

  • cargo fmt --all -- --check — clean
  • cargo clippy --workspace --all-targets --all-features -- -D warnings — clean
  • cargo test --workspace --lib — 419 tests pass (11 new: 5 strengthened symlink + 2 negative-control symlink + 4 MCP unknown-param)
  • End-to-end testing of the release binary against a temp palace dir confirmed both ported features actually work (full report posted as a PR comment).

End-to-end testing of the initial port surfaced that the new symlink
branch in both scan_project and scan_convos was dead code: walkdir
under follow_links(false) reports symlinks-to-files with
file_type().is_file() == false, and both functions had an early
`if !entry.file_type().is_file() { continue; }` filter that ran before
the diagnostic branch. The 5 regression tests still passed because they
only asserted the symlink was absent from the result set -- which the
prior filter already guaranteed.

Refactor scan_project and scan_convos to delegate to internal
scan_project_with_log / scan_convos_with_log helpers that accept an
arbitrary writer, so the diagnostic is now observable from unit tests
without spawning a subprocess. The public wrappers still write to
stderr. Change the early filter to let symlinks through alongside
regular files, and use entry.file_type().is_symlink() in the diagnostic
branch (uses walkdir's cached lstat, no extra syscall). For
scan_convos, also reorder so the extension filter runs before the
symlink-log branch, matching upstream Python ordering -- a .png
symlink is silently dropped at the extension gate rather than logged.

Strengthen the 5 #[cfg(unix)] regression tests to capture the writer
buffer and assert the exact "  SKIP: <rel-path> (symlink)" line fires
(would-have caught the dead-code bug). Add 2 negative-control tests:
test_scan_project_emits_no_skip_lines_when_no_symlinks_present locks
in that a clean scan stays silent, and
test_scan_convos_does_not_log_extension_filtered_symlinks locks in
the upstream extension-before-symlink ordering.

Verified end-to-end against the release binary on a temp project with
3 symlinks (top-level, nested two-deep, dangling): all 3 SKIP lines
appear on stderr with forward-slash, scan-root-relative paths.
@quangdang46 quangdang46 merged commit 280bf70 into main May 17, 2026
4 checks passed
@quangdang46 quangdang46 deleted the devin/1779012349-sync-upstream-mempalace branch May 17, 2026 12:14
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