From c2e24123204a326ecc259eea999a311a6d5bb466 Mon Sep 17 00:00:00 2001 From: Harsh Ranjan Date: Wed, 23 Sep 2026 19:09:56 +0000 Subject: [PATCH 1/3] fix(unified): strict item contract, probe fallback, implied kinds, caps (PRO-2196) Found by running #38 against staging with the unified switch on. - Follow the strict unified item contract (hydradb-application#1653): a conversation turn is exactly {role, content}, and a turn carrying `name` is refused locally; the speaker is the item's `user_name`, now set with --user-name on text and conversations alike; forceful relations are sent as {context_ids}. The 202's context id is read from `results[].id`. - A failed layout probe no longer fails every command. The probe gets 5s and no retries; on a network error, timeout, 429 or 5xx the command warns on stderr and goes out in the split shape, as before layouts existed, and is redone in the unified shape if the database refuses it as unified (CORPUS_TYPE_UNSUPPORTED, or the unified body the SDK cannot parse). Auth and permission failures are still reported as they are. - The deprecated read/delete aliases (recall full/preferences, memories list/delete, knowledge delete) picked their kind themselves, so on a unified database they now act on the one corpus instead of telling the user to drop a --kind they never typed. - The server's per-item caps are checked before sending: 1 MiB of text, a 1024-byte title, 4000 characters of instructions, a context id of at most 100 characters with no commas. - `database create` without --type leaves the layout to the server (unified on current servers); the help text and the success message now say so. - `list` drops the per-item type column on a unified database. Signed-off-by: Harsh Ranjan --- CHANGELOG.md | 6 +- README.md | 12 +- src/hydradb_cli/commands/_impl.py | 325 ++++++++++++++++++++------ src/hydradb_cli/commands/canonical.py | 37 ++- src/hydradb_cli/commands/knowledge.py | 2 +- src/hydradb_cli/commands/memories.py | 3 +- src/hydradb_cli/commands/recall.py | 2 + src/hydradb_cli/hydra/client.py | 14 +- tests/test_unified.py | 257 ++++++++++++++++++-- 9 files changed, 538 insertions(+), 120 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index daeeb4a..cf8a5b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,13 +4,13 @@ ### Added -- **Unified databases (PRO-1618).** `hydradb database create --type unified` provisions a database with ONE corpus instead of separate knowledge and memory corpora, and `database list` shows each database's type. The CLI reads a database's layout once per command from `GET /databases` (`details[].type`) and branches on that, never on a flag: a split database keeps every existing request and rendering exactly as it was, and a unified database never receives `type`. +- **Unified databases (PRO-1618).** `hydradb database create --type unified` provisions a database with ONE corpus instead of separate knowledge and memory corpora, and `database list` shows each database's type. The CLI reads a database's layout once per command from `GET /databases` (`details[].type`) and branches on that, never on a flag: a split database keeps every existing request and rendering exactly as it was, and a unified database never receives `type`. The check has a 5-second budget and no retries. When it cannot answer (network, timeout, 429 or 5xx) the command warns on stderr and goes out in the split shape, as it did before layouts existed; if the database then refuses it as unified (`CORPUS_TYPE_UNSUPPORTED`), the command is redone in the unified shape. An auth or permission failure on the check is reported as it is. Without `--type`, `database create` leaves the layout to the server, and current servers make it unified. On a unified database `query` is a JSON `POST /query` with no `type` and the answer is the four-key unified body (`chunks[]` with `context_id`, `score`, `content`, `enrichment` as a plain string and `enrichment_kind` beside it; `graph[]` with `origin`, `path_summary` and triplets; `forceful_relations[]`, whose `chunk` has the same shape; `llm_prompt`, a markdown document). The human view renders chunks, graph paths grouped by `origin` (query paths apart from chunk relation paths, each chunk relation listed under the returned chunk it hangs under, matched by `relation.chunk_id`) and, when there are any, the forceful relations; the new `--llm` flag prints the server-built markdown `llm_prompt` verbatim on stdout (feedback hint on stderr) so it can be piped into a model call; `--output json` prints the body verbatim, nothing added. `--follow-forceful-relations/--no-follow-forceful-relations` is forwarded. A parser detects the shape by the body (`llm_prompt`, or `graph`/`forceful_relations` as arrays, vs `chunk_content`/`graph_context`; a split body's `graph` and `forceful_relations` are objects), so a unified body that reaches the split path is still rendered as what it is. - `ingest` on a unified database is a JSON `POST /context/ingest` with the `context` list and one item of exactly one `--text` or `--conversation-file` (a JSON list of `{role, content, name?}` turns), plus `--context-id`, `--title`, `--enrich/--no-enrich`, `--instructions`, `--happened-at` (YYYY-MM-DD), `--attributes` and `--custom-attributes` (JSON objects), `--category`, repeatable `--forceful-relation` and `--acl`, and `--upsert/--no-upsert`. Every value is validated locally and named by turn or flag before a round trip. Files are refused on a unified database with a message pointing at `--text`; `--kind`, `--user-name` and `--markdown` are refused there too, and the unified-only options are refused on a split database. The 202's `results[].source_id` is rendered as the item's context id. + `ingest` on a unified database is a JSON `POST /context/ingest` with the `context` list and one item of exactly one `--text` or `--conversation-file` (a JSON list of `{role, content}` turns), plus `--context-id`, `--title`, `--user-name` (the item's `user_name`), `--enrich/--no-enrich`, `--instructions`, `--happened-at` (YYYY-MM-DD), `--attributes` and `--custom-attributes` (JSON objects), `--category`, repeatable `--forceful-relation` (sent as `forceful_relations.context_ids`) and `--acl`, and `--upsert/--no-upsert`. Every value is validated locally and named by turn or flag before a round trip, including the server's per-item caps (1 MiB of text, a 1024-byte title, 4000 characters of instructions, a context id of at most 100 characters with no commas). A turn carrying `name` is refused: the server takes a turn as exactly `{role, content}`. Files are refused on a unified database with a message pointing at `--text`; `--kind` and `--markdown` are refused there too, and the unified-only options are refused on a split database. The 202's `results[].id` is rendered as the item's context id (`source_id` is still read from older servers). - `list`, `delete`, `relations`, `subgraph` and `inspect` send no `type` on a unified database; an explicit `--kind` is refused there rather than silently dropped, and `delete` keeps its `knowledge` default on a split database. The deprecated aliases that hard-code a kind (`recall full`, `memories add`, `knowledge upload`, ...) are refused on a unified database with the same message. The pinned SDK cannot be relied on for any of this, so the unified calls (and `database create --type`) go over the wrapper's raw v2 path with the same headers, envelope unwrap and error translation as the SDK path. The conformance vectors gain `ingest-unified-json`. + `list`, `delete`, `relations`, `subgraph` and `inspect` send no `type` on a unified database; an explicit `--kind` is refused there rather than silently dropped, and `delete` keeps its `knowledge` default on a split database. The deprecated aliases that write with a hard-coded kind (`memories add`, `knowledge upload`, ...) are refused on a unified database with a message naming `hydradb ingest`; the read and delete aliases (`recall full`, `recall preferences`, `memories list`, `memories delete`, `knowledge delete`) chose that kind themselves, so on a unified database they drop it and act on the one corpus. `list` shows no per-item type column on a unified database. The pinned SDK cannot be relied on for any of this, so the unified calls (and `database create --type`) go over the wrapper's raw v2 path with the same headers, envelope unwrap and error translation as the SDK path. The conformance vectors gain `ingest-unified-json`. - **`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. diff --git a/README.md b/README.md index 5789c64..8b9b4fd 100644 --- a/README.md +++ b/README.md @@ -308,7 +308,7 @@ defaults to `--kind memory`, and file arguments are always knowledge sources. | `--text` / `-t` | Text to ingest. Use `-` to read from stdin | | `--title` | Optional title | | `--source-id` | Client-assigned source identifier (the `--context-id` on a unified database) | -| `--user-name` | User name (split memory only) | +| `--user-name` | Who is speaking (split: memory only; unified: any item, sent as `user_name`) | | `--infer` / `--no-infer` | Extract insights and build the knowledge graph (default on) | | `--markdown` | Treat text as markdown (split memory only) | | `--upsert` / `--no-upsert` | Update existing items with the same id (default on) | @@ -325,13 +325,13 @@ echo "piped note" | hydradb ingest apply to file ingest and are rejected rather than silently ignored. On a **unified database** `ingest` sends one JSON context item (exactly one of -`--text` or `--conversation-file`) and never a `type`. Files, `--kind`, -`--user-name` and `--markdown` are refused there with a message; these options -apply there and are refused on a split database: +`--text` or `--conversation-file`) and never a `type`. Files, `--kind` and +`--markdown` are refused there with a message; `--user-name` names the speaker. +These options apply there and are refused on a split database: | Option | Description | |--------|-------------| -| `--conversation-file` | Path to a JSON list of `{role, content, name?}` turns (roles `user`, `assistant`, `system`) | +| `--conversation-file` | Path to a JSON list of `{role, content}` turns (roles `user`, `assistant`, `system`); name the user with `--user-name` | | `--context-id` | Caller-assigned id for the item (server-generated when omitted) | | `--enrich` / `--no-enrich` | Extract facts and graph relations for the item (default on; `--no-infer` means the same) | | `--instructions` | Steer enrichment for this item | @@ -401,7 +401,7 @@ Create and manage databases. | Command | What it does | Key options | |---------|--------------|-------------| -| `database create ` | Provisions a new database; `--type unified` gives it one corpus (no `--kind` on later commands) instead of the default `split` layout | `--type` | +| `database create ` | Provisions a new database; `--type split` or `--type unified` picks the layout. Without `--type` the server picks, and current servers pick `unified` | `--type` | | `database list` | Lists all databases for the authenticated user, with each one's type (`split` or `unified`) | - | | `database collections [database]` | Lists collections within a database | — | | `database stats [database]` | Row-count statistics | — | diff --git a/src/hydradb_cli/commands/_impl.py b/src/hydradb_cli/commands/_impl.py index 18ddd00..21e6502 100644 --- a/src/hydradb_cli/commands/_impl.py +++ b/src/hydradb_cli/commands/_impl.py @@ -62,6 +62,12 @@ VALID_ROLES = {"user", "assistant", "system"} # happened_at is a calendar date, YYYY-MM-DD only: no time, no zone. _DATE_RE = re.compile(r"^\d{4}-\d{2}-\d{2}$") +# The server's per-item caps on a unified ingest (hydradb-application#1657), +# checked locally so an item it would refuse is named before the round trip. +UNIFIED_MAX_TEXT_BYTES = 1 << 20 +UNIFIED_MAX_TITLE_BYTES = 1024 +UNIFIED_MAX_INSTRUCTIONS_CHARS = 4000 +UNIFIED_MAX_CONTEXT_ID_CHARS = 100 _STATUS_LABELS = { "queued": "queued", @@ -95,30 +101,100 @@ def _execute(spinner_msg: str, call: Callable[[], Any]) -> Any: # ── storage layout (PRO-1618) ──────────────────────────────────────────────── -def _is_unified(wrapper: Any, database: str) -> bool: - """Whether ``database`` is a unified database. +#: The layout probe could not answer (network, timeout, 429 or 5xx). The +#: command then goes out in the split shape, which is what every database +#: predating PRO-1618 takes and what the CLI sent before layouts existed, and +#: is redone in the unified shape if the database refuses it as unified. +LAYOUT_UNKNOWN = "unknown" + + +def _probe_failure_is_transient(e: HydraDBClientError) -> bool: + """A probe failure the command itself may not hit: the server could not + answer the layout question just now. An auth or permission failure is not + one; the command's own call would fail the same way, so it is reported.""" + return e.status_code == 0 or e.status_code == 429 or e.status_code >= 500 + + +def _layout(wrapper: Any, database: str) -> str: + """``unified``, ``split`` or ``unknown`` for ``database``. One memoised ``GET /databases`` probe per wrapper. A successful probe that - does not list ``database`` reads as split, which is what every pre-PRO-1618 - database is; a FAILED probe is the error it is, not a guess: guessing - split would send the split request shape to a database that may be - unified. Compared by value so a mocked wrapper (whose ``layout`` returns a - MagicMock) reads as split too. - Every command branches on THIS, never on a request flag: a unified database - never receives ``type``, and a split one keeps every existing call as is. + does not list ``database`` reads as split, which is what every + pre-PRO-1618 database is. A transient probe failure reads as unknown, with + a warning on stderr; any other failure is the error it is. Compared by + value so a mocked wrapper (whose ``layout`` returns a MagicMock) reads as + split. Every command branches on THIS, never on a request flag: a unified + database never receives ``type``, and a split one keeps every existing + call as is. """ try: - return wrapper.databases.layout(database) == LAYOUT_UNIFIED + return LAYOUT_UNIFIED if wrapper.databases.layout(database) == LAYOUT_UNIFIED else LAYOUT_SPLIT except HydraDBClientError as e: - handle_api_error(e) + if not _probe_failure_is_transient(e): + handle_api_error(e) + reason = f"HTTP {e.status_code}" if e.status_code else "no response" except httpx.RequestError as e: - handle_network_error(e) + reason = type(e).__name__ + err_console.print( + f" [hydra.warning]![/hydra.warning] Could not check whether '{escape(database)}' is split or unified " + f"({escape(reason)}); sending the split request, and redoing it as unified if the database says it is." + ) + return LAYOUT_UNKNOWN + + +def _is_unified(wrapper: Any, database: str) -> bool: + """Whether ``database`` is known to be unified (see :func:`_layout`).""" + return _layout(wrapper, database) == LAYOUT_UNIFIED def database_layout(tenant_id: str | None) -> tuple[str, str]: - """The database a command is about to touch and its layout, ``unified`` or ``split``.""" + """The database a command is about to touch and its layout: ``unified``, ``split`` or ``unknown``.""" tid = require_tenant_id(tenant_id) - return tid, (LAYOUT_UNIFIED if _is_unified(get_wrapper(), tid) else LAYOUT_SPLIT) + return tid, _layout(get_wrapper(), tid) + + +def _refused_as_unified(e: HydraDBClientError) -> bool: + """The server's answer to a split-shaped call on a unified database. + + Either a refusal (400 ``CORPUS_TYPE_UNSUPPORTED``; the SDK path keeps only + the message, so that is matched too) or, for a type-less query, a 200 + carrying the unified body, which the SDK's split response model cannot + parse.""" + detail = e.detail or "" + if e.status_code == 400: + return "CORPUS_TYPE_UNSUPPORTED" in detail or "is not valid on a unified database" in detail + return e.status_code == 200 and "llm_prompt" in detail + + +def _execute_or_unified( + spinner_msg: str, + call: Callable[[], Any], + layout: str, + database: str, + on_unified: Callable[[], None], +) -> Any: + """:func:`_execute`, except that when the layout is unknown and the + database refuses the split call as unified, ``on_unified`` runs instead + (it redoes the command in the unified shape, or explains why it cannot) + and :data:`UNIFIED_DONE` is returned. With a known layout this is exactly + :func:`_execute`.""" + if layout != LAYOUT_UNKNOWN: + return _execute(spinner_msg, call) + try: + with spinner(spinner_msg): + return call() + except HydraDBClientError as e: + if not _refused_as_unified(e): + handle_api_error(e) + except httpx.RequestError as e: + handle_network_error(e) + err_console.print(f" [dim]'{escape(database)}' is unified; redoing the request in the unified shape.[/dim]") + on_unified() + return UNIFIED_DONE + + +#: Returned by :func:`_execute_or_unified` when the unified redo ran. +UNIFIED_DONE = object() def _refuse_kind_on_unified(kind: str | None, database: str) -> None: @@ -135,16 +211,22 @@ def _refuse_kind_on_unified(kind: str | None, database: str) -> None: ) -def _refuse_split_write_on_unified(wrapper: Any, database: str, layout: str | None, what: str) -> None: +def _refuse_split_write_on_unified(wrapper: Any, database: str, layout: str | None, what: str) -> str: """The split ingest shapes (``memories``/``app_knowledge``/``documents`` with ``type``) are refused by a unified database. Probed here unless the - caller already resolved the layout, so the deprecated aliases are covered.""" - unified = layout == LAYOUT_UNIFIED if layout is not None else _is_unified(wrapper, database) - if unified: - print_error( - f"Database '{database}' is unified: {what}. " - "Use 'hydradb ingest --text ...' or 'hydradb ingest --conversation-file ...' (no --kind)." - ) + caller already resolved the layout, so the deprecated aliases are covered. + Returns the layout (``split`` or ``unknown``) the write goes out under.""" + resolved = layout if layout is not None else _layout(wrapper, database) + if resolved == LAYOUT_UNIFIED: + _refuse_split_write(database, what) + return resolved + + +def _refuse_split_write(database: str, what: str) -> None: + print_error( + f"Database '{database}' is unified: {what}. " + "Use 'hydradb ingest --text ...' or 'hydradb ingest --conversation-file ...' (no --kind)." + ) # ── query ──────────────────────────────────────────────────────────────────── @@ -445,7 +527,11 @@ def do_query( tenant_id: str | None = None, sub_tenant_id: str | None = None, spinner_msg: str = "Searching...", + kind_implied: bool = False, ) -> None: + """``kind_implied`` marks a kind the command picked (the deprecated + ``recall`` aliases), not one the user typed: a unified database has no + kinds, so it is dropped there instead of refused.""" if not query.strip(): print_error("Query cannot be empty.") if kind and kind not in VALID_KINDS: @@ -477,8 +563,8 @@ def do_query( stid = resolve_sub_tenant_id(sub_tenant_id) wrapper = get_wrapper() - if _is_unified(wrapper, tid): - _refuse_kind_on_unified(kind, tid) + def run_unified() -> None: + _refuse_kind_on_unified(None if kind_implied else kind, tid) outcome = _execute( spinner_msg, lambda: wrapper.context.query_unified( @@ -500,17 +586,25 @@ def do_query( ) body, request_id = outcome if isinstance(outcome, tuple) else (outcome, None) _print_unified_query(body if isinstance(body, dict) else {}, request_id, llm=llm) + + layout = _layout(wrapper, tid) + if layout == LAYOUT_UNIFIED: + run_unified() return - if follow_forceful_relations is not None: + if follow_forceful_relations is not None and layout == LAYOUT_SPLIT: print_error( "--follow-forceful-relations/--no-follow-forceful-relations applies to unified databases only; " f"'{tid}' is a split database." ) - if llm: + if llm and layout == LAYOUT_SPLIT: print_error(f"--llm applies to unified databases only; '{tid}' is a split database and has no llm_prompt.") + if layout == LAYOUT_UNKNOWN and (llm or follow_forceful_relations is not None): + # Only a unified database takes these, so the request goes out unified. + run_unified() + return - result = _execute( + result = _execute_or_unified( spinner_msg, lambda: wrapper.context.query( query=query, @@ -528,7 +622,12 @@ def do_query( database=tid, collection=stid, ), + layout, + tid, + run_unified, ) + if result is UNIFIED_DONE: + return print_result(result, _format_query_result) @@ -640,13 +739,18 @@ def do_ingest_memory( tenant_id: str | None = None, sub_tenant_id: str | None = None, layout: str | None = None, + on_unified: Callable[[], None] | None = None, ) -> None: + """``on_unified`` redoes the write in the unified shape when the layout + was unknown and the database turned out to be unified; without one, the + refusal is explained instead.""" tid = require_tenant_id(tenant_id) stid = resolve_sub_tenant_id(sub_tenant_id) wrapper = get_wrapper() - _refuse_split_write_on_unified(wrapper, tid, layout, "a memory (kind) cannot be written to it") + what = "a memory (kind) cannot be written to it" + layout = _refuse_split_write_on_unified(wrapper, tid, layout, what) - result = _execute( + result = _execute_or_unified( "Adding memory...", lambda: wrapper.context.ingest( kind="memory", @@ -660,7 +764,12 @@ def do_ingest_memory( database=tid, collection=stid, ), + layout, + tid, + on_unified or (lambda: _refuse_split_write(tid, what)), ) + if result is UNIFIED_DONE: + return print_result(result, lambda r: _format_ingest_memory(r, text)) @@ -672,13 +781,16 @@ def do_ingest_knowledge_text( tenant_id: str | None = None, sub_tenant_id: str | None = None, layout: str | None = None, + on_unified: Callable[[], None] | None = None, ) -> None: + """``on_unified``: see :func:`do_ingest_memory`.""" tid = require_tenant_id(tenant_id) stid = resolve_sub_tenant_id(sub_tenant_id) wrapper = get_wrapper() - _refuse_split_write_on_unified(wrapper, tid, layout, "knowledge text (kind) cannot be written to it") + what = "knowledge text (kind) cannot be written to it" + layout = _refuse_split_write_on_unified(wrapper, tid, layout, what) - result = _execute( + result = _execute_or_unified( "Uploading text...", lambda: wrapper.context.ingest( kind="knowledge", @@ -688,7 +800,12 @@ def do_ingest_knowledge_text( database=tid, collection=stid, ), + layout, + tid, + on_unified or (lambda: _refuse_split_write(tid, what)), ) + if result is UNIFIED_DONE: + return def fmt(r: dict): preview = text[:80] + "..." if len(text) > 80 else text @@ -747,11 +864,10 @@ def do_ingest_knowledge_files( tid = require_tenant_id(tenant_id) stid = resolve_sub_tenant_id(sub_tenant_id) wrapper = get_wrapper() - _refuse_split_write_on_unified( - wrapper, tid, layout, "files are not accepted (text or a conversation only). Extract the text first" - ) + what = "files are not accepted (text or a conversation only). Extract the text first" + layout = _refuse_split_write_on_unified(wrapper, tid, layout, what) - result = _execute( + result = _execute_or_unified( f"Uploading {len(files)} file(s)...", lambda: wrapper.context.ingest_many( kind="knowledge", @@ -760,6 +876,9 @@ def do_ingest_knowledge_files( database=tid, collection=stid, ), + layout, + tid, + lambda: _refuse_split_write(tid, what), ) finally: for fh in opened: @@ -793,8 +912,8 @@ def fmt(r: dict): def _load_conversation(path: str) -> list[dict[str, Any]]: - """Read ``--conversation-file``: a JSON list of ``{role, content, name?}`` - turns. Every turn is checked here so a bad one is named by index locally + """Read ``--conversation-file``: a JSON list of ``{role, content}`` + turns (the speaker's name is the item's ``user_name``, from --user-name). Every turn is checked here so a bad one is named by index locally rather than as ``context[0]`` after a round trip.""" p = Path(path) if not p.is_file(): @@ -802,9 +921,9 @@ def _load_conversation(path: str) -> list[dict[str, Any]]: try: turns = json.loads(p.read_text(encoding="utf-8")) except (OSError, ValueError) as exc: - print_error(f"--conversation-file must be a JSON list of {{role, content, name?}} turns: {exc}") + print_error(f"--conversation-file must be a JSON list of {{role, content}} turns: {exc}") if not isinstance(turns, list) or not turns: - print_error("--conversation-file must be a non-empty JSON list of {role, content, name?} turns.") + print_error("--conversation-file must be a non-empty JSON list of {role, content} turns.") clean: list[dict[str, Any]] = [] for i, turn in enumerate(turns): if not isinstance(turn, dict): @@ -815,18 +934,17 @@ def _load_conversation(path: str) -> list[dict[str, Any]]: content = turn.get("content") if not isinstance(content, str) or not content.strip(): print_error(f"conversation[{i}].content must be a non-empty string.") - unknown = sorted(set(turn) - {"role", "content", "name"}) + if "name" in turn: + print_error( + f"conversation[{i}] has a name: a turn is only {{role, content}}. " + "Pass the user's name once with --user-name instead." + ) + unknown = sorted(set(turn) - {"role", "content"}) if unknown: print_error( - f"conversation[{i}] has unknown field(s): {', '.join(unknown)}. Only role, content and name are accepted." + f"conversation[{i}] has unknown field(s): {', '.join(unknown)}. Only role and content are accepted." ) - item: dict[str, Any] = {"role": role, "content": content} - name = turn.get("name") - if name is not None: - if not isinstance(name, str) or not name.strip(): - print_error(f"conversation[{i}].name must be a non-empty string when present.") - item["name"] = name - clean.append(item) + clean.append({"role": role, "content": content}) return clean @@ -848,6 +966,7 @@ def build_context_item( conversation: list[dict[str, Any]] | None = None, context_id: str | None = None, title: str | None = None, + user_name: str | None = None, enrich: bool = True, instructions: str | None = None, happened_at: str | None = None, @@ -868,16 +987,32 @@ def build_context_item( print_error("Pass exactly one of --text or --conversation-file.") item: dict[str, Any] = {} if context_id: + if len(context_id) > UNIFIED_MAX_CONTEXT_ID_CHARS or "," in context_id: + print_error(f"--context-id must be at most {UNIFIED_MAX_CONTEXT_ID_CHARS} characters with no commas.") item["context_id"] = context_id if title: + if len(title.encode("utf-8")) > UNIFIED_MAX_TITLE_BYTES: + print_error(f"--title must be at most {UNIFIED_MAX_TITLE_BYTES} bytes.") item["title"] = title if text is not None: item["text"] = text else: item["conversation"] = conversation + body = text if text is not None else "".join(turn["content"] for turn in conversation or []) + if len(body.encode("utf-8")) > UNIFIED_MAX_TEXT_BYTES: + print_error( + f"The item is {len(body.encode('utf-8')):,} bytes of text; a unified database takes at most " + f"{UNIFIED_MAX_TEXT_BYTES:,} per item. Split it into several items." + ) + if user_name is not None: + if not user_name.strip(): + print_error("--user-name cannot be empty or whitespace-only.") + item["user_name"] = user_name item["enrich"] = bool(enrich) item["upsert"] = bool(upsert) if instructions: + if len(instructions) > UNIFIED_MAX_INSTRUCTIONS_CHARS: + print_error(f"--instructions must be at most {UNIFIED_MAX_INSTRUCTIONS_CHARS} characters.") item["instructions"] = instructions if happened_at: try: @@ -903,7 +1038,7 @@ def build_context_item( print_error("--forceful-relation cannot be empty or whitespace-only.") if candidate not in ids: ids.append(candidate) - item["forceful_relations"] = {"ids": ids} + item["forceful_relations"] = {"context_ids": ids} if acl is not None: item["acl"] = list(acl) return item @@ -924,8 +1059,8 @@ def _format_ingest_unified(r: dict, item: dict[str, Any]): f"[dim]{escape(preview)}[/dim]", ] for res in r.get("results", []) or []: - # The 202 still spells the item's context_id `source_id`. - cid = res.get("source_id") or res.get("context_id") or res.get("id") or "unknown" + # The 202 names the item's context_id `id`; older servers said `source_id`. + cid = res.get("id") or res.get("context_id") or res.get("source_id") or "unknown" lines.append(f"[cyan]Context ID:[/cyan] {escape(str(cid))} [dim]({res.get('status', 'unknown')})[/dim]") if res.get("error"): code = f" ({res['error_code']})" if res.get("error_code") else "" @@ -940,6 +1075,7 @@ def do_ingest_unified( conversation_file: str | None = None, context_id: str | None = None, title: str | None = None, + user_name: str | None = None, enrich: bool = True, instructions: str | None = None, happened_at: str | None = None, @@ -964,6 +1100,7 @@ def do_ingest_unified( conversation=conversation, context_id=context_id, title=title, + user_name=user_name, enrich=enrich, instructions=instructions, happened_at=happened_at, @@ -1006,7 +1143,9 @@ def do_list( tenant_id: str | None = None, sub_tenant_id: str | None = None, spinner_msg: str = "Fetching sources...", + kind_implied: bool = False, ) -> None: + """``kind_implied``: see :func:`do_query`.""" if kind and kind not in VALID_KINDS: print_error(f"--kind must be one of: {', '.join(sorted(VALID_KINDS))}. Got '{kind}'.") if page is not None and page < 1: @@ -1017,21 +1156,29 @@ def do_list( tid = require_tenant_id(tenant_id) stid = resolve_sub_tenant_id(sub_tenant_id) wrapper = get_wrapper() - if _is_unified(wrapper, tid): + layout = _layout(wrapper, tid) + + def listing(list_kind: str | None) -> Callable[[], Any]: + return lambda: wrapper.context.list( + kind=list_kind, page=page, page_size=page_size, acl=acl, database=tid, collection=stid + ) + + unified_result: list[Any] = [] + + def run_unified() -> None: # One corpus: no kind is selected and none is sent. - _refuse_kind_on_unified(kind, tid) + _refuse_kind_on_unified(None if kind_implied else kind, tid) + unified_result.append(_execute(spinner_msg, listing(None))) - result = _execute( - spinner_msg, - lambda: wrapper.context.list( - kind=kind, - page=page, - page_size=page_size, - acl=acl, - database=tid, - collection=stid, - ), - ) + if layout == LAYOUT_UNIFIED: + run_unified() + result = unified_result[0] + else: + result = _execute_or_unified(spinner_msg, listing(kind), layout, tid, run_unified) + if result is UNIFIED_DONE: + result = unified_result[0] + # A unified database has one corpus, so a per-item kind would only mislead. + show_type = not unified_result def fmt(r: dict): items = r.get("sources") or r.get("user_memories") or [] @@ -1042,8 +1189,9 @@ def fmt(r: dict): sid = item.get("id") or item.get("memory_id") or item.get("source_id") or "unknown" title = item.get("title") or item.get("memory_content") or item.get("content") or item.get("text") or "" title = title[:100] + "..." if len(title) > 100 else title - rows.append([str(i), sid, title, item.get("type", "")]) - table = make_table("#", "ID", "Title", "Type", rows=rows, title=f"Found {len(items)} item(s)") + rows.append([str(i), sid, title, item.get("type", "")] if show_type else [str(i), sid, title]) + columns = ("#", "ID", "Title", "Type") if show_type else ("#", "ID", "Title") + table = make_table(*columns, rows=rows, title=f"Found {len(items)} item(s)") parts: list[Any] = [table] footer_parts = [] @@ -1133,7 +1281,9 @@ def do_delete( kind: str | None, tenant_id: str | None = None, sub_tenant_id: str | None = None, + kind_implied: bool = False, ) -> None: + """``kind_implied``: see :func:`do_query`.""" clean_ids = [i.strip() for i in ids if i.strip()] if not clean_ids: print_error("IDs cannot be empty.") @@ -1143,19 +1293,34 @@ def do_delete( tid = require_tenant_id(tenant_id) stid = resolve_sub_tenant_id(sub_tenant_id) wrapper = get_wrapper() - if _is_unified(wrapper, tid): + layout = _layout(wrapper, tid) + unified_result: list[Any] = [] + + def run_unified() -> None: # One corpus: no kind is selected and none is sent. - _refuse_kind_on_unified(kind, tid) - noun = "item(s)" + _refuse_kind_on_unified(None if kind_implied else kind, tid) + unified_result.append( + _execute( + "Deleting...", lambda: wrapper.context.delete(ids=clean_ids, kind=None, database=tid, collection=stid) + ) + ) + + if layout == LAYOUT_UNIFIED: + run_unified() + result, noun = unified_result[0], "item(s)" else: # The split default, unchanged: a delete without --kind is a knowledge delete. - kind = kind or "knowledge" - noun = "memory" if kind == "memory" else "knowledge source(s)" - - result = _execute( - "Deleting...", - lambda: wrapper.context.delete(ids=clean_ids, kind=kind, database=tid, collection=stid), - ) + split_kind = kind or "knowledge" + noun = "memory" if split_kind == "memory" else "knowledge source(s)" + result = _execute_or_unified( + "Deleting...", + lambda: wrapper.context.delete(ids=clean_ids, kind=split_kind, database=tid, collection=stid), + layout, + tid, + run_unified, + ) + if result is UNIFIED_DONE: + result, noun = unified_result[0], "item(s)" # v2 returns HTTP 200 with {success:false, deleted_count:0} when nothing # matched — that is a no-op, not a success. Surface it as an error (non-zero @@ -1385,7 +1550,13 @@ def do_database_create(database: str, layout: str | None = None) -> None: "Creating database...", lambda: wrapper.databases.create(database=database, layout=layout), ) - suffix = " (unified: one corpus, no --kind on later commands)" if layout == LAYOUT_UNIFIED else "" + if layout == LAYOUT_UNIFIED: + suffix = " (unified: one corpus, no --kind on later commands)" + elif layout == LAYOUT_SPLIT: + suffix = " (split: knowledge and memory, selected by --kind)" + else: + # No --type: the server picks, and current servers pick unified. + suffix = " (the server's default layout; 'hydradb database list' shows which)" print_result(result, lambda r: f"[green]✓[/green] Database [bold]{database}[/bold] created successfully.{suffix}") diff --git a/src/hydradb_cli/commands/canonical.py b/src/hydradb_cli/commands/canonical.py index c4d722c..135f658 100644 --- a/src/hydradb_cli/commands/canonical.py +++ b/src/hydradb_cli/commands/canonical.py @@ -153,14 +153,16 @@ def ingest( source_id: str | None = typer.Option( None, "--source-id", help="Source identifier (--context-id on a unified database)." ), - user_name: str | None = typer.Option(None, "--user-name", help="User name (split memory only)."), + user_name: str | None = typer.Option( + None, "--user-name", help="Who is speaking: the user's name (split: memory only; unified: any item)." + ), infer: bool = typer.Option(True, "--infer/--no-infer", help="Extract insights and build knowledge graph."), markdown: bool = typer.Option(False, "--markdown", help="Treat text as markdown (split memory only)."), upsert: bool = typer.Option(True, "--upsert/--no-upsert", help="Update existing items with the same id."), conversation_file: str | None = typer.Option( None, "--conversation-file", - help="Unified databases: path to a JSON list of {role, content, name?} turns to ingest as one conversation (roles: user, assistant, system).", + help="Unified databases: path to a JSON list of {role, content} turns to ingest as one conversation (roles: user, assistant, system). Name the user with --user-name.", ), context_id: str | None = typer.Option( None, @@ -210,7 +212,10 @@ def ingest( # database gets one JSON context item and never a kind; a split one keeps # every existing call exactly as it was. db, layout = _impl.database_layout(tid) - if layout == "unified": + + def run_unified(resolved_text: str | None = None) -> None: + """The unified write. ``resolved_text`` is the text already read on the + split path (a redo); otherwise it is read here, after the checks.""" if files: print_error( f"Database '{db}' is unified: files are not accepted (text or a conversation only). " @@ -218,21 +223,20 @@ def ingest( ) if kind: print_error(f"Database '{db}' is unified: it has one corpus, so --kind does not apply. Omit it.") - if user_name: - print_error( - "--user-name does not apply on a unified database; name speakers per turn in --conversation-file." - ) if markdown: print_error("--markdown does not apply on a unified database.") if conversation_file and text: print_error("Pass exactly one of --text or --conversation-file.") if context_id and source_id and context_id != source_id: print_error("--context-id and --source-id name the same thing on a unified database; pass one of them.") + if not conversation_file and resolved_text is None: + resolved_text = _resolve_text_input(text) _impl.do_ingest_unified( - text=None if conversation_file else _resolve_text_input(text), + text=None if conversation_file else resolved_text, conversation_file=conversation_file, context_id=context_id or source_id, title=title, + user_name=user_name, enrich=enrich and infer, instructions=instructions, happened_at=happened_at, @@ -245,6 +249,9 @@ def ingest( tenant_id=tid, sub_tenant_id=stid, ) + + if layout == "unified": + run_unified() return unified_only = { @@ -260,6 +267,11 @@ def ingest( "--acl": acl, } used = [flag for flag, value in unified_only.items() if value] + if used and layout == _impl.LAYOUT_UNKNOWN: + print_error( + f"{', '.join(used)} appl{'ies' if len(used) == 1 else 'y'} to unified databases only, and whether " + f"'{db}' is unified could not be checked just now. Try again." + ) if used: print_error(f"{', '.join(used)} appl{'ies' if len(used) == 1 else 'y'} to unified databases only.") if files: @@ -276,18 +288,20 @@ def ingest( print_error("--infer/--no-infer does not apply to file ingest; pass files only.") _impl.do_ingest_knowledge_files(files, upsert=upsert, tenant_id=tid, sub_tenant_id=stid, layout=layout) return + resolved = _resolve_text_input(text) if kind == "knowledge": _impl.do_ingest_knowledge_text( - _resolve_text_input(text), + resolved, title=title, source_id=source_id, tenant_id=tid, sub_tenant_id=stid, layout=layout, + on_unified=lambda: run_unified(resolved), ) return _impl.do_ingest_memory( - _resolve_text_input(text), + resolved, title=title, source_id=source_id, user_name=user_name, @@ -297,6 +311,7 @@ def ingest( tenant_id=tid, sub_tenant_id=stid, layout=layout, + on_unified=lambda: run_unified(resolved), ) @@ -487,7 +502,7 @@ def database_create( layout: str | None = typer.Option( None, "--type", - help="Storage layout: 'split' (default; separate knowledge and memory corpora selected by --kind) or 'unified' (one corpus; no --kind on later commands).", + help="Storage layout: 'split' (separate knowledge and memory corpora selected by --kind) or 'unified' (one corpus; no --kind on later commands). Omitted: the server's default, which is unified on current servers.", ), ) -> None: """Create a new database.""" diff --git a/src/hydradb_cli/commands/knowledge.py b/src/hydradb_cli/commands/knowledge.py index 8e9d808..fbeb2b8 100644 --- a/src/hydradb_cli/commands/knowledge.py +++ b/src/hydradb_cli/commands/knowledge.py @@ -66,4 +66,4 @@ def delete( print_error("At least one source ID is required.") if not confirm: typer.confirm(f"Delete {len(clean_ids)} knowledge source(s)? This action is irreversible.", abort=True) - _impl.do_delete(clean_ids, kind="knowledge", tenant_id=tenant_id, sub_tenant_id=sub_tenant_id) + _impl.do_delete(clean_ids, kind="knowledge", tenant_id=tenant_id, sub_tenant_id=sub_tenant_id, kind_implied=True) diff --git a/src/hydradb_cli/commands/memories.py b/src/hydradb_cli/commands/memories.py index 8a0ddca..508dad5 100644 --- a/src/hydradb_cli/commands/memories.py +++ b/src/hydradb_cli/commands/memories.py @@ -52,6 +52,7 @@ def list_memories( warn_deprecated("memories list", "list --kind memory") _impl.do_list( kind="memory", + kind_implied=True, acl=list(acl) if acl else None, tenant_id=tenant_id, sub_tenant_id=sub_tenant_id, @@ -70,4 +71,4 @@ def delete( warn_deprecated("memories delete", "delete --kind memory") if not confirm: typer.confirm(f"Delete memory '{memory_id}'? This action is irreversible.", abort=True) - _impl.do_delete([memory_id], kind="memory", tenant_id=tenant_id, sub_tenant_id=sub_tenant_id) + _impl.do_delete([memory_id], kind="memory", tenant_id=tenant_id, sub_tenant_id=sub_tenant_id, kind_implied=True) diff --git a/src/hydradb_cli/commands/recall.py b/src/hydradb_cli/commands/recall.py index fd2b9aa..0680804 100644 --- a/src/hydradb_cli/commands/recall.py +++ b/src/hydradb_cli/commands/recall.py @@ -35,6 +35,7 @@ def full_recall( _impl.do_query( query, kind="knowledge", + kind_implied=True, max_results=max_results, mode=mode, alpha=alpha, @@ -69,6 +70,7 @@ def recall_preferences( _impl.do_query( query, kind="memory", + kind_implied=True, max_results=max_results, mode=mode, alpha=alpha, diff --git a/src/hydradb_cli/hydra/client.py b/src/hydradb_cli/hydra/client.py index 28e12b2..2c73666 100644 --- a/src/hydradb_cli/hydra/client.py +++ b/src/hydradb_cli/hydra/client.py @@ -121,6 +121,11 @@ def _bool_str(value: bool | None) -> str | None: #: oversized batch is refused before it costs a round trip. UNIFIED_INGEST_MAX_ITEMS = 100 +#: The layout probe runs before every command, so it gets a short budget and +#: no SDK retries: a slow or failing ``GET /databases`` must not hold up (or +#: fail) a command that would otherwise succeed. +LAYOUT_PROBE_TIMEOUT_S = 5 + class _Resource: """Base for the ``databases``/``context`` sub-resources.""" @@ -205,7 +210,12 @@ def layouts(self) -> dict[str, str]: """ if self._w._layouts is not None: return self._w._layouts - listed = self.list() + listed = _unwrap( + self._invoke( + self._w._sdk.databases.list, + request_options={"timeout_in_seconds": LAYOUT_PROBE_TIMEOUT_S, "max_retries": 0}, + ) + ) layouts: dict[str, str] = {} rows = listed.get("details") if isinstance(listed, dict) else None for row in rows or []: @@ -510,7 +520,7 @@ def ingest_context( given; ``enrich``/``upsert``/``instructions`` are the request-level defaults for them and travel only when set. - Returns the 202 payload: ``results[].source_id`` is the item's + Returns the 202 payload: ``results[].id`` is the item's ``context_id`` (server-minted when the item carried none). """ if not items: diff --git a/tests/test_unified.py b/tests/test_unified.py index cc00c8d..8078e3b 100644 --- a/tests/test_unified.py +++ b/tests/test_unified.py @@ -68,7 +68,7 @@ "attributes": {"team": "support"}, "custom_attributes": {"source_app": "wiki"}, "context_category": "business_knowledge", - "forceful_relations": {"ids": ["chat-w1"]}, + "forceful_relations": {"context_ids": ["chat-w1"]}, "acl": ["user_email:a@x.com", "domain:acme.com"], } @@ -383,9 +383,10 @@ def test_request_level_defaults_travel_only_when_set(self, monkeypatch): def test_a_conversation_item_is_sent_as_given(self, monkeypatch): capture = _capture_post(monkeypatch, status=202, body={"success": True, "data": INGEST_202, "meta": {}}) - turns = [{"role": "user", "content": "hi", "name": "soham"}, {"role": "assistant", "content": "hello"}] - _real_wrapper(_sdk_500).context.ingest_context([{"context_id": "chat-w1", "conversation": turns}]) - assert capture.calls[0]["json"]["context"] == [{"context_id": "chat-w1", "conversation": turns}] + turns = [{"role": "user", "content": "hi"}, {"role": "assistant", "content": "hello"}] + item = {"context_id": "chat-w1", "user_name": "soham", "conversation": turns} + _real_wrapper(_sdk_500).context.ingest_context([item]) + assert capture.calls[0]["json"]["context"] == [item] def test_refuses_an_item_with_both_or_neither_shape(self): w = _real_wrapper(_sdk_500) @@ -531,15 +532,25 @@ def test_kind_is_refused_on_a_unified_database(self): w.context.query.assert_not_called() w.context.query_unified.assert_not_called() - def test_deprecated_recall_aliases_are_refused_on_a_unified_database(self): + def test_deprecated_recall_aliases_search_the_one_corpus_on_a_unified_database(self): + # These aliases picked their kind themselves; the user never typed one, + # so on a unified database it is dropped rather than refused. _auth() for argv in (["recall", "full", "x"], ["recall", "preferences", "x"]): - w = _mock("unified") + w = _mock("unified", **{"context.query_unified": ({"chunks": [], "graph": []}, None)}) with _patch(w): result = runner.invoke(app, argv) - assert result.exit_code != 0, argv - assert "unified" in result.output + assert result.exit_code == 0, (argv, result.output) w.context.query.assert_not_called() + w.context.query_unified.assert_called_once() + + def test_an_explicit_kind_is_still_refused_on_a_unified_database(self): + _auth() + w = _mock("unified") + with _patch(w): + result = runner.invoke(app, ["query", "x", "--kind", "memory"]) + assert result.exit_code != 0 and "Re-run without --kind" in _plain(result.output) + w.context.query_unified.assert_not_called() def test_llm_and_follow_flags_are_refused_on_a_split_database(self): _auth() @@ -809,7 +820,7 @@ def test_text_posts_one_context_item_with_every_field(self): def test_conversation_file(self, tmp_path): _auth() turns = [ - {"role": "user", "content": "Keep answers short please", "name": "soham"}, + {"role": "user", "content": "Keep answers short please"}, {"role": "assistant", "content": "Got it."}, {"role": "system", "content": "Never store account numbers"}, ] @@ -817,10 +828,20 @@ def test_conversation_file(self, tmp_path): f.write_text(json.dumps(turns)) w = _mock("unified", **{"context.ingest_context": INGEST_202}) with _patch(w): - result = runner.invoke(app, ["ingest", "--conversation-file", str(f), "--context-id", "chat-w1"]) + result = runner.invoke( + app, ["ingest", "--conversation-file", str(f), "--context-id", "chat-w1", "--user-name", "soham"] + ) assert result.exit_code == 0, result.output item = w.context.ingest_context.call_args.args[0][0] - assert item == {"context_id": "chat-w1", "conversation": turns, "enrich": True, "upsert": True} + # The speaker is the item's user_name (hydradb-application#1653): a + # turn is exactly {role, content}. + assert item == { + "context_id": "chat-w1", + "conversation": turns, + "user_name": "soham", + "enrich": True, + "upsert": True, + } assert "conversation, 3 turn(s)" in _plain(result.output) def test_defaults_are_explicit_and_nothing_else_is_sent(self): @@ -913,9 +934,9 @@ def test_files_are_refused(self, tmp_path): w.context.ingest_many.assert_not_called() w.context.ingest_context.assert_not_called() - def test_kind_user_name_and_markdown_are_refused(self): + def test_kind_and_markdown_are_refused(self): _auth() - for extra in (["--kind", "memory"], ["--kind", "knowledge"], ["--user-name", "ada"], ["--markdown"]): + for extra in (["--kind", "memory"], ["--kind", "knowledge"], ["--markdown"]): w = _mock("unified") with _patch(w): result = runner.invoke(app, ["ingest", "--text", "x", *extra]) @@ -941,7 +962,7 @@ def test_a_bad_conversation_is_named_by_turn(self, tmp_path): ([{"role": "bot", "content": "x"}], "conversation[0].role"), ([{"role": "user", "content": "x"}, {"role": "user", "content": ""}], "conversation[1].content"), ([{"role": "user", "content": "x", "extra": 1}], "unknown field"), - ([{"role": "user", "content": "x", "name": ""}], "conversation[0].name"), + ([{"role": "user", "content": "x", "name": "soham"}], "conversation[0] has a name"), (["not an object"], "conversation[0] must be an object"), ([], "non-empty JSON list"), ({"role": "user"}, "non-empty JSON list"), @@ -1011,7 +1032,7 @@ def test_forceful_relation_ids_are_trimmed_and_deduplicated(self): ], ) assert result.exit_code == 0, result.output - assert w.context.ingest_context.call_args.args[0][0]["forceful_relations"] == {"ids": ["a", "b"]} + assert w.context.ingest_context.call_args.args[0][0]["forceful_relations"] == {"context_ids": ["a", "b"]} w = _mock("unified") with _patch(w): result = runner.invoke(app, ["ingest", "--text", "x", "--forceful-relation", " "]) @@ -1178,13 +1199,15 @@ def test_inspect_sends_no_kind(self): kwargs = w.context.inspect.call_args.kwargs assert "kind" not in kwargs and "type" not in kwargs - def test_deprecated_memories_list_is_refused_on_a_unified_database(self): + def test_deprecated_memories_list_lists_the_one_corpus_on_a_unified_database(self): _auth() - w = _mock("unified") + w = _mock("unified", **{"context.list": {"sources": [{"id": "a", "title": "A", "type": "memory"}]}}) with _patch(w): result = runner.invoke(app, ["memories", "list"]) - assert result.exit_code != 0 and "unified" in result.output - w.context.list.assert_not_called() + assert result.exit_code == 0, result.output + assert w.context.list.call_args.kwargs["kind"] is None + # One corpus: no per-item kind column that would only mislead. + assert "Type" not in result.output and "│ memory" not in result.output # ── hydradb database create --type / list ──────────────────────────────────── @@ -1266,3 +1289,199 @@ def test_list_without_details_shows_split(self): result = runner.invoke(app, ["database", "list"], env=_WIDE) assert result.exit_code == 0, result.output assert any(" a " in line and "split" in line for line in _lines(result)) + + +# ── PRO-2196: probe fallback, strict item contract, caps ───────────────────── + +_REFUSED_AS_UNIFIED = HydraDBClientError( + 400, + '{"success": false, "error": {"code": "CORPUS_TYPE_UNSUPPORTED", ' + '"message": "type \\"memory\\" is not valid on a unified database"}}', +) + + +def _unknown(**returns): + """A mocked wrapper whose layout probe fails the way a busy server does.""" + w = _mock("split", **returns) + w.databases.layout.side_effect = HydraDBClientError(503, "unavailable") + return w + + +class TestLayoutProbeFallback: + def test_the_probe_has_a_short_budget_and_no_retries(self): + seen = [] + + def handler(request): + seen.append(request.url.path) + return httpx.Response(200, json=_databases_envelope([{"database": "a", "type": "unified"}])) + + w = _real_wrapper(handler) + original = w._sdk.databases.list + calls = [] + + def spy(**kwargs): + calls.append(kwargs) + return original(**kwargs) + + w._sdk.databases.list = spy + assert w.databases.layout("a") == "unified" + assert calls == [{"request_options": {"timeout_in_seconds": 5, "max_retries": 0}}] + + def test_a_transient_probe_failure_sends_the_split_request_with_a_warning(self): + _auth() + w = _unknown(**{"context.query": {"chunks": []}}) + with _patch(w): + result = runner.invoke(app, ["query", "x"]) + assert result.exit_code == 0, result.output + w.context.query.assert_called_once() + w.context.query_unified.assert_not_called() + assert "Could not check whether 't1' is split or unified (HTTP 503)" in _plain(result.stderr) + + def test_an_auth_failure_on_the_probe_is_still_reported(self): + _auth() + w = _mock("split") + w.databases.layout.side_effect = HydraDBClientError(401, "bad key") + with _patch(w): + result = runner.invoke(app, ["query", "x"]) + assert result.exit_code != 0 and "Authentication failed" in result.output + w.context.query.assert_not_called() + + def test_an_ingest_refused_as_unified_is_redone_as_one_context_item(self): + _auth() + w = _unknown(**{"context.ingest_context": INGEST_202}) + w.context.ingest.side_effect = _REFUSED_AS_UNIFIED + with _patch(w): + result = runner.invoke(app, ["ingest", "--text", "a note", "--user-name", "ada", "--source-id", "n1"]) + assert result.exit_code == 0, result.output + assert w.context.ingest.call_args.kwargs["kind"] == "memory" + item = w.context.ingest_context.call_args.args[0][0] + assert item == {"context_id": "n1", "text": "a note", "user_name": "ada", "enrich": True, "upsert": True} + assert "redoing the request in the unified shape" in _plain(result.stderr) + + def test_the_sdk_paths_message_only_refusal_is_recognised(self): + # The SDK path keeps the server's message and drops its code. + _auth() + w = _unknown(**{"context.ingest_context": INGEST_202}) + w.context.ingest.side_effect = HydraDBClientError( + 400, 'type "memory" is not valid on a unified database: knowledge and memory are one corpus' + ) + with _patch(w): + result = runner.invoke(app, ["ingest", "--text", "a note"]) + assert result.exit_code == 0, result.output + w.context.ingest_context.assert_called_once() + + def test_a_unified_body_the_sdk_cannot_parse_is_redone_as_a_unified_query(self): + _auth() + w = _unknown(**{"context.query_unified": ({"chunks": [], "llm_prompt": "# Query results"}, "req-1")}) + w.context.query.side_effect = HydraDBClientError(200, "{'chunks': [], 'llm_prompt': '# Query results'}") + with _patch(w): + result = runner.invoke(app, ["query", "x"]) + assert result.exit_code == 0, result.output + w.context.query_unified.assert_called_once() + + def test_an_explicit_kind_refused_as_unified_is_explained_not_redone(self): + _auth() + w = _unknown() + w.context.ingest.side_effect = _REFUSED_AS_UNIFIED + with _patch(w): + result = runner.invoke(app, ["ingest", "--text", "a note", "--kind", "knowledge"]) + assert result.exit_code != 0 + assert "--kind does not apply" in _plain(result.output) + w.context.ingest_context.assert_not_called() + + def test_a_deprecated_write_alias_refused_as_unified_points_at_ingest(self): + _auth() + w = _unknown() + w.context.ingest.side_effect = _REFUSED_AS_UNIFIED + with _patch(w): + result = runner.invoke(app, ["memories", "add", "--text", "a note"]) + assert result.exit_code != 0 + assert "hydradb ingest --text" in _plain(result.output) + w.context.ingest_context.assert_not_called() + + def test_a_delete_refused_as_unified_is_redone_without_a_kind(self): + _auth() + w = _unknown() + w.context.delete.side_effect = [_REFUSED_AS_UNIFIED, {"success": True, "deleted_count": 1}] + with _patch(w): + result = runner.invoke(app, ["delete", "a1", "--yes"]) + assert result.exit_code == 0, result.output + kinds = [c.kwargs["kind"] for c in w.context.delete.call_args_list] + assert kinds == ["knowledge", None] + assert "Deleted 1 item(s)" in _plain(result.output) + + def test_unified_only_flags_need_a_known_layout(self): + _auth() + w = _unknown() + with _patch(w): + result = runner.invoke(app, ["ingest", "--text", "a note", "--context-id", "n1"]) + assert result.exit_code != 0 + assert "could not be checked just now" in _plain(result.output) + w.context.ingest.assert_not_called() + w.context.ingest_context.assert_not_called() + + def test_llm_with_an_unknown_layout_goes_out_unified(self): + _auth() + w = _unknown(**{"context.query_unified": ({"chunks": [], "llm_prompt": "# Query results"}, None)}) + with _patch(w): + result = runner.invoke(app, ["query", "x", "--llm"]) + assert result.exit_code == 0, result.output + w.context.query.assert_not_called() + assert "# Query results" in result.stdout + + +class TestStrictItemContract: + def test_user_name_is_sent_on_a_text_item(self): + _auth() + w = _mock("unified", **{"context.ingest_context": INGEST_202}) + with _patch(w): + result = runner.invoke(app, ["ingest", "--text", "I prefer short answers", "--user-name", "Harsh"]) + assert result.exit_code == 0, result.output + assert w.context.ingest_context.call_args.args[0][0]["user_name"] == "Harsh" + + def test_the_202_id_is_read_from_id(self): + _auth() + body = {"success": True, "success_count": 1, "failed_count": 0, "results": [{"id": "n1", "status": "queued"}]} + w = _mock("unified", **{"context.ingest_context": body}) + with _patch(w): + result = runner.invoke(app, ["ingest", "--text", "a note"]) + assert result.exit_code == 0, result.output + assert "Context ID: n1 (queued)" in _plain(result.output) + + @pytest.mark.parametrize( + ("extra", "message"), + [ + (["--context-id", "a,b"], "no commas"), + (["--context-id", "x" * 101], "at most 100 characters"), + (["--title", "é" * 513], "at most 1024 bytes"), + (["--instructions", "x" * 4001], "at most 4000 characters"), + ], + ) + def test_the_server_caps_are_checked_before_sending(self, extra, message): + _auth() + w = _mock("unified") + with _patch(w): + result = runner.invoke(app, ["ingest", "--text", "a note", *extra]) + assert result.exit_code != 0 + assert message in _plain(result.output) + w.context.ingest_context.assert_not_called() + + def test_text_over_one_mebibyte_is_refused(self): + _auth() + w = _mock("unified") + with _patch(w): + result = runner.invoke(app, ["ingest", "--text", "x" * ((1 << 20) + 1)]) + assert result.exit_code != 0 + assert "Split it into several items" in _plain(result.output) + w.context.ingest_context.assert_not_called() + + +class TestDatabaseCreateDefault: + def test_create_without_type_says_the_server_picks(self): + _auth() + w = _mock("split", **{"databases.create": {"status": "accepted"}}) + with _patch(w): + result = runner.invoke(app, ["database", "create", "new-db"]) + assert result.exit_code == 0, result.output + assert w.databases.create.call_args.kwargs["layout"] is None + assert "the server's default layout" in _plain(result.output) From 7804e905f87343ab376b6d8e30e70f20aa0a6022 Mon Sep 17 00:00:00 2001 From: Harsh Ranjan Date: Wed, 23 Sep 2026 19:15:30 +0000 Subject: [PATCH 2/3] refactor(unified): typed SDK calls for the unified query and database create (PRO-2196) hydradb-sdk 2.1.6 knows the unified contract: /query without `type`, the four-key SearchQueryResult answer, follow_forceful_relations, and `type` on database create. The unified query and `database create --type` now go through it instead of the hand-built raw path, and the pin moves to >=2.1.6. The unified answer is dumped with exclude_unset, so `--output json` stays the server's body verbatim (checked live against staging). The unified ingest stays a JSON body on the raw path: the shared `ingest-unified-json` conformance vector requires one, and the SDK sends `context` only as a multipart form field. With 2.1.6 a type-less query that reaches a unified database while the layout probe is down now parses on the first request, so no redo is needed. Signed-off-by: Harsh Ranjan --- CHANGELOG.md | 2 +- pyproject.toml | 2 +- src/hydradb_cli/hydra/client.py | 98 +++++++++------------- tests/test_unified.py | 140 +++++++++++++------------------- 4 files changed, 95 insertions(+), 147 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf8a5b6..c24afca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ `ingest` on a unified database is a JSON `POST /context/ingest` with the `context` list and one item of exactly one `--text` or `--conversation-file` (a JSON list of `{role, content}` turns), plus `--context-id`, `--title`, `--user-name` (the item's `user_name`), `--enrich/--no-enrich`, `--instructions`, `--happened-at` (YYYY-MM-DD), `--attributes` and `--custom-attributes` (JSON objects), `--category`, repeatable `--forceful-relation` (sent as `forceful_relations.context_ids`) and `--acl`, and `--upsert/--no-upsert`. Every value is validated locally and named by turn or flag before a round trip, including the server's per-item caps (1 MiB of text, a 1024-byte title, 4000 characters of instructions, a context id of at most 100 characters with no commas). A turn carrying `name` is refused: the server takes a turn as exactly `{role, content}`. Files are refused on a unified database with a message pointing at `--text`; `--kind` and `--markdown` are refused there too, and the unified-only options are refused on a split database. The 202's `results[].id` is rendered as the item's context id (`source_id` is still read from older servers). - `list`, `delete`, `relations`, `subgraph` and `inspect` send no `type` on a unified database; an explicit `--kind` is refused there rather than silently dropped, and `delete` keeps its `knowledge` default on a split database. The deprecated aliases that write with a hard-coded kind (`memories add`, `knowledge upload`, ...) are refused on a unified database with a message naming `hydradb ingest`; the read and delete aliases (`recall full`, `recall preferences`, `memories list`, `memories delete`, `knowledge delete`) chose that kind themselves, so on a unified database they drop it and act on the one corpus. `list` shows no per-item type column on a unified database. The pinned SDK cannot be relied on for any of this, so the unified calls (and `database create --type`) go over the wrapper's raw v2 path with the same headers, envelope unwrap and error translation as the SDK path. The conformance vectors gain `ingest-unified-json`. + `list`, `delete`, `relations`, `subgraph` and `inspect` send no `type` on a unified database; an explicit `--kind` is refused there rather than silently dropped, and `delete` keeps its `knowledge` default on a split database. The deprecated aliases that write with a hard-coded kind (`memories add`, `knowledge upload`, ...) are refused on a unified database with a message naming `hydradb ingest`; the read and delete aliases (`recall full`, `recall preferences`, `memories list`, `memories delete`, `knowledge delete`) chose that kind themselves, so on a unified database they drop it and act on the one corpus. `list` shows no per-item type column on a unified database. The unified query and `database create --type` are typed SDK calls; this needs `hydradb-sdk` 2.1.6, which knows the unified request and the four-key answer, so the pin moves from `>=2.1.4` to `>=2.1.6`. The unified ingest stays a JSON body on the wrapper's raw v2 path (same headers, envelope unwrap and error translation as the SDK path), because the shared `ingest-unified-json` conformance vector requires a JSON body and the SDK sends `context` only as a multipart form field. The conformance vectors gain `ingest-unified-json`. - **`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. diff --git a/pyproject.toml b/pyproject.toml index f855c8f..b6d4152 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -36,7 +36,7 @@ dependencies = [ # OpenAPI summary text and its CI auto-bumps the patch digit on merge, so a # breaking rename can arrive as 2.1.2 -> 2.1.3. The wrapper is the firewall; # the exact pin is what keeps that churn from reaching users (CONTRACT §2). - "hydradb-sdk>=2.1.4,<3", + "hydradb-sdk>=2.1.6,<3", ] [project.scripts] diff --git a/src/hydradb_cli/hydra/client.py b/src/hydradb_cli/hydra/client.py index 2c73666..3b48270 100644 --- a/src/hydradb_cli/hydra/client.py +++ b/src/hydradb_cli/hydra/client.py @@ -166,28 +166,19 @@ def create( """Create a database. ``layout`` is the storage layout (PRO-1618), sent as the wire field - ``type``: ``split`` (the default, and what every pre-existing database - is) or ``unified`` (one corpus; ``type`` is never sent on later calls). - A layout goes over the raw v2 path so the request does not depend on - which pinned SDK build knows the value; without one this is the - unchanged SDK call. + ``type``: ``split`` or ``unified`` (one corpus; ``type`` is never sent + on later calls). Without one the server picks, and current servers + pick unified. """ - if layout is not None: - if layout not in (LAYOUT_SPLIT, LAYOUT_UNIFIED): - raise ValueError(f"layout must be '{LAYOUT_SPLIT}' or '{LAYOUT_UNIFIED}', got {layout!r}") - body: dict[str, Any] = {"database": database, "type": layout} - if embeddings_dimension is not None: - body["embeddings_dimension"] = embeddings_dimension - if database_metadata_schema is not None: - body["database_metadata_schema"] = database_metadata_schema - result = self._w._raw_post("/databases", json_body=body) - return result if isinstance(result, dict) else {} + if layout is not None and layout not in (LAYOUT_SPLIT, LAYOUT_UNIFIED): + raise ValueError(f"layout must be '{LAYOUT_SPLIT}' or '{LAYOUT_UNIFIED}', got {layout!r}") resp = self._invoke( self._w._sdk.databases.create, database=database, embeddings_dimension=embeddings_dimension, is_embeddings_tenant=is_embeddings_tenant, database_metadata_schema=database_metadata_schema, + type=layout, ) return _unwrap(resp) @@ -399,41 +390,37 @@ def query_unified( """``POST /query`` against a UNIFIED database (PRO-1618). Never sends ``type``: a unified database has one corpus, and the server - refuses ``knowledge``/``memory`` there. It goes over the raw v2 path - rather than the SDK because the pinned SDK spells the forceful-relations - switch by its deprecated alias and cannot be relied on to omit ``type``. + refuses ``knowledge``/``memory`` there. The SDK (2.1.6+) reads the + answer as its four-key ``SearchQueryResult``. Returns ``(body, request_id)``. ``body`` is the four-key response - (``chunks``, ``graph``, ``forceful_relations``, ``llm_prompt``) exactly - as the server sent it, with nothing added, so ``--output json`` prints - it verbatim; ``request_id`` is lifted from the envelope's ``meta`` for - ``hydradb feedback``, which is the one thing the body cannot carry. - ``request_id`` is the ONLY key read from that ``meta``: a unified - response's meta has no ``tenant_id``, ``sub_tenant_id`` or - ``source_type``. + (``chunks``, ``graph``, ``forceful_relations``, ``llm_prompt``) as the + server sent it: dumped with ``exclude_unset`` so no key the server did + not send is added, and ``--output json`` prints it verbatim. + ``request_id`` is lifted from the envelope's ``meta`` for ``hydradb + feedback``, the one thing the body cannot carry. """ - body = { - key: value - for key, value in { - "database": self._w._require_database(database), - "collection": self._w._resolve_collection(collection), - "query": query, - "operator": operator, - "max_results": max_results, - "mode": mode, - "alpha": alpha, - "recency_bias": recency_bias, - "graph_context": graph_context, - "additional_context": additional_context, - "query_by": query_by, - "titles": titles, - "acl": acl, - "follow_forceful_relations": follow_forceful_relations, - }.items() - if value is not None - } - data, meta = self._w._raw_post_with_meta("/query", json_body=body) - return (data if isinstance(data, dict) else {}), _request_id_of({"meta": meta}) + resp = self._invoke( + self._w._sdk.query, + database=self._w._require_database(database), + collection=self._w._resolve_collection(collection), + query=query, + operator=operator, + max_results=max_results, + mode=mode, + alpha=alpha, + recency_bias=recency_bias, + graph_context=graph_context, + additional_context=additional_context, + query_by=query_by, + titles=titles, + acl=acl, + follow_forceful_relations=follow_forceful_relations, + ) + data = getattr(resp, "data", None) + dump = getattr(data, "model_dump", None) + body = dump(mode="json", by_alias=True, exclude_unset=True) if callable(dump) else _unwrap(resp) + return (body if isinstance(body, dict) else {}), _request_id_of(resp) def ingest( self, @@ -530,6 +517,9 @@ def ingest_context( for index, item in enumerate(items): if not isinstance(item, dict) or ("text" in item) == ("conversation" in item): raise ValueError(f"context[{index}] must carry exactly one of 'text' or 'conversation'") + # A JSON body, as the shared conformance vector `ingest-unified-json` + # requires. The SDK (2.1.6) can only send `context` as a multipart form + # field, so this one call stays on the raw path. body: dict[str, Any] = {"database": self._w._require_database(database)} coll = self._w._resolve_collection(collection) if coll: @@ -1208,17 +1198,6 @@ def _raw_post(self, path: str, *, json_body: Any) -> Any: ``API-Version: 2`` headers, same shape-based unwrapping, same translated error type, so a caller cannot tell it from an SDK call. """ - return self._raw_post_with_meta(path, json_body=json_body)[0] - - def _raw_post_with_meta(self, path: str, *, json_body: Any) -> tuple[Any, dict]: - """:meth:`_raw_post`, also returning the envelope's ``meta``. - - ``_unwrap_payload`` keeps ``data`` and drops ``meta``, which is where - ``request_id`` lives. A unified ``/query`` must hand back ``data`` - untouched (the four-key body, printed verbatim) AND surface the request - id for ``hydradb feedback``, so this variant returns both. ``meta`` is - ``{}`` when the response was not an envelope. - """ headers = { "Authorization": f"Bearer {self._token}", "API-Version": "2", @@ -1241,8 +1220,7 @@ def _raw_post_with_meta(self, path: str, *, json_body: Any) -> tuple[Any, dict]: if response.is_error: raise HydraDBClientError(response.status_code, _stringify_body(body)) - meta = body.get("meta") if isinstance(body, dict) else None - return _unwrap_payload(body), (meta if isinstance(meta, dict) else {}) + return _unwrap_payload(body) def _require_database(self, database: str | None) -> str: db = database or self.default_database diff --git a/tests/test_unified.py b/tests/test_unified.py index 8078e3b..3f23d84 100644 --- a/tests/test_unified.py +++ b/tests/test_unified.py @@ -153,6 +153,19 @@ def _sdk_500(request): return httpx.Response(500, json={"success": False, "error": {"message": "the SDK path must not be used"}}) +def _sdk_server(routes: dict, seen: list): + """An SDK transport answering ``routes[path] -> (status, json)`` and + recording every request as ``(method, path, json body)``.""" + + def handler(request): + body = json.loads(request.content) if request.content else None + seen.append((request.method, request.url.path, body)) + status, payload = routes[request.url.path] + return httpx.Response(status, json=payload) + + return handler + + class _Capture: """Stands in for ``httpx.post`` on the raw path and records every call.""" @@ -172,43 +185,6 @@ def _capture_post(monkeypatch, status: int = 200, body: dict | None = None) -> _ return capture -class _WatchedMeta(dict): - """An envelope ``meta`` that records every key read from it (``*`` for a - read of the whole mapping), so a test can pin what the unified path reads.""" - - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - self.read: list[str] = [] - - def __getitem__(self, key): - self.read.append(key) - return super().__getitem__(key) - - def get(self, key, default=None): - self.read.append(key) - return super().get(key, default) - - def __contains__(self, key): - self.read.append(key) - return super().__contains__(key) - - def __iter__(self): - self.read.append("*") - return super().__iter__() - - def keys(self): - self.read.append("*") - return super().keys() - - def items(self): - self.read.append("*") - return super().items() - - def values(self): - self.read.append("*") - return super().values() - - def _path(origin: str | None, chunk_id: str, source: str, target: str, summary: str) -> dict: """One ``graph[]`` path of a single hop extracted from ``chunk_id``.""" path: dict = { @@ -301,11 +277,10 @@ def test_database_layout_resolves_the_scope_first(self): class TestUnifiedQueryWrapper: - def test_posts_no_type_and_returns_the_body_verbatim_with_the_request_id(self, monkeypatch): - capture = _capture_post( - monkeypatch, body={"success": True, "data": UNIFIED_BODY, "meta": {"request_id": "req-1", "latency_ms": 12}} - ) - w = _real_wrapper(_sdk_500) + def test_sends_no_type_through_the_sdk_and_returns_the_body_verbatim(self): + seen = [] + envelope = {"success": True, "data": UNIFIED_BODY, "meta": {"request_id": "req-1", "latency_ms": 12}} + w = _real_wrapper(_sdk_server({"/query": (200, envelope)}, seen)) body, request_id = w.context.query_unified( query="pro plan", operator="and", @@ -318,10 +293,9 @@ def test_posts_no_type_and_returns_the_body_verbatim_with_the_request_id(self, m ) assert body == UNIFIED_BODY, "the four keys, nothing added, nothing dropped" assert request_id == "req-1" - call = capture.calls[0] - assert call["url"] == "http://test.local/query" - assert call["headers"]["API-Version"] == "2" - assert call["json"] == { + method, path, sent = seen[0] + assert (method, path) == ("POST", "/query") + assert sent == { "database": "db_test", "collection": "col_test", "query": "pro plan", @@ -333,19 +307,20 @@ def test_posts_no_type_and_returns_the_body_verbatim_with_the_request_id(self, m "acl": ["a@x.com"], "follow_forceful_relations": False, } - assert "type" not in call["json"] - def test_unset_fields_are_omitted_not_sent_as_null(self, monkeypatch): - capture = _capture_post(monkeypatch, body={"success": True, "data": EMPTY_BODY, "meta": {}}) - body, request_id = _real_wrapper(_sdk_500).context.query_unified(query="q") - assert capture.calls[0]["json"] == {"database": "db_test", "collection": "col_test", "query": "q"} + def test_unset_fields_are_omitted_not_sent_as_null(self): + seen = [] + w = _real_wrapper(_sdk_server({"/query": (200, {"success": True, "data": EMPTY_BODY, "meta": {}})}, seen)) + body, request_id = w.context.query_unified(query="q") + assert seen[0][2] == {"database": "db_test", "collection": "col_test", "query": "q"} assert body == EMPTY_BODY assert request_id is None - def test_a_refusal_is_a_client_error(self, monkeypatch): - _capture_post(monkeypatch, status=400, body={"success": False, "error": {"message": "knowledge is not valid"}}) + def test_a_refusal_is_a_client_error(self): + refusal = {"success": False, "error": {"message": "knowledge is not valid"}} + w = _real_wrapper(_sdk_server({"/query": (400, refusal)}, [])) with pytest.raises(HydraDBClientError) as excinfo: - _real_wrapper(_sdk_500).context.query_unified(query="q") + w.context.query_unified(query="q") assert excinfo.value.status_code == 400 assert "knowledge is not valid" in str(excinfo.value.detail) @@ -576,55 +551,48 @@ def test_split_query_call_is_unchanged(self): assert "follow_forceful_relations" not in kwargs and "llm" not in kwargs w.context.query_unified.assert_not_called() - def test_only_the_request_id_is_read_from_the_unified_meta(self, monkeypatch): + def test_the_request_id_reaches_every_output_mode(self, monkeypatch): """A unified /query ``meta`` has no ``tenant_id``, ``sub_tenant_id`` or - ``source_type`` (PRO-1618): nothing on the unified path may read them, - and ``request_id`` is the one key it needs, in every output mode.""" + ``source_type`` (PRO-1618); ``request_id`` is the one key the unified + path needs, in every output mode.""" monkeypatch.setenv("HYDRADB_API_KEY", "x") monkeypatch.setenv("HYDRADB_DATABASE", "db_test") - meta = _WatchedMeta( - {"request_id": "req-7", "api_version": "2", "latency_ms": 9, "database": "db_test", "collection": "c"} - ) - wrapper = _real_wrapper( - lambda r: httpx.Response(200, json=_databases_envelope([{"database": "db_test", "type": "unified"}])) - ) - monkeypatch.setattr(wrapper, "_raw_post_with_meta", lambda path, *, json_body: (UNIFIED_BODY, meta)) + meta = {"request_id": "req-7", "api_version": "2", "latency_ms": 9, "database": "db_test", "collection": "c"} + routes = { + "/databases": (200, _databases_envelope([{"database": "db_test", "type": "unified"}])), + "/query": (200, {"success": True, "data": UNIFIED_BODY, "meta": meta}), + } outputs = {} for mode, argv in ( ("human", ["query", "pro plan"]), ("llm", ["query", "pro plan", "--llm"]), ("json", ["--output", "json", "query", "pro plan"]), ): + wrapper = _real_wrapper(_sdk_server(routes, [])) with patch("hydradb_cli.commands._impl.get_wrapper", return_value=wrapper): result = runner.invoke(app, argv, env=_WIDE) assert result.exit_code == 0, (mode, result.output) outputs[mode] = result - assert set(meta.read) == {"request_id"}, meta.read assert "hydradb feedback req-7" in outputs["human"].output assert "req-7" in outputs["llm"].stderr assert json.loads(outputs["json"].stdout) == UNIFIED_BODY def test_end_to_end_json_is_the_server_body_verbatim(self, monkeypatch): - """Real wrapper: the probe over the SDK transport, the query over the raw path.""" + """Real wrapper and SDK: the probe, then the type-less query.""" monkeypatch.setenv("HYDRADB_API_KEY", "x") monkeypatch.setenv("HYDRADB_DATABASE", "db_test") monkeypatch.setenv("HYDRADB_COLLECTION", "col_test") - probes = [] - - def sdk_handler(request): - probes.append(request.url.path) - return httpx.Response(200, json=_databases_envelope([{"database": "db_test", "type": "unified"}])) - - capture = _capture_post( - monkeypatch, body={"success": True, "data": UNIFIED_BODY, "meta": {"request_id": "req-9"}} - ) - with patch("hydradb_cli.commands._impl.get_wrapper", return_value=_real_wrapper(sdk_handler)): + seen = [] + routes = { + "/databases": (200, _databases_envelope([{"database": "db_test", "type": "unified"}])), + "/query": (200, {"success": True, "data": UNIFIED_BODY, "meta": {"request_id": "req-9"}}), + } + with patch("hydradb_cli.commands._impl.get_wrapper", return_value=_real_wrapper(_sdk_server(routes, seen))): result = runner.invoke(app, ["--output", "json", "query", "pro plan"]) assert result.exit_code == 0, result.output assert json.loads(result.stdout) == UNIFIED_BODY - assert probes == ["/databases"] - assert capture.calls[0]["url"] == "http://test.local/query" - assert "type" not in capture.calls[0]["json"] + assert [path for _, path, _ in seen] == ["/databases", "/query"] + assert "type" not in seen[1][2] # ── the renderer ───────────────────────────────────────────────────────────── @@ -1240,12 +1208,14 @@ def test_create_rejects_an_unknown_type(self): assert "split" in result.output and "unified" in result.output w.databases.create.assert_not_called() - def test_wrapper_create_with_a_layout_posts_type_over_the_raw_path(self, monkeypatch): - capture = _capture_post(monkeypatch, body={"success": True, "data": {"status": "accepted"}, "meta": {}}) - out = _real_wrapper(_sdk_500).databases.create(database="new", layout="unified") - assert out == {"status": "accepted"} - assert capture.calls[0]["url"] == "http://test.local/databases" - assert capture.calls[0]["json"] == {"database": "new", "type": "unified"} + def test_wrapper_create_with_a_layout_sends_type_through_the_sdk(self): + seen = [] + accepted = {"success": True, "data": {"status": "accepted"}, "meta": {}} + out = _real_wrapper(_sdk_server({"/databases": (200, accepted)}, seen)).databases.create( + database="new", layout="unified" + ) + assert out.get("status") == "accepted" + assert seen == [("POST", "/databases", {"database": "new", "type": "unified"})] def test_wrapper_create_without_a_layout_is_the_sdk_call(self): seen = {} From 8d0694ababa6a5e2854f6e3f04c594c18cc5142d Mon Sep 17 00:00:00 2001 From: Harsh Ranjan Date: Wed, 23 Sep 2026 20:15:00 +0000 Subject: [PATCH 3/3] fix(unified): use a 2xx query answer the SDK model rejects as the server sent it (PRO-2196) hydradb-sdk 2.1.6's four-key SearchQueryResult requires every list key, but the contract lets `forceful_relations` be absent. Such an answer raised a ParsingError and the CLI printed an error for a successful query; the hand-built path it replaced accepted it. The parsed body travels on the ParsingError, so a 2xx answer is now returned as sent (one request, the request id kept) and anything else is still translated to the usual error. Found by the staging pass: a proxied /query with forceful_relations dropped. Signed-off-by: Harsh Ranjan --- src/hydradb_cli/hydra/client.py | 46 +++++++++++++++++++++------------ tests/test_unified.py | 19 ++++++++++++++ 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/src/hydradb_cli/hydra/client.py b/src/hydradb_cli/hydra/client.py index 3b48270..0f3e4b6 100644 --- a/src/hydradb_cli/hydra/client.py +++ b/src/hydradb_cli/hydra/client.py @@ -400,23 +400,35 @@ def query_unified( ``request_id`` is lifted from the envelope's ``meta`` for ``hydradb feedback``, the one thing the body cannot carry. """ - resp = self._invoke( - self._w._sdk.query, - database=self._w._require_database(database), - collection=self._w._resolve_collection(collection), - query=query, - operator=operator, - max_results=max_results, - mode=mode, - alpha=alpha, - recency_bias=recency_bias, - graph_context=graph_context, - additional_context=additional_context, - query_by=query_by, - titles=titles, - acl=acl, - follow_forceful_relations=follow_forceful_relations, - ) + kwargs = { + "database": self._w._require_database(database), + "collection": self._w._resolve_collection(collection), + "query": query, + "operator": operator, + "max_results": max_results, + "mode": mode, + "alpha": alpha, + "recency_bias": recency_bias, + "graph_context": graph_context, + "additional_context": additional_context, + "query_by": query_by, + "titles": titles, + "acl": acl, + "follow_forceful_relations": follow_forceful_relations, + } + try: + resp = self._w._sdk.query(**{k: v for k, v in kwargs.items() if v is not None}) + except ParsingError as exc: + # A successful answer the SDK's model rejects (it requires every + # list key, and the contract lets `forceful_relations` be absent) + # is still the server's answer: use it as sent, no second request. + envelope = exc.body if isinstance(exc.body, dict) else None + body = envelope.get("data") if envelope else None + if exc.status_code and 200 <= exc.status_code < 300 and isinstance(body, dict): + return body, _request_id_of(envelope) + raise translate_sdk_error(exc) from exc + except (ApiError, httpx.HTTPError) as exc: + raise translate_sdk_error(exc) from exc data = getattr(resp, "data", None) dump = getattr(data, "model_dump", None) body = dump(mode="json", by_alias=True, exclude_unset=True) if callable(dump) else _unwrap(resp) diff --git a/tests/test_unified.py b/tests/test_unified.py index 3f23d84..0490a7f 100644 --- a/tests/test_unified.py +++ b/tests/test_unified.py @@ -316,6 +316,25 @@ def test_unset_fields_are_omitted_not_sent_as_null(self): assert body == EMPTY_BODY assert request_id is None + @pytest.mark.parametrize( + "body", + [ + {k: v for k, v in UNIFIED_BODY.items() if k != "forceful_relations"}, + {**UNIFIED_BODY, "graph": None, "forceful_relations": None}, + ], + ids=["forceful_relations absent", "lists null"], + ) + def test_an_answer_the_sdk_model_rejects_is_used_as_sent(self, body): + # The SDK's four-key model requires every list; the contract lets + # forceful_relations be absent. One request, the server's body as sent. + seen = [] + envelope = {"success": True, "data": body, "meta": {"request_id": "req-3"}} + w = _real_wrapper(_sdk_server({"/query": (200, envelope)}, seen)) + out, request_id = w.context.query_unified(query="q") + assert out == body + assert request_id == "req-3" + assert len(seen) == 1 + def test_a_refusal_is_a_client_error(self): refusal = {"success": False, "error": {"message": "knowledge is not valid"}} w = _real_wrapper(_sdk_server({"/query": (400, refusal)}, []))