Enhance strategy execution with CSV logging and dashboard support

[0xghXst]

• Introduced CSV logging for strategy execution, allowing tracking of NAV, positions, and orders over time.
• Added a new FastAPI-based web dashboard for real-time monitoring of strategy performance.
• Updated strategy initialization to include parameters for enabling CSV logging and specifying log directory.
• Created a CSVLogger utility for managing log file creation and data writing.
• Implemented API endpoints to serve strategy data and logs for the dashboard.
• Added documentation for CSV logging features and dashboard usage.

[claude]

Pull Request Review: CSV Logging and Dashboard Support

Summary

This PR adds CSV logging capabilities and a web dashboard for strategy execution monitoring. The implementation is well-structured and adds valuable observability features to the trading strategies.

Code Quality & Best Practices

Strengths

• Clean architecture: CSV logger is properly separated into a utility module with clear responsibilities
• Well-documented: Good docstrings throughout the code
• Follows repository style: Consistent with existing codebase patterns
• Optional feature: Logging is opt-in via enable_csv_logging parameter
• Proper resource management: Uses context managers for file operations

Issues to Address

1. CRITICAL: Linting Failures

The PR has failing lint checks. Please run uv run ruff check and fix all issues before merging.

2. Documentation Violates CLAUDE.md Rule #2

Per CLAUDE.md line 2: "DO NOT create a new document. Purge unnecessary code and files."

• docs/CSV_LOGGING.md is a new documentation file (134 lines)
• Consider integrating this documentation into the existing README or removing if not essential

3. Configuration Management (CLAUDE.md Rule #4)

The log_dir parameter defaults to "logs" but this could be placed in code constants instead of being configurable. Per CLAUDE.md: "Single Source of Truth: DO NOT place many variables in .env file. Place them in the code instead."

• Consider hardcoding the logs directory path as a constant in the CSVLogger class

4. File I/O Performance Concerns

csv_logger.py:115-117: Opening/closing the CSV file on every log_snapshot() call could impact performance.

with open(self.filepath, "a", newline="") as f:
    writer = csv.writer(f)
    writer.writerow(row)

Recommendation: Consider buffering writes or keeping the file handle open during strategy execution, flushing periodically.

5. Error Handling Gaps

• api.py:89-110: No exception handling when reading CSV files or parsing data
• api.py:46-50: Silent fallback for timestamp parsing could hide issues
• csv_logger.py:71-73: File write has no error handling

Recommendation: Add try-except blocks with proper logging for file operations.

6. Security: Hardcoded Host Binding

api.py:147: Dashboard binds to 0.0.0.0 which exposes it to all network interfaces.

uvicorn.run(app, host="0.0.0.0", port=8000)

Security Risk: This allows external access to the dashboard without authentication.
Recommendation: Default to 127.0.0.1 (localhost only) or add authentication middleware.

7. Missing Input Validation

• api.py:65-138: No validation of strategy_id parameter - could allow path traversal attacks
• Example: strategy_id="../../etc/passwd" could read arbitrary files

Recommendation: Validate strategy_id matches expected pattern (alphanumeric + underscore/hyphen only).

8. Resource Leak Risk

strategy.py:675-682: CSV logging happens inside the main loop without any exception handling around log_snapshot().

if self.csv_logger:
    self.refresh_state()
    self.csv_logger.log_snapshot(...)

Recommendation: Wrap in try-except to prevent logging failures from crashing the strategy.

9. Inconsistent Data Types

csv_logger.py:67: Column name sanitization truncates to 20 chars but no similar constraint on data values

safe_name = outcome.replace(" ", "_").replace(",", "").lower()[:20]

Potential Issue: Long outcome names could create columns that don't match their data.

10. HTML Dashboard Uses CDN Dependencies

dashboard.html:7-8: Loading external JS libraries from CDN

<script src="https://cdn.tailwindcss.com"></script>
<script src="https://cdn.jsdelivr.net/npm/chart.js"></script>

Risk: Dependency on external services; version changes could break the dashboard.
Recommendation: Consider vendoring these dependencies or at least pinning versions.

Performance Considerations

Concerns

1. File I/O on every tick: Opening CSV file every 5 seconds (default interval) could add latency
2. No write buffering: Each tick writes immediately to disk
3. CSV parsing in API: Reading entire CSV file on every dashboard refresh (no caching)

Recommendations

• Add optional in-memory buffering with periodic flushes
• Implement basic caching in the API layer with TTL
• Consider using SQLite instead of CSV for better query performance with larger datasets

Test Coverage

Missing Tests

No tests were added for the new functionality:

• No tests for CSVLogger class
• No tests for FastAPI endpoints
• No tests for CSV data integrity
• No integration tests for strategy with logging enabled

Recommendation: Add unit tests covering:

• CSV file creation and header generation
• Snapshot logging with various data types
• API endpoints with mock CSV files
• Error conditions (file write failures, missing files, etc.)

Additional Observations

Positive

1. Dashboard UX: The HTML dashboard looks well-designed with TailwindCSS
2. Stats calculations: Good metrics (PnL, PnL%, max/min NAV, avg delta)
3. CLI integration: --enable-logging flag is intuitive
4. Entry point: dr-manhattan-dashboard script is properly configured in pyproject.toml

Minor Issues

1. .gitignore has duplicate logs/ entry (lines 56 and 67 in the diff)
2. Missing CORS configuration in FastAPI app (may be needed for future frontend development)
3. No rate limiting on API endpoints
4. Dashboard has no auto-refresh capability (requires manual refresh)

Recommendations Priority

High Priority (Must Fix)

1. Fix linting errors
2. Add input validation for strategy_id (security)
3. Change dashboard host binding to localhost by default
4. Add error handling for file operations
5. Remove or consolidate docs/CSV_LOGGING.md per CLAUDE.md

Medium Priority (Should Fix)

1. Add test coverage
2. Improve file I/O performance with buffering
3. Add API caching layer
4. Wrap CSV logging in try-except

Low Priority (Nice to Have)

1. Vendor dashboard JS dependencies
2. Add CORS configuration
3. Add auto-refresh to dashboard
4. Fix duplicate .gitignore entry

Verdict

Overall Assessment: This is a valuable feature addition with good code structure, but it needs several fixes before merging:

• Security issues must be addressed (path traversal, host binding)
• Repository rules compliance (documentation file, variable placement)
• Linting must pass
• Error handling needs improvement

Recommendation: Request changes. The core implementation is solid, but the issues above should be resolved to maintain code quality and security standards.

---

Review conducted following CLAUDE.md guidelines

[claude]

PR Review: Enhance strategy execution with CSV logging and dashboard support

Summary

This PR adds CSV logging capabilities and a web-based dashboard for real-time monitoring of strategy execution. The implementation includes new utilities, FastAPI endpoints, and HTML dashboard with Chart.js visualizations.

Code Quality & Best Practices

Positive Aspects

1. Clean separation of concerns: CSV logging is properly isolated in utils/csv_logger.py
2. Optional feature: Logging is opt-in with enable_csv_logging parameter
3. Good documentation: Wiki documentation provides clear usage examples
4. Follows repository conventions: Code is focused and clean per CLAUDE.md guidelines

Issues & Recommendations

1. Delta Calculation Change (exchange_client.py:940-960)

CRITICAL: This appears to be a breaking change unrelated to CSV logging.

# Old: delta = max_pos - min_pos (unsigned)
# New: delta = position_values[0] - position_values[1] (signed)

Issues:

• Changes behavior for all existing strategies
• Assumes exactly 2 outcomes (will fail on multi-outcome markets)
• Dictionary iteration order is not guaranteed in older Python versions
• Should be in a separate PR with proper testing

Recommendation:

• Revert this change or move to separate PR
• If keeping, add tests and handle edge cases (1 outcome, >2 outcomes)
• Document migration guide for existing strategies

2. Missing Test Coverage

HIGH PRIORITY: No tests for new functionality.

Missing test coverage for:

• CSVLogger class (file creation, header writing, data logging)
• FastAPI endpoints (/api/strategies, /api/strategy/{id}/data)
• Path traversal validation in _validate_strategy_id()
• Delta calculation changes

Recommendation:
Create tests in tests/test_csv_logger.py and tests/web/test_api.py

3. Security Concerns

Path Traversal Validation (web/api.py:335-337)

def _validate_strategy_id(strategy_id: str) -> bool:
    return bool(re.match(r"^[a-zA-Z0-9_-]+$", strategy_id))

• Good: Input validation present
• Issue: Regex allows hyphens which could be exploited in edge cases
• Consider: ^[a-zA-Z0-9_]+$ (remove hyphen) or Path().resolve() check

CORS Missing (web/api.py)

• No CORS middleware configured
• Dashboard won't work if served from different origin
• Add: from fastapi.middleware.cors import CORSMiddleware

No Rate Limiting

• API endpoints have no rate limiting
• Could be abused to read all CSV files
• Consider: slowapi or similar rate limiting

4. Performance Issues

File I/O on Every Tick (strategy.py:119-126)

# Appends to CSV file every tick (default 5 seconds)
with open(self.filepath, "a", newline="") as f:
    writer = csv.writer(f)
    writer.writerow(row)

Issues:

• Disk I/O on every strategy tick
• Could impact strategy performance during high-frequency trading
• No buffering or batching

Recommendations:

• Add buffering: collect N rows, write in batch
• Use async I/O to avoid blocking strategy
• Add flush interval configuration

CSV File Reading (web/api.py:367)

with open(csv_file, "r") as f:
    reader = csv.DictReader(f)
    for row in reader:  # Reads entire file

Issues:

• Reads entire CSV into memory
• Large log files (days of trading) will cause memory issues
• No pagination or limits

Recommendations:

• Add ?limit=1000 query parameter
• Stream data or use pagination
• Consider tail-only reading for large files

5. Configuration & Environment (CLAUDE.md Violation)

CLAUDE.md Rule #4: "Single Source of Truth: DO NOT place many variables in .env file. Place them in the code instead."

Violation (examples/spread_strategy.py:751):

default=os.getenv("ENABLE_CSV_LOGGING", "").lower() in ("true", "1"),

Recommendation:

• Remove ENABLE_CSV_LOGGING env var
• Keep as command-line argument only
• Document in code defaults

6. Error Handling

Missing Error Handling:

• CSVLogger.__init__: No handling if log dir creation fails (permissions)
• CSVLogger._write_header: File write errors not caught
• CSVLogger.log_snapshot: Append failures silently ignored
• web/api.py: No handling for corrupted CSV files
• Dashboard: No reconnection logic for server disconnects

Recommendations:

# csv_logger.py
try:
    self.log_dir.mkdir(parents=True, exist_ok=True)
except PermissionError as e:
    logger.error(f"Failed to create log directory: {e}")
    raise

# api.py  
try:
    reader = csv.DictReader(f)
    # ...
except csv.Error as e:
    raise HTTPException(status_code=500, detail="Corrupted CSV file")

7. Code Style Issues

Hard-coded values (csv_logger.py:202):

safe_name = outcome.replace(" ", "_").replace(",", "").lower()[:20]

• Magic number 20 should be constant
• More sanitization needed (special chars, unicode)

Hard-coded port (api.py:425):

uvicorn.run(app, host="127.0.0.1", port=8000)

• Should be configurable via argument
• Blocks running multiple dashboards

Dashboard CDN dependencies (dashboard.html:442-443):

<script src="https://cdn.tailwindcss.com"></script>
<script src="https://cdn.jsdelivr.net/npm/chart.js"></script>

• Requires internet connection
• CDN outages break dashboard
• Consider vendoring or documenting offline usage

8. .gitignore Issue

Line 20-21 (.gitignore):

-claude-flow
\ No newline at end of file
+claude-flowlogs/

Issue:

• Appears to be merge/rebase error
• claude-flow and logs/ are duplicated
• Missing newline at EOF

Fix:

-claude-flow
\ No newline at end of file
+claude-flow
+logs/

Potential Bugs

1. Race Condition (strategy.py:119)

   • self.refresh_state() called before CSV logging
   • If on_tick() already called it, duplicate API calls
   • Recommendation: Document or optimize

2. Memory Leak (dashboard.html:534)

   • Charts destroyed but event listeners not cleaned up
   • Repeated strategy switches could leak memory
   • Use AbortController or proper cleanup

3. Timezone Issues

   • CSV logs use .isoformat() (local time)
   • Dashboard parses with toLocaleTimeString() (local time)
   • Multi-timezone usage will show incorrect times
   • Use UTC timestamps

4. Dictionary Ordering (exchange_client.py:43)

   • position_values = list(positions.values())
   • Assumes first 2 values are correct outcomes
   • Python 3.7+ guarantees order, but fragile design
   • Recommendation: Use explicit outcome keys

Missing Features

1. CSV Rotation: No log rotation, files grow indefinitely
2. Dashboard Auto-refresh: Manual refresh required
3. Export functionality: Can't download data from dashboard
4. Multi-strategy view: Can't compare multiple strategies
5. Alerts/Notifications: No alerts for PnL thresholds

Performance Considerations

• Dashboard loads all data points (could be 10k+ rows)
• Chart.js may lag with large datasets
• Consider downsampling for display

Testing Recommendations

Add these test files:

tests/test_csv_logger.py
tests/web/test_api.py
tests/web/test_dashboard_integration.py

Test coverage should include:

• CSV file creation and writing
• Outcome name sanitization edge cases
• API endpoint responses
• Path traversal security
• Large file handling
• Corrupted CSV handling

Documentation

Good:

• Wiki documentation is comprehensive
• Clear examples provided

Missing:

• API endpoint documentation (OpenAPI/Swagger)
• Dashboard keyboard shortcuts
• CSV schema documentation
• Migration guide for delta changes

Overall Assessment

Strengths:

• Useful feature for strategy monitoring
• Clean code structure
• Good documentation

Major Concerns:

1. Breaking delta calculation change (needs separate PR)
2. No test coverage (blocking issue)
3. Performance concerns (file I/O, memory usage)
4. Security issues (CORS, rate limiting)
5. Error handling gaps

Recommendation: Request changes before merge.

Action Items

Must Fix (Blocking):

• Revert or separate delta calculation changes
• Add comprehensive test coverage
• Fix .gitignore formatting issue
• Add error handling for file operations
• Remove env var (CLAUDE.md violation)

Should Fix (High Priority):

• Add CORS middleware
• Implement CSV buffering
• Add rate limiting
• Fix timezone handling
• Add pagination for large files

Nice to Have:

• Auto-refresh dashboard
• Vendor CDN dependencies
• Add log rotation
• Add CSV export from dashboard

Positive Notes

The feature itself is valuable and well-implemented conceptually. With the fixes above, this will be a great addition to the project. The dashboard UI looks clean and the separation of concerns is good. Just needs polish on error handling, testing, and performance optimization.

[claude]

PR Review: CSV Logging and Dashboard Support

Overview

This PR adds CSV logging for strategy execution and a FastAPI-based web dashboard for monitoring. The implementation is well-structured and provides valuable functionality for tracking strategy performance.

---

Code Quality & Best Practices

Strengths:

• Clean separation of concerns with dedicated CSVLogger class and web API module
• Good use of pathlib for file operations
• Proper error handling in API endpoints (404, 400 responses)
• Well-documented code with comprehensive docstrings
• Follows the repository's style guidelines (no emojis, clean code)

Issues & Recommendations:

1. File I/O Performance (csv_logger.py:114-116)

   • Opening/closing the CSV file on every log_snapshot() call is inefficient
   • For strategies that tick every 5 seconds, this creates unnecessary I/O overhead
   • Recommendation: Keep file handle open during strategy run, or use buffered writing with periodic flush

2. Missing newline at end of .gitignore (.gitignore:21)

   • The file ends with claude-flowlogs/ without a final newline
   • This violates POSIX standards and may cause issues with some tools
   • Fix: Add newline at end of file

3. Hardcoded logs directory (web/api.py:14)

   • LOGS_DIR = Path("logs") is hardcoded, but strategies can specify custom log_dir
   • Dashboard won't find logs if user specifies a different directory
   • Recommendation: Make this configurable via environment variable or CLI argument

4. Documentation placement

   • Per CLAUDE.md rule add logo #2: "DO NOT create a new document"
   • Created wiki/strategies/CSV_LOGGING.md which may be unnecessary
   • Recommendation: Consider consolidating into existing documentation or README

---

Potential Bugs

1. Delta calculation change (exchange_client.py:43-46)

   • Changed from unsigned max_pos - min_pos to signed position_values[0] - position_values[1]
   • This assumes binary markets and relies on dict ordering (which is stable in Python 3.7+, but still fragile)
   • Risk: May break for non-binary markets or cause unexpected behavior
   • Recommendation: Add validation that positions dict has exactly 2 entries, or use named keys

2. Race condition in CSV writing (csv_logger.py:114-116)

   • Multiple strategy instances writing to logs could cause file conflicts
   • File naming includes timestamp to milliseconds, but concurrent starts could collide
   • Recommendation: Add file locking or unique identifier (PID, UUID) to filename

3. Memory growth in dashboard (dashboard.html:168)

   • Chart data grows unbounded as timestamps array increases
   • For long-running strategies, this could cause browser memory issues
   • Recommendation: Consider pagination or data windowing for large datasets

4. Missing error handling (csv_logger.py:70-72, 114-116)

   • File write operations can fail (disk full, permissions, etc.)
   • No try/catch blocks or error logging
   • Recommendation: Add error handling and log failures without crashing strategy

---

Security Concerns

1. Path Traversal Protection (web/api.py:68-70) ✅

   • Good use of regex validation: r"^[a-zA-Z0-9_-]+$"
   • Prevents directory traversal attacks
   • Note: This is well implemented

2. CDN Dependencies (dashboard.html:7-8)

   • Loading Tailwind CSS and Chart.js from CDNs
   • Creates external dependencies and potential supply chain risk
   • Recommendation: Consider vendoring these dependencies or documenting the security trade-off

3. No CORS configuration (web/api.py:12)

   • FastAPI app has no CORS middleware
   • Dashboard serves from same origin, so not currently an issue
   • Note: Document this if dashboard needs to be accessed cross-origin in future

4. File path exposure (csv_logger.py:120)

   • get_filepath() returns absolute path which could leak system information
   • Currently only used internally, but be cautious if exposed via API
   • Recommendation: Keep this internal-only

---

Performance Considerations

1. CSV reading efficiency (web/api.py:100-120)

   • Reads entire CSV into memory for each API request
   • For long-running strategies with millions of rows, this will be slow
   • Recommendation:
     • Add pagination/limit parameters
     • Consider caching recently accessed data
     • Use streaming for large files

2. Strategy refresh_state() call (strategy.py:120)

   • Calls self.refresh_state() before every CSV log
   • This may fetch positions/orders from exchange API unnecessarily
   • Recommendation: Check if state was already refreshed in current tick to avoid duplicate API calls

3. Statistics calculation (web/api.py:122-129)

   • Recalculates min/max/avg on every API call
   • Could be cached or pre-calculated
   • Recommendation: Cache stats with TTL or invalidate only when new data arrives

4. Chart rendering (dashboard.html:167-293)

   • Destroys and recreates charts on every data load
   • Chart.js has .update() method for incremental updates
   • Recommendation: Use chart updates instead of recreating for better performance

---

Test Coverage

Critical Gap: No tests for new functionality

• No tests for CSVLogger class
• No tests for API endpoints
• No tests for CSV file format/parsing
• No integration tests for logging during strategy execution

Recommendation: Add test files:

tests/test_csv_logger.py
tests/test_web_api.py

Minimum test coverage should include:

• CSV header generation with different outcome counts
• CSV data writing and reading
• API endpoint responses (200, 404, 400)
• Path traversal attack prevention
• Strategy integration with logging enabled/disabled

---

Additional Observations

1. Good additions:

   • Example updated with --enable-logging flag (spread_strategy.py:750-755)
   • Proper .gitignore entries for CSV files
   • CLI entry point for dashboard (dr-manhattan-dashboard)
   • Comprehensive documentation in CSV_LOGGING.md

2. Delta calculation documentation (exchange_client.py:30-31)

   • Added helpful comments explaining signed delta
   • Clarifies the semantic meaning

3. Dependency management:

   • FastAPI and uvicorn properly added to pyproject.toml
   • Version constraints are reasonable (>=0.115.0, >=0.32.0)

---

Summary

Approval Status: ⚠️ Approve with Recommendations

This is a valuable feature that enhances strategy monitoring capabilities. The code is generally well-written and follows project conventions. However, there are some concerns that should be addressed:

Must Fix Before Merge:

• Fix .gitignore newline issue
• Add basic test coverage for new functionality
• Review delta calculation change for non-binary markets

Should Fix (can be follow-up PR):

• Optimize file I/O in CSVLogger
• Add error handling for file operations
• Make dashboard logs directory configurable
• Add pagination for large CSV files

Nice to Have:

• Cache API statistics
• Vendor dashboard dependencies
• Add file locking for concurrent writes

Overall, this is solid work that adds meaningful functionality. With the recommended fixes, it will be production-ready.