feat(ai): Single-turn Router → Supervisor 패턴 전환 (#51) - #63
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Intent StrEnum 및 intent 필드 삭제 - next_worker, workers_called 필드 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- SUPERVISOR_PROMPT 및 decide_next_worker() 메서드 추가 - supervisor 노드 생성 (workers_called 누적, FINISH 시 미포함) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- classify_intent.py 삭제 - builder.py: START → supervisor → [worker] → supervisor 순환 구조로 재작성 - FINISH 신호 시 generate_answer로 이동 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- AgentChatResponse: intent 필드 → workersCalled(list[str])로 교체
- routes.py: result["intent"] → result.get("workers_called", [])
- generate_answer(): Intent 열거형 비교 → workers_called in 검사로 교체
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- test_classify_intent.py 삭제 - test_supervisor.py 신규 생성 (decide_next_worker, supervisor 노드 단위 테스트 7개) - test_agent_chat.py 재작성: route_as() → supervisor mock 방식, body["intent"] → body["workersCalled"] Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR replaces intent-based routing with a supervisor loop that selects workers through ChangesSupervisor Pattern Migration
pgvector Query Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.22.1)docs/08_API_SPEC.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) phases/supervisor-pattern/phase3-builder-refactor.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0) phases/supervisor-pattern/phase2-supervisor-node.mdmarkdownlint-cli2 v0.22.1 (markdownlint v0.40.0)
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: 11
🧹 Nitpick comments (1)
phases/supervisor-pattern/phase5-tests.md (1)
178-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
GENERAL_CHATto the test migration matrix.Update the replacement list to include
route_as(monkeypatch, "GENERAL_CHAT")and correspondingworkersCalledassertion guidance for parity with the current worker set.🤖 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 `@phases/supervisor-pattern/phase5-tests.md` around lines 178 - 183, The test migration matrix in phase5-tests.md is missing the GENERAL_CHAT case, so update the replacement list used for route_as and workersCalled parity to include route_as(monkeypatch, "GENERAL_CHAT") alongside the existing Intent-to-string migrations. Make sure the guidance references the same test migration matrix section and keeps the worker assertion mapping aligned with the current worker set so GENERAL_CHAT is covered consistently with PROPERTY_SEARCH, LEGAL_CONSULT, PRICE_ANALYSIS, and SAFETY_ANALYSIS.
🤖 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 `@backend-ai/app/clients/llm_client.py`:
- Around line 176-200: generate_answer() is returning on the first worker match,
which discards multi-worker results and breaks the expected supervisor
aggregation behavior. Update the logic in llm_client.py inside generate_answer()
to collect and combine outputs from all relevant workers in workers_called
instead of short-circuiting on the first match, preserving both GENERAL_CHAT and
domain-specific responses when present. Use the existing helper methods
_generate_live_general_chat_answer, _generate_live_legal_answer,
_generate_live_analysis_answer, generate_legal_answer, and AnalysisAnswerService
to build each part, then merge them into a single final response.
- Around line 99-127: The decide_next_worker flow is turning every
transport/JSON/validation failure into FINISH, which prematurely routes the
graph to generate_answer even when no worker has run yet. Update
decide_next_worker in llm_client.py so only explicit invalid next_worker values
or repeated workers_called are mapped to FINISH, while first-call
request/parsing failures are handled by a non-terminal fallback path that keeps
routing available for another worker or retry. Keep the validation around
parsed["next_worker"] and workers_called, but avoid swallowing initial
httpx.HTTPError/json parsing errors into FINISH.
In `@backend-ai/app/clients/supabase_client.py`:
- Line 80: The timeout-setting SQL in SupabaseClient now uses a single literal
SQL string via execute, so the stale tests still expecting a parameterized call
must be updated. Adjust the FakeCursor.execute signatures in
test_legal_retriever and test_legal_ingestion_upsert to accept the new
one-argument form, and change their assertions to match the literal SET LOCAL
statement_timeout SQL emitted by the client. Ensure the expectations line up
with the behavior in SupabaseClient's statement_timeout handling.
In `@backend-ai/tests/eval/baseline_result.json`:
- Around line 8-14: The eval baseline artifact is stale and no longer matches
the current dataset split used by eval_set.py, so update baseline_result.json to
reflect the regenerated evaluation results. Recompute the baseline against the
current eval set, then replace the summary fields (including single_intent_total
and complex_total) and any affected case expectations so baseline-vs-supervisor
comparisons stay consistent.
In `@backend-ai/tests/eval/run_baseline.py`:
- Around line 148-155: The token average calculations in run_baseline.py are
using the wrong denominator, which can skew the reported values when some
results have missing or zero token counts. Update avg_prompt_tokens and
avg_completion_tokens to divide by each metric’s own count of non-zero entries
rather than len(all_tokens), and keep the existing zero-safe fallback behavior
in the aggregation logic around results and all_tokens.
- Around line 31-33: The baseline script still imports legacy symbols removed by
the supervisor migration, so fix the imports in run_baseline.py to use the new
supervisor-based types instead of RouteDecision and Intent. Update the code that
references these symbols to the current equivalents used by the migrated
graph/state modules, and ensure any eval logic continues to work without relying
on deleted app.graph.* definitions.
In `@backend-ai/tests/test_agent_chat.py`:
- Line 65: The single-worker tests are only checking that the expected worker
appears in workersCalled, which allows extra unintended workers to slip through.
Update the assertions in the affected test cases in test_agent_chat to verify
the exact workersCalled value for each single-worker scenario instead of using
inclusion checks, so the tests fail when over-routing occurs.
In `@backend-ai/tests/test_supervisor.py`:
- Around line 19-46: The `LLMClient.decide_next_worker` tests are mocking the
method under test, so they never exercise the real routing logic. Update the
tests in `test_supervisor.py` to patch only the external LLM dependency used by
`LLMClient.decide_next_worker`, then assert the actual method handles JSON
parsing, the valid-worker allowlist, repeated-worker fallback, and
failure-to-`FINISH` behavior. Use the existing `LLMClient` symbol and the
related tests around `test_decide_next_worker_*` to locate the cases and rewrite
them so they validate real behavior instead of patched return values.
In `@phases/supervisor-pattern/phase2-supervisor-node.md`:
- Line 79: The decide_next_worker allowlist is missing GENERAL_CHAT, causing the
phase2 supervisor contract to conflict with the supported worker set. Update the
valid set in the supervisor pattern doc’s decide_next_worker section to include
GENERAL_CHAT alongside the existing worker names so the documented routing
options match the current contract.
In `@phases/supervisor-pattern/phase3-builder-refactor.md`:
- Around line 13-15: The phase instructions are missing the GENERAL_CHAT route,
so update the supervisor graph documentation to include GENERAL_CHAT alongside
the existing worker mappings and loop-back edges. In the phase3 builder refactor
doc, adjust the checklist items and any related route/edge descriptions so
`GENERAL_CHAT` is explicitly covered in the same supervisor-to-worker and
worker-to-supervisor flow as the other nodes, ensuring the documented graph
matches the current supervisor behavior.
In `@phases/supervisor-pattern/phase4-api-and-answer.md`:
- Around line 52-78: The generate_answer dispatch logic is missing the
GENERAL_CHAT branch, so the phase documentation does not match the current
worker set. Update the generate_answer flow to include a GENERAL_CHAT condition
alongside the existing LEGAL_CONSULT, PRICE_ANALYSIS, SAFETY_ANALYSIS, and
PROPERTY_SEARCH handling, and make sure the fallback/default response still
applies only when no worker-specific branch matches.
---
Nitpick comments:
In `@phases/supervisor-pattern/phase5-tests.md`:
- Around line 178-183: The test migration matrix in phase5-tests.md is missing
the GENERAL_CHAT case, so update the replacement list used for route_as and
workersCalled parity to include route_as(monkeypatch, "GENERAL_CHAT") alongside
the existing Intent-to-string migrations. Make sure the guidance references the
same test migration matrix section and keeps the worker assertion mapping
aligned with the current worker set so GENERAL_CHAT is covered consistently with
PROPERTY_SEARCH, LEGAL_CONSULT, PRICE_ANALYSIS, and SAFETY_ANALYSIS.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40143b00-ee91-43d2-81c9-4dc6f35a2106
📒 Files selected for processing (26)
backend-ai/app/api/routes.pybackend-ai/app/api/schemas.pybackend-ai/app/clients/llm_client.pybackend-ai/app/clients/supabase_client.pybackend-ai/app/graph/builder.pybackend-ai/app/graph/nodes/classify_intent.pybackend-ai/app/graph/nodes/general_chat.pybackend-ai/app/graph/nodes/supervisor.pybackend-ai/app/graph/state.pybackend-ai/tests/eval/__init__.pybackend-ai/tests/eval/baseline_result.jsonbackend-ai/tests/eval/eval_set.pybackend-ai/tests/eval/run_baseline.pybackend-ai/tests/eval/run_supervisor.pybackend-ai/tests/eval/supervisor_result.jsonbackend-ai/tests/test_agent_chat.pybackend-ai/tests/test_classify_intent.pybackend-ai/tests/test_supervisor.pydocs/02_ARCHITECTURE.mddocs/03_ADR.mddocs/08_API_SPEC.mdphases/supervisor-pattern/phase1-state-schema.mdphases/supervisor-pattern/phase2-supervisor-node.mdphases/supervisor-pattern/phase3-builder-refactor.mdphases/supervisor-pattern/phase4-api-and-answer.mdphases/supervisor-pattern/phase5-tests.md
💤 Files with no reviewable changes (2)
- backend-ai/app/graph/nodes/classify_intent.py
- backend-ai/tests/test_classify_intent.py
| "set local statement_timeout = %s", | ||
| (self.statement_timeout_ms,), | ||
| ) | ||
| cursor.execute(f"SET LOCAL statement_timeout = {int(self.statement_timeout_ms)}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd test_legal_retriever.py backend-ai/tests --exec sed -n '120,185p' {}
fd supabase_client.py backend-ai/app/clients --exec sed -n '74,90p' {}Repository: ssafy-salman/salmanhae
Length of output: 3102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for all execute() test doubles and related expectations in backend-ai tests
rg -n "def execute\(self, sql|execute\(sql, params\)|SET LOCAL statement_timeout|statement_timeout" backend-ai/tests
# Show the full affected test file for context around the existing assertion
sed -n '1,260p' backend-ai/tests/test_legal_retriever.py
# Show the current client implementation around the timeout calls
sed -n '70,95p' backend-ai/app/clients/supabase_client.pyRepository: ssafy-salman/salmanhae
Length of output: 10386
Update the timeout test expectations. backend-ai/tests/test_legal_retriever.py still mocks FakeCursor.execute(self, sql, params) and asserts the old parameterized SET LOCAL statement_timeout call; it needs to accept the new one-argument SQL call and assert the literal timeout statement instead. The same stale expectation also appears in backend-ai/tests/test_legal_ingestion_upsert.py.
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 80-80: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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 `@backend-ai/app/clients/supabase_client.py` at line 80, The timeout-setting
SQL in SupabaseClient now uses a single literal SQL string via execute, so the
stale tests still expecting a parameterized call must be updated. Adjust the
FakeCursor.execute signatures in test_legal_retriever and
test_legal_ingestion_upsert to accept the new one-argument form, and change
their assertions to match the literal SET LOCAL statement_timeout SQL emitted by
the client. Ensure the expectations line up with the behavior in
SupabaseClient's statement_timeout handling.
workers_called가 비어있는 첫 번째 supervisor 호출에서 LLM 장애가 발생하면 FINISH를 반환해 빈 응답으로 generate_answer에 도달하는 문제를 수정. 첫 호출 실패 시 PROPERTY_SEARCH로 라우팅해 최소한의 응답을 보장한다. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
첫 번째 매칭 워커에서 return해 나머지 워커 결과가 버려지는 문제 수정. parts 리스트로 전체 워커 결과를 수집한 뒤 합쳐서 반환하도록 변경. PRICE_ANALYSIS + SAFETY_ANALYSIS는 analysis_cards를 공유하므로 _generate_live_analysis_answer 한 번 호출로 처리. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
supervisor 전환으로 삭제된 RouteDecision, Intent 임포트 제거. _classify_with_usage를 raw JSON 파싱 방식으로 교체해 ImportError 해소. prompt/completion 토큰 평균을 각자 non-zero 건수로 나누도록 수정. baseline_result.json의 eval_set 불일치(21/17 → 22/16) 및 case[21] 수정. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
자기 자신을 mock하던 decide_next_worker 테스트 3개를 http_post 주입 방식으로 교체해 JSON 파싱·valid 체크·중복 워커 guard 실제 동작을 검증. 첫 호출 실패 → PROPERTY_SEARCH, 이후 실패 → FINISH 동작도 테스트 추가. test_agent_chat.py 단일 워커 assertion을 in 포함 체크에서 == 정확 일치로 변경해 over-routing 감지 가능하도록 수정. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
phase2: valid set에 GENERAL_CHAT 추가, 첫 호출 실패 시 PROPERTY_SEARCH 폴백 반영. phase3: 라우팅 매핑 및 루프백 엣지에 general_chat 추가. phase4: generate_answer 스니펫에 GENERAL_CHAT 브랜치 및 복합 의도 parts 수집 방식 반영. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
첫 호출 실패 시 FINISH가 아닌 PROPERTY_SEARCH를 반환하는 실제 구현에 맞게 체크리스트 항목 수정. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
변경 내용
1. 핵심 구조 전환
AgentState에서Intent열거형 제거 →next_worker,workers_called필드로 교체classify_intent노드 삭제,supervisor순환 그래프로 교체LLMClient에decide_next_worker추가 (SUPERVISOR_PROMPT기반 LLM 판단)builder.py: supervisor → 워커 → supervisor 루프 구조로 재설계2. 기능 추가
GENERAL_CHAT워커 추가 (인사·잡담·서비스 소개 처리)workers_called터미널 로깅3. API 응답 변경
intent: string→workersCalled: string[](복수 워커 반환 가능)4. 버그 수정
supabase_client.py:SET LOCAL statement_timeout파라미터 바인딩 오류 수정supabase_client.py: IVFFlatprobes=1기본값으로 벡터 검색 결과 0개 반환 문제 수정 (probes=100설정)5. 성능 측정
연결 이슈
closes #51
테스트
test_supervisor.py,test_agent_chat.py)eval 측정 결과 (38케이스 기준)
리뷰 포인트
workers_called에 이미 있는 워커를 LLM이 다시 선택하면FINISH로 강제 처리 (decide_next_worker내 guard 로직)intent(string) →workersCalled(array) — Spring Boot 연동 시 프론트엔드 파싱 확인 필요lists=100,probes=100으로 전체 탐색. 데이터가 충분히 쌓이면 probes 값을 낮춰 성능 최적화 필요Summary by CodeRabbit
workersCalledto show which steps were executed.workersCalled.