feat: answer feedback capture and a quality-improvement loop (#43) - #79
Conversation
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 (Deen-Bridge#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 Deen-Bridge#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 Deen-Bridge#43
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds stable answer identifiers, feedback submission and admin APIs, SQLite/Redis persistence with rate limiting, and an export script for down-rated evaluation candidates. Tests, CI checks, environment examples, ignore rules, and README documentation cover the feedback loop. ChangesAnswer feedback quality loop
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatRoute
participant FeedbackRoute
participant FeedbackStore
participant ExportScript
Client->>ChatRoute: Request generated answer
ChatRoute->>Client: Return answer with message_id
Client->>FeedbackRoute: Submit rating and message_id
FeedbackRoute->>FeedbackStore: Upsert feedback record
ExportScript->>FeedbackStore: Read down-rated records
ExportScript->>Client: Write evaluation-candidate JSONL
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: 11
🧹 Nitpick comments (2)
feedback.py (1)
228-240: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider WAL + a busy timeout for the SQLite connections.
/feedbackwrites are dispatched throughrun_in_threadpool, so several worker threads can hit this database concurrently. With the default rollback journal and no explicittimeout, a concurrent writer getssqlite3.OperationalError: database is locked, which the endpoint turns into a 500 for the user. WAL mode plus a busy timeout makes writers queue instead of failing.♻️ Suggested pragmas
def _conn(self) -> sqlite3.Connection: if getattr(self._local, "conn", None) is None: - conn = sqlite3.connect(self._db_path, check_same_thread=False) + conn = sqlite3.connect(self._db_path, check_same_thread=False, timeout=5.0) conn.row_factory = sqlite3.Row + conn.execute("PRAGMA journal_mode=WAL") + conn.execute("PRAGMA busy_timeout=5000") self._local.conn = conn return self._local.conn🤖 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 228 - 240, Configure every SQLite connection created by _conn and _init_db with a busy timeout and WAL journal mode immediately after opening it, before concurrent database operations begin; ensure both the thread-local runtime connections and initialization connection receive the same settings.main.py (1)
638-644: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCache-hit responses carry a top-level
message_idbut untaggedhistory.The normal path calls
_tag_history_with_message_ids(Line 810) so every model turn inhistorycarries an id; this path returnscached.historyuntouched, so a frontend readinghistory[i].message_idseesNoneon cache hits only. That inconsistency is worth documenting here — importantly, the obvious fix is a trap:cached.historyis the shared list held by the semantic cache entry, so tagging it in place would write this user's message ids into the object replayed to the next requester. If the ids are needed inhistory, deep-copy the list first.A brief comment recording that reasoning will save the next contributor from "fixing" it unsafely.
🤖 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 638 - 644, Document at the cache-hit return path near _record_answer and the ChatResponse construction that cached.history is shared by the semantic cache entry and must not be tagged in place; if history message IDs are added here, deep-copy cached.history before applying _tag_history_with_message_ids so subsequent cache users are not mutated.
🤖 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 63-64: Protect import-time environment parsing with one shared
_env_int helper: in feedback.py, add the helper and use it for RATE_LIMIT_MAX
and RATE_LIMIT_WINDOW_SECONDS; in main.py at line 324, import and use that
helper for FEEDBACK_SNAPSHOT_MAX instead of direct int parsing. Ensure malformed
values fall back to their existing sane defaults without preventing application
startup.
- Around line 85-100: Update RateLimiter.is_allowed and add or use a _sweep
helper to remove IP entries whose deques become empty after evicting timestamps,
ensuring drained buckets are deleted from _buckets. Invoke _sweep(cutoff)
periodically while holding _lock so stale client-controlled keys are reclaimed
without changing rate-limit decisions; keep reset clearing all buckets.
- Around line 344-351: Update the daily trend query in the feedback statistics
flow to limit distinct days rather than grouped day/rating rows, ensuring up to
14 days are returned even when each day has both ratings. Preserve the existing
by_day aggregation and ordering, using a day-scoped subquery or equivalent
before applying the limit.
- Around line 385-400: Update FeedbackStore.upsert to read the existing record
for the same _record_key before writing, then remove that key from its previous
rating and category sorted-set indexes within the same Redis pipeline before
adding the new memberships. Preserve idempotent overwrite behavior and ensure
stale indexes are cleaned up when ratings or categories change; also remove
memberships for expired records when implementing the requested TTL-horizon
cleanup if supported by the existing store design.
In `@main.py`:
- Around line 1146-1151: Chain the HTTPException raises from the caught
exception in all three feedback handlers: main.py lines 1146-1151 for “Failed to
store feedback.”, 1166-1170 for “Failed to fetch stats.”, and 1192-1199 for
“Failed to fetch records.” Use the existing exc variable with each raise, and
preserve the current client-facing detail strings.
- Around line 304-312: Update generate to pass the existing GENERATION_CONFIG
constant to the model instead of maintaining an inline generation-config
dictionary. Remove the duplicate literal near the model invocation while
preserving the current generation settings and feedback-record behavior.
- Around line 169-177: Add declarative Pydantic length bounds to
FeedbackRequest.prompt, answer, chat_id, and message_id, using the existing
comment length-bound pattern and appropriate limits for each field. Also
constrain categories’ collection size to prevent excessive repeated entries,
while preserving the existing validation and taxonomy checks.
- Around line 1094-1098: Update _client_ip to trust X-Forwarded-For only when
the explicit TRUST_PROXY_HEADERS configuration is enabled; otherwise always use
request.client.host or "unknown". When enabled, parse the header from right to
left and select the rightmost untrusted hop, rather than the leftmost value,
while preserving the existing fallback behavior.
- Around line 354-362: Update require_admin so both the supplied token and
ADMIN_TOKEN are encoded to bytes before passing them to secrets.compare_digest.
Preserve the existing 403 response for missing or invalid tokens and the 503
response when ADMIN_TOKEN is unset.
In `@scripts/export_eval_candidates.py`:
- Around line 163-174: Update the argparse definitions for --limit and
--min-categories in the candidate export parser to reject negative values while
accepting zero and positive integers. Apply the same non-negative validation to
both arguments and preserve their existing defaults and behavior for valid
inputs.
- Around line 155-160: Wrap the long argparse declarations for the --output and
--db options in the argument-parser setup so every line stays within flake8’s
79-character limit, preserving the existing options, defaults, and help text.
---
Nitpick comments:
In `@feedback.py`:
- Around line 228-240: Configure every SQLite connection created by _conn and
_init_db with a busy timeout and WAL journal mode immediately after opening it,
before concurrent database operations begin; ensure both the thread-local
runtime connections and initialization connection receive the same settings.
In `@main.py`:
- Around line 638-644: Document at the cache-hit return path near _record_answer
and the ChatResponse construction that cached.history is shared by the semantic
cache entry and must not be tagged in place; if history message IDs are added
here, deep-copy cached.history before applying _tag_history_with_message_ids so
subsequent cache users are not mutated.
🪄 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: e1a85baf-2c1d-4bd4-9a4a-b84606b53811
📒 Files selected for processing (8)
.env.example.github/workflows/ci.yml.gitignoreREADME.mdfeedback.pymain.pyscripts/export_eval_candidates.pytests/test_feedback.py
| parser.add_argument("--output", metavar="PATH", help="Output JSONL file (default: stdout)") | ||
| parser.add_argument( | ||
| "--db", | ||
| metavar="PATH", | ||
| default=None, | ||
| help="Force this SQLite DB path, ignoring REDIS_URL (default: configured backend)", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
awk 'length($0) > 79 { printf "%s:%d:%d\n", FILENAME, FNR, length($0) }' \
scripts/export_eval_candidates.py
for config in .flake8 setup.cfg tox.ini pyproject.toml; do
if [ -f "$config" ]; then
echo "== $config =="
rg -n -i 'max-line-length|ignore|extend-ignore|E501' "$config" || true
fi
doneRepository: Deen-Bridge/dnb-ai
Length of output: 441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== potential flake8 config files ==\n'
git ls-files | rg '(^|/)(\.flake8|setup\.cfg|tox\.ini|pyproject\.toml|ruff\.toml|ruff\.json)$' || true
printf '\n== line-length / E501 settings ==\n'
for f in $(git ls-files | rg '(^|/)(\.flake8|setup\.cfg|tox\.ini|pyproject\.toml|ruff\.toml|ruff\.json)$' || true); do
echo "--- $f ---"
rg -n -i 'max-line-length|line-length|ignore|extend-ignore|E501|flake8' "$f" || true
done
printf '\n== surrounding lines in scripts/export_eval_candidates.py ==\n'
sed -n '145,170p' scripts/export_eval_candidates.py | cat -nRepository: Deen-Bridge/dnb-ai
Length of output: 1235
Wrap the long argparse declarations
These argparse.add_argument calls exceed flake8’s default 79-character limit, and there’s no repo-wide E501 override, so CI will fail until they’re wrapped.
🤖 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 155 - 160, Wrap the long
argparse declarations for the --output and --db options in the argument-parser
setup so every line stays within flake8’s 79-character limit, preserving the
existing options, defaults, and help text.
Source: Path instructions
Resolves the CodeRabbit findings on the feedback PR. feedback.py - env_int() guards the rate-limit knobs so a malformed value logs and falls back instead of raising at import and taking down boot (main.py imports this module). - RateLimiter sweeps buckets whose window has fully passed, so a flood of distinct client-controlled IPs can no longer grow _buckets without bound. - Redis upsert now reads the prior record and removes its stale rating/category/model index memberships in the same pipeline, so a re-rating (down->up) or a changed category set can't leave dangling members that double-count in stats() and mislead list_records(). - Daily stats limit distinct days, not grouped rows, so the "by day" window is a full 14 days rather than as few as 7. main.py - FeedbackRequest bounds chat_id, message_id, prompt, answer, and the categories list, so an anonymous caller on the snapshot-miss path can't fill the store with multi-megabyte bodies (the rate limiter caps count, not size). The redundant comment validator is dropped for a field cap. - GENERATION_CONFIG is now the single source consumed by generate(), so a feedback record can't attest to settings that were never used. - The admin token is compared as bytes; compare_digest raises on a non-ASCII str, which would have turned a crafted header into a 500. - X-Forwarded-For is honored only when TRUST_PROXY_HEADERS is set, and then the rightmost (proxy-observed) hop is used, so header rotation can't mint fresh rate-limit buckets on the unauthenticated write endpoint. - The three feedback handlers chain their 500s with `from exc`. export_eval_candidates.py - Rejects negative --limit / --min-categories (a negative limit reaches SQLite as "no limit"). Tests: +18 (56 total) covering env_int fallback, bucket reclamation, the Redis index-hygiene paths via an in-memory fake, oversized-field 422s, the non-ASCII-token 403, the proxy-trust gating, and the CLI guards.
|
All 11 findings addressed in 8b619bd. Thanks — several were real bugs, not just polish. feedback.py
main.py
export_eval_candidates.py
Net: +18 tests (56 total in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feedback.py (1)
111-131: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid scanning every IP bucket on every feedback submission.
Line 111 performs an O(active-IP) sweep while holding
_lock. Sincesubmit_feedbackcalls this synchronously before its firstawait, enough active clients can block the event loop. Sweep on an interval or every N admissions instead.Proposed change
class RateLimiter: def __init__(self, max_calls=RATE_LIMIT_MAX, window_seconds=RATE_LIMIT_WINDOW_SECONDS): ... + self._next_sweep = 0.0 def is_allowed(self, ip: str) -> bool: now = time.monotonic() cutoff = now - self._window with self._lock: - self._sweep(cutoff) + if now >= self._next_sweep: + self._sweep(cutoff) + self._next_sweep = now + self._window🤖 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 111 - 131, Update submit_feedback and _sweep so stale-bucket cleanup does not scan every IP on every submission while holding _lock. Trigger _sweep only periodically or after a bounded number of admissions, while preserving rate-limit decisions and eventual removal of stale entries.Source: Path instructions
🧹 Nitpick comments (3)
tests/test_feedback.py (3)
219-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNice, focused fake — one latent trap worth pre-empting in
zrevrange.Real Redis treats a negative
stopas an offset from the end (-1= last element), but this fake computes[start:stop + 1], sozrevrange(key, 0, -1)returns[]instead of every member. Nothing exercises that path today (onlyzcard/hgetall/statsare used here), butRedisFeedbackStore.list_recordsdoes callzrevrange, so the first test added for it would fail mysteriously against the fake rather than against the store.🛡️ Proposed negative-index handling
def zrevrange(self, key, start, stop): members = sorted(self.zsets.get(key, {}).items(), key=lambda kv: kv[1], reverse=True) - return [m for m, _ in members][start:stop + 1] + ordered = [m for m, _ in members] + end = None if stop == -1 else stop + 1 + return ordered[start:end]🤖 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 219 - 221, Update the fake Redis zrevrange method to normalize negative stop values using Python slice semantics before applying the inclusive Redis range behavior, so stop=-1 returns all members while preserving existing positive-range handling.
142-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider widening the timing margins so this can't flake on a loaded runner.
is_allowedsweeps on every call, so the== 50assertion at Line 147 only holds if all 50 loop iterations complete inside the 50 ms window. Normally that's microseconds, but a stalled CI runner (GC pause, noisy neighbour) would reclaim the early buckets and fail the assert for reasons unrelated to the code under test. A wider window plus a proportionally longer sleep keeps the same semantics with far more headroom.♻️ Proposed margin widening
- limiter = RateLimiter(max_calls=5, window_seconds=0.05) + limiter = RateLimiter(max_calls=5, window_seconds=0.5) for i in range(50): limiter.is_allowed(f"ip-{i}") assert len(limiter._buckets) == 50 import time - time.sleep(0.06) + time.sleep(0.6)Alternatively, monkeypatch
time.monotonicinfeedbackto advance a fake clock and drop the sleep entirely — deterministic and instant.🤖 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 142 - 153, Widen the timing margins in test_stale_buckets_are_reclaimed so the 50 one-shot calls reliably remain within the configured window on loaded runners. Increase window_seconds and use a proportionally longer sleep before the fresh call, preserving the assertions that all 50 buckets initially exist and only the fresh bucket remains after reclamation.
596-599: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGreat test — silence the
S106finding so lint stays green.Ruff flags Line 598 as
S106(hardcoded password in thetokenargument). It's a false positive semantically, but if theflake8/ruffSruleset runs in CI this will fail the build rather than just warn. An inline suppression with a short reason keeps the signal honest.🧹 Proposed suppression
with pytest.raises(HTTPException) as exc: - await main.require_admin(token="tøken-ünicode") + await main.require_admin(token="tøken-ünicode") # noqa: S106 - test literal, not a secretAs per path instructions, "CI enforces flake8, so style violations fail the build."
🤖 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 596 - 599, Suppress Ruff’s S106 finding on the intentional Unicode token argument in the main.require_admin test, using an inline noqa with a brief reason. Keep the existing exception assertion and test behavior unchanged.Sources: Path instructions, Linters/SAST tools
🤖 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`:
- Line 73: Wrap the overlong logger calls in feedback.py, including the warning
at line 73 and the corresponding calls around lines 129 and 449, so each
statement stays within Flake8’s 79-column limit while preserving the existing
log message, arguments, and behavior.
- Around line 437-450: Update the record update flow around self.get, the
existing pipeline, and subsequent index writes to make the read, old-index
cleanup, and new-record write atomic. Use Redis WATCH with retry on transaction
conflicts or an equivalent Lua script, ensuring concurrent re-ratings cannot use
stale previous data or leave the key in both old and new indexes.
---
Outside diff comments:
In `@feedback.py`:
- Around line 111-131: Update submit_feedback and _sweep so stale-bucket cleanup
does not scan every IP on every submission while holding _lock. Trigger _sweep
only periodically or after a bounded number of admissions, while preserving
rate-limit decisions and eventual removal of stale entries.
---
Nitpick comments:
In `@tests/test_feedback.py`:
- Around line 219-221: Update the fake Redis zrevrange method to normalize
negative stop values using Python slice semantics before applying the inclusive
Redis range behavior, so stop=-1 returns all members while preserving existing
positive-range handling.
- Around line 142-153: Widen the timing margins in
test_stale_buckets_are_reclaimed so the 50 one-shot calls reliably remain within
the configured window on loaded runners. Increase window_seconds and use a
proportionally longer sleep before the fresh call, preserving the assertions
that all 50 buckets initially exist and only the fresh bucket remains after
reclamation.
- Around line 596-599: Suppress Ruff’s S106 finding on the intentional Unicode
token argument in the main.require_admin test, using an inline noqa with a brief
reason. Keep the existing exception assertion and test behavior unchanged.
🪄 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: f21baaf2-0554-471a-afc6-3dd5f6988a01
📒 Files selected for processing (4)
feedback.pymain.pyscripts/export_eval_candidates.pytests/test_feedback.py
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/export_eval_candidates.py
- main.py
| logger.warning("%s=%r is not an integer; using %s", name, raw, default) | ||
| return default | ||
| if value < minimum: | ||
| logger.warning("%s=%s is below the minimum %s; using %s", name, value, minimum, default) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI -t f . | rg '/(\.flake8|setup\.cfg|tox\.ini|pyproject\.toml)$' \
| xargs -r rg -n 'max-line-length|extend-ignore|ignore'
awk 'length($0) > 79 { printf "%s:%d: %d columns\n", FILENAME, FNR, length($0) }' feedback.pyRepository: Deen-Bridge/dnb-ai
Length of output: 156
Wrap these logger calls to stay under Flake8’s 79-column limit. This repo doesn’t set a wider max-line-length, so the lines at 73, 129, and 449 still trigger E501.
🤖 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` at line 73, Wrap the overlong logger calls in feedback.py,
including the warning at line 73 and the corresponding calls around lines 129
and 449, so each statement stays within Flake8’s 79-column limit while
preserving the existing log message, arguments, and behavior.
Source: Path instructions
| previous = self.get(record.chat_id, record.message_id) | ||
|
|
||
| data = record.to_dict() | ||
| data["categories"] = json.dumps(data["categories"]) | ||
| data["generation_config"] = ( | ||
| json.dumps(data["generation_config"]) if data["generation_config"] else "" | ||
| ) | ||
| pipe = self._r.pipeline() | ||
| if previous is not None: | ||
| pipe.zrem(f"{self._PREFIX}:index:rating:{previous.rating}", key) | ||
| for cat in previous.categories: | ||
| pipe.zrem(f"{self._PREFIX}:index:cat:{cat}", key) | ||
| pipe.zrem(f"{self._PREFIX}:index:model:{previous.model_name or 'unknown'}", key) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate feedback.py and inspect relevant surrounding lines.
file=$(git ls-files | rg '(^|/)feedback\.py$' | head -n 1)
echo "FILE=$file"
if [ -z "${file:-}" ]; then
echo "feedback.py not found" >&2
exit 1
fi
wc -l "$file"
sed -n '380,520p' "$file"Repository: Deen-Bridge/dnb-ai
Length of output: 5813
🏁 Script executed:
echo "waiting"Repository: Deen-Bridge/dnb-ai
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(git ls-files | rg '(^|/)feedback\.py$' | head -n 1)
echo "FILE=$file"
echo "--- search for WATCH / MULTI / Lua / lock usage in feedback.py ---"
rg -n "WATCH|MULTI|pipeline\(|evalsha|eval\(|lock|threading|asyncio\.Lock" "$file" || true
echo "--- nearby methods ---"
python3 - <<'PY'
from pathlib import Path
import re
p = next(Path('.').rglob('feedback.py'))
text = p.read_text()
for name in ['get(', 'set(', 'save(', 'update(', 'upsert(']:
if name in text:
print(f"FOUND {name}")
PYRepository: Deen-Bridge/dnb-ai
Length of output: 539
Make the Redis read/cleanup/write atomic. previous = self.get(...) runs before the pipeline, so concurrent re-ratings can base their cleanup on the same stale record and leave the key in both old and new indexes. Use WATCH + retry or a Lua script so the old memberships are removed and the new ones are written as one unit.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 439-439: use jsonify instead of json.dumps for JSON output
Context: json.dumps(data["categories"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 441-441: use jsonify instead of json.dumps for JSON output
Context: json.dumps(data["generation_config"])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 437 - 450, Update the record update flow around
self.get, the existing pipeline, and subsequent index writes to make the read,
old-index cleanup, and new-record write atomic. Use Redis WATCH with retry on
transaction conflicts or an equivalent Lua script, ensuring concurrent
re-ratings cannot use stale previous data or leave the key in both old and new
indexes.
|
Both addressed in 145bc52. Redis read/cleanup/write atomicity — you're right that E501 — wrapped the three flagged lines. Worth noting this repo's CI runs 57 offline feedback tests; CI (Lint + Docker) green. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Closes #43. Supersedes #45 — see note below.
The service had no signal about which answers were failing or why. This closes that loop end-to-end.
What
message_idper answer onChatResponseand each model turn inhistory, so the frontend can rate a specific turn. Tracked in structures kept parallel toactive_chatsrather than restructuring it, since dev's streaming and memory-eviction paths depend on its current shape.POST /feedbackattaches an up/down rating and validated failure categories to one answer. The prompt and the displayed answer (after safety, hadith, and confidence shaping) are snapshotted server-side, so the stored record is what the user actually saw — the client is never trusted for it. On a free-tier restart the snapshot is gone; the client then suppliesprompt/answer, and a request missing both is a422. Idempotent per(chat_id, message_id); rate-limited per IP.feedback.pystores records in SQLite locally or Redis whenREDIS_URLis set — the same store direction the session-persistence and scholar-review work already use, not a parallel one.GET /feedback/statsandGET /feedback/records— expose the aggregate signal, gated onX-Admin-Token(constant-time comparison) and disabled entirely untilADMIN_TOKENis set.scripts/export_eval_candidates.pyturns down-rated records into evaluation-dataset candidates in the [Enhancement] Evaluation harness for Islamic answer quality #16 harness format: deduplicated, alwaysneeds_review: true, carrying ananswer_draftfor the reviewer but never a fabricated expected answer for religious content.Relationship to #45
#45 (another contributor's PR) implemented this feature but branched from a point far behind current
dev— it predates the safety pipeline, tafsir, confidence, zakat, memory, streaming, and telemetry layers, so itsmain.pyno longer merges and it targetedmainrather thandev. I was assigned to fix it. Rather than force a stale 300-linemain.pydiff through conflict resolution, I rebuilt the feature cleanly on currentdev, keeping the sound parts of the original design (the taxonomy, the SQLite/Redis store shape, the export contract) and integrating against the current chat handler.Every CodeRabbit finding on #45 is addressed:
upsert,stats,list_records) run viarun_in_threadpool, off the event loop.build_store()selects Redis-or-SQLite, and the export uses it;--dbstill forces a specific SQLite file. The README's Redis example now works instead of misleading.rate_limiternot reset between tests → the limiter has areset(), and the endpoint fixture calls it, so limiter state can't leak between tests and produce spurious 429s..gitignorenow also blocksvenv_linux/andfeedback.db.Tests —
tests/test_feedback.py, 38 offlineStore upsert / idempotency / rating & category filtering (with an exact-token check so
languagedoesn't matchwrong_language) / stats; rate-limiter allow / block / reset / window expiry; backend selection incl. Redis-unreachable fallback; export mapping / dedup /--dbvs configured backend; and the endpoints through httpx's ASGI transport with Gemini stubbed —message_idon responses and history, server-side snapshot resolution and the client-supplied fallback, all validation paths, admin auth (403 no/wrong token, 503 unconfigured), and rate limiting. No live model or Redis calls.CI lints and compiles the new modules and adds a
tests/test_feedback.pystep. The feedback suite passes; the full suite is green except threetest_chat_robustnesscases and the Redis-backed memory tests, which fail identically on unmodifieddev(pre-existing, environmental — no Redis server / mock-shape issues) and are untouched by this PR.Note
This branch touches
main.py,README.md,.env.example, andci.yml, which my open PRs #67 (docs) and #68 (zakat) also touch. Whichever merges first, I'll rebase the others.Summary by CodeRabbit
POST /feedbackwith up/down ratings, categories, and comments tied to stablemessage_idvalues returned from/chat..env.examplewith feedback loop configuration and rate-limiting/storage settings.pytestcoverage for feedback..gitignoreto exclude local feedback database artifacts.