fix: guard vendor_id/invoice_id=None in chat assistant tool callables (#418) - #579
Open
Deez-Automations wants to merge 1 commit into
Conversation
…GenAI-Security-Project#418) VendorChatAssistant and CoPilotAssistant's native tool callables (_call_get_vendor_details, _call_get_invoice_details, _call_get_vendor_invoices, _call_get_vendor_payment_summary, _call_get_vendor_contact_info) forwarded a None vendor_id/invoice_id straight to the underlying data functions with no guard. Corrections made against the linked issue while fixing: - Scope is 12 call sites, not 5: the same 5 methods are duplicated byte-for-byte across both VendorChatAssistant and CoPilotAssistant (10 sites), plus two CoPilotAssistant-only methods with the identical unguarded pattern found while auditing the rest of _build_native_callables() (_call_get_vendor_compliance_docs, _call_get_vendor_activity_report). - The issue claims a TypeError/SQLAlchemy error propagates. Traced the real behavior instead: SQLAlchemy translates `Column == None` to a valid `IS NULL` clause, so VendorRepository.get_vendor(None) just returns no row, and the application code's own `if not X: raise ValueError(...)` fires -- always ValueError, never TypeError. - Severity correction: every one of these methods is called exclusively through _execute_tool, which already wraps the call in a broad try/except and converts any exception into a clean JSON error string before it reaches the LLM. So this was never an uncaught crash -- the real bug is a misleading error message ("vendor not found" instead of "you forgot to supply vendor_id"), which risks the LLM reasoning incorrectly (e.g. concluding a real vendor doesn't exist). 13 new tests, one per guarded method plus one integration test through _execute_tool documenting the severity correction above.
This was referenced Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #418.
The bug
VendorChatAssistantandCoPilotAssistant's native tool callables forwardvendor_id/invoice_idstraight to the underlying data functions with no None check. If the LLM calls a tool without supplying the ID argument, it reaches the data layer asNone.Corrections made against the linked issue
VendorChatAssistant(vendor-portal-facing,POST /chat) andCoPilotAssistant(admin-portal-facing,POST /copilot/chat) — 10 sites total. Auditing the rest of_build_native_callables()for the same pattern turned up two more CoPilotAssistant-only methods with the identical gap:_call_get_vendor_compliance_docsand_call_get_vendor_activity_report. 12 sites fixed total.TypeError(or a raw SQLAlchemy error) propagates. Traced the actual behavior instead of trusting that: SQLAlchemy translatesColumn == Noneinto a validIS NULLclause, so e.g.VendorRepository.get_vendor(None)just returns no matching row (primary keys are never NULL) rather than raising anything at the DB layer. It's the application code's ownif not X: raise ValueError(...)checks that actually fire. Every affected path raisesValueError, neverTypeError._execute_tool, which already wraps the call in a broadtry/except Exceptionand converts any exception into a clean JSON error string before it reaches the LLM. So aNoneID here was never an uncaught crash — the real (still worth fixing) bug is a misleading error message (e.g. "Vendor not found" when the actual problem is "you forgot to pass vendor_id"), which risks the LLM reasoning incorrectly, like concluding a real vendor doesn't exist.Fix
Added an explicit
is Noneguard at the top of all 12 methods, returning{"error": "<field> is required"}before the call reaches the data layer, and widened the type hints toint | Noneto reflect reality.Tests
13 new tests in
tests/unit/agents/test_chat_assistant_none_guards.py: one per guarded method (5 onVendorChatAssistant, 7 onCoPilotAssistant), plus one integration test through_execute_tooldocumenting the severity-correction point above. Confirmed RED before the fix, GREEN after.Test plan
tests/unit/agents/,tests/unit/ctf/) run — 3 pre-existing failures intest_specialized_agents.pyconfirmed unrelated (different agent classes, reproduced on a clean stash of this branch's change, i.e. present onmainwithout this fix)origin/main— clean