feat: widen the provisioning tools, and mark the ones that can destroy data by rohan-hotdata · Pull Request #95 · hotdata-dev/hotdata-langchain

rohan-hotdata · 2026-09-01T12:26:49Z

Closes #91.

The provisioning tool signatures were narrower than the framework calls beneath them, so capabilities that already existed were unreachable from an agent. Three changes, plus a dependency floor that moved as a consequence of one of them.

What an agent can now do

keys and expires_at on the create tool. A key can only be declared at creation. A table made through the tool was keyless for the rest of its life, which meant upsert, update and delete were rejected against it and an agent could never make a re-run idempotent. expires_at takes an RFC 3339 timestamp or a relative window like "24h", which turns lifetime into a property of the database rather than a cleanup script's problem.

mode and key on the load tool. The load hardcoded replace. A keyed mode called without key now raises before the upload rather than being rejected by the engine at the far end of a transfer that had already happened.

DESTRUCTIVE_TOOL_NAMES, and metadata={"destructive": True} on the tool it names. HumanInTheLoopMiddleware(interrupt_on=...) is keyed by tool name, so wiring approval previously meant inferring the mutating set from naming. The constant holds the default names; a set built with tool_name_suffix carries different ones, and matching against the constant would silently find nothing, so the README shows filtering on the metadata instead and a test pins that a suffixed set does not intersect it.

Two design calls worth review

Only the load tool is marked destructive. Creating a database makes something new rather than overwriting something existing, and gating it would put an approval in front of the one call an agent has to make before it can do anything at all. The plan doc left this open; this is the answer, easy to change.

partition_by and sorted_by reach hl.create_managed_database but are not offered to a model. Layout is permanent — the API has no ALTER path, and undoing a choice means deleting the table and reloading it, which burns the table name in that database. A model has no basis for choosing a partition transform and cannot undo a wrong one. This follows the boundary #61 settled: the model chooses when, the runtime supplies what.

Why the floor moved to `hotdata-framework>=0.13.0`

Not housekeeping. 0.13.0 is where an append load became retryable. Before it, append was excluded from retries on idempotency grounds — the string `append` stays non-retryable is in 0.12.1's source and gone in 0.13.0. The destination serialises writes per table and refuses rather than queues, so concurrent writers get 409 RESOURCE_LOCKED, and an append had no budget to wait it out whatever max_retries was set to.

This PR is what makes append reachable from a tool, so shipping it against a floor three versions below the release that fixes its failure mode did not seem defensible.

The bump is not covering a break. The suite passes on 0.10.0, 0.11.0, 0.12.0, 0.12.1 and 0.13.0 — verified by running it against each, not by reading release notes. Accepting it costs the one known consumer nothing: it declares only hotdata-langchain>=0.12.0 and never names the framework, so there is no constraint to reconcile. It will not arrive there on its own, though — that consumer's committed uv.lock pins hotdata-framework 0.10.0, and uv sync resolves from the lock rather than from the constraint, so it takes a deliberate uv lock --upgrade-package hotdata-langchain. It also supersedes Dependabot #94, which bumped the lockfile to the same version.

Raising the floor is what brought partition_by/sorted_by into range; they arrive on the framework client at 0.12.0.

What is deliberately not here

format and result_id on the load. They exist on the LoadManagedTableRequest model, which is where the issue quotes them from, but HotdataClient.load_managed_table hardcodes the fields it forwards and exposes neither — on no released version. They need an upstream change rather than a wider signature here. Recorded on the issue with the version table.

One hazard no version fixes

Retrying a failed append from an agent duplicates rows. Re-sending the same upload replays the server's receipt, but a tool call has no memory across turns: a repeat stages a fresh upload, which has no receipt to replay. handle_errors=True makes this likelier, since the failure goes back to the model rather than ending the run. Not fixable inside this package, so the load tool's description now says it and points at replace or a keyed upsert, both of which land the same rows however many times they run.

Verification

589 tests (+14), ruff and mypy clean. The suite was run at the declared floor with --resolution lowest-direct, not only against the lockfile — which is how the ceiling had drifted two minor versions away from the old floor unnoticed. The README's worked examples were executed rather than written, and HumanInTheLoopMiddleware's import path and interrupt_on signature checked against the installed langchain.

Added after review

Blocking issue, fixed. The model-facing text named upsert, update and delete together with only the shared clause that each matches incoming rows against existing ones, so a model choosing between them had no statement of any one's effect. delete reads naturally as delete-then-insert by key; it removes matched rows and inserts nothing, so that misreading destroys rows and reports success. The API's own wording is on LoadManagedTableRequest.key — the key columns decide which existing row an incoming row "removes, updates, or replaces" — and each mode now states its effect on a matched row and on one that matches nothing, in LoadMode, the tool docstring, and the tool description.

Writing that surfaced a real break at the langchain-core>=1.0 floor. 1.0.0's docstring parser treats a colon inside a wrapped Args: continuation line as the start of a new argument, so Arg one by key in docstring not found in function signature aborted every make_hotdata_tools call. Newer langchain-core parses it without complaint, which is why the locked build was green and the matrix was green.

So CI now runs the suite at the declared floor, with uv --resolution lowest-direct, alongside the matrix. uv sync --locked installs the resolved ceiling, so nothing previously exercised the versions pyproject.toml advertises — which is both how that langchain-core break would have reached a release and how the framework floor drifted three minor versions behind what the package was developed against. It runs in ~21s.

This is scope this PR did not start with, and it is easy to drop if you would rather it were its own change. I added it because this PR moves the floor, so whether the floor is tested became part of its own correctness — and because a break was already sitting behind it.

The other four review nits are also fixed: a keys entry naming an undeclared table is refused rather than silently creating a permanently keyless table; TablePartitionKey and TableSortKey are re-exported now that both appear in a public signature; the mode-schema test reads the enum rather than the prose description that satisfied it either way; and one test binds the provisioning calls against the real HotdataClient signature, which a MagicMock cannot do — that is the check that would have caught format/result_id.

594 tests, green at both the lockfile and the floor.

On Dependabot #94

Superseded — close it. It changes exactly one package version in uv.lock and touches pyproject.toml not at all, so it changes what this repo develops against and nothing about what consumers install. This PR does both.

Why the floor job is the load-bearing part

The committed lockfile is the mechanism of the drift, and it is on both sides. This repo resolves from uv.lock, and so does the one consumer. Neither ever exercises the range it advertises, which is how the framework ceiling got three minor versions ahead of the declared floor with nothing noticing, and how a langchain-core 1.0.0 break sat behind a green matrix.

The offender being langchain-core rather than hotdata-framework is the part worth generalising: this is a property of declaring a version range and never testing its lower bound, not a quirk of one dependency. The lowest-direct job is the only thing in CI that can see it.

…y data The tool signatures were narrower than the framework calls beneath them, so capabilities that already existed were unreachable from an agent. `hotdata_create_managed_database` gains `keys` and `expires_at`. A key can only be declared at creation, so a table made through the tool was keyless for its whole life and every key-matched load mode was rejected against it. `hotdata_load_managed_table` gains `mode` and `key`. The load hardcoded `replace`. A keyed mode called without a key now raises before the upload rather than being rejected by the engine after it. `DESTRUCTIVE_TOOL_NAMES` is exported and the tools it names carry `metadata={"destructive": True}`, so `interrupt_on` can be wired from the package rather than from a guess about naming. Only the load tool is in it. `partition_by` and `sorted_by` are not included: they arrive on the framework client at 0.12.0 and this package declares `>=0.10.0`. `format` and `result_id` are on no released framework version. Measured across 0.10.0, 0.11.0, 0.12.0, 0.12.1 and 0.13.0. Closes #91

…Python 0.13.0 removed the exclusion that kept an `append` load from being retried. Before it, an append that hit `409 RESOURCE_LOCKED` — which the destination returns because it serialises writes per table and refuses rather than queues — failed whatever `max_retries` was configured. The previous commit makes `append` reachable from a tool, so the floor moves with it. The suite passes on 0.10.0, 0.11.0, 0.12.0, 0.12.1 and 0.13.0, so this is about the failure modes the new load modes can hit rather than a broken build. Raising the floor also brings `partition_by` and `sorted_by` into range. Both reach `hl.create_managed_database` and neither is offered to a model. The API has no ALTER path, so undoing a layout choice means deleting the table and reloading it, which burns the table name in that database. The load tool's description now says not to repeat a failed `append`. Re-sending one upload replays the server's receipt, but a tool call has no memory across turns, so a repeat stages a fresh upload with no receipt to replay and the rows land twice. No framework version changes that.

claude · 2026-09-01T12:31:42Z

+            mode: what happens to rows already in the table. 'replace' discards them,
+                'append' keeps them, and 'upsert', 'update' and 'delete' match incoming
+                rows against existing ones by key.
+            key: the key columns to match on, required by upsert, update and delete.
+                They must be the columns the table was declared with.


State what update and delete do to a matched row. The three key-matched modes are named together, and the shared clause says only that each one matches incoming rows against existing ones. A model choosing between the three has no statement of the effect of any of them. The same wording reaches the model a second time in the tool description at tools.py:765-766, and a third time on LoadMode at databases.py:36-37.

Failure scenario: a user asks the agent to refresh a subset of orders. The model uploads the new rows and picks mode="delete", reading delete as delete-then-insert by key. The engine removes every matched row and inserts nothing. The load reports success, the row count looks plausible, and the removed rows are unrecoverable.

delete is the mode with the worst outcome and the least defined name. Give each of the three an effect:

upsert inserts new rows and overwrites matched ones.

update overwrites matched rows and ignores rows that match nothing.

delete removes matched rows and inserts nothing.

The PR already documents the append retry hazard at this level of detail. delete warrants the same treatment.

claude · 2026-09-01T12:31:49Z

+    schema = json.dumps(tools[DEFAULT_LOAD_TABLE_TOOL_NAME].args["mode"])
+    for mode in ("replace", "append", "upsert", "update", "delete"):
+        assert mode in schema


nit: assert against the enum list, not the serialised field (not blocking).

args["mode"] carries the field description that parse_docstring=True parses out of tools.py:642-644, and that description names all five modes in prose. Every assert here is satisfied by the description text alone. The test passes even if the Literal enum is empty or missing, which is the one condition the test docstring says it guards.

Read the enum off the field instead, so the assertion fails when a mode leaves the schema.

claude · 2026-09-01T12:31:56Z

        table_names = [t.strip() for t in tables.replace(",", "\n").splitlines() if t.strip()]
        db = create_managed_database(
            client,
            name=name,
            schema=schema_name or DEFAULT_SCHEMA,
            tables=table_names or None,
+            keys=keys or None,
+            expires_at=expires_at or None,
        )


nit: reject a keys entry that names no declared table (not blocking).

Nothing compares set(keys) against table_names. A model that misspells a table name in keys, or names a table absent from tables, creates a table with no key. The tool then reports success.

The cost is permanent, not a retry: the docstring at tools.py:608 records that a key can only be set at creation, so upsert, update and delete stay rejected for the life of that database. Recovery means creating a second database.

Raise when set(keys or {}) - set(table_names) is non-empty. Skip this only if the API already rejects a keys entry for an undeclared table.

claude · 2026-09-01T12:32:04Z


 from hotdata_langchain._sql import DISTANCE_FUNCTIONS, DistanceMetric
 from hotdata_langchain.databases import (
+    LoadMode,


nit: re-export TablePartitionKey and TableSortKey (not blocking).

Both types now sit in the public signature of hl.create_managed_database (databases.py:172-173). Line 10 of this file already re-exports HotdataClient, ManagedDatabase and QueryResult from the framework for exactly that reason, so the convention is established.

Without the re-export, a caller who reached hl.create_managed_database through hotdata_langchain must add a second import from a package that is not named in this package's own docs. The new README example demonstrates the cost: from hotdata_framework import TableSortKey.

Add both names to line 10 and to __all__.

claude · 2026-09-01T12:32:12Z

        description="sales",
        schema="public",
        tables=["orders"],
+        keys=None,
+        expires_at=None,


nit: no test can catch a keyword the framework client does not accept (not blocking).

mock_client is a bare MagicMock (tests/conftest.py:201), so create_managed_database absorbs any keyword name. This assertion checks the forwarded names against themselves, never against the real client.

Consequence: a wrong name among the six new keywords — keys, expires_at, partition_by, sorted_by, mode, key — passes CI and raises TypeError on the first real call. The PR description reports the suite passing on 0.10.0, a version that predates partition_by on the client. A green run on a version lacking the parameter is direct evidence that the mock hides the signature.

Build the fixture with create_autospec(HotdataClient, instance=True), which binds each call against the installed signature.

claude

Review

The provisioning widening is well argued, and the two design calls in the description hold up. One blocking issue.

Blocking Issues

hotdata_langchain/tools.py:642-646 — the model-facing text names upsert, update and delete together and never states the effect of any one of them. delete removes matched rows and inserts nothing, which a model can read as delete-then-insert by key. The same wording repeats at tools.py:765-766 and databases.py:36-37.

Action Required

Give each key-matched mode an effect, in the docstring and in the tool description. The load tool is the one tool marked destructive, and delete is the mode with the worst outcome and the least defined name.

Non-blocking

Four nits are inline: a vacuous mode-schema test, an unvalidated keys mapping, two missing re-exports, and an unspecced client mock that cannot catch a wrong keyword name.

Context disclosure

CI reported only Check release metadata as passing when this review started. Every other check was queued or in progress, so this review makes no claim about the test suite.

Review found the model-facing text naming `upsert`, `update` and `delete` together with only the shared clause that each matches incoming rows against existing ones. A model choosing between them had no statement of any one's effect, and `delete` reads as delete-then-insert by key. It removes matched rows and inserts nothing, so that misreading destroys rows and reports success. The API's own wording is on `LoadManagedTableRequest.key`: the key columns decide which existing row an incoming row "removes, updates, or replaces". Each mode now states its effect on a matched row and on one that matches nothing, in `LoadMode`, in the tool docstring and in the tool description. Writing that surfaced a break at the `langchain-core>=1.0` floor: 1.0.0's docstring parser treats a colon in a wrapped `Args:` continuation as a new argument name, so every `make_hotdata_tools` call raised. Newer versions parse it, which is why the locked build was green. CI now runs the suite with `--resolution lowest-direct` so the advertised floor is exercised rather than only the resolved ceiling. Also from review: a `keys` entry naming an undeclared table is refused rather than silently creating a keyless table; `TablePartitionKey` and `TableSortKey` are re-exported now that both appear in a public signature; the mode-schema test reads the enum rather than the prose description that satisfied it either way; and one test binds the provisioning calls against the real client signature, which a `MagicMock` cannot do.

claude · 2026-09-01T12:41:33Z

+    spec.load_managed_table.return_value = LoadManagedTableResult(
+        connection_id="c1",
+        schema_name="public",
+        table_name="orders",
+        row_count=1,
+        full_name="sales.public.orders",
+    )
+    create_managed_database(
+        spec,
+        name="sales",
+        tables=["orders"],
+        keys={"orders": ["id"]},
+        expires_at="24h",
+        sorted_by={"orders": [TableSortKey(column="id")]},
+    )
+    inspect.signature(HotdataClient.create_managed_database).bind(
+        spec, **spec.create_managed_database.call_args.kwargs
+    )


nit: bind the load call too, not only the create call (not blocking).

The test name says "every provisioning call". Only create_managed_database reaches the autospec client. spec.load_managed_table.return_value is set on line 832 and never consumed, so mode and key — the two keywords this PR newly forwards at databases.py:405 and databases.py:413 — are still checked only against the MagicMock in test_load_passes_mode_and_key_through.

Consequence: if HotdataClient.load_managed_table does not accept mode or key on the declared floor, CI stays green and every non-replace load raises TypeError on the first real call.

Add a load_managed_table(spec, database_id=..., table=..., file=..., mode="upsert", key=["id"]) call against the same spec, which makes the existing stub live.

claude · 2026-09-01T12:41:35Z

+the tools, not chosen per call by a model:
+
+```python
+


super nit: drop the blank first line of the code block (not blocking). Removing the from hotdata_framework import TableSortKey line left the blank line that separated it from the call.

claude

All five prior threads are addressed: the keyed modes now state their effect in tools.py, databases.py and the README table, the mode test reads the enum, the create tool refuses a key on an undeclared table, and both layout types are re-exported. Two nits remain inline, neither blocking.

rohan-hotdata added 2 commits September 1, 2026 16:02

rohan-hotdata requested a review from a team as a code owner September 1, 2026 12:26

rohan-hotdata requested review from shefeek-jinnah and removed request for a team September 1, 2026 12:26

claude Bot reviewed Sep 1, 2026

View reviewed changes

claude Bot requested changes Sep 1, 2026

View reviewed changes

claude Bot reviewed Sep 1, 2026

View reviewed changes

claude Bot approved these changes Sep 1, 2026

View reviewed changes

rohan-hotdata merged commit 3d6b0e1 into main Sep 1, 2026
10 checks passed

Navigation Menu

Uh oh!

feat: widen the provisioning tools, and mark the ones that can destroy data - #95

feat: widen the provisioning tools, and mark the ones that can destroy data#95
rohan-hotdata merged 3 commits into
mainfrom
feat/91-widen-provisioning-tools

rohan-hotdata commented Sep 1, 2026 •

edited

Loading

Uh oh!

claude Bot Sep 1, 2026

Uh oh!

claude Bot Sep 1, 2026

Uh oh!

claude Bot Sep 1, 2026

Uh oh!

claude Bot Sep 1, 2026

Uh oh!

claude Bot Sep 1, 2026

Uh oh!

claude Bot left a comment

Uh oh!

claude Bot Sep 1, 2026

Uh oh!

claude Bot Sep 1, 2026

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

rohan-hotdata commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What an agent can now do

Two design calls worth review

Why the floor moved to hotdata-framework>=0.13.0

What is deliberately not here

One hazard no version fixes

Verification

Added after review

On Dependabot #94

Why the floor job is the load-bearing part

Uh oh!

claude Bot Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

rohan-hotdata commented Sep 1, 2026 •

edited

Loading

Why the floor moved to `hotdata-framework>=0.13.0`