chore: adopt ruff + mypy with full type hints in CI - #85
Conversation
Replaces the self-contradictory flake8 gate (max-line-length=120 with E501 ignored, no import sorting, no formatter, no type checking) with ruff and mypy, both configured in a single pyproject.toml and pinned in requirements-dev.txt so CI is reproducible. - ruff: E, F, I, UP, B, W at line length 120; `ruff format` applied across the repo (venv/ and data/ excluded) - mypy: pydantic plugin, disallow_untyped_defs for main.py, with ignore_missing_imports scoped to google.*/stellar_sdk.*/redis.*/yaml.* rather than set globally - main.py: return and parameter annotations on every function, including the route handlers and the SSE generator - annotations elsewhere needed to make `mypy .` pass: overloads on redact_secret_keys, an Awaitable-aware generator type on SafetyPipeline.run_async, and a handful of local type fixes - CI: flake8 and compileall steps replaced by ruff check, ruff format --check, and mypy - CONTRIBUTING.md documents the local commands; optional pre-commit config wires up the same tools Behaviour-preserving: formatting, annotations, and exception chaining (`raise ... from exc`) only. The B019 lru_cache-on-method finding in hadith.py is left in place with a note, since changing that caching strategy is a behaviour change that belongs in its own PR. Closes Deen-Bridge#21
WalkthroughThis PR replaces Flake8 and compile checks with Ruff formatting/linting and Mypy, adds pre-commit configuration and contributor guidance, and modernizes Python type annotations across application, storage, integration, telemetry, and test modules. ChangesDeveloper tooling and CI
Application and domain typing
Validation and tests
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
🧹 Nitpick comments (3)
main.py (1)
145-158: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounding
prompt/contextlength like the feedback model.
FeedbackRequest.prompt/answer/commentare explicitly length-capped with a documented rationale (preventing multi-megabyte bodies), butChatRequest.promptandcontexthave no such bound. An oversized prompt still gets redacted, classified, and forwarded to the Gemini API on every request, so a large body costs real work (and tokens) before any downstream limit kicks in.✏️ Proposed bound
- prompt: str + prompt: str = Field(..., max_length=8000) chat_id: str | None = None - context: str | None = None # Additional context for specific queries + context: str | None = Field(None, max_length=8000) # Additional context for specific queriesAs per path instructions, this file is a FastAPI service where CI enforces flake8/Ruff style checks and reviewers should flag "missing Pydantic validation on request bodies."
🤖 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 145 - 158, Update the ChatRequest model’s prompt and context fields with explicit Pydantic maximum-length validation, matching the existing FeedbackRequest bounds and rationale. Preserve their optional/required semantics while ensuring oversized request bodies are rejected before redaction, classification, or downstream API calls.Source: Path instructions
store.py (1)
42-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParameterize the helper’s return type.
While touching this helper, replace the bare
listreturn annotation withlist[protos.Content]or the SDK’s concrete content type. The current annotation permits an uncheckedAny-typed result under strict generic checks.🤖 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 `@store.py` around lines 42 - 44, Update the dicts_to_contents return annotation from bare list to list[protos.Content] (or the SDK’s concrete Content type), matching the objects constructed by the helper and preserving its existing behavior.review_store.py (1)
118-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep Redis persistence clients type-checked instead of using
Any.Both modules now disable mypy checking for their Redis calls. Preserve the concrete client type or introduce a shared narrow protocol for the methods each store uses.
review_store.py#L118-L120: replace_redis: Anywith the supported async Redis client type or protocol.store.py#L55-L56: replace_redis: Anywith the supported async Redis client type or protocol.🤖 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 `@review_store.py` around lines 118 - 120, Replace the Any annotation for _redis in review_store.py lines 118-120 and store.py lines 55-56 with the supported async Redis client type, or a shared narrow protocol covering the Redis methods used by both stores; retain the existing _use_redis behavior and keep Redis calls type-checked.
🤖 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 @.pre-commit-config.yaml:
- Around line 15-24: Update the mypy hook configuration around
additional_dependencies so its isolated environment matches CI: include the
complete pinned dependency set from requirements.txt and requirements-dev.txt,
including pytest and pytest-asyncio, and pin currently unpinned entries such as
pydantic, pydantic-settings, and types-PyYAML; alternatively configure the hook
to run against the repository’s managed environment.
In `@feedback.py`:
- Around line 459-460: Update _fetch_keys to return an empty list immediately
when limit is zero or negative, before calling zrevrange; preserve the existing
Redis range query for positive limits so list_records remains consistent with
SQLite.
In `@review.py`:
- Around line 165-167: Add Pydantic validation to the verdict request model
fields corrected_answer, reviewer, and note: enforce appropriate max_length
limits and reject whitespace-only string values before persistence or
cache/export processing. Keep corrected_answer optional while validating any
provided value, and ensure the FastAPI request body uses this validated model.
- Around line 67-68: Update the token validation condition around the
X-Review-Token check to handle non-ASCII strings before calling
secrets.compare_digest, either by comparing encoded bytes or explicitly
rejecting non-ASCII input. Ensure malformed tokens consistently raise the
existing HTTPException with status 401 rather than producing an internal error.
In `@safety/pipeline.py`:
- Around line 33-37: Update run_async and its _complete_async flow to use
inspect.isawaitable for all awaitable results, including Futures and custom
awaitables. Offload synchronous generator execution with asyncio.to_thread
before handling its result, while preserving direct awaiting for async-capable
results and the existing SafetyResult behavior.
---
Nitpick comments:
In `@main.py`:
- Around line 145-158: Update the ChatRequest model’s prompt and context fields
with explicit Pydantic maximum-length validation, matching the existing
FeedbackRequest bounds and rationale. Preserve their optional/required semantics
while ensuring oversized request bodies are rejected before redaction,
classification, or downstream API calls.
In `@review_store.py`:
- Around line 118-120: Replace the Any annotation for _redis in review_store.py
lines 118-120 and store.py lines 55-56 with the supported async Redis client
type, or a shared narrow protocol covering the Redis methods used by both
stores; retain the existing _use_redis behavior and keep Redis calls
type-checked.
In `@store.py`:
- Around line 42-44: Update the dicts_to_contents return annotation from bare
list to list[protos.Content] (or the SDK’s concrete Content type), matching the
objects constructed by the helper and preserving its existing behavior.
🪄 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: 55a812e6-af47-4646-9a82-0bb02bde890a
📒 Files selected for processing (57)
.github/workflows/ci.yml.pre-commit-config.yamlCONTRIBUTING.mdadhkar.pycitations.pyconfidence.pycorpus.pyfeedback.pyfiqh.pyhadith.pymain.pymemory/__init__.pymemory/extraction.pymemory/models.pymemory/store.pynisab.pypyproject.tomlrequirements-dev.txtreview.pyreview_store.pysafety/input_gate.pysafety/output_check.pysafety/pipeline.pysafety/policy.pyscripts/build_surah_index.pyscripts/eval_citations.pyscripts/export_eval_candidates.pysemantic_cache.pystellar.pystore.pystudy.pytafsir.pytelemetry.pytest/test_verifier.pytests/conftest.pytests/redteam/test_live.pytests/redteam/test_pipeline.pytests/test_adhkar.pytests/test_chat_robustness.pytests/test_citations.pytests/test_citations_integration.pytests/test_confidence.pytests/test_feedback.pytests/test_fiqh.pytests/test_hadith.pytests/test_memory_extraction.pytests/test_memory_integration.pytests/test_memory_profile.pytests/test_multilingual.pytests/test_purchases.pytests/test_review_queue.pytests/test_semantic_cache.pytests/test_session_persistence.pytests/test_study.pytests/test_tafsir.pytests/test_zakat.pyverifier.py
💤 Files with no reviewable changes (4)
- tests/test_adhkar.py
- tests/conftest.py
- tests/test_memory_extraction.py
- tests/test_memory_profile.py
| # mypy needs the app's own dependencies to resolve imports the way CI does. | ||
| additional_dependencies: | ||
| - fastapi==0.115.12 | ||
| - pydantic>=2.5.3 | ||
| - pydantic-settings>=2.0.0 | ||
| - httpx==0.28.1 | ||
| - numpy==2.2.4 | ||
| - types-PyYAML | ||
| pass_filenames: false | ||
| args: ["."] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the pre-commit mypy environment match CI.
This hook runs mypy . in an isolated environment, but it does not install requirements.txt or requirements-dev.txt; notably, pytest and pytest-asyncio are absent even though the checked tree contains pytest-based tests. It also leaves pydantic, pydantic-settings, and types-PyYAML unpinned, so local results can drift from CI. Add the complete pinned dependency set or run the hook against the repository’s managed environment.
#!/bin/bash
rg -n '^\s*(from|import)\s+(pytest|pytest_asyncio)\b' tests
rg -n 'additional_dependencies|requirements-(dev|txt)' .pre-commit-config.yaml .github/workflows/ci.yml🤖 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 @.pre-commit-config.yaml around lines 15 - 24, Update the mypy hook
configuration around additional_dependencies so its isolated environment matches
CI: include the complete pinned dependency set from requirements.txt and
requirements-dev.txt, including pytest and pytest-asyncio, and pin currently
unpinned entries such as pydantic, pydantic-settings, and types-PyYAML;
alternatively configure the hook to run against the repository’s managed
environment.
| def _fetch_keys(self, index_key: str, limit: int) -> list[str]: | ||
| return self._r.zrevrange(index_key, 0, limit - 1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return no records when limit is zero or negative.
With limit=0, limit - 1 becomes -1, which Redis interprets as the end of the sorted set. list_records(limit=0) can therefore load every record for rating/category queries instead of returning an empty list, unlike SQLite.
Proposed fix
def _fetch_keys(self, index_key: str, limit: int) -> list[str]:
+ if limit <= 0:
+ return []
return self._r.zrevrange(index_key, 0, limit - 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _fetch_keys(self, index_key: str, limit: int) -> list[str]: | |
| return self._r.zrevrange(index_key, 0, limit - 1) | |
| def _fetch_keys(self, index_key: str, limit: int) -> list[str]: | |
| if limit <= 0: | |
| return [] | |
| return self._r.zrevrange(index_key, 0, limit - 1) |
🤖 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 `@feedback.py` around lines 459 - 460, Update _fetch_keys to return an empty
list immediately when limit is zero or negative, before calling zrevrange;
preserve the existing Redis range query for positive limits so list_records
remains consistent with SQLite.
| if not token or not secrets.compare_digest(token, SCHOLAR_REVIEW_TOKEN): | ||
| raise HTTPException( | ||
| status_code=401, detail="A valid X-Review-Token header is required." | ||
| ) | ||
| raise HTTPException(status_code=401, detail="A valid X-Review-Token header is required.") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect review.py with line numbers.
file="$(git ls-files | rg '(^|/)review\.py$' | head -n 1)"
printf 'FILE=%s\n' "$file"
wc -l "$file"
cat -n "$file" | sed -n '1,180p'
printf '\n--- compare_digest probe ---\n'
python3 - <<'PY'
import secrets
cases = [
("ascii", "abc", "abc"),
("ascii-mismatch", "abc", "abd"),
("non-ascii-left", "åbc", "abc"),
("non-ascii-right", "abc", "åbc"),
("bytes-ascii", b"abc", b"abc"),
("bytes-non-ascii", "åbc".encode("utf-8"), "abc".encode("utf-8")),
]
for name, a, b in cases:
try:
result = secrets.compare_digest(a, b)
print(name, "=>", type(result).__name__, result)
except Exception as e:
print(name, "=>", type(e).__name__, str(e))
PYRepository: Deen-Bridge/dnb-ai
Length of output: 1922
🌐 Web query:
Python secrets.compare_digest non-ASCII str TypeError documentation header values latin-1 decoding ASGI
💡 Result:
The Python secrets.compare_digest function is designed for constant-time comparisons to prevent timing attacks [1][2]. It explicitly prohibits comparing strings that contain non-ASCII characters, raising a TypeError if such input is provided [3][4][5]. This restriction exists because constant-time guarantees for Unicode strings are difficult to maintain across different internal string representations [6]. In the context of ASGI applications, header values are provided as raw byte strings [7][8]. While modern HTTP standards suggest headers should be ASCII, historical and practical usage allows a wider range of characters [7][8]. Because these headers are often byte strings, developers attempting to decode them into Unicode (e.g., using UTF-8) before passing them to secrets.compare_digest may trigger a TypeError if the decoded string contains non-ASCII characters [3][4]. To resolve this issue, you should ensure that the values being compared are converted to consistent byte sequences before comparison [4][6]. Encoding the strings to bytes—typically using UTF-8—is the standard approach [9][4]: # Correct approach: encode to bytes before comparing safe_compare = secrets.compare_digest( candidate_str.encode('utf-8'), stored_str.encode('utf-8')) Additionally, note that ASGI headers often follow ISO-8859-1 (latin-1) when decoded, rather than UTF-8 [7][10]. If your application logic requires decoding ASGI header bytes to strings, be aware that many frameworks and utilities (like those for instrumentation) have moved toward using latin-1 or providing fallback mechanisms to avoid crashes when encountering non-ASCII byte sequences [10][11][12].
Citations:
- 1: https://docs.python.org/3.10/library/secrets.html
- 2: https://docs.python.org/3/library/secrets.html
- 3: Fail closed on non-ASCII bearer tokens instead of returning 500 Justinabox/Callstack#173
- 4: [Fix] Password comparison with non-ASCII characters BerriAI/litellm#19568
- 5: [Bug]: Passwords featuring a £ sign cause hashing failure BerriAI/litellm#19559
- 6: https://bugs.python.org/issue14955
- 7: https://asgi.readthedocs.io/en/stable/specs/www.html
- 8: https://asgi.readthedocs.io/en/latest/specs/www.html
- 9: 📝 Update docs for handling HTTP Basic Auth with
secrets.compare_digest()to account for non-ASCII characters fastapi/fastapi#3536 - 10: Assume ISO-8859-1 (instead of UTF-8) encoding for ASGI HTTP headers falconry/falcon#1911
- 11: [asgi] Headers with special characters cause the request to fail open-telemetry/opentelemetry-python-contrib#1814
- 12: Add fallback decoding for asgi headers open-telemetry/opentelemetry-python-contrib#2837
Handle non-ASCII review tokens before calling compare_digest. secrets.compare_digest() rejects non-ASCII str, so a malformed X-Review-Token can turn this auth path into a 500 instead of the intended 401. Compare bytes, or validate/reject non-ASCII input first.
🤖 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 `@review.py` around lines 67 - 68, Update the token validation condition around
the X-Review-Token check to handle non-ASCII strings before calling
secrets.compare_digest, either by comparing encoded bytes or explicitly
rejecting non-ASCII input. Ensure malformed tokens consistently raise the
existing HTTPException with status 401 rather than producing an internal error.
| corrected_answer: str | None = Field(None, description="Required when the verdict is 'correct'") | ||
| reviewer: str | None = Field(None, description="Reviewer's name or identifier") | ||
| note: str | None = Field(None, description="Optional note for the record") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add bounds to verdict request fields.
corrected_answer, reviewer, and note accept unbounded strings before being persisted and sent through cache/export processing. A valid reviewer token can submit oversized values and amplify memory, storage, and downstream work. Add appropriate max_length constraints and reject invalid whitespace-only values.
As per path instructions, FastAPI request bodies must include Pydantic validation.
🤖 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 `@review.py` around lines 165 - 167, Add Pydantic validation to the verdict
request model fields corrected_answer, reviewer, and note: enforce appropriate
max_length limits and reject whitespace-only string values before persistence or
cache/export processing. Keep corrected_answer optional while validating any
provided value, and ensure the FastAPI request body uses this validated model.
Source: Path instructions
| async def run_async(self, prompt: str, generator: Callable[[str], str | Awaitable[str]]) -> SafetyResult: | ||
| """Run the pipeline while keeping classification off the event loop. | ||
|
|
||
| ``generator`` may be sync or async; the async path is awaited. | ||
| """ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the async generator contract safe for both accepted forms.
This signature permits any Awaitable[str] and synchronous generators, but _complete_async() only checks inspect.iscoroutine(res) and invokes synchronous generators directly on the event-loop thread. A Future or custom awaitable can be passed to enforce() unawaited, while blocking synchronous network work can stall all requests. Use inspect.isawaitable() and offload the synchronous branch with asyncio.to_thread, or narrow the contract to async-only.
As per path instructions, blocking calls in this FastAPI service’s async flow must be awaited or offloaded.
# Add asyncio to the imports, then offload synchronous generators.
res = await asyncio.to_thread(generator, generation_prompt)
generated = await res if inspect.isawaitable(res) else res#!/bin/bash
python - <<'PY'
import inspect
class CustomAwaitable:
def __await__(self):
async def done():
return "ok"
return done().__await__()
assert inspect.isawaitable(CustomAwaitable())
assert not inspect.iscoroutine(CustomAwaitable())
PY🤖 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 `@safety/pipeline.py` around lines 33 - 37, Update run_async and its
_complete_async flow to use inspect.isawaitable for all awaitable results,
including Futures and custom awaitables. Offload synchronous generator execution
with asyncio.to_thread before handling its result, while preserving direct
awaiting for async-capable results and the existing SafetyResult behavior.
2 participants
Closes #21
What changed
The quality gate was
flake8 ... --max-line-length=120 --ignore=E501,W503pluscompileall— a length rule that disabled itself, no import sorting, no formatter, and no type checking, while CONTRIBUTING.md promised type hints on parameters and return values. This replaces that with ruff + mypy, configured in onepyproject.toml.Tooling
pyproject.toml— ruff (E,F,I,UP,B,W; line length 120;venv/anddata/excluded) and mypy (pydantic plugin,disallow_untyped_defsscoped tomain.py,ignore_missing_importsscoped togoogle.*/stellar_sdk.*/redis.*/yaml.*rather than set globally).requirements-dev.txt— pinnedruff==0.14.5,mypy==1.19.0,pytest,pytest-asyncio, so CI is reproducible.compileallsteps are gone; the job now runsruff check .,ruff format --check ., andmypy .. The per-suite pytest steps are unchanged..pre-commit-config.yaml— optional hook wiring the same tools at the same versions.ruff check .,ruff format .,mypy .) and therequirements-dev.txtinstall step.Code
ruff formatapplied repo-wide and everyruff checkfinding resolved.main.py: parameter and return annotations on every function — the route handlers, the SSEevent_generator, the retriever seams,get_safety_settings,get_model, and the rest.mypypasses with untyped defs disallowed for the module.mypy .pass tree-wide:@overloads onredact_secret_keys(str in → str out), anAwaitable-aware generator type onSafetyPipeline.run_async(it already accepted sync or async callables), plus small local fixes intafsir.py,study.py,hadith.py,store.py,review_store.py,safety/input_gate.py, andscripts/build_surah_index.py.raise ... from excadded where B904 flagged an unchained re-raise inside anexceptblock.Behaviour
No runtime behaviour change — formatting, annotations, and exception chaining only. Endpoints respond identically.
Two things deliberately not fixed here:
functools.cacheon a method) inhadith.py— the grading sources are process-lifetime singletons so the cache is bounded, but changing the caching strategy is a behaviour change and belongs in its own PR. Left with an explanatorynoqa.zip()intafsir.pyis nowstrict=True; the two sequences are built from each other, so lengths always match.Testing
ruff check .— cleanruff format --check .— 57 files already formattedmypy .— success, 57 source filespytest -q tests test— 820 passed, 1 skipped, 1 xfailed. The 3 failures intests/test_adhkar.pyare pre-existing ondev(verified by stashing this branch's changes) and are not run by CI.Summary by CodeRabbit