feat: add madhhab-aware fiqh answers with school attribution (#39) - #48
feat: add madhhab-aware fiqh answers with school attribution (#39)#48GEEKYFOCUS wants to merge 1 commit into
Conversation
…idge#39) - Add fiqh.py module with madhhab normalization and fiqh question classifier - Add madhhab request field to ChatRequest - Add ikhtilaf methodology to Islamic context and FIQH_INSTRUCTIONS - Add optional fiqh metadata to ChatResponse - Add 14 eval cases for comparative fiqh questions - Add unit tests (40 tests, all offline) - Update CI config for new module
WalkthroughChangesThe chat API now accepts an optional madhhab, classifies fiqh questions, conditionally applies comparative fiqh instructions, and returns fiqh metadata. A new module provides normalization and Gemini-backed classification, with offline tests, fixtures, and CI coverage. Fiqh-aware chat
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatClient
participant ChatHandler
participant FiqhClassifier
participant GeminiModel
ChatClient->>ChatHandler: Send prompt and optional madhhab
ChatHandler->>FiqhClassifier: Classify prompt
FiqhClassifier->>GeminiModel: Classify ambiguous prompt
GeminiModel-->>FiqhClassifier: Return is_fiqh result
FiqhClassifier-->>ChatHandler: Return classification
ChatHandler->>GeminiModel: Send assembled prompt
GeminiModel-->>ChatHandler: Return answer
ChatHandler-->>ChatClient: Return answer and fiqh metadata
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_fiqh.py (2)
170-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEval-case tests silently skip ambiguous cases, and richer fixture fields go unchecked.
test_all_eval_cases_classify_correctlyonly asserts when_quick_is_fiqh(...)returns non-None(Line 177). Since_quick_is_fiqhreturnsNonewhenever no fiqh keyword matches, non-fiqh fixture cases without fiqh keywords (e.g. the Musa/Pharaoh and 99-Names cases) are likely never actually checked against theiris_fiqh: falseground truth — the test passes trivially for them regardless of correctness. Given the PR explicitly calls for "offline unit tests with mocked Gemini calls" to validate the eval cases, consider exercisingFiqhClassifierwith a mocked model (returning per-case expectations) for the cases the quick filter can't resolve, so the fixture's ground truth is actually exercised end-to-end.Separately,
test_eval_cases_have_required_fields(Line 190-195) only checks forid/question/is_fiqh, so the fixture'sexpects_madhhabs,expects_consult_scholar, andexpects_madhhab_leadfields (tests/fixtures/fiqh_eval_cases.jsonl) are defined but never asserted anywhere — worth confirming whether these are intended for a future/manual eval harness or should be wired into these tests now.🤖 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 `@tests/test_fiqh.py` around lines 170 - 196, Update TestEvalCasesClassification so ambiguous cases where _quick_is_fiqh returns None are validated through FiqhClassifier using a mocked Gemini/model response that matches each case’s expected is_fiqh value, while preserving the quick-path assertions. Extend test_eval_cases_have_required_fields to validate expects_madhhabs, expects_consult_scholar, and expects_madhhab_lead when those fixture fields are part of the contract, and assert their expected values through the classifier result where supported.
228-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the real prompt assembly path in these tests.
TestFiqhPromptAssemblyrebuildsfull_promptinline, so regressions inchat()’s actualbuild_full_prompt()logic can slip through. Call the handler with a mocked Gemini client, or extract the prompt builder into a helper and assert on that output instead of duplicating the concatenation in the test.🤖 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 `@tests/test_fiqh.py` around lines 228 - 278, Update the tests in TestFiqhPromptAssembly to exercise chat()’s real prompt assembly through build_full_prompt(), using a mocked Gemini client where needed. Replace the inline full_prompt concatenation in the fiqh and non-fiqh cases with assertions against the handler-produced prompt, while preserving checks for FIQH_INSTRUCTIONS, madhhab guidance, and Islamic context.
🤖 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.
Inline comments:
In `@fiqh.py`:
- Around line 51-105: Update FIQH_KEYWORDS and the classification logic to
remove the "nullif" entry, match literal keywords with escaped word-boundary
patterns instead of raw substring searches, and keep intentional regex
expressions such as "raises.*hands.*prayer" in a separate pattern list. Apply
the same matching behavior in the logic referenced around the additional
classification block.
In `@main.py`:
- Around line 177-180: Update the `/chat` flow around `FiqhClassifier` to
construct it with the configured Gemini model so uncertain prompts can reach
`_classify_fiqh()` and use the dedicated fiqh prompt. Because this
classification may perform synchronous Gemini I/O, run it without blocking the
event loop or switch to the classifier’s async client while preserving the
existing `is_fiqh` decision flow.
---
Nitpick comments:
In `@tests/test_fiqh.py`:
- Around line 170-196: Update TestEvalCasesClassification so ambiguous cases
where _quick_is_fiqh returns None are validated through FiqhClassifier using a
mocked Gemini/model response that matches each case’s expected is_fiqh value,
while preserving the quick-path assertions. Extend
test_eval_cases_have_required_fields to validate expects_madhhabs,
expects_consult_scholar, and expects_madhhab_lead when those fixture fields are
part of the contract, and assert their expected values through the classifier
result where supported.
- Around line 228-278: Update the tests in TestFiqhPromptAssembly to exercise
chat()’s real prompt assembly through build_full_prompt(), using a mocked Gemini
client where needed. Replace the inline full_prompt concatenation in the fiqh
and non-fiqh cases with assertions against the handler-produced prompt, while
preserving checks for FIQH_INSTRUCTIONS, madhhab guidance, and Islamic context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 122a8b00-73c6-4155-9462-85ec3c6345dc
📒 Files selected for processing (6)
.github/workflows/ci.ymlfiqh.pymain.pytests/conftest.pytests/fixtures/fiqh_eval_cases.jsonltests/test_fiqh.py
| FIQH_KEYWORDS: list[str] = [ | ||
| "halal", | ||
| "haram", | ||
| "makruh", | ||
| "mustahab", | ||
| "mubah", | ||
| "wajib", | ||
| "fard", | ||
| "sunna", | ||
| "sunnah", | ||
| "bidah", | ||
| "wudu", | ||
| "wudhu", | ||
| "ghusl", | ||
| "tayammum", | ||
| "salah", | ||
| "salat", | ||
| "prayer", | ||
| "zakat", | ||
| "sawm", | ||
| "fasting", | ||
| "hajj", | ||
| "umrah", | ||
| "talaq", | ||
| "divorce", | ||
| "nikah", | ||
| "marriage", | ||
| "mahr", | ||
| "dowry", | ||
| "riba", | ||
| "interest", | ||
| "gharar", | ||
| "maysir", | ||
| "fiqh", | ||
| "ruling", | ||
| "fatwa", | ||
| "madhab", | ||
| "madhhab", | ||
| "raises.*hands.*prayer", | ||
| "wiping.*socks", | ||
| "masah", | ||
| "break.*wudu", | ||
| "nullif", | ||
| "invalidat", | ||
| "qiyam", | ||
| "ruku", | ||
| "sujood", | ||
| "tashahhud", | ||
| "dhabiha", | ||
| "slaughter", | ||
| "awrah", | ||
| "hijab", | ||
| "niqab", | ||
| "dress code", | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prevent non-fiqh prompts from being classified by substring matches.
re.search() matches substrings, so “interesting” matches interest; NULLIF SQL questions match nullif. These bypass the fallback and incorrectly apply fiqh-only instructions. Remove nullif, use word-boundary escaped matching for literal keywords, and keep intentional regex entries in a separate pattern list.
Also applies to: 178-191
🤖 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 `@fiqh.py` around lines 51 - 105, Update FIQH_KEYWORDS and the classification
logic to remove the "nullif" entry, match literal keywords with escaped
word-boundary patterns instead of raw substring searches, and keep intentional
regex expressions such as "raises.*hands.*prayer" in a separate pattern list.
Apply the same matching behavior in the logic referenced around the additional
classification block.
| madhhab = normalize_madhhab(request.madhhab) | ||
|
|
||
| classifier = FiqhClassifier() | ||
| is_fiqh = classifier.is_fiqh_question(request.prompt) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## main.py outline\n'
ast-grep outline main.py --view expanded || true
printf '\n## fiqh classifier files\n'
fd -a 'fiqh' . || true
fd -a 'classifier' . || true
printf '\n## search for FiqhClassifier usages/definition\n'
rg -n "class FiqhClassifier|FiqhClassifier\\(|is_fiqh_question\\(" . || true
printf '\n## main.py around the cited lines\n'
nl -ba main.py | sed -n '150,210p' || trueRepository: Deen-Bridge/dnb-ai
Length of output: 2436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## FiqhClassifier definition and related code\n'
rg -n -C 4 "class FiqhClassifier|def __init__|def is_fiqh_question|normalize_madhhab|async def chat" . || trueRepository: Deen-Bridge/dnb-ai
Length of output: 7647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## main.py top and /chat body\n'
sed -n '1,120p' main.py
printf '\n## main.py /chat continuation\n'
sed -n '170,240p' main.py
printf '\n## fiqh.py classifier implementation\n'
sed -n '149,260p' fiqh.py
printf '\n## search for any Gemini model wiring into FiqhClassifier\n'
rg -n "genai_model|FiqhClassifier\\(|configure\\(|google.generativeai|GenerativeModel|genai\\.GenerativeModel|api_key" main.py fiqh.py tests . || trueRepository: Deen-Bridge/dnb-ai
Length of output: 13329
Pass the Gemini model into the fiqh classifier
FiqhClassifier() is created without a model here, so uncertain prompts never reach _classify_fiqh() and default to False. That leaves the dedicated fiqh prompt unused for ambiguous questions. If this call is meant to hit Gemini from /chat, keep it off the event loop or use an async client so the endpoint stays responsive.
🤖 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 `@main.py` around lines 177 - 180, Update the `/chat` flow around
`FiqhClassifier` to construct it with the configured Gemini model so uncertain
prompts can reach `_classify_fiqh()` and use the dedicated fiqh prompt. Because
this classification may perform synchronous Gemini I/O, run it without blocking
the event loop or switch to the classifier’s async client while preserving the
existing `is_fiqh` decision flow.
Source: Path instructions
|
@GEEKYFOCUS fix conflict so I can merge asap |
2 participants
Overview
This PR adds madhhab-aware fiqh answer capabilities to the DeenBridge AI chat endpoint. When a user asks a fiqh (Islamic jurisprudence) question, the assistant now presents the positions of the four major Sunni schools (Hanafi, Maliki, Shafi'i, Hanbali) with attribution instead of a single flattened ruling.
Changes
🔍 Fiqh Question Detection (new
fiqh.py)🧭 Madhhab Request Field
ChatRequest.madhhab: Optional[str]— validated against{hanafi, maliki, shafii, hanbali}shafi'i,Shafie,shafii→shafii`)Nonewith a warning (no 422)📜 Ikhtilaf Methodology in System Prompt
ISLAMIC_CONTEXTupdated: replaced "Avoid controversial or divisive topics" with explicit acknowledgment of scholarly differenceFIQH_INSTRUCTIONS: state agreement first, attribute each school, never rank, cite evidence, consult-scholar closing for personal rulings📡 Response Metadata
ChatResponse.fiqh: Optional[FiqhMetadata]withis_fiqh_questionandmadhhab_requested🧪 Tests & Evaluation
tests/fixtures/fiqh_eval_cases.jsonlVerification
madhhab: \"shafii\"→ leads with Shafi'i positionChatResponsecarries optional fiqh metadataRelated Issue
Closes #39
Verification commands:
pytest tests/test_fiqh.py -q✅ 40 passedpython -m flake8 main.py fiqh.py tests/test_fiqh.py --max-line-length=120 --ignore=E501,W503✅ (no output)Summary by CodeRabbit
New Features
Tests