Repository navigation
fix: Phase 2 hardening, persistence, and Python 3.11+ fixes #345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e223b97
feat(ui): Deploy PMOVES.AI Unified Portal as Docker service
39387d2
fix(ui): Address PR #345 review findings for FluteClient
033d3a3
fix: Address CodeRabbit PR #345 review comments
dd00d1d
fix(compose): Address remaining CodeRabbit PR #345 comments
9fd7a53
fix(ui): Complete PMOVES UI backend integration
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # Docker DNS Case Sensitivity | ||
|
|
||
| **Date:** 2025-12-22 | ||
| **Source:** PR #345 CodeRabbit review | ||
|
|
||
| ## Issue | ||
|
|
||
| Mixed hostname casing in docker-compose.yml caused inconsistent DNS resolution: | ||
| - `supabase_kong_PMOVES.AI` (uppercase) | ||
| - `supabase_kong_pmoves.ai` (lowercase) | ||
|
|
||
| Docker DNS behavior varies by configuration - some resolve case-insensitively, others don't. | ||
|
|
||
| ## Pattern | ||
|
|
||
| **Always use lowercase for Docker hostnames.** | ||
|
|
||
| ```yaml | ||
| # Bad - inconsistent casing | ||
| - SUPABASE_URL=http://supabase_kong_PMOVES.AI:8000 | ||
| - SUPABASE_REALTIME_URL=ws://supabase_kong_PMOVES.AI:8000/realtime/v1 | ||
|
|
||
| # Good - consistent lowercase | ||
| - SUPABASE_URL=http://supabase_kong_pmoves.ai:8000 | ||
| - SUPABASE_REALTIME_URL=ws://supabase_kong_pmoves.ai:8000/realtime/v1 | ||
| ``` | ||
|
|
||
| ## Detection | ||
|
|
||
| ```bash | ||
| # Find mixed casing in docker-compose files | ||
| grep -n "supabase_kong" pmoves/docker-compose.yml | sort -u | ||
| ``` | ||
|
|
||
| ## Fix | ||
|
|
||
| Use `replace_all` to normalize: | ||
| ```bash | ||
| sed -i 's/supabase_kong_PMOVES\.AI/supabase_kong_pmoves.ai/g' pmoves/docker-compose.yml | ||
| ``` | ||
|
|
||
| ## Related | ||
|
|
||
| - `.claude/learnings/docker-entrypoint-patterns-2025.md` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # Dockerfile Non-Root Container Permissions | ||
|
|
||
| **Date:** 2025-12-22 | ||
| **Source:** PR #345 CodeRabbit review | ||
|
|
||
| ## Issue | ||
|
|
||
| Dockerfile hardening added user creation but missed creating directories | ||
| needed by entrypoint scripts: | ||
| - entrypoint.sh writes to `/models` via `snapshot_download()` | ||
| - Directory doesn't exist or has wrong ownership | ||
|
|
||
| ## Pattern | ||
|
|
||
| **Create and chown all directories written to at runtime BEFORE USER directive:** | ||
|
|
||
| ```dockerfile | ||
| # Security: Run as non-root user | ||
| RUN groupadd -r pmoves --gid=65532 && \ | ||
| useradd -r -g pmoves --uid=65532 --home-dir=/app --shell=/sbin/nologin pmoves && \ | ||
| mkdir -p /models && \ | ||
| chown -R pmoves:pmoves /app /app/vibevoice /models | ||
|
|
||
| USER pmoves | ||
| ``` | ||
|
|
||
| ## Checklist for Dockerfile Hardening | ||
|
|
||
| 1. **Identify write paths** - grep entrypoint/startup scripts for: | ||
| - File writes | ||
| - `mkdir` commands | ||
| - Model downloads (HuggingFace, etc.) | ||
| - Log directories | ||
| - Cache directories | ||
|
|
||
| 2. **Create directories** - Add `mkdir -p` for each path | ||
|
|
||
| 3. **Set ownership** - Add to `chown -R pmoves:pmoves` list | ||
|
|
||
| 4. **Order matters** - All `mkdir` and `chown` must be BEFORE `USER pmoves` | ||
|
|
||
| ## Detection | ||
|
|
||
| ```bash | ||
| # Find write operations in entrypoint scripts | ||
| grep -E "mkdir|>|>>|download|cache" services/*/entrypoint.sh | ||
| ``` | ||
|
|
||
| ## Anti-Pattern | ||
|
|
||
| ```dockerfile | ||
| # Bad - USER before mkdir means mkdir runs as non-root | ||
| USER pmoves | ||
| RUN mkdir -p /models # Permission denied! | ||
| ``` | ||
|
|
||
| ## Related | ||
|
|
||
| - `.claude/learnings/docker-entrypoint-patterns-2025.md` | ||
| - PMOVES Dockerfile hardening standard (UID/GID 65532) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # Next.js NEXT_PUBLIC Environment Variables | ||
|
|
||
| **Date:** 2025-12-22 | ||
| **Source:** PR #345 CodeRabbit review | ||
|
|
||
| ## Issue | ||
|
|
||
| `NEXT_PUBLIC_*` env vars were set to internal Docker hostnames: | ||
| ```yaml | ||
| - NEXT_PUBLIC_FLUTE_GATEWAY_URL=http://flute-gateway:8055 | ||
| - NEXT_PUBLIC_FLUTE_WS_URL=ws://flute-gateway:8056 | ||
| ``` | ||
|
|
||
| These hostnames resolve inside Docker network but **fail in browser** because: | ||
| 1. `NEXT_PUBLIC_*` vars are embedded at **build time** | ||
| 2. Browser JavaScript runs **client-side**, outside Docker network | ||
| 3. Browser cannot resolve internal Docker DNS names | ||
|
|
||
| ## Pattern | ||
|
|
||
| **Use env var overrides with localhost fallback for client-side URLs:** | ||
|
|
||
| ```yaml | ||
| # Good - allows override, defaults to host-accessible localhost | ||
| - NEXT_PUBLIC_FLUTE_GATEWAY_URL=${NEXT_PUBLIC_FLUTE_GATEWAY_URL:-http://localhost:8055} | ||
| - NEXT_PUBLIC_FLUTE_WS_URL=${NEXT_PUBLIC_FLUTE_WS_URL:-ws://localhost:8056} | ||
| ``` | ||
|
|
||
| ## Server-Side vs Client-Side | ||
|
|
||
| | Variable | Scope | Can use Docker hostname? | | ||
| |----------|-------|-------------------------| | ||
| | `FLUTE_GATEWAY_URL` | Server-side | Yes | | ||
| | `NEXT_PUBLIC_FLUTE_GATEWAY_URL` | Client-side | No - use localhost | | ||
|
|
||
| ## Detection | ||
|
|
||
| ```bash | ||
| # Find NEXT_PUBLIC vars with Docker hostnames | ||
| grep "NEXT_PUBLIC.*http://[a-z-]*:" pmoves/docker-compose.yml | ||
| ``` | ||
|
|
||
| ## Related | ||
|
|
||
| - Next.js documentation on environment variables | ||
| - `.claude/context/flute-gateway.md` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| # Prometheus Registry Double-Registration | ||
|
|
||
| **Date:** 2025-12-22 | ||
| **Source:** PR #345 CodeRabbit review | ||
|
|
||
| ## Issue | ||
|
|
||
| Direct access to `REGISTRY._names_to_collectors` is fragile: | ||
| 1. Private API may change across prometheus_client versions | ||
| 2. No type validation if metric with same name but different type exists | ||
| 3. No labelnames validation | ||
|
|
||
| ## Pattern | ||
|
|
||
| **Use try/except with type validation instead of direct private API access:** | ||
|
|
||
| ```python | ||
| from prometheus_client import Counter, REGISTRY | ||
|
|
||
| def _get_or_create_counter(name: str, description: str, labelnames: tuple) -> Counter: | ||
| """Get existing counter or create new one (safe for module reimport via -m flag).""" | ||
| try: | ||
| return Counter(name, description, labelnames=labelnames) | ||
| except ValueError as e: | ||
| if "Duplicated timeseries" in str(e): | ||
| # Metric already exists; retrieve and validate type | ||
| existing = REGISTRY._names_to_collectors.get(name) | ||
| if existing is not None and isinstance(existing, Counter): | ||
| return existing | ||
| raise TypeError( | ||
| f"Metric '{name}' exists but is {type(existing).__name__}, not Counter" | ||
| ) from e | ||
| raise | ||
| ``` | ||
|
|
||
| ## Why This Matters | ||
|
|
||
| The `python -m` flag can cause double module import, registering metrics twice. | ||
| This helper safely handles the duplicate registration case while validating types. | ||
|
|
||
| ## Anti-Pattern | ||
|
|
||
| ```python | ||
| # Bad - no type validation, silent bugs if metric type differs | ||
| if name in REGISTRY._names_to_collectors: | ||
| return REGISTRY._names_to_collectors[name] | ||
| return Counter(name, description, labelnames=labelnames) | ||
| ``` | ||
|
|
||
| ## Related | ||
|
|
||
| - prometheus_client library documentation | ||
| - Python `-m` flag double-import behavior | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| FROM node:20-slim AS builder | ||
|
|
||
| LABEL org.opencontainers.image.source="https://github.com/POWERFULMOVES/PMOVES.AI" | ||
| LABEL org.opencontainers.image.description="PMOVES.AI Unified Portal - Next.js Dashboard" | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| # Install curl for health checks | ||
| RUN apt-get update && apt-get install -y --no-install-recommends curl && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| # Install dependencies | ||
| COPY package*.json ./ | ||
| RUN npm install | ||
|
|
||
| # Copy source and build | ||
| COPY . . | ||
| RUN npm run build | ||
|
|
||
| # Production image | ||
| FROM node:20-slim AS runner | ||
| WORKDIR /app | ||
|
|
||
| # Install curl for health checks | ||
| RUN apt-get update && apt-get install -y --no-install-recommends curl && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| # Security: Non-root user (consistent with other PMOVES services) | ||
| RUN groupadd -r pmoves --gid=65532 && \ | ||
| useradd -r -g pmoves --uid=65532 --home-dir=/app --shell=/sbin/nologin pmoves | ||
|
|
||
| # Copy built assets from builder | ||
| COPY --from=builder /app/.next/standalone ./ | ||
| COPY --from=builder /app/.next/static ./.next/static | ||
| COPY --from=builder /app/public ./public | ||
|
|
||
| # Change ownership to non-root user | ||
| RUN chown -R pmoves:pmoves /app | ||
|
|
||
| USER pmoves | ||
|
|
||
| ENV NODE_ENV=production | ||
| ENV PORT=3000 | ||
| ENV HOSTNAME="0.0.0.0" | ||
|
|
||
| EXPOSE 3000 | ||
|
|
||
| HEALTHCHECK --interval=30s --timeout=10s --start-period=60s --retries=3 \ | ||
| CMD curl -sf http://localhost:3000/api/health || exit 1 | ||
|
|
||
| # Next.js standalone server | ||
| CMD ["node", "server.js"] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { NextResponse } from 'next/server'; | ||
|
|
||
| /** | ||
| * Health check endpoint for container orchestration. | ||
| * Returns service status, name, and timestamp. | ||
| */ | ||
| export async function GET() { | ||
| return NextResponse.json({ | ||
| status: 'healthy', | ||
| service: 'pmoves-ui', | ||
| timestamp: new Date().toISOString(), | ||
| version: process.env.npm_package_version || '0.1.0', | ||
| }); | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address labelnames mismatch scenarios.
The function accepts a
labelnamestuple but does not validate that an existing metric's labelnames match. If a metric with the same name but different labelnames exists,isinstance(existing, Counter)will pass, but the returned metric may have incompatible labelnames. Consider documenting:🤖 Prompt for AI Agents