fix(tools): skip a malformed person doc instead of 500ing the conversation list#9494
Conversation
…ation list get_conversations_text built its speaker-name people list with an unguarded [Person(**p) for p in people_data]. get_people_by_ids only guarantees the id field (it setdefaults id for legacy docs) while Person requires name, so one legacy or partially-written person doc missing name raises ValidationError. That block is not wrapped in try/except and the GET /v1/tools/conversations handler does not catch it, so a single bad person doc 500s the whole conversation list. Skip and log the malformed person and continue (that speaker's name just goes unresolved), matching the deserialize_conversation loop in the same function and the guard in the sibling search_conversations_text. Internal to utils/, no route/model/contract change. Verification: python -m pytest tests/unit/test_tools_router.py -> 86 passed, 1 skipped. black + pyright clean; check_module_stub_pollution -> 0.
|
Heads-up on CI: the single red check (Backend unit suite) is the pre-existing main breakage, not this PR. The failing test is tests/unit/test_app_client_schema_inventory.py::test_inventory_strict_raw_decode_site_gate_fails_with_actionable_sites, which hardcodes an app/lib/backend/schema/app.dart raw-decode line number that drifted from :31 to :32 when #9470 landed on main. The one-line fix is up as #9482; once that merges this suite goes green on rerun. This PR only touches utils/retrieval/tool_services/conversations.py and its test, and its own tests pass: tests/unit/test_tools_router.py -> 86 passed, 1 skipped. |
|
Thanks @ZachL111 — this is a clean, well-scoped fix. A couple of notes: Verified correct. The source change wraps the Tests. The new CI failure is unrelated. The only failing check is I have not formally approved because CI is red on this branch (approval gate requires passing checks), but the change itself looks correct and low-risk. Once the unrelated CI failure is resolved (or this is confirmed pre-existing), a maintainer should be good to approve/merge. Labeling |
|
Thanks David, appreciate the thorough read. Agreed on all points. Confirmed the only red check is the pre-existing tests/unit/test_app_client_schema_inventory.py gate (the app.dart raw-decode line drift from #9470), not this PR; the one-line fix is up as #9482 and this suite goes green on rerun once it merges. No code changes here otherwise. |
kodjima33
left a comment
There was a problem hiding this comment.
Backend hardening fix — reviewed, approving.
What
get_conversations_text(the service behindGET /v1/tools/conversations) builds its speaker-name lookup with an unguarded list comprehension:get_people_by_idsonly guarantees theidfield (itsetdefaultsidand its own comment notes legacy docs), whilePersonrequiresname. A single legacy or partially-written person doc that is missingname(or has a null field) raisesValidationError. That block is not wrapped in try/except, and the route handler atrouters/tools.pydoes not catch it either, so one bad person doc turns the entire conversation-list response into an HTTP 500.Fix
Skip and log the malformed person, then continue, so that speaker's name just goes unresolved instead of failing the whole list. This mirrors two guards that already sit right next to it:
deserialize_conversationloop in the same function (per-item try/except + continue)search_conversations_text, whose identicalPerson(**p)line is already inside a try/exceptConfined to
utils/retrieval/tool_services/conversations.py; no route, model, or contract change.Testing
python -m pytest tests/unit/test_tools_router.py-> 86 passed, 1 skipped. The newTestGetConversationsTextMalformedPersonfeeds one valid and one malformed person doc and asserts the call returns the conversations (good speaker resolved, malformed one skipped) instead of raising.