Merge pull request #6 from mindesbunister/copilot/document-recent-prs

docs: Document 4 undocumented PRs in copilot-instructions.md
2025-12-05 09:10:12 +01:00
parent 12212502bd 863f10dc77
commit aee12432f5
1 changed files with 268 additions and 0 deletions
--- 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 (add these lines if not present)
+      # .env
+      # .env.local  
+      # .env.*.local
+      
+      # 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)