feat: question-understanding pipeline with intent classification, clarifying questions, and suggested follow-ups - #81
Conversation
… clarifying questions, and suggested follow-ups
WalkthroughChangesIntent-aware chat pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatHandler
participant classify_intent
participant Gemini
Client->>ChatHandler: Submit chat message
ChatHandler->>classify_intent: Classify intent
classify_intent->>Gemini: Request structured classification
Gemini-->>classify_intent: Return intent and ambiguity
ChatHandler->>ChatHandler: Apply clarification guard
ChatHandler->>Gemini: Generate configured response
Gemini-->>ChatHandler: Return answer and follow-ups
ChatHandler-->>Client: Return ChatResponse metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@Joyful12-tech fix conflicts |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.py (1)
248-341: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftIntent classification blocks the event loop, runs before the cache check, and rebuilds a model client on every request.
Three compounding issues in this block:
- Blocking call inside an async endpoint.
classifier_model.generate_contentis called synchronously and un-awaited (line 261-264) insideasync def chat(...). This is a real (blocking) network round trip to Gemini that stalls the event loop for every concurrent request being served by this worker, not just the current one. The codebase already has an established pattern for offloading this kind of call —await safety_pipeline.run_async(request.prompt, generate)(line 382) — but this new call bypasses it entirely.- Runs unconditionally before the semantic-cache lookup. Classification (248-271) happens before
is_cacheableis computed (308-313) and before the cache-hit check (318-341). That means every cache-HIT request — which should be near-instant — now also pays for a full Gemini round trip it doesn't need, undermining the point of the cache.- A fresh
GenerativeModelis instantiated per request (254-260) instead of being created once and reused.🐛 Suggested direction
- import time - _intent_start = time.monotonic() - classifier_model = genai.GenerativeModel( - model_name="gemini-1.5-flash", - generation_config=genai.types.GenerationConfig( - temperature=0.1, - max_output_tokens=256, - ), - ) - classification = classify_intent( - request.prompt, - model_callable=classifier_model.generate_content, - ) + _intent_start = time.monotonic() + classification = await asyncio.to_thread( + classify_intent, + request.prompt, + model_callable=_CLASSIFIER_MODEL.generate_content, + )Hoist
_CLASSIFIER_MODEL = genai.GenerativeModel(...)to module scope, moveimport time/import asyncioto the top of the file, and consider moving this whole block below the semantic-cache hit check so cached answers skip it entirely.Separately (root-caused in
intent.py): because_llm_classifydoesn't catch generic exceptions from the model callable, any transient failure of this "lightweight" classifier currently fails the whole chat turn via the outerexcept Exceptionhandler — worth fixing there alongside offloading this call.As per path instructions, "Flag blocking calls inside async endpoints (network calls should be awaited or offloaded)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.py` around lines 248 - 341, Refactor the chat endpoint’s intent-classification flow around classify_intent so cacheable semantic-cache hits return before classification, while preserving classification for non-cached requests. Create the classifier GenerativeModel once at module scope, move time/asyncio imports to module scope, and offload the synchronous generate_content call through the established async execution pattern so it never blocks the event loop. Update intent.py’s _llm_classify to catch generic model-call exceptions and use its existing fallback behavior instead of failing the chat turn.Source: Path instructions
🧹 Nitpick comments (3)
tests/test_intent.py (2)
108-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for the
_GREETING_WORDSfalse-positive.Given the
"good"/"morning"/"afternoon"/"evening"standalone-token issue flagged inintent.py(lines 68-102), a case like_is_trivial_greeting("Good, thanks!")or_is_trivial_greeting("Morning, quick question")assertingFalsewould pin the fix and prevent regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_intent.py` around lines 108 - 157, Add regression coverage to TestDeterministicShortCircuit for standalone greeting-word false positives: assert _is_trivial_greeting returns False for substantive messages such as “Good, thanks!” and “Morning, quick question”, preserving the existing greeting and short-circuit tests.
164-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for
model_callableraising and for non-dict JSON output.Tied to the
_llm_classifyexception-handling gap flagged inintent.py(lines 196-251): a test wheremodel_callableraises (simulating a network/API error) and one where the fixture returns valid JSON that isn't an object (e.g.json.dumps([1, 2])) would both currently propagate an unhandled exception rather than falling back gracefully — worth locking in the fix with tests once it lands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_intent.py` around lines 164 - 241, Extend TestLLMClassification with coverage for _llm_classify failures: use a model_callable that raises an exception and assert classify_intent returns the safe factual-knowledge fallback, then use FakeModel with JSON representing a non-dict value such as a list and assert the same graceful fallback without propagating an exception.intent.py (1)
259-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
effective_params()env-var keys collide across intents sharing the samemax_output_tokens, and the method is currently unused.Keys are derived from
self.max_output_tokens(e.g.f"TEMP_{self.max_output_tokens}"), not from the intent's identity.FACTUAL_KNOWLEDGEandFIQH_RULINGboth default to2048→ they'd both read/writeTEMP_2048/TOP_P_2048/etc., even though their base temperatures differ (0.7 vs 0.6). Same collision forPERSONAL_GUIDANCE/PLATFORM_QUESTION(both1024). Separately,main.pyreadsintent_config.temperature/.top_p/.top_k/.max_output_tokensdirectly and never callseffective_params(), so the "env-overridable per intent" behavior promised in the docstring (line 268) doesn't actually apply today.Recommend keying by intent name instead (and deciding whether
main.pyshould actually call this method), so this doesn't silently misbehave if/when it gets wired up.♻️ Proposed direction
- def effective_params(self) -> Dict[str, Any]: + def effective_params(self, intent_name: str) -> Dict[str, Any]: """Return generation config dict, respecting env overrides.""" return { "temperature": float(os.getenv( - f"TEMP_{self.max_output_tokens}", + f"TEMP_{intent_name.upper()}", str(self.temperature), )), ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@intent.py` around lines 259 - 289, Update IntentConfig.effective_params() to derive environment-variable keys from each intent’s unique identity rather than max_output_tokens, adding and populating that identity wherever IntentConfig instances are defined. Then update the generation path in main.py to call effective_params() so per-intent environment overrides are actually applied while preserving configured defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@intent.py`:
- Around line 196-251: Update _llm_classify to catch any exception from
model_callable, response parsing, and JSON decoding, returning the existing
factual-knowledge ClassificationResult fallback while logging the error. After
json.loads succeeds, validate that data is a dict before calling data.get;
otherwise log the non-object result and return the same fallback.
- Around line 68-102: Remove the standalone “good”, “morning”, “afternoon”, and
“evening” entries from _GREETING_WORDS so _is_trivial_greeting only
short-circuits on unambiguous greetings; retain the existing _SALAM_PATTERNS
handling for complete greeting phrases and leave other greeting words unchanged.
In `@main.py`:
- Around line 277-291: Update the _last_classifications assignments in the
clarification response path and normal response path so the stored
classification cannot carry needs_clarification=True when no clarifying question
was asked on the current turn. Preserve the existing should_clarify decision and
response fields, but store a classification with needs_clarification cleared
after a clarification is suppressed, ensuring the next turn can clarify again.
---
Outside diff comments:
In `@main.py`:
- Around line 248-341: Refactor the chat endpoint’s intent-classification flow
around classify_intent so cacheable semantic-cache hits return before
classification, while preserving classification for non-cached requests. Create
the classifier GenerativeModel once at module scope, move time/asyncio imports
to module scope, and offload the synchronous generate_content call through the
established async execution pattern so it never blocks the event loop. Update
intent.py’s _llm_classify to catch generic model-call exceptions and use its
existing fallback behavior instead of failing the chat turn.
---
Nitpick comments:
In `@intent.py`:
- Around line 259-289: Update IntentConfig.effective_params() to derive
environment-variable keys from each intent’s unique identity rather than
max_output_tokens, adding and populating that identity wherever IntentConfig
instances are defined. Then update the generation path in main.py to call
effective_params() so per-intent environment overrides are actually applied
while preserving configured defaults.
In `@tests/test_intent.py`:
- Around line 108-157: Add regression coverage to TestDeterministicShortCircuit
for standalone greeting-word false positives: assert _is_trivial_greeting
returns False for substantive messages such as “Good, thanks!” and “Morning,
quick question”, preserving the existing greeting and short-circuit tests.
- Around line 164-241: Extend TestLLMClassification with coverage for
_llm_classify failures: use a model_callable that raises an exception and assert
classify_intent returns the safe factual-knowledge fallback, then use FakeModel
with JSON representing a non-dict value such as a list and assert the same
graceful fallback without propagating an exception.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ca9c6201-3ad5-47e7-a1de-632a3cec105a
📒 Files selected for processing (4)
.github/workflows/ci.ymlintent.pymain.pytests/test_intent.py
| _GREETING_WORDS = { | ||
| "hi", "hello", "hey", "salam", "salaam", "assalamu", "alaykum", | ||
| "assalamo", "alaikum", "as-salam", "marhaba", "ahlan", | ||
| "good", "morning", "afternoon", "evening", "peace", | ||
| } | ||
|
|
||
| # Short message length threshold — messages at or below this word count | ||
| # go through the deterministic check if they match greeting-like patterns. | ||
| _SHORT_MSG_THRESHOLD = 5 | ||
|
|
||
|
|
||
| def _is_trivial_greeting(message: str) -> bool: | ||
| """Return True if *message* is almost certainly a greeting / salam. | ||
|
|
||
| This is intentionally conservative — borderline cases fall through to | ||
| the LLM classifier so we never misclassify a real question as a greeting. | ||
| """ | ||
| stripped = message.strip() | ||
| if not stripped: | ||
| return False | ||
|
|
||
| words = stripped.split() | ||
| word_count = len(words) | ||
|
|
||
| # Very short pure-greeting messages | ||
| if word_count <= 3 and words[0].lower().strip("?!.,") in _GREETING_WORDS: | ||
| return True | ||
|
|
||
| # Salam pattern match | ||
| if word_count <= _SHORT_MSG_THRESHOLD: | ||
| for pat in _SALAM_PATTERNS: | ||
| if pat.match(stripped): | ||
| return True | ||
|
|
||
| return False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
"good"/"morning"/"afternoon"/"evening" as standalone greeting words will misclassify real replies.
Any message with ≤3 words whose first word is "good", "morning", "afternoon", or "evening" short-circuits straight to GREETING_SMALLTALK — e.g. "Good, thanks!", "Morning, quick question", "Good idea". These aren't greetings, and _SALAM_PATTERNS (lines 61-62) already handles the actual "good morning/afternoon/evening/day" phrase, so the standalone tokens are both redundant and unsafe. This directly contradicts the function's own stated intent: "Return True if message is almost certainly a greeting / salam. This is intentionally conservative — borderline cases fall through to the LLM classifier so we never misclassify a real question as a greeting."
🐛 Proposed fix
_GREETING_WORDS = {
"hi", "hello", "hey", "salam", "salaam", "assalamu", "alaykum",
"assalamo", "alaikum", "as-salam", "marhaba", "ahlan",
- "good", "morning", "afternoon", "evening", "peace",
+ "peace",
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _GREETING_WORDS = { | |
| "hi", "hello", "hey", "salam", "salaam", "assalamu", "alaykum", | |
| "assalamo", "alaikum", "as-salam", "marhaba", "ahlan", | |
| "good", "morning", "afternoon", "evening", "peace", | |
| } | |
| # Short message length threshold — messages at or below this word count | |
| # go through the deterministic check if they match greeting-like patterns. | |
| _SHORT_MSG_THRESHOLD = 5 | |
| def _is_trivial_greeting(message: str) -> bool: | |
| """Return True if *message* is almost certainly a greeting / salam. | |
| This is intentionally conservative — borderline cases fall through to | |
| the LLM classifier so we never misclassify a real question as a greeting. | |
| """ | |
| stripped = message.strip() | |
| if not stripped: | |
| return False | |
| words = stripped.split() | |
| word_count = len(words) | |
| # Very short pure-greeting messages | |
| if word_count <= 3 and words[0].lower().strip("?!.,") in _GREETING_WORDS: | |
| return True | |
| # Salam pattern match | |
| if word_count <= _SHORT_MSG_THRESHOLD: | |
| for pat in _SALAM_PATTERNS: | |
| if pat.match(stripped): | |
| return True | |
| return False | |
| _GREETING_WORDS = { | |
| "hi", "hello", "hey", "salam", "salaam", "assalamu", "alaykum", | |
| "assalamo", "alaikum", "as-salam", "marhaba", "ahlan", | |
| "peace", | |
| } | |
| # Short message length threshold — messages at or below this word count | |
| # go through the deterministic check if they match greeting-like patterns. | |
| _SHORT_MSG_THRESHOLD = 5 | |
| def _is_trivial_greeting(message: str) -> bool: | |
| """Return True if *message* is almost certainly a greeting / salam. | |
| This is intentionally conservative — borderline cases fall through to | |
| the LLM classifier so we never misclassify a real question as a greeting. | |
| """ | |
| stripped = message.strip() | |
| if not stripped: | |
| return False | |
| words = stripped.split() | |
| word_count = len(words) | |
| # Very short pure-greeting messages | |
| if word_count <= 3 and words[0].lower().strip("?!.,") in _GREETING_WORDS: | |
| return True | |
| # Salam pattern match | |
| if word_count <= _SHORT_MSG_THRESHOLD: | |
| for pat in _SALAM_PATTERNS: | |
| if pat.match(stripped): | |
| return True | |
| return False |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@intent.py` around lines 68 - 102, Remove the standalone “good”, “morning”,
“afternoon”, and “evening” entries from _GREETING_WORDS so _is_trivial_greeting
only short-circuits on unambiguous greetings; retain the existing
_SALAM_PATTERNS handling for complete greeting phrases and leave other greeting
words unchanged.
| def _llm_classify(message: str, model_callable) -> ClassificationResult: | ||
| """Call the LLM to classify *message*.""" | ||
| prompt = _CLASSIFIER_PROMPT.format(message=message) | ||
|
|
||
| if model_callable is None: | ||
| raise RuntimeError( | ||
| "classify_intent requires a model_callable for non-trivial messages." | ||
| ) | ||
|
|
||
| try: | ||
| response = model_callable(prompt) | ||
| raw = response.text.strip() | ||
|
|
||
| # Strip markdown code fences if present | ||
| if raw.startswith("```"): | ||
| # Find the first ``` and the closing ``` | ||
| start = raw.find("\n") + 1 if "\n" in raw else raw.find("```") + 3 | ||
| end = raw.rfind("```") | ||
| if end > start: | ||
| raw = raw[start:end].strip() | ||
|
|
||
| data = json.loads(raw) | ||
| except (json.JSONDecodeError, AttributeError, ValueError) as exc: | ||
| logger.warning("Intent classifier returned unparseable output: %s", exc) | ||
| return ClassificationResult( | ||
| intent=Intent.FACTUAL_KNOWLEDGE, # safe fallback | ||
| ) | ||
|
|
||
| # Extract and validate intent | ||
| intent_str = data.get("intent", "factual_knowledge") | ||
| try: | ||
| intent = Intent(intent_str) | ||
| except ValueError: | ||
| logger.warning("Unknown intent '%s'; falling back to factual_knowledge", intent_str) | ||
| intent = Intent.FACTUAL_KNOWLEDGE | ||
|
|
||
| # Ambiguity | ||
| needs_clarification = data.get("needs_clarification", False) | ||
| clarifying_question = data.get("clarifying_question", "") | ||
| ambiguity_confidence = float(data.get("ambiguity_confidence", 0.0)) | ||
|
|
||
| # Apply threshold — if confidence is below threshold, don't clarify | ||
| if ambiguity_confidence < AMBIGUITY_THRESHOLD: | ||
| needs_clarification = False | ||
| clarifying_question = "" | ||
|
|
||
| # Fallback if needs_clarification is True but no question provided | ||
| if needs_clarification and not clarifying_question: | ||
| clarifying_question = _FALLBACK_CLARIFY | ||
|
|
||
| return ClassificationResult( | ||
| intent=intent, | ||
| needs_clarification=needs_clarification, | ||
| clarifying_question=clarifying_question, | ||
| ambiguity_confidence=ambiguity_confidence, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Exception handling in _llm_classify has two gaps that let a classifier hiccup escape the "safe fallback" contract.
data.get(...)calls (lines 225, 233-235) sit outside thetryblock. Ifmodel_callablereturns valid-but-non-dict JSON (e.g."true",[1,2]),json.loadssucceeds, but the firstdata.get(...)raises an uncaughtAttributeErrorthat propagates straight out ofclassify_intent.- The
exceptclause only catches(json.JSONDecodeError, AttributeError, ValueError)— it does not catch exceptions raised bymodel_callable(prompt)itself (network errors, timeouts, quota/API errors from Gemini). Any transient classifier failure propagates uncaught out ofclassify_intent.
Since main.py's /chat handler calls classify_intent(...) directly with no local try/except, either failure mode turns a hiccup in this "lightweight" pre-classification step into a full 500 for the whole chat turn — even when the primary answer-generation call would have succeeded fine. Given the PR's stated goal of "safe fallbacks," this deserves a broader catch plus a type check.
🛡️ Proposed fix
try:
response = model_callable(prompt)
raw = response.text.strip()
# Strip markdown code fences if present
if raw.startswith("```"):
...
data = json.loads(raw)
- except (json.JSONDecodeError, AttributeError, ValueError) as exc:
+ except Exception as exc:
logger.warning("Intent classifier returned unparseable output: %s", exc)
return ClassificationResult(
intent=Intent.FACTUAL_KNOWLEDGE, # safe fallback
)
+ if not isinstance(data, dict):
+ logger.warning("Intent classifier returned non-object JSON: %r", data)
+ return ClassificationResult(intent=Intent.FACTUAL_KNOWLEDGE)
+
# Extract and validate intent
intent_str = data.get("intent", "factual_knowledge")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _llm_classify(message: str, model_callable) -> ClassificationResult: | |
| """Call the LLM to classify *message*.""" | |
| prompt = _CLASSIFIER_PROMPT.format(message=message) | |
| if model_callable is None: | |
| raise RuntimeError( | |
| "classify_intent requires a model_callable for non-trivial messages." | |
| ) | |
| try: | |
| response = model_callable(prompt) | |
| raw = response.text.strip() | |
| # Strip markdown code fences if present | |
| if raw.startswith("```"): | |
| # Find the first ``` and the closing ``` | |
| start = raw.find("\n") + 1 if "\n" in raw else raw.find("```") + 3 | |
| end = raw.rfind("```") | |
| if end > start: | |
| raw = raw[start:end].strip() | |
| data = json.loads(raw) | |
| except (json.JSONDecodeError, AttributeError, ValueError) as exc: | |
| logger.warning("Intent classifier returned unparseable output: %s", exc) | |
| return ClassificationResult( | |
| intent=Intent.FACTUAL_KNOWLEDGE, # safe fallback | |
| ) | |
| # Extract and validate intent | |
| intent_str = data.get("intent", "factual_knowledge") | |
| try: | |
| intent = Intent(intent_str) | |
| except ValueError: | |
| logger.warning("Unknown intent '%s'; falling back to factual_knowledge", intent_str) | |
| intent = Intent.FACTUAL_KNOWLEDGE | |
| # Ambiguity | |
| needs_clarification = data.get("needs_clarification", False) | |
| clarifying_question = data.get("clarifying_question", "") | |
| ambiguity_confidence = float(data.get("ambiguity_confidence", 0.0)) | |
| # Apply threshold — if confidence is below threshold, don't clarify | |
| if ambiguity_confidence < AMBIGUITY_THRESHOLD: | |
| needs_clarification = False | |
| clarifying_question = "" | |
| # Fallback if needs_clarification is True but no question provided | |
| if needs_clarification and not clarifying_question: | |
| clarifying_question = _FALLBACK_CLARIFY | |
| return ClassificationResult( | |
| intent=intent, | |
| needs_clarification=needs_clarification, | |
| clarifying_question=clarifying_question, | |
| ambiguity_confidence=ambiguity_confidence, | |
| ) | |
| def _llm_classify(message: str, model_callable) -> ClassificationResult: | |
| """Call the LLM to classify *message*.""" | |
| prompt = _CLASSIFIER_PROMPT.format(message=message) | |
| if model_callable is None: | |
| raise RuntimeError( | |
| "classify_intent requires a model_callable for non-trivial messages." | |
| ) | |
| try: | |
| response = model_callable(prompt) | |
| raw = response.text.strip() | |
| # Strip markdown code fences if present | |
| if raw.startswith(" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@intent.py` around lines 196 - 251, Update _llm_classify to catch any
exception from model_callable, response parsing, and JSON decoding, returning
the existing factual-knowledge ClassificationResult fallback while logging the
error. After json.loads succeeds, validate that data is a dict before calling
data.get; otherwise log the non-object result and return the same fallback.
| last_cls = _last_classifications.get(chat_id) | ||
| if should_clarify(classification, last_cls): | ||
| _last_classifications[chat_id] = classification | ||
| clarifying_question = classification.clarifying_question or ( | ||
| "Could you please provide more detail so I can help you better?" | ||
| ) | ||
| return ChatResponse( | ||
| response=clarifying_question, | ||
| chat_id=chat_id, | ||
| history=[], | ||
| intent=classification.intent.value, | ||
| needs_clarification=True, | ||
| clarifying_question=clarifying_question, | ||
| ) | ||
| _last_classifications[chat_id] = classification |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clarification suppression can cascade indefinitely instead of blocking just one turn.
Both branches unconditionally store the raw classification as _last_classifications[chat_id] (279, 291) — including its needs_clarification value — regardless of whether a clarifying question was actually asked this turn. Since classify_intent classifies each message in isolation (no history), a short reply like "Hanafi" answering a clarifying question can itself be independently flagged needs_clarification=True. That gets correctly suppressed this turn by should_clarify, but because the raw True is what gets remembered as "last", the next turn's guard also sees last_cls.needs_clarification=True and suppresses again — and so on. A run of consecutive ambiguous-in-isolation turns can permanently mute clarification for the rest of the session, well beyond the intended "never twice in a row."
🐛 Proposed fix
last_cls = _last_classifications.get(chat_id)
if should_clarify(classification, last_cls):
_last_classifications[chat_id] = classification
...
return ChatResponse(...)
- _last_classifications[chat_id] = classification
+ # Remember only an *unresolved* ambiguous signal; once we've answered
+ # past it, don't let it keep suppressing clarification later.
+ _last_classifications[chat_id] = (
+ classification if not classification.needs_clarification
+ else ClassificationResult(intent=classification.intent)
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| last_cls = _last_classifications.get(chat_id) | |
| if should_clarify(classification, last_cls): | |
| _last_classifications[chat_id] = classification | |
| clarifying_question = classification.clarifying_question or ( | |
| "Could you please provide more detail so I can help you better?" | |
| ) | |
| return ChatResponse( | |
| response=clarifying_question, | |
| chat_id=chat_id, | |
| history=[], | |
| intent=classification.intent.value, | |
| needs_clarification=True, | |
| clarifying_question=clarifying_question, | |
| ) | |
| _last_classifications[chat_id] = classification | |
| last_cls = _last_classifications.get(chat_id) | |
| if should_clarify(classification, last_cls): | |
| _last_classifications[chat_id] = classification | |
| clarifying_question = classification.clarifying_question or ( | |
| "Could you please provide more detail so I can help you better?" | |
| ) | |
| return ChatResponse( | |
| response=clarifying_question, | |
| chat_id=chat_id, | |
| history=[], | |
| intent=classification.intent.value, | |
| needs_clarification=True, | |
| clarifying_question=clarifying_question, | |
| ) | |
| # Remember only an *unresolved* ambiguous signal; once we've answered | |
| # past it, don't let it keep suppressing clarification later. | |
| _last_classifications[chat_id] = ( | |
| classification if not classification.needs_clarification | |
| else ClassificationResult(intent=classification.intent) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 277 - 291, Update the _last_classifications assignments
in the clarification response path and normal response path so the stored
classification cannot carry needs_clarification=True when no clarifying question
was asked on the current turn. Preserve the existing should_clarify decision and
response fields, but store a classification with needs_clarification cleared
after a clarification is suppressed, ensuring the next turn can clarify again.
|
@Joyful12-tech pls fix conflicts |
2 participants
Summary
Implements a complete question-understanding pipeline for the
/chatendpoint that classifies every message by intent, calibrates answer shape and length per intent, asks clarifying questions before guessing on ambiguous questions, and returns 2-3 suggested follow-up questions.Changes
/chatflow, clarifying-question early return, per-intent generation config replaces hardcoded values, follow-up extraction and stripping, intent metadata in ChatResponseAcceptance criteria
Closes #42
Summary by CodeRabbit
New Features
Bug Fixes
Tests