fix(hindsight): surface retain timeouts clearly - #59842
Conversation
(cherry picked from commit 38de0dc12571529d87b8905c57a0ba2ce4c9226c) (cherry picked from commit fabc245c382da529ad6deab5786dfee2a75a414e)
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing an actual visibility gap. On current main, _run_sync propagates the blank-message timeout from future.result() (plugins/memory/hindsight/__init__.py:274-281), and hindsight_retain exposes str(e) directly (plugins/memory/hindsight/__init__.py:1725-1727).
Problems
- The added production call uses
operation_name="hindsight client operation"(plugins/memory/hindsight/__init__.py:1157), so retain calls do not produce the retain-specific wording used by the new test. The test supplies"hindsight retain"directly to the private bridge instead (tests/plugins/memory/test_hindsight_provider.py, added test in PR commit61aa2ad3ce62). - The regression test does not exercise the public retain-tool response or the queued auto-retain path, whose failures are logged by the writer (
plugins/memory/hindsight/__init__.py:1124-1126).
Suggested changes
- Thread a retain-specific operation name through
_run_hindsight_operationfrom eacharetain_batchcaller. - Assert the actual
hindsight_retainJSON error contains the timeout and duration; cover queued retain logging if that path is in scope.
This is an automated hermes-sweeper review.
|
@teknium1 Current head |
SummaryTwo PRs address the Issue's timeout-visibility cause, while #57612 also bundles separate compression and environment-snapshot hardening. #59842 carries the Hindsight fix further than #57612 by propagating retain-specific operation names through public retain, auto-retain, and flush-on-switch paths, with public-path regression coverage. Related pull requests
Duplicates#57612 and #59842 overlap on the core Hindsight timeout-handling change, but they are not fully identical: #59842 adds retain-specific operation-name propagation through public retain, auto-retain, and flush-on-switch paths. The Hindsight portion of closed #57612 can therefore be treated as superseded by #59842; its compression and environment portions are separate slices. Suggested consolidationKeep #59842 open with a salvage path: request re-review from the contributor who posted the COMMENTED keep_open review, explicitly confirming the visible diff's public retain, auto-retain, and flush-on-switch coverage. Keep #57612's existing consolidation closure in place rather than issuing a new close action; this follows the discussion's maintained-destination evidence despite the earlier automated keep_open verdict, and its overlapping Hindsight slice can be considered superseded by #59842. Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 27 kB of PR diffs, 2 kB of issue/PR text, 3 kB of discussion (3 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Make Hindsight memory-provider retain timeouts explicit instead of failing silently, so a slow/unreachable retain surfaces a clear signal rather than a swallowed error.
Testing
pytest tests/plugins/memory/test_hindsight_provider.py→ 117 passed, on currentmain.