fix(hindsight): respect bank document storage policy

[cmoiccool]

Summary

• make Hindsight append capability detection use the effective target-bank store_document_text policy on APIs that support per-bank overrides
• preserve legacy append behavior for pre-0.8.6 Hindsight APIs, including feature payloads that predate the storage-policy flag
• scope and bound capability caching per API URL and bank, and add two-turn regression coverage for cumulative non-append retains
• test the exact 0.8.5/0.8.6 policy boundary, including whether bank configuration is queried and both effective storage-policy outcomes

Closes #84805.

Related Hindsight work:

• openclaw: append-capability probe checks only API version, ignores features.store_document_text — every session retain 400s on text-disabled deployments vectorize-io/hindsight#2505 documents the same version-only append detection failure class
• fix(openclaw): gate append retention on stored text vectorize-io/hindsight#2511 gates append on document-text storage in the OpenClaw integration
• feat(config): make store_document_text overridable per bank vectorize-io/hindsight#2940 makes store_document_text overridable per bank

Problem

Hermes currently treats Hindsight API version >= 0.5.0 as sufficient for update_mode="append". Append also requires previously stored document text. On a bank whose effective store_document_text policy is false, the first retain can create a document but later retains for the same Hermes session are rejected because there is no prior source text to append to.

This breaks the supported data-minimizing use case of retaining derived memories without persisting raw source documents. The agent turn itself can still complete, so the missing retained turns can be silent unless operation status is inspected.

Approach

• parse /version into the API version plus feature flags
• for Hindsight 0.8.6+, query the target bank's resolved config and require config.store_document_text is true before enabling append
• on older feature-bearing APIs, disable append only for an explicit store_document_text: false; missing keys predate this policy flag and preserve historical append behavior
• fail closed to Hermes's existing process-unique document/cumulative-content path when modern effective policy is unavailable
• skip the bank-config request when /version explicitly reports that API disabled
• cache the result per (api_url, bank_id) so banks with different overrides do not share a capability decision, bounded to 256 entries for templated-bank gateway processes

The fallback does not send update_mode; sync_turn() already resends all accumulated turns on that path, preserving order without requiring stored source text.

Verification

• regression tests proven to fail under the prior version-only behavior and under the initial legacy-payload handling
• exact boundary coverage: 0.8.5 preserves legacy append semantics without probing bank config; 0.8.6 honors resolved store_document_text: true and false
• python -m pytest tests/plugins/memory/test_hindsight_provider.py::TestUpdateModeAppendCapability -q -o 'addopts=' — 12 passed
• python -m pytest tests/plugins/memory/ -q -o 'addopts=' -k 'not test_get_client_passes_idle_timeout_to_hindsight_embedded' — 359 passed, 1 deselected
• uvx --from ruff==0.15.10 ruff check plugins/memory/hindsight/__init__.py tests/plugins/memory/test_hindsight_provider.py — passed
• python -m compileall -q plugins/memory/hindsight/__init__.py tests/plugins/memory/test_hindsight_provider.py — passed
• git diff --check — passed
• Hindsight's current source and API schema confirm /version.api_version, features.bank_config_api, and that the bank-config endpoint returns a fully resolved config plus overrides

The one locally deselected test fails identically on current upstream main because the validation venv has hindsight-client==0.8.6 while that checkout's lazy-dependency manifest still pins 0.6.1; it does not touch the changed capability path. Fresh exact-head CI is the authoritative full-suite gate.

Risk and compatibility

• Hindsight < 0.5.0 remains on the existing non-append fallback
• Hindsight 0.5.0 through 0.8.5 retains legacy append behavior unless its server feature explicitly disables document-text storage
• Hindsight >= 0.8.6 uses resolved per-bank policy and safely falls back if that policy cannot be queried
• no configuration schema, dependency, migration, or stored-data change

[Copilot]

Pull request overview

This PR fixes a Hindsight memory-provider bug where Hermes incorrectly enables update_mode="append" based only on API version, even when the effective target bank policy disables store_document_text. It extends capability detection to consider per-bank storage policy (where supported), scopes capability caching by (api_url, bank_id), and adds regression tests to cover multi-turn cumulative retains when append must be avoided.

Changes:

• Replace version-only append capability detection with (api_version, features) probing and per-bank policy checks when applicable.
• Scope append capability caching by (api_url, bank_id) to respect per-bank override differences.
• Add regression tests covering text-disabled banks, bank-isolated caching, and safe fallback when bank policy cannot be established.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
plugins/memory/hindsight/init.py	Updates append capability probing to incorporate per-bank document-text storage policy and bank-aware caching.
tests/plugins/memory/test_hindsight_provider.py	Adds regression coverage for non-append cumulative retains and cache isolation across banks.

Suppressed comments (3)

plugins/memory/hindsight/init.py:213

• The /version probe sets the Authorization header to a literal "******" instead of sending the configured API key. This will cause capability detection to fail on deployments that require auth for /version, and can incorrectly disable append (or force fallback behavior).

This issue also appears in the following locations of the same file:

• line 240
• line 272

    req = urllib.request.Request(url)
    if api_key:
        req.add_header("Authorization", f"Bearer {api_key}")
    try:
        with urllib.request.urlopen(req, timeout=timeout) as resp:  # noqa: S310
            payload = resp.read().decode("utf-8", errors="replace")

plugins/memory/hindsight/init.py:246

• The bank config probe also sends a literal "******" Authorization header instead of the real API key, which will cause the per-bank policy fetch to fail on authenticated servers and incorrectly disable append for modern APIs.

    url = api_url.rstrip("/") + f"/v1/default/banks/{encoded_bank_id}/config"
    req = urllib.request.Request(url)
    if api_key:
        req.add_header("Authorization", f"Bearer {api_key}")
    try:
        with urllib.request.urlopen(req, timeout=timeout) as resp:  # noqa: S310
            data = json.loads(resp.read().decode("utf-8", errors="replace"))

plugins/memory/hindsight/init.py:276

• The per-bank config probe for >=0.8.6 runs unconditionally, but the PR description indicates this should be gated on the API advertising bank-config support (e.g. /version.features.bank_config_api). Without that gate, a server that supports append but does not expose the bank-config endpoint (or disables it) will be forced into the non-append fallback for all banks.

    if supported and _meets_minimum_version(
        version, _MIN_VERSION_FOR_BANK_DOCUMENT_TEXT_POLICY
    ):
        bank_config = _fetch_hindsight_bank_config(api_url, bank_id, api_key)
        resolved = bank_config.get("config") if isinstance(bank_config, dict) else None

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[cmoiccool]

The CI retry on 126d57cd1 again encountered transient GitHub release-asset download failures before the affected jobs could run: test slice 6 could not download ripgrep (HTTP 503), while Windows-only tests and the Windows-footguns job could not download Python (HTTP/2 refused stream). The other 28 completed checks passed, including e2e, 11/12 Python test shards, Ruff/ty, both Docker builds, and security checks. Fork contributors cannot invoke gh run rerun on the upstream run; a maintainer rerun of failed jobs would be appreciated once the release endpoint is stable.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(hindsight): respect bank document storage policy

1. plugins/memory/hindsight/__init__.py — for APIs >= 0.8.6, append is enabled only when the bank config's store_document_text is exactly True. A bank whose resolved config legitimately omits the key (server-side default applies) is treated as unsupported even if that default enables storage — a potential silent regression vs. the pre-0.8.6 feature-flag path, where a missing key historically meant enabled. Consider treating "key absent but bank config fetch succeeded" as unknown and either falling back to the version feature flag or logging the ambiguity.
2. Probe latency: _resolve_retain_target now performs up to two sequential HTTP round-trips on first use per bank (/version then /v1/default/banks/<id>/config). The per-(url, bank) cache bounds repeat cost, but consider folding the version fetch into the bank-config call for >= 0.8.6 APIs if the endpoint supports it, or firing the two probes concurrently.
3. The cache dict _append_capability_cache is now keyed by (api_url, bank_id) and grows without bound across banks. Other module caches cap size (e.g. registry's _CHECK_FN_CACHE_MAX); consider a bounded cache or eviction policy for long-running processes that churn many banks.
4. Minor: quote(bank_id, safe="") is applied for the URL but the cache key uses the raw bank_id — fine since the key is just an identity, but worth a comment noting the URL encoding is transport-only.

[cmoiccool]

Thanks for the review. I rebased the PR onto current main; the latest follow-up is f646f39e70.

1. Missing resolved policy: the fail-closed behavior is intentional. Hindsight documents the bank-config response as fully resolved; if store_document_text is nevertheless absent, Hermes cannot prove that append is safe. An explicit regression test shows this case uses the cumulative non-append fallback rather than risking a rejected/lossy append.
2. Probe latency: when /version explicitly reports bank_config_api: false, Hermes skips the guaranteed-unavailable bank-config request and fails closed after one probe. When the feature is available, both the API version and effective per-bank policy are needed; the result remains cached per target.
3. Cache growth: the process-wide cache is bounded to 256 URL/bank entries with oldest-entry eviction, covered by a templated-bank regression test.
4. Raw vs encoded bank ID: the cache comment documents that the raw bank ID is identity, while URL encoding is transport-only.

The latest commit also adds exact boundary-value coverage:

• 0.8.5 preserves legacy append behavior and does not query bank config.
• Exact 0.8.6 queries the resolved bank policy.
• store_document_text: true enables append at 0.8.6.
• store_document_text: false uses the two-turn cumulative non-append path at 0.8.6.

Local verification on the rebased head:

• append-capability tests: 12 passed
• memory-plugin suite: 359 passed, 1 deselected (the deselected local dependency-version check fails identically on current upstream main in this environment)
• Ruff, compileall, and git diff --check: passed

Fresh exact-head CI is complete:

• 51 checks passed and 4 were intentionally skipped.
• All 12 Python test shards, e2e, Python lint, Windows/macOS checks, supply-chain checks, Hadolint, and both Docker builds passed.
• OSV failed before scanning because GitHub returned HTTP 429 while downloading actions/download-artifact after three attempts; consequently its SARIF artifact did not exist.
• The repository review gate still requires a maintainer to add ci-reviewed; the aggregate check reflects OSV plus that label gate.

There is no observed code or test failure on the updated head. The remaining handoff is maintainer review/label and rerunning the failed infrastructure job.

[cmoiccool]

Rebased onto current main again after the branch had accumulated substantial drift.

Current exact state:

• base: 1bbb6e5bce56e721ab685af4cd87df21bbff4d35
• head: cd02cc9cdd8cbf54d2a5e504f43f3577b7a33ccd
• GitHub: MERGEABLE / CLEAN, 0 commits behind
• diff remains limited to the Hindsight provider and its regression tests
• unresolved review threads: 0

Verification on the rebased head:

• focused append/policy compatibility tests: 11 passed
• memory-plugin suite: 370 passed, 1 deselected
• Ruff, compileall, and git diff --check: passed
• fresh exact-head CI: all required checks passed, including Python, e2e, Windows/macOS, OSV, supply-chain, Docker amd64/arm64, and Nix flake checks

No code or CI blocker remains on the updated head; the PR is ready for maintainer review.