Fix KRX API 400 Bad Request - Add Session-Based Authentication#196
Fix KRX API 400 Bad Request - Add Session-Based Authentication#196
Conversation
…l fields Add KRX credentials to Settings class and env.example for KRX data system integration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ices/krx Implement persistent authenticated session manager for KRX data API: - KRXSessionManager with lazy httpx.AsyncClient creation - Cookie-based login flow (MDCCOMS001D1.cmd) with duplicate login handling - Session expiry detection (400/LOGOUT) with automatic re-auth - 403 rate limiting with exponential backoff (3 retries, base 5s) - Concurrent login prevention via asyncio.Lock - Graceful fallback to unauthenticated on missing/invalid credentials - Module-level singleton (_krx_session) - Refactored _fetch_krx_data and _fetch_max_working_date to delegate Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tdown lifecycle Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n management Tests cover login success/failure/duplicate, re-auth on LOGOUT, no-credentials fallback, session reuse, concurrent login lock, fetch delegation, and session close. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All 103 tests pass (test_services_krx.py + test_mcp_screen_stocks.py). Linting clean on krx.py. No regressions from KRXSessionManager refactoring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change implements a session-based authentication flow for the KRX API to resolve 400 Bad Request errors. It introduces a KRXSessionManager class for handling login, session persistence, and automatic re-authentication, alongside configuration for KRX credentials, cleanup logic in the application shutdown hook, and comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant Config as Config/Settings
participant KRXMgr as KRXSessionManager
participant KRXApi as KRX API
participant Shutdown as App Shutdown
App->>Config: Load krx_member_id, krx_password
App->>KRXMgr: Create _krx_session singleton
Note over KRXMgr: Ready (not yet connected)
App->>App: Register shutdown handler
rect rgba(100, 150, 200, 0.5)
Note over App,KRXApi: API Call Flow
App->>KRXMgr: fetch_data()
KRXMgr->>KRXMgr: _ensure_session()
alt Credentials available
KRXMgr->>KRXApi: POST /Login (CD001)
KRXApi-->>KRXMgr: Session cookie + auth state
else No credentials
KRXMgr->>KRXMgr: Proceed unauthenticated
end
KRXMgr->>KRXApi: GET /Data
alt 400 Bad Request
KRXMgr->>KRXMgr: Trigger re-authentication
KRXMgr->>KRXApi: POST /Login (retry)
KRXApi-->>KRXMgr: New session
KRXMgr->>KRXApi: GET /Data (retry)
else 403 Forbidden
KRXMgr->>KRXMgr: Exponential backoff + retry
end
KRXApi-->>KRXMgr: Data response
KRXMgr-->>App: Cached/parsed data
end
rect rgba(200, 150, 100, 0.5)
Note over Shutdown,KRXMgr: Shutdown Flow
Shutdown->>KRXMgr: close()
KRXMgr->>KRXApi: Release persistent session
KRXMgr->>KRXMgr: Reset state
KRXMgr-->>Shutdown: Cleanup complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
Code Review: Fix KRX API 400 Bad Request - Session-Based AuthenticationGood overall implementation. The 🔴 Issues1. The file 2. Mixed HTTP/HTTPS — cookies may not be transmitted The login flow acquires session cookies via HTTPS: KRX_LOGIN_URL = "https://data.krx.co.kr/comm/login/MDCCOMS001D1.cmd"
KRX_BASE_URL = "https://data.krx.co.kr/"But the actual API calls go over HTTP: KRX_API_URL = "http://data.krx.co.kr/comm/bldAttendant/getJsonData.cmd"
KRX_RESOURCE_URL = "http://data.krx.co.kr/comm/bldAttendant/executeForResourceBundle.cmd"Session cookies flagged 3. Once set to 🟡 Minor Issues4. After exhausting 403 retries, the code falls through to # Remove this line at the end of fetch_data
return []5. Timeout regression in The original 6. Unlike ✅ Good Practices
SummaryThe core logic is correct and well-structured. Please address:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_services_krx.py (1)
1013-1014: Consider adding@pytest.mark.unitmarker for test categorization.Per coding guidelines, tests should use markers (
@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow) for categorization. The existing tests in this file don't have explicit markers either, so this is consistent with the current pattern but worth noting for future improvements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_services_krx.py` around lines 1013 - 1014, Add the pytest unit marker to the async test by decorating the test_session_manager_login_success coroutine with `@pytest.mark.unit` (i.e., add the `@pytest.mark.unit` decorator directly above the existing `@pytest.mark.asyncio` on the test_session_manager_login_success function) so it is categorized as a unit test; ensure pytest is imported if not already.app/services/krx.py (1)
129-197: Consider suppressing lint warnings for KRX API parameter names.The parameter names
mktId,trdDd, andidxIndClssCdmatch the actual KRX API parameter names, which improves code clarity when debugging API calls. The SonarCloud warnings about snake_case can be addressed with a targeted suppression comment if desired.Regarding cognitive complexity (21 vs allowed 15): the method handles distinct concerns (session management, 400/LOGOUT re-auth, 403 rate limiting). Consider extracting helper methods only if this becomes harder to maintain.
💡 Optional: Suppress parameter naming warnings
async def fetch_data( self, bld: str, - mktId: str | None = None, - trdDd: str | None = None, - idxIndClssCd: str | None = None, + mktId: str | None = None, # noqa: N803 - matches KRX API param + trdDd: str | None = None, # noqa: N803 - matches KRX API param + idxIndClssCd: str | None = None, # noqa: N803 - matches KRX API param ) -> list[dict[str, Any]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/krx.py` around lines 129 - 197, The Sonar/flake8 naming warnings come from using KRX API parameter names (mktId, trdDd, idxIndClssCd) in fetch_data; suppress the naming rule for this function by adding a targeted lint suppression comment (e.g., append "# noqa: N802" or the project's configured rule ID) to the fetch_data definition line so the params can remain as-is, and leave the method logic (session handling, 400/LOGOUT re-auth, 403 backoff) unchanged unless future complexity warrants extracting helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_services_krx.py`:
- Around line 1200-1210: Remove the unused assignment to original_login which
causes a lint error: delete the line "original_login = manager._login" in the
test where you replace manager._login with the async counting_login; keep the
counting_login closure (which references login_call_count and sets
manager._authenticated) and the reassignment "manager._login = counting_login"
as-is so the test behavior is unchanged.
---
Nitpick comments:
In `@app/services/krx.py`:
- Around line 129-197: The Sonar/flake8 naming warnings come from using KRX API
parameter names (mktId, trdDd, idxIndClssCd) in fetch_data; suppress the naming
rule for this function by adding a targeted lint suppression comment (e.g.,
append "# noqa: N802" or the project's configured rule ID) to the fetch_data
definition line so the params can remain as-is, and leave the method logic
(session handling, 400/LOGOUT re-auth, 403 backoff) unchanged unless future
complexity warrants extracting helpers.
In `@tests/test_services_krx.py`:
- Around line 1013-1014: Add the pytest unit marker to the async test by
decorating the test_session_manager_login_success coroutine with
`@pytest.mark.unit` (i.e., add the `@pytest.mark.unit` decorator directly above the
existing `@pytest.mark.asyncio` on the test_session_manager_login_success
function) so it is categorized as a unit test; ensure pytest is imported if not
already.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a68c4f8c-3f77-405e-b4d1-84851bb2f23d
📒 Files selected for processing (6)
.auto-claude/specs/001-fix-github-issue-195-auto-trader/implementation_plan.jsonapp/core/config.pyapp/main.pyapp/services/krx.pyenv.exampletests/test_services_krx.py
| login_call_count = 0 | ||
| original_login = manager._login | ||
|
|
||
| async def counting_login(): | ||
| nonlocal login_call_count | ||
| login_call_count += 1 | ||
| # Simulate slow login | ||
| await asyncio.sleep(0.01) | ||
| manager._authenticated = True | ||
|
|
||
| manager._login = counting_login |
There was a problem hiding this comment.
Remove unused variable original_login - pipeline failure.
The variable original_login is assigned but never used, causing a Ruff F841 lint error and pipeline failure.
🔧 Proposed fix
login_call_count = 0
- original_login = manager._login
async def counting_login():📝 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.
| login_call_count = 0 | |
| original_login = manager._login | |
| async def counting_login(): | |
| nonlocal login_call_count | |
| login_call_count += 1 | |
| # Simulate slow login | |
| await asyncio.sleep(0.01) | |
| manager._authenticated = True | |
| manager._login = counting_login | |
| login_call_count = 0 | |
| async def counting_login(): | |
| nonlocal login_call_count | |
| login_call_count += 1 | |
| # Simulate slow login | |
| await asyncio.sleep(0.01) | |
| manager._authenticated = True | |
| manager._login = counting_login |
🧰 Tools
🪛 GitHub Actions: Test
[error] 1201-1201: F841 Local variable original_login is assigned to but never used. Remove assignment to unused variable original_login.
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 1201-1201: Remove the unused local variable "original_login".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_services_krx.py` around lines 1200 - 1210, Remove the unused
assignment to original_login which causes a lint error: delete the line
"original_login = manager._login" in the test where you replace manager._login
with the async counting_login; keep the counting_login closure (which references
login_call_count and sets manager._authenticated) and the reassignment
"manager._login = counting_login" as-is so the test behavior is unchanged.
2 participants
The
screen_stocksMCP tool fails when called withmarket=kr, returning a400 Bad Requesterror from the Korea Exchange (KRX) data API (data.krx.co.kr/comm/bldAttendant/getJsonData.cmd). Starting December 27, 2025, KRX enforced session-based authentication for its data marketplace, breaking all unauthenticated POST requests. The current code inapp/services/krx.pycreates a freshhttpx.AsyncClientper request without any session/cookie management or login flow, which now results in400 Bad Requestresponses (with"LOGOUT"body). This fix requires implementing a persistent authenticated session manager for KRX API calls.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes