Feat/chat redis#94
Merged
Merged
Conversation
WalkthroughRedis에서 세션별 채팅 이력을 읽어 GraphState의 Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as "QA API"
participant Redis as "Redis\n(chat store)"
participant State as "GraphState"
participant Nodes as "LLM Nodes"
Client->>API: 요청 (sessionId)
API->>Redis: load_chat_history(sessionId)
Redis-->>API: 이전 메시지 목록
API->>State: 초기화 (initial_state + chat_history)
State-->>Nodes: 그래프 실행 (state 포함)
Nodes->>Nodes: build_messages(state, system, user)\n(이전 대화 포함)
Nodes-->>Client: 응답 (LLM 결과)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
시
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/qa.py (1)
92-119:⚠️ Potential issue | 🟠 Major동기 Redis 조회를 이벤트 루프에서 직접 실행 중입니다.
load_chat_history(req.sessionId)는 표준redis.Redis클라이언트로 동기 I/O를 수행하는데,await asyncio.to_thread(...)밖에서 직접 호출됩니다. Redis 연결이 느려지거나 재연결 중이면 async 핸들러 자체가 블로킹되어 같은 워커의 다른 동시 요청들이 지연됩니다.
load_chat_history호출을asyncio.to_thread()로 감싸거나 async Redis 클라이언트로 전환하세요.수정 예시
+ chat_history = await asyncio.to_thread(load_chat_history, req.sessionId) + initial_state = { "transcript": req.question, "language_code": lang2, "user_language": lang2, "site_id": req.siteId, "device_id": req.deviceId, "system_prompt": req.systemPrompt or "", "mascot_name": req.mascotName or "", "mascot_greeting": req.greetingMsg or "", "mascot_base_persona": (req.promptConfig.base_persona or "") if req.promptConfig else "", "mascot_smalltalk_style": (req.promptConfig.smalltalk_style or "") if req.promptConfig else "", "mascot_struct_db_style": (req.promptConfig.struct_db_style or "") if req.promptConfig else "", "mascot_RAG_style": (req.promptConfig.RAG_style or "") if req.promptConfig else "", "mascot_event_style": (req.promptConfig.event_node_style or "") if req.promptConfig else "", - "chat_history": load_chat_history(req.sessionId), + "chat_history": chat_history, "top_k": 5, "retry_count": 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/qa.py` around lines 92 - 119, The synchronous Redis call load_chat_history(req.sessionId) is being executed on the event loop when building initial_state, which can block; fix it by moving the Redis call off the event loop (e.g., await asyncio.to_thread(load_chat_history, req.sessionId)) or replace load_chat_history with an async Redis variant and await it, then use the returned value when constructing initial_state before calling await asyncio.to_thread(text_pipeline.graph.invoke, initial_state); reference load_chat_history, initial_state, and text_pipeline.graph.invoke to locate and update the code.
🧹 Nitpick comments (1)
app/graph/nodes/utils.py (1)
97-109: 외국어 분기에서lang_name빈값 대비가 있으면 더 안전합니다.현재
lang_name이 비어 있으면 지시문이 어색해져 출력 품질이 떨어질 수 있습니다. 기본값 보강을 권장합니다.수정 예시
else: lines = [] + effective_lang = lang_name or LANG_NAMES.get(user_language, "English") if base_prompt: lines.append( f"[Character setting (originally in Korean, for your reference only)]: {base_prompt}" ) @@ if style: - lines.append(build_foreign_style_line(style, lang_name)) + lines.append(build_foreign_style_line(style, effective_lang)) return "\n".join(lines) if lines else foreign_fallback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/graph/nodes/utils.py` around lines 97 - 109, The foreign-language branch can produce awkward directives when lang_name is empty; update the block that builds lines so that before calling build_foreign_style_line(style, lang_name) you coalesce lang_name to a safe default (e.g., "the target language" or a sensible language name) or skip/adjust the style line when lang_name is falsy; modify the code around variables base_prompt, name, greeting, style, lang_name and the call to build_foreign_style_line to use this default so the returned string (or foreign_fallback) remains well-formed even if lang_name is empty.
🤖 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/core/services/chat_history.py`:
- Around line 31-39: Current prints in chat_history (session_id, key, history,
msg content) expose PII; update the logging in the code that enumerates history
(variables: session_id, key, history, msg) to avoid printing raw content and
full session identifiers. Replace printing of session_id with a short,
non-reversible identifier (e.g., hash/hex prefix of session_id), do not print
msg["content"] — instead log only metadata such as msg.get("role") and length
(len(msg.get("content", ""))) or a fixed "[REDACTED]" token; similarly update
the exception handler to log the hashed/shortened session id and the exception
type/message only (no raw content). Use the existing symbols (session_id, key,
history, msg) to locate and change the print statements accordingly.
- Around line 25-37: The Redis-loaded payloads are not validated before being
added to history, allowing malformed records (e.g., role="system" or non-string
content) to pollute prompts; update the logic in the function that calls
_get_redis() and reads key=f"chat:{session_id}" (using CHAT_HISTORY_MAX_TURNS
and r.lrange) to validate each json.loads(item) result: ensure the parsed object
is a dict, has a role in the allowed set (e.g., "user" or "assistant" — exclude
"system" or unexpected roles), and that "content" exists and is a string; only
append validated records to history and skip or log invalid entries before
returning history.
- Around line 12-16: The Redis client created in _get_redis currently lacks
explicit network timeouts, so add socket_connect_timeout and socket_timeout to
the redis.Redis(...) constructor where _client is instantiated (alongside
REDIS_HOST and REDIS_PORT) to avoid worker threads blocking on network calls;
use sensible timeout values or existing config constants (e.g.,
SOCKET_CONNECT_TIMEOUT, SOCKET_TIMEOUT or read from env) and ensure
decode_responses=True remains set.
In `@app/graph/nodes/answer/struct_db_node.py`:
- Around line 50-61: The persona assembly in this node calls build_persona_block
but omits passing the shared mascot_base_persona fallback, causing
mascot_struct_db_style empty cases to use the hardcoded fallback; update the
build_persona_block invocation in struct_db_node (where name, style,
persona_block are set) to include the mascot_base_persona value (e.g., from
state.get("mascot_base_persona") or the common mascot fallback variable) so the
node falls back to the same base persona as other nodes rather than the
hardcoded strings.
In `@app/graph/nodes/utils.py`:
- Around line 22-27: 현재 state.get("chat_history")를 그대로 펼쳐 메시지 리스트에 넣으면 문자열, 누락
필드, 잘못된 role 등으로 downstream에서 실패할 수 있으니 history 항목을 정규화·검증해야 합니다;
state.get("chat_history")에서 꺼낸 history 요소가 dict인지 확인하고 각 항목에 'role'과 'content'
키가 있는지 검사하며 role은 {'system','user','assistant'} 중 하나로 제한하고 content를 문자열으로 강제(누락
시 빈 문자열로 대체)하거나 유효하지 않으면 해당 항목을 필터링한 뒤 system_content, user_content와 합쳐 반환하도록
변경하세요 (참조 심볼: history 변수, system_content, user_content).
---
Outside diff comments:
In `@app/api/qa.py`:
- Around line 92-119: The synchronous Redis call
load_chat_history(req.sessionId) is being executed on the event loop when
building initial_state, which can block; fix it by moving the Redis call off the
event loop (e.g., await asyncio.to_thread(load_chat_history, req.sessionId)) or
replace load_chat_history with an async Redis variant and await it, then use the
returned value when constructing initial_state before calling await
asyncio.to_thread(text_pipeline.graph.invoke, initial_state); reference
load_chat_history, initial_state, and text_pipeline.graph.invoke to locate and
update the code.
---
Nitpick comments:
In `@app/graph/nodes/utils.py`:
- Around line 97-109: The foreign-language branch can produce awkward directives
when lang_name is empty; update the block that builds lines so that before
calling build_foreign_style_line(style, lang_name) you coalesce lang_name to a
safe default (e.g., "the target language" or a sensible language name) or
skip/adjust the style line when lang_name is falsy; modify the code around
variables base_prompt, name, greeting, style, lang_name and the call to
build_foreign_style_line to use this default so the returned string (or
foreign_fallback) remains well-formed even if lang_name is empty.
🪄 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
Run ID: 6affc826-0e36-4946-80d2-9ce9cb6fefbe
📒 Files selected for processing (9)
app/api/qa.pyapp/core/services/chat_history.pyapp/graph/nodes/answer/answer_generate.pyapp/graph/nodes/answer/event_node.pyapp/graph/nodes/answer/smalltalk_node.pyapp/graph/nodes/answer/struct_db_node.pyapp/graph/nodes/utils.pyapp/graph/state.pyrequirements.txt
Comment on lines
+31
to
+39
| print(f"[ChatHistory] 조회: session_id={session_id}, key={key}, count={len(history)}") | ||
| for i, msg in enumerate(history): | ||
| role = msg.get("role", "?") | ||
| content = msg.get("content", "")[:50] # 50자까지만 출력 | ||
| print(f" [{i}] {role}: {content}{'...' if len(msg.get('content', '')) > 50 else ''}") | ||
|
|
||
| return history | ||
| except Exception as e: | ||
| print(f"[ChatHistory] 조회 실패: session_id={session_id}, {type(e).__name__}: {e}") |
There was a problem hiding this comment.
대화 본문과 session_id를 그대로 로그에 남기지 마세요.
여기는 사용자 질문/응답 일부와 session_id를 stdout에 그대로 찍고 있습니다. 운영 로그에 대화 내용이 남으면 PII/민감정보 유출 경로가 되므로, content는 제거하고 식별자도 해시나 축약값 정도로만 남기는 편이 좋습니다.
🧰 Tools
🪛 Ruff (0.15.9)
[warning] 38-38: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/core/services/chat_history.py` around lines 31 - 39, Current prints in
chat_history (session_id, key, history, msg content) expose PII; update the
logging in the code that enumerates history (variables: session_id, key,
history, msg) to avoid printing raw content and full session identifiers.
Replace printing of session_id with a short, non-reversible identifier (e.g.,
hash/hex prefix of session_id), do not print msg["content"] — instead log only
metadata such as msg.get("role") and length (len(msg.get("content", ""))) or a
fixed "[REDACTED]" token; similarly update the exception handler to log the
hashed/shortened session id and the exception type/message only (no raw
content). Use the existing symbols (session_id, key, history, msg) to locate and
change the print statements accordingly.
Contributor
Author
There was a problem hiding this comment.
현재 개발 단계이고, 추후에 완료된 후 삭제 진행하겠습니다.
There was a problem hiding this comment.
@hwangtae123, 이해했습니다! 개발 단계라면 디버그 로그가 유용할 수 있습니다. 다만, 운영 배포 전에 삭제하는 것을 잊지 않도록 코드에 TODO 주석을 남겨 두시면 추후 추적에 도움이 됩니다.
# TODO: Remove PII-containing debug logs before production deployment
print(f"[ChatHistory] 조회: session_id={session_id}, key={key}, count={len(history)}")또는 환경변수로 디버그 모드를 구분하는 방법도 간단하게 적용할 수 있습니다:
DEBUG = os.getenv("DEBUG", "false").lower() == "true"
if DEBUG:
print(f"[ChatHistory] 조회: session_id={session_id}, key={key}, count={len(history)}")
for i, msg in enumerate(history):
role = msg.get("role", "?")
content = msg.get("content", "")[:50]
print(f" [{i}] {role}: {content}{'...' if len(msg.get('content', '')) > 50 else ''}")이 방식을 사용하면 운영 환경에서는 DEBUG=false(기본값)로 로그가 출력되지 않아 별도로 코드를 삭제하지 않아도 안전하게 관리할 수 있습니다.
-타임아웃 설정 후: Redis 장애 → 1~2초 후 예외 → except에서 빈 리스트 반환 → 서버 정상 운영 -user/assistant + 문자열 content만 통과 → 안전한 데이터만 GPT에 전달
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/core/services/chat_history.py (1)
58-60: 예외 처리를 더 구체화하면 디버깅에 유리합니다.현재 모든
Exception을 포괄적으로 catch하여 resilience를 확보하는 것은 의도된 설계입니다. 다만,redis.RedisError를 명시적으로 catch하면 프로그래밍 오류(TypeError,AttributeError등)와 인프라 오류를 구분할 수 있어 디버깅 시 유용합니다.♻️ 선택적 개선안
- except Exception as e: - print(f"[ChatHistory] 조회 실패: session_id={session_id}, {type(e).__name__}: {e}") + except redis.RedisError as e: + print(f"[ChatHistory] Redis 조회 실패: session_id={session_id}, {type(e).__name__}: {e}") return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/core/services/chat_history.py` around lines 58 - 60, Change the broad except Exception block to explicitly handle redis.RedisError first and then fall back to a generic exception handler: catch redis.RedisError (from the redis package) and log the redis-specific error for infrastructure issues, then keep a separate except Exception to handle unexpected programming errors; update the handler around the block that currently contains "except Exception as e: print(f\"[ChatHistory] 조회 실패: session_id={session_id}, {type(e).__name__}: {e}\") return []" so you first "except redis.RedisError as e:" with the existing log/return behavior and then "except Exception as e:" for other errors.
🤖 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/core/services/chat_history.py`:
- Around line 26-35: load_chat_history is synchronous and blocks the event loop
when called from the async internal_qa during construction of initial_state;
either (A) change the call site in app/api/qa.py to run it off-thread using
await asyncio.to_thread(load_chat_history, req.sessionId) when building
initial_state, or (B) convert load_chat_history to an async function that uses
redis.asyncio.Redis via _get_redis (or a new async _get_redis_async) and update
callers to await load_chat_history(session_id); reference load_chat_history,
internal_qa, initial_state, _get_redis and CHAT_HISTORY_MAX_TURNS to locate the
code paths to modify.
In `@app/graph/nodes/answer/struct_db_node.py`:
- Line 26: The current lookup uses user_language directly (used in lang_name =
LANG_NAMES.get(user_language, user_language.upper())) which can raise if
user_language is None or empty; ensure defensive normalization by replacing the
raw state access (state.get("user_language", "ko")) with a normalized variable
(e.g., retrieve raw = state.get("user_language"); then set user_language = raw
if isinstance(raw, str) and raw.strip() else "ko") before using LANG_NAMES.get
or calling .upper() so LANG_NAMES.get(user_language, user_language.upper())
never receives None/empty or non-str values.
---
Nitpick comments:
In `@app/core/services/chat_history.py`:
- Around line 58-60: Change the broad except Exception block to explicitly
handle redis.RedisError first and then fall back to a generic exception handler:
catch redis.RedisError (from the redis package) and log the redis-specific error
for infrastructure issues, then keep a separate except Exception to handle
unexpected programming errors; update the handler around the block that
currently contains "except Exception as e: print(f\"[ChatHistory] 조회 실패:
session_id={session_id}, {type(e).__name__}: {e}\") return []" so you first
"except redis.RedisError as e:" with the existing log/return behavior and then
"except Exception as e:" for other errors.
🪄 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
Run ID: 36179ec6-c4f0-4788-830d-1ce368fe4a69
📒 Files selected for processing (2)
app/core/services/chat_history.pyapp/graph/nodes/answer/struct_db_node.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/core/services/chat_history.py (1)
16-22: 공유 Redis 풀은 연결 상한을 두는 편이 안전합니다.
internal_qa()요청 경로에서 이 풀을 계속 재사용하므로, 기본ConnectionPool설정을 그대로 두면 피크 시 동시 Redis 연결 수가 빠르게 늘 수 있습니다. 운영에서는max_connections를 명시해 연결 fan-out을 제한하는 편이 더 안전합니다.🔧 예시
_pool = aioredis.ConnectionPool.from_url( f"redis://{REDIS_HOST}:{REDIS_PORT}", decode_responses=True, + max_connections=50, # 예시값: 워커 수 / Redis 용량에 맞춰 조정 socket_connect_timeout=1, # 연결 타임아웃 1초 socket_timeout=2, # 읽기 타임아웃 2초 health_check_interval=30, # 30초마다 연결 상태 확인 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/core/services/chat_history.py` around lines 16 - 22, The shared Redis connection pool created in chat_history.py (_pool = aioredis.ConnectionPool.from_url(...)) lacks a max_connections cap, so when internal_qa() reuses this pool peak concurrent Redis connections can spike; update the ConnectionPool.from_url call to include a configurable max_connections (e.g., use an env var or REDIS_MAX_CONNECTIONS constant and pass max_connections=<value>) to limit fan-out and prevent unbounded connections, and ensure any documentation or default value is reasonable for your deployment.app/graph/nodes/answer/answer_generate.py (1)
107-108: 이 노드는 history를 토큰/문자 budget 기준으로 한 번 더 자르는 편이 안전합니다.
app/core/services/chat_history.py의 Line 35-Line 49는 턴 수만 제한하고 각 메시지 길이는 제한하지 않습니다. 여기서는 검색 context까지 함께 넣기 때문에, 몇 개의 긴 대화만으로도 관련 chunk가 밀리거나 모델 입력 한도에 걸릴 수 있습니다.build_messages()호출 전에 이 노드 전용 history budget을 한 번 더 적용하거나, helper 쪽에 budget 옵션을 두는 편이 안전합니다.🤖 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 107 - 108, The node currently passes the full state history into build_messages (using variables state, system_prompt, user_msg), which can exceed model input limits; before calling build_messages, apply a node-specific history budget by truncating the chat history by tokens/characters (e.g., via a new helper truncate_history_by_tokens(truncate_history, max_tokens) or by adding a budget option to the existing chat history service) and use that truncated history when assembling messages; update the call site around build_messages to use the truncated history (or add a budget parameter) and ensure the helper lives alongside app/core/services/chat_history.py so it reuses token/char counting logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/core/services/chat_history.py`:
- Around line 16-22: The shared Redis connection pool created in chat_history.py
(_pool = aioredis.ConnectionPool.from_url(...)) lacks a max_connections cap, so
when internal_qa() reuses this pool peak concurrent Redis connections can spike;
update the ConnectionPool.from_url call to include a configurable
max_connections (e.g., use an env var or REDIS_MAX_CONNECTIONS constant and pass
max_connections=<value>) to limit fan-out and prevent unbounded connections, and
ensure any documentation or default value is reasonable for your deployment.
In `@app/graph/nodes/answer/answer_generate.py`:
- Around line 107-108: The node currently passes the full state history into
build_messages (using variables state, system_prompt, user_msg), which can
exceed model input limits; before calling build_messages, apply a node-specific
history budget by truncating the chat history by tokens/characters (e.g., via a
new helper truncate_history_by_tokens(truncate_history, max_tokens) or by adding
a budget option to the existing chat history service) and use that truncated
history when assembling messages; update the call site around build_messages to
use the truncated history (or add a budget parameter) and ensure the helper
lives alongside app/core/services/chat_history.py so it reuses token/char
counting logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5822c890-7e2d-414d-95af-6274f116aa13
📒 Files selected for processing (7)
app/api/qa.pyapp/core/services/chat_history.pyapp/graph/nodes/answer/answer_generate.pyapp/graph/nodes/answer/event_node.pyapp/graph/nodes/answer/smalltalk_node.pyapp/graph/nodes/answer/struct_db_node.pyapp/graph/nodes/utils.py
✅ Files skipped from review due to trivial changes (1)
- app/api/qa.py
🚧 Files skipped from review as they are similar to previous changes (3)
- app/graph/nodes/answer/smalltalk_node.py
- app/graph/nodes/utils.py
- app/graph/nodes/answer/event_node.py
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.
Summary- #93
LangGraph에서 답변 생성 시 LLM이 chat-history를 조회 후 답변을 생성합니다.
Summary by CodeRabbit
새로운 기능
개선