Amirali/overlay

[aircode610]

No description provided.

[mytestingaccount2026]

🔍 TODO/FIXME Scan Results

I scanned all added lines in this PR (lines beginning with +, excluding +++ file headers) across the following changed files:

File	TODO/FIXME Found?
.github/workflows/lint.yml	✅ None
ai/form_fill.py	✅ None
ai/rag/retrieval.py	✅ None
backend/app/services/risk.py	✅ None

No TODO or FIXME comments were introduced in this PR. 🎉

[aircode610]

🔍 Automated Security & Quality Review — PR #7 (amirali/overlay)

Diff reviewed: git diff origin/main...origin/amirali/overlay — 31 files, +817 / −464.
~90% is pure ruff lint/format cleanup (no behavior change). The substantive functional changes are confined to:

• ai/form_fill.py — full rewrite of the form-overlay feature (Qwen image-edit qwen-image-2.0 → Qwen-VL bbox detection + Pillow drawing).
• backend/app/routers/public.py — generate_reply dead-code removal (action_titles/generate_reply_text); unused RagHit import dropped.
• ai/react_agent/ocr.py — import moved to top (E402 fix).
• .github/workflows/lint.yml — new ruff CI job.

---

1. Security vulnerabilities / unsafe patterns

• 🟠 PIL decompression-bomb / DoS (medium). _draw_placeholders calls Image.open(image_path).convert("RGB") on user-uploaded files without setting Image.MAX_IMAGE_PIXELS or a dimension guard. A small (<10 MB) but very high-pixel image can exhaust memory. (Note: the fix already exists in the uncommitted local working copy via a MAX_IMAGE_PIXELS env guard — it is not on the PR branch yet. Please commit it.)
• 🟢 No secrets committed. DASHSCOPE_API_KEY is read from env; no keys/tokens appear in the diff. 👍
• 🟡 Untrusted model-output parsing. _detect_fields does naive cleanup (replace("'", '"'), trailing-comma regex) then json.loads inside try/except → []. No eval, so safe; but the quote-swap can corrupt legitimate apostrophes inside string values. Low risk.
• 🟢 No path traversal. image_path = letter.original_file (server-generated, ownership already checked in the route). Bbox values are float()-cast and clamped to image bounds before drawing. OK.

2. Project-convention / image-library usage

• 🟠 Model selection violates CLAUDE.md. Convention: default to fast qwen-vl-plus, only escalate to qwen-vl-max when quality is insufficient ("Speed is a priority"). form_fill.py hardcodes "model": "qwen-vl-max". Recommend qwen-vl-plus, made env-configurable like the other modules.
• 🟠 Pillow not declared in ai/requirements.txt. form_fill.py now imports PIL, but it's only listed in backend/requirements.txt. If the ai/ module is installed/run on its own, the import fails. Add pillow>=11.0.0 to ai/requirements.txt.
• 🟡 Docstring vs. code mismatch. Module/_draw_placeholders docs say bbox is "0–1000 normalized" with pixel = coord/1000*dim, but the code derives scale from max coords (scale_x = img_w/(coord_max_x*1.02)). Misleading — fix the comment.
• 🟢 Good: env-driven QWEN_API_BASE, OpenAI-compatible /chat/completions, the E402 import fix in ocr.py, and the new ruff CI all align with conventions.

3. Hardcoded values that should move to config

• 🟠 Font paths hardcoded: /System/Library/Fonts/Helvetica.ttc (macOS) and /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf. On the Linux deploy targets (Railway/Render) the macOS path never matches and DejaVu may be absent → silent fallback to ImageFont.load_default() (tiny bitmap font), degrading the overlay. Make the font path configurable (env var) and/or bundle a font in the repo.
• 🟠 qwen-vl-max model string hardcoded — should be env-configurable (cf. QWEN_OCR_MODEL, QWEN_AGENT_MODEL).
• 🟡 Magic numbers: 1.02 scale padding, font_size = img.height // 50, max_tokens=2048, RGB color tuples. Acceptable for a hackathon, but worth named constants.

4. Summary / recommendation

No committed secrets and no critical injection/traversal vectors. The two items worth blocking on before merge:

1. Add the MAX_IMAGE_PIXELS guard (decompression-bomb DoS) — already done locally, just not committed.
2. Declare Pillow in ai/requirements.txt so the AI module doesn't break on import.

Then address conventions (qwen-vl-plus default, configurable font path/model) as follow-ups. The bulk of the PR is safe lint cleanup. ✅

_{🤖 Generated automatically. Note: ai/form_fill.py had uncommitted local edits at review time; findings above are against the pushed PR branch.}

[aircode610]

🤖 Automated PR Review — amirali/overlay (#7 → main)

Reviewed the diff against main (817 additions / 464 deletions across 31 files). The vast majority is mechanical ruff lint/format cleanup; the only substantive logic change is the new Pillow-based form-overlay in ai/form_fill.py (Qwen-VL bbox detection → Pillow draws red placeholders), plus small refactors in routers/public.py and pipeline/orchestrator.py.

🔒 Security

• ✅ No hardcoded secrets, no eval/exec/subprocess/os.system, no shell=True, no verify=False. Secrets come from env.
• ✅ Model JSON output is parsed with json.loads (not eval) inside try/except — safe.
• ✅ form_fill endpoint verifies ownership (letter.user_id == user.id) and uses a server-controlled path ({upload_dir}/{user_id}/{letter_id}.{ext}, resolved) — no path-traversal surface.
• ✅ Exceptions are logged server-side but return generic 502 to the client — no internal detail leakage.
• ⚠️ Decompression-bomb / memory-exhaustion DoS (ai/form_fill.py, _draw_placeholders): Image.open(image_path).convert("RGB") decodes a user-uploaded image with no pixel-dimension guard. The upload-time 10 MB byte cap does not protect against this — a tiny compressed PNG can decode to a gigapixel raster. Pillow's default MAX_IMAGE_PIXELS only emits a warning, not an error. Set Image.MAX_IMAGE_PIXELS and/or reject oversized dimensions before .convert().

📐 Conventions / correctness

• ⚠️ Qwen model selection violates CLAUDE.md. _detect_fields hardcodes "model": "qwen-vl-max". The project rule is "OCR/vision: qwen-vl-plus (fast). Only use qwen-vl-max if quality is insufficient. Speed is a priority." Default to qwen-vl-plus (env-overridable).
• ⚠️ Incorrect Pillow usage: img.save(buf, format="PNG", quality=95) — quality is a JPEG-only parameter and is ignored for (lossless) PNG. Drop it or use optimize=True / compress_level=....
• ℹ️ except (OSError, IOError) — IOError is an alias of OSError in Py3 (redundant).
• ℹ️ generate_reply in public.py: the else: pass leaves a dead no-op branch after the action_titles logic was removed — confirm intentional and clean up.
• ℹ️ Mixing a large mechanical reformat with a feature change makes the diff hard to review; consider splitting in future PRs.

⚙️ Hardcoded values that should be configuration

• ⚠️ Font paths (ai/form_fill.py): /System/Library/Fonts/Helvetica.ttc (macOS-only) and /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf are hardcoded. On the Railway/Render Linux deploy the macOS path won't exist, and if neither resolves it silently falls back to load_default() (a tiny bitmap font) — degraded output with no error. Expose a FORM_FILL_FONT_PATH env var.
• ⚠️ Model name qwen-vl-max should be env/config-driven (see above).
• ℹ️ Env-var naming inconsistency: form_fill.py reads DASHSCOPE_API_KEY while backend config.py / CLAUDE.md standardize on QWEN_API_KEY / QWEN_API_BASE. (Pre-existing, but worth aligning.)
• ℹ️ Magic numbers (red=(220,38,38), img.height // 50, scale padding * 1.02, max_tokens=2048, placeholders[:8]) are fine to keep, but the * 1.02 coordinate-scaling heuristic is fragile and worth a comment.

📝 Note

Your local working tree has uncommitted edits to ai/form_fill.py that already address three of the items above — the decompression-bomb guard (MAX_IMAGE_PIXELS), the qwen-vl-plus default via QWEN_VL_MODEL, and a configurable FORM_FILL_FONT_PATH. These are not committed/pushed, so they are not part of this PR. Commit and push them to land the fixes.

Verdict: No critical/exploitable vulnerabilities found. The decompression-bomb guard is the one security item to address before merge; the font-path and model-selection items are correctness/convention fixes (already staged locally).

[aircode610]

Migration Safety Review ✅

No new database migrations found in this PR. This PR is safe to run on production data.

What was checked

1. No migration files added or modified — There are no Alembic migration files, SQL scripts, or any other migration artifacts in this PR. The repository does not use a formal migration framework (it uses SQLModel.metadata.create_all() for schema creation).

2. No schema changes in models.py — The changes to backend/app/models.py are purely cosmetic formatting (ruff auto-formatting). Specifically:

   • Whitespace alignment changes in DocumentCategory enum values (e.g., "health_insurance" # → "health_insurance" #)
   • Comment alignment changes in Letter model fields (e.g., letter_type: str = "" # → letter_type: str = "" #)
   • Line wrapping of the GOVERNMENT_BENEFITS enum value
   • No columns added, removed, renamed, or type-changed
   • No table names changed
   • No relationship or index modifications

3. No schema changes in database.py — The only change is an added blank line (formatting). SQLModel.metadata.create_all(engine) call is unchanged.

4. No schema changes in schemas.py — Changes are formatting only (line wrapping of Field() kwargs, removing unused Any import). These are Pydantic response schemas, not ORM/DB models.

Summary of actual PR changes

This PR contains:

• ai/form_fill.py: Refactored to use Pillow overlay instead of image generation (Qwen-VL field detection + deterministic drawing)
• .github/workflows/lint.yml: New CI workflow for ruff linting
• All other files: Ruff auto-formatting (whitespace, import sorting, line wrapping) and removal of unused imports/variables — no behavioral changes to DB layer

Verdict: Safe to deploy. No database schema changes, no migrations needed.