Feat/post /fix-llm

[Dru-429]

Before opening: make sure there is an issue tracking this work, and link it below. PRs without a linked issue may be closed without review.

Linked issue

Closes #191 ## What this PR does

This PR adds a new endpoint POST /fix-llm. It connects vulnerability finding metadata and source context with Ollama to generate unified code diffs. The integration handles Ollama availability gracefully, falling back to an error response if the service is down,

Type of change

• Bug fix
• New feature
• ML model / training pipeline
• Refactor (no behaviour change)
• Documentation
• Tests only

ML tier (if applicable)

• Tier 1 — Triage
• Tier 2 — Predictive
• Tier 3 — Autonomous
• Not ML-related

Stack affected

• Backend
• Frontend
• Both

---

Changes

Backend

• config.py: Added environment variable parsing for OLLAMA_MODEL (defaulting to llama3) and OLLAMA_BASE_URL.
• prompt_builder.py: Created a prompt construction utility using vulnerability details and ~20 lines of surrounding code context to request clean unified diffs.
• llm_patcher.py: Integrated an async client wrapper to fire requests to Ollama's native generation endpoint and extract clean diff blocks, raising a custom OllamaUnavailableError if it fails.
• main.py: Registered the new POST /fix-llm route securely while keeping the original /fix logic isolated.
• test_llm_patcher.py: Added mock-based unit tests asserting successful diff extraction and proper HTTP 503 error handling during backend connection failures.

---

Testing

How did you test this?

Ran mock-driven unit tests inside test_llm_patcher.py to validate payload responses on success, and checked exception flows to confirm an HTTP 503 is returned appropriately when Ollama is unreachable. All tests passed successfully.

Checklist

• Tested locally end-to-end (upload ZIP or GitHub URL → scan → findings returned correctly)
• New ML model falls back gracefully when model file is absent
• No new console.error or unhandled Python exceptions introduced
• Added or updated tests where applicable
• requirements.txt / package.json updated if new dependencies added
• New model files (.pkl, .pt, etc.) are gitignored, not committed

---

Anything reviewers should focus on

Take a quick look at the exception handling block for OllamaUnavailableError in the router layer to make sure the HTTP 503 status code structure matches our frontend standards.

[github-actions]

🎉 Thank you @Dru-429 for submitting a Pull Request!

We're excited to review your contribution.

Before Review

✅ Ensure all CI checks pass
✅ Complete the PR template
✅ Link the related issue

⚡ Want faster reviews and contributor support?

Join our Discord community:

🔗 https://discord.gg/FcXuyw2Rs

Maintainers and mentors are active there and can help resolve blockers quickly.

Happy Contributing! 🚀

[github-actions]

✅ PR template check passed!

@arpit2006 this PR is ready for your review. 🚀

[arpit2006]

@Dru-429 !

Thanks for your contribution. Before merging you should solve the blocking issue.As soon as issue is resolved happy to Review.

---

Files Reviewed

File	Status
backend/app/config.py	✅ Acceptable
backend/app/services/prompt_builder.py	✅ Acceptable
backend/app/ml/llm_patcher.py	⚠️ Minor issue
backend/app/main.py (/fix-llm route)	🔴 Blocking issue
backend/tests/test_llm_patcher.py	🔴 Blocking issue

---

Blocking Issues

🔴 1. OllamaUnavailableError aborts the entire batch (main.py, route handler)

Location: /fix-llm handler, inner except OllamaUnavailableError block

# Current — raises HTTP 503 and kills ALL remaining findings
for row in rows:
    ...
    try:
        ...
        diff = await generate_patch(prompt)
        fixes.append(...)
    except OllamaUnavailableError as e:
        raise HTTPException(status_code=503, detail=str(e))  # ← aborts loop
    except Exception as e:
        logger.error(...)

Problem: If Ollama goes down mid-batch (say 5 findings are requested and it fails on finding #2), the entire request fails with a 503. Findings already processed are discarded and the loop exits. The reviewer checklist specifically calls out this block.

Expected behaviour: Since this is a multi-finding batch endpoint, the correct pattern is to record a "failed" Fix entry (or skip and log) for the failing finding, let the loop continue, and only return 503 if Ollama was unavailable for all findings, or surface per-item errors in the response body. This is consistent with how the existing /fix route handles partial results.

Fix:

except OllamaUnavailableError as e:
    logger.error(f"Ollama unavailable for finding {f_id}: {e}")
    fixes.append(Fix(
        finding_id=f_id,
        status="error",
        summary="Ollama service unavailable",
        diff=None,
        notes=[str(e)]
    ))
    # Optionally break if you know Ollama is entirely down

If you want to preserve the 503 behaviour, it should only trigger when req.finding_ids is a single item, or after the loop completes with zero successes.

---

🔴 2. Test mock does not actually exercise the async context manager — tests pass vacuously (test_llm_patcher.py)

Location: test_generate_patch_success and test_generate_patch_unavailable

# Current — patching the method, but AsyncClient is used as an async context manager
with patch("httpx.AsyncClient.post", return_value=mock_response):
    diff = await generate_patch(prompt)

Problem: generate_patch calls async with httpx.AsyncClient(...) as client: response = await client.post(...). Patching httpx.AsyncClient.post at the class level does not correctly mock the instance method when it's accessed through __aenter__. In practice this mock works on some versions of httpx/unittest.mock because AsyncClient isn't truly async-instantiated, but it's fragile and the intent is not explicit. The correct, robust pattern — already used in test_health.py from PR #197 — is to mock httpx.AsyncClient itself:

from unittest.mock import AsyncMock, patch, MagicMock

@pytest.mark.asyncio
async def test_generate_patch_success():
    mock_response = MagicMock()
    mock_response.raise_for_status.return_value = None
    mock_response.json.return_value = {"response": expected_diff}

    mock_client = AsyncMock()
    mock_client.post = AsyncMock(return_value=mock_response)

    with patch("app.ml.llm_patcher.httpx.AsyncClient") as MockClient:
        MockClient.return_value.__aenter__ = AsyncMock(return_value=mock_client)
        MockClient.return_value.__aexit__ = AsyncMock(return_value=False)
        diff = await generate_patch(prompt)
        assert diff == expected_diff

The same fix applies to test_generate_patch_unavailable. This is exactly the pattern the project already established in backend/tests/test_health.py.

---

Non-Blocking / Suggestions

⚠️ 3. Path traversal is not validated (main.py)

abs_path = repo_dir / file_path
if not abs_path.exists():
    continue

file_path comes from the database (written by the scanner), so it's low-risk — but it's still user-influenced (via the job_id → stored findings). A path like ../../etc/passwd in a maliciously crafted file_path column could escape repo_dir. Add a resolve() check:

abs_path = (repo_dir / file_path).resolve()
if not str(abs_path).startswith(str(repo_dir.resolve())):
    logger.warning(f"Path traversal attempt blocked: {file_path}")
    continue

⚠️ 4. config.py replaces entire module — no existing app config preserved

The PR ships config.py as a brand-new two-liner. If this project had any other config constants before, they are gone. Verify with the team that this file did not exist prior to this PR and that no other module depends on a previous config.py.

⚠️ 5. prompt_builder.py strips the title field but the finding_meta dict in the route doesn't include it

In main.py the route builds:

finding_meta = {
    "rule_id": rule_id,
    "severity": severity,
    "description": message
}

But PromptBuilder.build_patch_prompt also reads finding_meta.get("title", "Unknown Vulnerability"). The title is always "Unknown Vulnerability" in the LLM prompt because the route doesn't pass it. The message column (already passed as description) typically is the human-readable title in Semgrep findings. Either rename the key or pass "title": message explicitly so the prompt is informative.

ℹ️ 6. 20-line context window is hardcoded magic numbers

start_idx = max(0, line_number - 1 - 10)
end_idx = min(len(lines), line_number - 1 + 10)

The PR description says "~20 lines" — fine for now, but this should ideally be a constant at the top of the function or pulled from config.py to make it easy to tune. Not blocking, just a note.

ℹ️ 7. No integration/end-to-end test

Unit tests cover llm_patcher.py in isolation, which is good. But there's no HTTP-level test (via TestClient) that fires POST /fix-llm and validates the 503 shape, the 404 for unknown job_id, or the empty-list fast-return. Given the reviewer checklist explicitly asks about "HTTP 503 status code structure matching frontend standards," a TestClient-level test for the error shape would close that gap.

---

What's Done Well ✅

• Clean separation of concerns — prompt_builder.py and llm_patcher.py are properly isolated from the route handler. Good layering.
• _extract_diff — The markdown code-block stripping is a real-world necessity with LLMs and the unit tests cover both paths.
• Graceful httpx error handling — Distinguishing RequestError (network) from HTTPStatusError (Ollama returned non-2xx) is correct.
• Timeout set explicitly — timeout=120.0 is appropriate for local LLM inference.
• stream: False — Correct for simplicity; avoids needing to handle chunked streaming responses.
• requirements.txt — httpx was already present; no new dependency needed.

---

Summary

The core integration idea is solid. The two blocking issues are:

1. Abort-on-first-failure — OllamaUnavailableError inside a loop should record a failure entry, not kill the whole request.
2. Fragile async mock — The test mock pattern doesn't properly handle the async with context manager; use AsyncMock + __aenter__/__aexit__ as done in test_health.py.

Fix those two and the PR is in good shape to merge.

[arpit2006]

@Dru-429 , Please resolve CI fails.

[Dru-429]

@arpit2006 will start working on this