mirror of
https://github.com/vectorize-io/hindsight.git
synced 2026-09-14 19:31:49 +08:00
fix(retain): drop silent getattr config defaults so a wrong config fails loudly (#3610)
`getattr(config, "<field>", default)` cannot safely read a Hindsight config field: StaticConfigProxy signals "this field is bank-configurable" by raising ConfigFieldAccessError, which subclasses AttributeError — so the default swallows the guard and substitutes a global value for the bank's resolved one. That footgun manufactured the incorrect reproduction in #3584 (a bank resolved at retain_chunk_size 12000 read back as 3000). It also sat on all three sites that compute chunk boundaries, which must agree or delta retain sees every stored chunk as changed — including MemoryEngine._retain_chunking_config, annotated `config: HindsightConfig` and documented as mirroring the orchestrator. Converts every such read in the engine to direct attribute access. The two consolidator helpers that contract for a None config keep their explicit None checks; the dynamic `getattr(config, f"{prefix}llm_...")` lookups already raise and are untouched. Six test helpers were relying on these defaults via partial SimpleNamespace/MagicMock(spec=...) stubs — exactly the wrong-config case this surfaces — and now build a real config. Refs #3584
This commit is contained in:
@@ -217,7 +217,7 @@ def _dedup_active(config: Any) -> bool:
|
||||
skipped — it behaves exactly as it did before this feature, regardless of the configured
|
||||
threshold. This is why the feature can ship enabled-by-default without breaking Oracle.
|
||||
"""
|
||||
if config is None or getattr(config, "consolidation_dedup_threshold", 1.0) >= 1.0:
|
||||
if config is None or config.consolidation_dedup_threshold >= 1.0:
|
||||
return False
|
||||
return get_config().database_backend != "oracle"
|
||||
|
||||
@@ -962,7 +962,7 @@ def _effective_scope_limit(config: Any, fact_tags: list[str]) -> int:
|
||||
"""
|
||||
if config is None:
|
||||
return -1
|
||||
for rule in _parse_scope_limit_rules(getattr(config, "observation_scope_limits", None)):
|
||||
for rule in _parse_scope_limit_rules(config.observation_scope_limits):
|
||||
if _scope_matches_globs(rule.globs, fact_tags):
|
||||
return rule.limit
|
||||
return config.max_observations_per_scope
|
||||
@@ -2900,7 +2900,7 @@ async def _consolidate_batch_with_llm(
|
||||
# note, and response_schema (all bank/batch-variable) are kept OUT of the
|
||||
# cached prefix so one cache serves all and it never busts within a run.
|
||||
system_prompt = build_consolidation_system_prompt(
|
||||
llm_output_language=getattr(config, "llm_output_language", None),
|
||||
llm_output_language=config.llm_output_language if config is not None else None,
|
||||
)
|
||||
user_content = build_consolidation_input(
|
||||
facts_text=facts_lines,
|
||||
|
||||
@@ -38,7 +38,6 @@ from ..config import (
|
||||
DEFAULT_RECALL_INCLUDE_CHUNKS,
|
||||
DEFAULT_RECALL_MAX_TOKENS,
|
||||
DEFAULT_REFLECT_SOURCE_FACTS_MAX_TOKENS,
|
||||
DEFAULT_RETAIN_CHUNK_SIZE,
|
||||
DEFAULT_STORE_DOCUMENT_TEXT,
|
||||
ENV_MODEL_INIT_TIMEOUT,
|
||||
HindsightConfig,
|
||||
@@ -4896,8 +4895,8 @@ class MemoryEngine(MemoryEngineInterface):
|
||||
def _retain_chunking_config(config: HindsightConfig) -> _RetainChunkingConfig:
|
||||
"""The chunk boundaries ``config`` implies, as the retain pipeline uses them."""
|
||||
return _RetainChunkingConfig(
|
||||
chunk_size=getattr(config, "retain_chunk_size", DEFAULT_RETAIN_CHUNK_SIZE),
|
||||
structured_chunk_size=getattr(config, "retain_structured_chunk_size", None),
|
||||
chunk_size=config.retain_chunk_size,
|
||||
structured_chunk_size=config.retain_structured_chunk_size,
|
||||
)
|
||||
|
||||
async def _run_retain_execution(
|
||||
|
||||
@@ -1130,9 +1130,9 @@ def _build_extraction_prompt_and_schema(config) -> tuple[str, type]:
|
||||
base_response_class = FactExtractionResponseNoCausal
|
||||
|
||||
# Add entity labels section if configured and build dynamic schema
|
||||
entity_labels_raw = getattr(config, "entity_labels", None)
|
||||
entity_labels_raw = config.entity_labels
|
||||
labels_cfg = parse_entity_labels(entity_labels_raw)
|
||||
free_form_entities = getattr(config, "entities_allow_free_form", True)
|
||||
free_form_entities = config.entities_allow_free_form
|
||||
labels_section = _build_labels_prompt_section(labels_cfg, free_form_entities)
|
||||
if labels_section:
|
||||
prompt = prompt + labels_section
|
||||
@@ -1145,7 +1145,7 @@ def _build_extraction_prompt_and_schema(config) -> tuple[str, type]:
|
||||
# tokenization and LLM output language are separate concerns.
|
||||
from ..prompt_utils import output_language_directive
|
||||
|
||||
prompt = prompt + output_language_directive(getattr(config, "llm_output_language", None))
|
||||
prompt = prompt + output_language_directive(config.llm_output_language)
|
||||
|
||||
response_schema = base_response_class
|
||||
|
||||
@@ -1193,7 +1193,7 @@ def _retain_mission_preamble(config) -> str:
|
||||
No brace-escaping needed: unlike the system template, the user message is
|
||||
used verbatim, not passed through str.format().
|
||||
"""
|
||||
retain_mission = getattr(config, "retain_mission", None)
|
||||
retain_mission = config.retain_mission
|
||||
if not retain_mission:
|
||||
return ""
|
||||
return (
|
||||
@@ -1543,9 +1543,9 @@ async def _extract_facts_from_chunk(
|
||||
validated_entities = _coerce_entity_strings(get_value("entities"))
|
||||
|
||||
# Post-process label entities from structured labels object
|
||||
entity_labels_raw = getattr(config, "entity_labels", None)
|
||||
entity_labels_raw = config.entity_labels
|
||||
labels_cfg = parse_entity_labels(entity_labels_raw)
|
||||
free_form_entities = getattr(config, "entities_allow_free_form", True)
|
||||
free_form_entities = config.entities_allow_free_form
|
||||
if labels_cfg and labels_cfg.attributes:
|
||||
labels_lookup = build_labels_lookup(labels_cfg)
|
||||
labels_data = llm_fact.get("labels") or {}
|
||||
@@ -2297,9 +2297,9 @@ async def extract_facts_from_contents_batch_api(
|
||||
validated_entities = _coerce_entity_strings(get_value("entities"))
|
||||
|
||||
# Post-process label entities from structured labels object
|
||||
entity_labels_raw = getattr(config, "entity_labels", None)
|
||||
entity_labels_raw = config.entity_labels
|
||||
labels_cfg_batch = parse_entity_labels(entity_labels_raw)
|
||||
free_form_entities_batch = getattr(config, "entities_allow_free_form", True)
|
||||
free_form_entities_batch = config.entities_allow_free_form
|
||||
if labels_cfg_batch and labels_cfg_batch.attributes:
|
||||
labels_lookup_batch = build_labels_lookup(labels_cfg_batch)
|
||||
labels_data = llm_fact.get("labels") or {}
|
||||
@@ -2758,7 +2758,7 @@ def _inject_label_tags(facts: list[ExtractedFactType], config) -> None:
|
||||
This lets entity labels double as tags, enabling filtering via the
|
||||
existing tags API without any extra query infrastructure.
|
||||
"""
|
||||
labels_cfg = parse_entity_labels(getattr(config, "entity_labels", None))
|
||||
labels_cfg = parse_entity_labels(config.entity_labels)
|
||||
if not labels_cfg:
|
||||
return
|
||||
tag_group_keys = {g.key.lower() for g in labels_cfg.attributes if g.tag}
|
||||
|
||||
@@ -67,7 +67,7 @@ def redact_document_body(body: str, config: Any) -> str:
|
||||
whole document once per sub-batch (issue #3282).
|
||||
"""
|
||||
try:
|
||||
policy = parse_policy(getattr(config, "memory_defense", None))
|
||||
policy = parse_policy(config.memory_defense)
|
||||
except Exception:
|
||||
return body
|
||||
if not policy.enabled:
|
||||
@@ -425,7 +425,7 @@ async def _pre_resolve_phase1(
|
||||
processed_facts,
|
||||
log_buffer,
|
||||
user_entities_per_content=user_entities_per_content,
|
||||
entity_labels=getattr(config, "entity_labels", None),
|
||||
entity_labels=config.entity_labels,
|
||||
)
|
||||
|
||||
# Semantic ANN search on the same connection (autocommit, no transaction).
|
||||
@@ -714,7 +714,7 @@ async def _streaming_batch_write_ext(
|
||||
combined_content,
|
||||
retain_params,
|
||||
merged_tags,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
)
|
||||
log_buffer.append(
|
||||
f"[streaming] Document {effective_doc_id} updated (recovery, preserving existing chunks)"
|
||||
@@ -729,7 +729,7 @@ async def _streaming_batch_write_ext(
|
||||
retain_params,
|
||||
merged_tags,
|
||||
ops=pool.ops,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
txn=ext_txn,
|
||||
)
|
||||
log_buffer.append(f"[streaming] Document {effective_doc_id} tracked (full content)")
|
||||
@@ -761,7 +761,7 @@ async def _streaming_batch_write_ext(
|
||||
effective_doc_id,
|
||||
batch_chunk_meta,
|
||||
ops=pool.ops,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
)
|
||||
|
||||
# Entity registry reassert (Postgres `entities`): re-create the resolved parents
|
||||
@@ -926,7 +926,7 @@ async def _delta_batch_write_ext(
|
||||
effective_doc_id,
|
||||
remapped_chunks,
|
||||
ops=pool.ops,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
)
|
||||
|
||||
# Entity registry reassert (Postgres `entities`) — see the streaming path (#2662).
|
||||
@@ -1218,7 +1218,7 @@ async def retain_batch(
|
||||
# object at this point (see _retain_batch_async_internal). On a non-allow
|
||||
# decision we redact in place or drop the item, and fire a
|
||||
# memory_defense.triggered webhook when one is configured.
|
||||
_policy = parse_policy(getattr(config, "memory_defense", None))
|
||||
_policy = parse_policy(config.memory_defense)
|
||||
_blocked_violations: list[BlockedViolation] = []
|
||||
|
||||
if memory_defense_extension is not None and _policy.enabled:
|
||||
@@ -1492,9 +1492,15 @@ async def retain_batch(
|
||||
# Even small documents go through the same path — they just end up as a
|
||||
# single batch. This eliminates the maintenance burden of two separate
|
||||
# retain code paths.
|
||||
chunk_batch_size = getattr(config, "retain_chunk_batch_size", 100)
|
||||
chunk_size = getattr(config, "retain_chunk_size", 3000)
|
||||
structured_chunk_size = getattr(config, "retain_structured_chunk_size", None)
|
||||
chunk_batch_size = config.retain_chunk_batch_size
|
||||
# Direct attribute access, never getattr-with-default: these two decide chunk
|
||||
# boundaries, and the delta path must derive them from the very same resolved
|
||||
# config object. A getattr default silently substitutes the global value when
|
||||
# handed the wrong config (StaticConfigProxy raises ConfigFieldAccessError,
|
||||
# an AttributeError subclass, for bank-configurable fields) — which re-chunks
|
||||
# at different boundaries and makes every stored chunk look changed. Fail loud.
|
||||
chunk_size = config.retain_chunk_size
|
||||
structured_chunk_size = config.retain_structured_chunk_size
|
||||
all_pre_chunks: list[str] = []
|
||||
chunk_to_content: list[int] = [] # maps chunk index -> index into contents
|
||||
for content_idx, content in enumerate(contents):
|
||||
@@ -1703,7 +1709,7 @@ async def _store_document_bodies(
|
||||
content_hash=content_hash,
|
||||
# Honour store_document_text: when a deployment opts out of keeping the full text, only the
|
||||
# chunk texts (needed for citation) go to the store, not the whole document body.
|
||||
original_text=combined_content if getattr(config, "store_document_text", True) else None,
|
||||
original_text=combined_content if config.store_document_text else None,
|
||||
chunk_texts=list(chunk_texts),
|
||||
tags=list(merged_tags or []),
|
||||
metadata={},
|
||||
@@ -2138,7 +2144,7 @@ async def _streaming_retain_batch(
|
||||
combined_content,
|
||||
retain_params,
|
||||
merged_tags,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
)
|
||||
else:
|
||||
# A 0-fact re-ingest still deletes the outgoing memories — tag that
|
||||
@@ -2155,7 +2161,7 @@ async def _streaming_retain_batch(
|
||||
retain_params,
|
||||
merged_tags,
|
||||
ops=pool.ops,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
txn=_edge_txn,
|
||||
)
|
||||
# Re-record the witness now that the group's writes have happened, so
|
||||
@@ -2329,7 +2335,7 @@ async def _streaming_retain_batch(
|
||||
combined_content,
|
||||
retain_params,
|
||||
merged_tags,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
)
|
||||
log_buffer.append(
|
||||
f"[streaming] Document {effective_doc_id} updated "
|
||||
@@ -2345,7 +2351,7 @@ async def _streaming_retain_batch(
|
||||
retain_params,
|
||||
merged_tags,
|
||||
ops=pool.ops,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
txn=_group_txn,
|
||||
)
|
||||
log_buffer.append(f"[streaming] Document {effective_doc_id} tracked (full content)")
|
||||
@@ -2391,7 +2397,7 @@ async def _streaming_retain_batch(
|
||||
effective_doc_id,
|
||||
batch_chunk_meta,
|
||||
ops=pool.ops,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
)
|
||||
log_buffer.append(
|
||||
f" Store chunks: {len(batch_chunk_meta)} chunks in {time.time() - step_start:.3f}s"
|
||||
@@ -2589,7 +2595,7 @@ async def _streaming_retain_batch(
|
||||
combined_content,
|
||||
retain_params,
|
||||
merged_tags,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
)
|
||||
else:
|
||||
# A no-facts re-ingest still deletes the outgoing memories — tag that
|
||||
@@ -2606,7 +2612,7 @@ async def _streaming_retain_batch(
|
||||
retain_params,
|
||||
merged_tags,
|
||||
ops=pool.ops,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
txn=_edge_txn,
|
||||
)
|
||||
# Re-record the witness now that the group's writes have happened, so the
|
||||
@@ -3211,7 +3217,7 @@ async def _try_delta_retain(
|
||||
effective_doc_id,
|
||||
remapped_chunks,
|
||||
ops=pool.ops,
|
||||
store_document_text=getattr(config, "store_document_text", True),
|
||||
store_document_text=config.store_document_text,
|
||||
)
|
||||
for chunk_idx, chunk_id in chunk_id_map.items():
|
||||
chunk_id_map_by_doc[(effective_doc_id, chunk_idx)] = chunk_id
|
||||
@@ -3402,14 +3408,17 @@ def _chunk_contents_for_delta(contents: list[RetainContent], config) -> dict[int
|
||||
Chunk contents the same way the streaming path does, returning a map of
|
||||
global_chunk_index -> chunk_text.
|
||||
|
||||
Must use the same chunk_size as the streaming path (default 3000) so that
|
||||
chunk boundaries match and delta can detect unchanged chunks.
|
||||
Previously defaulted to 120000, causing all chunks to appear changed on retry.
|
||||
Must read chunk_size/structured_chunk_size off the same resolved ``config``
|
||||
the streaming path uses, so chunk boundaries match and delta can detect
|
||||
unchanged chunks. Two earlier incidents came from this drifting: a 120000
|
||||
default made all chunks appear changed on retry, and a getattr default
|
||||
silently swapped a bank's resolved size for the global one.
|
||||
"""
|
||||
result = {}
|
||||
global_chunk_idx = 0
|
||||
chunk_size = getattr(config, "retain_chunk_size", 3000)
|
||||
structured_chunk_size = getattr(config, "retain_structured_chunk_size", None)
|
||||
# Same resolved-config invariant as the streaming path — see the note there.
|
||||
chunk_size = config.retain_chunk_size
|
||||
structured_chunk_size = config.retain_structured_chunk_size
|
||||
for content in contents:
|
||||
chunks = fact_extraction.chunk_text(
|
||||
content.content,
|
||||
|
||||
@@ -14,7 +14,7 @@ from dataclasses import dataclass, field
|
||||
from datetime import UTC, datetime
|
||||
from typing import TYPE_CHECKING, Any, Optional
|
||||
|
||||
from ...config import DEFAULT_BM25_MAX_QUERY_TERMS, get_config
|
||||
from ...config import get_config
|
||||
from ..db.ops import UpdatedWindow
|
||||
from ..memory_engine import fq_table, get_current_schema
|
||||
from ..sql import create_sql_dialect
|
||||
@@ -267,7 +267,7 @@ async def retrieve_semantic_bm25_combined_sql(
|
||||
# --- BM25 UNION ALL arms (one per fact_type, only when tokens present) ---
|
||||
if _include_bm25:
|
||||
text_ext = config.text_search_extension
|
||||
max_query_terms = getattr(config, "bm25_max_query_terms", DEFAULT_BM25_MAX_QUERY_TERMS)
|
||||
max_query_terms = config.bm25_max_query_terms
|
||||
bm25_tokens = tokens
|
||||
# Native tsvector has no IDF and ranks every `@@` match, so a long OR
|
||||
# query over common terms scans and ranks a large fraction of the bank
|
||||
@@ -281,7 +281,7 @@ async def retrieve_semantic_bm25_combined_sql(
|
||||
text_ext == "native"
|
||||
and max_query_terms > 0
|
||||
and len(tokens) > max_query_terms
|
||||
and getattr(config, "bm25_selective_terms", True)
|
||||
and config.bm25_selective_terms
|
||||
and getattr(conn, "backend_type", "postgresql") == "postgresql"
|
||||
):
|
||||
bm25_tokens = await select_selective_bm25_tokens(
|
||||
|
||||
@@ -20,6 +20,8 @@ Covers:
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import dataclasses
|
||||
|
||||
import random
|
||||
import uuid
|
||||
from types import SimpleNamespace
|
||||
@@ -106,7 +108,10 @@ class FakeConn:
|
||||
@pytest.fixture
|
||||
def search_path(monkeypatch):
|
||||
dialect = FakeDialect()
|
||||
config = SimpleNamespace(
|
||||
from hindsight_api.config import _get_raw_config
|
||||
|
||||
config = dataclasses.replace(
|
||||
_get_raw_config(),
|
||||
semantic_min_similarity=0.0,
|
||||
bm25_min_score=0.0,
|
||||
text_search_extension="native",
|
||||
|
||||
@@ -1,8 +1,10 @@
|
||||
import dataclasses
|
||||
import json
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
|
||||
from hindsight_api.config import _get_raw_config
|
||||
from hindsight_api.engine.response_models import TokenUsage
|
||||
from hindsight_api.engine.retain import fact_extraction
|
||||
from hindsight_api.engine.retain.fact_extraction import CausalRelation, Fact
|
||||
@@ -30,7 +32,7 @@ async def test_causal_targets_are_offset_from_extraction_group_start(monkeypatch
|
||||
monkeypatch.setattr(fact_extraction, "_add_temporal_offsets", lambda *_args: None)
|
||||
monkeypatch.setattr(fact_extraction, "_inject_label_tags", lambda *_args: None)
|
||||
|
||||
config = SimpleNamespace(retain_extraction_mode="normal", retain_batch_enabled=False)
|
||||
config = dataclasses.replace(_get_raw_config(), retain_extraction_mode="normal", retain_batch_enabled=False)
|
||||
facts, _, _ = await fact_extraction.extract_facts_from_contents(
|
||||
[RetainContent(content="preceding"), RetainContent(content="causal group")],
|
||||
llm_config=None,
|
||||
@@ -64,7 +66,7 @@ async def test_each_chunk_uses_its_own_causal_index_base(monkeypatch):
|
||||
monkeypatch.setattr(fact_extraction, "_add_temporal_offsets", lambda *_args: None)
|
||||
monkeypatch.setattr(fact_extraction, "_inject_label_tags", lambda *_args: None)
|
||||
|
||||
config = SimpleNamespace(retain_extraction_mode="normal", retain_batch_enabled=False)
|
||||
config = dataclasses.replace(_get_raw_config(), retain_extraction_mode="normal", retain_batch_enabled=False)
|
||||
facts, _, _ = await fact_extraction.extract_facts_from_contents(
|
||||
[RetainContent(content="two chunks")],
|
||||
llm_config=None,
|
||||
@@ -145,7 +147,8 @@ async def test_batch_causal_targets_use_each_chunk_start(monkeypatch):
|
||||
monkeypatch.setattr(fact_extraction, "_add_temporal_offsets", lambda *_args: None)
|
||||
monkeypatch.setattr(fact_extraction, "_inject_label_tags", lambda *_args: None)
|
||||
|
||||
config = SimpleNamespace(
|
||||
config = dataclasses.replace(
|
||||
_get_raw_config(),
|
||||
retain_extract_causal_links=True,
|
||||
retain_chunk_size=100,
|
||||
retain_structured_chunk_size=100,
|
||||
|
||||
@@ -9,6 +9,7 @@ BaseException'), which happened when last_error was only set in the
|
||||
BadRequestError handler and not for non-dict JSON responses.
|
||||
"""
|
||||
|
||||
import dataclasses
|
||||
import json
|
||||
from datetime import datetime, timezone
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
@@ -134,22 +135,23 @@ async def test_output_too_long_drops_unsplittable_subchunk_without_recursing():
|
||||
|
||||
def _make_config(llm_max_retries: int = 3, retain_llm_max_retries: int | None = None):
|
||||
"""Build a minimal HindsightConfig for fact extraction tests."""
|
||||
from hindsight_api.config import HindsightConfig
|
||||
from hindsight_api.config import _get_raw_config
|
||||
|
||||
cfg = MagicMock(spec=HindsightConfig)
|
||||
cfg.retain_llm_max_retries = retain_llm_max_retries
|
||||
cfg.llm_max_retries = llm_max_retries
|
||||
cfg.retain_llm_initial_backoff = None
|
||||
cfg.llm_initial_backoff = 0.0
|
||||
cfg.retain_llm_max_backoff = None
|
||||
cfg.llm_max_backoff = 0.0
|
||||
cfg.retain_max_completion_tokens = 8192
|
||||
cfg.retain_extraction_mode = "concise"
|
||||
cfg.retain_extract_causal_links = False
|
||||
cfg.retain_mission = None
|
||||
cfg.llm_temperature_retain = 0.1
|
||||
cfg.llm_strict_schema_retain = False
|
||||
return cfg
|
||||
return dataclasses.replace(
|
||||
_get_raw_config(),
|
||||
retain_llm_max_retries=retain_llm_max_retries,
|
||||
llm_max_retries=llm_max_retries,
|
||||
retain_llm_initial_backoff=None,
|
||||
llm_initial_backoff=0.0,
|
||||
retain_llm_max_backoff=None,
|
||||
llm_max_backoff=0.0,
|
||||
retain_max_completion_tokens=8192,
|
||||
retain_extraction_mode="concise",
|
||||
retain_extract_causal_links=False,
|
||||
retain_mission=None,
|
||||
llm_temperature_retain=0.1,
|
||||
llm_strict_schema_retain=False,
|
||||
)
|
||||
|
||||
|
||||
def _make_llm_config(mock_response):
|
||||
|
||||
@@ -23,6 +23,7 @@ reads the same retain-scoped field rather than the global flag, keeping the batc
|
||||
and streaming paths in agreement.
|
||||
"""
|
||||
|
||||
import dataclasses
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
@@ -45,18 +46,16 @@ class _Resp(BaseModel):
|
||||
|
||||
|
||||
def _config_with(strict: bool) -> object:
|
||||
"""A config proxy that overrides only llm_strict_schema (avoids recursion)."""
|
||||
from hindsight_api.config import get_config
|
||||
"""A real config overriding only llm_strict_schema.
|
||||
|
||||
real = get_config()
|
||||
Delegating to ``get_config()`` used to work by accident: the retain path read
|
||||
bank-configurable fields through getattr defaults, so the StaticConfigProxy
|
||||
guard was swallowed. It now reads them directly, so the test needs the raw
|
||||
resolved config rather than the global proxy.
|
||||
"""
|
||||
from hindsight_api.config import _get_raw_config
|
||||
|
||||
class _Cfg:
|
||||
llm_strict_schema = strict
|
||||
|
||||
def __getattr__(self, name):
|
||||
return getattr(real, name)
|
||||
|
||||
return _Cfg()
|
||||
return dataclasses.replace(_get_raw_config(), llm_strict_schema=strict)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
@@ -298,22 +297,23 @@ def _retain_config(strict_retain: bool):
|
||||
A bank-resolved config, not the global one -- retain_extraction_mode and friends
|
||||
are bank-configurable and raise if read off global config.
|
||||
"""
|
||||
from hindsight_api.config import HindsightConfig
|
||||
from hindsight_api.config import _get_raw_config
|
||||
|
||||
cfg = MagicMock(spec=HindsightConfig)
|
||||
cfg.retain_llm_max_retries = 1
|
||||
cfg.llm_max_retries = 1
|
||||
cfg.retain_llm_initial_backoff = 0.0
|
||||
cfg.llm_initial_backoff = 0.0
|
||||
cfg.retain_llm_max_backoff = 0.0
|
||||
cfg.llm_max_backoff = 0.0
|
||||
cfg.retain_max_completion_tokens = 8192
|
||||
cfg.retain_extraction_mode = "concise"
|
||||
cfg.retain_extract_causal_links = False
|
||||
cfg.retain_mission = None
|
||||
cfg.llm_temperature_retain = 0.1
|
||||
cfg.llm_strict_schema_retain = strict_retain
|
||||
return cfg
|
||||
return dataclasses.replace(
|
||||
_get_raw_config(),
|
||||
retain_llm_max_retries=1,
|
||||
llm_max_retries=1,
|
||||
retain_llm_initial_backoff=0.0,
|
||||
llm_initial_backoff=0.0,
|
||||
retain_llm_max_backoff=0.0,
|
||||
llm_max_backoff=0.0,
|
||||
retain_max_completion_tokens=8192,
|
||||
retain_extraction_mode="concise",
|
||||
retain_extract_causal_links=False,
|
||||
retain_mission=None,
|
||||
llm_temperature_retain=0.1,
|
||||
llm_strict_schema_retain=strict_retain,
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
@@ -233,7 +233,17 @@ def test_postgresql_extension_bm25_keeps_raw_query_text():
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_combined_retrieval_uses_default_bm25_cap_for_legacy_config(monkeypatch):
|
||||
async def test_combined_retrieval_rejects_config_missing_bm25_cap(monkeypatch):
|
||||
"""A config object lacking the BM25 cap fields fails loudly instead of defaulting.
|
||||
|
||||
This used to read ``getattr(config, "bm25_max_query_terms", DEFAULT_...)`` and
|
||||
silently fall back, on the theory that a config could predate the field. It
|
||||
cannot: ``HindsightConfig`` is a dataclass that always defines both fields, so
|
||||
only a stub like the one below can produce that state — and when one does reach
|
||||
here it is a wrong-config bug, not a legacy config. Silently substituting a
|
||||
global default for a resolved value is exactly what made #3584 undiagnosable.
|
||||
"""
|
||||
|
||||
class FakeDialect:
|
||||
max_query_terms: int | None = None
|
||||
|
||||
@@ -263,18 +273,18 @@ async def test_combined_retrieval_uses_default_bm25_cap_for_legacy_config(monkey
|
||||
monkeypatch.setattr(retrieval_mod, "get_config", lambda: legacy_config)
|
||||
monkeypatch.setattr(retrieval_mod, "create_sql_dialect", lambda backend: fake_dialect)
|
||||
|
||||
result = await retrieval_mod.retrieve_semantic_bm25_combined_sql(
|
||||
FakeConn(),
|
||||
"[0.0]",
|
||||
"alpha beta",
|
||||
"bank-1",
|
||||
["observation"],
|
||||
5,
|
||||
)
|
||||
with pytest.raises(AttributeError, match="bm25_max_query_terms"):
|
||||
await retrieval_mod.retrieve_semantic_bm25_combined_sql(
|
||||
FakeConn(),
|
||||
"[0.0]",
|
||||
"alpha beta",
|
||||
"bank-1",
|
||||
["observation"],
|
||||
5,
|
||||
)
|
||||
|
||||
assert result == {"observation": retrieval_mod.SemanticBm25Result(semantic=[], bm25=[], graph_seeds=None)}
|
||||
# A config predating the field falls back to the current default cap.
|
||||
assert fake_dialect.max_query_terms == retrieval_mod.DEFAULT_BM25_MAX_QUERY_TERMS
|
||||
# It raised while resolving the cap, so the dialect was never handed a bogus one.
|
||||
assert fake_dialect.max_query_terms is None
|
||||
|
||||
|
||||
@pytest.mark.parametrize("selective", [True, False])
|
||||
|
||||
@@ -4,6 +4,7 @@ User-supplied text (missions, custom instructions) may contain literal braces
|
||||
(e.g. JSON examples). These must survive ``str.format()`` without crashing.
|
||||
"""
|
||||
|
||||
import dataclasses
|
||||
import pytest
|
||||
|
||||
from hindsight_api.engine.prompt_utils import escape_for_prompt
|
||||
@@ -104,18 +105,21 @@ class TestReflectBraceSafety:
|
||||
|
||||
class TestRetainBraceSafety:
|
||||
def _make_config(self, **overrides):
|
||||
"""Minimal config-like object for _build_extraction_prompt_and_schema."""
|
||||
from types import SimpleNamespace
|
||||
"""A real config for _build_extraction_prompt_and_schema.
|
||||
|
||||
Not a stub: the retain path reads its config fields directly, so a partial
|
||||
namespace raises AttributeError on the first field it does not define.
|
||||
"""
|
||||
from hindsight_api.config import _get_raw_config
|
||||
|
||||
defaults = {
|
||||
"retain_extraction_mode": "concise",
|
||||
"retain_extract_causal_links": False,
|
||||
"retain_mission": None,
|
||||
"retain_custom_instructions": None,
|
||||
"retain_taxonomy": None,
|
||||
}
|
||||
defaults.update(overrides)
|
||||
return SimpleNamespace(**defaults)
|
||||
return dataclasses.replace(_get_raw_config(), **defaults)
|
||||
|
||||
def test_retain_mission_with_json(self):
|
||||
from hindsight_api.engine.retain.fact_extraction import (
|
||||
|
||||
@@ -0,0 +1,49 @@
|
||||
"""
|
||||
The retain path must read its chunking parameters off the *resolved* config.
|
||||
|
||||
Regression guard for #3584. Both the streaming write path and the delta compare
|
||||
chunk the same content, and delta detects unchanged chunks by comparing content
|
||||
hashes at equal chunk_index — so the two must agree on chunk boundaries. They do
|
||||
only as long as both derive chunk_size/structured_chunk_size from the same
|
||||
resolved HindsightConfig.
|
||||
|
||||
These used to be read as ``getattr(config, "retain_chunk_size", 3000)``. Because
|
||||
``StaticConfigProxy`` signals "this field is bank-configurable" by raising
|
||||
``ConfigFieldAccessError`` — an ``AttributeError`` subclass — that default
|
||||
swallowed the guard and silently substituted the global value for the bank's.
|
||||
A bank configured at 12000 would then be re-chunked at 3000, no chunk index
|
||||
could match, and every append fell back to a full re-extraction of the whole
|
||||
document. Direct attribute access makes a wrong config object fail loudly.
|
||||
"""
|
||||
|
||||
import dataclasses
|
||||
|
||||
import pytest
|
||||
|
||||
from hindsight_api.config import ConfigFieldAccessError, StaticConfigProxy, _get_raw_config
|
||||
from hindsight_api.engine.retain.orchestrator import _chunk_contents_for_delta
|
||||
from hindsight_api.engine.retain.types import RetainContent
|
||||
|
||||
|
||||
def _contents(text: str) -> list[RetainContent]:
|
||||
return [RetainContent(content=text)]
|
||||
|
||||
|
||||
def test_delta_chunking_rejects_global_config():
|
||||
"""Handed global config, the delta chunker raises instead of defaulting to 3000."""
|
||||
proxy = StaticConfigProxy(_get_raw_config())
|
||||
|
||||
with pytest.raises(ConfigFieldAccessError, match="retain_chunk_size"):
|
||||
_chunk_contents_for_delta(_contents("word. " * 5000), proxy)
|
||||
|
||||
|
||||
def test_delta_chunking_uses_resolved_chunk_size():
|
||||
"""Boundaries come from the resolved value, not from a hardcoded default."""
|
||||
text = "word. " * 5000
|
||||
|
||||
base = _get_raw_config()
|
||||
at_12k = _chunk_contents_for_delta(_contents(text), dataclasses.replace(base, retain_chunk_size=12000))
|
||||
at_3k = _chunk_contents_for_delta(_contents(text), dataclasses.replace(base, retain_chunk_size=3000))
|
||||
|
||||
assert max(len(c) for c in at_12k.values()) > 3000
|
||||
assert len(at_12k) < len(at_3k)
|
||||
Reference in New Issue
Block a user