feat: implement feedback system with storage, API endpoints, evaluati… - #45
Conversation
…on export script, and pytest suite
WalkthroughAdds per-message feedback capture with SQLite/Redis persistence, throttling, admin reporting routes, evaluation-candidate export, expanded tests and CI checks, API documentation, dependencies, and virtual-environment artifacts. ChangesFeedback platform
Virtual environment artifacts
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
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: 7
🧹 Nitpick comments (4)
.github/workflows/ci.yml (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLint scope excludes tests/, letting real flake8 violations (E741) slip into test_feedback.py.
The
flake8invocation in ci.yml never lints thetests/directory, so the ambiguous-variable-name violations (for l in f if l.strip()) added intests/test_feedback.pypass CI silently today but will start failing the moment lint coverage is widened — better to fix both now.
.github/workflows/ci.yml#L30-33: addtests/to the flake8 file list (flake8 main.py feedback.py stellar.py scripts/export_eval_candidates.py tests/).tests/test_feedback.py#L590-659: rename the loop variableltolineat lines 598, 621, 632, and 643.🔧 Suggested fix for tests/test_feedback.py
- lines = [json.loads(l) for l in f if l.strip()] + lines = [json.loads(line) for line in f if line.strip()](apply the same rename at the other three occurrences)
🤖 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 at line 1, Update the flake8 invocation in the CI workflow to include the tests/ directory, then update the four loops in tests/test_feedback.py that use the ambiguous variable l to use line consistently, including their references in the comprehensions or loop bodies.Sources: Path instructions, Linters/SAST tools
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
--max-line-length=120is neutralized by--ignore=E501.E501 is the line-length rule that
--max-line-lengthconfigures the threshold for; ignoring E501 means the length flag has no effect. Either drop--max-line-length=120or stop ignoringE501(and reformat any offending lines).🤖 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 30 - 33, Update the flake8 invocation in the CI workflow to make the line-length configuration effective: either remove --max-line-length=120 or remove E501 from --ignore, then reformat any lines that violate the enforced 120-character limit.tests/test_feedback.py (1)
598-598: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAmbiguous variable name
l(flake8 E741).
for l in f if l.strip()trips E741. This file isn't currently in the CI lint scope (see.github/workflows/ci.yml), but it's still a real flake8 violation and should be fixed defensively — see the consolidated comment for the full picture.As per path instructions, this is a FastAPI service where "CI enforces flake8, so style violations fail the build."
Also applies to: 621-621, 632-632, 643-643
🤖 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_feedback.py` at line 598, Rename the ambiguous loop variable l in the JSON-reading comprehensions in tests/test_feedback.py to a descriptive name such as line, updating all affected occurrences around lines 598, 621, 632, and 643 while preserving the existing filtering and json.loads behavior.Sources: Path instructions, Linters/SAST tools
requirements.txt (1)
7-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin new dependency versions; separate dev/test tooling.
httpx,pytest,flake8, andredisuse>=while every other pinned dep in this file uses==, so a routinepip install -r requirements.txtin CI/prod can pull in an unvetted major/minor bump and silently change build behavior. Also,pytest/flake8are dev-only tools shipped into the production install closure.🔧 Suggested fix
-httpx>=0.27.0 -pytest>=8.0.0 -flake8>=7.0.0 -redis>=5.0.0 +httpx==0.27.0 +redis==5.0.0Move
pytest/flake8into arequirements-dev.txtand have CI install both files.As per path instructions, "new dependencies not pinned in requirements.txt" should be flagged.
🤖 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 `@requirements.txt` around lines 7 - 10, Pin httpx and redis in requirements.txt with exact == versions consistent with the existing dependency policy, and remove pytest and flake8 from the production requirements. Add pytest and flake8 to requirements-dev.txt, then update the CI dependency-installation configuration to install both requirements files.Source: Path instructions
🤖 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 `@feedback.py`:
- Around line 406-414: Update the record persistence logic around the pipeline
in the relevant method to ensure every rating, category, and model sorted-set
member is removed when its hash expires. Add expiration-aware pruning for the
zsets used by stats() and list_records(), including the model index, while
preserving the existing hash TTL and indexing behavior.
In `@main.py`:
- Around line 68-76: Update require_admin to compare the provided token and
ADMIN_TOKEN using a constant-time comparison primitive instead of direct
inequality. Preserve the existing 503 response for missing configuration and 403
response for invalid or absent tokens, including safe handling of differing
token lengths.
- Around line 249-252: Offload all synchronous I/O from the async handlers: in
main.py lines 249-252, wrap chat_session.send_message with run_in_threadpool or
asyncio.to_thread; in lines 406-410, 429-433, and 456-461, likewise offload
feedback_store.upsert, feedback_store.stats, and feedback_store.list_records
respectively. Preserve each call’s arguments and returned values while ensuring
slow Gemini, SQLite, or Redis operations do not block the event loop.
In `@README.md`:
- Around line 128-136: Remove the Redis invocation and related Redis-store
wording from the “Eval-candidate Export” section in README.md, since
scripts/export_eval_candidates.py only uses SQLiteFeedbackStore. Keep the
documented SQLite command and export behavior unchanged.
In `@scripts/export_eval_candidates.py`:
- Around line 118-119: Update export() to obtain the feedback store through the
shared backend-aware store builder instead of directly constructing
SQLiteFeedbackStore, so REDIS_URL and other configured backends are honored.
Preserve the existing list_records filtering and limit behavior, and only use a
SQLite-specific path if an explicit override or warning prevents silent backend
mismatch.
In `@tests/test_feedback.py`:
- Around line 128-146: Reset the shared rate_limiter singleton in the app_client
fixture alongside the SQLiteFeedbackStore setup, creating a fresh limiter for
each test before reloading main. Ensure submit_feedback uses this reset instance
so feedback and admin endpoint tests do not share request counts or produce
spurious 429 responses.
In `@venv_linux/pyvenv.cfg`:
- Around line 1-5: Remove the committed virtual environment artifacts:
venv_linux/pyvenv.cfg lines 1-5, venv_linux/bin/python line 1,
venv_linux/bin/python3 line 1, venv_linux/bin/python3.12 line 1, and
venv_linux/lib64 line 1. Add venv_linux/ to the repository ignore configuration
while retaining reproducible inputs such as requirements.txt.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 1: Update the flake8 invocation in the CI workflow to include the tests/
directory, then update the four loops in tests/test_feedback.py that use the
ambiguous variable l to use line consistently, including their references in the
comprehensions or loop bodies.
- Around line 30-33: Update the flake8 invocation in the CI workflow to make the
line-length configuration effective: either remove --max-line-length=120 or
remove E501 from --ignore, then reformat any lines that violate the enforced
120-character limit.
In `@requirements.txt`:
- Around line 7-10: Pin httpx and redis in requirements.txt with exact ==
versions consistent with the existing dependency policy, and remove pytest and
flake8 from the production requirements. Add pytest and flake8 to
requirements-dev.txt, then update the CI dependency-installation configuration
to install both requirements files.
In `@tests/test_feedback.py`:
- Line 598: Rename the ambiguous loop variable l in the JSON-reading
comprehensions in tests/test_feedback.py to a descriptive name such as line,
updating all affected occurrences around lines 598, 621, 632, and 643 while
preserving the existing filtering and json.loads 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: 83ecc318-05e0-4bf5-9f61-c55737c74d9f
📒 Files selected for processing (13)
.github/workflows/ci.ymlREADME.mdfeedback.pymain.pyrequirements.txtscripts/export_eval_candidates.pytests/__init__.pytests/test_feedback.pyvenv_linux/bin/pythonvenv_linux/bin/python3venv_linux/bin/python3.12venv_linux/lib64venv_linux/pyvenv.cfg
| response = chat_session.send_message( | ||
| full_prompt, | ||
| generation_config={ | ||
| "temperature": 0.7, | ||
| "top_p": 0.8, | ||
| "top_k": 40, | ||
| "max_output_tokens": 2048, | ||
| } | ||
| generation_config=GENERATION_CONFIG, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('main.py')
lines = p.read_text().splitlines()
for start in [220, 390, 420, 445]:
end = min(start + 40, len(lines))
print(f"\n--- main.py lines {start}-{end} ---")
for i in range(start-1, end):
print(f"{i+1:4}: {lines[i]}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 6819
🏁 Script executed:
rg -n "class .*feedback|def upsert\(|def stats\(|def list_records\(|feedback_store\s*=" -S main.py .Repository: Deen-Bridge/dnb-ai
Length of output: 1150
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('feedback.py')
lines = p.read_text().splitlines()
for start in [160, 236, 380, 430, 460]:
end = min(start + 45, len(lines))
print(f"\n--- feedback.py lines {start}-{end} ---")
for i in range(start-1, end):
print(f"{i+1:4}: {lines[i]}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 9965
Offload synchronous I/O from these async handlers. chat_session.send_message(...), feedback_store.upsert(...), feedback_store.stats(), and feedback_store.list_records(...) all run synchronously inside async def endpoints, so a slow Gemini/SQLite/Redis call blocks the event loop and stalls unrelated requests. Wrap them with run_in_threadpool/asyncio.to_thread or use async clients instead.
📍 Affects 1 file
main.py#L249-L252(this comment)main.py#L406-L410main.py#L429-L433main.py#L456-L461
🤖 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 249 - 252, Offload all synchronous I/O from the async
handlers: in main.py lines 249-252, wrap chat_session.send_message with
run_in_threadpool or asyncio.to_thread; in lines 406-410, 429-433, and 456-461,
likewise offload feedback_store.upsert, feedback_store.stats, and
feedback_store.list_records respectively. Preserve each call’s arguments and
returned values while ensuring slow Gemini, SQLite, or Redis operations do not
block the event loop.
Source: Path instructions
| ### Eval-candidate Export | ||
|
|
||
| Convert down-rated records into evaluation-dataset candidates in the issue #16 harness format: | ||
|
|
||
| ```bash | ||
| python scripts/export_eval_candidates.py --output candidates.jsonl | ||
| # Redis store: | ||
| REDIS_URL=redis://localhost:6379 python scripts/export_eval_candidates.py --output candidates.jsonl | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether export_eval_candidates.py selects a backend based on REDIS_URL
rg -n "REDIS_URL|SQLiteFeedbackStore|RedisFeedbackStore|get_store" scripts/export_eval_candidates.pyRepository: Deen-Bridge/dnb-ai
Length of output: 283
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' scripts/export_eval_candidates.py
printf '\n--- README excerpt ---\n'
sed -n '120,140p' README.mdRepository: Deen-Bridge/dnb-ai
Length of output: 7384
Remove or implement the Redis example
scripts/export_eval_candidates.py always uses SQLiteFeedbackStore and never reads REDIS_URL, so the README’s Redis invocation is misleading. Either add backend selection or remove that example.
🤖 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 `@README.md` around lines 128 - 136, Remove the Redis invocation and related
Redis-store wording from the “Eval-candidate Export” section in README.md, since
scripts/export_eval_candidates.py only uses SQLiteFeedbackStore. Keep the
documented SQLite command and export behavior unchanged.
| store = SQLiteFeedbackStore(db_path=db_path) | ||
| records = store.list_records(rating="down", limit=limit) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files 'scripts/export_eval_candidates.py' 'feedback.py' '**/feedback.py' '**/*feedback*' | sed -n '1,120p'
printf '\n== outline export script ==\n'
ast-grep outline scripts/export_eval_candidates.py --view expanded || true
printf '\n== outline feedback candidates ==\n'
for f in $(git ls-files | grep -E '(^|/)(feedback|store).*\.py$' || true); do
echo "--- $f ---"
ast-grep outline "$f" --view expanded || true
doneRepository: Deen-Bridge/dnb-ai
Length of output: 3007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== export_eval_candidates.py (relevant section) ==\n'
sed -n '1,220p' scripts/export_eval_candidates.py | cat -n
printf '\n== feedback.py (store selection section) ==\n'
sed -n '180,540p' feedback.py | cat -n
printf '\n== tests/test_feedback.py (store-selection tests) ==\n'
sed -n '1,260p' tests/test_feedback.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 33274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== references to export_eval_candidates or REDIS_URL ==\n'
rg -n --hidden --no-ignore-vcs \
'export_eval_candidates|REDIS_URL|FEEDBACK_DB_PATH|feedback.db|evaluation candidates|evaluation-dataset|user_feedback' \
. \
-g '!*.pyc' -g '!*.git' | sed -n '1,220p'Repository: Deen-Bridge/dnb-ai
Length of output: 2860
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== README export section ==\n'
sed -n '120,145p' README.md | cat -n
printf '\n== export script tests ==\n'
sed -n '533,690p' tests/test_feedback.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 8460
Export should honor the configured feedback backend.
The README examples run this command with REDIS_URL, but export() still hardcodes SQLiteFeedbackStore, so a Redis-backed deployment will export from feedback.db instead of the live feedback store. Reuse the shared store builder or add an explicit warning/override so the wrong backend isn’t silently used.
🤖 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 `@scripts/export_eval_candidates.py` around lines 118 - 119, Update export() to
obtain the feedback store through the shared backend-aware store builder instead
of directly constructing SQLiteFeedbackStore, so REDIS_URL and other configured
backends are honored. Preserve the existing list_records filtering and limit
behavior, and only use a SQLite-specific path if an explicit override or warning
prevents silent backend mismatch.
| @pytest.fixture() | ||
| def app_client(tmp_db, monkeypatch): | ||
| """Return a TestClient for the FastAPI app with a patched feedback store.""" | ||
| # Point feedback store at the temp DB before app loads | ||
| monkeypatch.setenv("FEEDBACK_DB_PATH", tmp_db) | ||
| monkeypatch.setenv("GEMINI_API_KEY", "test-key") | ||
| monkeypatch.setenv("ADMIN_TOKEN", "test-admin-token") | ||
|
|
||
| # Force re-import so the store picks up the new env var | ||
| import feedback as fb_module | ||
| from feedback import SQLiteFeedbackStore | ||
| fb_module.store = SQLiteFeedbackStore(db_path=tmp_db) | ||
|
|
||
| import main as main_module | ||
| importlib.reload(main_module) | ||
| main_module.feedback_store = fb_module.store | ||
|
|
||
| from fastapi.testclient import TestClient | ||
| return TestClient(main_module.app) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Shared rate_limiter singleton isn't reset between tests — flakiness risk.
TestFeedbackEndpoint and TestAdminEndpoints fire roughly a dozen /feedback calls from the same default TestClient host, but app_client only rebuilds the feedback store, not feedback.rate_limiter. Since submit_feedback checks the limiter before body validation, even the "invalid input" tests (bad rating/category/comment) consume a slot. Only test_rate_limiting_blocks_flood explicitly patches the limiter — every other test shares cumulative state, so the suite can start returning spurious 429s as more tests are added or run out of order.
🔧 Suggested fix
`@pytest.fixture`()
def app_client(tmp_db, monkeypatch):
"""Return a TestClient for the FastAPI app with a patched feedback store."""
monkeypatch.setenv("FEEDBACK_DB_PATH", tmp_db)
monkeypatch.setenv("GEMINI_API_KEY", "test-key")
monkeypatch.setenv("ADMIN_TOKEN", "test-admin-token")
import feedback as fb_module
from feedback import SQLiteFeedbackStore
fb_module.store = SQLiteFeedbackStore(db_path=tmp_db)
+ from feedback import RateLimiter
+ fb_module.rate_limiter = RateLimiter()
import main as main_module
importlib.reload(main_module)
main_module.feedback_store = fb_module.store
+ main_module.rate_limiter = fb_module.rate_limiter📝 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.
| @pytest.fixture() | |
| def app_client(tmp_db, monkeypatch): | |
| """Return a TestClient for the FastAPI app with a patched feedback store.""" | |
| # Point feedback store at the temp DB before app loads | |
| monkeypatch.setenv("FEEDBACK_DB_PATH", tmp_db) | |
| monkeypatch.setenv("GEMINI_API_KEY", "test-key") | |
| monkeypatch.setenv("ADMIN_TOKEN", "test-admin-token") | |
| # Force re-import so the store picks up the new env var | |
| import feedback as fb_module | |
| from feedback import SQLiteFeedbackStore | |
| fb_module.store = SQLiteFeedbackStore(db_path=tmp_db) | |
| import main as main_module | |
| importlib.reload(main_module) | |
| main_module.feedback_store = fb_module.store | |
| from fastapi.testclient import TestClient | |
| return TestClient(main_module.app) | |
| `@pytest.fixture`() | |
| def app_client(tmp_db, monkeypatch): | |
| """Return a TestClient for the FastAPI app with a patched feedback store.""" | |
| # Point feedback store at the temp DB before app loads | |
| monkeypatch.setenv("FEEDBACK_DB_PATH", tmp_db) | |
| monkeypatch.setenv("GEMINI_API_KEY", "test-key") | |
| monkeypatch.setenv("ADMIN_TOKEN", "test-admin-token") | |
| # Force re-import so the store picks up the new env var | |
| import feedback as fb_module | |
| from feedback import SQLiteFeedbackStore | |
| fb_module.store = SQLiteFeedbackStore(db_path=tmp_db) | |
| from feedback import RateLimiter | |
| fb_module.rate_limiter = RateLimiter() | |
| import main as main_module | |
| importlib.reload(main_module) | |
| main_module.feedback_store = fb_module.store | |
| main_module.rate_limiter = fb_module.rate_limiter | |
| from fastapi.testclient import TestClient | |
| return TestClient(main_module.app) |
🤖 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_feedback.py` around lines 128 - 146, Reset the shared rate_limiter
singleton in the app_client fixture alongside the SQLiteFeedbackStore setup,
creating a fresh limiter for each test before reloading main. Ensure
submit_feedback uses this reset instance so feedback and admin endpoint tests do
not share request counts or produce spurious 429 responses.
| home = /usr/bin | ||
| include-system-site-packages = false | ||
| version = 3.12.3 | ||
| executable = /usr/bin/python3.12 | ||
| command = /usr/bin/python3 -m venv /home/femi-john/Documents/Grant/dnb-ai/venv_linux |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the committed virtual environment.
These generated files are machine-specific, non-relocatable, and include a developer filesystem path. Keep only reproducible project inputs such as requirements.txt and recreate the environment in local setup or CI.
venv_linux/pyvenv.cfg#L1-L5: remove the absolute environment metadata and ignorevenv_linux/.venv_linux/bin/python#L1-L1: remove the generated launcher symlink.venv_linux/bin/python3#L1-L1: remove the hardcoded/usr/bin/python3launcher.venv_linux/bin/python3.12#L1-L1: remove the generated Python 3.12 launcher.venv_linux/lib64#L1-L1: remove the platform-specific library symlink.
📍 Affects 5 files
venv_linux/pyvenv.cfg#L1-L5(this comment)venv_linux/bin/python#L1-L1venv_linux/bin/python3#L1-L1venv_linux/bin/python3.12#L1-L1venv_linux/lib64#L1-L1
🤖 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 `@venv_linux/pyvenv.cfg` around lines 1 - 5, Remove the committed virtual
environment artifacts: venv_linux/pyvenv.cfg lines 1-5, venv_linux/bin/python
line 1, venv_linux/bin/python3 line 1, venv_linux/bin/python3.12 line 1, and
venv_linux/lib64 line 1. Add venv_linux/ to the repository ignore configuration
while retaining reproducible inputs such as requirements.txt.
|
@Johnsource-hub fix CI and am ready to merge once its green |
|
@zeemscript this PR Is linked to another contributor. |
|
@Fury03 let's go |
|
@zeemscript @Johnsource-hub I was assigned to get this green. The blocker turned out to be structural: this branch was cut from a point well behind Rather than force a stale 300-line #79 also resolves all seven CodeRabbit findings from this PR: blocking store I/O is offloaded with Original credit to @Johnsource-hub for the design and first implementation. Suggest closing this in favour of #79 once a maintainer has looked it over. |
Maintainers had no signal about which answers were failing or why. This closes that loop end-to-end. - Every chat answer now carries a stable message_id so the frontend can address one turn, tracked in structures parallel to active_chats rather than restructuring it (the streaming and memory paths depend on its current shape). - POST /feedback attaches an up/down rating and validated failure categories to a specific answer. The prompt and the *displayed* answer (after safety/hadith/confidence shaping) are snapshotted server-side, so the stored record is what the user actually saw; on a free-tier restart the snapshot is gone and the client supplies prompt/answer, else 422. Idempotent per (chat_id, message_id); rate-limited per IP. - feedback.py stores records in SQLite locally or Redis when REDIS_URL is set — the same store direction the session and scholar-review work use. - Two admin endpoints (GET /feedback/stats, /feedback/records) expose the aggregate signal, gated on X-Admin-Token with constant-time comparison and disabled until ADMIN_TOKEN is set. - scripts/export_eval_candidates.py turns down-rated records into evaluation-dataset candidates (#16 format), deduplicated, always needs_review:true, never fabricating an expected answer for religious content. It reads whichever store the service is configured to use. This is a rebuild of #45 onto current dev (which had moved far past that branch) with the review feedback applied: blocking SQLite/Redis I/O in the async endpoints is offloaded via run_in_threadpool; the export honours the configured backend instead of hardcoding SQLite; the shared rate limiter has a reset() and tests clear it so limiter state never leaks between them; and no virtualenv is committed. Tests (tests/test_feedback.py, 38 offline): store upsert/idempotency/ filtering/stats, rate-limiter allow/block/reset/expiry, backend selection, export mapping/dedup/backend, and the endpoints — message_id on responses and history, snapshot resolution and client fallback, validation, admin auth (401/403/503), and rate limiting. No live model or Redis calls. Closes #43
3 participants
#closes
#43
Summary
The maintainers had zero signal about which answers were failing and why. This PR closes that loop end-to-end:
Every model answer now carries a stable message_id (UUID) so the frontend can address a specific turn.
A new POST /feedback endpoint lets users attach a up/down rating and one or more validated failure categories to any answer.
Records are durably stored in SQLite (local / free-tier Render) or Redis (production via REDIS_URL), surviving service restarts.
Two admin-only endpoints (GET /feedback/stats, GET /feedback/records) give maintainers aggregate quality metrics and the ability to filter flagged records.
A scripts/export_eval_candidates.py script converts down-rated records into evaluation-dataset candidates in the issue-#16 harness format — deduplicated, always needs_review: true, never fabricating expected answers.
A 30+ test offline pytest suite covers all paths: store upsert/idempotency, rate limiting, endpoint validation, admin auth, stats correctness, and export formatting.
CI extended to lint all new modules and run the full pytest suite
Summary by CodeRabbit
New Features
Documentation
Tests