feat: madhhab-aware fiqh answers with ikhtilaf methodology - #51
Conversation
- fiqh.py: fiqh-question classifier (keyword pre-filter), madhhab normalization, FiqhInfo model, and ikhtilaf methodology system-prompt blocks - main.py: madhhab field on ChatRequest, fiqh detection + context injection in /chat handler, FiqhInfo on ChatResponse - tests/test_fiqh.py: 38 unit tests for madhhab norm, keyword pre-filter, context structure, and eval case validation - tests/fixtures/fiqh_cases.jsonl: 9 comparative-fiqh eval cases - CI: extend lint/syntax to fiqh.py, add pytest step for fiqh tests Closes Deen-Bridge#39
WalkthroughAdds madhhab-aware fiqh detection to chat requests, enriches fiqh prompts with school-specific context, returns classification metadata, and adds offline tests, fixtures, linting, syntax checks, and CI execution for the new module. ChangesFiqh-aware chat flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatRequest
participant classify_fiqh
participant generate
participant ChatResponse
Client->>ChatRequest: prompt and optional madhhab
ChatRequest->>classify_fiqh: classify prompt
classify_fiqh-->>ChatRequest: FiqhInfo
ChatRequest->>generate: enriched system context
generate-->>ChatResponse: generated text and FiqhInfo
ChatResponse-->>Client: response metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/workflows/ci.ymlTraceback (most recent call last): 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fixtures/fiqh_cases.jsonl (1)
10-10: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the invalid trailing JSONL record.
Line 10 parses as the integer
10, so the loader later attempts dictionary access on an integer and the fiqh tests fail before completing.🤖 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/fixtures/fiqh_cases.jsonl` at line 10, Remove the invalid trailing JSONL record from the fiqh fixture so every line contains a JSON object; specifically delete the standalone integer record `10` that causes dictionary access to fail during fixture loading.
🤖 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 @.github/workflows/ci.yml:
- Around line 31-34: Update the flake8 and compileall commands in the CI
workflow to include tests/test_fiqh.py, or target the complete tests directory,
so the new test module is covered by lint and syntax checks.
In `@fiqh.py`:
- Around line 65-76: Update classify_fiqh so keyword_match remains only a
pre-filter and keyword hits do not automatically return True when classify_fn is
absent. Ensure the /chat call path supplies or invokes semantic classification,
or explicitly narrow the classifier contract and adjust expected cases
accordingly; add coverage for both positive and negative classifications,
including substring and broad-keyword matches.
- Around line 81-84: Update the fiqh/madhhab methodology instruction to say
“points of agreement, when present” rather than unconditionally invoking ijma'.
Preserve the requirement to present genuine agreement among the schools first,
without implying consensus exists for every fiqh question.
In `@main.py`:
- Around line 186-190: The semantic-cache identity must account for the
normalized madhhab when handling fiqh requests. Update the cache lookup and
storage flow associated with `madhhab`, `is_fiqh`, and `fiqh_info` to either
include the normalized madhhab in the cache key or bypass caching for
madhhab-aware fiqh requests, preventing responses from being reused across
different preferences.
- Line 100: Update the ChatRequest.madhhab field to retain permissive string
values while enforcing a bounded Pydantic length constraint. Then update the
semantic-cache key generation to include the normalized madhhab value, ensuring
requests with different schools do not reuse the same cached answer.
In `@tests/test_fiqh.py`:
- Around line 146-167: Add a parametrized test covering every case from
load_cases() that asserts classify_fiqh(case["prompt"]) equals the fixture’s
case["is_fiqh"] value. Place it with the existing fiqh fixture tests and
preserve the current positive keyword-match test.
- Line 6: Add a pinned pytest version to requirements.txt and update the CI
workflow to install dependencies from that file instead of installing pytest ad
hoc, while preserving the existing flake8 installation and test behavior.
---
Outside diff comments:
In `@tests/fixtures/fiqh_cases.jsonl`:
- Line 10: Remove the invalid trailing JSONL record from the fiqh fixture so
every line contains a JSON object; specifically delete the standalone integer
record `10` that causes dictionary access to fail during fixture loading.
🪄 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: 9c15c509-a8f1-4354-a313-1fde3d92c426
📒 Files selected for processing (5)
.github/workflows/ci.ymlfiqh.pymain.pytests/fixtures/fiqh_cases.jsonltests/test_fiqh.py
| run: flake8 main.py stellar.py safety tests/redteam study.py fiqh.py --max-line-length=120 --ignore=E501,W503 | ||
|
|
||
| - name: Check syntax | ||
| run: python -m compileall -q main.py stellar.py safety tests/redteam study.py | ||
| run: python -m compileall -q main.py stellar.py safety tests/redteam study.py fiqh.py |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Include tests/test_fiqh.py in lint and syntax checks.
Both commands omit the new test module, so a flake8 violation or syntax regression there is not covered by the explicit checks. Include tests/test_fiqh.py or target the complete tests directory.
As per path instructions, CI-enforced flake8 coverage should include every changed Python file.
🤖 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 @.github/workflows/ci.yml around lines 31 - 34, Update the flake8 and
compileall commands in the CI workflow to include tests/test_fiqh.py, or target
the complete tests directory, so the new test module is covered by lint and
syntax checks.
Source: Path instructions
| def keyword_match(text: str) -> bool: | ||
| lower = text.casefold() | ||
| return any(kw in lower for kw in FIQH_KEYWORDS) | ||
|
|
||
|
|
||
| def classify_fiqh(prompt: str, classify_fn=None) -> bool: | ||
| kw = keyword_match(prompt) | ||
| if not kw: | ||
| return False | ||
| if classify_fn is None: | ||
| return True | ||
| return classify_fn(prompt) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the keyword scan as a pre-filter, not the final classifier.
classify_fiqh returns True for every keyword hit when classify_fn is omitted, and /chat calls it that way. This marks the fixture’s “How many rak'ahs…” case as fiqh because "rak'ah" is a substring, while broad entries such as "can i" can match unrelated questions. Add the semantic classification step, or narrow the contract and update the expected cases; test both positive and negative classifications.
🤖 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 65 - 76, Update classify_fiqh so keyword_match remains
only a pre-filter and keyword hits do not automatically return True when
classify_fn is absent. Ensure the /chat call path supplies or invokes semantic
classification, or explicitly narrow the classifier contract and adjust expected
cases accordingly; add coverage for both positive and negative classifications,
including substring and broad-keyword matches.
| FIQH / MADHHAB METHODOLOGY: | ||
| When the user asks a fiqh (jurisprudence) question, follow these rules: | ||
|
|
||
| 1. State points of agreement (ijma') among the schools first. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers
sed -n '1,160p' fiqh.py | nl -ba | sed -n '70,95p'
# Search for other consensus/agreement wording in the file
rg -n "ijma|agreement|consensus|madhhab|school" fiqh.pyRepository: Deen-Bridge/dnb-ai
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the relevant region with line numbers using awk
awk 'NR>=70 && NR<=95 { printf "%d:%s\n", NR, $0 }' fiqh.py
# Search for related methodology wording elsewhere
rg -n "ijma|agreement|consensus|madhhab|school" fiqh.pyRepository: Deen-Bridge/dnb-ai
Length of output: 1891
Avoid unconditional ijma' wording
“ijma'” is a technical claim of unanimous consensus, so this should say “points of agreement, when present” instead of treating every fiqh answer as established consensus. That keeps the prompt accurate and avoids the model overstating agreement where there may only be common or majority views.
🤖 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 81 - 84, Update the fiqh/madhhab methodology
instruction to say “points of agreement, when present” rather than
unconditionally invoking ijma'. Preserve the requirement to present genuine
agreement among the schools first, without implying consensus exists for every
fiqh question.
| prompt: str | ||
| chat_id: Optional[str] = None | ||
| context: Optional[str] = None # Additional context for specific queries | ||
| madhhab: Optional[str] = None # User's madhhab: hanafi, maliki, shafii, hanbali |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== file list ==\n'
git ls-files main.py
printf '\n== line count ==\n'
wc -l main.py
printf '\n== main.py around request model ==\n'
sed -n '1,160p' main.py | cat -n
printf '\n== main.py around cache flow ==\n'
sed -n '200,280p' main.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 10208
🏁 Script executed:
set -euo pipefail
printf '\n== madhhab-related code in main.py ==\n'
rg -n "madhhab|normalize_madhhab|fiqh_info|semantic_cache|embed_text|normalize_text" main.py
printf '\n== fiqh.py outline ==\n'
ast-grep outline fiqh.py --view expanded
printf '\n== fiqh.py madhhab helpers ==\n'
sed -n '1,220p' fiqh.py | cat -n
printf '\n== semantic_cache.py outline ==\n'
ast-grep outline semantic_cache.py --view expanded
printf '\n== semantic_cache.py key functions ==\n'
sed -n '1,240p' semantic_cache.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 15311
🏁 Script executed:
set -euo pipefail
sed -n '176,220p' main.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 2548
Bound ChatRequest.madhhab and include it in the cache identity.
Keep unknown madhhab values permissive, but add a bounded Pydantic constraint (for example max_length) so arbitrarily large raw input is rejected. Also vary the semantic-cache key on the normalized madhhab, since the answer changes when the school-specific instruction is added.
🤖 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` at line 100, Update the ChatRequest.madhhab field to retain
permissive string values while enforcing a bounded Pydantic length constraint.
Then update the semantic-cache key generation to include the normalized madhhab
value, ensuring requests with different schools do not reuse the same cached
answer.
Source: Path instructions
| # --- Fiqh classification & madhhab --- | ||
| madhhab = normalize_madhhab(request.madhhab) | ||
| is_fiqh = classify_fiqh(request.prompt) | ||
| fiqh_info = FiqhInfo(is_fiqh_question=is_fiqh, madhhab_requested=madhhab) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the semantic-cache key madhhab-aware.
The normalized madhhab changes the generated prompt, but the cache lookup is keyed only by the prompt embedding. A response generated for a Shafi‘i user can therefore be returned to a Hanafi or no-preference user while fiqh_info reports the current request’s preference. Bypass caching for madhhab-aware fiqh requests or include the normalized madhhab in the cache identity.
🤖 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 186 - 190, The semantic-cache identity must account for
the normalized madhhab when handling fiqh requests. Update the cache lookup and
storage flow associated with `madhhab`, `is_fiqh`, and `fiqh_info` to either
include the normalized madhhab in the cache key or bypass caching for
madhhab-aware fiqh requests, preventing responses from being reused across
different preferences.
| import json | ||
| import os | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files | rg '(^|/)(requirements(\.txt)?|pyproject\.toml|poetry\.lock|Pipfile(\.lock)?|tox\.ini|noxfile\.py|\.github/workflows/.*\.ya?ml|tests/test_fiqh\.py)$'
printf '\n== requirements-like files ==\n'
for f in requirements.txt pyproject.toml poetry.lock Pipfile Pipfile.lock tox.ini noxfile.py .github/workflows; do
if [ -e "$f" ]; then
echo "--- $f ---"
if [ -d "$f" ]; then
find "$f" -maxdepth 2 -type f | sort
else
sed -n '1,220p' "$f"
fi
fi
done
printf '\n== test file ==\n'
sed -n '1,220p' tests/test_fiqh.pyRepository: Deen-Bridge/dnb-ai
Length of output: 5672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' .github/workflows/ci.ymlRepository: Deen-Bridge/dnb-ai
Length of output: 1311
Pin pytest in the dependency set. tests/test_fiqh.py imports pytest, but CI installs it ad hoc with pip install pytest flake8 instead of from requirements.txt, so the test environment isn’t reproducible. Add a pinned pytest entry and install from the pinned file in CI.
🤖 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` at line 6, Add a pinned pytest version to
requirements.txt and update the CI workflow to install dependencies from that
file instead of installing pytest ad hoc, while preserving the existing flake8
installation and test behavior.
Source: Path instructions
| def test_fiqh_cases_jsonl_loads(): | ||
| cases = load_cases() | ||
| assert len(cases) >= 8 | ||
| for case in cases: | ||
| assert "prompt" in case | ||
| assert "is_fiqh" in case | ||
| assert "expect_schools" in case | ||
|
|
||
|
|
||
| def test_non_fiqh_cases_have_empty_schools(): | ||
| for case in load_cases(): | ||
| if not case["is_fiqh"]: | ||
| assert case["expect_schools"] == [] | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Keyword pre-filter: eval cases | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| @pytest.mark.parametrize("case", [c for c in load_cases() if c["is_fiqh"]], ids=lambda c: c["prompt"][:40]) | ||
| def test_fiqh_cases_keyword_match_true(case): | ||
| assert keyword_match(case["prompt"]), f"Expected keyword match for fiqh case: {case['prompt'][:60]}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the fixture’s is_fiqh contract.
These tests only validate JSON structure and positive keyword matches. Add a parametrized assertion that classify_fiqh(case["prompt"]) == case["is_fiqh"] for every fixture case; this will catch false-positive regressions in the non-fiqh cases.
🤖 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 146 - 167, Add a parametrized test covering
every case from load_cases() that asserts classify_fiqh(case["prompt"]) equals
the fixture’s case["is_fiqh"] value. Place it with the existing fiqh fixture
tests and preserve the current positive keyword-match test.
2 participants
Closes #39
Summary
Makes the assistant madhhab-aware: detects fiqh questions via keyword pre-filter, presents positions of all four major Sunni schools with attribution, and supports an optional
madhhabfield onChatRequestto lead with the user's preferred school.Changes
fiqh.py— new module: keyword-based fiqh-question classifier, madhhab normalization (handlesshafi'i,shafie, etc.),FiqhInforesponse model, ikhtilaf methodology system-prompt blocksmain.py—madhhabfield onChatRequest(validated permissively: unknown values →Nonewith warning), fiqh detection + context injection in/chathandler,fiqhmetadata onChatResponsetests/test_fiqh.py— 38 unit tests (madhhab normalization, keyword pre-filter, context structure, eval case validation)tests/fixtures/fiqh_cases.jsonl— 9 comparative-fiqh eval cases (6 fiqh + 3 non-fiqh).github/workflows/ci.yml— lint/syntax extended tofiqh.py, new pytest step for fiqh testsKey design decisions
classify_fnparameter is already wired).FIQH_IKHTILAF_CONTEXTis appended toISLAMIC_CONTEXTonly when a fiqh question is detected; non-fiqh answers are unaffected.Nonewith a warning, mirroring the existing safety-pipeline pattern — no 422 errors for typos.Summary by CodeRabbit
New Features
Tests