Skip to content

fix(mcp): pass the retain strategy to the worker - #3296

Open
fhiltscher wants to merge 1 commit into
vectorize-io:mainfrom
fhiltscher:fix/mcp-retain-pass-strategy
Open

fix(mcp): pass the retain strategy to the worker#3296
fhiltscher wants to merge 1 commit into
vectorize-io:mainfrom
fhiltscher:fix/mcp-retain-pass-strategy

Conversation

@fhiltscher

Copy link
Copy Markdown

Summary

The MCP retain tool accepts a strategy but never delivers it to the worker, so every strategy override is silently ignored and the bank-level configuration is used instead.

build_content_dict puts the strategy into the content item, and the call passes only the contents:

content_dict, error = build_content_dict(content, context, timestamp, tags, metadata, document_id, strategy, update_mode)
...
result = await memory.submit_async_retain(
    bank_id=target_bank,
    contents=[content_dict],
    request_context=request_context,
)

submit_async_retain writes the strategy into the task payload from its own keyword argument only:

if strategy:
    task_payload["strategy"] = strategy

Nothing reads a strategy back out of the content items, so the payload has none, apply_strategy() never runs, and retain_mission, entity_labels, retain_chunk_size and entities_allow_free_form all fall back to the bank config. No error, no warning.

Both async retain registrations are affected. sync_retain, a few lines below, already does it correctly — this PR applies the same idiom:

strategy=content_dict.pop("strategy", None),

api_retain (REST) is unaffected: it groups items by item.strategy and forwards it.

Impact

Measured on a bank whose canonical_document strategy restricts record_type to state, rule, procedure via per-strategy entity_labels, importing the same three documents twice:

Import path Facts Types outside the strategy schema
MCP 123 4 (3 analysis, 1 decision)
REST, identical config 61 0

The illegal values are only possible because the bank-level vocabulary was in force — with the strategy's entity_labels applied, build_labels_model() turns them into a Literal[...] that structured output cannot violate.

Worth noting for triage: the dry-run extract endpoint cannot reveal this, because it strips the extracted labels from its response. The defect is only visible in the stored facts, where it looks like model non-compliance rather than lost configuration.

Test

test_retain_passes_strategy_to_the_worker asserts that submit_async_retain receives strategy as a keyword. It fails without the change; the existing TestRetainNewParams cases keep passing. tests/test_mcp_tools.py: 201 passed. ./scripts/hooks/lint.sh passes.

The async retain tool built the strategy into the content dict and then
called submit_async_retain without it. That method only writes a strategy
into the task payload when it is passed as the keyword argument, so the
worker never saw one: apply_strategy() did not run and every strategy
override — mission, entity labels, chunk size, free-form setting — fell
back to the bank configuration, silently.

Pass it the way sync_retain right below already does. The REST endpoint
is unaffected; api_retain groups items by strategy and forwards it.
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.

1 participant