Uh oh!
There was an error while loading. Please reload this page.
feat(flashcards): flashcard generation/import → Pydantic AI agent (#146) - #300
Conversation
…I agent (#146) Routes all five flashcard LLM seams through one `flashcard_agent`: - New agents/flashcard.py (Flashcards{cards: list[FlashCard{front,back}]}). Registered `flashcard` (flash) in _providers. - flashcard_import_service gains `_run_flashcard_agent(prompt)` (filters empty cards, degrades to [] on agent failure — preserving the old bad-output→[] resilience). extract_cards_from_image / gemini_generate_cards / gemini_cleanup_cards / gemini_cloze rewired onto it; call_gemini import gone. gemini_cleanup_cards still falls back to its input; extract still short-circuits on empty OCR. - generate_flashcards (the main AI-generation path) moved here from gemini_service (prompt-building verbatim) and runs the agent; routes/flashcards imports it from flashcard_import_service, so no gemini_service import remains in the flashcard path. Tests: rewrote the 7 call_gemini patches to mock flashcard_agent; added empty-filter, agent-failure→[], cleanup-fallback, and generate_flashcards tests. Route tests unchanged (they patch whole functions). Full suite 832 passed (2 pre-existing storage-env failures). ruff clean. Spec: specs/146-flashcard-agent.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughFlashcard import and generation now use a structured ChangesFlashcard agent migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant FlashcardsRoute
participant FlashcardImportService
participant flashcard_agent
FlashcardsRoute->>FlashcardImportService: Request flashcard generation
FlashcardImportService->>flashcard_agent: Submit structured prompt
flashcard_agent-->>FlashcardImportService: Return Flashcards
FlashcardImportService-->>FlashcardsRoute: Return filtered cards
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 947fda2 | Commit Preview URL Branch Preview URL | Jul 14 2026, 05:12 PM |
AndresL230
left a comment
There was a problem hiding this comment.
Automated review (workflow-backed, xhigh effort) of the flashcard → Pydantic AI agent migration.
One root cause dominates: _run_flashcard_agent's blanket except Exception: return [] turns LLM/API/transport outages into silent HTTP 200 with an empty (or un-cleaned) result across every migrated route — 12 of the finder hits collapse to this one issue. Alongside it: a dropped transient-error retry, a silent model-config default flip masked by a misleading comment, a latent async-loop swallow, and three cleanup items. Details inline, ranked by severity.
🤖 Generated with Claude Code
| "bad LLM output → []" resilience (never raises).""" | ||
| try: | ||
| result = run_agent_sync(flashcard_agent.run(prompt)) | ||
| except Exception: |
There was a problem hiding this comment.
🔴 P1 — this blanket except Exception: return [] swallows every LLM / API / transport failure.
The old call_gemini path re-raised on 429 / 500 / timeout / missing key, so routes surfaced a retryable HTTP 502. Now a Gemini outage (or an unset GEMINI_API_KEY) makes every migrated entry point — extract_cards_from_image, gemini_generate_cards, gemini_cloze, generate_flashcards — return [], so POST /api/flashcards/import/generate and /import/cloze reply 200 {"cards": []}.
Consequences:
- The frontend can't distinguish an AI outage from 'genuinely no cards' — the user sees zero cards with nothing to retry.
- There's no logging in the
except, so operators get no signal. - Each route's existing 502 / 422 handler becomes dead code.
Suggest: only degrade to [] on genuine bad-output / parse cases; let transient / transport errors propagate (or at minimum logger.exception(...) before returning).
| behavior) and degrades to an empty list on agent failure — matching the old | ||
| "bad LLM output → []" resilience (never raises).""" | ||
| try: | ||
| result = run_agent_sync(flashcard_agent.run(prompt)) |
There was a problem hiding this comment.
🟠 P2 — lost transient-error retry.call_gemini auto-retried once on 429 / 500 with a ~2s backoff (retries=1). A bare flashcard_agent.run drops that, so a single rate-limit / 5xx blip now fails the whole call instead of retrying.
🟡 P3 (latent) — async-loop swallow.run_agent_sync wraps asyncio.run, which raises RuntimeError if ever called from a running event loop; the except just below then swallows it to []. Harmless today (all callers are sync def), but the moment an async handler or on-loop task calls one of these helpers it will silently return zero cards instead of failing loudly.
| # Social summary is short-form prose → the cheaper lite tier is enough. | ||
| "social_summary": "gemini-2.5-flash-lite", | ||
| # Flashcard generation/cleanup/cloze — content quality matters → full Flash | ||
| # (matches the legacy gemini_service default the flashcard path used). |
There was a problem hiding this comment.
🟠 P2 — this comment is misleading. Only the model name carries over. The legacy call_gemini flashcard path also pinned temperature=0.7, max_output_tokens=8192, and thinking_budget=0 (thinking disabled) — none of which this agent replicates.
Flashcard generation / cleanup / cloze now run on GoogleModel defaults: dynamic thinking enabled (instead of budget 0) and provider-default temperature (instead of the pinned 0.7) → higher per-call latency / cost and less deterministic output. Either replicate those settings on the agent or drop the 'matches the legacy default' claim so it doesn't hide the flip from reviewers.
| def _run_flashcard_agent(prompt: str) -> list[Card]: | ||
| """Run the flashcard agent on a rendered prompt and return front/back dicts. | ||
| Filters out cards missing either side (preserving `_parse_card_json`'s |
There was a problem hiding this comment.
🟡 P3 — _parse_card_json is now dead code. Every caller was rewired to _run_flashcard_agent; the ~18-line parser (around line 203) is unreferenced apart from this docstring mention. Its front/back strip+filter logic is duplicated inside _run_flashcard_agent, so a future edit to the dead copy silently no-ops. Recommend deleting it.
| @@ -170,91 +170,3 @@ def extract_graph_update(response_text: str) -> tuple: | |||
| conversational = response_text | |||
| return conversational.strip(), graph_update | |||
There was a problem hiding this comment.
🟡 P3 — stale ruff ignore (cleanup). Deleting generate_flashcards removed the last F541 (f-string-without-placeholder) source in this file, but the "services/gemini_service.py" = ["F541"] per-file-ignore in ruff.toml is left behind. ruff.toml states the baseline can only shrink — drop that line so an accidental placeholder-less f-string in this file fails CI again instead of passing silently.
| import zipfile | ||
| from unittest.mock import MagicMock, patch | ||
| from types import SimpleNamespace | ||
| from unittest.mock import AsyncMock, MagicMock, patch |
There was a problem hiding this comment.
🟡 P3 — import json (line 3) is now unused after the rewrite dropped the json.dumps(...) payloads in favor of Flashcards objects. It's tolerated only by the F401 baseline the ruff docs ask to burn down — removing it shrinks the baseline rather than growing it.
Resolve the conflict in backend/agents/_providers.py by keeping both sides: the branch's `flashcard` agent task alongside main's `course_summary`, `quiz_context`, and `concept_scan` additions in both AgentTask and _DEFAULTS. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve AndresL230's review of the flashcard → Pydantic AI migration.
_run_flashcard_agent no longer swallows every failure into an empty list:
- Transport / runtime errors (missing GEMINI_API_KEY, run_agent_sync event-loop
misuse, network) now propagate so routes surface a retryable 502 instead of a
misleading 200 {"cards": []} — the existing 502/422 handlers are live again. (P1)
- Restore the one-retry-with-~2s-backoff on transient HTTP 429/5xx that the old
call_gemini(retries=1) path provided. (P2)
- The event-loop RuntimeError is no longer masked. (P3)
- Only genuine bad model output (UnexpectedModelBehavior) degrades to [], and every
failure path logs via logger.exception — never silent.
Pin the flashcard agent's model_settings (temperature=0.7, max_tokens=8192,
thinking_budget=0) to match the legacy call_gemini path instead of silently
inheriting GoogleModel defaults, and correct the misleading _providers.py comment
that claimed full legacy parity from the model name alone. (P2)
Delete the now-dead _parse_card_json (all callers use _run_flashcard_agent). (P3)
ruff.toml: drop the stale services/gemini_service.py F541 ignore (no F541 source
remains after generate_flashcards moved out), and shrink the test file's ignore to
just E402 by removing the now-unused `json` and `time` imports. (P3)
Update tests to the new contract: bad output → [], transport errors propagate,
transient 429/5xx retries once. Full backend suite green (live-Gemini integration
tests aside, which are CI-skipped without GEMINI_API_KEY).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>There was a problem hiding this comment.
🧹 Nitpick comments (2)
specs/146-flashcard-agent.md (1)
28-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpec text is stale vs. the implemented error contract.
R2 says
_run_flashcard_agent"degrades to[]on agent failure ... no raise," and acceptance criterion 3 repeats "returns[]on agent failure." The shipped implementation is more nuanced: only bad/invalid model output (UnexpectedModelBehavior) degrades to[]; transient HTTP 429/5xx retries once; everything else (transport, runtime, non-transient HTTP) propagates. This was an explicit, deliberate follow-up fix (per the PR's own commit messages) to avoid silently swallowing real outages — the spec should reflect that nuance so it isn't read as license to regress to blanket-catch behavior later.📝 Suggested wording
-- Add `flashcard_import_service._run_flashcard_agent(prompt: str) -> list[Card]`: runs the agent via- `run_agent_sync`, returns `{front, back}` dicts with empty-front/back filtered out (preserving- `_parse_card_json`'s filtering), and **degrades to `[]` on agent failure** (preserving the old- "bad output → []" resilience — no raise).+- Add `flashcard_import_service._run_flashcard_agent(prompt: str) -> list[Card]`: runs the agent via+ `run_agent_sync`, returns `{front, back}` dicts with empty-front/back filtered out (preserving+ `_parse_card_json`'s filtering), and **degrades to `[]` only on unusable/invalid model output**+ (`UnexpectedModelBehavior`). Transient HTTP 429/5xx retries once; all other failures (transport,+ runtime, non-transient HTTP) propagate so the route surfaces a retryable 502.Also applies to: 63-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/146-flashcard-agent.md` around lines 28 - 39, Update the R2 specification and acceptance criterion 3 to document the implemented _run_flashcard_agent error contract: return [] only for invalid model output represented by UnexpectedModelBehavior, retry transient HTTP 429/5xx failures once, and propagate transport, runtime, and other non-transient HTTP errors. Remove wording that implies blanket failure swallowing or unconditional [] on agent failure.backend/tests/test_flashcard_import_service.py (1)
378-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
documents=/context=prompt-building branches.
TestGenerateFlashcardsonly exercisestopic/weak_concepts; thedoc_blocks/concept_notesloop andextra_blocklogic ingenerate_flashcards(services/flashcard_import_service.py:322-361) — including thecategory.upper()call flagged above — has no test coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_flashcard_import_service.py` around lines 378 - 392, Add tests in TestGenerateFlashcards that call generate_flashcards with documents/context inputs, exercising both the doc_blocks/concept_notes loop and extra_block prompt branch. Assert the generated prompt includes the supplied document and concept content, and use a category value that verifies category.upper() is applied while preserving existing topic/weak-concepts coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/tests/test_flashcard_import_service.py`:
- Around line 378-392: Add tests in TestGenerateFlashcards that call
generate_flashcards with documents/context inputs, exercising both the
doc_blocks/concept_notes loop and extra_block prompt branch. Assert the
generated prompt includes the supplied document and concept content, and use a
category value that verifies category.upper() is applied while preserving
existing topic/weak-concepts coverage.
In `@specs/146-flashcard-agent.md`:
- Around line 28-39: Update the R2 specification and acceptance criterion 3 to
document the implemented _run_flashcard_agent error contract: return [] only for
invalid model output represented by UnexpectedModelBehavior, retry transient
HTTP 429/5xx failures once, and propagate transport, runtime, and other
non-transient HTTP errors. Remove wording that implies blanket failure
swallowing or unconditional [] on agent failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ab7cc72b-91a2-4970-897a-12a8dc9ae17f
📒 Files selected for processing (8)
backend/agents/_providers.pybackend/agents/flashcard.pybackend/routes/flashcards.pybackend/ruff.tomlbackend/services/flashcard_import_service.pybackend/services/gemini_service.pybackend/tests/test_flashcard_import_service.pyspecs/146-flashcard-agent.md
💤 Files with no reviewable changes (1)
- backend/services/gemini_service.py
…act (#146) Address remaining CodeRabbit nitpicks on the flashcard agent migration PR; Andres's P1-P3 findings were already resolved in 7b57acc. - Add a TestGenerateFlashcards case exercising the documents/concept_notes loop and the free-text context branch, asserting the rendered prompt carries the document block (with upper-cased category), concept notes, and context, while preserving the existing topic/weak-concept coverage. - Update spec R2, acceptance criterion 3, and R5 to document the implemented _run_flashcard_agent error contract ([] only on UnexpectedModelBehavior, retry transient 429/5xx once, propagate transport/runtime/non-transient HTTP) instead of the stale "blanket [] on failure" wording. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes#146 (Agent-migration epic #152, milestone #2).
What
Routes all five flashcard LLM seams through one
flashcard_agent— nogemini_serviceimport remains in the flashcard path.extract_cards_from_image(OCR-split)call_geminiflashcard_agentgemini_generate_cardscall_geminiflashcard_agentgemini_cleanup_cardscall_geminiflashcard_agentgemini_clozecall_geminiflashcard_agentgenerate_flashcards(main AI gen)gemini_service.generate_flashcardsflashcard_import_service, runsflashcard_agentNotes
agents/flashcard.py—Flashcards { cards: list[FlashCard{front, back}] }; one agent, task instructions stay in the existing prompt templates (passed as the user message). Registeredflashcard(gemini-2.5-flash) in_providers._run_flashcard_agent(prompt)filters cards missing a side (preserving_parse_card_json's behavior) and degrades to[]on agent failure — matching the old "bad LLM output → []" resilience (never raises).gemini_cleanup_cardsstill falls back to its input on empty cleanup;extract_cards_from_imagestill short-circuits on empty OCR without calling the agent.generate_flashcardsmoved offgemini_service(prompt-building verbatim);routes/flashcards.pyimports it fromflashcard_import_service.Testing
call_geminipatches intest_flashcard_import_service.pyto mockflashcard_agent; added empty-filter, agent-failure→[], cleanup-fallback, andgenerate_flashcardstests. Route tests are unchanged (they patch the whole functions).test_storage_servicefailures pre-exist onmain— missing SUPABASE env).ruffclean.Out of scope: the non-LLM parsers (
parse_xlsx/parse_anki_apkg/scrape_quizlet_url/dedup), and deleting the rest ofservices/gemini_service.py(#151).🤖 Generated with Claude Code
Summary by CodeRabbit