From 2481fda68c2ba582cbf34420747004846004ac66 Mon Sep 17 00:00:00 2001 From: Marko Djordjevic Date: Tue, 17 Feb 2026 23:34:12 +0100 Subject: [PATCH] fix: remove all SQLite references (migrate.ts, migration script, package.json) --- .agents/.skill-lock.json | 15 + .agents/skills/code-reviewer/SKILL.md | 88 +++ .../code-reviewer/references/common-issues.md | 142 +++++ .../references/feedback-examples.md | 144 +++++ .../references/receiving-feedback.md | 238 +++++++++ .../references/report-template.md | 109 ++++ .../references/review-checklist.md | 88 +++ .../references/spec-compliance-review.md | 258 +++++++++ .cursor/skills/code-reviewer | 1 + .qwen/skills/code-reviewer | 1 + CODE_REVIEW.md | 438 +++++++++++++++ package.json | 3 +- scripts/migrate-sqlite-to-postgres.ts | 499 ------------------ src/lib/db/migrate.ts | 23 - 14 files changed, 1523 insertions(+), 524 deletions(-) create mode 100644 .agents/.skill-lock.json create mode 100644 .agents/skills/code-reviewer/SKILL.md create mode 100644 .agents/skills/code-reviewer/references/common-issues.md create mode 100644 .agents/skills/code-reviewer/references/feedback-examples.md create mode 100644 .agents/skills/code-reviewer/references/receiving-feedback.md create mode 100644 .agents/skills/code-reviewer/references/report-template.md create mode 100644 .agents/skills/code-reviewer/references/review-checklist.md create mode 100644 .agents/skills/code-reviewer/references/spec-compliance-review.md create mode 120000 .cursor/skills/code-reviewer create mode 120000 .qwen/skills/code-reviewer create mode 100644 CODE_REVIEW.md delete mode 100644 scripts/migrate-sqlite-to-postgres.ts delete mode 100644 src/lib/db/migrate.ts diff --git a/.agents/.skill-lock.json b/.agents/.skill-lock.json new file mode 100644 index 0000000..48db8b0 --- /dev/null +++ b/.agents/.skill-lock.json @@ -0,0 +1,15 @@ +{ + "version": 3, + "skills": { + "code-reviewer": { + "source": "jeffallan/claude-skills", + "sourceType": "github", + "sourceUrl": "https://github.com/jeffallan/claude-skills.git", + "skillPath": "skills/code-reviewer/SKILL.md", + "skillFolderHash": "c63068de74482c9a26534c9f384f434ab6362eda", + "installedAt": "2026-02-17T21:51:45.606Z", + "updatedAt": "2026-02-17T21:51:45.606Z" + } + }, + "dismissed": {} +} \ No newline at end of file diff --git a/.agents/skills/code-reviewer/SKILL.md b/.agents/skills/code-reviewer/SKILL.md new file mode 100644 index 0000000..032b129 --- /dev/null +++ b/.agents/skills/code-reviewer/SKILL.md @@ -0,0 +1,88 @@ +--- +name: code-reviewer +description: Use when reviewing pull requests, conducting code quality audits, or identifying security vulnerabilities. Invoke for PR reviews, code quality checks, refactoring suggestions. +license: MIT +allowed-tools: Read, Grep, Glob +metadata: + author: https://github.com/Jeffallan + version: "1.0.0" + domain: quality + triggers: code review, PR review, pull request, review code, code quality + role: specialist + scope: review + output-format: report + related-skills: security-reviewer, test-master, architecture-designer +--- + +# Code Reviewer + +Senior engineer conducting thorough, constructive code reviews that improve quality and share knowledge. + +## Role Definition + +You are a principal engineer with 12+ years of experience across multiple languages. You review code for correctness, security, performance, and maintainability. You provide actionable feedback that helps developers grow. + +## When to Use This Skill + +- Reviewing pull requests +- Conducting code quality audits +- Identifying refactoring opportunities +- Checking for security vulnerabilities +- Validating architectural decisions + +## Core Workflow + +1. **Context** - Read PR description, understand the problem +2. **Structure** - Review architecture and design decisions +3. **Details** - Check code quality, security, performance +4. **Tests** - Validate test coverage and quality +5. **Feedback** - Provide categorized, actionable feedback + +## Reference Guide + +Load detailed guidance based on context: + + + +| Topic | Reference | Load When | +|-------|-----------|-----------| +| Review Checklist | `references/review-checklist.md` | Starting a review, categories | +| Common Issues | `references/common-issues.md` | N+1 queries, magic numbers, patterns | +| Feedback Examples | `references/feedback-examples.md` | Writing good feedback | +| Report Template | `references/report-template.md` | Writing final review report | +| Spec Compliance | `references/spec-compliance-review.md` | Reviewing implementations, PR review, spec verification | +| Receiving Feedback | `references/receiving-feedback.md` | Responding to review comments, handling feedback | + +## Constraints + +### MUST DO +- Understand context before reviewing +- Provide specific, actionable feedback +- Include code examples in suggestions +- Praise good patterns +- Prioritize feedback (critical → minor) +- Review tests as thoroughly as code +- Check for security issues + +### MUST NOT DO +- Be condescending or rude +- Nitpick style when linters exist +- Block on personal preferences +- Demand perfection +- Review without understanding the why +- Skip praising good work + +## Output Templates + +Code review report should include: +1. Summary (overall assessment) +2. Critical issues (must fix) +3. Major issues (should fix) +4. Minor issues (nice to have) +5. Positive feedback +6. Questions for author +7. Verdict (approve/request changes/comment) + +## Knowledge Reference + +SOLID, DRY, KISS, YAGNI, design patterns, OWASP Top 10, language idioms, testing patterns diff --git a/.agents/skills/code-reviewer/references/common-issues.md b/.agents/skills/code-reviewer/references/common-issues.md new file mode 100644 index 0000000..b1b51f3 --- /dev/null +++ b/.agents/skills/code-reviewer/references/common-issues.md @@ -0,0 +1,142 @@ +# Common Issues + +## N+1 Query Problem + +```typescript +// N+1 queries - BAD +const posts = await Post.findAll(); +for (const post of posts) { + post.author = await User.findById(post.authorId); // N queries! +} + +// Single query with join - GOOD +const posts = await Post.findAll({ include: [User] }); + +// Or batch load +const posts = await Post.findAll(); +const authorIds = posts.map(p => p.authorId); +const authors = await User.findByIds(authorIds); +``` + +## Missing Error Handling + +```typescript +// Unhandled rejection - BAD +const data = await fetch('/api/data').then(r => r.json()); + +// Proper error handling - GOOD +try { + const response = await fetch('/api/data'); + if (!response.ok) { + throw new Error(`HTTP ${response.status}`); + } + const data = await response.json(); +} catch (error) { + logger.error('Failed to fetch data', { error }); + throw new DataFetchError('Could not load data'); +} +``` + +## Magic Numbers/Strings + +```typescript +// Magic number - BAD +if (user.age >= 18) { ... } +setTimeout(fn, 86400000); + +// Named constant - GOOD +const MINIMUM_AGE = 18; +const ONE_DAY_MS = 24 * 60 * 60 * 1000; + +if (user.age >= MINIMUM_AGE) { ... } +setTimeout(fn, ONE_DAY_MS); +``` + +## Deep Nesting + +```typescript +// Deep nesting - BAD +if (user) { + if (user.isActive) { + if (user.hasPermission) { + doSomething(); + } + } +} + +// Early returns - GOOD +if (!user || !user.isActive || !user.hasPermission) { + return; +} +doSomething(); +``` + +## God Functions + +```typescript +// Does too much - BAD +async function processOrder(order) { + // validate + // check inventory + // process payment + // send email + // update database + // log analytics +} + +// Single responsibility - GOOD +async function processOrder(order) { + await validateOrder(order); + await reserveInventory(order); + await chargePayment(order); + await sendConfirmation(order); +} +``` + +## Mutable Shared State + +```typescript +// Shared mutable - BAD +const config = { debug: false }; +function enableDebug() { + config.debug = true; +} + +// Immutable pattern - GOOD +function createConfig(overrides = {}) { + return Object.freeze({ debug: false, ...overrides }); +} +``` + +## Missing Null Checks + +```typescript +// Unsafe access - BAD +const name = user.profile.name; + +// Safe access - GOOD +const name = user?.profile?.name ?? 'Unknown'; +``` + +## Synchronous File Operations + +```typescript +// Blocks event loop - BAD +const data = fs.readFileSync('file.txt'); + +// Non-blocking - GOOD +const data = await fs.promises.readFile('file.txt'); +``` + +## Quick Reference + +| Issue | Impact | Fix | +|-------|--------|-----| +| N+1 queries | Performance | Eager load or batch | +| Missing error handling | Reliability | Try/catch + logging | +| Magic numbers | Maintainability | Named constants | +| Deep nesting | Readability | Early returns | +| God functions | Testability | Single responsibility | +| Mutable shared state | Bugs | Immutable patterns | +| Missing null checks | Crashes | Optional chaining | +| Sync file operations | Performance | Async operations | diff --git a/.agents/skills/code-reviewer/references/feedback-examples.md b/.agents/skills/code-reviewer/references/feedback-examples.md new file mode 100644 index 0000000..629f474 --- /dev/null +++ b/.agents/skills/code-reviewer/references/feedback-examples.md @@ -0,0 +1,144 @@ +# Feedback Examples + +## Good vs Bad Feedback + +### Be Specific, Not Vague + +```markdown +BAD: "This is confusing" + +GOOD: "This function handles both validation and persistence. Consider + splitting into `validateUser()` and `saveUser()` for single + responsibility and easier testing." +``` + +### Be Actionable, Not Just Critical + +```markdown +BAD: "Fix the query" + +GOOD: "This will cause N+1 queries - one per post. Use `include: [Author]` + to eager load authors in a single query. See: [link to docs]" +``` + +### Be Constructive, Not Demanding + +```markdown +BAD: "Add tests" + +GOOD: "Missing test for the case when `email` is already taken. Add a test + that verifies 409 is returned with appropriate error message." +``` + +### Ask Questions, Don't Assume + +```markdown +BAD: "This is wrong" + +GOOD: "I notice this returns null instead of throwing. Is that intentional? + The other methods throw on not-found. Should this be consistent?" +``` + +## Praise Examples + +Reinforce good patterns with specific praise: + +```markdown +"Great use of early returns here - much more readable than nested ifs!" + +"Nice extraction of this validation logic into a reusable function." + +"Excellent error messages - they'll help debugging in production." + +"Good choice using a discriminated union here instead of optional fields." + +"Appreciate the comprehensive test coverage, especially the edge cases." +``` + +## Feedback by Category + +### Critical (Must Fix) + +```markdown +**[CRITICAL] Security: SQL Injection** +Location: `src/users/service.ts:45` + +The query uses string interpolation: +`SELECT * FROM users WHERE id = ${id}` + +This is vulnerable to SQL injection. Use parameterized query: +`db.query('SELECT * FROM users WHERE id = $1', [id])` +``` + +### Major (Should Fix) + +```markdown +**[MAJOR] Performance: N+1 Query** +Location: `src/posts/service.ts:23` + +Current code fetches users in a loop (N+1 problem): +```typescript +for (const post of posts) { + post.author = await User.findById(post.authorId); +} +``` + +Suggestion: Use eager loading: +```typescript +const posts = await Post.findAll({ include: [User] }); +``` + +Impact: ~100 extra DB queries per request with current approach. +``` + +### Minor (Nice to Have) + +```markdown +**[MINOR] Naming: Unclear variable** +Location: `src/utils/date.ts:12` + +`d` is unclear. Consider `createdDate` or `timestamp` for better readability. + +**[MINOR] Style: Prefer const** +Location: `src/config/index.ts:8` + +`let config` is never reassigned. Use `const` for immutability. +``` + +## Question Format + +```markdown +**[QUESTION]** +Location: `src/orders/service.ts:67` + +What's the expected behavior when the user has an existing pending order? +Should this: +- Return the existing order? +- Create a new one anyway? +- Return an error? +``` + +## Summary Format + +```markdown +## Summary + +Overall this is a solid implementation of the user registration flow. +The validation logic is clean and the error handling is comprehensive. + +**Blocking Issues**: 1 critical (SQL injection) +**Suggestions**: 2 major, 3 minor + +Once the SQL injection is fixed, this is ready to merge. The major +suggestions are performance improvements worth considering. +``` + +## Quick Reference + +| Feedback Type | Tone | Required Action | +|---------------|------|-----------------| +| Critical | Firm, clear | Must fix before merge | +| Major | Suggestive | Should fix | +| Minor | Optional | Nice to have | +| Praise | Positive | None - reinforcement | +| Question | Curious | Response needed | diff --git a/.agents/skills/code-reviewer/references/receiving-feedback.md b/.agents/skills/code-reviewer/references/receiving-feedback.md new file mode 100644 index 0000000..ac1c4f4 --- /dev/null +++ b/.agents/skills/code-reviewer/references/receiving-feedback.md @@ -0,0 +1,238 @@ +# Receiving Feedback + +--- + +## Core Mindset + +> "Verify before implementing. Ask before assuming. Technical correctness over social comfort." + +Code review feedback is a technical discussion, not a social one. Focus on the code, not on feelings. + +--- + +## The Six-Step Process + +### Step 1: Read Completely + +**Without reacting.** Read the entire comment before forming any response. + +```markdown +❌ BAD: Read first sentence → start typing defense +✅ GOOD: Read entire comment → understand full context → then respond +``` + +### Step 2: Restate Requirements + +Rephrase the reviewer's feedback in your own words to confirm understanding. + +```markdown +Reviewer: "This function is doing too much. It handles validation, +transformation, and persistence all in one place." + +Your restatement: "You're suggesting I split this into three separate +functions: validate(), transform(), and persist()?" +``` + +### Step 3: Check Against Codebase + +Verify the feedback against actual code conditions before responding. + +```typescript +// Reviewer says: "This will throw if user is null" + +// Check the code: +function getUsername(user: User): string { + return user.name; // No null check - reviewer is correct +} + +// Or discover context: +function getUsername(user: User): string { + return user.name; // TypeScript enforces User, null not possible +} +``` + +### Step 4: Evaluate Technical Soundness + +Consider whether the feedback applies to your specific stack and context. + +```markdown +Reviewer: "You should use useMemo here for performance" + +Evaluate: +- Is this component re-rendering frequently? → Check React DevTools +- Is the computation expensive? → Profile it +- Does React 19's compiler auto-optimize this? → Check version +``` + +### Step 5: Respond with Substance + +Provide technical acknowledgment or reasoned objection. + +```markdown +✅ GOOD: "Fixed. Split into validate(), transform(), persist() + at lines 24, 45, 67." + +✅ GOOD: "Respectfully disagree. This list has max 5 items + (see schema.ts:12), so filter performance is O(5)." + +❌ BAD: "You're absolutely right! Great catch!" +❌ BAD: "I don't think that's necessary." +``` + +### Step 6: Implement One at a Time + +Address each piece of feedback individually with verification. + +```markdown +Feedback item 1: Add null check +→ Implement → Test → Commit → Verify → Move to next + +Feedback item 2: Extract helper function +→ Implement → Test → Commit → Verify → Move to next + +NOT: Try to address all feedback in one massive commit +``` + +--- + +## Avoiding Agreement Theater + +### The Problem + +Performative agreement wastes time and provides no information. When you write "Great point!" you're adding noise, not signal. + +### Forbidden Phrases + +| Phrase | Why It's Wrong | +|--------|----------------| +| "You're absolutely right!" | Sycophantic, adds no information | +| "Great point!" | Empty praise, not a response | +| "Excellent feedback!" | Flattery, not engagement | +| "Thanks for catching this!" | Unnecessary, just fix it | +| "I really appreciate..." | Social fluff, not technical | + +### Actions Demonstrate Understanding + +```markdown +❌ "You're absolutely right! Great catch on that null check! + Thanks so much for pointing this out!" + +✅ "Fixed. Added null check at line 42." +``` + +The code change shows you understood. Words are redundant. + +### When Acknowledgment IS Appropriate + +Brief, technical acknowledgment when learning something new: + +```markdown +✅ "I wasn't aware of that edge case. Added handling at line 42." +✅ "Good point about thread safety. Added mutex at line 67." +``` + +--- + +## When to Push Back + +### Valid Reasons to Disagree + +Push back with technical reasoning when feedback: + +| Situation | How to Respond | +|-----------|----------------| +| Breaks existing functionality | "This change would break Feature X (see test at tests/feature-x.spec.ts:34)" | +| Lacks full codebase context | "This pattern exists because of Y (see architecture.md#constraints)" | +| Violates YAGNI | "This flexibility isn't needed yet - only one caller exists" | +| Is technically incorrect | "This actually works because of Z (link to docs)" | +| Conflicts with established architecture | "This conflicts with our JWT approach (see auth/README.md)" | + +### Good Pushback Format + +```markdown +## Template +This conflicts with [X]. [Evidence]. Was that the intent, or should we [alternative]? + +## Example +This conflicts with our JWT authentication architecture (see auth/token.js:45). +Switching to sessions would require restructuring the API middleware. +Was that the intent, or should we keep JWT? +``` + +### Bad Pushback + +```markdown +❌ "I don't think that's right." +❌ "That won't work." +❌ "We've always done it this way." +❌ "That's too much work." +``` + +--- + +## Verification Before Claiming Fixed + +### The Checklist + +Before writing "Fixed" or "Done": + +- [ ] Change is implemented +- [ ] Tests pass (full suite, not just changed files) +- [ ] Specific behavior mentioned in feedback is verified +- [ ] Edge cases are tested +- [ ] No unintended side effects introduced + +### Acceptable Responses + +```markdown +✅ "Fixed. Added null check. Tests pass." +✅ "Fixed at line 42. Verified with test case X." +✅ "Implemented. All 47 tests pass." +``` + +### Unacceptable Responses + +```markdown +❌ "I think this addresses your concern." +❌ "Should be fixed now." +❌ "Done, I believe." +❌ "Fixed (probably)." +``` + +### When You Can't Verify + +If you cannot verify a fix: + +```markdown +✅ "Implemented the change, but I'm unable to verify because + [specific reason]. Can you confirm on your end?" +``` + +--- + +## Quick Reference + +| Situation | Response | +|-----------|----------| +| Reviewer is correct | "Fixed. [What you changed]." | +| You need clarification | "To confirm: you're suggesting [restatement]?" | +| Reviewer is incorrect | "This works because [evidence]. [Link to proof]." | +| You disagree on approach | "This conflicts with [X]. Should we [alternative]?" | +| You learned something | "I wasn't aware of [X]. Fixed at line [N]." | +| You can't verify | "Implemented. Unable to verify because [reason]." | + +--- + +## Anti-Patterns + +| Pattern | Problem | Fix | +|---------|---------|-----| +| Defensive responses | Creates conflict, wastes time | Assume good faith, respond technically | +| Apologetic responses | Unprofessional, adds noise | Just fix it | +| Delayed responses | Blocks review cycle | Respond within hours, not days | +| Vague responses | Leaves reviewer uncertain | Be specific about changes | +| Ignoring feedback | Disrespectful, creates friction | Address every point | + +--- + +*Content adapted from [obra/superpowers](https://github.com/obra/superpowers) by Jesse Vincent (@obra), MIT License.* diff --git a/.agents/skills/code-reviewer/references/report-template.md b/.agents/skills/code-reviewer/references/report-template.md new file mode 100644 index 0000000..7cdc5fc --- /dev/null +++ b/.agents/skills/code-reviewer/references/report-template.md @@ -0,0 +1,109 @@ +# Report Template + +## Full Review Report Template + +```markdown +# Code Review: [PR Title] + +## Summary +[1-2 sentence overview of the changes and overall assessment] + +**Verdict**: [ ] Approve | [x] Request Changes | [ ] Comment + +## Critical Issues (Must Fix) + +### 1. [File:Line] Security: SQL Injection Risk +- **Current**: String interpolation in query +- **Suggested**: Use parameterized query +- **Impact**: Potential data breach + +```typescript +// Current (vulnerable) +const query = `SELECT * FROM users WHERE id = ${id}`; + +// Suggested (secure) +const query = 'SELECT * FROM users WHERE id = $1'; +db.query(query, [id]); +``` + +## Major Issues (Should Fix) + +### 1. [File:Line] Performance: N+1 Query +- **Current**: Fetching users in loop +- **Suggested**: Use eager loading with include +- **Impact**: ~100 extra DB queries per request + +### 2. [File:Line] Logic: Missing edge case +- **Current**: No handling for empty array +- **Suggested**: Add guard clause +- **Impact**: Potential runtime error + +## Minor Issues (Nice to Have) + +### 1. [File:Line] Naming: Unclear variable name +- **Current**: `d` +- **Suggested**: `createdDate` + +### 2. [File:Line] Style: Inconsistent formatting +- **Current**: Mixed quotes +- **Suggested**: Use single quotes consistently + +## Positive Feedback +- Clean separation of concerns in service layer +- Comprehensive input validation on DTOs +- Good test coverage for edge cases +- Excellent error messages + +## Questions for Author +- What's the expected behavior when X happens? +- Should this support pagination for large datasets? +- Is the retry logic intentional or accidental? + +## Test Coverage Assessment +- [ ] Happy path tested +- [x] Error cases tested +- [ ] Edge cases tested (missing empty array test) +- [x] Integration tests present + +## Checklist +- [x] No security vulnerabilities +- [ ] Performance is acceptable (N+1 issue) +- [x] Code is readable +- [x] Tests are adequate +- [x] Documentation is present +``` + +## Verdict Guidelines + +| Verdict | When to Use | +|---------|-------------| +| **Approve** | No blocking issues, minor suggestions only | +| **Request Changes** | Critical or major issues must be fixed | +| **Comment** | Questions need answers, no blocking issues | + +## Severity Definitions + +| Severity | Definition | Examples | +|----------|------------|----------| +| **Critical** | Security risk, data loss, crashes | SQL injection, auth bypass | +| **Major** | Significant performance, maintainability | N+1 queries, god functions | +| **Minor** | Style, naming, small improvements | Variable names, formatting | + +## Time Boxing + +| Section | Suggested Time | +|---------|----------------| +| Context & understanding | 5 minutes | +| Critical/security review | 10 minutes | +| Logic & performance | 15 minutes | +| Tests review | 10 minutes | +| Writing report | 10 minutes | +| **Total** | ~50 minutes | + +## Quick Checks Before Submitting + +- [ ] All critical issues have clear remediation +- [ ] Major issues explain the impact +- [ ] At least one positive comment included +- [ ] Questions are specific and answerable +- [ ] Verdict matches the issues found diff --git a/.agents/skills/code-reviewer/references/review-checklist.md b/.agents/skills/code-reviewer/references/review-checklist.md new file mode 100644 index 0000000..1ec2bde --- /dev/null +++ b/.agents/skills/code-reviewer/references/review-checklist.md @@ -0,0 +1,88 @@ +# Review Checklist + +## Comprehensive Review Checklist + +| Category | Key Questions | +|----------|---------------| +| **Design** | Does it fit existing patterns? Right abstraction level? | +| **Logic** | Edge cases handled? Race conditions? Null checks? | +| **Security** | Input validated? Auth checked? Secrets safe? | +| **Performance** | N+1 queries? Memory leaks? Caching needed? | +| **Tests** | Adequate coverage? Edge cases tested? Mocks appropriate? | +| **Naming** | Clear, consistent, intention-revealing? | +| **Error Handling** | Errors caught? Meaningful messages? Logged? | +| **Documentation** | Public APIs documented? Complex logic explained? | + +## Review Process + +### 1. Context (5 min) +- [ ] Read PR description +- [ ] Understand the problem being solved +- [ ] Check linked issues/tickets +- [ ] Note expected changes + +### 2. Structure (10 min) +- [ ] Review file organization +- [ ] Check architectural fit +- [ ] Verify design patterns used +- [ ] Note any breaking changes + +### 3. Code Details (20 min) +- [ ] Review logic correctness +- [ ] Check edge cases +- [ ] Verify error handling +- [ ] Look for security issues +- [ ] Check performance concerns +- [ ] Review naming clarity + +### 4. Tests (10 min) +- [ ] Verify test coverage +- [ ] Check test quality +- [ ] Look for edge case tests +- [ ] Ensure mocks are appropriate + +### 5. Final Pass (5 min) +- [ ] Note positive patterns +- [ ] Prioritize feedback +- [ ] Write summary + +## Category Deep Dive + +### Design Questions +- Does this change belong in this file/module? +- Is the abstraction level appropriate? +- Could this be simpler? +- Does it follow existing patterns? +- Is it extensible without modification? + +### Logic Questions +- What happens with null/undefined inputs? +- Are boundary conditions handled? +- Could there be race conditions? +- Is the order of operations correct? +- Are all code paths tested? + +### Security Questions +- Is all user input validated? +- Are SQL queries parameterized? +- Is output properly encoded? +- Are secrets handled safely? +- Is authentication checked? +- Is authorization enforced? + +### Performance Questions +- Are there N+1 query patterns? +- Is data fetched efficiently? +- Are expensive operations cached? +- Could this cause memory leaks? +- Is pagination implemented? + +## Quick Reference + +| Review Focus | Time % | +|--------------|--------| +| Context & PR description | 10% | +| Architecture & design | 20% | +| Code logic & details | 40% | +| Tests & coverage | 20% | +| Final review & summary | 10% | diff --git a/.agents/skills/code-reviewer/references/spec-compliance-review.md b/.agents/skills/code-reviewer/references/spec-compliance-review.md new file mode 100644 index 0000000..be4cb7e --- /dev/null +++ b/.agents/skills/code-reviewer/references/spec-compliance-review.md @@ -0,0 +1,258 @@ +# Spec Compliance Review + +--- + +## Two-Stage Review Architecture + +``` + ┌─────────────────────┐ + │ Implementation │ + └──────────┬──────────┘ + │ + ┌──────────▼──────────┐ + │ STAGE 1: Spec │ + │ Compliance Review │ + └──────────┬──────────┘ + │ + ┌────────────────┴────────────────┐ + │ │ + ┌───────▼───────┐ ┌────────▼────────┐ + │ ✗ Issues │ │ ✓ Compliant │ + │ Found │ │ │ + └───────┬───────┘ └────────┬────────┘ + │ │ + │ ┌────────▼────────┐ + │ │ STAGE 2: Code │ + │ │ Quality Review │ + │ └────────┬────────┘ + │ │ + │ ┌─────────────┴─────────────┐ + │ │ │ + │ ┌───────▼───────┐ ┌────────▼────────┐ + │ │ ✗ Issues │ │ ✓ Approved │ + │ │ Found │ │ │ + │ └───────┬───────┘ └─────────────────┘ + │ │ + └────────────────────┴────────────────────┐ + │ + ┌─────────▼─────────┐ + │ Return to Author │ + └───────────────────┘ +``` + +**Critical:** Complete Stage 1 (spec compliance) BEFORE Stage 2 (code quality). Never review code quality for functionality that doesn't meet the specification. + +--- + +## Stage 1: Spec Compliance Review + +### Core Directive + +> "The implementer finished suspiciously quickly. Their report may be incomplete, inaccurate, or optimistic." + +Approach every review with professional skepticism. Verify claims independently. + +### The Three Verification Categories + +#### Category 1: Missing Requirements + +**Check for features that were requested but not implemented.** + +| Question | How to Verify | +|----------|---------------| +| Did they skip requested features? | Compare PR to original requirements line by line | +| Are edge cases handled? | Check error paths, empty states, boundaries | +| Were error scenarios addressed? | Look for try/catch, error boundaries, validation | +| Is the happy path complete? | Trace through primary use case manually | + +```markdown +## Example Review Finding + +**Missing Requirement:** Issue #42 requested "password must be at least 8 characters" + +**Found in code:** +```typescript +// No length validation present +function validatePassword(password: string) { + return password.length > 0; // Only checks non-empty +} +``` + +**Status:** ❌ Incomplete - minimum length validation missing +``` + +#### Category 2: Unnecessary Additions + +**Check for scope creep and over-engineering.** + +| Question | How to Verify | +|----------|---------------| +| Features beyond specification? | Compare to original requirements | +| Over-engineering? | Is complexity justified by requirements? | +| Premature optimization? | Is performance cited without measurements? | +| Unrequested abstractions? | Are there helpers/utils for one-time use? | + +```markdown +## Example Review Finding + +**Unnecessary Addition:** Added caching layer not in requirements + +**Found in code:** +```typescript +// Original requirement: "Fetch user by ID" +// Actual implementation: +class CachedUserRepository { // Not requested + private cache = new Map(); + private ttl = 60000; + + async getUser(id: string) { + if (this.cache.has(id)) { ... } + // 50 lines of cache logic + } +} +``` + +**Status:** ⚠️ Scope creep - discuss before merging +``` + +#### Category 3: Interpretation Gaps + +**Check for misunderstandings of requirements.** + +| Question | How to Verify | +|----------|---------------| +| Different understanding of requirements? | Ask author to explain their interpretation | +| Unclarified assumptions? | Look for comments like "assuming..." | +| Ambiguous specs resolved incorrectly? | Compare to similar existing features | + +```markdown +## Example Review Finding + +**Interpretation Gap:** "Sort by date" implemented as ascending + +**Requirement stated:** "Sort by date" (ambiguous) + +**Author implemented:** Oldest first (ascending) + +**Expected:** Most recent first is typical UX pattern + +**Status:** ❓ Clarify - which sort order was intended? +``` + +--- + +## Why Order Matters + +### Stage 1 Must Come First + +| Scenario | Waste from Wrong Order | +|----------|------------------------| +| Skip Stage 1 | Review 500 lines of code quality, then discover wrong feature was built | +| Stage 2 First | Suggest refactoring, then realize the code shouldn't exist | +| Combined | Mix concerns, miss systematic issues | + +### Separation of Concerns + +- **Stage 1 (Spec):** Does it do the right thing? +- **Stage 2 (Quality):** Does it do the thing right? + +Code quality review is meaningless if the code doesn't implement the correct functionality. + +--- + +## Spec Compliance Checklist + +### Before You Start + +- [ ] Read the original issue/ticket completely +- [ ] Identify all explicit requirements +- [ ] Identify implicit requirements from context +- [ ] Note any acceptance criteria listed + +### During Review + +**Missing Requirements:** +- [ ] All required features present +- [ ] Edge cases covered (empty, null, max values) +- [ ] Error handling as specified +- [ ] Happy path fully functional +- [ ] UI matches mockups/specs if provided + +**Unnecessary Additions:** +- [ ] No unrequested features +- [ ] No speculative abstractions +- [ ] No premature optimizations +- [ ] Scope matches requirements exactly + +**Interpretation Gaps:** +- [ ] Author's understanding matches spec +- [ ] Ambiguities resolved correctly +- [ ] Assumptions are documented and valid +- [ ] Behavior matches similar existing features + +### After Review + +- [ ] Document all findings with file:line references +- [ ] Categorize as missing/unnecessary/interpretation +- [ ] Prioritize: blocking vs. non-blocking issues + +--- + +## Output Format + +### Compliant Result + +```markdown +## Spec Compliance Review: ✅ PASS + +All requirements verified: +- ✅ User can upload profile image (req #1) +- ✅ Image resized to 200x200 (req #2) +- ✅ Invalid formats rejected with error message (req #3) +- ✅ Progress indicator during upload (req #4) + +**Proceed to:** Code Quality Review +``` + +### Issues Found + +```markdown +## Spec Compliance Review: ❌ ISSUES FOUND + +### Missing Requirements + +1. **Progress indicator not implemented** (req #4) + - File: `ProfileUpload.tsx` + - Expected: Progress bar during upload + - Found: No progress indication + +2. **Error messages not user-friendly** (req #3) + - File: `ProfileUpload.tsx:45` + - Expected: "Please upload a JPG or PNG file" + - Found: "Error: INVALID_FORMAT" + +### Unnecessary Additions + +1. **Image cropping feature not requested** + - File: `ImageCropper.tsx` (new file, 150 lines) + - Impact: Adds complexity, delays delivery + - Recommendation: Remove or create separate PR + +**Action Required:** Address missing requirements before code quality review +``` + +--- + +## Common Mistakes to Avoid + +| Mistake | Why It's Wrong | +|---------|----------------| +| Reviewing code style before spec compliance | Wasted effort if wrong thing was built | +| Assuming spec was followed | Verify independently | +| Skipping edge cases | Bugs hide in boundaries | +| Accepting "we can add it later" | Technical debt accumulates | +| Missing scope creep | Unreviewed code enters codebase | + +--- + +*Content adapted from [obra/superpowers](https://github.com/obra/superpowers) by Jesse Vincent (@obra), MIT License.* diff --git a/.cursor/skills/code-reviewer b/.cursor/skills/code-reviewer new file mode 120000 index 0000000..4cdfd00 --- /dev/null +++ b/.cursor/skills/code-reviewer @@ -0,0 +1 @@ +../../.agents/skills/code-reviewer \ No newline at end of file diff --git a/.qwen/skills/code-reviewer b/.qwen/skills/code-reviewer new file mode 120000 index 0000000..4cdfd00 --- /dev/null +++ b/.qwen/skills/code-reviewer @@ -0,0 +1 @@ +../../.agents/skills/code-reviewer \ No newline at end of file diff --git a/CODE_REVIEW.md b/CODE_REVIEW.md new file mode 100644 index 0000000..7e78490 --- /dev/null +++ b/CODE_REVIEW.md @@ -0,0 +1,438 @@ +# Code Review: Candle Annotator + +**Date**: 2026-02-17 +**Reviewer**: Claude Code (Automated Audit) +**Scope**: Full codebase - frontend (Next.js), backend (FastAPI), configuration, deployment +**Verdict**: **Request Changes** - Critical and major issues must be fixed before production deployment. + +--- + +## Executive Summary + +85 issues identified across the entire codebase: **15 Critical**, **30 Major**, **40 Minor**. + +The application has **zero SQL injection vulnerabilities** (Drizzle ORM and SQLAlchemy parameterize all queries), good error handling structure (every route has try/catch), and well-implemented timeouts on proxy routes. + +However, the codebase has **no authentication anywhere**, hardcoded credentials in committed files, path traversal vectors, unsafe deserialization, and numerous frontend race conditions/memory leaks. + +**Zero test files** exist anywhere in the codebase. + +--- + +## Table of Contents + +1. [Critical Issues](#1-critical-issues) +2. [Major Issues](#2-major-issues) +3. [Minor Issues](#3-minor-issues) +4. [Positive Feedback](#4-positive-feedback) +5. [Prioritized Fix Plan](#5-prioritized-fix-plan) + +--- + +## 1. Critical Issues + +### C-1. No Authentication on Any Endpoint (Frontend + Backend) + +- **Files**: All 27 API route files in `src/app/api/`, all FastAPI endpoints in `services/ml/app/main.py` +- **Impact**: Anyone with network access can read all data, delete all charts/annotations, upload files, trigger training, load arbitrary models. +- **Fix**: Add Next.js middleware (`src/middleware.ts`) with API key or session auth. Add FastAPI `Depends()` for API key auth on the ML service. At minimum, protect write/delete/training operations. + +### C-2. SSRF / Path Traversal via `run_id` in Training Runs DELETE + +- **File**: `src/app/api/training/runs/[run_id]/route.ts:15` +- **Code**: `` fetch(`${INFERENCE_API_URL}/training/runs/${run_id}`) `` +- **Impact**: A crafted `run_id` like `../../admin/delete-all` traverses to arbitrary internal endpoints. +- **Fix**: Validate `run_id` matches `/^[a-zA-Z0-9_-]+$/` before interpolation. + +### C-3. `.env` File NOT Gitignored - Credentials in Repo + +- **File**: `.gitignore` only ignores `.env*.local`, not `.env` +- **Impact**: Database credentials (`ml_user:ml_password`) in `DATABASE_URL` are committed to git. +- **Fix**: Add `.env` to `.gitignore`, remove `.env` from git history with `git rm --cached .env`. + +### C-4. Hardcoded Database Credentials Everywhere + +- **Files**: `docker-compose.yml:10,37,73-74`, `services/ml/app/db.py:18-31`, `.env.example:3` +- **Impact**: `ml_user` / `ml_password` in plaintext in committed files. +- **Fix**: Use Docker secrets or `${POSTGRES_PASSWORD}` interpolation from `.env`. Remove SQL comment with credentials from `db.py`. Replace real creds in `.env.example` with placeholders. + +### C-5. PostgreSQL Port Exposed to Host + +- **File**: `docker-compose.yml:69` - `ports: "5432:5432"` +- **Impact**: Combined with weak hardcoded password, database is directly accessible externally. +- **Fix**: Remove port mapping or bind to `127.0.0.1:5432:5432`. + +### C-6. No File Upload Size Limit + +- **File**: `src/app/api/upload/route.ts:27-39` +- **Impact**: Denial of service via memory exhaustion or database flooding with unlimited CSV rows. +- **Fix**: Add explicit `file.size` check (e.g., reject > 10MB), limit row count inserted. + +### C-7. Unsafe Pickle Deserialization via `joblib.load` + +- **File**: `services/ml/app/main.py:266` +- **Impact**: `joblib.load(model_path)` uses pickle internally - can execute arbitrary code if a malicious `.pkl` is placed in `models/`. +- **Fix**: Add SHA256 integrity checks for model files. Consider ONNX/safetensors format. + +### C-8. CORS Wildcard with Credentials on ML Service + +- **File**: `services/ml/app/main.py:51-57` +- **Code**: `allow_origins=["*"]` + `allow_credentials=True` +- **Impact**: API accessible from any origin with credentials. +- **Fix**: Replace `*` with explicit origins like `["http://localhost:3000"]`. + +### C-9. Stale Closure in `fetchPredictions` + +- **File**: `src/app/page.tsx:552` +- **Impact**: `predictionState.modelInfo` captured in `generateCacheKey` (line 489) can be stale, producing incorrect cache keys and showing wrong predictions. +- **Fix**: Extract `modelInfo` into its own `useState` or use a ref for model version in `generateCacheKey`. + +### C-10. Race Condition in `fetchPredictions` - No AbortController + +- **File**: `src/app/page.tsx:494-552` +- **Impact**: Rapid clicks on "Run on Visible" causes earlier responses to resolve after later ones, overwriting correct state. Same for `handleFetchBatchPredictions` (lines 600-663). +- **Fix**: Use `AbortController` to cancel previous in-flight requests, or track a request ID and discard stale results. + +### C-11. Stale Closure in CandleChart Click Handler + +- **File**: `src/components/CandleChart.tsx:572-971` +- **Impact**: Async click handler captures `drawingState`, `selectedLineId`, `dragState`, `annotations` from closure. 12+ item dependency array causes excessive re-subscriptions and stale values between re-subscriptions. +- **Fix**: Use refs for frequently-changing values that the click handler reads, reducing re-subscription frequency. + +### C-12. Pervasive `any` Types Disable Type Safety + +- **Files**: `page.tsx:134,147,196-198`, `CandleChart.tsx:62,389`, `SpanAnnotationManager.tsx:25`, `Toolbox.tsx:23`, `SpanAnnotationList.tsx:15`, `TalibPatternPanel.tsx:19`, `FileUpload.tsx:45`, `rectangle-drawing.ts:60,63` +- **Impact**: Type errors at runtime go undetected at compile time. Especially dangerous for `geometry`, `sub_spans`, and prediction cache values. +- **Fix**: Define proper interfaces for `geometry`, `sub_spans`. Type candle arrays properly. + +--- + +## 2. Major Issues + +### M-1. Bulk Delete All Annotations Without Guard + +- **File**: `src/app/api/annotations/route.ts:102-106` +- **Code**: `if (all === 'true')` deletes ALL annotations across ALL charts in one unauthenticated request. +- **Fix**: Require `chartId` for bulk deletes. Never allow unscoped deletion. + +### M-2. Proxy Routes Forward Arbitrary JSON Without Validation + +- **Files**: `src/app/api/predict/route.ts`, `predict/batch/route.ts`, `model/load/route.ts`, `training/start/route.ts`, `patterns/detect/route.ts` +- **Impact**: Arbitrary payloads forwarded to internal service. +- **Fix**: Validate request body with Zod schemas before forwarding. + +### M-3. Error Messages Leak Internal Details (Frontend) + +- **Files**: `health/route.ts:25`, `candles/route.ts:40`, `annotations/route.ts:35,86`, `annotations/[id]/route.ts:47,83`, `upload/route.ts:135,153`, `export/route.ts:67`, `span-annotations/export/route.ts:123` +- **Impact**: Raw `error.message` returned to clients can reveal table names, paths, connection strings. +- **Fix**: Return generic error messages to clients. Log full errors server-side only. + +### M-4. Error Messages Leak Internal Details (Backend) + +- **File**: `services/ml/app/main.py:640,778,1091,1134,1199,1296` +- **Impact**: Exception details including tracebacks returned to clients. +- **Fix**: Return generic messages. Log details server-side. + +### M-5. Race Condition in Chart Cascade Delete + +- **File**: `src/app/api/charts/[id]/route.ts:43-45` +- **Impact**: Three separate DELETE queries without a transaction. +- **Fix**: Wrap in `db.transaction(async (tx) => { ... })`. + +### M-6. Missing Span Annotations Deletion in Chart Delete + +- **File**: `src/app/api/charts/[id]/route.ts:42-45` +- **Impact**: Deletes annotations and candles but NOT `spanAnnotations` - FK violation or orphans. +- **Fix**: Add `db.delete(spanAnnotations).where(eq(spanAnnotations.chart_id, chartId))`. + +### M-7. Race Condition in Unique Chart Name Generation + +- **File**: `src/app/api/upload/route.ts:7-25` +- **Impact**: Concurrent uploads with same filename can cause unique constraint violation. +- **Fix**: Handle constraint error with retry, or use database-level upsert. + +### M-8. Path Traversal via `run_id` in Model Load/Delete (Python) + +- **File**: `services/ml/app/main.py:1203,1312` +- **Code**: `Path("models") / f"{run_id}.pkl"` - not sanitized. +- **Fix**: Validate UUID format, use `Path.resolve()` and verify within expected directory. + +### M-9. Dynamic Module Import from Config + +- **File**: `services/ml/features/custom_loader.py:54` +- **Code**: `importlib.import_module(feature_path)` from YAML config. +- **Fix**: Restrict to a whitelist of allowed module paths. + +### M-10. No Resource Limits on Training + +- **File**: `services/ml/app/main.py:907-1030` +- **Impact**: Loads entire dataset into memory, no timeout, daemon thread can corrupt models. +- **Fix**: Add file size check, training timeout, consider Celery/RQ task queue. + +### M-11. Thread Safety Issue with Model State + +- **File**: `services/ml/app/main.py:59-74,579-588,1330` +- **Impact**: Model read during prediction has no lock, but writes use `_model_swap_lock`. +- **Fix**: Use same lock for reads, or atomic reference swap pattern. + +### M-12. ML Service Container Runs as Root + +- **File**: `services/ml/Dockerfile` - no `USER` directive. +- **Fix**: Add `RUN useradd -m -r appuser && USER appuser`. + +### M-13. MLflow + ML Service Ports Exposed Without Auth + +- **File**: `docker-compose.yml:31,54-55` +- **Fix**: Remove port mappings or bind to `127.0.0.1`. + +### M-14. TA-Lib Downloaded Over HTTP Without Checksum + +- **File**: `services/ml/Dockerfile:10` +- **Fix**: Use HTTPS, add `sha256sum -c` verification. + +### M-15. No Security Response Headers (CSP, X-Frame-Options) + +- **File**: `next.config.js` +- **Fix**: Add `headers()` function with security headers. + +### M-16. Unvalidated JSONB Fields Accept Arbitrary JSON + +- **Files**: `annotations/route.ts`, `span-annotations/route.ts` (POST/PATCH) +- **Impact**: Storage abuse, deeply nested JSON DoS. +- **Fix**: Validate JSON structure and impose size limits. + +### M-17. Full `node_modules` Copied to Production Image + +- **File**: `Dockerfile:29` +- **Fix**: Remove the COPY line - standalone output bundles what it needs. + +### M-18. `@types/*` in Production Dependencies + +- **File**: `package.json:23-27` +- **Fix**: Move `@types/*`, `typescript`, `eslint`, `autoprefixer`, `postcss` to `devDependencies`. + +### M-19. Missing `response.ok` Checks Before `.json()` (page.tsx) + +- **File**: `src/app/page.tsx:214,230,245,257` +- **Impact**: If API returns 500 with non-JSON body, uncaught exception and silent failure. +- **Fix**: Add `if (!response.ok) throw new Error(...)` before parsing JSON. + +### M-20. Missing `response.ok` Checks Before `.json()` (CandleChart) + +- **File**: `src/components/CandleChart.tsx:163-164,178-179,192-193` +- **Fix**: Check response status before parsing. + +### M-21. Memory Leak - SpanAnnotationManager Preview Primitive + +- **File**: `src/components/SpanAnnotationManager.tsx:305-316` +- **Impact**: New `SpanRectanglePrimitive` on every mouse move, state/effect feedback loop. +- **Fix**: Use a ref for the preview primitive instead of state. + +### M-22. Duplicate Interface Definitions Across 6 Files + +- **Files**: `page.tsx:123-161`, `CandleChart.tsx:21-76`, `SpanAnnotationManager.tsx:8-39`, `Toolbox.tsx:9-50`, `SpanAnnotationList.tsx:6-29`, `SpanPopover.tsx:25-34` +- **Impact**: Maintenance burden and drift bugs. +- **Fix**: Centralize shared interfaces in `/src/types/` and import everywhere. + +### M-23. Chart Re-creation on Theme Change Destroys State + +- **File**: `src/components/CandleChart.tsx:333` +- **Impact**: Theme toggle destroys entire chart and re-creates it - loses all primitives, causes flicker. +- **Fix**: Use `chart.applyOptions()` to update theme colors without re-creating. + +### M-24. Unbounded Prediction Cache + +- **File**: `src/app/page.tsx:195-199` +- **Impact**: `predictionCacheRef` Map grows without limit. +- **Fix**: Implement LRU cache or limit to fixed number of entries. + +### M-25. `fitContent()` Called on Every Span Change + +- **File**: `src/components/SpanAnnotationManager.tsx:160` +- **Impact**: Resets user zoom/scroll position on every span annotation change. +- **Fix**: Remove from reconciliation effect. Only call on initial load. + +### M-26. No Database SSL/TLS + +- **Files**: `.env:3`, `docker-compose.yml:10,37` +- **Fix**: For production, add `?sslmode=require` to connection strings. + +### M-27. `eslint-disable` Suppressing Missing Dependencies + +- **File**: `src/components/TrainingPanel.tsx:128-129` +- **Fix**: Include stable callbacks in dependency array and remove the suppress. + +### M-28. Hardcoded DB Credentials in Python Source + +- **File**: `services/ml/app/db.py:18-31` +- **Fix**: Remove SQL comment with credentials. Require env vars, fail fast if missing. + +### M-29. Health Check Fakes MLflow/DB Status + +- **File**: `services/ml/app/main.py:396-409` +- **Fix**: Implement actual connectivity checks (`SELECT 1` for DB). + +### M-30. Delete All Annotations Has No Confirmation Dialog + +- **File**: `src/app/page.tsx:412-425` +- **Fix**: Add confirmation dialog before destructive action. + +--- + +## 3. Minor Issues + +### Configuration & Deployment + +| # | File | Issue | Fix | +|---|------|-------|-----| +| m-1 | All routes | No rate limiting on any endpoint | Add rate limiting middleware | +| m-2 | Dockerfiles | Base images not pinned to digest | Use `@sha256:` | +| m-3 | (missing) | No `.dockerignore` file | Create with `.git`, `.env`, `node_modules` | +| m-4 | `.gitignore` | `models/` and `.pkl` files not ignored | Add `models/`, `*.pkl` | +| m-5 | `Dockerfile` vs `docker-compose.yml` | Healthcheck tool mismatch (wget vs curl) | Use same tool | +| m-6 | `package.json` | Wide dependency ranges with `^` | Pin critical deps | +| m-7 | `Dockerfile:38` | `EURUSD.csv` data file baked into image | Mount at runtime | + +### API Routes + +| # | File | Issue | Fix | +|---|------|-------|-----| +| m-8 | Multiple routes | `parseInt()` missing radix or NaN check | Always `parseInt(v, 10)` + `isNaN()` guard | +| m-9 | `upload/route.ts:30-42` | No file type validation on upload | Check MIME type/extension | +| m-10 | Export routes | CSV injection (values starting with `=+@-`) | Prefix dangerous cells with `'` | +| m-11 | Multiple routes | `console.error` logs full error objects | Use structured logger | +| m-12 | `src/lib/db/migrate.ts` | Dead SQLite migration code | Remove if unused | + +### Python ML Service + +| # | File | Issue | Fix | +|---|------|-------|-----| +| m-13 | `app/main.py:307` | Deprecated `@app.on_event("startup")` | Migrate to `lifespan` pattern | +| m-14 | `app/db.py:45` | Deprecated `declarative_base()` | Use `class Base(DeclarativeBase)` | +| m-15 | `app/main.py:325,1000,1019,1080` | Deprecated `datetime.utcnow()` | Use `datetime.now(datetime.UTC)` | +| m-16 | `app/patterns.py` + `generate_talib_annotations.py` | Duplicate `TALIB_PATTERNS` dict | Import from one location | +| m-17 | `app/db.py:99-111` | `get_db_session()` leaks sessions (dead code) | Remove | +| m-18 | `app/main.py:129-134` | No input size limits on batch prediction | Add max date range validation | +| m-19 | `app/main.py:93` | No validation candles are time-sorted | Add sort validation | +| m-20 | `features/talib_features.py:169-190` | Missing fallback return for volume indicators | Add default return/raise | +| m-21 | `pyproject.toml:32` | Dead `inference*` package reference | Remove | +| m-22 | `app/main.py:9` | Inconsistent `uuid as uuid_lib` import | Use standard `import uuid` | + +### React Components + +| # | File | Issue | Fix | +|---|------|-------|-----| +| m-23 | `Toolbox.tsx:4` | Unused imports: `TrendingUp`, `ChevronUp` | Remove | +| m-24 | `CandleChart.tsx:452,569,837` | Magic numbers (8px, 60s, hardcoded 1-min interval) | Extract to named constants | +| m-25 | Multiple files | Missing ARIA labels on buttons, dropdowns, modals | Add `aria-label`, `aria-pressed` | +| m-26 | `ChartSelector.tsx:38-109` | Custom dropdown not keyboard accessible | Use Radix DropdownMenu | +| m-27 | `KeyboardShortcutsModal.tsx:38-77` | Modal not focus-trapped, missing `role="dialog"` | Add focus trapping | +| m-28 | `SpanAnnotationList.tsx:110-115` | Falsy check on `confidence` fails for value `0` | Use `!= null` check | +| m-29 | `page.tsx:56-85` | O(n*m) in `detectDisagreements` | Use sweep-line algorithm | +| m-30 | `page.tsx:49-53` | Dead filter code (TODO comment, no-op) | Implement or remove | +| m-31 | `CandleChart.tsx:125` | `new Set()` default prop recreated each render | Use module-level constant | +| m-32 | `page.tsx` | ~20 props drilled to CandleChart | Consider React Context | +| m-33 | `ui/tooltip.tsx` | Tooltip component is a no-op placeholder | Use Radix Tooltip or remove | +| m-34 | `SpanAnnotationList.tsx:105` | White text on light label colors - poor contrast | Dynamic text color from luminance | +| m-35 | `page.tsx:892-906` | Settings menu uses `` tags instead of Next.js `Link` | Use `Link` component | +| m-36 | `Toolbox.tsx:246-247` | `@ts-ignore` for CSS custom property | Use type assertion | +| m-37 | `SpanPopover.tsx:114-117` | Manual Escape handler conflicts with Radix Dialog | Remove manual handler | +| m-38 | `TalibPatternPanel.tsx:39` | Re-fetches when API returns empty array | Use `hasLoaded` flag | +| m-39 | `CandleChart.tsx:202-221` | `useImperativeHandle` missing dependency array | Add `[candles]` dep | +| m-40 | `CandleChart.tsx:1059-1110` | Missing `activeChartId` in primitive cleanup effects | Add to dependency arrays | + +--- + +## 4. Positive Feedback + +- **Zero SQL injection** - Drizzle ORM parameterizes all frontend queries; SQLAlchemy does the same on the backend. +- **Consistent error handling** - Every API route has try/catch with appropriate HTTP status codes. +- **Good timeout management** - All proxy routes use `AbortController` with configurable timeouts. +- **Solid Docker practices for Next.js** - Multi-stage build, non-root user (`nextjs`), healthcheck, standalone output. +- **Clean project structure** - Clear separation between frontend, API routes, ML service, and database layers. +- **Pydantic validation on ML endpoints** - Request/response models are well-defined with proper types. +- **Thread-safe training state** - Uses `threading.Lock` for `active_training_run_id` management. +- **Parameterized queries throughout** - Both Drizzle and SQLAlchemy prevent injection by design. +- **Proper OHLC data model** - Schema is well-designed with unique constraints and proper FK relationships. + +--- + +## 5. Prioritized Fix Plan + +### Phase 1: Security Critical (Immediate) + +| Task | Files to Change | Effort | +|------|----------------|--------| +| Add `.env` to `.gitignore`, remove from git history | `.gitignore` | Low | +| Replace hardcoded creds with env var interpolation | `docker-compose.yml` | Low | +| Remove real creds from `.env.example` and `db.py` comments | `.env.example`, `services/ml/app/db.py` | Low | +| Bind DB port to `127.0.0.1` only | `docker-compose.yml` | Low | +| Validate `run_id` format in training routes | `src/app/api/training/runs/[run_id]/route.ts`, `services/ml/app/main.py` | Low | +| Add file upload size limit | `src/app/api/upload/route.ts` | Low | +| Fix CORS on ML service | `services/ml/app/main.py` | Low | + +### Phase 2: Before Production + +| Task | Files to Change | Effort | +|------|----------------|--------| +| Add API key authentication middleware | `src/middleware.ts` (new), ML service | Medium | +| Add Zod input validation on proxy routes | 5 proxy route files | Medium | +| Wrap chart cascade delete in transaction + add span delete | `src/app/api/charts/[id]/route.ts` | Low | +| Stop leaking error details to clients | 7+ route files, ML `main.py` | Medium | +| Add security response headers | `next.config.js` | Low | +| Fix ML Dockerfile to run as non-root | `services/ml/Dockerfile` | Low | +| Create `.dockerignore` | `.dockerignore` (new) | Low | +| Remove bulk delete-all without chartId guard | `src/app/api/annotations/route.ts` | Low | +| Add model file integrity checks | `services/ml/app/main.py` | Medium | + +### Phase 3: Code Quality & Frontend Fixes + +| Task | Files to Change | Effort | +|------|----------------|--------| +| Add AbortController to prediction fetching | `src/app/page.tsx` | Medium | +| Fix SpanAnnotationManager preview primitive memory leak | `src/components/SpanAnnotationManager.tsx` | Medium | +| Use `chart.applyOptions()` for theme changes | `src/components/CandleChart.tsx` | Medium | +| Add `response.ok` checks on all fetch calls | `page.tsx`, `CandleChart.tsx` | Low | +| Centralize duplicate type definitions | `src/types/` (new files), 6 component files | Medium | +| Remove `fitContent()` from reconciliation effect | `src/components/SpanAnnotationManager.tsx` | Low | +| Use refs for click handler closure values | `src/components/CandleChart.tsx` | Medium | +| Move dev deps to `devDependencies` | `package.json` | Low | +| Remove dead code (`migrate.ts`, `get_db_session`, dead filter) | Multiple files | Low | + +### Phase 4: Hardening & Polish + +| Task | Files to Change | Effort | +|------|----------------|--------| +| Add rate limiting | Middleware | Medium | +| Add confirmation dialog for delete-all | `src/app/page.tsx` | Low | +| Fix accessibility (ARIA labels, focus trapping, keyboard nav) | Multiple components | Medium | +| Add CSV injection protection to exports | 3 export route files | Low | +| Pin Docker images to digests | Dockerfiles | Low | +| Implement proper health checks (ML service) | `services/ml/app/main.py` | Low | +| Replace deprecated Python APIs | `services/ml/app/main.py`, `db.py` | Low | +| Extract magic numbers to constants | `CandleChart.tsx`, `page.tsx` | Low | +| Write tests | New test files | High | + +--- + +## Test Coverage Assessment + +- [ ] No unit tests for API routes +- [ ] No integration tests +- [ ] No ML model/pipeline tests +- [ ] No frontend component tests +- [ ] No end-to-end tests + +--- + +## Summary by Area + +| Category | Critical | Major | Minor | Total | +|----------|----------|-------|-------|-------| +| API Routes (TS) | 3 | 7 | 5 | 15 | +| React Components | 4 | 9 | 18 | 31 | +| Python ML Service | 2 | 7 | 10 | 19 | +| Config & Deployment | 3 | 7 | 7 | 17 | +| **Total** | **12** | **30** | **40** | **82** | + +> Note: Some issues overlap across categories (e.g., auth affects both frontend and backend). diff --git a/package.json b/package.json index d358be4..e527b97 100644 --- a/package.json +++ b/package.json @@ -9,8 +9,7 @@ "start": "next start", "lint": "next lint", "import-annotations": "tsx scripts/import_talib_annotations.ts", - "list-charts": "tsx scripts/list_charts.ts", - "migrate:sqlite-to-postgres": "tsx scripts/migrate-sqlite-to-postgres.ts" + "list-charts": "tsx scripts/list_charts.ts" }, "keywords": [], "author": "", diff --git a/scripts/migrate-sqlite-to-postgres.ts b/scripts/migrate-sqlite-to-postgres.ts deleted file mode 100644 index 1da6145..0000000 --- a/scripts/migrate-sqlite-to-postgres.ts +++ /dev/null @@ -1,499 +0,0 @@ -#!/usr/bin/env tsx -/** - * SQLite to PostgreSQL Migration Script - * - * Migrates data from the legacy SQLite database to PostgreSQL. - * - * Features: - * - Migrates all 6 tables: charts, candles, annotation_types, annotations, span_label_types, span_annotations - * - Applies type conversions: integer timestamps → PostgreSQL timestamps, integer booleans → booleans, text JSON → jsonb - * - Idempotent: Can be run multiple times safely (skips existing data by default) - * - Supports --clear flag to delete all data before migrating - * - * Usage: - * npm run migrate:sqlite-to-postgres # Migrate (skip existing) - * npm run migrate:sqlite-to-postgres -- --clear # Clear and re-migrate - * npm run migrate:sqlite-to-postgres -- --help # Show help - */ - -import Database from 'better-sqlite3'; -import { drizzle as drizzlePg } from 'drizzle-orm/node-postgres'; -import { Pool } from 'pg'; -import { sql } from 'drizzle-orm'; -import * as schema from '../src/lib/db/schema'; - -// Command-line arguments -const args = process.argv.slice(2); -const shouldClear = args.includes('--clear'); -const showHelp = args.includes('--help') || args.includes('-h'); - -if (showHelp) { - console.log(` -SQLite to PostgreSQL Migration Script - -Usage: - npm run migrate:sqlite-to-postgres # Migrate (skip existing data) - npm run migrate:sqlite-to-postgres -- --clear # Clear all data before migrating - npm run migrate:sqlite-to-postgres -- --help # Show this help - -Environment Variables: - DATABASE_PATH (default: ./data/candles.db) # SQLite database path - DATABASE_URL # PostgreSQL connection string - `); - process.exit(0); -} - -// Configuration -const SQLITE_PATH = process.env.DATABASE_PATH || './data/candles.db'; -const POSTGRES_URL = process.env.DATABASE_URL; - -if (!POSTGRES_URL) { - console.error('ERROR: DATABASE_URL environment variable is required'); - process.exit(1); -} - -console.log('='.repeat(60)); -console.log('SQLite to PostgreSQL Migration'); -console.log('='.repeat(60)); -console.log(`SQLite source: ${SQLITE_PATH}`); -console.log(`PostgreSQL target: ${POSTGRES_URL.replace(/:[^:@]+@/, ':****@')}`); -console.log(`Mode: ${shouldClear ? 'CLEAR AND MIGRATE' : 'SKIP EXISTING'}`); -console.log('='.repeat(60)); -console.log(); - -// Initialize databases -const sqlite = new Database(SQLITE_PATH, { readonly: true }); -const pgPool = new Pool({ connectionString: POSTGRES_URL }); -const pg = drizzlePg(pgPool, { schema }); - -interface MigrationStats { - table: string; - sourceCount: number; - migratedCount: number; - skippedCount: number; - errorCount: number; -} - -const stats: MigrationStats[] = []; - -/** - * Convert SQLite integer timestamp (Unix seconds) to JavaScript Date - */ -function sqliteTimestampToDate(timestamp: number | null): Date | undefined { - if (!timestamp) return undefined; - return new Date(timestamp * 1000); -} - -function sqliteTimestampToDateRequired(timestamp: number | null): Date { - if (!timestamp) throw new Error(`Required timestamp is null/zero: ${timestamp}`); - return new Date(timestamp * 1000); -} - -/** - * Convert SQLite integer boolean (0/1) to JavaScript boolean - */ -function sqliteBooleanToBoolean(value: number | null): boolean { - return value === 1; -} - -/** - * Parse SQLite JSON text to object - */ -function sqliteJsonToObject(json: string | null): any { - if (!json) return null; - try { - return JSON.parse(json); - } catch (e) { - console.warn('Failed to parse JSON:', json); - return null; - } -} - -/** - * Clear all data from PostgreSQL tables - */ -async function clearPostgresData() { - console.log('Clearing PostgreSQL data...'); - - const tables = [ - 'span_annotations', - 'annotations', - 'candles', - 'span_label_types', - 'annotation_types', - 'charts', - ]; - - for (const table of tables) { - await pgPool.query(`DELETE FROM ${table}`); - console.log(` Cleared table: ${table}`); - } - - console.log('All tables cleared.\n'); -} - -/** - * Migrate charts table - */ -async function migrateCharts() { - const tableName = 'charts'; - console.log(`Migrating ${tableName}...`); - - const rows = sqlite.prepare('SELECT * FROM charts').all() as any[]; - - let migrated = 0; - let skipped = 0; - let errors = 0; - - for (const row of rows) { - try { - // Check if already exists - if (!shouldClear) { - const existing = await pg.query.charts.findFirst({ - where: sql`${schema.charts.id} = ${row.id}`, - }); - - if (existing) { - skipped++; - continue; - } - } - - await pg.insert(schema.charts).values({ - id: row.id, - name: row.name, - created_at: sqliteTimestampToDateRequired(row.created_at), - }); - - migrated++; - } catch (e: any) { - console.error(` Error migrating chart ${row.id}:`, e.message); - errors++; - } - } - - stats.push({ table: tableName, sourceCount: rows.length, migratedCount: migrated, skippedCount: skipped, errorCount: errors }); - console.log(` ${tableName}: ${migrated} migrated, ${skipped} skipped, ${errors} errors\n`); -} - -/** - * Migrate candles table - */ -async function migrateCandles() { - const tableName = 'candles'; - console.log(`Migrating ${tableName}...`); - - const rows = sqlite.prepare('SELECT * FROM candles').all() as any[]; - - let migrated = 0; - let skipped = 0; - let errors = 0; - - for (const row of rows) { - try { - // Check if already exists - if (!shouldClear) { - const existing = await pg.query.candles.findFirst({ - where: sql`${schema.candles.id} = ${row.id}`, - }); - - if (existing) { - skipped++; - continue; - } - } - - await pg.insert(schema.candles).values({ - id: row.id, - chart_id: row.chart_id, - time: sqliteTimestampToDateRequired(row.time), - open: row.open, - high: row.high, - low: row.low, - close: row.close, - }); - - migrated++; - } catch (e: any) { - console.error(` Error migrating candle ${row.id}:`, e.message); - errors++; - } - } - - stats.push({ table: tableName, sourceCount: rows.length, migratedCount: migrated, skippedCount: skipped, errorCount: errors }); - console.log(` ${tableName}: ${migrated} migrated, ${skipped} skipped, ${errors} errors\n`); -} - -/** - * Migrate annotation_types table - */ -async function migrateAnnotationTypes() { - const tableName = 'annotation_types'; - console.log(`Migrating ${tableName}...`); - - const rows = sqlite.prepare('SELECT * FROM annotation_types').all() as any[]; - - let migrated = 0; - let skipped = 0; - let errors = 0; - - for (const row of rows) { - try { - // Check if already exists - if (!shouldClear) { - const existing = await pg.query.annotationTypes.findFirst({ - where: sql`${schema.annotationTypes.id} = ${row.id}`, - }); - - if (existing) { - skipped++; - continue; - } - } - - await pg.insert(schema.annotationTypes).values({ - id: row.id, - name: row.name, - display_name: row.display_name, - color: row.color, - category: row.category, - icon: row.icon, - is_active: sqliteBooleanToBoolean(row.is_active), - created_at: sqliteTimestampToDateRequired(row.created_at), - }); - - migrated++; - } catch (e: any) { - console.error(` Error migrating annotation_type ${row.id}:`, e.message); - errors++; - } - } - - stats.push({ table: tableName, sourceCount: rows.length, migratedCount: migrated, skippedCount: skipped, errorCount: errors }); - console.log(` ${tableName}: ${migrated} migrated, ${skipped} skipped, ${errors} errors\n`); -} - -/** - * Migrate annotations table - */ -async function migrateAnnotations() { - const tableName = 'annotations'; - console.log(`Migrating ${tableName}...`); - - const rows = sqlite.prepare('SELECT * FROM annotations').all() as any[]; - - let migrated = 0; - let skipped = 0; - let errors = 0; - - for (const row of rows) { - try { - // Check if already exists - if (!shouldClear) { - const existing = await pg.query.annotations.findFirst({ - where: sql`${schema.annotations.id} = ${row.id}`, - }); - - if (existing) { - skipped++; - continue; - } - } - - await pg.insert(schema.annotations).values({ - id: row.id, - chart_id: row.chart_id, - timestamp: sqliteTimestampToDateRequired(row.timestamp), - label_type: row.label_type, - geometry: sqliteJsonToObject(row.geometry), - color: row.color || '#3b82f6', - created_at: sqliteTimestampToDateRequired(row.created_at), - }); - - migrated++; - } catch (e: any) { - console.error(` Error migrating annotation ${row.id}:`, e.message); - errors++; - } - } - - stats.push({ table: tableName, sourceCount: rows.length, migratedCount: migrated, skippedCount: skipped, errorCount: errors }); - console.log(` ${tableName}: ${migrated} migrated, ${skipped} skipped, ${errors} errors\n`); -} - -/** - * Migrate span_label_types table - */ -async function migrateSpanLabelTypes() { - const tableName = 'span_label_types'; - console.log(`Migrating ${tableName}...`); - - const rows = sqlite.prepare('SELECT * FROM span_label_types').all() as any[]; - - let migrated = 0; - let skipped = 0; - let errors = 0; - - for (const row of rows) { - try { - // Check if already exists - if (!shouldClear) { - const existing = await pg.query.spanLabelTypes.findFirst({ - where: sql`${schema.spanLabelTypes.id} = ${row.id}`, - }); - - if (existing) { - skipped++; - continue; - } - } - - await pg.insert(schema.spanLabelTypes).values({ - id: row.id, - name: row.name, - display_name: row.display_name, - color: row.color, - hotkey: row.hotkey, - is_active: sqliteBooleanToBoolean(row.is_active), - sort_order: row.sort_order || 0, - created_at: sqliteTimestampToDateRequired(row.created_at), - }); - - migrated++; - } catch (e: any) { - console.error(` Error migrating span_label_type ${row.id}:`, e.message); - errors++; - } - } - - stats.push({ table: tableName, sourceCount: rows.length, migratedCount: migrated, skippedCount: skipped, errorCount: errors }); - console.log(` ${tableName}: ${migrated} migrated, ${skipped} skipped, ${errors} errors\n`); -} - -/** - * Migrate span_annotations table - */ -async function migrateSpanAnnotations() { - const tableName = 'span_annotations'; - console.log(`Migrating ${tableName}...`); - - const rows = sqlite.prepare('SELECT * FROM span_annotations').all() as any[]; - - let migrated = 0; - let skipped = 0; - let errors = 0; - - for (const row of rows) { - try { - // Check if already exists - if (!shouldClear) { - const existing = await pg.query.spanAnnotations.findFirst({ - where: sql`${schema.spanAnnotations.id} = ${row.id}`, - }); - - if (existing) { - skipped++; - continue; - } - } - - await pg.insert(schema.spanAnnotations).values({ - id: row.id, - chart_id: row.chart_id, - start_time: sqliteTimestampToDateRequired(row.start_time), - end_time: sqliteTimestampToDateRequired(row.end_time), - label: row.label, - confidence: row.confidence, - outcome: row.outcome, - notes: row.notes, - sub_spans: sqliteJsonToObject(row.sub_spans), - color: row.color || '#2196F3', - source: row.source || 'human', - model_prediction: sqliteJsonToObject(row.model_prediction), - created_at: sqliteTimestampToDateRequired(row.created_at), - }); - - migrated++; - } catch (e: any) { - console.error(` Error migrating span_annotation ${row.id}:`, e.message); - errors++; - } - } - - stats.push({ table: tableName, sourceCount: rows.length, migratedCount: migrated, skippedCount: skipped, errorCount: errors }); - console.log(` ${tableName}: ${migrated} migrated, ${skipped} skipped, ${errors} errors\n`); -} - -/** - * Print migration summary - */ -function printSummary() { - console.log('='.repeat(60)); - console.log('Migration Summary'); - console.log('='.repeat(60)); - console.log(); - console.log('Table | Source | Migrated | Skipped | Errors'); - console.log('-'.repeat(60)); - - let totalSource = 0; - let totalMigrated = 0; - let totalSkipped = 0; - let totalErrors = 0; - - for (const stat of stats) { - console.log( - `${stat.table.padEnd(24)} | ${String(stat.sourceCount).padStart(6)} | ${String(stat.migratedCount).padStart(8)} | ${String(stat.skippedCount).padStart(7)} | ${String(stat.errorCount).padStart(6)}` - ); - - totalSource += stat.sourceCount; - totalMigrated += stat.migratedCount; - totalSkipped += stat.skippedCount; - totalErrors += stat.errorCount; - } - - console.log('-'.repeat(60)); - console.log( - `${'TOTAL'.padEnd(24)} | ${String(totalSource).padStart(6)} | ${String(totalMigrated).padStart(8)} | ${String(totalSkipped).padStart(7)} | ${String(totalErrors).padStart(6)}` - ); - console.log('='.repeat(60)); - - if (totalErrors > 0) { - console.log(`\n⚠️ Migration completed with ${totalErrors} errors. Check logs above.`); - } else { - console.log('\n✅ Migration completed successfully!'); - } -} - -/** - * Main migration function - */ -async function main() { - try { - // Clear data if requested - if (shouldClear) { - await clearPostgresData(); - } - - // Migrate tables in dependency order - await migrateCharts(); - await migrateCandles(); - await migrateAnnotationTypes(); - await migrateAnnotations(); - await migrateSpanLabelTypes(); - await migrateSpanAnnotations(); - - // Print summary - printSummary(); - - } catch (error: any) { - console.error('\n❌ Migration failed:', error.message); - process.exit(1); - } finally { - // Close connections - sqlite.close(); - await pgPool.end(); - } -} - -// Run migration -main(); diff --git a/src/lib/db/migrate.ts b/src/lib/db/migrate.ts deleted file mode 100644 index 79a38ac..0000000 --- a/src/lib/db/migrate.ts +++ /dev/null @@ -1,23 +0,0 @@ -import Database from 'better-sqlite3'; -import { drizzle } from 'drizzle-orm/better-sqlite3'; -import { migrate } from 'drizzle-orm/better-sqlite3/migrator'; -import path from 'path'; -import fs from 'fs'; - -export function runMigrations() { - // Ensure data directory exists - const dataDir = path.join(process.cwd(), 'data'); - if (!fs.existsSync(dataDir)) { - fs.mkdirSync(dataDir, { recursive: true }); - } - - const dbPath = path.join(dataDir, 'candles.db'); - const sqlite = new Database(dbPath); - const db = drizzle(sqlite); - - // Run migrations - migrate(db, { migrationsFolder: './drizzle' }); - - sqlite.close(); - console.log('✅ Migrations complete'); -}