diff --git a/hindsight-api-slim/hindsight_api/engine/consolidation/consolidator.py b/hindsight-api-slim/hindsight_api/engine/consolidation/consolidator.py index 4d4022b69..e63c788f5 100644 --- a/hindsight-api-slim/hindsight_api/engine/consolidation/consolidator.py +++ b/hindsight-api-slim/hindsight_api/engine/consolidation/consolidator.py @@ -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, diff --git a/hindsight-api-slim/hindsight_api/engine/memory_engine.py b/hindsight-api-slim/hindsight_api/engine/memory_engine.py index f9e11fcd9..f6f17bafa 100644 --- a/hindsight-api-slim/hindsight_api/engine/memory_engine.py +++ b/hindsight-api-slim/hindsight_api/engine/memory_engine.py @@ -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( diff --git a/hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py b/hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py index b121d3813..3d275f5cc 100644 --- a/hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py +++ b/hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py @@ -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} diff --git a/hindsight-api-slim/hindsight_api/engine/retain/orchestrator.py b/hindsight-api-slim/hindsight_api/engine/retain/orchestrator.py index b380ce235..42ce50f52 100644 --- a/hindsight-api-slim/hindsight_api/engine/retain/orchestrator.py +++ b/hindsight-api-slim/hindsight_api/engine/retain/orchestrator.py @@ -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, diff --git a/hindsight-api-slim/hindsight_api/engine/search/retrieval.py b/hindsight-api-slim/hindsight_api/engine/search/retrieval.py index 9b2b835b4..00be800d2 100644 --- a/hindsight-api-slim/hindsight_api/engine/search/retrieval.py +++ b/hindsight-api-slim/hindsight_api/engine/search/retrieval.py @@ -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( diff --git a/hindsight-api-slim/tests/test_ann_iterative_scan.py b/hindsight-api-slim/tests/test_ann_iterative_scan.py index c0afc0d5b..7644f88a8 100644 --- a/hindsight-api-slim/tests/test_ann_iterative_scan.py +++ b/hindsight-api-slim/tests/test_ann_iterative_scan.py @@ -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", diff --git a/hindsight-api-slim/tests/test_causal_relation_offsets.py b/hindsight-api-slim/tests/test_causal_relation_offsets.py index 5aaff7fe3..b83b73902 100644 --- a/hindsight-api-slim/tests/test_causal_relation_offsets.py +++ b/hindsight-api-slim/tests/test_causal_relation_offsets.py @@ -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, diff --git a/hindsight-api-slim/tests/test_fact_extraction_retry.py b/hindsight-api-slim/tests/test_fact_extraction_retry.py index 389b6296c..0a671cfc9 100644 --- a/hindsight-api-slim/tests/test_fact_extraction_retry.py +++ b/hindsight-api-slim/tests/test_fact_extraction_retry.py @@ -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): diff --git a/hindsight-api-slim/tests/test_llm_strict_schema.py b/hindsight-api-slim/tests/test_llm_strict_schema.py index 222f13e36..b10e2a5b5 100644 --- a/hindsight-api-slim/tests/test_llm_strict_schema.py +++ b/hindsight-api-slim/tests/test_llm_strict_schema.py @@ -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 diff --git a/hindsight-api-slim/tests/test_multilingual_bm25.py b/hindsight-api-slim/tests/test_multilingual_bm25.py index 9042430c5..b033cd744 100644 --- a/hindsight-api-slim/tests/test_multilingual_bm25.py +++ b/hindsight-api-slim/tests/test_multilingual_bm25.py @@ -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]) diff --git a/hindsight-api-slim/tests/test_prompt_brace_escape.py b/hindsight-api-slim/tests/test_prompt_brace_escape.py index 13c7abf33..03cbb25d9 100644 --- a/hindsight-api-slim/tests/test_prompt_brace_escape.py +++ b/hindsight-api-slim/tests/test_prompt_brace_escape.py @@ -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 ( diff --git a/hindsight-api-slim/tests/test_retain_chunking_config_access.py b/hindsight-api-slim/tests/test_retain_chunking_config_access.py new file mode 100644 index 000000000..f12b2c2b4 --- /dev/null +++ b/hindsight-api-slim/tests/test_retain_chunking_config_access.py @@ -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)