Skip to content

fix(reflect): safely strip leaked done-tool siblings - #2972

Closed
gvilleg wants to merge 2 commits into
vectorize-io:mainfrom
gvilleg:fix/cleaner-leaked-sibling-fields
Closed

fix(reflect): safely strip leaked done-tool siblings#2972
gvilleg wants to merge 2 commits into
vectorize-io:mainfrom
gvilleg:fix/cleaner-leaked-sibling-fields

Conversation

@gvilleg

@gvilleg gvilleg commented Jul 26, 2026

Copy link
Copy Markdown

Why

Follow-up to #2298. Some providers can close the answer string and then leak sibling done arguments such as memory_ids, observation_ids, mental_model_ids, or directive_compliance into the returned answer text. The existing cleanup handled complete JSON envelopes and a few textual patterns, but missed truncated sibling fields after a quoted answer. A broad regex would also risk deleting legitimate prose that merely discusses those field names.

What changed

  • parse the terminal suffix as JSON-aware sibling fields instead of matching field names alone
  • require the leaked IDs to match positive retrieval provenance from the current reflect run
  • preserve complete JSON documents and prose without matching provenance
  • retain an empty cleaned result so metadata-only leakage reaches the existing NO_ANSWER_TEXT failure guard
  • add regression coverage for field permutations, multiple ID fields, escaped content, unexpected keys, unknown IDs, intended terminal quotes, and metadata-only leakage

Checks

  • uv run --frozen ruff check hindsight_api/engine/reflect/agent.py tests/test_reflect_agent.py
  • uv run --frozen ruff format --check hindsight_api/engine/reflect/agent.py tests/test_reflect_agent.py
  • uv run --frozen pytest tests/test_reflect_agent.py -k TestCleanDoneAnswer -q — 20 passed
  • independent review suite — 68 deterministic cases passed; 3 environment-dependent cases stopped at missing LLM configuration
  • repository-wide scripts/hooks/lint.sh was also run: the changed files passed their scoped Ruff checks, while unrelated control-plane ESLint failed because @eslint/js is absent and Windows ty reported existing Unix/MLX-only imports

No public API or schema changes.

gvilleg added 2 commits July 26, 2026 13:47
…swer

A rebuilt mental model was persisted with 17 memory UUIDs and a trailing JSON
object appended to its content. The answer string had been closed and its
sibling done-arguments followed it:

    ...Admin services inaccessible except via encrypted tunnel", "memory_ids":
    ["97424a2f-...", ... ]
    }

None of the four existing mechanisms catches that shape, each for a different
reason:

- _unwrap_leaked_done_arguments requires the whole text to parse as JSON, but
  the opening {"answer": " was consumed as the answer's start, so the remainder
  is not parseable.
- _DONE_CALL_PATTERN requires a literal done( in the text.
- _LEAKED_JSON_SUFFIX requires a ``` fence.
- _strip_trailing_id_json_object requires the run to begin at "{".
- _TRAILING_IDS_PATTERN requires an UNQUOTED key and end-of-string after the
  "]"; here the key is quoted and a "}" follows.

_LEAKED_SIBLING_FIELDS_PATTERN matches a well-formed run of trailing JSON
fields -- an id list followed by any number of "key": <json-value> pairs and an
optional closing brace -- anchored to end of string. Requiring that shape is
what keeps it from eating prose: ordinary text does not end that way, and a
fenced JSON example mid-document is untouched.

The stripper runs only when the text does not parse as JSON. A complete JSON
document is either a real done-argument object, already handled upstream by
_unwrap_leaked_done_arguments (which deliberately declines objects with
unexpected keys), or a JSON answer the caller actually requested. Without that
gate this pattern rewrote the existing
test_clean_answer_rejects_done_arguments_with_unexpected_keys fixture, whose
last field is an empty id list -- truncation, not the presence of an id field,
is what identifies the leak.

Three regressions added: the exact production shape, multiple sibling id fields
with and without a closing brace, and prose that merely mentions memory_ids or
embeds a fenced JSON example. Verified by mutation -- disabling the gate fails
two of them. Suite: 63 passed, same 3 pre-existing errors as unmodified main
(they require live LLM credentials).
@gvilleg
gvilleg marked this pull request as ready for review July 26, 2026 22:31
@gvilleg

gvilleg commented Jul 26, 2026

Copy link
Copy Markdown
Author

Coordination note: #2960 and this PR were both cut from the same main commit and both modify _process_done_tool answer handling. If #2960 lands first, this PR will need a rebase/conflict resolution in that block.

The safe composed ordering is:

  1. Clean raw_answer with _clean_done_answer(...), passing all three available_*_ids sets.
  2. Compute answer_failure_reason = None if answer else "empty_answer" from the cleaned answer.
  3. Only then substitute NO_ANSWER_TEXT for an empty answer.

Please do not compute the failure reason from raw_answer: a metadata-only leaked sibling tail is non-empty before cleaning, so that resolution silently reopens the persistence bypass while still passing cleaner-only tests. The combined path has been verified end-to-end: a tail-only leak produces answer_failure_reason="empty_answer" plus the sentinel, while a genuine synthesis produces None and non-sentinel text.

nicoloboschi added a commit that referenced this pull request Jul 28, 2026
… text

Reflect is driven by structured tool calls. Some provider transports don't
actually support function calling and silently strip the tool definitions from
the request (e.g. litellm's Vertex AI gpt-oss MaaS path drops tools/tool_choice
when the model is flagged unsupported). The model then answers in free text that
mimics a done() payload, which landed in message.content with empty tool_calls.
The old code served that raw text as the answer, so a growing pile of regex/JSON
"strippers" tried to claw the leaked memory_ids/observation_ids/directive_compliance
siblings back out of the user-facing answer.

Instead of salvaging untooled text, fail loudly:

- Track whether the model ever produced a tool call reflect could parse. If it
  never does (the stripped-tools case), raise ReflectToolCallError -> HTTP 500
  (the request is valid; the server's configured model can't do the job) with a
  clear message (provider, model, response snippet).
- Keep the done tool; _process_done_tool now trusts args["answer"] verbatim.
  A parsed tool call can't bleed its sibling id fields into the answer string.
- A model that DID tool-call and later stops with text is a legitimate stop and
  still routes through the clean forced-final synthesis path.
- Delete the entire strip zoo: _clean_done_answer, _unwrap_leaked_done_arguments,
  _strip_trailing_id_json_object, _clean_answer_text, _DONE_CALL_PATTERN, and the
  leaked-JSON regexes/key-sets. The forced-final paths return the model's prose
  directly (tools are disabled there, so there is no tool syntax to strip).

No static supports_function_calling gate -- reflect just tries and fails.

Supersedes the answer-salvage approach in #2972.
@nicoloboschi

Copy link
Copy Markdown
Collaborator

Superseded by #3013, which changes the approach: instead of adding more salvage logic to strip leaked done siblings out of the answer text, #3013 removes the entire strip zoo and fails reflect loudly (ReflectToolCallError → HTTP 500) when a transport can't produce a usable tool call. Root cause traced to litellm's Vertex AI gpt-oss MaaS transformer stripping tools/tool_choice from the request (the same model tool-calls cleanly over Ollama). Closing in favor of that fix.

nicoloboschi added a commit that referenced this pull request Jul 28, 2026
… text

Reflect is driven by structured tool calls. Some provider transports don't
actually support function calling and silently strip the tool definitions from
the request (e.g. litellm's Vertex AI gpt-oss MaaS path drops tools/tool_choice
when the model is flagged unsupported). The model then answers in free text that
mimics a done() payload, which landed in message.content with empty tool_calls.
The old code served that raw text as the answer, so a growing pile of regex/JSON
"strippers" tried to claw the leaked memory_ids/observation_ids/directive_compliance
siblings back out of the user-facing answer.

Instead of salvaging untooled text, fail loudly:

- Track whether the model ever produced a tool call reflect could parse. If it
  never does (the stripped-tools case), raise ReflectToolCallError -> HTTP 500
  (the request is valid; the server's configured model can't do the job) with a
  clear message (provider, model, response snippet).
- Keep the done tool; _process_done_tool now trusts args["answer"] verbatim.
  A parsed tool call can't bleed its sibling id fields into the answer string.
- A model that DID tool-call and later stops with text is a legitimate stop and
  still routes through the clean forced-final synthesis path.
- Delete the entire strip zoo: _clean_done_answer, _unwrap_leaked_done_arguments,
  _strip_trailing_id_json_object, _clean_answer_text, _DONE_CALL_PATTERN, and the
  leaked-JSON regexes/key-sets. The forced-final paths return the model's prose
  directly (tools are disabled there, so there is no tool syntax to strip).

No static supports_function_calling gate -- reflect just tries and fails.

Supersedes the answer-salvage approach in #2972.
nicoloboschi added a commit that referenced this pull request Jul 28, 2026
… text (#3013)

* fix(reflect): fail on unusable tool calls instead of salvaging leaked text

Reflect is driven by structured tool calls. Some provider transports don't
actually support function calling and silently strip the tool definitions from
the request (e.g. litellm's Vertex AI gpt-oss MaaS path drops tools/tool_choice
when the model is flagged unsupported). The model then answers in free text that
mimics a done() payload, which landed in message.content with empty tool_calls.
The old code served that raw text as the answer, so a growing pile of regex/JSON
"strippers" tried to claw the leaked memory_ids/observation_ids/directive_compliance
siblings back out of the user-facing answer.

Instead of salvaging untooled text, fail loudly:

- Track whether the model ever produced a tool call reflect could parse. If it
  never does (the stripped-tools case), raise ReflectToolCallError -> HTTP 500
  (the request is valid; the server's configured model can't do the job) with a
  clear message (provider, model, response snippet).
- Keep the done tool; _process_done_tool now trusts args["answer"] verbatim.
  A parsed tool call can't bleed its sibling id fields into the answer string.
- A model that DID tool-call and later stops with text is a legitimate stop and
  still routes through the clean forced-final synthesis path.
- Delete the entire strip zoo: _clean_done_answer, _unwrap_leaked_done_arguments,
  _strip_trailing_id_json_object, _clean_answer_text, _DONE_CALL_PATTERN, and the
  leaked-JSON regexes/key-sets. The forced-final paths return the model's prose
  directly (tools are disabled there, so there is no tool syntax to strip).

No static supports_function_calling gate -- reflect just tries and fails.

Supersedes the answer-salvage approach in #2972.

* test(mock): drive the reflect loop via tool calls, not bare prose

The reflect agent now rejects a turn that yields no usable tool call
(ReflectToolCallError). MockLLM's default call_with_tools returned bare
"mock response" content with no tool calls, which the old salvage path served
as the answer -- so ~15 reflect integration tests (empty-bank, tracing,
based_on, tags, think) started failing with 500 under the new guard.

Make MockLLM simulate a compliant tool-calling provider in its default path:
honor a forced retrieval tool_choice (so recall/search actually run and populate
based_on), and otherwise finish via the done tool. Tests that script their own
turns via _response_callback / _mock_response are unaffected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants