fix(consolidation): stop emitting unsupported maxItems that breaks all Bedrock consolidation (#2500) - #2502
Conversation
nicoloboschi
left a comment
There was a problem hiding this comment.
@r266-tech can you make this change only true if a new config about the llm "supports max items" is set to false (default = true). the maxItems is actually very important here and removing from any llm is quite concerning to me
36c717c to
d56ae5e
Compare
|
Reworked this around an explicit capability flag as requested.
I also added zero-slot and positive-slot tests that make the LLM overproduce with schema enforcement disabled and assert the returned creates are still capped, plus environment/default and schema-shape coverage. The focused tests pass (10 tests), changed-source Ruff checks pass, and the reworked diff passed Codex adversarial review. |
|
@r266-tech please rebase |
…l Bedrock consolidation (vectorize-io#2500) _build_response_model attached a Pydantic max_length to creates, which serializes to JSON-schema maxItems; Bedrock Converse rejects maxItems on array types, failing 100% of consolidation for capped Bedrock banks. The cap is already enforced by the prompt capacity note + unconditional truncation to remaining_observation_slots, so the schema constraint is dropped.
…-io#2500 regression) Rewrite TestBuildResponseModel to the new contract: factory always returns the base model, schema omits maxItems (Bedrock-compatible), over-cap creates are accepted (truncated downstream) rather than rejected. End-to-end cap enforcement remains covered by the existing max_observations_per_scope integration tests.
d56ae5e to
ea9a6d7
Compare
|
Rebased onto During the post-rebase adversarial review I also tightened the new capability flag: it now defaults to |
Fixes #2500.
Problem
Setting
max_observations_per_scope > 0on a Bedrock-backed bank (any Claude model via Bedrock Converse) causes 100% of consolidation batches to fail — every batch fails all 3 retries and is silently skipped, so memory quality decays permanently:Root cause
_build_response_model()dynamically subclasses the consolidation response model when an observation cap is active and puts a Pydanticmax_lengthoncreates. Pydantic v2 serializes listmax_lengthto the JSON-schemamaxItemskeyword, and Bedrock Converse rejectsmaxItemson array types outright — so a singlemaxItemsin the structured-output schema breaks the whole call.Fix
Drop the schema-level constraint:
_build_response_model()now always returns the base_ConsolidationBatchResponse, whosecreatesfield carries no length constraint, so the emitted schema is provider-portable.This is safe because the observation cap does not depend on the schema hint — it is enforced by two other layers that are unchanged:
remaining_observation_slots <= len(memories), or== 0), the consolidation prompt already tells the model"This scope has {k} observation slot(s) remaining … Prefer UPDATE over CREATE"/"OBSERVATION LIMIT REACHED … Do NOT create new ones". Every provider (Bedrock or not) still receives the cap.creates = creates[:remaining_observation_slots], with your own comment "some LLM providers may not enforce JSON schema max_length". This was already the guaranteed enforcement path.Known tradeoff (documented deliberately)
Providers that did honor
maxItemslose that redundant schema-level ceiling and now rely on the prompt note + keep-first-N truncation. But keep-first-N was already your designated fallback for non-honoring providers, and consolidation creates are not priority-ordered, so this matches existing semantics. If you'd prefer to preserve the hint for compatible providers, a provider-gated variant (emitmaxItemsonly when the provider supports it) is a straightforward follow-up — I kept this PR to the minimal, provably-safe change that unblocks Bedrock.Tests
TestBuildResponseModelis rewritten to the new contract:test_returns_base_model_regardless_of_cap— factory always returns the base model.test_schema_omits_max_items— max_observations_per_scope > 0 breaks ALL consolidation on Bedrock: emits unsupported "maxItems" JSON Schema keyword #2500 regression: emitted schema must not containmaxItemsoncreates.test_model_does_not_reject_creates_over_cap— over-cap responses are accepted (truncated downstream), not rejected wholesale.End-to-end cap enforcement remains covered by the existing
test_max_observations_per_scope_*integration tests (which exercise the truncation path via a mock LLM, independent of schema validation).