feat(llm): route operations by metadata - #3900
Conversation
Strix Security ReviewWarning This pull request has 1 commit after the last Strix review ( No security issues found. Updated for Reviewed by Strix |
7c4ed2d to
57f3876
Compare
nicoloboschi
left a comment
There was a problem hiding this comment.
Reviewed and tested locally on 57f3876 (rebased check: the branch is 33 commits behind main).
Verdict: not mergeable as-is — it breaks 26 existing tests, and CI will not tell you. This is a fork PR, so the test-api jobs are skipped rather than run; the green checkmarks here cover a much smaller subset than they appear to.
Verification performed
| Check | Result |
|---|---|
./scripts/hooks/lint.sh |
pass |
Generated docs skill in sync (git status after lint) |
clean |
| New tests: routing, config, provider, batch | 84 passed |
Full -k "retain or multi_llm or consolidation or reflect or tag or metadata or config" |
27 failed, 1819 passed |
Same suites on origin/main |
101 passed, 0 failed |
Of the 27 failures, 26 are introduced by this PR (24 consolidation + 2 async-retain-tags; see the two inline Must fix comments). The remaining two — test_worker.py::...test_claim_batch_skips_consolidation_when_same_bank_processing and test_fact_extraction_agent_experience.py::...test_user_interaction_classified_as_experience — both pass in isolation and are pre-existing flakes, not attributable here.
Summary of findings
Must fix
consolidator.py:2215—with_configcalled on an already-configured wrapper; works in prod only via__getattr__passthrough, breaks 24 tests.memory_engine.py:18435— unconditionalself._retain_llm_configaccess breaks the test this PR itself edits.
Should fix
metadata.*classification is only persisted foritems[0], so a multi-item document can silently lose its route and a later append lands on the primary.- Pooled DB connection held across external store I/O in
_stored_append_routing_states, plus an N+1. - Two tag routes to different members construct fine but break every reflect and every consolidation at runtime — validate it in
MultiLLMProvider.__init__.
Nits — comment/implementation mismatch on route ordering; document_tags=[] still dropped on the file-convert path.
What holds up well
The batch path is genuinely well covered — test_multi_llm_batch.py verifies the sensitive lane owns both submit and resume, which was the leak path I most expected to find untested. The fail-closed reasoning for reflect (expand can widen a fact to its full document) and for consolidation (facts arriving mid-job) is sound and well documented. Routing values correctly stay out of LLM trace metadata.
57f3876 to
02399a5
Compare
|
Addressed every review item and rebased onto current
Validation: repository lint and @nicoloboschi this should be ready for re-review. @strix-security please review the latest commits. |
02399a5 to
0954c0d
Compare
|
Rebased once more onto current Fresh validation on the rebased branch:
All seven addressed review threads are now resolved, and GitHub reports the PR mergeable. @nicoloboschi ready for re-review when convenient. @strix-security please review the latest head. |
|
superseded by #4188 |
Summary
metadatamulti-LLM strategy for exact tag andmetadata.*routes.Configuration
Unmatched retains use member
0. Retain can also route on user-defined values such asmetadata.classification. Reflect and consolidation route from tags; with a tag route configured, they bind fail-closed to its member because they can admit new or transitive data during the operation. Routing values are ephemeral and are not written to LLM trace metadata.Verification
./scripts/hooks/lint.shgit diff --checkCloses #1140