diff --git a/CHANGELOG.md b/CHANGELOG.md index 0126325..bf65962 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ ### Added +- **`hydradb feedback` — report whether a query's results were actually useful.** `POST /feedback` had no CLI surface. It correlates on one key, the `request_id` from the query's `meta`, and nothing else about the original query is re-sent, so nothing has to be trusted from the client. + + That key was unreachable: the wrapper's `_unwrap` returns `.data` and drops `meta`, so a successful query discarded its own request id before any caller saw it. `query` now carries it into the payload — additive, since `/query`'s data has no `request_id` of its own, so the documented `--output json` shape gains a key and loses none — and prints it with a copy-pasteable `hydradb feedback` line. It prints on an EMPTY result too: a query that found nothing is the case most worth reporting, and the one with no chunk ids to fall back on. + + Ground truth (`--ground-truth-answer`, repeatable `--ground-truth-source-id`) is accepted alongside prose because it is machine-checkable. Source ids are trimmed and de-duplicated before sending, since they are scored and the same document listed twice would weight one piece of evidence as two. `--source` defaults to `user` rather than `agent`: a person at a terminal is a user, and the two populations are separated at write time. + + Hand-rolled rather than routed through `sdk.feedback.submit` (CONTRACT §2 rule 7). The SDK *has* the resource, but `submit` takes an undiscriminated union of `{feedback}` and `{ground_truth}`, and `ground_truth` is itself a union of `{answer}` and `{source_ids}`. Both unions dropped every field their branches share, `request_id` included. It happens to work today only because the models are `extra="allow"`, so the endpoint's required field travels as an accident of pydantic config at two levels of nesting — and sending an answer together with source ids means picking one branch and smuggling the other past it. When the spec is fixed, this is a one-file change back to the SDK. + - **`hydradb graph` — full Cypher over graph collections you own (BYOG).** HydraDB's graph database offering had no CLI surface at all: `query`, `ingest` and the rest address the memory and knowledge corpora, and the property graphs users model and own end to end were reachable only through the raw API. This adds `graph query`, `graph collections`, `graph load`, `graph database create/delete` and `graph collection delete`. Everything existing is untouched — the two stores are separate, and nothing crosses between them. `graph query` takes parameters through `--param k=v` (values parse as JSON when they can, so `--param n=3` is the number 3) or `--params-json`. `--output json` prints the rows verbatim, so `hydradb graph query ... | jq '.[].name'` works without unwrapping an envelope. diff --git a/README.md b/README.md index a64f555..5d27b9f 100644 --- a/README.md +++ b/README.md @@ -233,6 +233,44 @@ hydradb query "What does the user prefer?" --kind memory hydradb query "pricing AND enterprise" --operator and ``` +Every query prints a `request_id`. That is the only key `feedback` correlates +on, so keep it if you intend to rate the answer: + +```bash +hydradb --output json query "contract terms" | jq -r .request_id +``` + +--- + +### feedback + +Report whether a query's results were actually useful. It correlates on the +`request_id` that query printed — nothing else about the original query is +re-sent. + +| Flag | Meaning | +|---|---| +| `--feedback` / `-f` | What was right or wrong about the results | +| `--rating` | Overall verdict: `positive`, `negative`, `neutral` | +| `--ground-truth-answer` | The answer the query *should* have produced | +| `--ground-truth-source-id` | A source id that should have been retrieved (repeatable) | +| `--source` | Who is reporting: `user` (default), or `agent` for scripted runs | + +Ground truth is worth far more than prose: it is machine-checkable, so it can +be scored automatically rather than read by a person. + +```bash +hydradb feedback 8f1c0e8a-... --rating positive +hydradb feedback 8f1c0e8a-... -f "returned the 2023 policy, not the current one" --rating negative +hydradb feedback 8f1c0e8a-... \ + --ground-truth-answer "Net 30, per the 2026 MSA" \ + --ground-truth-source-id src_abc --ground-truth-source-id src_def +``` + +A submission needs at least one of `--feedback`, `--ground-truth-answer` or +`--ground-truth-source-id`; an empty one is refused locally rather than after a +round trip. + --- ### ingest diff --git a/src/hydradb_cli/commands/_impl.py b/src/hydradb_cli/commands/_impl.py index c97fda9..dba426d 100644 --- a/src/hydradb_cli/commands/_impl.py +++ b/src/hydradb_cli/commands/_impl.py @@ -35,6 +35,11 @@ VALID_MODES = {"fast", "thinking"} VALID_OPERATORS = {"or", "and", "phrase"} VALID_KINDS = {"knowledge", "memory"} +VALID_RATINGS = {"positive", "negative", "neutral"} +# Who is reporting. Validated locally for the same reason --rating is: the +# server rejects anything else, but only after a round trip, and a typo like +# "agnet" is worth catching before it costs one. +VALID_SOURCES = {"user", "agent"} VALID_FETCH_MODES = {"content", "url", "both"} _STATUS_LABELS = { @@ -69,10 +74,23 @@ def _execute(spinner_msg: str, call: Callable[[], Any]) -> Any: # ── query ──────────────────────────────────────────────────────────────────── +def _feedback_hint(r: dict) -> str: + """The line that makes ``hydradb feedback`` reachable. + + Printed verbatim so it can be copied, and printed on an EMPTY result too: + a query that found nothing is exactly the case most worth reporting, and + it is the one where there are no chunk ids to fall back on. + """ + request_id = r.get("request_id") + if not request_id: + return "" + return f'\n[dim]request_id: {request_id} · rate it: hydradb feedback {request_id} --feedback "..."[/dim]' + + def _format_query_result(r: dict): chunks = r.get("chunks") or [] if not chunks: - return "[dim]No relevant results found.[/dim]" + return "[dim]No relevant results found.[/dim]" + _feedback_hint(r) panels: list[Any] = [] for i, chunk in enumerate(chunks, 1): @@ -99,6 +117,10 @@ def _format_query_result(r: dict): if query_paths: panels.append(Text(f" Graph: {len(query_paths)} entity path(s) found.", style="dim")) + hint = _feedback_hint(r) + if hint: + panels.append(Text.from_markup(hint.lstrip("\n"))) + header = Text(f" Found {len(chunks)} result(s)", style="bold") return Group(header, *panels) @@ -159,6 +181,77 @@ def do_query( print_result(result, _format_query_result) +# ── feedback ───────────────────────────────────────────────────────────────── + + +def _format_feedback_result(r: dict): + # `recorded: false` means accepted but not durably stored. Reporting that + # as success would tell someone their evaluation run was captured when it + # was not. + if r.get("recorded") is False: + return Panel( + "[yellow]![/yellow] Feedback accepted but NOT durably stored.\n" + "[dim]The server answered without recording it; treat this run as uncaptured.[/dim]", + border_style="yellow", + padding=(0, 1), + ) + + lines = [f"[green]✓[/green] Feedback recorded for request {r.get('request_id', '(unknown)')}"] + if r.get("feedback_id"): + lines.append(f"[cyan]Feedback ID:[/cyan] {r['feedback_id']}") + if r.get("created_at"): + lines.append(f"[dim]{r['created_at']}[/dim]") + return Panel("\n".join(lines), border_style="green", padding=(0, 1)) + + +def do_feedback( + request_id: str, + *, + feedback: str | None = None, + rating: str | None = None, + ground_truth_answer: str | None = None, + ground_truth_source_ids: list[str] | None = None, + source: str | None = None, + tenant_id: str | None = None, + sub_tenant_id: str | None = None, +) -> None: + if rating and rating not in VALID_RATINGS: + print_error(f"--rating must be one of: {', '.join(sorted(VALID_RATINGS))}. Got '{rating}'.") + if source and source not in VALID_SOURCES: + print_error(f"--source must be one of: {', '.join(sorted(VALID_SOURCES))}. Got '{source}'.") + if not request_id.strip(): + print_error("REQUEST_ID cannot be empty. Run 'hydradb query' and use the request id it prints.") + # The wrapper guards this too, for anyone importing it as a library. But it + # reports a refusal as HydraDBClientError(0, ...), and status 0 is this + # codebase's marker for a TRANSPORT failure (errors.py uses it only for + # connect/timeout), which `handle_api_error` renders as "Connection error:". + # Caught here instead, the way --rating and --kind are, so a local refusal + # reads as one rather than blaming the network. + if not any((value or "").strip() for value in [feedback, ground_truth_answer, *(ground_truth_source_ids or [])]): + print_error( + "feedback needs something to record: pass --feedback, --ground-truth-answer, or --ground-truth-source-id." + ) + + tid = require_tenant_id(tenant_id) + stid = resolve_sub_tenant_id(sub_tenant_id) + wrapper = get_wrapper() + + result = _execute( + "Recording feedback...", + lambda: wrapper.feedback.submit( + request_id=request_id, + feedback=feedback, + rating=rating, + ground_truth_answer=ground_truth_answer, + ground_truth_source_ids=ground_truth_source_ids, + source=source, + database=tid, + collection=stid, + ), + ) + print_result(result, _format_feedback_result) + + # ── ingest ─────────────────────────────────────────────────────────────────── diff --git a/src/hydradb_cli/commands/canonical.py b/src/hydradb_cli/commands/canonical.py index 28b6c38..343d526 100644 --- a/src/hydradb_cli/commands/canonical.py +++ b/src/hydradb_cli/commands/canonical.py @@ -85,6 +85,42 @@ def query( ) +def feedback( + request_id: str = typer.Argument(metavar="REQUEST_ID", help="The request id printed by 'hydradb query'."), + feedback_text: str | None = typer.Option( + None, "--feedback", "-f", help="What was right or wrong about the results." + ), + rating: str | None = typer.Option(None, "--rating", help="Overall verdict: 'positive', 'negative', or 'neutral'."), + ground_truth_answer: str | None = typer.Option( + None, "--ground-truth-answer", help="The answer the query SHOULD have produced." + ), + ground_truth_source_id: list[str] | None = typer.Option( + None, + "--ground-truth-source-id", + help="A source id that should have been retrieved, repeatable. Scored as a retrieval judgement.", + ), + source: str | None = typer.Option( + None, "--source", help="Who is reporting: 'user' (default) or 'agent' for scripted runs." + ), + database: str | None = typer.Option(None, "--database", "-d", help="Database. Uses default if not specified."), + collection: str | None = typer.Option(None, "--collection", help="Collection."), + tenant_id: str | None = typer.Option(None, "--tenant-id", hidden=True), + sub_tenant_id: str | None = typer.Option(None, "--sub-tenant-id", hidden=True), +) -> None: + """Report whether a query's results were actually useful.""" + tid, stid = resolve_scope_flags(database, collection, tenant_id, sub_tenant_id) + _impl.do_feedback( + request_id, + feedback=feedback_text, + rating=rating, + ground_truth_answer=ground_truth_answer, + ground_truth_source_ids=list(ground_truth_source_id) if ground_truth_source_id else None, + source=source, + tenant_id=tid, + sub_tenant_id=stid, + ) + + def ingest( files: list[str] | None = typer.Argument(None, help="Knowledge file path(s) to ingest."), kind: str | None = typer.Option(None, "--kind", help="Kind to ingest: 'memory' (default) or 'knowledge'."), diff --git a/src/hydradb_cli/hydra/client.py b/src/hydradb_cli/hydra/client.py index 0ed9643..285d801 100644 --- a/src/hydradb_cli/hydra/client.py +++ b/src/hydradb_cli/hydra/client.py @@ -84,6 +84,26 @@ def _unwrap_payload(body: Any) -> Any: return body if body is not None else {} +def _request_id_of(obj: Any) -> str | None: + """The ``meta.request_id`` of an SDK envelope, if it carries one. + + ``_unwrap`` returns ``.data`` and drops ``meta``, which is where the request + id lives. That id is the ONLY key ``POST /feedback`` correlates on, so a + query that does not surface it leaves ``hydradb feedback`` with nothing to + attach to. Read defensively: the SDK hands back a pydantic model, but the + hand-rolled path parses plain JSON. + """ + meta = getattr(obj, "meta", None) + if meta is None and isinstance(obj, dict): + meta = obj.get("meta") + if meta is None: + return None + rid = getattr(meta, "request_id", None) + if rid is None and isinstance(meta, dict): + rid = meta.get("request_id") + return rid if isinstance(rid, str) and rid else None + + def _bool_str(value: bool | None) -> str | None: """The SDK's multipart ``upsert`` field is a string; map bools to it.""" if value is None: @@ -240,7 +260,17 @@ def query( database=self._w._require_database(database), collection=self._w._resolve_collection(collection), ) - return _unwrap(resp) + data = _unwrap(resp) + # Carry the request id into the payload rather than dropping it with + # the rest of `meta`. It is additive -- /query's data has no + # `request_id` of its own -- so the documented `--output json` shape + # gains a key and loses none, and `hydradb feedback` becomes reachable + # by piping it. Without this the feedback command is unusable: nothing + # else in the response identifies the query it would be about. + request_id = _request_id_of(resp) + if request_id and isinstance(data, dict) and "request_id" not in data: + data["request_id"] = request_id + return data def ingest( self, @@ -609,6 +639,118 @@ def drop_database(self, *, database: str | None = None) -> dict: return result if isinstance(result, dict) else {} +class _Feedback(_Resource): + """``POST /feedback`` — was this query's answer any good? + + Hand-rolled rather than routed through ``sdk.feedback.submit`` (CONTRACT §2 + rule 7). The SDK *has* the resource, but its generated model cannot express + a valid request: ``submit`` takes a union of ``{feedback}`` and + ``{ground_truth}``, and ``ground_truth`` is itself a union of ``{answer}`` + and ``{source_ids}``. Both unions are undiscriminated and dropped every + field their branches share -- ``request_id`` included -- so the correlation + key the endpoint exists for is not on either model. + + It does happen to work today: the models are ``extra="allow"``, so + undeclared fields ride along and reach the wire (verified against staging). + But that means every field this endpoint needs travels as an accident of + pydantic config, at two levels of nesting, and sending an answer together + with source ids means picking one branch and smuggling the other past it. + A generated model that cannot name its own required field is equivalent to + no model. When the spec is fixed so the branches keep their siblings, this + becomes a one-file change back to the SDK. + """ + + #: The server de-duplicates before applying its own 100-id cap, so the cap + #: is checked HERE, after our own de-duplication -- 150 ids collapsing to + #: 80 is a valid request and must not be refused locally. + MAX_GROUND_TRUTH_SOURCE_IDS = 100 + + def submit( + self, + *, + request_id: str, + feedback: str | None = None, + rating: str | None = None, + ground_truth_answer: str | None = None, + ground_truth_source_ids: list[str] | None = None, + source: str | None = None, + metadata: dict | None = None, + database: str | None = None, + collection: str | None = None, + ) -> dict: + """Record feedback about a query that already ran. + + ``request_id`` comes from that query's ``meta.request_id``, which + ``context.query`` surfaces on its result. Nothing else about the + original query is re-sent, so nothing has to be trusted from here. + """ + rid = (request_id or "").strip() + if not rid: + raise HydraDBClientError( + 0, + "feedback needs the request_id of the query it is about. " + "Run 'hydradb query' and use the request id it prints.", + ) + + text = (feedback or "").strip() + + # Trimmed and de-duplicated because they are SCORED: the same document + # listed twice would weight one piece of evidence as two. + ids: list[str] = [] + seen: set[str] = set() + for value in ground_truth_source_ids or []: + candidate = (value or "").strip() + if candidate and candidate not in seen: + seen.add(candidate) + ids.append(candidate) + + answer = (ground_truth_answer or "").strip() + + if not text and not answer and not ids: + raise HydraDBClientError( + 0, + "feedback needs something to record: pass --feedback, " + "--ground-truth-answer, or --ground-truth-source-id. The server " + "refuses an empty submission too, but only after a round trip.", + ) + + if len(ids) > self.MAX_GROUND_TRUTH_SOURCE_IDS: + raise HydraDBClientError( + 0, + f"at most {self.MAX_GROUND_TRUTH_SOURCE_IDS} distinct ground-truth " + f"source ids, got {len(ids)}. A question answered by that many " + "documents is not specific enough to grade retrieval against.", + ) + + ground_truth: dict = {} + if answer: + ground_truth["answer"] = answer + if ids: + ground_truth["source_ids"] = ids + + body: dict = { + "request_id": rid, + # The server defaults this to "user"; say it explicitly so the row + # records who it came from rather than inheriting a default that + # could change. + "source": source or "user", + "database": self._w._require_database(database), + } + scope = self._w._resolve_collection(collection) + if scope: + body["collection"] = scope + if text: + body["feedback"] = text + if rating: + body["rating"] = rating + if ground_truth: + body["ground_truth"] = ground_truth + if metadata: + body["metadata"] = metadata + + return self._w._raw_post("/feedback", json_body=body) + + class _Connectors(_Resource): """Managed integrations that sync external sources into a database. @@ -809,6 +951,7 @@ def __init__( self.databases = _Databases(self) self.context = _Context(self) self.graph = _Graph(self) + self.feedback = _Feedback(self) self.connectors = _Connectors(self) def _raw_get(self, path: str, *, params: dict | None = None) -> Any: @@ -850,6 +993,37 @@ def _raw_get(self, path: str, *, params: dict | None = None) -> Any: return data if data is not None else {} return body if body is not None else {} + def _raw_post(self, path: str, *, json_body: Any) -> Any: + """POST an endpoint the SDK cannot express, with the same error contract. + + The sibling of :meth:`_raw_get` (CONTRACT §2 rule 7). Same auth and + ``API-Version: 2`` headers, same shape-based unwrapping, same + translated error type, so a caller cannot tell it from an SDK call. + """ + headers = { + "Authorization": f"Bearer {self._token}", + "API-Version": "2", + } + try: + response = httpx.post( + f"{self._base_url.rstrip('/')}{path}", + headers=headers, + json=json_body, + timeout=self._timeout, + ) + except httpx.HTTPError as exc: + raise translate_sdk_error(exc) from exc + + try: + body = response.json() if response.content else None + except ValueError: + body = response.text or None + + if response.is_error: + raise HydraDBClientError(response.status_code, _stringify_body(body)) + + return _unwrap_payload(body) + def _require_database(self, database: str | None) -> str: db = database or self.default_database if not db or not str(db).strip(): diff --git a/src/hydradb_cli/main.py b/src/hydradb_cli/main.py index 3d9a47e..777a9f4 100644 --- a/src/hydradb_cli/main.py +++ b/src/hydradb_cli/main.py @@ -94,6 +94,7 @@ def main( app.command(name="subgraph", help="Everything connected to one item: thread, replies, hierarchy, links.")( canonical.subgraph ) +app.command(name="feedback", help="Report whether a query's results were useful.")(canonical.feedback) app.command(name="verify", help="Check per-source ingestion status.")(canonical.verify) app.command(name="doctor", help="Check config and API reachability.")(canonical.doctor) app.add_typer(canonical.database_app, name="database", help="[bold]Database[/bold] management.") diff --git a/tests/test_cli_commands.py b/tests/test_cli_commands.py index 9553f10..178999b 100644 --- a/tests/test_cli_commands.py +++ b/tests/test_cli_commands.py @@ -1082,3 +1082,108 @@ def test_delete_collection_reports_async_cleanup(): out = _ANSI_RE.sub("", result.output).lower() assert "scheduled for deletion" in out assert "background" in out + + +class TestFeedbackCommand: + """The public ``hydradb feedback`` surface. + + The wrapper tests call ``feedback.submit`` directly, which leaves the + command itself unproven: registration, option-to-wrapper mapping, local + validation, both renderings and the exit code can all regress while those + stay green. + """ + + def test_registered_with_its_flags(self): + text = _help_text("feedback") + for flag in ("--feedback", "--rating", "--ground-truth-answer", "--ground-truth-source-id", "--source"): + assert flag in text, flag + + def test_maps_every_option_onto_the_wrapper(self): + _auth() + w = _wrapper(**{"feedback.submit": {"recorded": True, "feedback_id": "fb_1", "request_id": "r1"}}) + with _patch_wrapper(w): + result = runner.invoke( + app, + [ + "feedback", + "r1", + "--feedback", + "not the current policy", + "--rating", + "negative", + "--ground-truth-answer", + "Net 30", + "--ground-truth-source-id", + "src_a", + "--ground-truth-source-id", + "src_b", + "--source", + "agent", + ], + ) + assert result.exit_code == 0 + kwargs = w.feedback.submit.call_args.kwargs + assert kwargs["request_id"] == "r1" + assert kwargs["feedback"] == "not the current policy" + assert kwargs["rating"] == "negative" + assert kwargs["ground_truth_answer"] == "Net 30" + assert kwargs["ground_truth_source_ids"] == ["src_a", "src_b"] + assert kwargs["source"] == "agent" + + def test_human_output_shows_the_feedback_id(self): + _auth() + w = _wrapper(**{"feedback.submit": {"recorded": True, "feedback_id": "fb_42", "request_id": "r1"}}) + with _patch_wrapper(w): + result = runner.invoke(app, ["feedback", "r1", "--feedback", "good"]) + assert result.exit_code == 0 + assert "fb_42" in result.output + + def test_json_output_is_the_payload(self): + _auth() + w = _wrapper(**{"feedback.submit": {"recorded": True, "feedback_id": "fb_1", "request_id": "r1"}}) + with _patch_wrapper(w): + result = runner.invoke(app, ["--output", "json", "feedback", "r1", "--feedback", "good"]) + assert result.exit_code == 0 + assert json.loads(result.output)["feedback_id"] == "fb_1" + + # `recorded: false` means accepted but not durably stored. Rendering that as + # success would tell an evaluation run it was captured when it was not. + def test_not_durably_stored_is_not_reported_as_success(self): + _auth() + w = _wrapper(**{"feedback.submit": {"recorded": False, "request_id": "r1"}}) + with _patch_wrapper(w): + result = runner.invoke(app, ["feedback", "r1", "--feedback", "good"]) + assert "NOT durably stored" in result.output + assert "✓" not in result.output + + def test_rejects_an_invalid_rating_before_calling_out(self): + _auth() + w = _wrapper(**{"feedback.submit": {}}) + with _patch_wrapper(w): + result = runner.invoke(app, ["feedback", "r1", "--feedback", "x", "--rating", "great"]) + assert result.exit_code == 1 + assert "--rating must be one of" in result.output + w.feedback.submit.assert_not_called() + + def test_rejects_an_invalid_source_before_calling_out(self): + """A typo like 'agnet' should cost a message, not a round trip.""" + _auth() + w = _wrapper(**{"feedback.submit": {}}) + with _patch_wrapper(w): + result = runner.invoke(app, ["feedback", "r1", "--feedback", "x", "--source", "agnet"]) + assert result.exit_code == 1 + assert "--source must be one of" in result.output + w.feedback.submit.assert_not_called() + + # Status 0 is this codebase's marker for a TRANSPORT failure, so a wrapper + # refusal raised with it renders as "Connection error:". A local refusal + # must not blame the network. + def test_an_empty_submission_reads_as_a_local_refusal(self): + _auth() + w = _wrapper(**{"feedback.submit": {}}) + with _patch_wrapper(w): + result = runner.invoke(app, ["feedback", "r1"]) + assert result.exit_code == 1 + assert "feedback needs something to record" in result.output + assert "Connection error" not in result.output + w.feedback.submit.assert_not_called() diff --git a/tests/test_wrapper.py b/tests/test_wrapper.py index ad44309..49b747c 100644 --- a/tests/test_wrapper.py +++ b/tests/test_wrapper.py @@ -552,3 +552,143 @@ def fake_request(method, url, **kwargs): with pytest.raises(HydraDBClientError) as exc: self._wrapper().databases.delete_collection(collection="support") assert exc.value.status_code == 0 + + +class TestFeedback: + """``POST /feedback`` — the raw path, and the guards in front of it.""" + + @staticmethod + def _submit(**kwargs): + """Run ``feedback.submit`` against a mock and return (body, result).""" + seen: dict = {} + + def handler(request: httpx.Request) -> httpx.Response: + seen["request"] = request + seen["body"] = json.loads(request.content) + return httpx.Response( + 200, + json={ + "data": { + "recorded": True, + "feedback_id": "fb_1", + "request_id": seen["body"].get("request_id"), + }, + "success": True, + "meta": {"request_id": "outer"}, + }, + ) + + w = _wrapper_with_response({}) + transport = httpx.MockTransport(handler) + with httpx.Client(transport=transport) as client: + original = httpx.post + + def fake_post(url, **kw): + kw.pop("timeout", None) + return client.post(url, **kw) + + httpx.post = fake_post + try: + result = w.feedback.submit(**kwargs) + finally: + httpx.post = original + return seen, result + + def test_submits_over_the_raw_path_and_unwraps(self): + seen, result = self._submit(request_id="r1", feedback="useful") + assert seen["request"].url.path == "/feedback" + # CONTRACT §2 rule 6 — the raw path must send the version header too. + assert seen["request"].headers["API-Version"] == "2" + assert result == {"recorded": True, "feedback_id": "fb_1", "request_id": "r1"} + + def test_request_id_actually_reaches_the_wire(self): + """The whole point of the raw path. + + The SDK's generated model is a union of ``{feedback}`` and + ``{ground_truth}``; neither branch declares ``request_id``, which is the + only key the endpoint correlates on. + """ + seen, _ = self._submit(request_id="r1", feedback="useful") + assert seen["body"]["request_id"] == "r1" + + def test_an_answer_and_source_ids_travel_together(self): + """Also impossible through the SDK: ``ground_truth`` is itself a union + of ``{answer}`` and ``{source_ids}``, so one branch drops the other.""" + seen, _ = self._submit( + request_id="r1", + ground_truth_answer="42", + ground_truth_source_ids=["a", "b"], + ) + assert seen["body"]["ground_truth"] == {"answer": "42", "source_ids": ["a", "b"]} + + def test_source_ids_are_trimmed_and_de_duplicated(self): + """They are SCORED: the same document twice would weight one piece of + evidence as two.""" + seen, _ = self._submit(request_id="r1", ground_truth_source_ids=[" a ", "a", "b", "", " "]) + assert seen["body"]["ground_truth"]["source_ids"] == ["a", "b"] + + def test_rejects_a_submission_with_no_signal(self): + with pytest.raises(HydraDBClientError) as exc: + self._submit(request_id="r1") + assert "--feedback" in str(exc.value) + + def test_rejects_a_blank_request_id(self): + with pytest.raises(HydraDBClientError) as exc: + self._submit(request_id=" ", feedback="useful") + assert "request_id" in str(exc.value) + + def test_caps_ids_only_after_de_duplication(self): + """150 ids collapsing to 80 distinct is a request the server accepts, so + refusing it locally would be worse than the round trip this avoids.""" + seen, _ = self._submit(request_id="r1", ground_truth_source_ids=[f"s{i % 80}" for i in range(150)]) + assert len(seen["body"]["ground_truth"]["source_ids"]) == 80 + + with pytest.raises(HydraDBClientError) as exc: + self._submit(request_id="r1", ground_truth_source_ids=[f"s{i}" for i in range(101)]) + assert "100" in str(exc.value) + + def test_source_defaults_to_user_not_the_servers_default(self): + """A person at a terminal is a user; the MCP client says 'agent' + because its caller is always a model. The row records which.""" + seen, _ = self._submit(request_id="r1", feedback="useful") + assert seen["body"]["source"] == "user" + + seen, _ = self._submit(request_id="r1", feedback="useful", source="agent") + assert seen["body"]["source"] == "agent" + + +class TestQueryRequestId: + """``context.query`` has to surface ``meta.request_id``. + + ``_unwrap`` returns ``.data`` and drops ``meta``, so without this the id is + gone before any caller sees it and ``hydradb feedback`` has nothing to + attach to — the feedback command would ship unusable. + """ + + def test_query_carries_the_request_id_into_its_payload(self): + w = _wrapper_with_response( + { + "data": {"chunks": [{"chunk_content": "x"}]}, + "success": True, + "meta": {"request_id": "rid-123"}, + } + ) + result = w.context.query(query="hello") + assert result["request_id"] == "rid-123" + assert result["chunks"] == [{"chunk_content": "x"}] + + def test_query_without_a_request_id_gains_no_key(self): + """Absent is absent — never invent the one key /feedback correlates on.""" + w = _wrapper_with_response({"data": {"chunks": []}, "success": True, "meta": {}}) + assert "request_id" not in w.context.query(query="hello") + + def test_query_never_overwrites_a_payload_request_id(self): + """If the server ever puts one in `data`, that one is authoritative.""" + w = _wrapper_with_response( + { + "data": {"chunks": [], "request_id": "from-data"}, + "success": True, + "meta": {"request_id": "from-meta"}, + } + ) + assert w.context.query(query="hello")["request_id"] == "from-data"