From 793169529c6e318af47e493ae8dc9bc2fbe70db0 Mon Sep 17 00:00:00 2001 From: rayhpeng Date: Wed, 29 Jul 2026 18:28:54 +0800 Subject: [PATCH] refactor(feedback): align the slice with the hexagonal spec - command-ify the write use cases (RateRun / RetractRunRating; queries keep plain parameters, commands stay dumb data) - split the domain errors into exceptions.py, a peer of model.py (PEP 8 Error suffixes, AWS-style module name) - unify the aggregate->row mapping as _apply(row, feedback) so one explicit field list serves both the insert and the update path - drop the unused feedback.message_id column (migration 0011): feedback is bound to a run, nothing ever wrote or read the field - pin remove_for_run's equality semantics for user_id=None in the contract suite and fix the port docstring that contradicted both implementations --- .../adapters/feedback/feedback_repository.py | 45 ++++++------ backend/app/gateway/routers/feedback.py | 72 +++++++++++-------- .../deerflow/domain/feedback/__init__.py | 7 +- .../deerflow/domain/feedback/commands.py | 38 ++++++++++ .../deerflow/domain/feedback/exceptions.py | 27 +++++++ .../harness/deerflow/domain/feedback/model.py | 30 ++------ .../harness/deerflow/domain/feedback/ports.py | 8 ++- .../deerflow/domain/feedback/service.py | 38 +++++----- .../deerflow/persistence/feedback/model.py | 3 - .../versions/0011_feedback_drop_message_id.py | 33 +++++++++ backend/tests/test_feedback.py | 14 ++++ backend/tests/test_feedback_service.py | 49 ++++++++----- backend/tests/test_persistence_bootstrap.py | 2 +- .../test_persistence_bootstrap_regression.py | 4 +- 14 files changed, 244 insertions(+), 126 deletions(-) create mode 100644 backend/packages/harness/deerflow/domain/feedback/commands.py create mode 100644 backend/packages/harness/deerflow/domain/feedback/exceptions.py create mode 100644 backend/packages/harness/deerflow/persistence/migrations/versions/0011_feedback_drop_message_id.py diff --git a/backend/app/adapters/feedback/feedback_repository.py b/backend/app/adapters/feedback/feedback_repository.py index e6086a027..edb54ae16 100644 --- a/backend/app/adapters/feedback/feedback_repository.py +++ b/backend/app/adapters/feedback/feedback_repository.py @@ -20,7 +20,8 @@ from sqlalchemy import select from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker -from deerflow.domain.feedback.model import DuplicateFeedbackError, Feedback +from deerflow.domain.feedback.exceptions import DuplicateFeedbackError +from deerflow.domain.feedback.model import Feedback from deerflow.domain.feedback.ports import FeedbackRepository # Transitional: the ORM row stays in the harness until PR-N moves engine, @@ -53,31 +54,33 @@ class SqlFeedbackRepository(FeedbackRepository): thread_id=row.thread_id, rating=row.rating, user_id=row.user_id, - message_id=row.message_id, comment=row.comment, tags=tuple(row.tags or ()), created_at=_tz_aware(row.created_at), ) @staticmethod - def _to_row(feedback: Feedback) -> FeedbackRow: - """Aggregate -> ORM row. Explicit field list: new columns stay + def _apply(row: FeedbackRow, feedback: Feedback) -> None: + """Aggregate -> ORM row, in place. One explicit field list serves + BOTH write paths (insert and update), so a new field cannot be + stored on insert yet silently dropped on update. ``feedback_id`` + is deliberately absent: the surrogate key is fixed at insert and + an upsert keeps the existing identity (see the port contract). + Explicit field list rather than ``**asdict()``: new fields stay private until deliberately mapped here.""" - return FeedbackRow( - feedback_id=feedback.feedback_id, - run_id=feedback.run_id, - thread_id=feedback.thread_id, - user_id=feedback.user_id, - message_id=feedback.message_id, - rating=feedback.rating, - comment=feedback.comment, - tags=list(feedback.tags) or None, - created_at=feedback.created_at, - ) + row.thread_id = feedback.thread_id + row.run_id = feedback.run_id + row.user_id = feedback.user_id + row.rating = feedback.rating + row.comment = feedback.comment + row.tags = list(feedback.tags) or None + row.created_at = feedback.created_at async def save(self, feedback: Feedback) -> Feedback: # Migrated from legacy ``upsert``: look up by aggregate identity, - # then update in place or insert. + # then apply the aggregate onto the existing or a fresh row. The + # identity fields are rewritten with equal values on update -- + # idempotent, and the price of a single field list. async with self._session_factory() as session: stmt = select(FeedbackRow).where( FeedbackRow.thread_id == feedback.thread_id, @@ -85,14 +88,10 @@ class SqlFeedbackRepository(FeedbackRepository): FeedbackRow.user_id == feedback.user_id, ) row = (await session.execute(stmt)).scalar_one_or_none() - if row is not None: - row.rating = feedback.rating - row.comment = feedback.comment - row.tags = list(feedback.tags) or None - row.created_at = feedback.created_at - else: - row = self._to_row(feedback) + if row is None: + row = FeedbackRow(feedback_id=feedback.feedback_id) session.add(row) + self._apply(row, feedback) try: await session.commit() except IntegrityError as exc: diff --git a/backend/app/gateway/routers/feedback.py b/backend/app/gateway/routers/feedback.py index d3e7b2f45..38d796034 100644 --- a/backend/app/gateway/routers/feedback.py +++ b/backend/app/gateway/routers/feedback.py @@ -24,6 +24,8 @@ from deerflow.domain.feedback import ( Feedback, InvalidRatingError, InvalidTagError, + RateRun, + RetractRunRating, RunNotFoundError, ) from deerflow.utils.time import coerce_iso @@ -37,7 +39,14 @@ router = APIRouter(prefix="/api/threads", tags=["feedback"]) # --------------------------------------------------------------------------- -class FeedbackUpsertRequest(BaseModel): +class RateRunRequest(BaseModel): + """Request model of the RateRun use case (name = command name + Request). + + Carries only what the client may supply: identity (user_id) never + appears on a request model -- it is resolved server-side and injected + through ``to_command``. + """ + rating: int = Field(..., description="Feedback rating: +1 (positive) or -1 (negative)") comment: str | None = Field(default=None, description="Optional text feedback") tags: list[str] = Field( @@ -45,33 +54,47 @@ class FeedbackUpsertRequest(BaseModel): description="Optional thumbs-down reason slugs (e.g. 'incorrect', 'slow')", ) + def to_command(self, thread_id: str, run_id: str, user_id: str | None) -> RateRun: + """Body + path + resolved identity -> the use-case command. + + The wire shape owns the inbound translation into domain vocabulary + (wire types become domain types here, e.g. list -> tuple). + """ + return RateRun( + thread_id=thread_id, + run_id=run_id, + rating=self.rating, + user_id=user_id, + comment=self.comment, + tags=tuple(self.tags), + ) + class FeedbackResponse(BaseModel): feedback_id: str run_id: str thread_id: str user_id: str | None = None - message_id: str | None = None rating: int comment: str | None = None tags: list[str] = [] created_at: str = "" - -def _to_response(feedback: Feedback) -> FeedbackResponse: - """Domain object -> wire shape. ``created_at`` keeps the legacy - ``coerce_iso`` serialization so the API output is byte-identical.""" - return FeedbackResponse( - feedback_id=feedback.feedback_id, - run_id=feedback.run_id, - thread_id=feedback.thread_id, - user_id=feedback.user_id, - message_id=feedback.message_id, - rating=feedback.rating, - comment=feedback.comment, - tags=list(feedback.tags), - created_at=coerce_iso(feedback.created_at), - ) + @classmethod + def from_domain(cls, feedback: Feedback) -> FeedbackResponse: + """Domain object -> wire shape (allowlist). ``created_at`` keeps the + legacy ``coerce_iso`` serialization so the API output is + byte-identical.""" + return cls( + feedback_id=feedback.feedback_id, + run_id=feedback.run_id, + thread_id=feedback.thread_id, + user_id=feedback.user_id, + rating=feedback.rating, + comment=feedback.comment, + tags=list(feedback.tags), + created_at=coerce_iso(feedback.created_at), + ) # --------------------------------------------------------------------------- @@ -84,21 +107,14 @@ def _to_response(feedback: Feedback) -> FeedbackResponse: async def upsert_feedback( thread_id: str, run_id: str, - body: FeedbackUpsertRequest, + body: RateRunRequest, request: Request, ) -> FeedbackResponse: """Set the current user's rating for a run (idempotent upsert).""" user_id = await get_current_user(request) service = get_feedback_service(request) try: - feedback = await service.rate_run( - thread_id, - run_id, - rating=body.rating, - comment=body.comment, - user_id=user_id, - tags=body.tags, - ) + feedback = await service.rate_run(body.to_command(thread_id, run_id, user_id)) except InvalidRatingError: raise HTTPException(status_code=400, detail="rating must be +1 or -1") except InvalidTagError as exc: @@ -109,7 +125,7 @@ async def upsert_feedback( # Lost a concurrent-upsert race (legacy behavior was a 500); the # client can simply retry. raise HTTPException(status_code=409, detail="Concurrent feedback update, please retry") - return _to_response(feedback) + return FeedbackResponse.from_domain(feedback) @router.delete("/{thread_id}/runs/{run_id}/feedback") @@ -122,7 +138,7 @@ async def delete_run_feedback( """Retract the current user's rating for a run.""" user_id = await get_current_user(request) service = get_feedback_service(request) - retracted = await service.retract_run_rating(thread_id, run_id, user_id=user_id) + retracted = await service.retract_run_rating(RetractRunRating(thread_id=thread_id, run_id=run_id, user_id=user_id)) if not retracted: raise HTTPException(status_code=404, detail="No feedback found for this run") return {"success": True} diff --git a/backend/packages/harness/deerflow/domain/feedback/__init__.py b/backend/packages/harness/deerflow/domain/feedback/__init__.py index 172eeed4b..545b4365b 100644 --- a/backend/packages/harness/deerflow/domain/feedback/__init__.py +++ b/backend/packages/harness/deerflow/domain/feedback/__init__.py @@ -6,14 +6,15 @@ here; import ports (FeedbackRepository, RunLookup) from adapters and tests, not everyday call-site symbols. """ -from deerflow.domain.feedback.model import ( +from deerflow.domain.feedback.commands import RateRun, RetractRunRating +from deerflow.domain.feedback.exceptions import ( DuplicateFeedbackError, - Feedback, FeedbackError, InvalidRatingError, InvalidTagError, RunNotFoundError, ) +from deerflow.domain.feedback.model import Feedback from deerflow.domain.feedback.service import FeedbackService __all__ = [ @@ -23,5 +24,7 @@ __all__ = [ "FeedbackService", "InvalidRatingError", "InvalidTagError", + "RateRun", + "RetractRunRating", "RunNotFoundError", ] diff --git a/backend/packages/harness/deerflow/domain/feedback/commands.py b/backend/packages/harness/deerflow/domain/feedback/commands.py new file mode 100644 index 000000000..25089d883 --- /dev/null +++ b/backend/packages/harness/deerflow/domain/feedback/commands.py @@ -0,0 +1,38 @@ +"""Commands of the feedback context. + +One frozen dataclass per state-changing use case -- the named carrier of +"the information required to perform an operation on the domain" (AWS +hexagonal guidance). Commands are dumb data on purpose: business +validation stays on the aggregate (``Feedback.__post_init__``) and +structural validation stays on the primary adapter's api model, so error +attribution (invalid rating reported before an unknown run) is unchanged. + +Queries are deliberately NOT commands: the read methods on +``FeedbackService`` keep plain parameters -- a command expresses an +intent to change state, and wrapping reads would be pure boilerplate. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field + + +@dataclass(frozen=True) +class RateRun: + """Set the user's current rating for a run (idempotent upsert).""" + + thread_id: str + run_id: str + rating: int + user_id: str | None + comment: str | None = None + tags: tuple[str, ...] = field(default=()) + + +@dataclass(frozen=True) +class RetractRunRating: + """Withdraw the user's rating for a run (clicking the active button again).""" + + thread_id: str + run_id: str + user_id: str | None diff --git a/backend/packages/harness/deerflow/domain/feedback/exceptions.py b/backend/packages/harness/deerflow/domain/feedback/exceptions.py new file mode 100644 index 000000000..8bf4a53da --- /dev/null +++ b/backend/packages/harness/deerflow/domain/feedback/exceptions.py @@ -0,0 +1,27 @@ +"""The known errors of the feedback context. + +One family under one base class, so the primary adapter can map the whole +family onto protocol codes in a single table. Class names keep the PEP 8 +``Error`` suffix; the module is named ``exceptions`` after the AWS +hexagonal guidance's domain folder of the same name. +""" + + +class FeedbackError(Exception): + """Base error for the feedback domain.""" + + +class InvalidRatingError(FeedbackError): + """Raised when a rating is not +1 or -1.""" + + +class InvalidTagError(FeedbackError): + """Raised when a feedback tag is not a known reason slug.""" + + +class DuplicateFeedbackError(FeedbackError): + """Raised when a concurrent upsert conflicts on the same run's feedback.""" + + +class RunNotFoundError(FeedbackError): + """Raised when the run does not exist or does not belong to the thread.""" diff --git a/backend/packages/harness/deerflow/domain/feedback/model.py b/backend/packages/harness/deerflow/domain/feedback/model.py index 482d10320..0e76e4f80 100644 --- a/backend/packages/harness/deerflow/domain/feedback/model.py +++ b/backend/packages/harness/deerflow/domain/feedback/model.py @@ -4,6 +4,8 @@ import uuid from dataclasses import dataclass, field from datetime import UTC, datetime +from deerflow.domain.feedback import InvalidRatingError, InvalidTagError + VALID_RATINGS = (-1, 1) # Language-neutral reason slugs for thumbs-down feedback. The UI translates @@ -21,33 +23,12 @@ VALID_FEEDBACK_TAGS = frozenset( ) -class FeedbackError(Exception): - """Base error for the feedback domain.""" - - -class InvalidRatingError(FeedbackError): - """Raised when a rating is not +1 or -1.""" - - -class InvalidTagError(FeedbackError): - """Raised when a feedback tag is not a known reason slug.""" - - -class DuplicateFeedbackError(FeedbackError): - """Raised when a concurrent upsert conflicts on the same run's feedback.""" - - -class RunNotFoundError(FeedbackError): - """Raised when the run does not exist or does not belong to the thread.""" - - @dataclass(frozen=True) class Feedback: """A user's rating of a single run: at most one per (thread, run, user). - Single-entity aggregate. ``message_id`` optionally narrows the rating - to one message within the run instead of the whole run. ``tags`` carry - optional thumbs-down reason slugs from the feedback dialog. + Single-entity aggregate. ``tags`` carry optional thumbs-down reason + slugs from the feedback dialog. """ feedback_id: str @@ -55,7 +36,6 @@ class Feedback: thread_id: str rating: int user_id: str | None = None - message_id: str | None = None comment: str | None = None tags: tuple[str, ...] = () created_at: datetime = field(default_factory=lambda: datetime.now(UTC)) @@ -75,7 +55,6 @@ class Feedback: thread_id: str, rating: int, user_id: str | None = None, - message_id: str | None = None, comment: str | None = None, tags: tuple[str, ...] | list[str] = (), ) -> Feedback: @@ -86,7 +65,6 @@ class Feedback: thread_id=thread_id, rating=rating, user_id=user_id, - message_id=message_id, comment=comment, tags=tuple(tags), ) diff --git a/backend/packages/harness/deerflow/domain/feedback/ports.py b/backend/packages/harness/deerflow/domain/feedback/ports.py index 3a81126f6..8041ddc2f 100644 --- a/backend/packages/harness/deerflow/domain/feedback/ports.py +++ b/backend/packages/harness/deerflow/domain/feedback/ports.py @@ -44,8 +44,12 @@ class FeedbackRepository(Protocol): """Retract the user's feedback for a run. Returns True if an entry was removed, False if none existed. - Ownership filter: a non-None user_id restricts results to the - user's entries; None means no filtering (no-auth mode). + Unlike the read methods, ownership here is an EQUALITY match, not a + filter: a non-None user_id removes only that user's entry, and None + matches only entries stored with a NULL owner (what no-auth mode + writes) -- it does NOT mean "remove regardless of owner". A delete + must never reach across owners, so the asymmetry with the reads' + "None = no filtering" is deliberate. """ ... diff --git a/backend/packages/harness/deerflow/domain/feedback/service.py b/backend/packages/harness/deerflow/domain/feedback/service.py index cc8cd638d..7c8ffc349 100644 --- a/backend/packages/harness/deerflow/domain/feedback/service.py +++ b/backend/packages/harness/deerflow/domain/feedback/service.py @@ -1,4 +1,6 @@ -from deerflow.domain.feedback.model import Feedback, RunNotFoundError +from deerflow.domain.feedback.commands import RateRun, RetractRunRating +from deerflow.domain.feedback.exceptions import RunNotFoundError +from deerflow.domain.feedback.model import Feedback from deerflow.domain.feedback.ports import FeedbackRepository, RunLookup @@ -8,8 +10,9 @@ class FeedbackService: Orchestrates use cases only: fetch/verify -> apply domain rules -> persist through output ports. Holds no business rules itself (those live on the Feedback aggregate) and knows nothing about HTTP or - storage. user_id is always passed in explicitly -- resolving the - current user is the primary adapter's job. + storage. State-changing use cases take a command object (the handler + is the method); queries take plain parameters. Resolving the current + user is the primary adapter's job -- it arrives on the command. """ def __init__(self, repository: FeedbackRepository, runs: RunLookup): @@ -21,16 +24,7 @@ class FeedbackService: if await self._runs.thread_of(run_id) != thread_id: raise RunNotFoundError("Run does not belong to the specified thread") - async def rate_run( - self, - thread_id: str, - run_id: str, - *, - rating: int, - comment: str | None, - user_id: str | None, - tags: tuple[str, ...] | list[str] = (), - ) -> Feedback: + async def rate_run(self, cmd: RateRun) -> Feedback: """Set the user's current rating for a run (idempotent). Backs the PUT endpoint: "my current verdict on this run is X". @@ -48,20 +42,20 @@ class FeedbackService: to the given thread (cross-thread ids are rejected). """ feedback = Feedback.create( - run_id=run_id, - thread_id=thread_id, - rating=rating, - user_id=user_id, - comment=comment, - tags=tags, + run_id=cmd.run_id, + thread_id=cmd.thread_id, + rating=cmd.rating, + user_id=cmd.user_id, + comment=cmd.comment, + tags=cmd.tags, ) - await self._require_run(thread_id, run_id) + await self._require_run(cmd.thread_id, cmd.run_id) return await self._repository.save(feedback) - async def retract_run_rating(self, thread_id: str, run_id: str, *, user_id: str | None) -> bool: + async def retract_run_rating(self, cmd: RetractRunRating) -> bool: """Withdraw the user's rating for a run (clicking the active button again). Returns False when there was nothing to retract.""" - return await self._repository.remove_for_run(thread_id, run_id, user_id=user_id) + return await self._repository.remove_for_run(cmd.thread_id, cmd.run_id, user_id=cmd.user_id) async def latest_per_run_in_thread(self, thread_id: str, *, user_id: str | None) -> dict[str, Feedback]: """Current feedback per run across a thread -- powers the message-list diff --git a/backend/packages/harness/deerflow/persistence/feedback/model.py b/backend/packages/harness/deerflow/persistence/feedback/model.py index a7968fbf9..788790b5f 100644 --- a/backend/packages/harness/deerflow/persistence/feedback/model.py +++ b/backend/packages/harness/deerflow/persistence/feedback/model.py @@ -19,9 +19,6 @@ class FeedbackRow(Base): run_id: Mapped[str] = mapped_column(String(64), nullable=False, index=True) thread_id: Mapped[str] = mapped_column(String(64), nullable=False, index=True) user_id: Mapped[str | None] = mapped_column(String(64), index=True) - message_id: Mapped[str | None] = mapped_column(String(64)) - # message_id is an optional RunEventStore event identifier — - # allows feedback to target a specific message or the entire run rating: Mapped[int] = mapped_column(nullable=False) # +1 (thumbs-up) or -1 (thumbs-down) diff --git a/backend/packages/harness/deerflow/persistence/migrations/versions/0011_feedback_drop_message_id.py b/backend/packages/harness/deerflow/persistence/migrations/versions/0011_feedback_drop_message_id.py new file mode 100644 index 000000000..792a57f34 --- /dev/null +++ b/backend/packages/harness/deerflow/persistence/migrations/versions/0011_feedback_drop_message_id.py @@ -0,0 +1,33 @@ +"""Drop the unused feedback.message_id column. + +Feedback is bound to a run, not to a single message: nothing ever wrote or +read ``message_id`` (no API field, no frontend usage, no query), so the +column is redundant design and the domain aggregate no longer carries it. + +Revision ID: 0011_feedback_drop_message_id +Revises: 0010_feedback_tags +Create Date: 2026-07-29 +""" + +from __future__ import annotations + +from collections.abc import Sequence + +import sqlalchemy as sa + +from deerflow.persistence.migrations._helpers import safe_add_column, safe_drop_column + +revision: str = "0011_feedback_drop_message_id" +down_revision: str | Sequence[str] | None = "0010_feedback_tags" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + # Idempotent: a DB provisioned by create_all from the new metadata never + # had the column, and a retried upgrade must not fail on the second run. + safe_drop_column("feedback", "message_id") + + +def downgrade() -> None: + safe_add_column("feedback", sa.Column("message_id", sa.String(64), nullable=True)) diff --git a/backend/tests/test_feedback.py b/backend/tests/test_feedback.py index 139cdc118..a8f596015 100644 --- a/backend/tests/test_feedback.py +++ b/backend/tests/test_feedback.py @@ -88,6 +88,20 @@ class FeedbackRepositoryContract: assert await repo.remove_for_run("t1", "r1", user_id="u1") is True assert await repo.latest_per_run_in_thread("t1", user_id="u1") == {} + @pytest.mark.anyio + async def test_remove_for_run_none_user_matches_only_null_owner(self, repo): + # Deletion is an equality match, not a filter: user_id=None targets + # only the NULL-owner entry (no-auth writes) and must never reach + # across into a named user's entry. + await repo.save(Feedback.create(run_id="r1", thread_id="t1", rating=1, user_id="u1")) + await repo.save(Feedback.create(run_id="r1", thread_id="t1", rating=-1, user_id=None)) + + assert await repo.remove_for_run("t1", "r1", user_id=None) is True + assert await repo.remove_for_run("t1", "r1", user_id=None) is False + + remaining = await repo.latest_per_run_in_thread("t1", user_id="u1") + assert remaining["r1"].rating == 1 + @pytest.mark.anyio async def test_remove_for_run_nonexistent(self, repo): assert await repo.remove_for_run("t1", "r1", user_id="u1") is False diff --git a/backend/tests/test_feedback_service.py b/backend/tests/test_feedback_service.py index 05e5554b6..4ede8534f 100644 --- a/backend/tests/test_feedback_service.py +++ b/backend/tests/test_feedback_service.py @@ -1,13 +1,15 @@ """FeedbackService tests with injected fakes — zero IO, no engine. The seam the ports create is what makes these tests possible: the service -is exercised end-to-end against dict-backed doubles. +is exercised end-to-end against dict-backed doubles. State-changing use +cases go through command objects (RateRun / RetractRunRating); queries +keep plain parameters. """ import pytest from feedback_fakes import FakeRunLookup, InMemoryFeedbackRepository -from deerflow.domain.feedback import FeedbackService, InvalidRatingError, RunNotFoundError +from deerflow.domain.feedback import FeedbackService, InvalidRatingError, RateRun, RetractRunRating, RunNotFoundError def _service(runs: dict[str, str] | None = None) -> FeedbackService: @@ -17,34 +19,38 @@ def _service(runs: dict[str, str] | None = None) -> FeedbackService: ) +def _rate(thread_id: str = "t1", run_id: str = "r1", *, rating: int = 1, user_id: str | None = "u1", comment: str | None = None, tags: tuple[str, ...] = ()) -> RateRun: + return RateRun(thread_id=thread_id, run_id=run_id, rating=rating, user_id=user_id, comment=comment, tags=tags) + + class TestRateRun: @pytest.mark.anyio async def test_stores_rating(self): svc = _service() - fb = await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1") + fb = await svc.rate_run(_rate(rating=1)) assert fb.rating == 1 assert fb.user_id == "u1" @pytest.mark.anyio async def test_idempotent_upsert_keeps_identity(self): svc = _service() - first = await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1") - second = await svc.rate_run("t1", "r1", rating=-1, comment="meh", user_id="u1") + first = await svc.rate_run(_rate(rating=1)) + second = await svc.rate_run(_rate(rating=-1, comment="meh")) assert second.feedback_id == first.feedback_id assert second.rating == -1 @pytest.mark.anyio async def test_enrich_with_tags(self): svc = _service() - await svc.rate_run("t1", "r1", rating=-1, comment=None, user_id="u1") - fb = await svc.rate_run("t1", "r1", rating=-1, comment="wrong", user_id="u1", tags=["incorrect"]) + await svc.rate_run(_rate(rating=-1)) + fb = await svc.rate_run(_rate(rating=-1, comment="wrong", tags=("incorrect",))) assert fb.tags == ("incorrect",) @pytest.mark.anyio async def test_unknown_run_rejected(self): svc = _service({}) with pytest.raises(RunNotFoundError): - await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1") + await svc.rate_run(_rate()) @pytest.mark.anyio async def test_cross_thread_run_rejected(self): @@ -52,7 +58,7 @@ class TestRateRun: this thread's URL (ownership check).""" svc = _service({"r1": "other-thread"}) with pytest.raises(RunNotFoundError): - await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1") + await svc.rate_run(_rate()) @pytest.mark.anyio async def test_invalid_rating_rejected_before_run_lookup(self): @@ -61,29 +67,38 @@ class TestRateRun: # Expecting InvalidRatingError is what pins validation ahead of I/O. svc = _service() with pytest.raises(InvalidRatingError): - await svc.rate_run("t1", "unknown-run", rating=0, comment=None, user_id="u1") + await svc.rate_run(_rate(run_id="unknown-run", rating=0)) + + def test_command_is_dumb_data(self): + # The command carries intent without validating it: business rules + # stay on the aggregate, so error attribution (invalid rating before + # unknown run) is owned by the handler's construction order, not by + # the command's own constructor. + cmd = _rate(rating=0) + assert cmd.rating == 0 class TestRetractAndReads: @pytest.mark.anyio async def test_retract(self): svc = _service() - await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1") - assert await svc.retract_run_rating("t1", "r1", user_id="u1") is True - assert await svc.retract_run_rating("t1", "r1", user_id="u1") is False + await svc.rate_run(_rate(rating=1)) + retract = RetractRunRating(thread_id="t1", run_id="r1", user_id="u1") + assert await svc.retract_run_rating(retract) is True + assert await svc.retract_run_rating(retract) is False @pytest.mark.anyio async def test_latest_per_run_in_thread(self): svc = _service({"r1": "t1", "r2": "t1"}) - await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1") - await svc.rate_run("t1", "r2", rating=-1, comment=None, user_id="u1") + await svc.rate_run(_rate(run_id="r1", rating=1)) + await svc.rate_run(_rate(run_id="r2", rating=-1)) grouped = await svc.latest_per_run_in_thread("t1", user_id="u1") assert {run_id: fb.rating for run_id, fb in grouped.items()} == {"r1": 1, "r2": -1} @pytest.mark.anyio async def test_latest_for_runs(self): svc = _service({"r1": "t1", "r2": "t1"}) - await svc.rate_run("t1", "r1", rating=1, comment=None, user_id="u1") - await svc.rate_run("t1", "r2", rating=-1, comment=None, user_id="u1") + await svc.rate_run(_rate(run_id="r1", rating=1)) + await svc.rate_run(_rate(run_id="r2", rating=-1)) grouped = await svc.latest_for_runs("t1", {"r2"}, user_id="u1") assert set(grouped) == {"r2"} diff --git a/backend/tests/test_persistence_bootstrap.py b/backend/tests/test_persistence_bootstrap.py index 2bd27255a..616ef0054 100644 --- a/backend/tests/test_persistence_bootstrap.py +++ b/backend/tests/test_persistence_bootstrap.py @@ -48,7 +48,7 @@ from deerflow.persistence.migrations._helpers import _normalize_default asyncio_test = pytest.mark.asyncio -HEAD = "0010_feedback_tags" +HEAD = "0011_feedback_drop_message_id" BASELINE = "0001_baseline" diff --git a/backend/tests/test_persistence_bootstrap_regression.py b/backend/tests/test_persistence_bootstrap_regression.py index d07a2f612..9a52273ad 100644 --- a/backend/tests/test_persistence_bootstrap_regression.py +++ b/backend/tests/test_persistence_bootstrap_regression.py @@ -76,7 +76,7 @@ async def test_legacy_database_recovers_token_usage_column(tmp_path: Path) -> No cols = {row[1] for row in raw.execute("PRAGMA table_info(runs)").fetchall()} assert "token_usage_by_model" in cols version_row = raw.execute("SELECT version_num FROM alembic_version").fetchone() - assert version_row[0] == "0010_feedback_tags" + assert version_row[0] == "0011_feedback_drop_message_id" # And the read path that originally 500'd must now succeed. sf = get_session_factory() @@ -116,6 +116,6 @@ async def test_legacy_database_with_manual_alter_still_bootstraps(tmp_path: Path # No duplicate column -- list, not set, to catch dupes. assert cols.count("token_usage_by_model") == 1 version_row = raw.execute("SELECT version_num FROM alembic_version").fetchone() - assert version_row[0] == "0010_feedback_tags" + assert version_row[0] == "0011_feedback_drop_message_id" finally: await close_engine()