Skip to content

feat(memory): make per-response memory recall count user-configurable (#4948)#5006

Open
maxmilian wants to merge 2 commits into
odysseus-dev:devfrom
maxmilian:feat/4948-memory-recall-count
Open

feat(memory): make per-response memory recall count user-configurable (#4948)#5006
maxmilian wants to merge 2 commits into
odysseus-dev:devfrom
maxmilian:feat/4948-memory-recall-count

Conversation

@maxmilian

@maxmilian maxmilian commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

The number of extended (non-pinned) memories retrieved into the model context per response was hardcoded to k=3 in ChatProcessor.build_context_preface, so users had no way to recall more memories (long-running projects where many small facts matter) or fewer (leaner, cheaper prompts) without editing source. This adds a "Max memories per response" control under Settings → Memory, wired through the existing per-user prefs system (/api/prefs/memory_recall_count) with a global default in settings.py. It mirrors the existing skill_max_injected setting exactly. Default stays at 3, so existing installs behave identically; the backend read clamps to [1, 50] so a malformed pref can't silently disable recall (k<=0) or balloon the prompt.

Target branch

  • This PR targets dev, not main. All PRs land in dev; main is curated by the maintainer at each release. If your PR is on main by accident, click "Edit" on this PR and change the base.

Linked Issue

Fixes #4948

Type of Change

  • Bug fix (non-breaking — fixes a confirmed issue)
  • New feature (non-breaking — adds new behaviour)
  • Breaking change (changes or removes existing behaviour)
  • Refactor / cleanup (behaviour unchanged)
  • Documentation only
  • CI / tooling / configuration

Checklist

  • I searched open issues and open PRs — this is not a duplicate.
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
  • I actually ran the app (uvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough. (Walkthrough documented in the comment below; screenshot of the rendered control included under "Screenshots / clips".)

How to Test

Automated (added with this PR):

python -m pytest tests/test_memory_recall_count_4948.py -q
  • test_mem_recall_count_drives_retrieval_k — a non-default count is forwarded to the retrieval step as k.
  • test_mem_recall_count_defaults_to_current_behavior — omitting it preserves the previous hardcoded k=3.

Manual (reviewer):

  1. Run the app (uvicorn app:app or docker compose up) and open Memory → Settings.
  2. Confirm a new "Memory recall — Max memories per response" number input (default 3).
  3. Change it (e.g. to 10); it persists to /api/prefs/memory_recall_count (verify with GET /api/prefs/memory_recall_count).
  4. Chat with several saved memories; more/fewer extended memories are pulled into the preface as the value changes.

Visual / UI changes — REQUIRED if you touched anything that renders

  • Screenshot or short clip of the change in the running app — attached below (Settings -> Memory, the new "Memory recall" card with "Max memories per response" set to 10).
  • Style match: reuses the existing admin-card layout, admin-toggle-sub labels, and the same <input type="number"> styling as the adjacent Inject Skills control — no new color values, font sizes, spacing units, or classes introduced.
  • No new component patterns. Extends the existing memory settings panel and reuses the shared syncPrefNumber helper (added an optional toast-label arg, default unchanged) rather than writing a parallel one.
  • I am not an LLM agent submitting a bulk PR. Single focused feature against a pre-existing, labeled issue (Make the number of recalled memories per response user-configurable #4948); happy to adjust to maintainer preference (presets vs. raw number, "Unlimited" semantics) before merge.

Screenshots / clips

Settings -> Memory: the new "Memory recall" card with "Max memories per response" set to 10, sitting between "Auto-extract memories" and "Auto-extract skills" and matching the existing "Inject Skills" card

@github-actions github-actions Bot added the ready for review Description complete — ready for maintainer review label Jun 29, 2026
@maxmilian

Copy link
Copy Markdown
Contributor Author

One design question before I flip this out of draft, since the issue (#4948) left the control shape open:

The issue proposed two shapes — presets (Low / Default / High / Unlimited) vs. a plain "max memories per response" number input. This PR currently implements the number input: minimal, backward-compatible (defaults to the current 3, clamped to [1, 50]), and it sits right next to the existing Inject Skills → Max skills per request control in the same panel, so the two read consistently.

I leaned number-input for that consistency, but I'm glad to switch to the preset buttons if you'd prefer. If you do want presets, two sub-questions:

  • Should Unlimited mean "all memories above the similarity threshold" (the issue's recommendation) rather than literally every memory, to avoid filling context with irrelevant entries?
  • Any preferred default preset / values for Low and High?

Happy to rework the UI either way before review — just let me know which shape you want. (Backend is the same either way: the count flows through to the recall k.)

@maxmilian

Copy link
Copy Markdown
Contributor Author

Following up on the design question above — going with the number input as implemented, for the consistency reasons noted (it sits beside the existing "Max skills per request" control and reads the same way), and it's minimal + backward-compatible (defaults to the current 3, clamped [1,50]). Flipping out of draft now; happy to switch to the preset buttons during review if a maintainer prefers — the backend is identical either way.

@maxmilian
maxmilian marked this pull request as ready for review July 11, 2026 02:23
maxmilian and others added 2 commits July 12, 2026 22:13
…odysseus-dev#4948)

The number of extended (non-pinned) memories retrieved into context per
response was hardcoded to k=3 in build_context_preface. Expose it as a
"Max memories per response" setting in Settings → Memory, mirroring the
existing skill_max_injected pattern (per-user pref via /api/prefs, global
default in settings.py). Defaults to 3 so existing installs are unchanged;
the read path clamps to [1,50] so a malformed pref can't silently disable
or balloon recall.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ry_recall_count helper (odysseus-dev#4948)

Adopts fresh-review nit: the new robustness logic (malformed-pref fallback +
[1,50] clamp) had no test coverage. Extracted the inline pref resolution into a
pure resolve_memory_recall_count() helper and added cases for None/NaN/string/
negative/oversized/float inputs. Behaviour unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@maxmilian
maxmilian force-pushed the feat/4948-memory-recall-count branch from 8770405 to 47668e9 Compare July 12, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make the number of recalled memories per response user-configurable

1 participant