From aa85b80501add34f9b683ecfa034cab96ca4dd3f Mon Sep 17 00:00:00 2001 From: gun29may Date: Fri, 11 Sep 2026 20:07:54 +0530 Subject: [PATCH 1/2] feat(feedback): add hydradb feedback, and surface the request id it needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /feedback records whether a query's results were actually useful. It correlates on ONE key -- the request_id from that query's response meta -- and nothing else about the original query is re-sent, so nothing has to be trusted from the client. That key was unreachable from the CLI. `_unwrap` returns `.data` and drops `meta`, so a successful query discarded its own request id before any caller saw it. A feedback command alone would therefore have shipped unusable: you could run a query and then have nothing to give feedback ON. So this is two changes: 1. `context.query` carries the request id into its payload. Additive -- /query's data has no `request_id` of its own -- so the documented `--output json` shape gains a key and loses none, and piping it to the feedback command works. The human output prints a copy-pasteable line, including on an EMPTY result: a query that found nothing is the case most worth reporting, and the one with no chunk ids to fall back on. 2. `hydradb feedback REQUEST_ID` itself, plus a _Feedback resource. 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 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 -- the correlation key the endpoint exists for is on neither model. It does work today, and I verified that against staging: the models are extra="allow", so undeclared fields ride along to the wire. 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, this is a one-file change back to the SDK. Details worth knowing: - Ground-truth source ids are trimmed and de-duplicated, because they are scored: the same document listed twice would weight one piece of evidence as two. The 100-id cap is checked AFTER that, since the server also de-duplicates first -- 150 ids collapsing to 80 is a request it accepts, and refusing it locally would be worse than the round trip. - A submission with neither prose nor ground truth is refused HERE. The server refuses it too, but after a round trip. - `--source` defaults to "user", not the "agent" the MCP client sends. A person at a terminal is a user; MCP's caller is always a model. The two populations are separated at write time, so the default has to follow who is actually calling. `--source agent` is there for scripted runs. - `recorded: false` is reported as "accepted but NOT durably stored". Calling it success would tell an eval run it was captured when it was not. 11 tests, and verified end to end against staging: query -> request id surfaced -> feedback recorded, prose and ground-truth paths both. 342 tests + 33 conformance pass, ruff clean. Signed-off-by: gun29may --- CHANGELOG.md | 8 ++ README.md | 38 ++++++ src/hydradb_cli/commands/_impl.py | 77 ++++++++++- src/hydradb_cli/commands/canonical.py | 36 ++++++ src/hydradb_cli/hydra/client.py | 176 +++++++++++++++++++++++++- src/hydradb_cli/main.py | 1 + tests/test_wrapper.py | 140 ++++++++++++++++++++ 7 files changed, 474 insertions(+), 2 deletions(-) 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..249388e 100644 --- a/src/hydradb_cli/commands/_impl.py +++ b/src/hydradb_cli/commands/_impl.py @@ -35,6 +35,7 @@ VALID_MODES = {"fast", "thinking"} VALID_OPERATORS = {"or", "and", "phrase"} VALID_KINDS = {"knowledge", "memory"} +VALID_RATINGS = {"positive", "negative", "neutral"} VALID_FETCH_MODES = {"content", "url", "both"} _STATUS_LABELS = { @@ -69,10 +70,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 +113,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 +177,63 @@ 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}'.") + + 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_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" From e844544e647eed38bc0965c58a7e17c718fc75cf Mon Sep 17 00:00:00 2001 From: gun29may Date: Fri, 11 Sep 2026 20:36:10 +0530 Subject: [PATCH 2/2] fix(feedback): validate --source, and refuse locally without blaming the network MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Greptile P2s on #37, both correct. 1. --source forwarded any string. --rating was validated locally but its sibling was not, so `--source agnet` cost a round trip and a server rejection to learn what a local message could say immediately. 2. The command itself was untested. The wrapper tests call feedback.submit() directly, which left registration, option-to-wrapper mapping, both renderings and the exit code unproven -- and unlike the dashboard PR, this repo already has the harness for it (108 CliRunner invocations in test_cli_commands.py), so there was no reason not to use it. Eight command-level tests cover what the wrapper tests could not: the flags are registered, every option reaches the wrapper, an invalid rating or source is refused BEFORE the call goes out, `recorded: false` does not render as success, and --output json is the payload. Writing them surfaced a third thing, found by driving the real CLI: an empty submission reported ✗ Error: Connection error: feedback needs something to record: ... The wrapper raises HydraDBClientError(0, ...) for a local refusal, but status 0 is this codebase's marker for a TRANSPORT failure -- errors.py uses it only for connect/timeout/network -- and handle_api_error renders it as "Connection error:". So a local refusal blamed the network. The pre-existing guards avoid this by validating in the command layer (require_tenant_id), which is why `No database specified` reads cleanly; the feedback guards now do the same. The wrapper keeps its checks for anyone importing it as a library, but the command layer catches first. 350 tests + 33 conformance pass, ruff clean. Re-verified end to end against staging: every command, all four feedback shapes, and all three guards. Signed-off-by: gun29may --- src/hydradb_cli/commands/_impl.py | 18 +++++ tests/test_cli_commands.py | 105 ++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/src/hydradb_cli/commands/_impl.py b/src/hydradb_cli/commands/_impl.py index 249388e..dba426d 100644 --- a/src/hydradb_cli/commands/_impl.py +++ b/src/hydradb_cli/commands/_impl.py @@ -36,6 +36,10 @@ 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 = { @@ -213,6 +217,20 @@ def do_feedback( ) -> 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) 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()