An agent can say what it based a query on, and why - #234
Merged
Conversation
The activity log records what ran — the statement, the outcome, and two
self-reported columns carrying the caller's framing of the question. What it
has never recorded is the reasoning in between: which curated example was
mirrored, which table was chosen over a similar one, which metric definition
was used, why a filter is there. So an operator can see that a query was wrong
without seeing where the reasoning went wrong.
execute_sql gains one optional argument, basis: a list of {kind, ref, why}
entries, one per choice. A list of small uniform entries rather than eight
named fields, because the tool schema ships on every request so its size is a
running cost, a ninth kind becomes one more name in a sentence rather than
another schema change, and every entry splits the same way into a ref that can
be checked against the SQL and a why that cannot.
Optional and absent is normal: not in required, and a call that omits it writes
a row byte-identical to today's but for a NULL column. Declared all the same,
because additionalProperties is false and an undeclared argument is unsendable.
The declaration is a bare array, and the eight kinds are prose on the property
rather than a schema enum. The MCP SDK validates arguments against inputSchema
before the handler runs, so any constraint there refuses the WHOLE query rather
than bounding the field, and writes no tool_calls row at all — a 260-character
ref is an ordinary IN-list predicate, and losing a user's answer over an
advisory note is the opposite of the point. That validation is also per-item and
runs on the event loop: a max-size body of entries measured 2,210 ms of blocked
worker with an items schema against 3 ms without it.
So the boundary is the only bound, which is where a bound belongs anyway: a
bound the caller applies is not a bound. An entry naming an unknown kind, or
missing the ref that identifies it, is dropped; ref and why are trimmed. ref is
bounded like why, not only why, because for filter and date_range the ref IS the
predicate and can carry a literal out of the customer's data. The stored value
is an object rather than a bare list so one column can carry both the entries
and the fact that they were cut; truncated means only that what is stored is not
what was sent, which covers a dropped entry and a trimmed string alike — a drop
that set no flag would be a silent loss.
Core records the claim and does not adjudicate it. Nothing scores it, and
nothing checks ref against the SQL; a consumer holding the receipt can.
The renderer trusts nothing it reads: basis is plain TEXT and an embedder builds
ToolCallRecord itself, so a non-string value there would reach ui.esc and take
out the whole activity page rather than one card.
Tool schema cost, measured with cl100k_base and paid on every request:
execute_sql 1795 -> 1935 tokens (+140, +7.8%), the whole tools/list 3333 -> 3505
(+172, +5.2%), of which ~130 is the basis property, ~40 the sentence on
execute_sql's own description and ~32 the one on get_prompt_examples telling the
agent its example ids may be cited.
Spec: ACE-110
vishalkalbi27
requested review from
ashwin-agami and
sandeep-agami
as code owners
August 18, 2026 05:32
There was a problem hiding this comment.
Pull request overview
Adds an optional basis payload to execute_sql so agents can self-report what they based a query on (and why), with server-side bounding and safe rendering in the admin activity log—without relying on tool-schema constraints that would cause MCP SDK pre-validation to refuse entire calls.
Changes:
- Adds bounded, serialized
basishandling (_bounded_basis) and exposesbasisas a bare array in theexecute_sqlinput schema (to avoid SDK whole-call refusals). - Persists
basisvia migration020, updates write/read paths (model_store) and contract (ToolCallRecord) accordingly. - Renders
basissafely in the admin activity UI and adds comprehensive tests (including schema/validation, truncation/drop semantics, migration behavior, and XSS escaping).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_ace110_query_basis.py | New end-to-end + unit tests covering bounding, schema expectations, persistence, and UI rendering of basis. |
| tests/test_ace042_no_filter_injection.py | Strengthens surface scanning to recurse through nested schema descriptions. |
| packages/agami-core/src/tools.py | Implements basis bounding/storage and updates tool schemas/descriptions to advertise basis. |
| packages/agami-core/src/model_store.py | Adds basis to tool_calls insert path and to the read SELECT column list. |
| packages/agami-core/src/migrations/core/020_tool_calls_basis.sql | Adds nullable basis TEXT column to tool_calls. |
| packages/agami-core/src/contracts.py | Extends ToolCallRecord contract to include basis. |
| packages/agami-core/src/admin.py | Adds robust, escaped rendering of stored basis under the SQL in the activity view. |
Suppressed comments (1)
tests/test_ace110_query_basis.py:143
- This test docstring says "The schema declares the enum", but
basisis now deliberately a bare array with no enum/items constraints. The rationale is still correct, but the wording no longer matches the actual schema shape and could confuse future readers.
@pytest.mark.parametrize("bad", ["nonsense", "", None, "EXAMPLE", 7, True])
def test_an_unknown_kind_is_dropped_at_the_boundary(bad):
"""Criterion 3. The schema declares the enum, but nothing in this repo validates an incoming
payload against a tool's inputSchema — `enum` is honoured by the client — so a non-compliant
caller reaches this code path and the drop is what makes 'rejected rather than stored' true."""
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+98
to
+100
| def test_every_declared_kind_is_accepted(): | ||
| """The enum on the schema and the set enforced at the boundary have to be the same set, or a | ||
| compliant client sends something the boundary silently drops.""" |
Comment on lines
+8
to
+11
| -- ONE COLUMN, HOLDING JSON, because the shape is a list rather than a value: each entry is a `kind` | ||
| -- from a closed set, a `ref` naming what was chosen, and a `why` that is one sentence. Eight named | ||
| -- columns would be eight ALTERs, and a ninth kind would be a ninth; this way a new kind is a new | ||
| -- enum value on the tool schema and no schema change at all here. |
`basis` declares no `items` schema and no `enum`: the MCP SDK validates arguments against `inputSchema` before the handler runs, so a constraint there refuses the whole call rather than trimming one entry. The kinds are advertised as prose and enforced at the boundary instead. `tools.py` and one of the tests say so already; three places did not follow. One of them is worse than stale. `test_an_unknown_kind_is_dropped_at_the_boundary` read "nothing in this repo validates an incoming payload against a tool's inputSchema" — the opposite of what is true, sitting in the suite that exists to keep the constraint out. A reader who believed it would conclude the schema is a safe place for bounds and put one back, and every over-long ref would start losing the caller's answer. No behaviour change; the tests assert exactly what they asserted before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The activity log records what ran — the statement, the outcome, and two self-reported columns
(
user_question,agent_query) carrying the caller's framing of the question. What it has neverrecorded is the reasoning in between: which curated example was mirrored, which table was chosen
over a similar one, which metric definition was used, why a filter is there. So an operator reading
the log can see that a query was wrong without seeing where the reasoning went wrong.
execute_sqlgains one optional argument,basis— a list of{kind, ref, why}entries, one perchoice — recorded on
tool_callsand rendered in the activity view under the statement it explains.Core records the claim; it does not adjudicate it.
Changes
basisonexecute_sql— a list of small uniform entries rather than eight named fields: theschema ships on every request so its size is a running cost, a ninth kind is one more name in a
sentence rather than another schema change, and every entry splits into a
refthat can be checkedagainst the SQL and a
whythat cannot. Optional, absent is normal, and still declared —additionalProperties: falsemakes an undeclared argument unsendable.enum. The MCP SDK registersthe handler with
validate_input=True, so anything constrained there refuses the whole queryinstead of bounding the field, and writes no
tool_callsrow at all. See below._bounded_basisis the only bound, applied where the call is handled — a bound the callerapplies is not a bound. Unknown
kindor missingref→ the entry is dropped;refandwhyaretrimmed; the list is capped.
refis bounded likewhy, not onlywhy, because forfilteranddate_rangetherefis the predicate and can carry a literal out of the customer's data.{"entries": [...], "truncated": bool}, so one column carriesboth the entries and the fact that they were cut.
truncatedmeans only what is stored is notwhat was sent — a dropped entry and a trimmed string alike, because a drop that set no flag would
be a silent loss in an audit log.
020— one nullable TEXT column, portableALTER, no rebuild._TOOL_CALL_COLSisthe SELECT list and narrower than the INSERT, so it is updated too; a column missing from it is
written on every row and read by nobody.
admin._basis_blockrenders it under the SQL, escaped, and trusts nothing it reads:basisisplain TEXT and an embedder builds
ToolCallRecorditself, so a non-string value would reachui.escand take out the whole activity page rather than one card.get_prompt_examplesgains one sentence telling the agent its exampleidmay be cited.ports.pyis untouched —ActivitySinkdeclares onlyrecord_query_execution.The design changed under review, and it is the main thing to look at
The field was first built with a full item schema —
enumonkind,maxLengthonref/why,maxItems,additionalProperties: false— on the stated belief that nothing server-side validates apayload against a tool's
inputSchema. That belief was wrong, and it was wrong in the directionthat matters:
Server.call_tool(validate_input=True)is the SDK default, sojsonschema.validateruns on every call before the handler. Measured consequences of the original shape:
refof 260 chars (an ordinaryIN-list predicate)That made success criterion 4 — "truncated rather than refused" — unmeetable on the served path,
and made
truncateddead code there. Validation is also per-item and runs on the event loop:Dropping the item schema fixes the refusal, removes the stall, and returns ~90 tokens per request.
The cost is that the eight kinds are prose the model reads rather than an enum a client can validate
against.
Tool schema cost — paid on every request
Measured with
cl100k_base:execute_sqltools/listRoughly ~130 for the
basisproperty, ~40 for the sentence onexecute_sql's description, ~32 forthe one on
get_prompt_examples.Checklist
tests/test_ace110_query_basis.py. No existing test weakened orremoved. One existing test was strengthened:
test_no_notice_leaks_a_spec_id_to_a_clientswept only top-level properties, and this diff is the first to put a description below that
level, so its sweep now recurses.
uv run dev.py checkgreen — ruff lint + format, full suite, gitleaks.uv run dev.py cover— 100% diff coverage.Three must-fix findings, all fixed and independently reproduced before and after: the schema
refusal/stall above, a
TypeErroron an unhashablekindthat escaped the audit-failurepolicy, and a renderer that raised on six shapes its docstring promised to tolerate.
020is the next free number, portable, forward-only, noIF NOT EXISTS.set.
Known gap
A schema-rejected tool call writes no
tool_callsrow on any tool — the SDK validates before thehandler's
finally. Pre-existing and not introduced here; this change now avoids adding to it byconstraining nothing at the schema.
Spec: ACE-110