feedback: thumb up/down on logs + reviewer self-score 0-10
Two unrelated features bundled because they ship together and share
migration 0028.
Strategic-log feedback (thumb up/down):
- New strategic_log_feedback table with UNIQUE(log_id, user_id) so each
user has one vote per log, flippable in place (up -> down -> clear).
UI shows aggregate counts only.
- app/services/log_feedback.py: set_vote, get_counts, sign/verify
feedback tokens (same itsdangerous pattern as auth.sign_pending,
30-day TTL for email links).
- POST /api/log/{id}/feedback: web vote, auth required, returns counts
+ the requesting user's own vote.
- GET /feedback?token=...&vote=...: email-link target, no auth, signed
token encodes (user, log, vote), renders feedback_thanks.html.
- partials/log.html: thumbs row below content, JS-driven swap via the
POST endpoint. Dashboard latest-log card and /log page both render
this partial via htmx, so the buttons appear in all three surfaces.
- digest emails: a "How was today's read?" row above the unsub footer,
with signed-token URLs against the latest StrategicLog at send time.
Plain-text fallback included.
Reviewer self-score (0-10):
- _SYSTEM_PROMPT asks for an integer score with anchors (10 exemplary,
5 borderline, 0 unfit). Verdict gains score: int | None.
- Deterministic-layer hits get score=0 (hard rule, no nuance);
error rows get None; LLM rows get the model's score clamped 0..10.
- ReviewerVerdict.score, StrategicLog.reviewer_score, and
IndicatorSummary.reviewer_score all new SMALLINT NULL columns.
- ai_log_job + indicator_summary_job persist verdict.score onto their
content rows when committing the row alongside content.
Tests:
- tests/test_strategic_log_feedback.py: vote, flip, clear, aggregate
across users, invalid vote, token round-trip + tamper + garbage +
'clear' not signable for email path.
- tests/test_output_review.py: score parsing, clamping (>10, <0),
missing/non-numeric -> None, deterministic-layer score=0.
Full suite: 427 passed (was 412), 5 skipped, no regressions.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
parent
f3ac65f8f7
commit
8946dee2e0
14 changed files with 962 additions and 14 deletions
|
|
@ -325,3 +325,109 @@ async def test_review_portfolio_rider_active_when_flag_enabled(monkeypatch):
|
|||
surface="portfolio",
|
||||
)
|
||||
assert "# Surface: portfolio commentary" in seen_systems[0]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Reviewer self-score (0-10)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_review_parses_score_from_llm_json(monkeypatch):
|
||||
_configure(monkeypatch)
|
||||
def handler(_req):
|
||||
return httpx.Response(200, json={
|
||||
"choices": [{"message": {"content":
|
||||
'{"clean": true, "reason": "exemplary", "score": 9}'},
|
||||
"finish_reason": "stop"}],
|
||||
"usage": {"prompt_tokens": 50, "completion_tokens": 12, "cost": 0.00007},
|
||||
})
|
||||
async with httpx.AsyncClient(transport=_mock_post(handler)) as client:
|
||||
v = await review_read(client, "Markets pricing tighter policy.")
|
||||
assert v.clean is True
|
||||
assert v.score == 9
|
||||
assert v.layer == "llm"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_review_score_clamped_to_0_10(monkeypatch):
|
||||
"""A model returning 17 or -3 is buggy but must not blow up — clamp."""
|
||||
_configure(monkeypatch)
|
||||
def handler(_req):
|
||||
return httpx.Response(200, json={
|
||||
"choices": [{"message": {"content":
|
||||
'{"clean": false, "reason": "x", "score": 17}'},
|
||||
"finish_reason": "stop"}],
|
||||
"usage": {"prompt_tokens": 50, "completion_tokens": 8, "cost": 0.00003},
|
||||
})
|
||||
async with httpx.AsyncClient(transport=_mock_post(handler)) as client:
|
||||
v = await review_read(client, "Some content.")
|
||||
assert v.score == 10
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_review_score_negative_clamped_to_0(monkeypatch):
|
||||
_configure(monkeypatch)
|
||||
def handler(_req):
|
||||
return httpx.Response(200, json={
|
||||
"choices": [{"message": {"content":
|
||||
'{"clean": false, "reason": "x", "score": -3}'},
|
||||
"finish_reason": "stop"}],
|
||||
"usage": {"prompt_tokens": 50, "completion_tokens": 8, "cost": 0.00003},
|
||||
})
|
||||
async with httpx.AsyncClient(transport=_mock_post(handler)) as client:
|
||||
v = await review_read(client, "Some content.")
|
||||
assert v.score == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_review_missing_score_yields_none(monkeypatch):
|
||||
"""Older mocked responses don't carry score; verdict still valid,
|
||||
score is None."""
|
||||
_configure(monkeypatch)
|
||||
def handler(_req):
|
||||
return httpx.Response(200, json={
|
||||
"choices": [{"message": {"content":
|
||||
'{"clean": true, "reason": "ok"}'},
|
||||
"finish_reason": "stop"}],
|
||||
"usage": {"prompt_tokens": 50, "completion_tokens": 6, "cost": 0.00002},
|
||||
})
|
||||
async with httpx.AsyncClient(transport=_mock_post(handler)) as client:
|
||||
v = await review_read(client, "Plain state-level prose.")
|
||||
assert v.clean is True
|
||||
assert v.score is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_review_score_non_numeric_yields_none(monkeypatch):
|
||||
"""Defensive: a string or null in the score field doesn't poison the
|
||||
verdict; score becomes None."""
|
||||
_configure(monkeypatch)
|
||||
def handler(_req):
|
||||
return httpx.Response(200, json={
|
||||
"choices": [{"message": {"content":
|
||||
'{"clean": true, "reason": "ok", "score": "high"}'},
|
||||
"finish_reason": "stop"}],
|
||||
"usage": {"prompt_tokens": 50, "completion_tokens": 6, "cost": 0.00002},
|
||||
})
|
||||
async with httpx.AsyncClient(transport=_mock_post(handler)) as client:
|
||||
v = await review_read(client, "Plain state-level prose.")
|
||||
assert v.clean is True
|
||||
assert v.score is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_review_deterministic_layer_score_is_zero(monkeypatch):
|
||||
"""A deterministic-layer hit is a hard reject by rule; the audit row
|
||||
carries score=0 (no nuance to score)."""
|
||||
_configure(monkeypatch)
|
||||
calls = []
|
||||
def handler(_req):
|
||||
calls.append(1)
|
||||
return httpx.Response(500, json={"error": "should not fire"})
|
||||
async with httpx.AsyncClient(transport=_mock_post(handler)) as client:
|
||||
v = await review_read(client, "You should buy the dip.")
|
||||
assert v.clean is False
|
||||
assert v.layer == "deterministic"
|
||||
assert v.score == 0
|
||||
assert calls == []
|
||||
|
|
|
|||
186
tests/test_strategic_log_feedback.py
Normal file
186
tests/test_strategic_log_feedback.py
Normal file
|
|
@ -0,0 +1,186 @@
|
|||
"""Strategic-log feedback service + token helpers.
|
||||
|
||||
Covers the pure service path (set_vote, get_counts, clear) plus the
|
||||
token sign/verify round-trip. The web endpoint POST /api/log/{id}/feedback
|
||||
is covered in tests/test_api_feedback.py (separate file because it
|
||||
needs the full FastAPI + auth stack)."""
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
|
||||
|
||||
def _build_db(tmp_path):
|
||||
from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine
|
||||
|
||||
from app import db as db_mod
|
||||
from app.db import Base
|
||||
import app.models # noqa: F401
|
||||
|
||||
engine = create_async_engine(f"sqlite+aiosqlite:///{tmp_path}/fb.db")
|
||||
factory = async_sessionmaker(engine, expire_on_commit=False)
|
||||
db_mod._engine = engine
|
||||
db_mod._session_factory = factory
|
||||
|
||||
async def _seed():
|
||||
from app.models import StrategicLog, User
|
||||
from app.db import utcnow
|
||||
|
||||
async with engine.begin() as conn:
|
||||
await conn.run_sync(Base.metadata.create_all)
|
||||
async with factory() as s:
|
||||
s.add(User(id=1, email="alice@example.com", tier="free",
|
||||
settings_json={}, created_at=utcnow()))
|
||||
s.add(User(id=2, email="bob@example.com", tier="free",
|
||||
settings_json={}, created_at=utcnow()))
|
||||
s.add(StrategicLog(
|
||||
id=10, generated_at=utcnow(),
|
||||
model="m", anchor_date=None, prompt_version=1,
|
||||
tone="INTERMEDIATE", analysis="DRY",
|
||||
content="x", prompt_tokens=1, completion_tokens=1,
|
||||
cost_usd=0.0,
|
||||
))
|
||||
await s.commit()
|
||||
|
||||
asyncio.run(_seed())
|
||||
return factory
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Service: set_vote / get_counts
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_set_vote_up_writes_one_row(tmp_path):
|
||||
factory = _build_db(tmp_path)
|
||||
|
||||
async def _go():
|
||||
from app.services.log_feedback import set_vote
|
||||
async with factory() as s:
|
||||
counts = await set_vote(s, log_id=10, user_id=1, vote="up")
|
||||
return counts
|
||||
|
||||
counts = asyncio.run(_go())
|
||||
assert counts.up == 1
|
||||
assert counts.down == 0
|
||||
assert counts.user_vote == "up"
|
||||
|
||||
|
||||
def test_set_vote_flip_replaces_existing(tmp_path):
|
||||
"""Voting again with a different value flips the existing row;
|
||||
no duplicate is written, the up/down counts swap."""
|
||||
factory = _build_db(tmp_path)
|
||||
|
||||
async def _go():
|
||||
from app.services.log_feedback import set_vote
|
||||
async with factory() as s:
|
||||
await set_vote(s, log_id=10, user_id=1, vote="up")
|
||||
return await set_vote(s, log_id=10, user_id=1, vote="down")
|
||||
|
||||
counts = asyncio.run(_go())
|
||||
assert counts.up == 0
|
||||
assert counts.down == 1
|
||||
assert counts.user_vote == "down"
|
||||
|
||||
|
||||
def test_set_vote_clear_removes_row(tmp_path):
|
||||
factory = _build_db(tmp_path)
|
||||
|
||||
async def _go():
|
||||
from app.services.log_feedback import set_vote, get_counts
|
||||
async with factory() as s:
|
||||
await set_vote(s, log_id=10, user_id=1, vote="up")
|
||||
await set_vote(s, log_id=10, user_id=1, vote="clear")
|
||||
return await get_counts(s, log_id=10, user_id=1)
|
||||
|
||||
counts = asyncio.run(_go())
|
||||
assert counts.up == 0
|
||||
assert counts.down == 0
|
||||
assert counts.user_vote is None
|
||||
|
||||
|
||||
def test_aggregate_counts_across_users(tmp_path):
|
||||
"""Two distinct users vote — counts aggregate; each user's own_vote
|
||||
field reflects only their own row."""
|
||||
factory = _build_db(tmp_path)
|
||||
|
||||
async def _go():
|
||||
from app.services.log_feedback import set_vote, get_counts
|
||||
async with factory() as s:
|
||||
await set_vote(s, log_id=10, user_id=1, vote="up")
|
||||
await set_vote(s, log_id=10, user_id=2, vote="down")
|
||||
alice_view = await get_counts(s, log_id=10, user_id=1)
|
||||
bob_view = await get_counts(s, log_id=10, user_id=2)
|
||||
return alice_view, bob_view
|
||||
|
||||
alice, bob = asyncio.run(_go())
|
||||
assert alice.up == 1 and alice.down == 1 and alice.user_vote == "up"
|
||||
assert bob.up == 1 and bob.down == 1 and bob.user_vote == "down"
|
||||
|
||||
|
||||
def test_invalid_vote_rejected(tmp_path):
|
||||
factory = _build_db(tmp_path)
|
||||
|
||||
async def _go():
|
||||
from app.services.log_feedback import FeedbackError, set_vote
|
||||
async with factory() as s:
|
||||
try:
|
||||
await set_vote(s, log_id=10, user_id=1, vote="meh")
|
||||
except FeedbackError as e:
|
||||
return str(e)
|
||||
return "no error"
|
||||
|
||||
msg = asyncio.run(_go())
|
||||
assert "up" in msg.lower() and "down" in msg.lower()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Token sign / verify
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_feedback_token_round_trips(monkeypatch):
|
||||
"""A signed token decodes back to the same (user, log, vote) tuple."""
|
||||
monkeypatch.setenv("CASSANDRA_SESSION_SECRET", "test-secret-32-chars-long-okay")
|
||||
from app.config import get_settings
|
||||
get_settings.cache_clear()
|
||||
from app.services.log_feedback import sign_feedback_token, verify_feedback_token
|
||||
|
||||
tok = sign_feedback_token(user_id=42, log_id=99, vote="up")
|
||||
payload = verify_feedback_token(tok)
|
||||
assert payload == {"user_id": 42, "log_id": 99, "vote": "up"}
|
||||
|
||||
|
||||
def test_feedback_token_tampered_returns_none(monkeypatch):
|
||||
monkeypatch.setenv("CASSANDRA_SESSION_SECRET", "test-secret-32-chars-long-okay")
|
||||
from app.config import get_settings
|
||||
get_settings.cache_clear()
|
||||
from app.services.log_feedback import sign_feedback_token, verify_feedback_token
|
||||
|
||||
tok = sign_feedback_token(user_id=42, log_id=99, vote="up")
|
||||
tampered = tok[:-1] + ("a" if tok[-1] != "a" else "b")
|
||||
assert verify_feedback_token(tampered) is None
|
||||
|
||||
|
||||
def test_feedback_token_garbage_returns_none(monkeypatch):
|
||||
monkeypatch.setenv("CASSANDRA_SESSION_SECRET", "test-secret-32-chars-long-okay")
|
||||
from app.config import get_settings
|
||||
get_settings.cache_clear()
|
||||
from app.services.log_feedback import verify_feedback_token
|
||||
|
||||
assert verify_feedback_token("not.a.real.token") is None
|
||||
assert verify_feedback_token("") is None
|
||||
|
||||
|
||||
def test_feedback_token_clear_is_not_a_valid_email_link_vote(monkeypatch):
|
||||
"""The 'clear' sentinel is a web-only path; the email link can only
|
||||
apply a positive vote (up or down). Trying to sign 'clear' raises."""
|
||||
monkeypatch.setenv("CASSANDRA_SESSION_SECRET", "test-secret-32-chars-long-okay")
|
||||
from app.config import get_settings
|
||||
get_settings.cache_clear()
|
||||
from app.services.log_feedback import FeedbackError, sign_feedback_token
|
||||
|
||||
try:
|
||||
sign_feedback_token(user_id=1, log_id=10, vote="clear")
|
||||
except FeedbackError:
|
||||
return
|
||||
raise AssertionError("expected FeedbackError")
|
||||
Loading…
Add table
Add a link
Reference in a new issue