mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(memory): honor explicit store=false on Responses requests (#2017)
## Description This PR addresses the source-backed `store=false` mutation documented inside #1944. Headroom currently injects Responses memory tools by silently flipping explicit `store=false` to `true`, which Codex-backed Responses requests reject. The fix respects explicit `store=false` by skipping only the tool-continuation memory path for those requests. Memory context injection stays unchanged. Refs #1944 ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - Honor explicit `store=false` on `/v1/responses`. - Skip only Responses memory tools that depend on stored-response continuation. - Preserve current behavior when the client does not opt out of storage. - Add focused regression coverage and a changelog note. ## Testing - [x] Unit tests pass - [x] Linting passes - [ ] Type checking passes - [x] New tests added for new functionality when applicable - [ ] Manual testing performed ### Test Output ```text uv run pytest tests/test_openai_responses_context_compaction.py -q 11 passed uv run ruff check headroom/proxy/handlers/openai.py tests/test_openai_responses_context_compaction.py All checks passed uv run ruff format --check headroom/proxy/handlers/openai.py tests/test_openai_responses_context_compaction.py 2 files already formatted ``` ## Real Behavior Proof - Environment: Responses client with explicit `store=false` - Exact command / steps: send a memory-enabled `/v1/responses` request with `store=false` - Observed result: the handler now preserves explicit `store=false` and skips only the Responses memory-tool injection path that depends on stored-response continuation; focused regression coverage proves stored/default requests still allow the path - Not tested: the Desktop disconnect tracked separately in #1944 ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the CHANGELOG.md if applicable ## Additional Notes The large Codex Desktop mid-stream disconnect remains a separate external-proof problem. This PR is intentionally limited to the explicit `store=false` mutation proven in the same issue thread. Co-authored-by: JerrettDavis <mxjerrett@gmail.com> Co-authored-by: Tejas Chopra <chopratejas@gmail.com>
This commit is contained in:
Rod Boev
GitHub
parent
f542b70413
commit
31abb696dd
3 changed files with 37 additions and 70 deletions
|
|
@ -102,6 +102,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
|
||||
### Bug Fixes
|
||||
|
||||
* **memory:** honor explicit `store=false` on OpenAI `/v1/responses` requests by skipping Headroom memory-tool injection that depends on stored-response continuations. Memory context injection stays available, and requests no longer get rewritten to `store=true` behind the client's back ([#1944](https://github.com/headroomlabs-ai/headroom/issues/1944)).
|
||||
* **proxy/batch:** stop corrupting Google `batchGenerateContent` requests whose contents interleave text turns with text-less entries (functionCall/functionResponse/images). The batch handler restored preserved (non-text) entries with the raw-index loop that #836 replaced everywhere else — indexing the shorter `optimized_contents` (text-less entries produce no message) by the original `contents[]` index, which overwrites the wrong entry and drops any preserved entry whose original index is past the optimized length. A request like `[user text, model functionCall, user functionResponse, model text]` was forwarded to Google as two entries: the model's answer overwritten by the functionCall and the functionResponse dropped. The batch handler now uses the shared `_rebuild_gemini_contents` interleaving helper, so all entries survive in order.
|
||||
* **proxy/gemini:** preserve Gemini code-execution parts (`executableCode` / `codeExecutionResult`) across the compression round-trip. `_has_non_text_parts` only recognized `inlineData`/`fileData`/`functionCall`/`functionResponse`, so a content entry carrying code-execution parts was not marked as preserved. A mixed `text`+`executableCode` entry lost its code payload (only the text survived), and a text-less code-execution entry was treated as a phantom that dropped the entire turn and shifted a neighboring message into the wrong role slot. Both keys are now recognized so those entries are preserved verbatim.
|
||||
* **cache/ccr:** don't evict a live entry when a duplicate hash is re-stored at capacity. `CompressionStore.store` ran `_evict_if_needed()` before checking whether the key already existed, so re-storing an already-present hash while the store was full evicted the oldest *distinct* entry to "make room" and then merely overwrote the existing key in place — no room was ever needed. The store dropped below `max_entries` and a live, never-retrieved entry was destroyed, so its `<<ccr:...>>` marker (still in the conversation) resolved to a 404. The CCR mirror bridge re-stores the same `explicit_hash` every turn a marker is re-encountered, so this fired routinely. Eviction now runs only for a genuinely new key.
|
||||
|
|
|
|||
|
|
@ -603,27 +603,17 @@ def _compact_openai_responses_tools(
|
|||
return updated, True, before, after
|
||||
|
||||
|
||||
def _ensure_responses_store_for_memory_tools(
|
||||
payload: dict[str, Any],
|
||||
*,
|
||||
memory_tools_injected: bool,
|
||||
) -> bool:
|
||||
"""Keep Responses API memory-tool continuations addressable.
|
||||
def _responses_request_allows_memory_tool_continuation(payload: dict[str, Any]) -> bool:
|
||||
"""Return whether Responses memory tools may rely on stored continuations.
|
||||
|
||||
Memory tools are transparent to clients: Headroom executes the emitted
|
||||
function_call, then sends function_call_output in a continuation request
|
||||
using previous_response_id. OpenAI only allows that continuation when the
|
||||
previous response was stored. Clients such as pi/Codex can set store=false
|
||||
to avoid retaining ordinary responses, but that makes memory-tool
|
||||
continuations fail with previous_response_not_found.
|
||||
|
||||
Return True when this function changes the payload.
|
||||
Headroom memory tools use ``previous_response_id`` continuations after a
|
||||
tool call. Those continuations require the originating response to be
|
||||
stored. When a client explicitly sends ``store=false``, preserve that
|
||||
contract and skip the Responses memory-tool injection path instead of
|
||||
mutating the request.
|
||||
"""
|
||||
|
||||
if memory_tools_injected and payload.get("store") is False:
|
||||
payload["store"] = True
|
||||
return True
|
||||
return False
|
||||
return payload.get("store") is not False
|
||||
|
||||
|
||||
def _responses_input_item_text_bytes(item: Any) -> int:
|
||||
|
|
@ -4066,28 +4056,27 @@ class OpenAIHandlerMixin:
|
|||
else:
|
||||
memory_tool_defs_responses.append(t)
|
||||
|
||||
resp_tools = body.get("tools") or []
|
||||
resp_tools, mem_tools_injected = _apply_sticky_mem_tools_resp(
|
||||
provider="openai",
|
||||
session_id=_responses_session_id,
|
||||
request_id=request_id,
|
||||
existing_tools=resp_tools,
|
||||
memory_tools_to_inject=memory_tool_defs_responses,
|
||||
inject_this_turn=bool(self.memory_handler.config.inject_tools),
|
||||
)
|
||||
if mem_tools_injected:
|
||||
body["tools"] = resp_tools
|
||||
body_mutation_tracker.mark_mutated("responses_memory_tools")
|
||||
logger.info(f"[{request_id}] Memory: Injected memory tools (openai/responses)")
|
||||
|
||||
if _ensure_responses_store_for_memory_tools(
|
||||
body,
|
||||
memory_tools_injected=True,
|
||||
):
|
||||
body_mutation_tracker.mark_mutated("responses_memory_store")
|
||||
if _responses_request_allows_memory_tool_continuation(body):
|
||||
resp_tools = body.get("tools") or []
|
||||
resp_tools, mem_tools_injected = _apply_sticky_mem_tools_resp(
|
||||
provider="openai",
|
||||
session_id=_responses_session_id,
|
||||
request_id=request_id,
|
||||
existing_tools=resp_tools,
|
||||
memory_tools_to_inject=memory_tool_defs_responses,
|
||||
inject_this_turn=bool(self.memory_handler.config.inject_tools),
|
||||
)
|
||||
if mem_tools_injected:
|
||||
body["tools"] = resp_tools
|
||||
body_mutation_tracker.mark_mutated("responses_memory_tools")
|
||||
logger.info(
|
||||
f"[{request_id}] Memory: forced store=true for Responses memory tool continuation"
|
||||
f"[{request_id}] Memory: Injected memory tools (openai/responses)"
|
||||
)
|
||||
elif self.memory_handler.config.inject_tools:
|
||||
logger.info(
|
||||
"[%s] Memory: skipped Responses memory tools because client set store=false",
|
||||
request_id,
|
||||
)
|
||||
except Exception as e:
|
||||
logger.warning(f"[{request_id}] Memory injection failed (responses): {e}")
|
||||
elif self.memory_handler and memory_user_id and _bypass:
|
||||
|
|
|
|||
|
|
@ -6,8 +6,8 @@ from typing import Any
|
|||
from headroom.proxy.handlers.openai import (
|
||||
OpenAIHandlerMixin,
|
||||
_compact_openai_responses_tools,
|
||||
_ensure_responses_store_for_memory_tools,
|
||||
_openai_responses_context_budget,
|
||||
_responses_request_allows_memory_tool_continuation,
|
||||
)
|
||||
from headroom.transforms.content_router import (
|
||||
CompressionStrategy,
|
||||
|
|
@ -438,48 +438,25 @@ def test_content_router_retries_kompress_when_structured_strategy_noops(monkeypa
|
|||
assert strategy_chain == ["smart_crusher", "kompress"]
|
||||
|
||||
|
||||
def test_responses_memory_tools_store_false_regression() -> None:
|
||||
"""Regression: store=false makes previous_response_id continuations fail."""
|
||||
def test_responses_memory_tools_skip_explicit_store_false() -> None:
|
||||
"""Regression: explicit store=false must block Responses memory-tool injection."""
|
||||
|
||||
payload = {"model": "gpt-5.5", "input": "remember this", "store": False}
|
||||
|
||||
changed = _ensure_responses_store_for_memory_tools(
|
||||
payload,
|
||||
memory_tools_injected=True,
|
||||
)
|
||||
|
||||
assert changed is True
|
||||
assert payload["store"] is True
|
||||
assert _responses_request_allows_memory_tool_continuation(payload) is False
|
||||
assert payload["store"] is False
|
||||
|
||||
|
||||
def test_responses_memory_tools_do_not_change_unrelated_requests() -> None:
|
||||
def test_responses_memory_tools_allow_default_and_stored_requests() -> None:
|
||||
no_memory_payload = {"model": "gpt-5.5", "input": "plain", "store": False}
|
||||
already_stored_payload = {"model": "gpt-5.5", "input": "plain", "store": True}
|
||||
default_store_payload = {"model": "gpt-5.5", "input": "plain"}
|
||||
|
||||
assert (
|
||||
_ensure_responses_store_for_memory_tools(
|
||||
no_memory_payload,
|
||||
memory_tools_injected=False,
|
||||
)
|
||||
is False
|
||||
)
|
||||
assert _responses_request_allows_memory_tool_continuation(no_memory_payload) is False
|
||||
assert no_memory_payload["store"] is False
|
||||
|
||||
assert (
|
||||
_ensure_responses_store_for_memory_tools(
|
||||
already_stored_payload,
|
||||
memory_tools_injected=True,
|
||||
)
|
||||
is False
|
||||
)
|
||||
assert _responses_request_allows_memory_tool_continuation(already_stored_payload) is True
|
||||
assert already_stored_payload["store"] is True
|
||||
|
||||
assert (
|
||||
_ensure_responses_store_for_memory_tools(
|
||||
default_store_payload,
|
||||
memory_tools_injected=True,
|
||||
)
|
||||
is False
|
||||
)
|
||||
assert _responses_request_allows_memory_tool_continuation(default_store_payload) is True
|
||||
assert "store" not in default_store_payload
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue