diff --git a/CLAUDE.md b/CLAUDE.md index e1890ccc..d2aeb9b6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -114,7 +114,7 @@ patherly/ │ └── tailwind.config.js ├── CLAUDE.md # This file ├── CURRENT-STATE.md # Detailed feature status -├── LESSONS-LEARNED.md # Bugs and fixes (READ THIS!) +├── LESSONS-LEARNED.md # (Deprecated — consolidated into CLAUDE.md) └── docs/plans/ # Design docs & implementation plans ``` @@ -194,8 +194,6 @@ gh run view --json jobs --jq '.jobs[] | {name: .name, conclusion: .conclusi ## Critical Lessons Learned -**Full reference:** [LESSONS-LEARNED.md](LESSONS-LEARNED.md) — read before making changes! - ### Top Gotchas (most commonly hit) **1. DateTime Handling — Always timezone-aware:** @@ -290,6 +288,22 @@ navigate(`/trees/${newTree.id}/edit`) **29. ESLint `no-unused-vars` has no `argsIgnorePattern`:** Underscore-prefixed callback params (e.g., `_count`) still trigger errors. Use `// eslint-disable-next-line @typescript-eslint/no-unused-vars` or remove the param if the type signature allows it. +**30. Alembic `env.py` must import all models:** New models won't be discovered by `--autogenerate` unless imported in `alembic/env.py`. If a migration runs but the table isn't created, check imports first. + +**31. JSONB fields — convert datetime to `.isoformat()` string:** Storing `datetime` objects directly in JSONB columns causes serialization errors. Always convert: `"timestamp": datetime.now(timezone.utc).isoformat()`. + +**32. Export pipeline order matters:** Generate export → resolve session variables → apply redaction. Redaction MUST run last or sensitive data injected via variables bypasses it. See `sessions.py` export endpoint. + +**33. Railway: `DATABASE_URL_SYNC` is a property, not an env var:** It derives from `DATABASE_URL` by replacing `postgresql+asyncpg://` with `postgresql://`. Delete any stale `DATABASE_URL_SYNC` variable in Railway dashboard. + +**34. Railway: run migrations in Docker CMD, not `releaseCommand`:** `releaseCommand` in `railway.toml` is unreliable. Use `CMD alembic upgrade head && uvicorn ...` in the Dockerfile instead. + +**35. `bcrypt==4.0.1` pinned for passlib compatibility:** Newer bcrypt versions break password hashing. Keep `bcrypt==4.0.1` and `passlib[bcrypt]==1.7.4` pinned in `requirements.txt`. + +**36. Email validator rejects `.local` TLD:** Pydantic's email validation (via `email-validator`) rejects `.local` as a reserved TLD. Use `example.com` for test/seed user emails. + +**37. First deployed user needs manual admin promotion:** New users default to `engineer` role. Promote via SQL: `UPDATE users SET role = 'admin' WHERE email = '...';` then re-login for new JWT. + --- ## RBAC & Permissions @@ -392,6 +406,6 @@ When a feature, fix, or significant piece of work is finished and merged/committ | Detailed Status | [CURRENT-STATE.md](CURRENT-STATE.md) | | Development Roadmap | [03-DEVELOPMENT-ROADMAP.md](03-DEVELOPMENT-ROADMAP.md) | | GitHub Issues | `gh issue list --state open` | -| Bugs & Fixes | [LESSONS-LEARNED.md](LESSONS-LEARNED.md) | +| Bugs & Fixes | CLAUDE.md → Critical Lessons Learned section | | Feature Specs | [04-FEATURE-SPECIFICATIONS.md](04-FEATURE-SPECIFICATIONS.md) | | Design System | [docs/plans/Frontend/DESIGN_SYSTEM_GUIDE.md](docs/plans/Frontend/DESIGN_SYSTEM_GUIDE.md) | diff --git a/CURRENT-STATE.md b/CURRENT-STATE.md index 0f5ee754..75925e64 100644 --- a/CURRENT-STATE.md +++ b/CURRENT-STATE.md @@ -75,7 +75,7 @@ ### Documentation - CLAUDE.md (project context for Claude Code) -- LESSONS-LEARNED.md (bugs and fixes reference) +- CLAUDE.md includes consolidated lessons learned (formerly LESSONS-LEARNED.md) - Design system guide, component examples - Feature specifications through Phase 4 - Rebrand implementation guide diff --git a/LESSONS-LEARNED.md b/LESSONS-LEARNED.md deleted file mode 100644 index a2fcba13..00000000 --- a/LESSONS-LEARNED.md +++ /dev/null @@ -1,932 +0,0 @@ -# Lessons Learned - -> **Purpose:** This file documents bugs, fixes, and gotchas encountered during development. -> **For Claude Code:** Read this file at the start of each session to avoid repeating past mistakes. -> **Last Updated:** February 12, 2026 - ---- - -## Environment Setup (New Machine) - -### Database Name Mismatch After Fresh Clone -**Problem:** After cloning the repo and running `docker-compose up -d`, Alembic migrations fail with `database "decision_tree" does not exist`. - -**Cause:** The `.env` file contains the old database name (`decision_tree`) but `docker-compose.yml` creates a database called `patherly`. - -**Solution:** Update `.env` to use the correct database name: -``` -DATABASE_URL=postgresql+asyncpg://postgres:postgres@localhost:5432/patherly -DATABASE_URL_SYNC=postgresql://postgres:postgres@localhost:5432/patherly -``` - -**Files affected:** `backend/.env` - ---- - -### Missing @/lib/utils (cn function) -**Problem:** Frontend fails to compile with error: `Failed to resolve import "@/lib/utils" from "src/pages/SessionDetailPage.tsx". Does the file exist?` - -**Cause:** The `src/lib/utils.ts` file wasn't committed to the repo or was missed during setup. This file provides the `cn()` utility function used for combining Tailwind classes. - -**Solution:** Create `frontend/src/lib/utils.ts`: -```typescript -import { type ClassValue, clsx } from 'clsx' -import { twMerge } from 'tailwind-merge' - -export function cn(...inputs: ClassValue[]) { - return twMerge(clsx(inputs)) -} -``` - -**Dependencies required:** `clsx` and `tailwind-merge` (already in package.json) - -**Files affected:** `frontend/src/lib/utils.ts` - ---- - -### pip install in venv doesn't need --break-system-packages -**Problem:** Confusion about whether to use `--break-system-packages` flag. - -**Clarification:** The `--break-system-packages` flag is only needed when installing packages **outside** of a virtual environment. When your venv is active (you see `(venv)` prefix), you don't need this flag. - ---- - -### New Machine Setup Checklist -When setting up development on a new machine: - -1. **Clone repo:** `git clone ` -2. **Start Docker Desktop** -3. **Start database:** `cd backend && docker-compose up -d` -4. **Fix .env database name** if it says `decision_tree` → change to `patherly` -5. **Create venv:** `python -m venv venv` -6. **Activate venv:** `.\venv\Scripts\Activate` -7. **Install backend deps:** `pip install -r requirements.txt` -8. **Run migrations:** `alembic upgrade head` -9. **Start backend:** `uvicorn app.main:app --reload` -10. **Install frontend deps:** `cd ../frontend && npm install` -11. **Create lib/utils.ts** if missing (see above) -12. **Start frontend:** `npm run dev` - ---- - -## Python / Backend - -### DateTime Timezone Handling ⚠️ CRITICAL -**Problem:** SQLAlchemy `DateTime` fields caused Internal Server Errors when mixing timezone-aware and timezone-naive datetimes. - -**Error:** `can't subtract offset-naive and offset-aware datetimes` - -**Solution:** -- Always use `DateTime(timezone=True)` in SQLAlchemy models -- Always use `datetime.now(timezone.utc)` for defaults and assignments -- Never use `datetime.utcnow()` (deprecated and timezone-naive) - -**Correct pattern:** -```python -from datetime import datetime, timezone -from sqlalchemy import DateTime - -created_at = Column(DateTime(timezone=True), default=lambda: datetime.now(timezone.utc)) -``` - -**Files affected:** All models with timestamp fields (user.py, tree.py, session.py, team.py, attachment.py) - ---- - -### pytest-asyncio Version Compatibility -**Problem:** Tests fail with `AttributeError: 'Package' object has no attribute 'obj'` - -**Cause:** Incompatibility between pytest 8.0.0 and pytest-asyncio 0.23.3 - -**Solution:** Upgrade pytest-asyncio to 0.24.0 -```powershell -pip install pytest-asyncio==0.24.0 --upgrade -``` - ---- - -### bcrypt / passlib Compatibility -**Problem:** Password hashing fails with newer bcrypt versions. - -**Solution:** Pin bcrypt version in requirements.txt: -``` -bcrypt==4.0.1 -passlib[bcrypt]==1.7.4 -``` - ---- - -### Virtual Environment Best Practices -**Problem:** OneDrive sync causes conflicts with venv, `__pycache__`, and lock files. - -**Solution:** -- Keep project in `C:\Dev\Projects\`, NOT in OneDrive-synced folders -- Add to `.gitignore`: `venv/`, `__pycache__/`, `*.pyc` - ---- - -### Installing Packages in venv -**Problem:** `pip install` sometimes installs globally instead of in venv. - -**Solution:** -- Always verify venv is active: look for `(venv)` prefix in terminal -- If VS Code prompts to create a new venv when one exists, click "Don't show again" -- Use `pip install --break-system-packages` flag if needed (Python 3.12+) - ---- - -### Alembic Migrations -**Problem:** Model changes not reflected in database. - -**Solution:** Always run migrations after model changes: -```powershell -cd backend -alembic revision --autogenerate -m "Description of change" -alembic upgrade head -``` - ---- - -## TypeScript / Frontend - -### React State: Don't Store Object Snapshots for Editing ⚠️ CRITICAL -**Problem:** Form inputs don't update when typing - characters appear then immediately disappear. - -**Cause:** Storing a full object from a Zustand store in local state creates a snapshot. When the store updates, the local state still holds the old reference, causing the input's `value` to revert. - -**Broken pattern:** -```tsx -// BAD: Stores a snapshot that won't update -const [editingNode, setEditingNode] = useState(null) - -// When modal opens: -setEditingNode(node) // snapshot captured here - -// In modal form: - // always shows old value -``` - -**Solution:** Store only the ID, then fetch the current object from the store on each render: -```tsx -// GOOD: Store only the ID -const [editingNodeId, setEditingNodeId] = useState(null) -const editingNode = editingNodeId ? findNode(editingNodeId) : null - -// When modal opens: -setEditingNodeId(node.id) - -// In modal form - now gets fresh data each render: - -``` - -**Why it works:** By calling `findNode()` on each render, the component always gets the current state from the store after updates. - -**Files affected:** Any component that opens a modal/form to edit store data (NodeList.tsx) - ---- - -### Modal Draft State: Don't Overwrite Store-Managed Fields ⚠️ CRITICAL -**Problem:** Child nodes created while a modal is open disappear when clicking "Done". Tree validation fails with errors about non-existent nodes. - -**Cause:** When using local draft state for Cancel/Done functionality in a modal: -1. Modal opens and clones the entire node (including `children: []`) into local state -2. User creates new child nodes via a dropdown (e.g., NodePicker) - these are added to the **store** -3. User clicks "Done" → modal saves draft back to store -4. The draft's stale `children: []` **overwrites** the store's actual children array -5. Newly created nodes are deleted - -**Symptoms:** -- Child nodes don't appear in the tree after closing modal -- Validation errors: "Option references non-existent node" -- Tree won't save due to validation failures - -**Broken pattern:** -```tsx -// Modal with local draft state -const [draft, setDraft] = useState(() => structuredClone(node)) - -const handleSave = () => { - // BAD: This overwrites children with stale snapshot! - updateNode(node.id, draft) - onClose() -} -``` - -**Solution:** Exclude fields that are managed by other store actions (like `children` managed by `addNode`/`deleteNode`): -```tsx -const handleSave = () => { - // GOOD: Exclude 'children' - it's managed separately by addNode/deleteNode - const { children, ...draftWithoutChildren } = draft - updateNode(node.id, draftWithoutChildren) - onClose() -} -``` - -**General rule:** When a modal uses local draft state, identify which fields are: -- **Editable by the form** → include in draft save -- **Managed by other actions** (addNode, deleteNode, etc.) → exclude from draft save - -**Files affected:** `NodeEditorModal.tsx`, any modal that edits objects with nested collections - ---- - -### Modal Scroll/Overflow with Fixed Header and Footer -**Problem:** Modal content extends beyond screen when there's too much content, pushing close button and action buttons off-screen. - -**Solution:** Use flex layout with fixed header/footer and scrollable body: -```tsx -// Modal structure -
- {/* Header - fixed at top */} -
-

{title}

- -
- - {/* Body - scrollable */} -
- {children} -
- - {/* Footer - fixed at bottom */} - {footer && ( -
- {footer} -
- )} -
-``` - -**Key points:** -- `max-h-[85vh]` constrains total modal height -- `flex-col` enables vertical flex layout -- `flex-shrink-0` on header/footer prevents them from shrinking -- `flex-1 overflow-y-auto` on body makes it fill remaining space and scroll - -**Files affected:** Modal.tsx, any component using modals for forms - ---- - -### Lucide React Icons: No Title Prop -**Problem:** TypeScript error when trying to add `title` prop to Lucide icons. - -**Error:** `Property 'title' does not exist on type 'LucideProps'` - -**Broken pattern:** -```tsx - // ❌ Error -``` - -**Solution:** Wrap the icon in a span with the title: -```tsx - - - -``` - ---- - -### Tree Traversal: Preventing Infinite Loops with Visited Set -**Problem:** When traversing a tree structure that has cross-references (like `next_node_id` pointing to nodes elsewhere in the tree), you can get infinite loops. - -**Solution:** Use a `visited` Set to track already-processed nodes: -```tsx -function hasSolutionInSubtree( - node: TreeStructure, - findNode: (id: string) => TreeStructure | null, - visited: Set = new Set() -): boolean { - // Prevent infinite loops - if (visited.has(node.id)) return false - visited.add(node.id) - - if (node.type === 'solution') return true - - // Check children array - if (node.children) { - for (const child of node.children) { - if (hasSolutionInSubtree(child, findNode, visited)) return true - } - } - - // Check next_node_id reference (could point anywhere in tree) - if (node.next_node_id) { - const nextNode = findNode(node.next_node_id) - if (nextNode && hasSolutionInSubtree(nextNode, findNode, visited)) { - return true - } - } - - return false -} -``` - -**Why it matters:** Decision trees can have shared nodes where multiple paths converge on the same target. Without loop detection, recursive traversal will hang. - ---- - -### SharedLinksMap Pattern for Tracking Cross-References -**Problem:** Need to know which nodes link to the same target node (for showing "shared by X nodes" indicators). - -**Solution:** Build a map at the parent component level, pass down to children: -```tsx -// Type definition -type SharedLinksMap = Map> - -// Build the map by traversing tree once -function buildSharedLinksMap( - node: TreeStructure, - map: SharedLinksMap = new Map() -): SharedLinksMap { - const nodeLabel = node.type === 'decision' ? node.question : node.title - - // Record decision option targets - if (node.type === 'decision' && node.options) { - for (const opt of node.options) { - if (opt.next_node_id) { - const existing = map.get(opt.next_node_id) || [] - existing.push({ id: node.id, label: nodeLabel || 'Untitled' }) - map.set(opt.next_node_id, existing) - } - } - } - - // Record action next_node_id targets - if (node.type === 'action' && node.next_node_id) { - const existing = map.get(node.next_node_id) || [] - existing.push({ id: node.id, label: nodeLabel || 'Untitled' }) - map.set(node.next_node_id, existing) - } - - // Recurse - if (node.children) { - for (const child of node.children) { - buildSharedLinksMap(child, map) - } - } - - return map -} - -// Use in parent with useMemo -const sharedLinksMap = useMemo(() => { - if (!treeStructure) return new Map() - return buildSharedLinksMap(treeStructure) -}, [treeStructure]) -``` - -**Usage in child:** Check `sharedLinksMap.get(node.id)?.length > 1` to see if node is shared. - ---- - -### tsconfig.json Strict Mode -**Problem:** VS Code shows warnings about missing compiler options. - -**Solution:** Add to `compilerOptions` in tsconfig.json: -```json -{ - "compilerOptions": { - "strict": true, - "forceConsistentCasingInFileNames": true - } -} -``` - -**Why it matters:** `forceConsistentCasingInFileNames` prevents issues when deploying from Windows (case-insensitive) to Linux (case-sensitive). - ---- - -### Tailwind Dark Mode -**Pattern:** Use Tailwind's `dark:` variant for dark mode styling. - -**Setup:** In `tailwind.config.js`: -```javascript -module.exports = { - darkMode: 'class', // or 'media' for system preference only - // ... -} -``` - -**Usage:** -```jsx -
-``` - ---- - -## Docker / PostgreSQL - -### Accessing PostgreSQL Without Local psql -**Problem:** `psql` command not recognized on Windows (not installed locally). - -**Solution:** Use Docker exec to run psql inside the container: -```powershell -# Single command -docker exec -it patherly_postgres psql -U postgres -c "SELECT * FROM users;" - -# Interactive session -docker exec -it patherly_postgres psql -U postgres - -# Create database -docker exec -it patherly_postgres psql -U postgres -c "CREATE DATABASE patherly_test;" -``` - ---- - -### Docker Container Not Running -**Problem:** Database connection errors. - -**Solution:** Check and start the container: -```powershell -docker ps # See running containers -docker start patherly_postgres # Start if stopped -``` - ---- - -## Git / Version Control - -### git add from Wrong Directory -**Problem:** `git add .` doesn't stage files in parent directories. - -**Solution:** Always run git commands from project root: -```powershell -cd C:\Dev\Projects\patherly -git add . -git commit -m "Your message" -git push origin main -``` - ---- - -### Untracked .claude/ Folder -**Problem:** `.claude/` folder appears in untracked files. - -**Solution:** Either: -1. Add to `.gitignore` if you don't want to track it -2. Or `git add .claude/` if you want Claude Code settings in repo - ---- - -## Environment-Specific Notes - -### Windows Path Handling -- Python and Node handle forward slashes `/` fine on Windows -- Use `os.path.join()` or `pathlib.Path` for cross-platform compatibility -- Avoid hardcoding backslashes `\` in code - -### PowerShell vs CMD -- Some Node.js/Python tools work better in CMD than PowerShell -- If a command fails in PowerShell, try CMD or add to Desktop Commander config: - - `defaultShell: "cmd"` - ---- - -## API / Endpoint Patterns - -### JSONB Fields with Timestamps -**Problem:** Storing datetime objects directly in JSONB fields causes serialization errors. - -**Solution:** Convert to ISO string before storing: -```python -decision = { - "node_id": node_id, - "answer": answer, - "timestamp": datetime.now(timezone.utc).isoformat() # String, not datetime -} -``` - ---- - -### Soft Delete Pattern -**Pattern:** Use `is_active` boolean instead of actually deleting records. - -**Note:** Our schema uses `is_active`, not `is_deleted` (documentation was corrected on Jan 28, 2026). - ---- - -## Testing - -### Test Database Setup -**Requirement:** Tests need a separate database. - -**One-time setup:** -```powershell -docker exec -it patherly_postgres psql -U postgres -c "CREATE DATABASE patherly_test;" -``` - -**Run tests:** -```powershell -cd backend -pytest -``` - ---- - -## Common Mistakes to Avoid - -| Mistake | Correct Approach | -|---------|------------------| -| Using `datetime.utcnow()` | Use `datetime.now(timezone.utc)` | -| Running git from subdirectory | Always `cd` to project root first | -| Forgetting to activate venv | Check for `(venv)` prefix | -| Editing files in OneDrive folder | Use `C:\Dev\Projects\` | -| Using `psql` directly on Windows | Use `docker exec` instead | -| Storing datetime in JSON | Convert to `.isoformat()` string | - ---- - -## Seed Scripts - -### httpx Not Installed -**Problem:** Running `python -m scripts.seed_trees` fails with `ModuleNotFoundError: No module named 'httpx'` - -**Cause:** The seed script uses `httpx` for async HTTP requests, which isn't in the base requirements. - -**Solution:** Install httpx before running seed scripts: -```powershell -pip install httpx -``` - ---- - -### Email Validation Rejects .local Domains -**Problem:** Seed script fails with email validation error when using `.local` domain for seed user email. - -**Error:** `value is not a valid email address: The part after the @-sign is a special-use or reserved name that cannot be used with email.` - -**Cause:** The `email-validator` library (used by Pydantic) correctly rejects `.local` as it's a reserved TLD per RFC 6761. - -**Solution:** Use a standard domain like `example.com` for seed/test users: -```python -# BAD -"email": "seed.admin@patherly.local" - -# GOOD -"email": "seed.admin@example.com" -``` - -**Files affected:** `backend/scripts/seed_trees.py`, any test fixtures with email addresses - ---- - -## Railway Deployment - -### DATABASE_URL_SYNC Must Derive from DATABASE_URL ⚠️ CRITICAL -**Problem:** Alembic migrations run but connect to localhost instead of Railway's Postgres. Database tables never get created. - -**Cause:** `DATABASE_URL_SYNC` was a separate pydantic field with a default value pointing to `localhost:5432`. Railway only provides `DATABASE_URL`, so the sync version wasn't being set correctly. - -**Solution:** Make `DATABASE_URL_SYNC` a property that derives from `DATABASE_URL`: -```python -# BAD: Separate field with default -DATABASE_URL_SYNC: str = "postgresql://postgres:postgres@localhost:5432/patherly" - -# GOOD: Property that derives from DATABASE_URL -@property -def DATABASE_URL_SYNC(self) -> str: - """Get sync URL by removing asyncpg prefix from DATABASE_URL.""" - return self.DATABASE_URL.replace("postgresql+asyncpg://", "postgresql://", 1) -``` - -**Files affected:** `backend/app/core/config.py` - ---- - -### Railway releaseCommand May Not Execute -**Problem:** Migrations in `railway.toml` `releaseCommand` don't run. Deploy logs show the app starting but no migration output. - -**Cause:** Railway's `releaseCommand` feature may not work reliably with all configurations, especially with Dockerfile builds. - -**Solution:** Run migrations in the Docker CMD instead: -```dockerfile -# Instead of relying on railway.toml releaseCommand: -CMD alembic upgrade head && uvicorn app.main:app --host 0.0.0.0 --port ${PORT:-8000} -``` - -**Files affected:** `backend/Dockerfile`, `backend/railway.toml` (keep releaseCommand as backup) - ---- - -### Alembic env.py Must Import All Models -**Problem:** Migration runs but table isn't created. Or migration fails with model-related errors. - -**Cause:** Alembic's `env.py` only discovers models that are imported. If you add a new model (like `InviteCode`), you must add it to the imports. - -**Solution:** Add new models to the import statement in `alembic/env.py`: -```python -# BEFORE: Missing InviteCode -from app.models import User, Team, Tree, Session, Attachment - -# AFTER: All models imported -from app.models import User, Team, Tree, Session, Attachment, InviteCode -``` - -**Files affected:** `backend/alembic/env.py` - ---- - -### New Users Default to "engineer" Role, Not Admin -**Problem:** After registering, user cannot access admin-only endpoints (like creating invite codes). Returns 403 Forbidden. - -**Cause:** The User model defaults to `role="engineer"`. The first user isn't automatically made admin. - -**Solution:** Manually promote user to admin via Railway's Postgres Query tab: -```sql -UPDATE users SET role = 'admin' WHERE email = 'your@email.com'; -``` - -**Then log out and back in** to get a new JWT with the updated role. - -**Files affected:** Database (manual update required for first admin) - ---- - -### Frontend VITE_API_URL Must Use HTTPS, No Port -**Problem:** Frontend gets "Network Error" when trying to call API on Railway. - -**Cause:** Common mistakes: -- Using `http://` instead of `https://` -- Including port number (`:8080`) when Railway handles routing -- Typos in variable names (e.g., `FRONTED_URL` vs `FRONTEND_URL`) - -**Solution:** Ensure correct format: -``` -# WRONG -VITE_API_URL=http://api.patherly.com:8080 -VITE_API_URL=http://api.patherly.com - -# CORRECT -VITE_API_URL=https://api.patherly.com -``` - -**Note:** This is a **build-time** variable. After changing it, you must trigger a new frontend build/deploy. - ---- - -### Swagger OAuth2 Password Flow Returns 401 -**Problem:** Clicking "Authorize" in Swagger UI with username/password returns 401, even with correct credentials. - -**Workaround:** Use the login endpoint directly instead: -1. Call `POST /api/v1/auth/login` with your credentials -2. Copy the `access_token` from the response -3. Click "Authorize" button -4. Paste just the token (no "Bearer" prefix) in the authorization field - -**Files affected:** This is a Swagger UI / FastAPI OAuth2 configuration quirk - ---- - -### Seed Script: Use --email/--password Instead of Creating User -**Problem:** Seed script fails when `REQUIRE_INVITE_CODE=true` because it tries to register a new seed user without an invite code. - -**Solution:** Modified seed script to accept admin credentials via CLI arguments: -```bash -python -m scripts.seed_trees \ - --api-url https://api.patherly.com/api/v1 \ - --email admin@patherly.com \ - --password "your-password" -``` - -**Files affected:** `backend/scripts/seed_trees.py` - ---- - -### Seed Script: Use venv Python, Not System Python -**Problem:** Running `python -m scripts.seed_trees` fails with `ModuleNotFoundError: No module named 'httpx'` even after installing httpx. - -**Cause:** Installing with system Python (`pip install httpx`) but running with venv Python, or vice versa. - -**Solution:** Always use the venv's Python explicitly: -```powershell -# Use venv Python directly -./venv/Scripts/python -m scripts.seed_trees --api-url ... --email ... --password ... - -# Or ensure venv is activated first -.\venv\Scripts\Activate -python -m scripts.seed_trees ... -``` - -**Files affected:** Any script in `backend/scripts/` - ---- - -### Railway Environment Variable Naming -**Problem:** Environment variable isn't being read by the application. - -**Checklist:** -- Case-sensitive: `FRONTEND_URL` ≠ `frontend_url` ≠ `Frontend_Url` -- No typos: `FRONTEND_URL` ≠ `FRONTED_URL` -- Empty string vs not set: Setting a variable to empty string is different from not setting it -- Delete stale variables: If you changed a variable from a field to a property (like `DATABASE_URL_SYNC`), delete the Railway variable - -**Files affected:** Railway dashboard → Service → Variables tab - ---- - -## SQLAlchemy / Database - -### Two Foreign Keys to Same Table Require `foreign_keys=` ⚠️ CRITICAL -**Problem:** SQLAlchemy raises `AmbiguousForeignKeysError` when a model has two ForeignKey columns pointing to the same table (e.g., `deleted_by` and `author_id` both referencing `users.id`). - -**Cause:** SQLAlchemy can't figure out which FK to use for each relationship. - -**Solution:** Add `foreign_keys=` parameter on BOTH relationship sides: -```python -# In User model -deleted_by: Mapped[Optional[uuid.UUID]] = mapped_column( - UUID(as_uuid=True), ForeignKey("users.id"), nullable=True -) - -# In Tree model with two user FKs -author: Mapped["User"] = relationship("User", foreign_keys=[author_id]) -deleted_by_user: Mapped[Optional["User"]] = relationship("User", foreign_keys=[deleted_by]) -``` - -**Files affected:** Any model with multiple FK references to the same table (User, Tree) - ---- - -### Circular FK Between Tables Requires Nullable FK -**Problem:** `Account.owner_id → users.id` and `User.account_id → accounts.id` creates a circular dependency. `CREATE TABLE` fails because neither table can be created first. - -**Cause:** Both tables reference each other, so neither can exist before the other. - -**Solution:** Make one FK nullable and set it after both records exist: -```python -# Create Account first (owner_id=NULL) -account = Account(name="My Account") -db.add(account) -await db.flush() - -# Create User with account_id -user = User(account_id=account.id, ...) -db.add(user) -await db.flush() - -# Now set owner_id -account.owner_id = user.id -await db.commit() -``` - -**Files affected:** `backend/app/models/account.py`, registration endpoint - ---- - -## Authentication / Security - -### Backend Enforcement of `must_change_password` via Dependency Injection -**Problem:** Need to force users to change their password before accessing any endpoint, but some endpoints (like the change-password endpoint itself) must be exempt. - -**Solution:** Add a check in `get_current_active_user` dependency with a route allowlist: -```python -async def get_current_active_user(request: Request, ...): - user = ... # existing auth logic - - # Check must_change_password (with route allowlist) - if user.must_change_password: - allowed = ["/auth/password/change", "/auth/logout", "/auth/me"] - if not any(request.url.path.endswith(p) for p in allowed): - raise HTTPException(403, detail="password_change_required") - - return user -``` - -**Key insight:** This requires adding `request: Request` parameter to the dependency. The frontend checks the 403 detail string and redirects to `/change-password`. - -**Files affected:** `backend/app/api/deps.py`, `frontend/src/components/layout/ProtectedRoute.tsx` - ---- - -### Password Reset Tokens: DB-Backed Single-Use via Hashed JTI -**Pattern:** Password reset tokens use JWT for transport but are tracked in the database for single-use enforcement. - -**How it works:** -1. Generate JWT with `type: "password_reset"`, `jti: uuid4()`, `exp: 30min` -2. Store `SHA-256(jti)` in `password_reset_tokens` table -3. On reset: decode JWT → hash jti → look up in DB → check `used_at IS NULL` -4. After use: set `used_at = now()` to prevent reuse - -**Why not just JWT?** JWTs are stateless — you can't revoke them. DB tracking ensures each token is used exactly once. - -**Files affected:** `backend/app/models/password_reset_token.py`, `backend/app/core/security.py` - ---- - -### Anti-Enumeration on Forgot Password Endpoint -**Pattern:** The `POST /auth/password/forgot` endpoint always returns the same generic success message, regardless of whether the email exists. - -**Why:** If the endpoint returned "email not found" for non-existent users, an attacker could use it to enumerate valid email addresses. - -```python -# Always return success, even if email doesn't exist -return {"message": "If an account exists with that email, a reset link has been sent."} -``` - -**Files affected:** `backend/app/api/endpoints/auth.py` - ---- - -## Seed Scripts - -### Seed Tree Data Missing Required Validation Fields -**Problem:** All 18 seed trees fail to create with validation errors: "Action nodes must have a non-empty action" and "Solution nodes must have a non-empty solution". - -**Cause:** The tree validation (added after the seed scripts were written) requires: -- Action nodes: `action` field (not just `description`) -- Solution nodes: `solution` field (not just `description`) -- Decision nodes with children: at least 2 branches - -The seed data uses `title` and `description` but not the specific `action`/`solution` fields. - -**Solution:** Add a recursive `_fix_node_fields()` function in the seeder that patches nodes before sending to the API: -```python -def _fix_node_fields(node): - if node["type"] == "action" and not node.get("action"): - node["action"] = node.get("title") or node.get("description") or "Action" - elif node["type"] == "solution" and not node.get("solution"): - node["solution"] = node.get("title") or node.get("description") or "Solution" - elif node["type"] == "decision": - children = node.get("children", []) - if len(children) == 1: # Add fallback branch - children.append({"id": "..._fallback", "type": "solution", ...}) - for child in node.get("children", []): - _fix_node_fields(child) -``` - -**Files affected:** `backend/scripts/seed_trees_v2.py` - ---- - -## Admin Features - -### Two-Step Hard Delete Pattern -**Pattern:** Hard-deleting a user requires two API calls — a precheck followed by the actual delete. - -1. `GET /admin/users/{id}/hard-delete-check` → returns `{can_delete: bool, blockers: {owned_accounts: 3, sessions: 12, ...}}` -2. `DELETE /admin/users/{id}/hard-delete` → only succeeds if user is archived AND precheck passes - -**Pre-conditions enforced:** -- User must be archived (`deleted_at IS NOT NULL`) before hard delete -- User must have no blockers (owned accounts, authored trees, sessions, audit logs, invite codes) -- Cannot delete yourself or other super admins -- Audit log entry created BEFORE the delete (since user won't exist after) - -**Files affected:** `backend/app/api/endpoints/admin.py` - ---- - -### Admin User Creation with Temp Password (M365-Style) -**Pattern:** Admin creates a user → gets a one-time temp password → user must change on first login. - -**Key design decisions:** -- Temp password is generated server-side (`secrets`-based, 16 chars, guaranteed complexity) -- Temp password is returned once in the response, never stored as plaintext -- `must_change_password=True` is set on the user, enforced at the dependency level -- Welcome email with temp password is best-effort (never blocks user creation) -- Two modes: "existing account" (join with role) or "personal" (new account created) - -**Files affected:** `backend/app/api/endpoints/admin.py`, `backend/app/core/security.py` - ---- - -## Export / Redaction - -### CORS `expose_headers` Required for Custom Response Headers -**Problem:** Frontend reads custom response headers (e.g. `X-Redaction-Summary`) but gets `undefined` — the header exists in the response but Axios can't access it. - -**Cause:** Browsers enforce CORS restrictions on response headers. Only "CORS-safelisted" headers (Cache-Control, Content-Type, etc.) are accessible by default. Custom headers require explicit exposure. - -**Solution:** Add `expose_headers` to CORS middleware in `main.py` (both branches): -```python -app.add_middleware( - CORSMiddleware, - allow_origins=settings.allowed_origins, - expose_headers=["X-Redaction-Mode", "X-Redaction-Summary"], - ... -) -``` - -**Files affected:** `backend/app/main.py` (both CORS middleware branches) - ---- - -### Redaction Must Run AFTER Variable Resolution -**Problem:** Sensitive data injected via session variables (e.g. `{{client_ip}}` → `192.168.1.1`) would bypass redaction if it ran before variable substitution. - -**Solution:** Export pipeline order matters: -1. Generate export by format (markdown/html/text/psa) -2. Resolve session variables -3. Apply redaction (if `redaction_mode == "mask"`) - -**Key file:** `backend/app/api/endpoints/sessions.py` — the redaction block is placed after both generation and variable resolution, with fail-closed error handling (500 on failure, never return unredacted content). - ---- - -## Adding New Lessons - -When you encounter and fix a bug, add it here with: -1. **Problem:** What error/symptom occurred -2. **Cause:** Why it happened (if known) -3. **Solution:** How to fix it -4. **Files affected:** Where to look (if applicable) diff --git a/MICHAEL-NOTES.md b/MICHAEL-NOTES.md index 5367056c..a4ed4dfe 100644 --- a/MICHAEL-NOTES.md +++ b/MICHAEL-NOTES.md @@ -5,8 +5,8 @@ Use this for personal thoughts, todos, and reminders. ## 📝 Quick Thoughts -- Read CLAUDE-SETUP.md, CURRENT-STATE.md, and LESSONS-LEARNED.md before starting work on [your task]. -- Update CURRENT-STATE.md with what was completed and update LESSONS-LEARNED.md if we encountered any new issues. +- Read CLAUDE.md and CURRENT-STATE.md before starting work on [your task]. +- Update CURRENT-STATE.md with what was completed and update CLAUDE.md if we encountered any new issues. - ## ✅ Personal TODO