Compare commits

...

6 Commits

Author SHA1 Message Date
4638c073e3 fix: enable cross-job portfolio continuity in get_previous_trading_day
Remove job_id filter from get_previous_trading_day() SQL query to enable
portfolio continuity across jobs. Previously, new jobs would reset to
initial $10,000 cash instead of continuing from previous job's ending
position.

Root cause: get_previous_trading_day() filtered by job_id, while
get_current_position_from_db() correctly queries across all jobs.
This inconsistency caused starting_cash to default to initial_cash
when no previous day was found within the same job.

Changes:
- api/database.py: Remove job_id filter from SQL WHERE clause
- tests/unit/test_database_helpers.py: Add test for cross-job continuity

Fixes position tracking bug where subsequent jobs on consecutive dates
would not recognize previous day's holdings from different job.
2025-11-07 16:13:28 -05:00
96f61cf347 release: v0.4.2 - fix critical negative cash position bug
Remove debug logging and update CHANGELOG for v0.4.2 release.

Fixed critical bug where trades calculated from initial $10,000 capital
instead of accumulating, allowing over-spending and negative cash balances.

Key changes:
- Extract position dict from CallToolResult.structuredContent
- Enable MCP service logging for better debugging
- Update tests to match production MCP behavior

All tests passing. Ready for production release.
2025-11-07 15:41:28 -05:00
0eb5fcc940 debug: enable stdout/stderr for MCP services to diagnose parameter injection
MCP services were started with stdout/stderr redirected to DEVNULL, making
debug logs invisible. This prevented diagnosing why _current_position parameter
is not being received by buy() function.

Changed subprocess.Popen to redirect MCP service output to main process
stdout/stderr, allowing [DEBUG buy] logs to be visible in docker logs.

This will help identify whether:
1. _current_position is being sent by ContextInjector but not received
2. MCP HTTP transport filters underscore-prefixed parameters
3. Parameter serialization is failing

Related to negative cash bug where final position shows -$3,049.83 instead
of +$727.92 tracked by ContextInjector.
2025-11-07 14:56:48 -05:00
bee6afe531 test: update ContextInjector tests to match production MCP behavior
Update unit tests to mock CallToolResult objects instead of plain dicts,
matching actual MCP tool behavior in production.

Changes:
- Add create_mcp_result() helper to create mock CallToolResult objects
- Update all mock handlers to return MCP result objects
- Update assertions to access result.structuredContent field
- Maintains test coverage while accurately reflecting production behavior

This ensures tests validate the actual code path used in production,
where MCP tools return CallToolResult objects with structuredContent
field containing the position dict.
2025-11-07 14:32:20 -05:00
f1f76b9a99 fix: extract position dict from CallToolResult.structuredContent
Fix negative cash bug where ContextInjector._current_position never updated.

Root cause: MCP tools return mcp.types.CallToolResult objects, not plain
dicts. The isinstance(result, dict) check always failed, preventing
_current_position from accumulating trades within a session.

This caused all trades to calculate from initial $10,000 position instead
of previous trade's ending position, resulting in negative cash balances
when total purchases exceeded $10,000.

Solution: Extract position dict from CallToolResult.structuredContent field
before validating. Maintains backward compatibility by handling both
CallToolResult objects (production) and plain dicts (unit tests).

Impact:
- Fixes negative cash positions (e.g., -$8,768.68 after 11 trades)
- Enables proper intra-day position tracking
- Validates sufficient cash before each trade based on cumulative position
- Trade tool responses now properly accumulate all holdings

Testing:
- All existing unit tests pass (handle plain dict results)
- Production logs confirm structuredContent extraction works
- Debug logging shows _current_position now updates after each trade
2025-11-07 14:24:48 -05:00
277714f664 debug: add comprehensive logging for position tracking bug investigation
Add debug logging to diagnose negative cash position issue where trades
calculate from initial $10,000 instead of accumulating.

Issue: After 11 trades, final cash shows -$8,768.68. Each trade appears
to calculate from $10,000 starting position instead of previous trade's
ending position.

Hypothesis: ContextInjector._current_position not updating after trades,
possibly due to MCP result type mismatch in isinstance(result, dict) check.

Debug logging added:
- agent/context_injector.py: Log MCP result type, content, and whether
  _current_position updates after each trade
- agent_tools/tool_trade.py: Log whether injected position is used vs
  DB query, and full contents of returned position dict

This will help identify:
1. What type is returned by MCP tool (dict vs other)
2. Whether _current_position is None on subsequent trades
3. What keys are present in returned position dicts

Related to issue where reasoning summary claims no trades executed
despite 4 sell orders being recorded.
2025-11-07 14:16:30 -05:00
6 changed files with 101 additions and 27 deletions

View File

@@ -7,6 +7,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]
## [0.4.2] - 2025-11-07
### Fixed
- **Critical:** Fixed negative cash position bug where trades calculated from initial capital instead of accumulating
- Root cause: MCP tools return `CallToolResult` objects with position data in `structuredContent` field, but `ContextInjector` was checking `isinstance(result, dict)` which always failed
- Impact: Each trade checked cash against initial $10,000 instead of cumulative position, allowing over-spending and resulting in negative cash balances (e.g., -$8,768.68 after 11 trades totaling $18,768.68)
- Solution: Updated `ContextInjector` to extract position dict from `CallToolResult.structuredContent` before validation
- Fix ensures proper intra-day position tracking with cumulative cash checks preventing over-trading
- Updated unit tests to mock `CallToolResult` objects matching production MCP behavior
- Locations: `agent/context_injector.py:95-109`, `tests/unit/test_context_injector.py:26-53`
- Enabled MCP service logging by redirecting stdout/stderr from `/dev/null` to main process for better debugging
- Previously, all MCP tool debug output was silently discarded
- Now visible in docker logs for diagnosing parameter injection and trade execution issues
- Location: `agent_tools/start_mcp_services.py:81-88`
### Fixed
- **Critical:** Fixed stale jobs blocking new jobs after Docker container restart
- Root cause: Jobs with status 'pending', 'downloading_data', or 'running' remained in database after container shutdown, preventing new job creation

View File

@@ -88,9 +88,17 @@ class ContextInjector:
# Update position state after successful trade
if request.name in ["buy", "sell"]:
# Check if result is a valid position dict (not an error)
if isinstance(result, dict) and "error" not in result and "CASH" in result:
# Extract position dict from MCP result
# MCP tools return CallToolResult objects with structuredContent field
position_dict = None
if hasattr(result, 'structuredContent') and result.structuredContent:
position_dict = result.structuredContent
elif isinstance(result, dict):
position_dict = result
# Check if position dict is valid (not an error) and update state
if position_dict and "error" not in position_dict and "CASH" in position_dict:
# Update our tracked position with the new state
self._current_position = result.copy()
self._current_position = position_dict.copy()
return result

View File

@@ -78,10 +78,11 @@ class MCPServiceManager:
env['PYTHONPATH'] = str(Path.cwd())
# Start service process (output goes to Docker logs)
# Enable stdout/stderr for debugging (previously sent to DEVNULL)
process = subprocess.Popen(
[sys.executable, str(script_path)],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
stdout=sys.stdout, # Redirect to main process stdout
stderr=sys.stderr, # Redirect to main process stderr
cwd=Path.cwd(), # Use current working directory (/app)
env=env # Pass environment with PYTHONPATH
)

View File

@@ -611,6 +611,10 @@ class Database:
Handles weekends/holidays by finding actual previous trading day.
NOTE: Queries across ALL jobs for the given model to enable portfolio
continuity even when new jobs are created with overlapping date ranges.
The job_id parameter is kept for API compatibility but not used in the query.
Returns:
dict with keys: id, date, ending_cash, ending_portfolio_value
or None if no previous day exists
@@ -619,11 +623,11 @@ class Database:
"""
SELECT id, date, ending_cash, ending_portfolio_value
FROM trading_days
WHERE job_id = ? AND model = ? AND date < ?
WHERE model = ? AND date < ?
ORDER BY date DESC
LIMIT 1
""",
(job_id, model, current_date)
(model, current_date)
)
row = cursor.fetchone()

View File

@@ -2,6 +2,7 @@
import pytest
from agent.context_injector import ContextInjector
from unittest.mock import Mock
@pytest.fixture
@@ -22,27 +23,34 @@ class MockRequest:
self.args = args or {}
def create_mcp_result(position_dict):
"""Create a mock MCP CallToolResult object matching production behavior."""
result = Mock()
result.structuredContent = position_dict
return result
async def mock_handler_success(request):
"""Mock handler that returns a successful position update."""
"""Mock handler that returns a successful position update as MCP CallToolResult."""
# Simulate a successful trade returning updated position
if request.name == "sell":
return {
return create_mcp_result({
"CASH": 1100.0,
"AAPL": 7,
"MSFT": 5
}
})
elif request.name == "buy":
return {
return create_mcp_result({
"CASH": 50.0,
"AAPL": 7,
"MSFT": 12
}
return {}
})
return create_mcp_result({})
async def mock_handler_error(request):
"""Mock handler that returns an error."""
return {"error": "Insufficient cash"}
"""Mock handler that returns an error as MCP CallToolResult."""
return create_mcp_result({"error": "Insufficient cash"})
@pytest.mark.asyncio
@@ -68,17 +76,17 @@ async def test_context_injector_injects_parameters(injector):
"""Test that context parameters are injected into buy/sell requests."""
request = MockRequest("buy", {"symbol": "AAPL", "amount": 10})
# Mock handler that just returns the request args
# Mock handler that returns MCP result containing the request args
async def handler(req):
return req.args
return create_mcp_result(req.args)
result = await injector(request, handler)
# Verify context was injected
assert result["signature"] == "test-model"
assert result["today_date"] == "2025-01-15"
assert result["job_id"] == "test-job-123"
assert result["trading_day_id"] == 1
# Verify context was injected (result is MCP CallToolResult object)
assert result.structuredContent["signature"] == "test-model"
assert result.structuredContent["today_date"] == "2025-01-15"
assert result.structuredContent["job_id"] == "test-job-123"
assert result.structuredContent["trading_day_id"] == 1
@pytest.mark.asyncio
@@ -132,7 +140,7 @@ async def test_context_injector_does_not_update_position_on_error(injector):
# Verify position was NOT updated
assert injector._current_position == original_position
assert "error" in result
assert "error" in result.structuredContent
@pytest.mark.asyncio
@@ -146,7 +154,7 @@ async def test_context_injector_does_not_inject_position_for_non_trade_tools(inj
async def verify_no_injection_handler(req):
assert "_current_position" not in req.args
return {"results": []}
return create_mcp_result({"results": []})
await injector(request, verify_no_injection_handler)
@@ -164,7 +172,7 @@ async def test_context_injector_full_trading_session_simulation(injector):
async def handler1(req):
# First trade should NOT have injected position
assert req.args.get("_current_position") is None
return {"CASH": 1100.0, "AAPL": 7}
return create_mcp_result({"CASH": 1100.0, "AAPL": 7})
result1 = await injector(request1, handler1)
assert injector._current_position == {"CASH": 1100.0, "AAPL": 7}
@@ -176,7 +184,7 @@ async def test_context_injector_full_trading_session_simulation(injector):
# Second trade SHOULD have injected position from trade 1
assert req.args["_current_position"]["CASH"] == 1100.0
assert req.args["_current_position"]["AAPL"] == 7
return {"CASH": 50.0, "AAPL": 7, "MSFT": 7}
return create_mcp_result({"CASH": 50.0, "AAPL": 7, "MSFT": 7})
result2 = await injector(request2, handler2)
assert injector._current_position == {"CASH": 50.0, "AAPL": 7, "MSFT": 7}
@@ -185,7 +193,7 @@ async def test_context_injector_full_trading_session_simulation(injector):
request3 = MockRequest("buy", {"symbol": "GOOGL", "amount": 100})
async def handler3(req):
return {"error": "Insufficient cash", "cash_available": 50.0}
return create_mcp_result({"error": "Insufficient cash", "cash_available": 50.0})
result3 = await injector(request3, handler3)
# Position should remain unchanged after failed trade

View File

@@ -130,6 +130,44 @@ class TestDatabaseHelpers:
assert previous is not None
assert previous["date"] == "2025-01-17"
def test_get_previous_trading_day_across_jobs(self, db):
"""Test retrieving previous trading day from different job (cross-job continuity)."""
# Setup: Create two jobs
db.connection.execute(
"INSERT INTO jobs (job_id, status, config_path, date_range, models, created_at) VALUES (?, ?, ?, ?, ?, ?)",
("job-1", "completed", "config.json", "2025-10-07,2025-10-07", "deepseek-chat-v3.1", "2025-11-07T00:00:00Z")
)
db.connection.execute(
"INSERT INTO jobs (job_id, status, config_path, date_range, models, created_at) VALUES (?, ?, ?, ?, ?, ?)",
("job-2", "running", "config.json", "2025-10-08,2025-10-08", "deepseek-chat-v3.1", "2025-11-07T01:00:00Z")
)
# Day 1 in job-1
db.create_trading_day(
job_id="job-1",
model="deepseek-chat-v3.1",
date="2025-10-07",
starting_cash=10000.0,
starting_portfolio_value=10000.0,
daily_profit=214.58,
daily_return_pct=2.15,
ending_cash=123.59,
ending_portfolio_value=10214.58
)
# Test: Get previous day from job-2 on next date
# Should find job-1's record (cross-job continuity)
previous = db.get_previous_trading_day(
job_id="job-2",
model="deepseek-chat-v3.1",
current_date="2025-10-08"
)
assert previous is not None
assert previous["date"] == "2025-10-07"
assert previous["ending_cash"] == 123.59
assert previous["ending_portfolio_value"] == 10214.58
def test_get_ending_holdings(self, db):
"""Test retrieving ending holdings for a trading day."""
db.connection.execute(