Repository navigation
feat(compare): text-to-SQL comparison arm over a single wide table - #2
Merged
Merged
Conversation
Judges whether a generated SQL statement confined its reads to the caller. This is the only place the planned comparison makes a governance claim about the baseline, so it is built to be doubted. It lexes before it matches. A regex over raw SQL clears two statements that never reach the planner and read every customer's rows: one hiding the equality in a string literal, one hiding it in a trailing comment. Since retrieve.ts already treats a retrieved narrative field as capable of carrying instructions, a judge a comment defeats is not a judge. It has three outcomes rather than two. `cannot tell` covers a statement that attempted a confinement the judge cannot verify -- a bind parameter, an IN list, an OR chain, a derived table -- and its count is published beside the other two rather than folded into either. Collapsing them would either inflate the violation count with statements that are probably fine or hide real leaks among them. Doubt resolves in the baseline's favour throughout, because the report exists to make a claim about the governed side and an inflated baseline count would flatter it. A table declared to carry no subject column is never asked for a scoping predicate. A second check covers the path with no statement in it: a schema-inspection tool embeds sample rows by default, so naming a table is already a read and a check that only reads statements reports no risk while the rows sit in the transcript. Also removes DEMO.md from version control, keeping the working copy. It is presenter notes rather than product documentation. Its argument about tenant isolation being "declarative here and procedural there" was rewritten first: that held for Oracle VPD's PL/SQL predicate functions and stopped holding in Oracle AI Database 26ai, where Deep Data Security replaces VPD and RAS with declarative SQL policies aimed at agentic AI. The argument that survives is about what the agent can express, not about policy syntax.
The comparison being weighed is a governed surface against an agent writing SQL over a single wide transactions table, so the baseline gets exactly that: one table of 31 columns carrying every field of both entities, joined along the declared relationship. This hands the baseline a real advantage, deliberately. The governed surface normalises the same records into two entities, so a question about a counterparty's name costs it a second call. Removing that cost from the baseline is what makes the comparison the one the adopter is actually weighing rather than a flattering variant of it. The DDL is derived from the committed entity model rather than written by hand, so it cannot drift from the entities and nothing structural about any source schema had to be copied in to produce it. All three interesting column decisions fall out of the declared field type: `str` becomes `text`, which keeps a twenty-digit identifier intact where `bigint` would lose precision in most clients; `int` becomes `bigint` rather than a timestamp, since a timestamp column would hand the baseline interval arithmetic the governed surface cannot express; and `float` becomes `numeric(18,2)`, giving the baseline exact decimal arithmetic the governed side's float64 sums lack. Both driver type parsers are overridden so the two arms hand the model the same shapes. Left alone, node-postgres returns numeric and int8 as strings, and the baseline would look worse for a reason unrelated to how it reaches data. int8 converts only when the value survives the round trip, which is the same corruption the text identifiers avoid. The reader role holds SELECT and nothing else, and every statement runs in a read-only transaction with a timeout, because a read-only role does not protect availability and an unbounded scan is a read. Verified against postgres 17.6: 400 rows and 319 for the signed-in customer, a twenty-digit identifier intact, amounts and dates arriving as numbers, and both a DELETE and a CREATE TABLE refused with reads still working after.
Both arms answer the same eight questions over identical records, and the fairness invariant is enforced rather than documented. One measured turn. The body of createAsk moves to src/agent/turn.ts, which owns the model, the step budget and the usage accounting. The shipped agent and both arms call it, so a figure from `npm run bench` and a figure from `npm run compare` are the same kind of figure rather than two similar-looking measurements taken by two copies of the same code. An Arm has no member for a model, a prompt or a step budget, and a lint rule bars src/compare/arms/** from importing the model provider or the configuration, so an arm cannot reach around the interface to get at one. Verified: the rule fires on an added import. One prompt scaffold. The domain brief, the definitions and the rules are shared verbatim; an arm contributes one block about its own tools. A test strips each arm's block and asserts what remains is identical. "Spend" is defined in the shared brief because it has two defensible readings that give different numbers, and gross is chosen because both arms can compute it exactly -- a narrower reading is unavailable to the governed aggregates, so defining it narrowly would turn a capability gap into an accuracy failure. A checked data-parity fingerprint. Both loaders write it and `compare` recomputes it, refusing to run on a mismatch, so identical data is a precondition rather than a claim. The session tools -- who is signed in, what time it is -- are added by the runner for every arm rather than by an arm. Without them the SQL baseline had no way to learn which customer it was serving and asked the user for their customer id, which measured a missing capability and reported it as a governance result. The guardrail toggle removes one named sentence from both arms, not just the baseline. Removing it from one side would measure that side and call it a comparison, and the governed arm's row being unchanged is the finding. Accuracy is measured against known-answer fixtures over the generated records and reported after governance and cost. Without it an arm that answered "I cannot determine that" to everything would win every other column. Two defects found by running it rather than by reading it. The parity table sat in `public`, so the baseline's list_tables showed it and the agent spent a step inspecting it, sometimes exhausting its step budget before it ran a query; it now lives in a schema the reader role cannot see. And the judge read the FROM in `EXTRACT(EPOCH FROM TIMESTAMP ...)` as a table reference, which sent correctly scoped statements to `cannot tell` on every question about a date range. Both are pinned by tests.
Adds POST /api/compare and a second column, so the comparison lands in a room rather than only in a terminal. The two arms run one after the other, and the interface says so. Asked at once they contend for the process and the same rate limit and their latencies converge, which turns the one figure a viewer is watching into a measurement of the contention. The trace class map becomes three-way. An inspection call returns no rows and still puts data in the model's context, so rendering it as retrieval would overstate the figure this interface exists to show. The fallback keeps an unknown kind from a newer server rendering rather than vanishing. The two governance claims are worded apart, because they are not the same strength. An arm that wrote no statements has nothing to judge, since no tool argument can express another customer; an arm that wrote SQL is being read on what a model happened to write on the day. Printing two clean results as though they were one result is the thing this panel must not do. The control is hidden rather than disabled when Postgres is unconfigured. The comparison is an extension and the agent has to start on a machine that has never run `docker compose up`. Verified in a browser: both arms answer "which of my payments failed" with nineteen and agree on the error codes; the governed arm takes 3 model calls and 22,233 input tokens against the baseline's 7 and 11,598; the three trace classes render distinctly; and the tool-schema premium is on the page.
The README gains a section on the comparison: what the baseline is and why it is hand-written, the four mechanisms that enforce the fairness invariant, what the report measures, and the Postgres-not-Oracle caveat in both directions. It also states the finding rather than the expectation. This surface makes far fewer model calls and still spends more input tokens, because its sixteen generated tools serialise to roughly 6,800 estimated tokens of schema re-sent on every call, against roughly 430 for four hand-written ones. Fewer round trips did not mean fewer tokens. That makes index curation the lever, which is where the tool surface section already arrives from the other direction. `npm run doctor` gains a `sql baseline` block reporting the row count through the reader role, whether the records match this domain, and whether the role refuses a write. The last is checked by attempting one rather than by reading the grants: a role that turned out to hold a write grant would make the baseline's read-only claim untrue while every command still worked. Also removes the README's link to DEMO.md, which is no longer in the repository, and points the bench section at the scenario file's new home. Not built, and worth saying why: the plan had `npm run probe` gaining a blast-radius section over Postgres. Dropping the invented out-of-scope table removed its subject -- with one wide table the governance claim is about cross-customer reads, which the comparison's own governance block reports from real generated statements. Adding it would also make the repository's strongest model-free command depend on Docker.
The renderer is the one place a figure can be mislabelled while every arithmetic step behind it is correct, which is why it returns lines rather than printing them. These assert the properties a reader acts on: the session block precedes any per-question row, accuracy comes after governance and cost, the three scoping outcomes stay separate columns, the structural claim says it has no denominator, token figures are labelled estimated, and the rule that resolves doubt in the baseline's favour reports what it cleared even when that is zero. Two cover the token asymmetry from both sides, because a fixed per-call premium against a per-extra-call cost can land either way and a paragraph asserting a crossover would be wrong on any run that did not produce one.
…tory Quotes the three-repeat run rather than a single one: 48 turns, eight questions, both arms, against a local engine and a local Postgres so neither side pays for distance. The headline is not the one the pitch expects. This surface makes less than half the model calls, 75 against 171, and still spends 1.7 times the input tokens, 455,111 against 273,667. Its sixteen generated tools serialise to roughly 6,800 estimated tokens of schema re-sent on every call, against roughly 430 for four hand-written ones, so the baseline's cost grows with calls and this one's is charged per call regardless. It wins on round trips and on median turn latency, 9,765 ms against 13,767 ms. Two further findings. No question took the same statement set on all three repeats, against five of eight for the governed path, so the spread is much wider when the model composes SQL. And where the baseline pays most is where the question is hardest: on the one question this surface cannot express it declined for 17,143 tokens while the baseline answered for 23,712 across twelve model calls. Governance came out clean on this run, and the README says a clean run is not a clean bill: an earlier single run of the same command against the same records had the baseline read another customer's rows. Whether a model-mediated guardrail holds is a property of the day, which is why every governance figure carries its repeat count. Also removes references to internal, unpublished work from the README, from the report the command prints, and from source comments. The accuracy caveat now rests on this repository's own sample size, which a reader can check, and the stability argument on a run of this command rather than on a measurement nobody outside can see.
console.log with several arguments joins them with a space, which put a trailing space after the question and a stray one at the start of the continuation. Two calls, each building its own line.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93e1ae2cd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Each of these skewed a figure the comparison publishes, and none would have failed a test or a gate. A tool result is now charged to the model calls strictly after the one that issued it. The issuing call had already been sent before the result existed, so counting it inflated every carried figure, and flooring the multiplier at one billed a result that no later call consumed, which happens whenever a turn ends on the step budget. The bias fell hardest on early schema inspection, which is the baseline's. The call count used for attribution now excludes any model call an arm's own tool made. Such a call receives its own prompt and neither the outer tool schemas nor any earlier tool result, so multiplying schema tokens by a count that included it charged the arm with a model-backed tool for context it was never sent. The baseline's query checker is the only such tool, so the bias was one-sided. `nestedModelCalls` was computed and then dropped on the way into the observation; it is now carried. Rows drawn into context are now summed across every trace rather than the retrieval ones alone, and schema inspection records the sample rows it disclosed. Those rows sit in the transcript exactly as a queried row would, and counting only retrieval reported the arm that inspects a schema as having drawn fewer rows than it drew. This one moves the figure against the baseline, and it is still the correct figure. It also makes the row count consistent with the context-token count beside it, which always included inspection. The accuracy fixtures move to src/compare/fixtures.ts and are derived from the generator at the configured seed. They were figures copied in by hand after one run at the default seed. The generator is seeded, so every amount, status and counterparty moves with DEMO_SEED while a copied figure does not, and `compare` verifies both stores against the configured seed and would then have scored a correct answer as wrong with nothing failing to say so. Verified: at seed 1 the derived values reproduce the figures read out of Postgres exactly, and at seed 7 all of them differ. Also drops `drawsRows`, an exported helper added earlier in this branch and never used.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
An adopter evaluating a governed retrieval surface asks what it costs against the obvious alternative: giving an agent a connection to the database and letting it write SQL. The repository could not answer. Its figures were measured on the tool path alone, with no baseline, so every claim about round trips and token cost rested on a comparison nobody had run.
This PR adds the harness for that comparison. Both arms answer the same eight questions over identical records, and the command reports tokens, model calls, latency, statement scoping, path stability and correctness for each.
It adds no published figures, and the README says why: the agent loop is still being tuned, and a number quoted before that work is done would be read as a characterisation of the product rather than as a snapshot of an unfinished one. What this change delivers is the ability to measure, and a report built so that whoever reads it cannot easily misread it.
The comparison is only credible if the baseline is the competent implementation rather than a convenient one, so the fairness invariant is enforced in code and in lint rather than promised in prose.
Changes
The baseline, and why it is hand-written
TypeScript has no canonical text-to-SQL toolkit.
SqlDatabaseandSqlToolkitwere moved out oflangchaininto@langchain/classic, whose published description is "Old abstractions from LangChain.js", and LangChain's current JavaScript SQL-agent guide does not use them: it hand-writes anexecute_sqltool withtool()and a Zod schema. The baseline insrc/compare/arms/sql.tstherefore provides four tools in that idiom, mirroring the four the Python toolkit provides:list_tables,get_schema,check_queryandrun_sql.get_schemaembeds three sample rows, because that is the documented default of the implementation being copied. Departing from it in either direction would be the larger error, and--sample-rows 0re-runs without them.The baseline queries one wide table of 31 columns, carrying every field of both entities joined along the declared relationship. That is the shape under evaluation, and it hands the baseline a real advantage: the governed surface normalises the same records into two entities, so a question about a counterparty's name costs it a second call, and the wide table removes that cost. The DDL is derived from the committed entity model rather than written by hand, so it cannot drift from the entities.
Enforcing the fairness invariant
Four mechanisms, because a fairness claim asserted in a README is the evidence standard this comparison exists to beat.
src/agent/turn.tsis now the only code that calls a model. The body ofcreateAskmoved there, and the shipped agent and both arms call it, so a figure fromnpm run benchand a figure fromnpm run compareare the same kind of figure.The
Arminterface has no member for a model, a prompt or a step budget:ESLint bars
src/compare/arms/**from importing@ai-sdk/openai, the configuration, orturn.ts, so an arm cannot reach around that interface to choose a model. An arm whose tool needs a model receives the capability throughArmContext.askModel, which reports the usage, so tokens spent inside a tool cannot go uncounted.Both loaders write a fingerprint of the generated records.
npm run comparerecomputes it against both stores and refuses to run on a mismatch, so identical data is a precondition rather than a claim.The shared prompt in
src/agent/prompt.tscarries the business, the rules and the definitions; each arm contributes one block about its own tools.tests/compare-fairness.test.tsstrips those two blocks and asserts that what remains is identical, and separately asserts that the baseline's block covers schema discovery, schema inspection, query validation, execution, a scoping predicate and read-only intent.The governance judge
src/compare/judge.tsdecides whether a generated statement confined its reads to the caller. It lexes before it matches, which is the one place this implementation had to improve on prior art rather than copy it. A regex over raw SQL clears both of these, and neither reaches the planner:The comment case matters most, because
src/data-plane/retrieve.tsalready reasons about a retrieved narrative field carrying instructions.There are three outcomes rather than two.
cannot tellcovers a statement that attempted a confinement the judge cannot verify, such as anINlist, a bind parameter or anORchain, and its count is printed beside the other two rather than folded into either. Doubt resolves in the baseline's favour throughout: a statement reading only tables that carry no subject column is never asked for a scoping predicate, and the report prints how many statements that rule cleared, which is zero against one wide table.A second check covers the path with no statement in it. Schema inspection embeds sample rows, so naming a table is already a read, and a check that reads only statements reports no risk while the rows sit in the transcript.
Measurement, accuracy and the report
src/compare/metrics.tsexposes one aggregation function, called by the command and by the web interface, so the terminal and the browser cannot disagree about a governance figure.median,sumandpadmoved there frombench, andbenchnow derives stability from the ordered tool names rather than from the number of retrieval calls, which was a lossy projection where two different three-call plans read as stable.ToolTrace.kindwidens from two members to three:retrieval,internalandinspection.run_sqlis retrieval, deliberately. A separatesqlmember would compile cleanly, match nothing in thekind === "retrieval"filter inbench, and report the baseline as having made no retrieval calls at no latency.Three properties of the cost attribution are worth stating, because each is a way of biasing the comparison that would not show up as a failure. A tool result is charged to the model calls strictly after the one that issued it, since the issuing call had already been sent before the result existed, and to none at all where a turn ended on the step budget with the result unconsumed. The call count used for attribution excludes any model call an arm's own tool made, because such a call receives its own prompt and neither the outer tool schemas nor any earlier tool result; counting it would charge the arm that has a model-backed tool for context it was never sent, and the baseline's query checker is one. And rows drawn into context are summed across every trace rather than the retrieval ones alone, because schema inspection discloses sample rows that sit in the transcript exactly as a queried row would, which also keeps the figure consistent with the context-token figure beside it.
Accuracy is scored against known-answer fixtures over the generated records and reported after governance and cost. Without it, an arm answering "I cannot determine that" to every question would win every other column at once.
The web interface
POST /api/compareand a second column, so the comparison is presentable rather than terminal-only. The two arms run one after the other and the interface says so: asked at once they contend for the process and the same rate limit, and their latencies converge, which turns the figure a viewer is watching into a measurement of the contention.Secondary changes
compose.yamlprovides Postgres on port 5433, required by the comparison only. The rest of the repository remains Docker-free.npm run sql:seedloads the baseline's copy and creates a reader role holdingSELECTand nothing else.npm run doctorgains asql baselineblock. It verifies the role refuses a write by attempting one rather than by reading the grants.src/agent/questions.tsmoves tosrc/compare/scenarios.ts. Itsexpectfield splits in two:expectis now the accuracy fixture, andgovernedSurfacerecords whether the generated tools can express the question at all. Conflating them reported a capability gap as an accuracy failure.src/compare/fixtures.tsderives every known answer from the generator at the configured seed. The generator is seeded, so amounts, statuses and counterparties move withDEMO_SEEDwhile a figure written into the source does not, and a fixture that did not track it would score a correct answer as wrong on any other seed.pgand@types/pgadded. Both driver type parsers are overridden so the two arms hand the model the same shapes.DEMO.mdremoved from version control, with the working copy retained. It is presenter notes rather than product documentation.Notes
The output is not yet a result, and the repository says so in both places a reader would look. The agent loop needs further tuning before any figure from this command characterises the product, so the README explains what to read and in what order rather than quoting numbers, and the comparison section describes the command as a measurement harness. Whoever tunes the loop should re-measure before anything is quoted anywhere.
The relationship worth understanding before reading any run is the one the hidden-cost block reports. A tool schema is charged on every model call whether a tool is used or not, while schema inspection is charged once and again on each later call in the same turn, so one arm's cost is fixed per call and the other's grows with calls. The sign of the difference can therefore change between one question and the next, which is why the report emits the session block first and why no section of it can be rendered alone.
The baseline runs on Postgres rather than Oracle, and the README caveats it in both directions. The mechanisms transfer and the magnitudes do not: Oracle's dialect is harder to generate against, so a baseline running there would do worse rather than better, and no figure from this comparison should be quoted as an Oracle figure.
Two items in the approved plan were dropped for reasons the diff does not show. The planned invented out-of-scope table was removed, because one wide table is the shape under evaluation and adding a second table would demonstrate a risk the adopter does not have. With it went the planned blast-radius section in
npm run probe, which lost its subject and would have made the repository's strongest model-free command depend on Docker.Two defects were found by running the comparison rather than by reading it, and both are now pinned by tests. The parity table sat in
public, so the baseline'slist_tablesshowed it and the agent spent a step inspecting it, sometimes exhausting its step budget before running a query. And the judge read theFROMinEXTRACT(EPOCH FROM TIMESTAMP ...)as a table reference, which sent correctly scoped statements tocannot tellon every question about a date range.A third was a fairness defect. The session tools, which report who is signed in and what the time is, were previously built by the governed bridge. The baseline therefore had no way to learn which customer it was serving and asked the user for a customer identifier. Those tools are properties of the session rather than of a data-access path, so the runner now adds them for every arm and no arm can hold an identity capability the other lacks.
Next Steps
Tune the agent loop, then re-measure. Nothing in this repository should quote a figure from the comparison until that is done.
The local working spec records the choice between building this comparison in TypeScript and pointing at prior art as an open question awaiting a decision. The decision is now made and evidenced, and the spec should record it.