fix(hindsight): complete recall tool methods - #8281
Conversation
c0ff9a6 to
a31b46e
Compare
|
Clean fix prepared, but I could not push directly to this PR head branch from the available GitHub token. Why: PR #8281 is from Clean replacement branch/commit is available here:
Summary of the fix:
Validation run locally:
To make this PR merge-ready, the PR author or someone with push access to |
a31b46e to
153af57
Compare
|
Updated this PR branch with the cleaner implementation from Thomas Marcelis's replacement branch. What changed:
Validation I ran after the branch update:
Credit to Thomas for the cleaner implementation and test coverage. |
|
Follow-up pushed: This is only CI housekeeping on top of Thomas's implementation commit:
Re-ran:
|
0dab379 to
8769f3b
Compare
8769f3b to
2b85aa5
Compare
Integrated from ThomasMarcelis/jd/hindsight-recall-methods-clean for PR NousResearch#8281.
Add Thomas Marcelis to AUTHOR_MAP and mark the existing skills action table for ascii-guard so the PR docs workflow can pass.
2b85aa5 to
35cc8ac
Compare
# Conflicts: # agent/model_metadata.py # gateway/platforms/base.py # tests/conftest.py
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the expanded recall implementation and focused Hindsight coverage. The current main premise is valid: plugins/memory/hindsight/__init__.py:314-325 exposes only query, and the handler at :1729-1751 only forwards configured controls.
Problems
- PR head
plugins/memory/hindsight/__init__.py:1461resets an absentrecall_typesconfiguration toNone. Current main deliberately defaults to['observation']at:699-707and:1344-1351(490b3e76b), so this needs reconciliation rather than a direct salvage. - The branch also carries unrelated model metadata, gateway reply, and global fixture changes (
713020d8,35cc8ac0,44923486). The intended commita60a1616is limited to Hindsight files.
Suggested changes
- Preserve main's observation-only default and add coverage for it alongside the new per-call override behavior.
- Define/test whether entity retrieval should override that default with
world; as written, itsdefault_typesfallback is bypassed once main's default is retained. - Salvage only the Hindsight-focused changes and keep unrelated fixes separate.
This is an automated hermes-sweeper review.
| self._recall_max_tokens = int(self._config.get("recall_max_tokens", 4096)) | ||
| self._recall_types = self._config.get("recall_types") or None | ||
| self._recall_max_tokens = _bounded_int(self._config.get("recall_max_tokens", 4096), 4096) | ||
| self._recall_types = _normalize_recall_types(self._config.get("recall_types")) |
There was a problem hiding this comment.
Current main deliberately defaults absent recall_types to ['observation'] (490b3e76b; current main lines 1344-1351). _normalize_recall_types(None) returns None, so carrying this line forward would silently widen default recall. Preserve the main default and add a regression test for an absent config value.
| @@ -1672,15 +1673,21 @@ def get_model_context_length( | |||
| if ctx: | |||
There was a problem hiding this comment.
This model-context-resolution change is unrelated to the stated Hindsight recall work. Please exclude it from the Hindsight salvage so its behavior can be reviewed independently.
Related to open #18268. This branch describes a rebuild of that recall-method work but also contains additional live surfaces, so it is treated as salvage/competing work rather than a duplicate; please isolate the intended contribution. |
Related to #18268 for the Hindsight recall-method work. This live branch also changes unrelated model metadata, Telegram delivery, and MCP tool-name handling, so it is not a duplicate; please consider splitting those scopes for review. |
Related to open #18268, which covers the same Hindsight recall-method goal. This branch also changes model metadata, Telegram reply routing, and MCP naming, so it is retained as competing mixed-scope work rather than marked duplicate; please split or rebase for review. |
Related to focused open #18268 for the Hindsight recall-method feature. This live patch also changes model metadata and Telegram routing; please split or rebase those independent hunks for review. |
Summary
Updates
hindsight_recallso agents can choose the retrieval strategy and filters per call while keeping the default call backward compatible.The core implementation was rebuilt from Thomas Marcelis's cleaner branch,
ThomasMarcelis/hermes-agent:jd/hindsight-recall-methods-clean, and preserved as a Thomas-authored commit. A small follow-up commit only adds Thomas toAUTHOR_MAPand marks an existing docs table forascii-guardso CI can pass.Changes
hindsight_recallcontrols:method,budget,max_tokens,types,tags,tags_match,tag_groups,metadata,max_entity_tokens,limit, andoffset.method="recall"as the default semantic recall path usingclient.arecall(...).method="list"through the publicclient.memory.list_memories(...)API with pagination and formatted output.method="entity"throughclient.arecall(..., include_entities=True)and formats memory plus entity context.Validation
python3 -m py_compile scripts/release.py plugins/memory/hindsight/__init__.py tests/plugins/memory/test_hindsight_provider.pyuv run --extra dev pytest tests/plugins/memory/test_hindsight_provider.py -q -o addopts=: 107 passeduv run --extra dev pytest tests/plugins/memory -q -o addopts=: 153 passeduv run --extra dev ruff check plugins/memory/hindsight/__init__.py tests/plugins/memory/test_hindsight_provider.pygit diff --check origin/main...HEADNote: local
npm run lint:diagramscould not be run becauseascii-guardwas not installed in this temporary worktree; the added ignore markers match existing repo convention and CI will validate it.