From 1dc5778900fd236f910575f39f6a2f6b77f4d810 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 5 Dec 2025 07:38:18 +0000 Subject: [PATCH 1/3] Initial plan From 6ee29279b7f2b54657f6ca8dac8c39b135d4f218 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 5 Dec 2025 07:47:51 +0000 Subject: [PATCH 2/3] docs: Document test infrastructure, security fix, common pitfalls reorganization, and CI/CD pipeline MANDATORY DOCUMENTATION UPDATE (4 PRs): 1. Test Infrastructure (PR #2): 113 tests, coverage requirements, how to run 2. Security Fix (PR #3): Expanded Pitfall #72 with process details 3. Common Pitfalls (PR #1): Reorganization structure and benefits 4. CI/CD Pipeline (PR #5): 4 workflows, troubleshooting, branch protection Restores compliance with mandatory git commit + documentation workflow (lines 217-256 of copilot-instructions.md). Related PRs: #1, #2, #3, #5 Co-authored-by: mindesbunister <32161838+mindesbunister@users.noreply.github.com> --- .github/copilot-instructions.md | 268 ++++++++++++++++++++++++++++++++ 1 file changed, 268 insertions(+) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 53dbbb3..5688af0 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -578,6 +578,59 @@ Documentation is not bureaucracy - it's **protecting future profitability** by p --- +## ๐Ÿ“š Common Pitfalls Documentation Structure (Dec 5, 2025) + +**Purpose:** Centralized documentation of all production incidents, bugs, and lessons learned from real trading operations. + +**Documentation Reorganization (PR #1):** +- **Problem Solved:** Original copilot-instructions.md was 6,575 lines with 72 pitfalls mixed throughout +- **Solution:** Extracted to dedicated `docs/COMMON_PITFALLS.md` (1,556 lines) +- **Result:** 45% reduction in main file size (6,575 โ†’ 3,608 lines) + +**New Structure:** +``` +docs/COMMON_PITFALLS.md +โ”œโ”€โ”€ Quick Reference Table (all 72 pitfalls with severity, category, date) +โ”œโ”€โ”€ ๐Ÿ”ด CRITICAL Pitfalls (Financial/Data Integrity) +โ”‚ โ”œโ”€โ”€ Race Conditions & Duplicates (#27, #41, #48, #49, #59, #60, #61, #67) +โ”‚ โ”œโ”€โ”€ P&L Calculation Errors (#41, #49, #50, #54, #57) +โ”‚ โ””โ”€โ”€ SDK/API Integration (#2, #24, #36, #44, #66) +โ”œโ”€โ”€ โš ๏ธ HIGH Pitfalls (System Stability) +โ”‚ โ”œโ”€โ”€ Deployment & Verification (#1, #31, #47) +โ”‚ โ””โ”€โ”€ Database Operations (#29, #35, #58) +โ”œโ”€โ”€ ๐ŸŸก MEDIUM Pitfalls (Performance/UX) +โ”œโ”€โ”€ ๐Ÿ”ต LOW Pitfalls (Code Quality) +โ”œโ”€โ”€ Pattern Analysis (common root causes) +โ””โ”€โ”€ Contributing Guidelines (how to add new pitfalls) +``` + +**Top 10 Critical Pitfalls (Summary):** +1. **Drift SDK Memory Leak (#1)** - JS heap OOM after 10+ hours โ†’ Smart health monitoring +2. **Wrong RPC Provider (#2)** - Alchemy breaks Drift SDK โ†’ Use Helius only +3. **P&L Compounding Race Condition (#48, #49, #61)** - Multiple closures โ†’ Atomic Map.delete() +4. **Database-First Pattern (#29)** - Save DB before Position Manager +5. **Container Deployment Verification (#31)** - Always check container timestamp +6. **Position.size Tokens vs USD (#24)** - SDK returns tokens โ†’ multiply by price +7. **External Closure Race Condition (#67)** - 16 duplicate notifications โ†’ Atomic lock +8. **Smart Entry Wrong Price (#66)** - Use Pyth oracle, not webhook percentage +9. **MAE/MFE Wrong Units (#54)** - Store percentages, not dollars +10. **Execute Endpoint Quality Bypass (#62)** - Quality check after timeframe validation + +**How to Use:** +- **Quick lookup:** Check Quick Reference Table in `docs/COMMON_PITFALLS.md` +- **By category:** Navigate to severity/category sections +- **Pattern recognition:** See Pattern Analysis for common root causes +- **Adding new pitfalls:** Follow Contributing Guidelines template + +**When Adding New Pitfalls:** +1. Add full details to `docs/COMMON_PITFALLS.md` with standard template +2. Assign severity (๐Ÿ”ด Critical, โš ๏ธ High, ๐ŸŸก Medium, ๐Ÿ”ต Low) +3. Include: symptom, incident details, root cause, fix, prevention, code example +4. Update Quick Reference Table +5. If more critical than existing Top 10, update this section + +--- + ## ๐ŸŽฏ BlockedSignal Minute-Precision Tracking (Dec 2, 2025 - OPTIMIZED) **Purpose:** Track exact minute-by-minute price movements for blocked signals to determine EXACTLY when TP1/TP2 would have been hit @@ -836,6 +889,159 @@ Frequency penalties (overtrading / flip-flop / alternating) now ignore 1-minute - Penalty for recent losing trades, bonus for winning streaks - **Note:** Analytics check is advisory only - manual trades execute even if rejected by analytics +## ๐Ÿงช Test Infrastructure (Dec 5, 2025 - PR #2) + +**Purpose:** Comprehensive integration test suite for Position Manager - the 1,938-line core trading logic managing real capital. + +**Test Suite Structure:** +``` +tests/ +โ”œโ”€โ”€ setup.ts # Global mocks (Drift, Pyth, DB, Telegram) +โ”œโ”€โ”€ helpers/ +โ”‚ โ””โ”€โ”€ trade-factory.ts # Factory functions for mock trades +โ””โ”€โ”€ integration/ + โ””โ”€โ”€ position-manager/ + โ”œโ”€โ”€ tp1-detection.test.ts # 16 tests - TP1 triggers for LONG/SHORT + โ”œโ”€โ”€ breakeven-sl.test.ts # 14 tests - SL moves to entry after TP1 + โ”œโ”€โ”€ adx-runner-sl.test.ts # 18 tests - ADX-based runner SL tiers + โ”œโ”€โ”€ trailing-stop.test.ts # 16 tests - ATR-based trailing with peak tracking + โ”œโ”€โ”€ edge-cases.test.ts # 15 tests - Token vs USD, phantom detection + โ”œโ”€โ”€ price-verification.test.ts # 18 tests - Size AND price verification + โ””โ”€โ”€ decision-helpers.test.ts # 16 tests - shouldStopLoss, shouldTakeProfit1/2 +``` + +**Total:** 7 test files, 113 tests + +**Test Configuration:** +- **Framework:** Jest + ts-jest +- **Config:** `jest.config.js` at project root (created by PR #2) +- **Coverage Threshold:** 60% minimum +- **Mocks:** Drift SDK, Pyth price feeds, PostgreSQL, Telegram notifications + +**How to Run Tests:** +```bash +# Run all tests +npm test + +# Run tests in watch mode (development) +npm run test:watch + +# Run with coverage report +npm run test:coverage + +# Run specific test file +npm test -- tests/integration/position-manager/tp1-detection.test.ts +``` + +**Trade Factory Helpers:** +```typescript +import { createLongTrade, createShortTrade, createTradeAfterTP1 } from '../helpers/trade-factory' + +// Create basic trades +const longTrade = createLongTrade({ entryPrice: 140, adx: 26.9 }) +const shortTrade = createShortTrade({ entryPrice: 140, atr: 0.43 }) + +// Create runner after TP1 +const runner = createTradeAfterTP1('short', { positionSize: 8000 }) +``` + +**Common Pitfalls Prevented by Tests:** +- **#24:** Position.size tokens vs USD conversion +- **#43:** TP1 false detection without price verification +- **#45:** Wrong entry price for breakeven SL (must use DB entry, not Drift) +- **#52:** ADX-based runner SL tier calculations +- **#54:** MAE/MFE stored as percentages, not dollars +- **#67:** Duplicate closure race conditions + +**Test Data (Standard Values):** +| Parameter | LONG | SHORT | +|-----------|------|-------| +| Entry | $140.00 | $140.00 | +| TP1 (+0.86%) | $141.20 | $138.80 | +| TP2 (+1.72%) | $142.41 | $137.59 | +| SL (-0.92%) | $138.71 | $141.29 | +| ATR | 0.43 | 0.43 | +| ADX | 26.9 | 26.9 | +| Position Size | $8,000 | $8,000 | + +**Why Tests Matter:** +- Position Manager handles **real money** ($540 capital, targeting $100k) +- Zero test coverage before this PR despite 170+ trades and 71 documented bugs +- Prevents regressions when refactoring critical trading logic +- Validates calculations match documented behavior + +--- + +## ๐Ÿ”„ CI/CD Pipeline (Dec 5, 2025 - PR #5) + +**Purpose:** Automated quality gates ensuring code reliability before deployment to production trading system. + +**Workflows:** + +### 1. Test Workflow (`test.yml`) +**Triggers:** Push/PR to main/master/develop +```yaml +- npm ci # Install dependencies +- npm test # Run 113 tests +- npm run build # Verify TypeScript compiles +``` +**Blocking:** โœ… PRs cannot merge if tests fail + +### 2. Build Workflow (`build.yml`) +**Triggers:** Push/PR to main/master +```yaml +- docker build # Build production image +- Buildx caching # Layer caching for speed +``` +**Blocking:** โœ… PRs cannot merge if Docker build fails + +### 3. Lint Workflow (`lint.yml`) +**Triggers:** Every push/PR +```yaml +- ESLint check # Code quality +- console.log scan # Find debug statements in production code +- TypeScript strict # Type checking +``` +**Blocking:** โš ๏ธ Warnings only (does not block merge) + +### 4. Security Workflow (`security.yml`) +**Triggers:** Push/PR + weekly schedule +```yaml +- npm audit # Check for vulnerable dependencies +- Secret scanning # Basic credential detection +``` +**Blocking:** โœ… Fails on high/critical vulnerabilities + +**Status Badges (README.md):** +```markdown +![Tests](https://github.com/mindesbunister/trading_bot_v4/workflows/Tests/badge.svg) +![Docker Build](https://github.com/mindesbunister/trading_bot_v4/workflows/Docker%20Build/badge.svg) +![Security](https://github.com/mindesbunister/trading_bot_v4/workflows/Security/badge.svg) +``` + +**Branch Protection Recommendations:** +Enable in GitHub Settings โ†’ Branches โ†’ Add rule: +- โœ… Require status checks to pass (test, build) +- โœ… Require PR reviews before merging +- โœ… Require branches to be up to date + +**Troubleshooting Common Failures:** + +| Failure | Cause | Fix | +|---------|-------|-----| +| Test failure | Position Manager logic changed | Update tests or fix regression | +| Build failure | TypeScript error | Check `npm run build` locally | +| Lint warning | console.log in code | Remove or use proper logging | +| Security alert | Vulnerable dependency | `npm audit fix` or update package | + +**Why CI/CD Matters:** +- **Real money at stake:** Bugs cost actual dollars +- **Confidence to deploy:** Green pipeline = safe to merge +- **Fast feedback:** Know within minutes if change breaks something +- **Professional practice:** Industry standard for production systems + +--- + ## VERIFICATION MANDATE: Financial Code Requires Proof **CRITICAL: THIS IS A REAL MONEY TRADING SYSTEM - NOT A TOY PROJECT** @@ -5578,6 +5784,68 @@ trade.realizedPnL += actualRealizedPnL // NOT: result.realizedPnL from SDK - **Git commit:** N/A (configuration fix, no code changes) - **Status:** โœ… Fixed and verified working (user confirmed `/status` responds correctly) +72. **CRITICAL SECURITY: .env file tracked in git (CRITICAL - Fixed Dec 5, 2025 - PR #3):** + - **Symptom:** Sensitive credentials exposed in git repository history + - **Credentials exposed:** + * Database connection strings (PostgreSQL) + * Drift Protocol private keys (wallet access) + * Telegram bot tokens + * API keys and secrets + * RPC endpoints + - **Root Cause:** `.env` file was tracked in git from initial commit, exposing all secrets to anyone with repository access + - **Files modified:** + * `.gitignore` - Added `.env`, `.env.local`, `.env.*.local` patterns + * `.env` - Removed from git tracking (kept locally) + * `.env.telegram-bot` - Removed from git tracking (contains bot token) + - **Fix Process (Dec 5, 2025):** + ```bash + # 1. Update .gitignore first + echo ".env" >> .gitignore + echo ".env.local" >> .gitignore + echo ".env.*.local" >> .gitignore + + # 2. Remove from git tracking (keeps local file) + git rm --cached .env + git rm --cached .env.telegram-bot + + # 3. Commit the fix + git commit -m "security: Remove .env from git tracking" + ``` + - **Impact:** + * โœ… Future commits will NOT include .env files + * โœ… Local development unaffected (files still exist locally) + * โš ๏ธ Historical commits still contain secrets (until git history rewrite) + - **POST-FIX ACTIONS REQUIRED:** + 1. **Rotate all credentials immediately:** + - Database passwords + - Telegram bot token (create new bot if needed) + - Drift Protocol keys (if exposed to public) + - Any API keys in .env + 2. **Verify .env.example exists** - Template for new developers + 3. **Consider git history cleanup** - Use BFG Repo-Cleaner if secrets were public + - **Prevention:** + * Always add `.env` to `.gitignore` BEFORE first commit + * Use `.env.example` with placeholder values + * CI/CD should fail if .env detected in commit + * Regular security audits with `git log -p | grep -i password` + - **Why This Matters for Trading Bot:** + * **Private keys = wallet access** - Could drain trading account + * **Database = trade history** - Could manipulate records + * **Telegram = notifications** - Could send fake alerts + * This is a **real money system** managing $540 capital + - **Verification:** + ```bash + # Confirm .env is ignored + git check-ignore .env + # Should output: .env + + # Confirm .env not tracked + git ls-files | grep "\.env" + # Should output: only .env.example + ``` + - **Git commit:** PR #3 on branch `copilot/remove-env-from-git-tracking` + - **Status:** โœ… Fixed - .env removed from tracking, .gitignore updated + ## File Conventions - **API routes:** `app/api/[feature]/[action]/route.ts` (Next.js 15 App Router) From 863f10dc77f9d6ebcd4a2f77d4f95079ccc53cc6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 5 Dec 2025 07:49:22 +0000 Subject: [PATCH 3/3] docs: Improve .gitignore fix instructions in Pitfall #72 Co-authored-by: mindesbunister <32161838+mindesbunister@users.noreply.github.com> --- .github/copilot-instructions.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 5688af0..8830ae0 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -5799,10 +5799,10 @@ trade.realizedPnL += actualRealizedPnL // NOT: result.realizedPnL from SDK * `.env.telegram-bot` - Removed from git tracking (contains bot token) - **Fix Process (Dec 5, 2025):** ```bash - # 1. Update .gitignore first - echo ".env" >> .gitignore - echo ".env.local" >> .gitignore - echo ".env.*.local" >> .gitignore + # 1. Update .gitignore first (add these lines if not present) + # .env + # .env.local + # .env.*.local # 2. Remove from git tracking (keeps local file) git rm --cached .env