Skip to content

fix(config): support legacy memory embedding keys#400

Merged
penso merged 5 commits intomainfrom
issue-109
Mar 12, 2026
Merged

fix(config): support legacy memory embedding keys#400
penso merged 5 commits intomainfrom
issue-109

Conversation

@penso
Copy link
Copy Markdown
Collaborator

@penso penso commented Mar 11, 2026

Summary

Closes #109.

  • accept legacy [memory] keys like embedding_provider, embedding_base_url, embedding_model, and embedding_api_key
  • emit deprecation warnings with the modern replacement keys instead of unknown-field errors
  • treat embedding_dimensions as a deprecated ignored field so old configs still validate cleanly
  • surface deprecated config warnings in moltis doctor
  • add regression tests for legacy key deserialization and validation behavior

Validation

Completed

  • cargo +nightly-2025-11-30 fmt --all -- --check
  • cargo test -p moltis-config memory_embedding_legacy_aliases_map_to_current_fields
  • cargo test -p moltis-config legacy_memory_embedding_fields_warn_but_do_not_error
  • cargo clippy -p moltis-config --tests -- -D warnings
  • MOLTIS_CONFIG_DIR="$tmpdir" cargo run -q -p moltis --no-default-features -- config check
  • ./scripts/local-validate.sh 400 (started, progressed through fmt/biome/i18n/zizmor, CSS build, Rust lint, WASM build and precompile, then stopped at the 5-minute repo limit during a subsequent long compile phase)

Remaining

  • Re-run ./scripts/local-validate.sh 400 to completion in an environment where the full validation window is available.

Manual QA

  • Create a moltis.toml using the legacy [memory] keys from issue [Bug]: Custom endpoint for embeddings is not working #109.
  • Run moltis config check and verify it reports warnings, not errors, with replacement keys for provider, base_url, and model.
  • Start Moltis with that config and confirm embeddings use the configured custom endpoint instead of falling through to an auto-detected provider.

@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq bot commented Mar 11, 2026

Merging this PR will degrade performance by 17.2%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 2 regressed benchmarks
✅ 37 untouched benchmarks
⏩ 5 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
session_key_to_filename[2026-02-09T12:00:00Z] 745 ns 832.5 ns -10.51%
env_substitution 10 µs 12 µs -17.2%

Comparing issue-109 (83aabfc) with main (5287718)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 11, 2026

Greptile Summary

This PR adds backward-compatibility support for legacy [memory] embedding keys (embedding_provider, embedding_base_url, embedding_model, embedding_api_key, embedding_dimensions) so that old configs continue to work without errors. It does this through two complementary mechanisms: serde #[serde(alias)] attributes on MemoryEmbeddingConfig for transparent deserialization, and a new check_deprecated_fields validation pass that emits deprecated-field warnings pointing users to the modern key names. The moltis doctor command is updated to surface these warnings.

Key changes:

  • crates/config/src/schema.rs: Adds #[serde(alias = "embedding_*")] on four MemoryEmbeddingConfig fields; adds regression test for alias deserialization.
  • crates/config/src/validate.rs: Registers legacy keys in build_schema_map (preventing unknown-field errors), adds check_deprecated_fields / check_deprecated_memory_field helpers, and adds a comprehensive validation test.
  • crates/cli/src/doctor_commands.rs: Extends the category filter to display deprecated-field diagnostics in moltis doctor output.

Issue: When a config contains both a legacy key and its modern replacement (e.g. embedding_provider and provider in the same [memory] table), the validation pipeline emits a deprecated-field warning alongside a cryptic serde type-error ("duplicate field provider"). The deprecation message instructs the user to switch to memory.provider, but they already have it — the two signals are contradictory and the actionable path is unclear. A pre-deserialization coexistence check in check_deprecated_memory_field would surface a targeted error before serde gets involved.

Confidence Score: 3/5

  • Safe to merge for the common case; the duplicate-key scenario produces confusing diagnostics but does not silently corrupt config values.
  • The core aliasing and deprecation-warning logic is correct and well-tested. The score is reduced because of the unhandled edge case where a user has both a legacy key and its modern replacement simultaneously — they receive a cryptic serde "duplicate field" type-error alongside a misleading deprecation warning, with no clear remediation path. This doesn't break production for users migrating from legacy configs (the primary goal of the PR), but it is a real usability gap worth addressing before merging.
  • crates/config/src/validate.rs — specifically the check_deprecated_memory_field helper and how it interacts with the serde type-error step when both legacy and modern keys are present.

Important Files Changed

Filename Overview
crates/config/src/validate.rs Adds check_deprecated_fields and check_deprecated_memory_field helpers; registers legacy keys in build_schema_map so they are not flagged as unknown. Logic issue: when both a legacy key and its modern replacement are present simultaneously, a misleading deprecated-field warning is emitted alongside a cryptic serde type-error ("duplicate field") with no actionable guidance.
crates/config/src/schema.rs Adds #[serde(alias)] attributes for the four legacy embedding_* keys on MemoryEmbeddingConfig, and a regression test that verifies aliases deserialize to the correct fields. Change is straightforward and well-tested.
crates/cli/src/doctor_commands.rs Extends the existing diagnostic category filter in check_config to also surface deprecated-field diagnostics in moltis doctor output. Minimal, correct change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Raw TOML string] --> B[Step 1: Parse TOML syntax]
    B -->|Syntax error| ERR1[Emit syntax diagnostic\nReturn early]
    B -->|OK| C[Step 2: check_unknown_fields\nagainst build_schema_map]
    C -->|Legacy keys now in schema map| D[Step 3: check_deprecated_fields]
    D --> D1{embedding_provider\npresent?}
    D1 -->|Yes| W1[Emit deprecated-field warning\n→ use memory.provider]
    D1 -->|No| D2{embedding_base_url\npresent?}
    W1 --> D2
    D2 -->|Yes| W2[Emit deprecated-field warning\n→ use memory.base_url]
    D2 -->|No| D3{embedding_model /\nembedding_api_key /\nembedding_dimensions?}
    W2 --> D3
    D3 -->|Yes| W3[Emit deprecated-field warning]
    D3 -->|No| E
    W3 --> E[Step 4: check_provider_names]
    E --> F[Step 5: toml::from_str MoltisConfig\nSerde alias resolves legacy → modern field]
    F -->|Deser error e.g. duplicate field| ERR2[Emit type-error diagnostic]
    F -->|OK| G[Step 6: check_semantic_warnings\non parsed config]
    G --> H[Return ValidationResult]
    ERR2 --> H
    H --> I[moltis doctor check_config\nDisplays unknown-field / deprecated-field /\ntype-error / security diagnostics]
Loading

Last reviewed commit: 29fa048

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 96.31336% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/cli/src/doctor_commands.rs 80.95% 4 Missing ⚠️
crates/config/src/validate.rs 97.68% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

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: 29fa048b44

ℹ️ 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".

@penso penso merged commit 564b9a7 into main Mar 12, 2026
34 of 36 checks passed
@penso penso deleted the issue-109 branch March 12, 2026 08:55
penso added a commit that referenced this pull request Mar 23, 2026
* fix(config): support legacy memory embedding keys

* fix(config): address legacy key review feedback

* test(web): wait for openclaw scan in onboarding e2e

* test(web): wait for llm step in onboarding e2e

* test(tools): make sandbox off test explicit
jmikedupont2 pushed a commit to meta-introspector/moltis that referenced this pull request Mar 23, 2026
* fix(config): support legacy memory embedding keys

* fix(config): address legacy key review feedback

* test(web): wait for openclaw scan in onboarding e2e

* test(web): wait for llm step in onboarding e2e

* test(tools): make sandbox off test explicit
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.

[Bug]: Custom endpoint for embeddings is not working

1 participant