refactor: organize chatbot rag orchestration - #121
Merged
Conversation
📝 Walkthrough워크스루채봇 도구 라우팅 결과에 이유(reason) 필드를 추가하여 각 라우팅 결정을 추적하고, 서비스 계층에서 단계별 로깅을 구현하며, 회귀 테스트로 모든 라우팅 경로를 검증합니다. 변경사항도구 라우팅 로깅 및 회귀 검증
예상되는 코드 리뷰 난이도🎯 3 (중간) | ⏱️ ~25 분 관련된 PR
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
LLM/OSS/service.py (1)
404-441:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOSS 빈 응답 경로에서 모델 호출이 중복될 수 있습니다.
mode=="oss"에서grounded_fallback.text가 비어 있는 경우, Line 404에서 1회 호출 후 미해결 시 Line 430에서 빈 컨텍스트로 다시 호출될 수 있습니다. 동일 요청당 외부 호출이 최대 2회 발생해 지연/비용이 커집니다.🔧 제안 수정
@@ if output: latency = int((time.monotonic() - start) * 1000) _log_executor.submit(_log_chatbot, user_text, "oss", output, None, False, latency) return {"engine": "oss", "text": output} + + if grounded_fallback is not None and not grounded_fallback.text.strip(): + return cache_and_return({ + "engine": "oss", + "text": "관련 근거를 충분히 확인하지 못했어요. 질문을 조금 더 구체적으로 다시 입력해 주세요.", + }) fallback = grounded_fallback or run_final_fallback_tools(mode, user_text)🤖 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 `@LLM/OSS/service.py` around lines 404 - 441, The OSS path can call call_oss_async twice for the same request when the first output is empty and grounded_fallback.text is empty, causing duplicate external calls; fix by tracking the initial OSS attempt (e.g., a flag like oss_attempted_output or reusing the variable output) and skip the second fused call if the first OSS call already ran for this user_text and produced an empty/unsuccessful result, or only build the fused prompt when grounded_fallback.text (or fallback.text) is non-empty; update logic around call_oss_async, grounded_fallback, fallback, and the fused call to avoid making the second call for the same empty-context case.
🧹 Nitpick comments (1)
tests/regression/check_chatbot_tool_routing.py (1)
80-81: ⚡ Quick winfallback reason 검증이 문구 결합에 과도하게 의존합니다.
현재는
"rag context"문자열 포함 여부를 고정 검사해서, 의미는 같아도 문구만 바뀌면 테스트가 실패합니다. 이 케이스는 non-empty 또는 구조적 신호(llm_required,name) 중심으로 확인하는 편이 더 안정적입니다.🔧 제안 수정
- if "rag context" not in fallback.reason: - errors.append(f"fallback_reason_unexpected:{fallback.reason}") + if not fallback.reason: + errors.append("fallback_reason_missing")🤖 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/regression/check_chatbot_tool_routing.py` around lines 80 - 81, The current test relies on a brittle substring match of fallback.reason ("rag context"); instead, update the check to validate structural/non-empty signals: verify that fallback.llm_required is truthy or fallback.name is a non-empty string, or at minimum that fallback.reason is non-empty, and only append to errors via errors.append(...) if none of those structural conditions hold; target the fallback object attributes (fallback.reason, fallback.llm_required, fallback.name) and replace the substring check with these resilience checks.
🤖 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.
Outside diff comments:
In `@LLM/OSS/service.py`:
- Around line 404-441: The OSS path can call call_oss_async twice for the same
request when the first output is empty and grounded_fallback.text is empty,
causing duplicate external calls; fix by tracking the initial OSS attempt (e.g.,
a flag like oss_attempted_output or reusing the variable output) and skip the
second fused call if the first OSS call already ran for this user_text and
produced an empty/unsuccessful result, or only build the fused prompt when
grounded_fallback.text (or fallback.text) is non-empty; update logic around
call_oss_async, grounded_fallback, fallback, and the fused call to avoid making
the second call for the same empty-context case.
---
Nitpick comments:
In `@tests/regression/check_chatbot_tool_routing.py`:
- Around line 80-81: The current test relies on a brittle substring match of
fallback.reason ("rag context"); instead, update the check to validate
structural/non-empty signals: verify that fallback.llm_required is truthy or
fallback.name is a non-empty string, or at minimum that fallback.reason is
non-empty, and only append to errors via errors.append(...) if none of those
structural conditions hold; target the fallback object attributes
(fallback.reason, fallback.llm_required, fallback.name) and replace the
substring check with these resilience checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a636c700-c28f-4f97-98c6-dd839032754f
📒 Files selected for processing (5)
.github/workflows/rag-light-check.yml.gitignoreLLM/OSS/service.pyLLM/OSS/tools.pytests/regression/check_chatbot_tool_routing.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
LLM/OSS/service.py (1)
404-429:⚠️ Potential issue | 🟠 Major | ⚡ Quick win빈 RAG 컨텍스트에서도 자유형 OSS 호출이 다시 열립니다.
Line 404에서
grounded_fallback.text가 비어 있어도call_oss_async(messages_for_oss)를 먼저 호출합니다.LLM/OSS/tools.py의run_final_fallback_tools()는llm_required=True인rag_context_required를 빈text로도 반환할 수 있어서, 현재 분기에서는 근거가 없는데도 OSS가 임의 답변을 생성할 수 있습니다. Line 425의 안내 응답은 OSS 출력도 비었을 때만 실행돼 방어가 되지 않습니다.🔧 제안 수정
- output = await call_oss_async(messages_for_oss) + if grounded_fallback is not None and not grounded_fallback.text.strip(): + return cache_and_return({ + "engine": "oss", + "text": "관련 근거를 충분히 확인하지 못했어요. 질문을 조금 더 구체적으로 다시 입력해 주세요.", + }) + + output = await call_oss_async(messages_for_oss) if not any(keyword in user_text for keyword in ("연락처", "전화", "번호", "문의")) and not any(keyword in user_text for keyword in COUNCIL_KWS): output = scrub_non_contact(output) @@ - if grounded_fallback is not None and not grounded_fallback.text.strip(): - return cache_and_return({ - "engine": "oss", - "text": "관련 근거를 충분히 확인하지 못했어요. 질문을 조금 더 구체적으로 다시 입력해주세요." - })🤖 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 `@LLM/OSS/service.py` around lines 404 - 429, The code calls call_oss_async(messages_for_oss) before checking grounded_fallback, allowing OSS to answer even when RAG context is empty; move the grounded_fallback check earlier: run run_final_fallback_tools() (or otherwise obtain grounded_fallback) before calling call_oss_async, and if grounded_fallback is not None and grounded_fallback.text.strip() is false, immediately return the guidance response via cache_and_return instead of invoking call_oss_async; update the flow around messages_for_oss / call_oss_async and the subsequent empty-output guard so OSS is only invoked when grounded_fallback permits generating free-form responses.
🤖 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.
Outside diff comments:
In `@LLM/OSS/service.py`:
- Around line 404-429: The code calls call_oss_async(messages_for_oss) before
checking grounded_fallback, allowing OSS to answer even when RAG context is
empty; move the grounded_fallback check earlier: run run_final_fallback_tools()
(or otherwise obtain grounded_fallback) before calling call_oss_async, and if
grounded_fallback is not None and grounded_fallback.text.strip() is false,
immediately return the guidance response via cache_and_return instead of
invoking call_oss_async; update the flow around messages_for_oss /
call_oss_async and the subsequent empty-output guard so OSS is only invoked when
grounded_fallback permits generating free-form responses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85a8e6cf-d47a-47dd-ab42-b1913a407d0d
📒 Files selected for processing (1)
LLM/OSS/service.py
9 tasks
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
1 participant
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.
🎯 배경
🔍 주요 내용
ToolResult에reason을 추가해 pseudo tool 라우팅 사유를 남기도록 개선chatbot_tool_route로그 추가oss모드에서 fast path 이후 RAG context 기반 grounded fallback을 먼저 시도하도록 조정.gitignore에 추가변경 요약(1~3줄)
OSS 챗봇의 서버 주도형(의사) 도구 라우팅을 명확히 하기 위해 ToolResult에 라우팅 근거(reason)를 추가하고 단계별 라우팅 로깅을 도입했습니다. OSS 모드에서 빠른 경로 후 RAG 컨텍스트 기반의 근거 있는 폴백을 우선 시도하도록 흐름을 재구성했습니다.
주요 변경점
주의/리스크
다음 액션