feat: simplify trade profile schema and add MCP tools#193
Conversation
📝 WalkthroughWalkthroughRemoves broker_account_id from trade-profile schema and models, adds Alembic migration to apply the change, introduces MCP trade-profile tooling (get/set) and KR symbol utilities, deletes legacy seeding script and an integration test, and adds unit tests for the new tooling. Changes
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Registry as Tool Registry
participant Handler as TradeProfile Handler
participant DB as Database (SQLAlchemy)
participant Logger as ProfileChangeLog
Client->>Registry: register_trade_profile_tools(mcp)
Registry->>Handler: apply `@mcp.tool` to get/set handlers
Client->>Handler: get_asset_profile(params)
Handler->>DB: open session & query AssetProfile
DB-->>Handler: rows
Handler->>Handler: serialize results
Handler-->>Client: {data, count, tier_rule_params?}
Client->>Handler: set_asset_profile(payload)
Handler->>Handler: normalize inputs, determine instrument_type
Handler->>Handler: _apply_profile_rules(profile)
Handler->>DB: upsert AssetProfile / TierRuleParam
DB-->>Handler: commit success
Handler->>Logger: insert ProfileChangeLog entry
Logger-->>Handler: log stored
Handler-->>Client: {action, profile_data}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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)
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 |
PR Review: feat/trade-profile-simplifyOverall this is a clean, well-structured PR with good test coverage. The schema simplification is a clear improvement and the MCP tooling is nicely layered. A few issues worth addressing before merging: Bugs / Correctness1. In if profile_value == "exit":
if requested_buy_allowed is True:
raise ValueError("profile=exit requires buy_allowed=False")
if requested_sell_mode is not None and requested_sell_mode != "any":
raise ValueError("profile=exit requires sell_mode='any'")
return False, "any"If an existing profile has 2. In existing = existing_result.scalar_one_or_none()If a symbol somehow matches more than one candidate pair (edge case, but possible in theory if data is inconsistent), this raises 3. Korean company name auto-detection fails silently on create
Code Quality4. Non-standard Alembic revision ID revision: str = "a1b2c3d4e5f6"This is a sequential/patterned ID rather than a randomly generated one. Alembic generates random 12-char hex IDs (e.g. 5. Double def _session_factory() -> async_sessionmaker[AsyncSession]:
return cast(async_sessionmaker[AsyncSession], cast(object, AsyncSessionLocal))The 6. def normalize_kr_symbol(symbol_input: str) -> str:
...
mapped = KR_SYMBOLS.get(candidate)
if mapped is None:
raise ValueError(f"KR symbol mapping missing for input: {symbol_input}")
return mappedThis means adding any new Korean stock to the system requires a code change to 7. Serialization duplication between Both functions serialize overlapping fields of Test Coverage Gaps8. No test for The 9. No test for the ambiguous-symbol The path where 10. stmt = mock_session.execute.await_args.args[0]
compiled = stmt.compile()
assert InstrumentType.equity_kr in compiled.params.values()Asserting on compiled SQL params is brittle — it couples the test to SQLAlchemy's internal representation. Prefer testing observable behavior (e.g., check that Minor Nits
Summary
The core changes (schema simplification + MCP tooling) are solid. The main items to address before merging are #2 (possible unhandled exception), #3 (silent misclassification of KR names), and #9 (missing test for auto-detect flow in |
![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
🧹 Nitpick comments (1)
tests/test_mcp_trade_profile_tools.py (1)
42-55: Add unsupportedmarket_typeerror-path tests for contract completeness.You covered positive behavior well; please also pin the unsupported-market path (e.g.,
"jp") for bothget_asset_profileandset_asset_profileso market-mapping errors remain stable.Based on learnings: When adding new MCP behavior, update both positive test cases and unsupported-market/error-path tests to maintain comprehensive coverage.
Also applies to: 62-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_trade_profile_tools.py` around lines 42 - 55, Add negative-path unit tests that assert unsupported market_type values return the expected error response: create async tests mirroring test_get_asset_profile_filters_by_market_type but call get_asset_profile(market_type="jp") and assert the response indicates an unsupported-market error (e.g., success False or specific error payload your API returns) and likewise add a parallel test for set_asset_profile(market_type="jp") to cover the error path; reuse the same AsyncMock/MagicMock session setup pattern and patching of _session_factory so the tests follow the positive-case structure but validate the unsupported-market error behavior for both get_asset_profile and set_asset_profile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@alembic/versions/a1b2c3d4e5f6_simplify_trade_profile_schema.py`:
- Around line 13-44: In upgrade(), before calling op.create_index for
"uq_asset_profiles_user_symbol_instrument" and
"uq_market_filters_user_instrument_filter", add a preflight duplicate check
using op.get_bind() to query asset_profiles grouped by (user_id, symbol,
instrument_type) and market_filters grouped by (user_id, instrument_type,
filter_name) to detect rows having count>1; if duplicates exist either
deterministically merge/choose a single row (e.g., keep the latest/first by id
or updated_at) and delete others, or raise a clear MigrationError so the
migration stops, ensuring the cleanup runs prior to creating the unique indexes
to prevent index creation failure.
In `@app/core/kr_symbols.py`:
- Around line 22-24: The lookup uses exact key matching on KR_SYMBOLS with
variable candidate, causing lowercase aliases like "naver" to miss; normalize
candidate before lookup (e.g., use candidate = candidate.casefold() or .upper()
consistently) and ensure KR_SYMBOLS keys are compared using the same
normalization so KR_SYMBOLS.get(candidate) succeeds; keep the existing
ValueError on missing mapping if lookup still returns None. Reference symbols:
KR_SYMBOLS, candidate, symbol_input.
In `@app/mcp_server/tooling/trade_profile_tools.py`:
- Around line 158-160: The symbol is only stripped in get_asset_profile before
building the filter, so queries like "1234" or "NAVER" won’t match normalized
stored symbols for market_type="kr"; update get_asset_profile to fully normalize
the symbol (use the same normalization/mapping used when storing symbols—e.g.,
call the existing symbol-normalization helper or mapping used elsewhere) and
assign it to normalized_symbol before constructing the filter, and apply the
same change in the second occurrence around lines 171-172 so both filter builds
use the normalized form; keep the existing calls to _to_instrument_type and
_normalize_profile as they are.
- Around line 20-24: Currently _to_instrument_type silently returns None when
normalize_market(market_type) yields None, allowing callers like
get_asset_profile and set_asset_profile to treat invalid filters as unset;
change _to_instrument_type to raise a clear exception instead of returning None:
if normalize_market(market_type) is None, raise ValueError (or a
project-standard validation error) containing the original market_type value and
a short message that it is invalid, otherwise return InstrumentType(normalized);
update callers if needed to let the error propagate or handle it explicitly.
In `@tests/test_mcp_trade_profile_tools.py`:
- Around line 24-152: Add a module-level pytest marker to classify these tests
as unit tests: import pytest at the top of the file and set pytestmark =
pytest.mark.unit so all tests in this module (e.g.
test_get_asset_profile_returns_empty_when_no_profiles,
test_set_asset_profile_create_requires_tier_and_profile,
test_set_asset_profile_auto_detect_kr_symbol, etc.) are marked; place the import
and pytestmark near the other imports so the test suite with --strict-markers
recognizes this module as unit tests.
---
Nitpick comments:
In `@tests/test_mcp_trade_profile_tools.py`:
- Around line 42-55: Add negative-path unit tests that assert unsupported
market_type values return the expected error response: create async tests
mirroring test_get_asset_profile_filters_by_market_type but call
get_asset_profile(market_type="jp") and assert the response indicates an
unsupported-market error (e.g., success False or specific error payload your API
returns) and likewise add a parallel test for
set_asset_profile(market_type="jp") to cover the error path; reuse the same
AsyncMock/MagicMock session setup pattern and patching of _session_factory so
the tests follow the positive-case structure but validate the unsupported-market
error behavior for both get_asset_profile and set_asset_profile.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignorealembic/versions/a1b2c3d4e5f6_simplify_trade_profile_schema.pyapp/core/kr_symbols.pyapp/mcp_server/__init__.pyapp/mcp_server/tooling/__init__.pyapp/mcp_server/tooling/registry.pyapp/mcp_server/tooling/trade_profile_registration.pyapp/mcp_server/tooling/trade_profile_tools.pyapp/models/trade_profile.pyscripts/seed_trade_profiles.pytests/integration/test_trade_profile_persistence.pytests/models/test_trade_profile.pytests/test_mcp_tool_registration.pytests/test_mcp_trade_profile_tools.py
💤 Files with no reviewable changes (2)
- tests/integration/test_trade_profile_persistence.py
- scripts/seed_trade_profiles.py
| def upgrade() -> None: | ||
| op.execute("DROP INDEX IF EXISTS uq_asset_profiles_with_broker") | ||
| op.execute("DROP INDEX IF EXISTS uq_asset_profiles_without_broker") | ||
| op.execute("DROP INDEX IF EXISTS uq_market_filters_with_broker") | ||
| op.execute("DROP INDEX IF EXISTS uq_market_filters_without_broker") | ||
|
|
||
| op.drop_constraint( | ||
| op.f("fk_asset_profiles_broker_account_id_broker_accounts"), | ||
| "asset_profiles", | ||
| type_="foreignkey", | ||
| ) | ||
| op.drop_constraint( | ||
| op.f("fk_market_filters_broker_account_id_broker_accounts"), | ||
| "market_filters", | ||
| type_="foreignkey", | ||
| ) | ||
|
|
||
| op.drop_column("asset_profiles", "broker_account_id") | ||
| op.drop_column("market_filters", "broker_account_id") | ||
|
|
||
| op.create_index( | ||
| "uq_asset_profiles_user_symbol_instrument", | ||
| "asset_profiles", | ||
| ["user_id", "symbol", "instrument_type"], | ||
| unique=True, | ||
| ) | ||
| op.create_index( | ||
| "uq_market_filters_user_instrument_filter", | ||
| "market_filters", | ||
| ["user_id", "instrument_type", "filter_name"], | ||
| unique=True, | ||
| ) |
There was a problem hiding this comment.
Guard against pre-existing duplicates before creating new unique indexes.
This migration can fail on live data if multiple broker-scoped rows collapse to the same new key. Add an explicit preflight duplicate check (or deterministic merge) before creating the new unique indexes.
🛡️ Suggested preflight guard
def upgrade() -> None:
+ bind = op.get_bind()
+
+ duplicate_asset = bind.execute(
+ sa.text(
+ """
+ SELECT 1
+ FROM asset_profiles
+ GROUP BY user_id, symbol, instrument_type
+ HAVING COUNT(*) > 1
+ LIMIT 1
+ """
+ )
+ ).first()
+ if duplicate_asset is not None:
+ raise RuntimeError(
+ "Cannot apply migration: duplicate asset_profiles rows exist for "
+ "(user_id, symbol, instrument_type). Deduplicate first."
+ )
+
+ duplicate_filter = bind.execute(
+ sa.text(
+ """
+ SELECT 1
+ FROM market_filters
+ GROUP BY user_id, instrument_type, filter_name
+ HAVING COUNT(*) > 1
+ LIMIT 1
+ """
+ )
+ ).first()
+ if duplicate_filter is not None:
+ raise RuntimeError(
+ "Cannot apply migration: duplicate market_filters rows exist for "
+ "(user_id, instrument_type, filter_name). Deduplicate first."
+ )
+
op.execute("DROP INDEX IF EXISTS uq_asset_profiles_with_broker")
op.execute("DROP INDEX IF EXISTS uq_asset_profiles_without_broker")
op.execute("DROP INDEX IF EXISTS uq_market_filters_with_broker")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@alembic/versions/a1b2c3d4e5f6_simplify_trade_profile_schema.py` around lines
13 - 44, In upgrade(), before calling op.create_index for
"uq_asset_profiles_user_symbol_instrument" and
"uq_market_filters_user_instrument_filter", add a preflight duplicate check
using op.get_bind() to query asset_profiles grouped by (user_id, symbol,
instrument_type) and market_filters grouped by (user_id, instrument_type,
filter_name) to detect rows having count>1; if duplicates exist either
deterministically merge/choose a single row (e.g., keep the latest/first by id
or updated_at) and delete others, or raise a clear MigrationError so the
migration stops, ensuring the cleanup runs prior to creating the unique indexes
to prevent index creation failure.
| mapped = KR_SYMBOLS.get(candidate) | ||
| if mapped is None: | ||
| raise ValueError(f"KR symbol mapping missing for input: {symbol_input}") |
There was a problem hiding this comment.
Handle English alias lookup case-insensitively.
Line 22 performs an exact key lookup, so inputs like naver fail even though NAVER is mapped.
🔧 Proposed fix
- mapped = KR_SYMBOLS.get(candidate)
+ mapped = KR_SYMBOLS.get(candidate)
+ if mapped is None:
+ mapped = KR_SYMBOLS.get(candidate.upper())
if mapped is None:
raise ValueError(f"KR symbol mapping missing for input: {symbol_input}")📝 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.
| mapped = KR_SYMBOLS.get(candidate) | |
| if mapped is None: | |
| raise ValueError(f"KR symbol mapping missing for input: {symbol_input}") | |
| mapped = KR_SYMBOLS.get(candidate) | |
| if mapped is None: | |
| mapped = KR_SYMBOLS.get(candidate.upper()) | |
| if mapped is None: | |
| raise ValueError(f"KR symbol mapping missing for input: {symbol_input}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/core/kr_symbols.py` around lines 22 - 24, The lookup uses exact key
matching on KR_SYMBOLS with variable candidate, causing lowercase aliases like
"naver" to miss; normalize candidate before lookup (e.g., use candidate =
candidate.casefold() or .upper() consistently) and ensure KR_SYMBOLS keys are
compared using the same normalization so KR_SYMBOLS.get(candidate) succeeds;
keep the existing ValueError on missing mapping if lookup still returns None.
Reference symbols: KR_SYMBOLS, candidate, symbol_input.
| instrument_type = _to_instrument_type(market_type) | ||
| normalized_profile = _normalize_profile(profile) | ||
| normalized_symbol = symbol.strip() if symbol is not None else None |
There was a problem hiding this comment.
Normalize symbol in get_asset_profile before building the filter.
Line 160 uses raw strip() only, so KR queries like 1234 or NAVER won’t match normalized stored symbols (001234, mapped code) even when market_type="kr" is provided.
🔧 Proposed fix
- normalized_symbol = symbol.strip() if symbol is not None else None
+ normalized_symbol: str | None = None
+ if symbol is not None:
+ candidate = symbol.strip()
+ if candidate:
+ normalized_symbol = (
+ _normalize_symbol_for_instrument(candidate, instrument_type)
+ if instrument_type is not None
+ else candidate
+ )Also applies to: 171-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/mcp_server/tooling/trade_profile_tools.py` around lines 158 - 160, The
symbol is only stripped in get_asset_profile before building the filter, so
queries like "1234" or "NAVER" won’t match normalized stored symbols for
market_type="kr"; update get_asset_profile to fully normalize the symbol (use
the same normalization/mapping used when storing symbols—e.g., call the existing
symbol-normalization helper or mapping used elsewhere) and assign it to
normalized_symbol before constructing the filter, and apply the same change in
the second occurrence around lines 171-172 so both filter builds use the
normalized form; keep the existing calls to _to_instrument_type and
_normalize_profile as they are.
| @pytest.mark.asyncio | ||
| async def test_get_asset_profile_returns_empty_when_no_profiles() -> None: | ||
| mock_session = AsyncMock() | ||
| mock_result = MagicMock() | ||
| mock_result.scalars.return_value.all.return_value = [] | ||
| mock_session.execute.return_value = mock_result | ||
|
|
||
| session_factory = MagicMock(return_value=_build_session_cm(mock_session)) | ||
| with patch( | ||
| "app.mcp_server.tooling.trade_profile_tools._session_factory", | ||
| return_value=session_factory, | ||
| ): | ||
| result = await get_asset_profile() | ||
|
|
||
| assert result == {"success": True, "data": [], "count": 0} | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_asset_profile_filters_by_market_type() -> None: | ||
| mock_session = AsyncMock() | ||
| mock_result = MagicMock() | ||
| mock_result.scalars.return_value.all.return_value = [] | ||
| mock_session.execute.return_value = mock_result | ||
|
|
||
| session_factory = MagicMock(return_value=_build_session_cm(mock_session)) | ||
| with patch( | ||
| "app.mcp_server.tooling.trade_profile_tools._session_factory", | ||
| return_value=session_factory, | ||
| ): | ||
| result = await get_asset_profile(market_type="kr") | ||
|
|
||
| assert result == {"success": True, "data": [], "count": 0} | ||
| stmt = mock_session.execute.await_args.args[0] | ||
| compiled = stmt.compile() | ||
| assert InstrumentType.equity_kr in compiled.params.values() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_set_asset_profile_create_requires_tier_and_profile() -> None: | ||
| mock_session = MagicMock() | ||
| mock_session.execute = AsyncMock( | ||
| return_value=SimpleNamespace(scalar_one_or_none=lambda: None) | ||
| ) | ||
| tx_cm = AsyncMock() | ||
| tx_cm.__aenter__.return_value = None | ||
| tx_cm.__aexit__.return_value = None | ||
| mock_session.begin = MagicMock(return_value=tx_cm) | ||
|
|
||
| session_factory = MagicMock(return_value=_build_session_cm(mock_session)) | ||
| with patch( | ||
| "app.mcp_server.tooling.trade_profile_tools._session_factory", | ||
| return_value=session_factory, | ||
| ): | ||
| missing_tier = await set_asset_profile(symbol="AAPL") | ||
| missing_profile = await set_asset_profile(symbol="AAPL", tier=2) | ||
|
|
||
| assert missing_tier == { | ||
| "success": False, | ||
| "error": "tier is required for new profile", | ||
| } | ||
| assert missing_profile == { | ||
| "success": False, | ||
| "error": "profile is required for new profile", | ||
| } | ||
|
|
||
|
|
||
| def test_set_asset_profile_exit_forces_buy_allowed_false() -> None: | ||
| buy_allowed, sell_mode = _apply_profile_rules( | ||
| profile_value="exit", | ||
| buy_allowed_value=True, | ||
| sell_mode_value="any", | ||
| requested_buy_allowed=None, | ||
| requested_sell_mode=None, | ||
| ) | ||
|
|
||
| assert buy_allowed is False | ||
| assert sell_mode == "any" | ||
|
|
||
|
|
||
| def test_set_asset_profile_exit_rejects_buy_allowed_true() -> None: | ||
| with pytest.raises(ValueError, match="profile=exit requires buy_allowed=False"): | ||
| _apply_profile_rules( | ||
| profile_value="exit", | ||
| buy_allowed_value=True, | ||
| sell_mode_value="any", | ||
| requested_buy_allowed=True, | ||
| requested_sell_mode=None, | ||
| ) | ||
|
|
||
|
|
||
| def test_set_asset_profile_hold_only_forces_sell_mode_rebalance() -> None: | ||
| buy_allowed, sell_mode = _apply_profile_rules( | ||
| profile_value="hold_only", | ||
| buy_allowed_value=True, | ||
| sell_mode_value="any", | ||
| requested_buy_allowed=None, | ||
| requested_sell_mode=None, | ||
| ) | ||
|
|
||
| assert buy_allowed is True | ||
| assert sell_mode == "rebalance_only" | ||
|
|
||
|
|
||
| def test_set_asset_profile_hold_only_rejects_invalid_sell_mode() -> None: | ||
| with pytest.raises( | ||
| ValueError, | ||
| match="profile=hold_only requires sell_mode='rebalance_only'", | ||
| ): | ||
| _apply_profile_rules( | ||
| profile_value="hold_only", | ||
| buy_allowed_value=True, | ||
| sell_mode_value="any", | ||
| requested_buy_allowed=None, | ||
| requested_sell_mode="any", | ||
| ) | ||
|
|
||
|
|
||
| def test_set_asset_profile_auto_detect_kr_symbol() -> None: | ||
| instrument_type, normalized_symbol = _auto_detect_for_create("5930") | ||
|
|
||
| assert instrument_type == InstrumentType.equity_kr | ||
| assert normalized_symbol == "005930" | ||
|
|
||
|
|
||
| def test_set_asset_profile_auto_detect_crypto_symbol() -> None: | ||
| instrument_type, normalized_symbol = _auto_detect_for_create("KRW-BTC") | ||
|
|
||
| assert instrument_type == InstrumentType.crypto | ||
| assert normalized_symbol == "KRW-BTC" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for existing markers in the test file
rg -n '(^pytestmark\s*=)|(`@pytest`\.mark\.(unit|integration|slow))' tests/test_mcp_trade_profile_tools.py
# Check if markers are registered in pyproject.toml
echo "=== Checking pytest marker registration ==="
rg -A 20 '\[tool\.pytest\.ini_options\]' pyproject.toml | head -40Repository: mgh3326/auto_trader
Length of output: 708
🏁 Script executed:
# Check the actual file to see current state
head -30 tests/test_mcp_trade_profile_tools.py
# Also check if any markers are used anywhere in the file
echo "=== All pytest markers in file ==="
rg '@pytest\.' tests/test_mcp_trade_profile_tools.pyRepository: mgh3326/auto_trader
Length of output: 980
🏁 Script executed:
# Count total tests in the file to understand scope
rg -c 'def test_' tests/test_mcp_trade_profile_tools.py
# Verify the exact line count of the file
wc -l tests/test_mcp_trade_profile_tools.py
# Double-check that strict-markers is enabled
rg 'strict-markers' pyproject.tomlRepository: mgh3326/auto_trader
Length of output: 130
🏁 Script executed:
# Verify if there are similar test files that DO have unit markers for comparison
echo "=== Checking marker patterns in other test files ==="
rg -l 'pytestmark = pytest.mark.unit' tests/ | head -5
# Also check tests with `@pytest.mark.unit` at function level
echo "=== Function-level unit markers ==="
rg -B 2 '@pytest.mark.unit' tests/test_*.py | head -20Repository: mgh3326/auto_trader
Length of output: 800
🏁 Script executed:
# Check if there are any integration tests mixed into other test files using function-level markers
echo "=== Checking for mixed markers in test files ==="
rg -l '@pytest.mark.integration' tests/test_*.py | head -3
# Sample one file to see if it uses both unit and integration markers
echo "=== Sample: test_trading_integration.py ==="
rg '@pytest.mark\.(unit|integration|slow)' tests/test_trading_integration.py | sort | uniq -cRepository: mgh3326/auto_trader
Length of output: 207
🏁 Script executed:
# Final check: Run pytest with strict-markers to see if it would actually fail
echo "=== Checking if strict-markers would catch this ==="
cd /tmp && python3 << 'EOF'
# Simulate what pytest strict-markers checks
# A test without a unit/integration/slow marker would fail with --strict-markers
markers_registered = {'unit', 'integration', 'slow', 'asyncio'}
test_markers_in_file = {'asyncio'} # Only `@pytest.mark.asyncio` is present
category_markers = {'unit', 'integration', 'slow'}
has_category_marker = any(m in test_markers_in_file for m in category_markers)
if not has_category_marker:
print("❌ VIOLATION: No category marker (`@pytest.mark.unit/integration/slow`) found")
print(f" Tests have only: {test_markers_in_file}")
print(f" Required one of: {category_markers}")
else:
print("✓ Has category marker")
EOFRepository: mgh3326/auto_trader
Length of output: 273
Add required pytest category markers for this new test module.
All tests in this file are missing the registered category markers (unit, integration, or slow) required by the test suite. With --strict-markers enabled in pyproject.toml, this will cause test runs to fail. Add a module-level marker to categorize all tests here as unit tests:
from __future__ import annotations
from types import SimpleNamespace
from unittest.mock import AsyncMock, MagicMock, patch
import pytest
+
+pytestmark = pytest.mark.unit
from app.mcp_server.tooling.trade_profile_tools import (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mcp_trade_profile_tools.py` around lines 24 - 152, Add a
module-level pytest marker to classify these tests as unit tests: import pytest
at the top of the file and set pytestmark = pytest.mark.unit so all tests in
this module (e.g. test_get_asset_profile_returns_empty_when_no_profiles,
test_set_asset_profile_create_requires_tier_and_profile,
test_set_asset_profile_auto_detect_kr_symbol, etc.) are marked; place the import
and pytestmark near the other imports so the test suite with --strict-markers
recognizes this module as unit tests.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
==========================================
+ Coverage 63.81% 63.94% +0.12%
==========================================
Files 181 184 +3
Lines 22574 22788 +214
==========================================
+ Hits 14405 14571 +166
- Misses 8169 8217 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PR Review: feat: simplify trade profile schema and add MCP toolsOverall this is a clean and well-structured PR. The schema simplification is a meaningful reduction in complexity, the new MCP tools follow existing patterns, and the test coverage is solid. A few issues worth addressing before merging: Bugs / Correctness Issues1. Symbol filter in # trade_profile_tools.py:456
normalized_symbol = symbol.strip() if symbol is not None else NoneIf a caller passes a Korean company name (e.g., 2. # Lines 331-335 and 338-342 are byte-for-byte identical
def _normalize_profile(value: str | None) -> str | None: ...
def _normalize_sell_mode(value: str | None) -> str | None: ...These can be collapsed into one Design Concerns3. KR_SYMBOLS hardcoded mapping vs. DB universe
4. Alembic revision ID looks manually crafted revision: str = "a1b2c3d4e5f6"This looks like a hand-typed placeholder rather than an Alembic-generated ID. It works, but it's a pattern mismatch from the rest of 5. Broad exception swallowing in except Exception as exc:
return {"success": False, "error": str(exc)}This silently catches connection errors, ORM failures, etc., returning them as normal-looking tool errors. Unexpected exceptions should at minimum be logged (or re-raised) so they surface in monitoring rather than appearing as benign tool misuse. 6. def _session_factory() -> async_sessionmaker[AsyncSession]:
return cast(async_sessionmaker[AsyncSession], cast(object, AsyncSessionLocal))This pattern exists to silence a type checker complaint, but it's obscuring a real issue. The same factory pattern used in other tools in the codebase may have a cleaner approach. At minimum, a comment explaining why the double-cast is needed would help future readers. Test Coverage Gaps7. The 8. Symbol auto-detection path (no The Minor / Style9. Missing blank line before _VALID_PROFILES = frozenset({...})
def _validate_tier(tier: int | None) -> None: # ← only one blank line before thisPEP 8 requires two blank lines between top-level definitions. Ruff may not catch this in all configurations. Summary
The schema migration and deletion of the seed script are clean. The MCP tool structure follows the existing registration pattern well. Resolving the symbol normalization bug (#1) and the exception swallowing (#5) before merge would be my top priorities. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
tests/test_mcp_trade_profile_tools.py (1)
7-8:⚠️ Potential issue | 🟠 MajorAdd the required pytest category marker (
unit) at module level.This module is still missing
unit/integration/slowcategorization under strict marker policy.As per coding guidelines `tests/test_*.py`: Use test markers (`@pytest.mark.unit`, `@pytest.mark.integration`, `@pytest.mark.slow`) to categorize tests appropriately.🔧 Suggested fix
import pytest +pytestmark = pytest.mark.unit + from app.mcp_server.tooling.trade_profile_tools import (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_trade_profile_tools.py` around lines 7 - 8, Add the module-level pytest marker for the test file by declaring the pytestmark variable and assigning it pytest.mark.unit so the whole module is categorized as unit tests; since pytest is already imported in this file, add a single module-level statement like pytestmark = pytest.mark.unit near the top (e.g., directly after the import) to satisfy the strict marker policy.app/mcp_server/tooling/trade_profile_tools.py (1)
177-189:⚠️ Potential issue | 🟠 MajorNormalize
symbolbefore filtering profiles.Line [177] only applies
strip(). This misses normalized stored symbols (e.g., KR code/name normalization), so valid lookups can return empty results even when data exists.🔧 Suggested fix
- normalized_symbol = symbol.strip() if symbol is not None else None + normalized_symbol: str | None = None + if symbol is not None: + candidate = symbol.strip() + if candidate: + normalized_symbol = ( + _normalize_symbol_for_instrument(candidate, instrument_type) + if instrument_type is not None + else candidate + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mcp_server/tooling/trade_profile_tools.py` around lines 177 - 189, The code currently only strips whitespace when building normalized_symbol (symbol.strip()), which fails to match stored normalized symbols (e.g., KR code/name normalization); update the normalized_symbol assignment to apply the same normalization routine used when persisting symbols (e.g., call the existing normalize_symbol/normalize_name helper or implement the same normalization logic) before adding the filter on AssetProfile.symbol so lookups use the consistent normalized form; change the line that sets normalized_symbol and keep the subsequent filter using AssetProfile.symbol unchanged.
🤖 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/mcp_server/tooling/trade_profile_tools.py`:
- Line 35: Ruff formatting is failing for this module due to style issues around
the constant _VALID_PROFILES; run ruff and/or black auto-fixes (e.g. ruff --fix
. && black .) on the file and commit the changes so the file conforms to project
formatting, ensuring the _VALID_PROFILES definition remains a
frozenset({"aggressive", "balanced", "conservative", "exit", "hold_only"}) and
that imports/whitespace/newline conventions are corrected.
In `@tests/test_mcp_tool_registration.py`:
- Around line 1-4: Add a pytest marker to classify this test module (e.g.,
`@pytest.mark.unit` or `@pytest.mark.integration`) by importing pytest if not
already and applying the marker at module level or to the test functions in
tests/test_mcp_tool_registration.py; locate the top of the file where pytest is
imported and add the appropriate marker annotation to the test definitions or a
module-level marker variable so the suite adheres to the strict marker policy
(choose unit/integration/slow based on the test's scope).
In `@tests/test_mcp_trade_profile_tools.py`:
- Around line 275-277: The test module tests/test_mcp_trade_profile_tools.py
fails Ruff formatting; normalize the file formatting (fix tuple/line breaks and
any lint issues) so it passes CI — for example reformat the attributes tuple
used in the for loop that iterates over ("id", "symbol", "instrument_type",
"tier", "profile", "sector", "tags", "max_position_pct", "buy_allowed",
"sell_mode", "note", "updated_by", "created_at", "updated_at") and any
surrounding lines, then run ruff (and black if configured) to auto-fix style
violations before committing.
---
Duplicate comments:
In `@app/mcp_server/tooling/trade_profile_tools.py`:
- Around line 177-189: The code currently only strips whitespace when building
normalized_symbol (symbol.strip()), which fails to match stored normalized
symbols (e.g., KR code/name normalization); update the normalized_symbol
assignment to apply the same normalization routine used when persisting symbols
(e.g., call the existing normalize_symbol/normalize_name helper or implement the
same normalization logic) before adding the filter on AssetProfile.symbol so
lookups use the consistent normalized form; change the line that sets
normalized_symbol and keep the subsequent filter using AssetProfile.symbol
unchanged.
In `@tests/test_mcp_trade_profile_tools.py`:
- Around line 7-8: Add the module-level pytest marker for the test file by
declaring the pytestmark variable and assigning it pytest.mark.unit so the whole
module is categorized as unit tests; since pytest is already imported in this
file, add a single module-level statement like pytestmark = pytest.mark.unit
near the top (e.g., directly after the import) to satisfy the strict marker
policy.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/mcp_server/tooling/__init__.pyapp/mcp_server/tooling/trade_profile_registration.pyapp/mcp_server/tooling/trade_profile_tools.pytests/test_mcp_tool_registration.pytests/test_mcp_trade_profile_tools.py
| def _normalize_sell_mode(value: str | None) -> str | None: | ||
| if value is None: | ||
| return None | ||
| normalized = value.strip().lower() | ||
| return normalized or None | ||
|
|
There was a problem hiding this comment.
Validate sell_mode at input time instead of deferring to DB constraints.
Line [59] only normalizes text. Invalid values can reach persistence and fail as generic DB errors, which weakens tool contract/error clarity.
🔧 Suggested fix
_VALID_PROFILES = frozenset({"aggressive", "balanced", "conservative", "exit", "hold_only"})
+_VALID_SELL_MODES = frozenset({"any", "rebalance_only", "none"})
@@
def _normalize_sell_mode(value: str | None) -> str | None:
@@
return normalized or None
+
+
+def _validate_sell_mode(value: str | None) -> None:
+ if value is None:
+ return
+ normalized = value.strip().lower()
+ if normalized and normalized not in _VALID_SELL_MODES:
+ raise ValueError("sell_mode must be one of: any, rebalance_only, none")
@@
_validate_tier(tier)
_validate_profile(profile)
+ _validate_sell_mode(sell_mode)
requested_profile = _normalize_profile(profile)
requested_sell_mode = _normalize_sell_mode(sell_mode)Also applies to: 255-256
| from typing import Any, cast | ||
|
|
||
| import pytest | ||
|
|
There was a problem hiding this comment.
Add a pytest category marker for this test module.
This file lacks a unit/integration/slow marker classification, which conflicts with strict marker policy.
🔧 Suggested fix
from typing import Any, cast
import pytest
+
+pytestmark = pytest.mark.unit📝 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.
| from typing import Any, cast | |
| import pytest | |
| from typing import Any, cast | |
| import pytest | |
| pytestmark = pytest.mark.unit |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mcp_tool_registration.py` around lines 1 - 4, Add a pytest marker
to classify this test module (e.g., `@pytest.mark.unit` or
`@pytest.mark.integration`) by importing pytest if not already and applying the
marker at module level or to the test functions in
tests/test_mcp_tool_registration.py; locate the top of the file where pytest is
imported and add the appropriate marker annotation to the test definitions or a
module-level marker variable so the suite adheres to the strict marker policy
(choose unit/integration/slow based on the test's scope).
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/mcp_server/tooling/trade_profile_tools.py (2)
63-67:⚠️ Potential issue | 🟠 MajorValidate
sell_modeat input time, not at DB constraint time.
_normalize_sell_modecurrently normalizes but never validates. Invalid values can reach persistence and fail as generic DB errors instead of returning a clear tool-level validation error.🔧 Proposed fix
_VALID_PROFILES = frozenset( {"aggressive", "balanced", "conservative", "exit", "hold_only"} ) +_VALID_SELL_MODES = frozenset({"any", "rebalance_only", "none"}) @@ def _normalize_sell_mode(value: str | None) -> str | None: if value is None: return None normalized = value.strip().lower() return normalized or None + + +def _validate_sell_mode(value: str | None) -> None: + if value is None: + return + normalized = value.strip().lower() + if normalized and normalized not in _VALID_SELL_MODES: + raise ValueError("sell_mode must be one of: any, rebalance_only, none") @@ _validate_tier(tier) _validate_profile(profile) + _validate_sell_mode(sell_mode) requested_profile = _normalize_profile(profile) requested_sell_mode = _normalize_sell_mode(sell_mode)Also applies to: 256-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mcp_server/tooling/trade_profile_tools.py` around lines 63 - 67, The _normalize_sell_mode function only normalizes but does not validate allowed values, so invalid sell_mode strings reach the DB; modify _normalize_sell_mode to check the normalized value against an explicit allowed set (e.g., ALLOWED_SELL_MODES constant) and, if not None and not in that set, raise a clear tool-level exception (ValueError or your project ValidationError) with a descriptive message; apply the same change to the duplicate implementation referenced at the other location (lines 256-260) so all input paths validate before persistence.
181-193:⚠️ Potential issue | 🟠 MajorNormalize
symbolinget_asset_profilebefore building the filter.Line 181 only trims whitespace. That can miss rows stored in normalized form (notably KR symbols and case-normalized symbols), so valid lookups may return no matches.
🔧 Proposed fix
- normalized_symbol = symbol.strip() if symbol is not None else None + normalized_symbol: str | None = None + if symbol is not None: + candidate = symbol.strip() + if candidate: + normalized_symbol = ( + _normalize_symbol_for_instrument(candidate, instrument_type) + if instrument_type is not None + else candidate + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/mcp_server/tooling/trade_profile_tools.py` around lines 181 - 193, The symbol is only whitespace-trimmed (normalized_symbol = symbol.strip()) in get_asset_profile which can miss rows stored in normalized/case-normalized form; before building filters, apply the same normalization used for stored symbols (the same logic/helper used for normalized_profile) to symbol (e.g., call the existing normalize function or replicate its normalization rules) and assign that to normalized_symbol so the AssetProfile.symbol filter compares against the normalized form; update references to normalized_symbol in the condition that appends AssetProfile.symbol == normalized_symbol.
🤖 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/mcp_server/tooling/trade_profile_tools.py`:
- Around line 269-313: The current set_asset_profile flow does a race-prone
SELECT then conditional INSERT/UPDATE on AssetProfile (keys: user_id, symbol,
instrument_type), so replace the read-then-write with an atomic upsert or a
protected transaction: use a single database upsert (INSERT ... ON CONFLICT
(user_id, symbol, instrument_type) DO UPDATE ...) executed via SQLAlchemy core
(insert().on_conflict_do_update or equivalent) to create-or-update the row using
instrument_type and normalized_symbol, or wrap the logic in a transaction and
acquire a row-level lock (SELECT ... FOR UPDATE) when you must read first;
update code paths that reference set_asset_profile, AssetProfile, MCP_USER_ID,
instrument_type, and normalized_symbol to use the atomic upsert or locked
transaction to eliminate the race.
---
Duplicate comments:
In `@app/mcp_server/tooling/trade_profile_tools.py`:
- Around line 63-67: The _normalize_sell_mode function only normalizes but does
not validate allowed values, so invalid sell_mode strings reach the DB; modify
_normalize_sell_mode to check the normalized value against an explicit allowed
set (e.g., ALLOWED_SELL_MODES constant) and, if not None and not in that set,
raise a clear tool-level exception (ValueError or your project ValidationError)
with a descriptive message; apply the same change to the duplicate
implementation referenced at the other location (lines 256-260) so all input
paths validate before persistence.
- Around line 181-193: The symbol is only whitespace-trimmed (normalized_symbol
= symbol.strip()) in get_asset_profile which can miss rows stored in
normalized/case-normalized form; before building filters, apply the same
normalization used for stored symbols (the same logic/helper used for
normalized_profile) to symbol (e.g., call the existing normalize function or
replicate its normalization rules) and assign that to normalized_symbol so the
AssetProfile.symbol filter compares against the normalized form; update
references to normalized_symbol in the condition that appends
AssetProfile.symbol == normalized_symbol.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/mcp_server/tooling/trade_profile_tools.pytests/test_mcp_trade_profile_tools.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_mcp_trade_profile_tools.py
| if explicit_instrument_type is not None: | ||
| instrument_type = explicit_instrument_type | ||
| normalized_symbol = _normalize_symbol_for_instrument( | ||
| symbol_input, instrument_type | ||
| ) | ||
| existing_stmt = select(AssetProfile).where( | ||
| AssetProfile.user_id == MCP_USER_ID, | ||
| AssetProfile.symbol == normalized_symbol, | ||
| AssetProfile.instrument_type == instrument_type, | ||
| ) | ||
| existing_result = await db.execute(existing_stmt) | ||
| existing = existing_result.scalar_one_or_none() | ||
| else: | ||
| candidate_pairs: list[tuple[InstrumentType, str]] = [] | ||
| if symbol_input.isdigit() and len(symbol_input) <= 6: | ||
| candidate_pairs.append( | ||
| (InstrumentType.equity_kr, symbol_input.zfill(6)) | ||
| ) | ||
| upper_symbol = symbol_input.upper() | ||
| if upper_symbol.startswith("KRW-"): | ||
| candidate_pairs.append((InstrumentType.crypto, upper_symbol)) | ||
| if not candidate_pairs: | ||
| candidate_pairs.append((InstrumentType.equity_us, upper_symbol)) | ||
|
|
||
| predicates = [ | ||
| and_( | ||
| AssetProfile.instrument_type == candidate_type, | ||
| AssetProfile.symbol == candidate_symbol, | ||
| ) | ||
| for candidate_type, candidate_symbol in candidate_pairs | ||
| ] | ||
| existing_stmt = select(AssetProfile).where( | ||
| AssetProfile.user_id == MCP_USER_ID, | ||
| or_(*predicates), | ||
| ) | ||
| existing_result = await db.execute(existing_stmt) | ||
| existing = existing_result.scalar_one_or_none() | ||
|
|
||
| if existing is not None: | ||
| instrument_type = existing.instrument_type | ||
| normalized_symbol = existing.symbol | ||
| else: | ||
| raise ValueError("market_type is required for new profile") | ||
|
|
||
| if existing is None: |
There was a problem hiding this comment.
set_asset_profile read-then-write flow is race-prone for concurrent calls.
This path does SELECT → conditional INSERT/UPDATE. Two concurrent requests for the same (user_id, symbol, instrument_type) can both see no row and one will fail on the unique index, which breaks reliable upsert behavior.
Also applies to: 331-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/mcp_server/tooling/trade_profile_tools.py` around lines 269 - 313, The
current set_asset_profile flow does a race-prone SELECT then conditional
INSERT/UPDATE on AssetProfile (keys: user_id, symbol, instrument_type), so
replace the read-then-write with an atomic upsert or a protected transaction:
use a single database upsert (INSERT ... ON CONFLICT (user_id, symbol,
instrument_type) DO UPDATE ...) executed via SQLAlchemy core
(insert().on_conflict_do_update or equivalent) to create-or-update the row using
instrument_type and normalized_symbol, or wrap the logic in a transaction and
acquire a row-level lock (SELECT ... FOR UPDATE) when you must read first;
update code paths that reference set_asset_profile, AssetProfile, MCP_USER_ID,
instrument_type, and normalized_symbol to use the atomic upsert or locked
transaction to eliminate the race.
PR Review: feat: simplify trade profile schema and add MCP toolsOverall this is a clean and well-structured PR. The schema simplification is a good architectural decision, and the new MCP tools follow the existing tooling patterns well. Below are issues ranging from bugs to nits. Bug / Correctness Issues1. Cannot clear optional fields once setIn the update path of 2. Hard-coded KR symbol dictionary conflicts with the project single source of truth
More critically, Recommendation: Fall back to querying 3. Symbol auto-detection silently misroutes Korean company namesWhen Design / Behavioral Concerns4. Silent auto-correction with no feedback to the caller
5. Non-standard migration revision ID
6. Downgrade migration is schema-only
Code Quality7. Double-cast in
|
2 participants
Summary
broker_account_idfromasset_profilesandmarket_filterstables, making trade profiles account-agnostic. Alembic migration replaces partial unique indexes with simple ones.get_asset_profile(read with filters) andset_asset_profile(upsert with auto-correction rules forexit/hold_onlyprofiles). Externalmarket_typeparam (kr/us/crypto) mapped internally toinstrument_typeenum.app/core/kr_symbols.pywith symbol dict and normalization helper.scripts/seed_trade_profiles.pyand its integration tests — profiles are now managed exclusively viaset_asset_profile.Changes
Test Plan
uv run ruff check app/ tests/— passeduv run ty checkon all changed app files — passeduv run pytest --no-cov -q— 1635 passed, 4 skippedSummary by CodeRabbit
New Features
Refactor
Chores
Tests