fix(coffee_order, customer_support): fix incorrect knowledge initialization check logic - #266
Open
tongjixl wants to merge 1 commit into
Open
fix(coffee_order, customer_support): fix incorrect knowledge initialization check logic#266tongjixl wants to merge 1 commit into
tongjixl wants to merge 1 commit into
Conversation
…zation check logic Fix the same bug in two use-cases (coffee_order + customer_support), which was originally introduced for customer_support and later copied to coffee_order. Bugs in original code: - `len(test_knowledge) >= 0` is always True for any list, so it cannot detect an empty search result. - Accessing `test_knowledge[0]` without checking list non-emptiness raises `IndexError` on empty knowledgebase; the error was silently caught by the broad `except Exception`, causing the init decision to rely on an exception path instead of explicit logic. - `str(...).__contains__(...)` is an unpythonic way of membership test. - The inverted `not (A and B and C)` structure makes intent harder to read. Fix summary: - Assume knowledge base needs initialization by default (True). - Use Python short-circuit evaluation: only access `test_knowledge[0]` when `test_knowledge` is truthy, avoiding IndexError on empty list. - Replace `__contains__` magic call with the `in` operator. - Keep the broad exception guard so any transient search error still falls through to initialization, but remove the redundant assignment (it was already the default), using `pass` inside except for clarity. Verified via: - `python3 -m py_compile` on both modified files (syntax OK). - Simulated 8 scenario cases (empty list, empty content, mismatch, exact match, multi-item list, None input, search raises) -> all pass.
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.
Summary
Fix the knowledge base initialization check logic used by both coffee_order and customer_support use-cases. The identical buggy snippet was originally introduced for customer_support and later copied to coffee_order. It contains a logic flaw that cannot reliably detect an empty knowledge base and may fall through to an IndexError disguised by the broad exception handler.
Bug Analysis
Both modified files share the same defects:
len(test_knowledge) >= 0 is always True for any list, so this check cannot detect an empty search result.
Because of (1), when the knowledge base is empty test_knowledge[0] is accessed directly on an empty list, which raises IndexError. The outer "except Exception" swallows it and happens to flip should_init_knowledge to True - the result is correct, but the decision is made through an unintended exception path instead of explicit logic.
str(x).contains(y) is an unpythonic way of writing "y in x".
The inverted structure "not (A and B and C)" reduces readability.
Affected lines:
python/02-use-cases/coffee_order/agent.py lines 46-55
python/02-use-cases/customer_support/assistant/agent.py lines 121-130
Changes
Before (old code):
After (new code):
Key points:
Default assumption = needs initialization (True), only flips to False when data is explicitly valid.
Leverages Python short-circuit evaluation: test_knowledge[0] is never evaluated when test_knowledge is empty, eliminating the IndexError at the source.
Replaces contains magic-method call with the idiomatic "in" operator.
Keeps the broad exception guard (any transient search failure still falls through to initialization), but uses "pass" instead of a redundant reassignment.
Files Changed
Modified 2 files, 8 insertions(+), 14 deletions(-):
python/02-use-cases/coffee_order/agent.py (-11 changed lines, new logic at lines 46-52)
python/02-use-cases/customer_support/assistant/agent.py (-11 changed lines, new logic at lines 121-127)
The two patches are symmetric; only the probe value differs (literal "拿铁咖啡" vs variable knowledge_probe).
Verification
Syntax check: python3 -m py_compile on both files -> OK
Static logic simulation, 8 scenarios ALL PASSED:
Empty list (uninitialized knowledge base) -> should_init = True, OK
List with empty-string content -> should_init = True, OK
List with non-matching content -> should_init = True, OK
List with exact matching content -> should_init = False, OK
Multi-item list, first item matches -> should_init = False, OK
Multi-item list, only second item matches -> should_init = True, OK (matches top_k=1 semantics)
None as input -> no exception leaked, OK
knowledge.search raises runtime error -> falls through to init, OK