fix(hindsight): complete recall tool methods - #18268
Conversation
5ee1b3e to
09176b2
Compare
09176b2 to
691f873
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for expanding the Hindsight recall surface. The premise remains valid on current main: hindsight_recall still exposes only query and directly calls arecall (plugins/memory/hindsight/__init__.py:314-326, :1729-1754).
Problems
- The PR omits Hindsight 0.6.1's supported
opinionfact type from_VALID_RECALL_TYPES(plugins/memory/hindsight/__init__.py:57in PR head), so_normalize_recall_typesdrops a valid request at:373. Main pins that client version inpyproject.toml:159. - The list filter rejects every untagged item at proposed
plugins/memory/hindsight/__init__.py:437, includingtags_match="any"and"all". Hindsight 0.6.1 documents those modes as including untagged entries; only the strict modes exclude them. This makes list behavior differ from normal recall. - The entity
default_types=["world"]at proposed:1724is shadowed by current main's non-empty observation-only default (plugins/memory/hindsight/__init__.py:1344-1351), so the PR's base-era world-default test needs reconciliation during salvage.
Suggested changes
- Preserve the client-supported
opiniontype and add a regression test. - Make list post-filtering preserve the documented strict/non-strict tag semantics, with coverage for all four modes.
- Resolve entity type defaults against current main's recall-type policy before transplanting the tests.
Automated hermes-sweeper review.
| _DEFAULT_IDLE_TIMEOUT = 300 # seconds — Hindsight embedded daemon default | ||
| _VALID_BUDGETS = {"low", "mid", "high"} | ||
| _VALID_RECALL_METHODS = {"recall", "list", "entity"} | ||
| _VALID_RECALL_TYPES = {"world", "experience", "observation"} |
There was a problem hiding this comment.
hindsight-client==0.6.1 also supports the opinion fact type for arecall. Dropping it here means a valid per-call types: ["opinion"] silently falls back; please include it or intentionally document and test a Hermes-level restriction.
| if not tags: | ||
| return True | ||
| item_tags = _item_tags(item) | ||
| if not item_tags: |
There was a problem hiding this comment.
This makes any and all reject untagged memories, but Hindsight defines only any_strict and all_strict as excluding untagged results. Preserve those non-strict semantics so list filtering matches normal recall.
| recall_kwargs, metadata_filter, _, _ = self._build_tool_recall_kwargs( | ||
| args, | ||
| query, | ||
| default_types=["world"], |
There was a problem hiding this comment.
On current main, _recall_types defaults to ["observation"], so _build_tool_recall_kwargs() selects that before this fallback. Reconcile this entity default and its test with the current observation-only policy during salvage.
Summary
hindsight_recallsupport formethod="recall",method="list", andmethod="entity".budget,max_tokens,types,tags,tags_match,tag_groups, metadata filtering, list pagination, and entity token budget.client.memory.list_memories(...)API.methodvalues, matching existing tolerant control handling.Investigation
mainuses_run_hindsight_operation(...)for Hindsight calls, so this keeps recall/list/entity behavior inside that provider operation wrapper.client.memory.list_memories(...)path instead of private client internals.RecallResponse.entitiesas a mapping of entity ids to entity state objects withentity_id,canonical_name, andobservations; entity output now formats those fields directly.type,fact_type, andfactTypeso current and older response shapes filter consistently.Validation
python3 -m py_compile plugins/memory/hindsight/__init__.py tests/plugins/memory/test_hindsight_provider.py-> passedHERMES_HOME=/tmp/hermes-pr18268-provider PYTHONDONTWRITEBYTECODE=1 python3 -m pytest tests/plugins/memory/test_hindsight_provider.py -q -o addopts=-> 110 passedHERMES_HOME=/tmp/hermes-pr18268-memory PYTHONDONTWRITEBYTECODE=1 python3 -m pytest tests/plugins/memory -q -o addopts=-> 156 passedpython3 -m ruff check plugins/memory/hindsight/__init__.py tests/plugins/memory/test_hindsight_provider.py-> passedgit diff --check-> passedHERMES_HOME=/tmp/hermes-pr18268-full PYTHONDONTWRITEBYTECODE=1 python3 -m pytest tests/ -q-> blocked before collection because this local environment lackspytest-xdistfor the repo default-n auto.HERMES_HOME=/tmp/hermes-pr18268-full PYTHONDONTWRITEBYTECODE=1 python3 -m pytest tests/ -q -o addopts=-> 29 collection errors, 4 skipped; errors are missing local optional/dev dependencies (acp,fire).origin/mainalso reports 29 collection errors and 4 skipped for the same missing dependencies, so this PR does not introduce those local full-suite failures.Notes / Caveats
8040bd900714855407972f66fb5bc1c2662af7f6.main(ahead_by=4,behind_by=34at verification time).main, an owner/maintainer token with workflow scope may be needed because upstreammainhas workflow-file changes.