Feature/router unified routing#34
Merged
Merged
Conversation
Contributor
|
황태토끼가 리뷰하러 찾아왔어요! |
Contributor
Author
|
@coderabbitai review 리뷰해줘봐 |
|
✅ Actions performedReview triggered.
|
개요그래프 빌더의 모듈화 및 의도 기반 라우팅 시스템 재설계. 핵심 노드/엣지 등록 헬퍼 도입, 규칙 기반 의도 분류를 LLM 기반 랭킹으로 변경, 폴백 디스패치 라우터 추가, 및 각 노드의 실행 흐름 추적 메커니즘 강화. 변경 사항
시퀀스 다이어그램sequenceDiagram
actor User
participant intent_gate as intent_gate<br/>(LLM Ranking)
participant fallback_dispatch as fallback_dispatch<br/>(Intent Processor)
participant rag_path as RAG Path<br/>(retrieve/translate_ko)
participant other_intents as Other Intents<br/>(smalltalk/event/struct_db)
participant clarify as clarify<br/>(Final Fallback)
User->>intent_gate: normalized_text
intent_gate-->>intent_gate: LLM rank intents<br/>[rag, struct_db, ...]
intent_gate->>fallback_dispatch: intent_ranking, index=0
fallback_dispatch->>fallback_dispatch: check_result?
alt check_result == "good"
fallback_dispatch-->>User: fallback_next="done"<br/>(Return answer)
else check_result != "good" & has next intent
fallback_dispatch->>fallback_dispatch: advance to next intent<br/>(index+1)
fallback_dispatch->>fallback_dispatch: determine next_intent<br/>from ranking[index]
alt next_intent == "rag"
fallback_dispatch->>rag_path: fallback_next=retrieve<br/>or translate_ko
rag_path-->>fallback_dispatch: RAG result + trace
else next_intent in {smalltalk, event, struct_db}
fallback_dispatch->>other_intents: fallback_next=next_intent
other_intents-->>fallback_dispatch: result + trace
end
fallback_dispatch->>fallback_dispatch: append to<br/>fallback_history
fallback_dispatch-->>fallback_dispatch: next iteration
else all intents exhausted
fallback_dispatch->>clarify: fallback_next="clarify"
clarify-->>User: clarification prompt
end
예상 코드 리뷰 시간🎯 4 (복잡) | ⏱️ ~45분 시 🐰
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
Author
일해주세요 |
Contributor
리뷰 의견
제안: trace에 실제 사용되는 ranking도 기록
|
Contributor
총평작고 명확한 변경이며, 불필요한 하위 의도 시도를 줄여 응답 속도를 개선하려는 의도가 잘 드러납니다. trace 불일치 부분만 보완하면 좋겠습니다. |
Contributor
추가 발견 사항1. fallback 기회가 4회 → 2회로 줄어들면서 clarify 도달이 너무 빨라짐fallback_router.py:40에서 next_index >= len(ranking)이면 clarify로 빠집니다. 이전에는 4개 의도를 모두 시도한 후 clarify였지만, 이제 2번만 실패하면 바로 clarify입니다. 사용자 입장에서 답변을 못 받는 케이스가 늘어날 수 있습니다. 2. LLM이 순위를 잘못 매기면 올바른 의도가 아예 시도되지 않음예를 들어 LLM이 ["smalltalk", "event", "rag", "struct_db"]를 반환하면, 슬라이싱 후 ["smalltalk", "event"]만 남습니다. 실제 정답이 rag나 struct_db였다면 시도조차 하지 않고 clarify로 빠지게 됩니다. 4개를 모두 시도할 때는 순위가 틀려도 결국 정답에 도달했지만, 2개 제한에서는 LLM 분류 정확도에 대한 의존도가 크게 높아집니다. 3. docstring과 실제 동작 불일치intent_gate.py:16-17의 docstring: 4개 의도(rag, smalltalk, event, struct_db)를 순위로 반환. 실제로는 2개만 반환합니다. 또한 state.py:22의 주석도 4개를 가정하고 있습니다: intent_ranking: List[str] # ["rag", "event", "struct_db", "smalltalk"] 순위 4. 유효성 검증 로직과의 비효율intent_gate.py:70-73에서 누락된 의도를 보충하여 4개를 맞추는 로직이 있는데, 어차피 뒤에서 2개로 잘리므로 3번째·4번째에 추가된 의도는 의미 없는 연산입니다. 치명적이진 않지만, 코드 의도가 모호해집니다. 5. _MAX_INTENTS를 환경변수/설정으로 빼지 않은 점현재 하드코딩 상수라 값을 바꾸려면 코드 수정 + 재배포가 필요합니다. 이 값은 정확도-속도 트레이드오프에 해당하므로, 튜닝 시 환경변수나 설정 파일로 관리하면 실험이 훨씬 수월합니다. 가장 중요한 것은 1번과 2번입니다. 의도 분류 LLM의 Top-2 정확도가 충분히 높은지 검증 없이 적용하면, 기존에 잘 동작하던 케이스에서 clarify가 많이 발생할 수 있습니다. |
Contributor
Author
1번은 의도적으로 변경가능하게 한거야 |
Contributor
|
의도하신 내용이였군요! 확인해주셔서 감사합니다! |
![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)
app/graph/nodes/answer/answer_generate.py (1)
108-126:⚠️ Potential issue | 🟠 MajorLLM 예외가 성공 응답처럼 종료될 수 있습니다.
Line 110에서
llm.chat()이 실패해도 여기서는 일반 오류 문구만answer_text에 넣고 끝납니다. 그러면 뒤의answer_check_node가 내부 생성 실패를 구분하지 못해"good"으로 종료할 수 있으니, 명시적 실패 플래그를 state/trace에 남기고bad로 처리되게 연결하는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/graph/nodes/answer/answer_generate.py` around lines 108 - 126, When llm.chat(...) in answer_generate fails the code currently returns an error string as answer_text which downstream nodes (like answer_check_node) may treat as a successful response; modify the except block in answer_generate so it sets a clear failure flag in the trace/state (e.g., trace["answer_generate"]["failed"] = True or state["answer_generate_failed"] = True) and still populate answer_text with the error message or empty string, then return that trace; ensure the failure flag is added to the trace dict built just before return so downstream logic can detect the failure and mark the flow as "bad".
🧹 Nitpick comments (4)
app/graph/nodes/rag/translate_node.py (1)
46-49: Flow 추적 이름과 trace 메타데이터 키의 일관성 검토.
_flow에는"translate_ko"를 추가하고 있지만, trace 메타데이터 키는"translate"를 사용하고 있습니다. 다른 노드들은 동일한 이름을 사용합니다 (예:"retrieve"→trace["retrieve"]). 일관성을 위해 둘 중 하나로 통일하는 것을 고려해 주세요.♻️ 일관성 개선 제안
flow = list(trace.get("_flow") or []) flow.append("translate_ko") trace["_flow"] = flow - trace["translate"] = { + trace["translate_ko"] = { "original": text, "user_language": user_language, "retrieval_query_ko": retrieval_query_ko, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/graph/nodes/rag/translate_node.py` around lines 46 - 49, The flow name added to trace["_flow"] ("translate_ko") is inconsistent with the trace metadata key trace["translate"]; update either the flow entry or the metadata key so they match (e.g., change trace["_flow"].append("translate_ko") to append("translate") or rename trace["translate"] to trace["translate_ko"]) in translate_node.py so the flow trace and the node metadata key are consistent; adjust any downstream usages of trace["translate"] or the flow name accordingly.app/graph/nodes/rag/retrieve_node_v2.py (1)
20-55: MMR 로직 중복에 대한 선택적 리팩토링 고려.
_cosine_sim과_mmr_select함수가retrieve_node.py와 동일하게 중복 구현되어 있습니다. 향후 유지보수 시 두 파일을 동시에 수정해야 할 수 있습니다. 공통 유틸리티 모듈로 추출하는 것을 고려해 볼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/graph/nodes/rag/retrieve_node_v2.py` around lines 20 - 55, The _cosine_sim and _mmr_select implementations are duplicated; extract them into a shared utility (e.g., a new module like emb_utils or graph.utils) and replace the local definitions in retrieve_node_v2.py and retrieve_node.py with imports from that module; ensure the shared functions keep the same signatures (_cosine_sim(query_emb, doc_emb) and _mmr_select(query_emb, doc_embs, chunks, top_k, lambda_param=0.5)), update any references, run/adjust tests and linters, and remove the duplicate local implementations to avoid divergence.app/graph/nodes/answer/smalltalk_node.py (1)
50-52: 예외 발생 시 로깅 추가를 권장합니다.현재
Exception을 catch하고 있지만 로깅 없이 조용히 실패 처리됩니다. 디버깅 및 운영 모니터링을 위해 예외 정보를 trace에 기록하거나 로깅하는 것이 좋습니다.♻️ 예외 로깅 추가 제안
+ import logging + logger = logging.getLogger(__name__) + check_result = "good" try: answer = llm.chat(messages, max_tokens=100) - except Exception: + except Exception as e: + logger.warning("smalltalk LLM call failed: %s", e) answer = "" check_result = "bad"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/graph/nodes/answer/smalltalk_node.py` around lines 50 - 52, The except block in smalltalk_node.py currently swallows all exceptions (except Exception:) and sets answer/ check_result without logging; modify the exception handler in the function containing that block to log the exception details (use the module logger.exception(...) or logging.exception(...) to capture message and stack trace) before setting answer = "" and check_result = "bad", so failures are recorded for debugging and monitoring.app/graph/nodes/tool/event_node.py (1)
5-5: TODO 항목에 대해 도움을 드릴 수 있습니다.이벤트 DB 테이블 구현 시 이 노드의 내부 로직을 교체해야 합니다. 이벤트 조회 및 LLM 기반 답변 생성 로직 구현에 도움이 필요하시면 말씀해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/graph/nodes/tool/event_node.py` at line 5, The TODO comment indicates this node is a placeholder and must be replaced with real event DB access and LLM response generation; update the EventNode implementation to query the new events table instead of the placeholder logic, implementing a function/method like get_events (or fetch_events) that uses your DB client to retrieve events by id/time/filters, and replace the placeholder response generation in methods such as handle_request or generate_answer to call the LLM with the fetched event data and return structured results; ensure you wire in the existing DB client/connection, add error handling and tests for EventNode.get_events and EventNode.generate_answer, and remove the TODO comment once the real logic is in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/graph/graph_builder.py`:
- Around line 73-75: The event_node and struct_db_node currently return
check_result="bad" in their stub implementations; update the actual
data-fetching logic inside the event_node and struct_db_node handlers so that on
successful lookup/validation they emit or return check_result="good" (preserving
existing error/bad paths), ensuring compatibility with fallback_dispatch_node
which routes to "done" only when check_result="good"; locate the implementations
referenced as event_node and struct_db_node in graph_builder.py and change their
success branch to set check_result="good" while leaving failure branches as
"bad".
In `@app/graph/nodes/classify/intent_gate.py`:
- Around line 71-75: Normalize LLM-provided ranking items with strip().lower()
and remove duplicates while preserving order before filtering against
_ALLOWED_INTENTS: transform the incoming ranking list into a normalized_dedup
list (e.g., [s.strip().lower() for s in ranking] then dedupe preserving first
occurrence), then compute valid = [r for r in normalized_dedup if r in
_ALLOWED_INTENTS], missing = [i for i in _DEFAULT_RANKING if i not in valid],
and set ranking = valid + missing; keep the existing method = "llm_partial_fix"
assignment when len(valid) < len(_ALLOWED_INTENTS). Apply the same normalization
+ dedupe step to the other place where ranking is truncated/used (the later
block that slices or selects top intents) so duplicates/case/whitespace variants
cannot cause repeated intents to be tried.
In `@app/graph/nodes/tool/struct_db_node.py`:
- Around line 19-30: The current stub in struct_db_node.py always returns
check_result="bad", which prevents the struct_db intent from ever winning in
intent_router/graph_builder; update the stub behavior so it doesn't permanently
fail: either set check_result to a neutral value (e.g., "skip" or "defer") and
add trace["struct_db"]["note"]="stub - excluded from ranking" so intent_router
can ignore it, or return check_result="good" with a user-facing placeholder
answer_text explaining DB is not available (e.g., "Temporary: DB lookup not
available") and keep trace entries as-is; modify the return object in the
struct_db block accordingly and ensure trace["_flow"] still appends "struct_db"
so routing logic can see the decision.
---
Outside diff comments:
In `@app/graph/nodes/answer/answer_generate.py`:
- Around line 108-126: When llm.chat(...) in answer_generate fails the code
currently returns an error string as answer_text which downstream nodes (like
answer_check_node) may treat as a successful response; modify the except block
in answer_generate so it sets a clear failure flag in the trace/state (e.g.,
trace["answer_generate"]["failed"] = True or state["answer_generate_failed"] =
True) and still populate answer_text with the error message or empty string,
then return that trace; ensure the failure flag is added to the trace dict built
just before return so downstream logic can detect the failure and mark the flow
as "bad".
---
Nitpick comments:
In `@app/graph/nodes/answer/smalltalk_node.py`:
- Around line 50-52: The except block in smalltalk_node.py currently swallows
all exceptions (except Exception:) and sets answer/ check_result without
logging; modify the exception handler in the function containing that block to
log the exception details (use the module logger.exception(...) or
logging.exception(...) to capture message and stack trace) before setting answer
= "" and check_result = "bad", so failures are recorded for debugging and
monitoring.
In `@app/graph/nodes/rag/retrieve_node_v2.py`:
- Around line 20-55: The _cosine_sim and _mmr_select implementations are
duplicated; extract them into a shared utility (e.g., a new module like
emb_utils or graph.utils) and replace the local definitions in
retrieve_node_v2.py and retrieve_node.py with imports from that module; ensure
the shared functions keep the same signatures (_cosine_sim(query_emb, doc_emb)
and _mmr_select(query_emb, doc_embs, chunks, top_k, lambda_param=0.5)), update
any references, run/adjust tests and linters, and remove the duplicate local
implementations to avoid divergence.
In `@app/graph/nodes/rag/translate_node.py`:
- Around line 46-49: The flow name added to trace["_flow"] ("translate_ko") is
inconsistent with the trace metadata key trace["translate"]; update either the
flow entry or the metadata key so they match (e.g., change
trace["_flow"].append("translate_ko") to append("translate") or rename
trace["translate"] to trace["translate_ko"]) in translate_node.py so the flow
trace and the node metadata key are consistent; adjust any downstream usages of
trace["translate"] or the flow name accordingly.
In `@app/graph/nodes/tool/event_node.py`:
- Line 5: The TODO comment indicates this node is a placeholder and must be
replaced with real event DB access and LLM response generation; update the
EventNode implementation to query the new events table instead of the
placeholder logic, implementing a function/method like get_events (or
fetch_events) that uses your DB client to retrieve events by id/time/filters,
and replace the placeholder response generation in methods such as
handle_request or generate_answer to call the LLM with the fetched event data
and return structured results; ensure you wire in the existing DB
client/connection, add error handling and tests for EventNode.get_events and
EventNode.generate_answer, and remove the TODO comment once the real logic is in
place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc851ee7-4208-4d3e-8750-a286234818d8
📒 Files selected for processing (25)
app/graph/graph_builder.pyapp/graph/nodes/answer/answer_check.pyapp/graph/nodes/answer/answer_compose.pyapp/graph/nodes/answer/answer_generate.pyapp/graph/nodes/answer/clarify_node.pyapp/graph/nodes/answer/direct_llm_node.pyapp/graph/nodes/answer/smalltalk_node.pyapp/graph/nodes/classify/infotype_gate.pyapp/graph/nodes/classify/intent_gate.pyapp/graph/nodes/input/normalize_node.pyapp/graph/nodes/input/stt_node.pyapp/graph/nodes/output/tts_builder.pyapp/graph/nodes/rag/context_pack.pyapp/graph/nodes/rag/query_rewrite.pyapp/graph/nodes/rag/retrieve_node.pyapp/graph/nodes/rag/retrieve_node_v2.pyapp/graph/nodes/rag/translate_node.pyapp/graph/nodes/tool/event_node.pyapp/graph/nodes/tool/map_tool_node.pyapp/graph/nodes/tool/struct_db_node.pyapp/graph/routers/answer_check_router.pyapp/graph/routers/fallback_router.pyapp/graph/routers/infotype_router.pyapp/graph/routers/intent_router.pyapp/graph/state.py
💤 Files with no reviewable changes (6)
- app/graph/nodes/answer/direct_llm_node.py
- app/graph/routers/infotype_router.py
- app/graph/nodes/rag/query_rewrite.py
- app/graph/nodes/tool/map_tool_node.py
- app/graph/nodes/answer/answer_compose.py
- app/graph/nodes/classify/infotype_gate.py
Comment on lines
+73
to
+75
| builder.add_edge("smalltalk", "fallback_dispatch") | ||
| builder.add_edge("event", "fallback_dispatch") | ||
| builder.add_edge("struct_db", "fallback_dispatch") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
app/graph/nodes/answer/smalltalk_node.py \
app/graph/nodes/tool/event_node.py \
app/graph/nodes/tool/struct_db_node.py \
app/graph/routers/fallback_router.py
do
echo "===== $f ====="
sed -n '1,220p' "$f"
echo
doneRepository: Project-Guideon/guideon-AI
Length of output: 7112
event_node와 struct_db_node는 실제 구현 시 성공 반환값에 check_result="good"을 반드시 추가하세요.
현재 상태:
smalltalk_node: ✓ 성공 시check_result="good"설정 (line 45)event_node: 현재 stub (TODO)으로 항상check_result="bad"반환 → 실제 데이터 조회 로직 추가 시 성공 케이스에서"good"설정 필요struct_db_node: 현재 stub (TODO)으로 항상check_result="bad"반환 → 실제 DB 조회 로직 추가 시 성공 케이스에서"good"설정 필요
fallback_dispatch_node의 로직은 올바릅니다: check_result="good"이면 "done", 아니면 다음 의도로 재라우팅됩니다. 따라서 두 노드를 완성할 때 필수적으로 성공 경로에 check_result="good"을 추가해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/graph/graph_builder.py` around lines 73 - 75, The event_node and
struct_db_node currently return check_result="bad" in their stub
implementations; update the actual data-fetching logic inside the event_node and
struct_db_node handlers so that on successful lookup/validation they emit or
return check_result="good" (preserving existing error/bad paths), ensuring
compatibility with fallback_dispatch_node which routes to "done" only when
check_result="good"; locate the implementations referenced as event_node and
struct_db_node in graph_builder.py and change their success branch to set
check_result="good" while leaving failure branches as "bad".
Comment on lines
+19
to
+30
| # ── stub: 데이터 없음 → check_result = "bad" ───────────────────── | ||
| trace = dict(state.get("trace") or {}) | ||
| trace["struct_db"] = {"status": "stub", "query": text} | ||
|
|
||
| return {"db_result": db_result, "trace": trace} | ||
| flow = list(trace.get("_flow") or []) | ||
| flow.append("struct_db") | ||
| trace["_flow"] = flow | ||
| trace["struct_db"] = {"status": "stub", "query": text, "site_id": site_id} | ||
|
|
||
| return { | ||
| "answer_text": "", | ||
| "check_result": "bad", | ||
| "trace": trace, | ||
| } |
There was a problem hiding this comment.
현재 stub이 실제 라우팅에 노출되어 있습니다.
intent_router와 graph_builder에서 struct_db가 실제 분기 대상이라, 이 구현처럼 항상 check_result="bad"를 반환하면 struct_db 의도는 절대 성공할 수 없습니다. 실제 DB 연결 전까지는 최소한 이 intent를 ranking에서 제외하거나, 사용자에게 임시 안내를 good으로 반환하는 방어가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/graph/nodes/tool/struct_db_node.py` around lines 19 - 30, The current
stub in struct_db_node.py always returns check_result="bad", which prevents the
struct_db intent from ever winning in intent_router/graph_builder; update the
stub behavior so it doesn't permanently fail: either set check_result to a
neutral value (e.g., "skip" or "defer") and add trace["struct_db"]["note"]="stub
- excluded from ranking" so intent_router can ignore it, or return
check_result="good" with a user-facing placeholder answer_text explaining DB is
not available (e.g., "Temporary: DB lookup not available") and keep trace
entries as-is; modify the return object in the struct_db block accordingly and
ensure trace["_flow"] still appends "struct_db" so routing logic can see the
decision.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/graph/nodes/classify/intent_gate.py (1)
10-10:_MAX_INTENTS를 설정값으로 노출하면 운영 튜닝이 훨씬 쉬워집니다.현재 값이 하드코딩이라 배포 없이 실험이 어렵습니다. 환경변수/설정으로 빼고 1~4 범위로 clamp 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/graph/nodes/classify/intent_gate.py` at line 10, Make _MAX_INTENTS configurable instead of hardcoded: expose it via a configuration or environment variable (e.g., read from os.getenv or the app config) and replace the literal _MAX_INTENTS usage with that value; when parsing, coerce/convert to int and clamp it to the allowed range 1..4 (use min/max or a clamp helper) so callers of the intent gate (symbol _MAX_INTENTS in intent_gate.py) always get a value between 1 and 4; add a sensible default of 2 if the config/ENV is missing or invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/graph/nodes/classify/intent_gate.py`:
- Around line 14-18: The docstring and function contract in intent_gate.py are
inconsistent: update the docstring to state that the node returns a top-N list
(not "4개 의도") and revise the implementation to explicitly separate and persist
both the full LLM ranking and the top-N tried ranking; specifically, add/ensure
two distinct variables/keys (ranking_full and ranking_tried) are produced and
stored in the trace/log, and make the function return only the top-N list while
keeping ranking_full in the trace for debugging; apply the same change to the
analogous block around the code referenced at lines 91-103 so both docstrings,
return values, and trace keys match (use the existing names ranking_full and
ranking_tried to locate the logic).
- Around line 83-85: The current broad except in intent_gate.py (around the
llm.chat/json.loads handling) hides root causes; replace the single "except
Exception" that sets ranking=_DEFAULT_RANKING and method="llm_fallback_default"
with multiple specific except clauses for json.JSONDecodeError,
connection/timeouts (APIConnectionError, APITimeoutError), RateLimitError, and a
general APIError, and in each branch set ranking=_DEFAULT_RANKING, a distinct
method string (e.g., "llm_fallback_invalid_json",
"llm_fallback_connection_error", "llm_fallback_rate_limit",
"llm_fallback_api_error"), and record the exception text into
trace["intent_gate"]["error"]; ensure this handling wraps the llm.chat(...) and
json.loads(...) logic where ranking, method, trace, and _DEFAULT_RANKING are
used so errors are categorized for later analysis.
---
Nitpick comments:
In `@app/graph/nodes/classify/intent_gate.py`:
- Line 10: Make _MAX_INTENTS configurable instead of hardcoded: expose it via a
configuration or environment variable (e.g., read from os.getenv or the app
config) and replace the literal _MAX_INTENTS usage with that value; when
parsing, coerce/convert to int and clamp it to the allowed range 1..4 (use
min/max or a clamp helper) so callers of the intent gate (symbol _MAX_INTENTS in
intent_gate.py) always get a value between 1 and 4; add a sensible default of 2
if the config/ENV is missing or invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9dcd4a5-f03c-4468-bac2-36104994fe91
📒 Files selected for processing (1)
app/graph/nodes/classify/intent_gate.py
Comment on lines
+14
to
+18
| """LLM-only 의도 분류 노드 팩토리. | ||
|
|
||
| 4개 의도(rag, smalltalk, event, struct_db)를 순위로 반환. | ||
| 키워드 룰 없이 LLM 한 번 호출로 분류 + 순위 결정. | ||
| """ |
There was a problem hiding this comment.
함수 계약(설명/trace/반환값) 불일치로 디버깅 혼선이 생깁니다.
설명은 “4개 의도 반환”인데 실제 반환은 top-N이고, trace에는 전체 ranking만 기록됩니다. ranking_full/ranking_tried를 분리해 명시적으로 남기고 docstring도 top-N 기준으로 맞추는 게 안전합니다.
제안 수정
- 4개 의도(rag, smalltalk, event, struct_db)를 순위로 반환.
+ 4개 의도(rag, smalltalk, event, struct_db)를 순위화하고 상위 N개를 반환.
- trace["intent_gate"] = {
- "text": text,
- "ranking": ranking,
- "method": method,
- }
-
- # 상위 N개 의도만 시도
- ranking = ranking[:_MAX_INTENTS]
+ ranking_full = ranking
+ ranking_tried = ranking_full[:_MAX_INTENTS]
+ trace["intent_gate"] = {
+ "text": text,
+ "ranking_full": ranking_full,
+ "ranking_tried": ranking_tried,
+ "method": method,
+ }
return {
- "intent_ranking": ranking,
+ "intent_ranking": ranking_tried,
"current_intent_index": 0,
"trace": trace,
}Also applies to: 91-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/graph/nodes/classify/intent_gate.py` around lines 14 - 18, The docstring
and function contract in intent_gate.py are inconsistent: update the docstring
to state that the node returns a top-N list (not "4개 의도") and revise the
implementation to explicitly separate and persist both the full LLM ranking and
the top-N tried ranking; specifically, add/ensure two distinct variables/keys
(ranking_full and ranking_tried) are produced and stored in the trace/log, and
make the function return only the top-N list while keeping ranking_full in the
trace for debugging; apply the same change to the analogous block around the
code referenced at lines 91-103 so both docstrings, return values, and trace
keys match (use the existing names ranking_full and ranking_tried to locate the
logic).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/graph/nodes/classify/intent_gate.py (1)
16-20:⚠️ Potential issue | 🟡 Minordocstring/trace가 실제로 시도한 ranking과 아직 어긋납니다.
Line 18은 4개 의도를 반환한다고 읽히지만, 실제 반환값은 Line 116-119에서 top-2입니다. 반면 Line 109의 trace에는 pre-slice 전체 ranking이 남아서, 디버깅 시 “기록된 값”과 “실제로 시도한 값”이 달라집니다.
ranking_full/ranking_tried를 분리해 남기거나, 의도대로 tried ranking만 trace에 기록하는 쪽으로 맞추는 편이 안전합니다.🛠️ 제안 수정
- 4개 의도(rag, smalltalk, event, struct_db)를 순위로 반환. + 4개 의도를 순위화하고, 실제로는 상위 N개만 반환.- trace["intent_gate"] = { - "text": text, - "ranking": ranking, - "method": method, - } - if error: - trace["intent_gate"]["error"] = error - - # 상위 N개 의도만 시도 - ranking = ranking[:_MAX_INTENTS] + ranking_full = ranking + ranking_tried = ranking_full[:_MAX_INTENTS] + trace["intent_gate"] = { + "text": text, + "ranking_full": ranking_full, + "ranking_tried": ranking_tried, + "method": method, + } + if error: + trace["intent_gate"]["error"] = error- "intent_ranking": ranking, + "intent_ranking": ranking_tried,Also applies to: 107-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/graph/nodes/classify/intent_gate.py` around lines 16 - 20, The docstring and trace disagree about returned intents: the code builds a full ranking (ranking_full) but actually returns only the top-2 (sliced) in the intent gate logic (see variables ranking_full and the slice used at return around where ranking_tried is created); update the implementation so the trace records the exact "tried" ranking (e.g., create and use ranking_tried for the sliced/top-N set) or persist both ranking_full and ranking_tried separately in the trace, and ensure the returned value and process that uses ranking_tried (or the top-N) are consistent with the docstring and trace output in the IntentGate/intent_gate functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/graph/nodes/classify/intent_gate.py`:
- Around line 64-66: Validate the LLM response before parsing and using it:
check the return of llm.chat(...) (the local variable raw) is a non-empty
string, then safely call json.loads(raw) inside a try/except, and after parsing
ensure the result is a dict before accessing .get("ranking"); if any check fails
(raw is None/empty, json.loads raises, or parsed is not a dict) set ranking =
_DEFAULT_RANKING and mark method = "fallback" so the fallback path runs; apply
the same pattern to the second occurrence handling lines around where parsed and
ranking are used (the block that currently sets parsed = json.loads(...) and
ranking = parsed.get(...)) to avoid TypeError/AttributeError propagation.
---
Duplicate comments:
In `@app/graph/nodes/classify/intent_gate.py`:
- Around line 16-20: The docstring and trace disagree about returned intents:
the code builds a full ranking (ranking_full) but actually returns only the
top-2 (sliced) in the intent gate logic (see variables ranking_full and the
slice used at return around where ranking_tried is created); update the
implementation so the trace records the exact "tried" ranking (e.g., create and
use ranking_tried for the sliced/top-N set) or persist both ranking_full and
ranking_tried separately in the trace, and ensure the returned value and process
that uses ranking_tried (or the top-N) are consistent with the docstring and
trace output in the IntentGate/intent_gate functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bce2c08-96ac-46db-bbdd-e7f369af24b3
📒 Files selected for processing (1)
app/graph/nodes/classify/intent_gate.py
Comment on lines
+64
to
+66
| raw = llm.chat(messages, max_tokens=60) | ||
| parsed = json.loads(raw) | ||
| ranking = parsed.get("ranking", _DEFAULT_RANKING) |
There was a problem hiding this comment.
LLM 응답 shape를 먼저 검증해야 fallback이 살아 있습니다.
Line 64-66은 raw가 문자열인지, json.loads() 결과가 dict인지 확인하기 전에 바로 사용합니다. 그래서 llm.chat()이 None을 돌려주면 TypeError, 모델이 []나 "rag"처럼 유효한 JSON이지만 dict가 아닌 값을 주면 AttributeError가 그대로 전파되어 fallback 경로를 못 탑니다. {}도 지금은 _DEFAULT_RANKING으로 내려가면서 method가 "llm"으로 남습니다.
🛠️ 제안 수정
try:
raw = llm.chat(messages, max_tokens=60)
+ if not isinstance(raw, str):
+ raise TypeError(f"Unexpected LLM response type: {type(raw).__name__}")
parsed = json.loads(raw)
- ranking = parsed.get("ranking", _DEFAULT_RANKING)
+ if not isinstance(parsed, dict) or "ranking" not in parsed:
+ ranking = _DEFAULT_RANKING
+ method = "llm_fallback_invalid_shape"
+ error = f"Unexpected JSON shape: {type(parsed).__name__}"
+ else:
+ ranking = parsed["ranking"]- except json.JSONDecodeError as e:
+ except (TypeError, json.JSONDecodeError) as e:
ranking = _DEFAULT_RANKING
- method = "llm_fallback_invalid_json"
+ method = "llm_fallback_invalid_response"
error = str(e)Also applies to: 86-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/graph/nodes/classify/intent_gate.py` around lines 64 - 66, Validate the
LLM response before parsing and using it: check the return of llm.chat(...) (the
local variable raw) is a non-empty string, then safely call json.loads(raw)
inside a try/except, and after parsing ensure the result is a dict before
accessing .get("ranking"); if any check fails (raw is None/empty, json.loads
raises, or parsed is not a dict) set ranking = _DEFAULT_RANKING and mark method
= "fallback" so the fallback path runs; apply the same pattern to the second
occurrence handling lines around where parsed and ranking are used (the block
that currently sets parsed = json.loads(...) and ranking = parsed.get(...)) to
avoid TypeError/AttributeError propagation.
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
2 participants
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 by CodeRabbit
Release Notes
Refactor
Bug Fixes
Chores