fix(memory): localize background LLM prompts#2447
Conversation
📝 WalkthroughWalkthroughAdds an optional ChangesOutput Language Configuration for Background LLM Artifacts
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Config
participant LlmSummariser
participant system_prompt
Caller->>Config: read output_language (env/setting)
Caller->>LlmSummariser: construct config with output_language
LlmSummariser->>system_prompt: build with output_language
system_prompt->>system_prompt: prepend language directive if present
system_prompt-->>LlmSummariser: prompt with directive
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Clean, well-structured PR — good test coverage across all four prompt surfaces, sensible defaults (None = no change), and the control-character stripping in normalize_output_language is a nice defensive touch.
One minor observation below on the locale mapping table; everything else looks solid.
Signed-off-by: sunilkumarvalmiki <g.sunilkumarvalmiki@gmail.com>
sunilkumarvalmiki
force-pushed
the
codex/2436-background-llm-locale
branch
from
204bca8 to
f24ea46
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/openhuman/memory/tree/score/extract/llm_tests.rs (1)
21-28: 💤 Low valueConsider asserting threading, not directive wording.
This test duplicates assertions that live with the
output_language_directivehelper itself (e.g., theSimplified Chinesemapping and theKeep JSON keys/importance_reasonphrasing). Any future rewording of the directive will break this test even though the extractor's contract — "prepend the directive when configured" — is intact.Consider checking that the prompt starts with (or contains) the directive returned by the helper, leaving the wording assertions to the helper's own tests:
♻️ Suggested refactor
#[test] fn build_system_prompt_includes_output_language_directive() { - let p = build_system_prompt(true, Some("zh-CN")); - assert!(p.contains("Simplified Chinese")); - assert!(p.contains("Keep JSON keys")); - assert!(p.contains("\"importance_reason\"")); + let directive = crate::openhuman::config::output_language_directive(Some("zh-CN")) + .expect("zh-CN should produce a directive"); + let p = build_system_prompt(true, Some("zh-CN")); + assert!(p.contains(&directive), "prompt should embed the language directive"); + // The directive must appear before the extractor's role description so + // the model reads it first. + let directive_pos = p.find(&directive).unwrap(); + let role_pos = p.find("You are a named-entity extractor").unwrap(); + assert!(directive_pos < role_pos); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/memory/tree/score/extract/llm_tests.rs` around lines 21 - 28, The test build_system_prompt_includes_output_language_directive should assert that build_system_prompt(true, Some("zh-CN")) contains (or starts with) the exact directive produced by the existing helper output_language_directive(true, Some("zh-CN")) rather than duplicating its wording; update the assertions to call output_language_directive and check containment (e.g., assert!(p.contains(&output_language_directive(true, Some("zh-CN"))))) so the test verifies that the directive is prepended without coupling to its text.src/openhuman/memory/tree/tree_source/summariser/llm.rs (2)
315-320: 💤 Low valueMinor formatting glitch when
output_languageis set in plain-text mode.In the non-structured path,
language_directiveis appended as\n\n{directive}and then immediately followed by a space +No commentary, no preamble...on the same line. When a directive is present this yields:...Return the summary text first. <directive text> No commentary, no preamble, no headings, ...i.e. the trailing constraints run onto the same line as the directive. It's still readable, but a
\n\nseparator (matching the structured branch on line 323) would keep the prompt shape consistent and easier to grep.✏️ Suggested tweak
if !structured_facets { return format!( - "{base}{language_directive} No commentary, no preamble, no headings, \ - no markdown wrappers, no JSON — just the prose summary." + "{base}{language_directive}\n\n\ + No commentary, no preamble, no headings, no markdown wrappers, \ + no JSON — just the prose summary." ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/memory/tree/tree_source/summariser/llm.rs` around lines 315 - 320, The non-structured return builds the prompt by concatenating base and language_directive then immediately appending the constraint text, causing the directive and constraints to share a line; update the format string in the non-structured branch (the block using structured_facets and language_directive) to insert a "\n\n" after {language_directive} (i.e. "{base}{language_directive}\n\nNo commentary, no preamble, no headings, no markdown wrappers, no JSON — just the prose summary.") so it matches the structured branch's spacing.
513-519: ⚡ Quick winConsider also covering the non-structured + language path.
The new test asserts the directive lands in the structured-facets prompt, but the plain-prose branch (line 315-320) is the other code path that interpolates
language_directiveand currently has no test coveringSome(...). A second assertion (e.g.system_prompt(4096, false, Some("zh-CN"))contains"Simplified Chinese"and still contains"no json") would lock in both branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/memory/tree/tree_source/summariser/llm.rs` around lines 513 - 519, Add coverage for the non-structured (plain-prose) branch of system_prompt by invoking system_prompt with the structured flag set to false (e.g., system_prompt(4096, false, Some("zh-CN"))) and assert the returned prompt contains the language directive ("Simplified Chinese") and the plain-prose instruction marker (e.g., "no json" or "no JSON") so both code paths that interpolate language_directive are tested; update or add a test function alongside system_prompt_includes_output_language_directive that performs these assertions against the system_prompt function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/memory/tree/score/extract/llm_tests.rs`:
- Around line 21-28: The test
build_system_prompt_includes_output_language_directive should assert that
build_system_prompt(true, Some("zh-CN")) contains (or starts with) the exact
directive produced by the existing helper output_language_directive(true,
Some("zh-CN")) rather than duplicating its wording; update the assertions to
call output_language_directive and check containment (e.g.,
assert!(p.contains(&output_language_directive(true, Some("zh-CN"))))) so the
test verifies that the directive is prepended without coupling to its text.
In `@src/openhuman/memory/tree/tree_source/summariser/llm.rs`:
- Around line 315-320: The non-structured return builds the prompt by
concatenating base and language_directive then immediately appending the
constraint text, causing the directive and constraints to share a line; update
the format string in the non-structured branch (the block using
structured_facets and language_directive) to insert a "\n\n" after
{language_directive} (i.e. "{base}{language_directive}\n\nNo commentary, no
preamble, no headings, no markdown wrappers, no JSON — just the prose summary.")
so it matches the structured branch's spacing.
- Around line 513-519: Add coverage for the non-structured (plain-prose) branch
of system_prompt by invoking system_prompt with the structured flag set to false
(e.g., system_prompt(4096, false, Some("zh-CN"))) and assert the returned prompt
contains the language directive ("Simplified Chinese") and the plain-prose
instruction marker (e.g., "no json" or "no JSON") so both code paths that
interpolate language_directive are tested; update or add a test function
alongside system_prompt_includes_output_language_directive that performs these
assertions against the system_prompt function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0de2937-26c4-4692-b3fe-ad79c15ae86e
📒 Files selected for processing (14)
.env.examplesrc/openhuman/agent/harness/archivist.rssrc/openhuman/config/mod.rssrc/openhuman/config/schema/load.rssrc/openhuman/config/schema/load_tests.rssrc/openhuman/config/schema/types.rssrc/openhuman/learning/reflection.rssrc/openhuman/learning/reflection_tests.rssrc/openhuman/memory/tree/score/extract/llm.rssrc/openhuman/memory/tree/score/extract/llm_tests.rssrc/openhuman/memory/tree/score/extract/mod.rssrc/openhuman/memory/tree/score/mod.rssrc/openhuman/memory/tree/tree_source/summariser/llm.rssrc/openhuman/memory/tree/tree_source/summariser/mod.rs
✅ Files skipped from review due to trivial changes (2)
- src/openhuman/memory/tree/tree_source/summariser/mod.rs
- .env.example
|
Thanks @graycyrus, I addressed the locale-mapping follow-up in All CI checks are green now, including Rust quality, Rust/core coverage, diff-cover, Windows secrets tests, Tauri build, Linux E2E, installer smoke, and PR quality checks. Could you please take another look and merge if everything looks good? |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Continuation review — minor finding addressed ✓
The locale-mapping gap flagged in the previous pass (missing ja, de, zh-TW/zh-Hant, th, vi, tr) is fully resolved in f24ea46. The expanded mapping now covers all the obvious omissions, and the updated test in types.rs exercises each new tag. No new issues found.
All four background LLM surfaces (summariser, entity extractor, archivist recap, learning reflection) correctly plumb the directive, default to None so existing behavior is unchanged, and are covered by focused unit tests. Issue #2436 acceptance criteria met.
Good to merge when ready.
3 participants
Summary
output_languageconfig/env override for background LLM artifacts.OPENHUMAN_OUTPUT_LANGUAGEin.env.example.Problem
#2436 reports that background LLM surfaces are hardcoded to English even when chat/UI localization is configured. The affected paths build their own prompts and do not read the web chat locale directive or profile context:
Solution
This adds a small shared background output-language directive in config:
Config.output_languagecan be set in TOML or viaOPENHUMAN_OUTPUT_LANGUAGE.zh-CNare normalized to language names; custom language names are accepted after trimming/control-character cleanup.ReflectionHookreads the directive from its existing full config.Submission Checklist
Closes #2436in Related.Impact
output_language/OPENHUMAN_OUTPUT_LANGUAGEis set.output_language = None.Validation
Passed:
cargo fmt --manifest-path Cargo.toml --all --checkpnpm --filter openhuman-app rust:format:checkgit diff --check$env:CARGO_BUILD_JOBS='1'; cargo test --manifest-path Cargo.toml --lib output_language -- --nocapture(6 passed)Blocked / unrelated:
pnpm format:checkfails in the app Prettier pass with existing formatting drift across 931 app files. This branch does not changeapp/.Related
Summary by CodeRabbit