ACE-039: deny recon functions, and stop relaying driver text - #176
Merged
Conversation
5 tasks
There was a problem hiding this comment.
Pull request overview
This PR hardens the SQL execution boundary against two leakage channels: (1) metadata/recon function calls that bypass object-scope gating, and (2) raw driver error text being relayed back across the LLM boundary. It also preserves operator diagnostics by capturing raw error text server-side (audit-only) while returning fixed value-free failure messages to callers.
Changes:
- Extend SQL neutralization to return stripped neutralized text plus quoted-identifier spans, enabling a recon gate to distinguish keywords from quoted identifiers.
- Add a recon gate that refuses server-fingerprinting/object-probing functions (incl. niladic keywords and reg* casts) while leaving catalog relations to model scope.
- Sanitize driver-originated failures by classifying from driver text but returning fixed messages; store raw driver details in
query_executions.error_detail(bounded), and rebuild failure messages from exit codes on the forked path.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_sql_guard.py | Updates neutralizer contract assertions to use .text. |
| tests/test_ah012_executor_seam.py | Adjusts seam tests to assert fixed sanitized messages instead of driver text. |
| tests/test_ace039_recon.py | New corpus tests for recon function denial and false-positive boundaries. |
| tests/test_ace039_neutralizer_spans.py | New tests for quoted-identifier span reporting and frame correctness. |
| tests/test_ace039_exit_codes.py | New tests ensuring every failure kind round-trips through exit codes and docs. |
| tests/test_ace039_error_sanitization.py | New tests asserting driver text never crosses the boundary and fork parity holds. |
| tests/test_ace039_error_detail.py | New tests for audit-only raw error capture, fork NULL behavior, and bounding. |
| tests/test_ace035_read_only_refusal.py | Refactors tests to separate authored vs sanitized exit-code bands. |
| tests/test_ace035_no_enumeration.py | Removes strict xfail; asserts driver HINT no longer leaks; adds recon vector. |
| tests/test_ace035_guardrail_contract.py | Pins recon reason as unsafe now that the rule is produced. |
| tests/test_ace035_guardrail_audit.py | Adds recon refusal row to prove auditing applies to refusals generically. |
| tests/test_ace035_envelope.py | Updates failure kind expectation from syntax to column_not_found + sanitized message. |
| plugins/agami/skills/agami-query/SKILL.md | Updates skill guidance re: timeout vs resource_limit and new kinds. |
| plugins/agami/shared/db_error_classifier.md | Removes timeout row; documents watchdog-based timeout classification and vocabulary ownership. |
| plugins/agami/lib/sql_guard.py | Vendored mirror: neutralizer spans + recon gate implementation. |
| plugins/agami/lib/guardrail.py | Vendored mirror: pins RULE_RECON reason to unsafe. |
| plugins/agami/lib/execute_sql.py | Vendored mirror: failure classification + sanitization + exit-code table widening. |
| packages/agami-core/src/tools.py | Rebuilds forked failure messages from exit codes; adds bounded error_detail capture/reset. |
| packages/agami-core/src/sql_guard.py | Neutralizer spans + recon gate implementation in core package. |
| packages/agami-core/src/model_store.py | Extends query_executions insert to include error_detail. |
| packages/agami-core/src/migrations/core/016_query_executions_error_detail.sql | Adds error_detail column to query_executions. |
| packages/agami-core/src/guardrail.py | Pins RULE_RECON reason to unsafe in core contract. |
| packages/agami-core/src/execute_sql.py | Widens exit codes, classifies/sanitizes driver errors, and captures raw detail operator-side. |
| packages/agami-core/src/contracts.py | Adds error_detail to QueryExecutionRecord. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+669
to
+673
| # allowed is a schema-listing endpoint. | ||
| hit = match.group(0).strip(" .(:") | ||
| return refuse( | ||
| RULE_RECON, | ||
| detail=f"metadata/recon access is not allowed (`{hit}`)", |
Comment on lines
+669
to
+673
| # allowed is a schema-listing endpoint. | ||
| hit = match.group(0).strip(" .(:") | ||
| return refuse( | ||
| RULE_RECON, | ||
| detail=f"metadata/recon access is not allowed (`{hit}`)", |
Comment on lines
+302
to
+318
| if code == 3 or has("no module named", "modulenotfounderror", "command not found"): | ||
| return "driver_missing" | ||
| # Ten engines spell an authorization failure ten ways, and getting this wrong is not cosmetic: | ||
| # a `permission` failure tells the operator to GRANT, while `auth` tells them to re-credential | ||
| # and `syntax` makes the skill auto-retry the identical statement twice. | ||
| if has( | ||
| "permission denied", | ||
| "insufficient_privileges", | ||
| "insufficient privileges", # Oracle ORA-01031, a space rather than an underscore | ||
| "command denied", | ||
| "insufficient_access_or_readonly", | ||
| "permission was denied", # SQL Server | ||
| "access denied for user", # MySQL 1044/1045 — narrower than the bare needle in `auth` | ||
| ) or ("access denied" in lowered and has("table", "dataset", "cannot select")): | ||
| # BigQuery and Trino both open with "Access Denied:", which the `auth` arm's bare | ||
| # "access denied" would otherwise swallow into a credentials problem. | ||
| return "permission" |
Comment on lines
+302
to
+318
| if code == 3 or has("no module named", "modulenotfounderror", "command not found"): | ||
| return "driver_missing" | ||
| # Ten engines spell an authorization failure ten ways, and getting this wrong is not cosmetic: | ||
| # a `permission` failure tells the operator to GRANT, while `auth` tells them to re-credential | ||
| # and `syntax` makes the skill auto-retry the identical statement twice. | ||
| if has( | ||
| "permission denied", | ||
| "insufficient_privileges", | ||
| "insufficient privileges", # Oracle ORA-01031, a space rather than an underscore | ||
| "command denied", | ||
| "insufficient_access_or_readonly", | ||
| "permission was denied", # SQL Server | ||
| "access denied for user", # MySQL 1044/1045 — narrower than the bare needle in `auth` | ||
| ) or ("access denied" in lowered and has("table", "dataset", "cannot select")): | ||
| # BigQuery and Trino both open with "Access Denied:", which the `auth` arm's bare | ||
| # "access denied" would otherwise swallow into a credentials problem. | ||
| return "permission" |
| 7 — the statement referenced a column the database does not have | ||
| 8 — the statement referenced a table the database does not have | ||
| 9 — the connection's role lacks SELECT on a referenced object | ||
| 10 — the database was unreachable mid-statement (connection refused / reset) |
| 7 — the statement referenced a column the database does not have | ||
| 8 — the statement referenced a table the database does not have | ||
| 9 — the connection's role lacks SELECT on a referenced object | ||
| 10 — the database was unreachable mid-statement (connection refused / reset) |
…e frame (S1)
The recon gate needs to tell `SELECT "current_user" FROM audit_log` (a column that
happens to share a keyword's name) from `SELECT current_user` (the keyword). The
neutralizer drops quote delimiters on purpose, so that information cannot be
recovered from the text afterwards and has to be carried alongside it.
`_neutralize` now returns `_Neutralized(text, quoted)`: the neutralized statement
and one `[start, end)` per double-quoted identifier, naming that identifier's
CONTENT. A stdlib NamedTuple, because this module is vendored byte-identical into
the no-pip plugin mirror and must stay stdlib-only and importable on 3.9.
ONE COORDINATE FRAME. The scan re-supplies separator spaces and drops delimiters,
so input offsets are not output offsets, and the caller's `.strip()` shifted them a
third time. The strip moves inside the function and spans are reported against the
stripped result, so nothing reconciles frames. This is the part worth reviewing: a
span in the wrong frame does not fail loudly, it mis-identifies a token, and the
failure is silent in both directions.
A span the strip disturbed is DROPPED, never clamped. Dropping means the niladic
matcher will not skip that token, so the gate over-refuses; clamping a span that ran
off either end would widen the skipped region and could hide a live keyword. The
safe direction is chosen explicitly rather than left to arithmetic.
THE UNWRAPPING IS UNCHANGED. `check_read_only` moves by one token, `.text`, and
never reads `.quoted`. That unwrapping is `c42f96c`, the fix for a file read
verified against PostgreSQL 16, where `SELECT*FROM"pg_read_file"('/etc/passwd')`
executed while the guard passed it because the weld destroyed the `\b` anchor.
Reversing it here, or applying quoted-is-an-identifier to a call matcher, reopens
that hole. The welded corpus is re-asserted in this commit rather than the one that
adds the consumer, so the guard exists before anything can lean on it.
Tests: new tests/test_ace039_neutralizer_spans.py proves each span indexes the
identifier it came from (including the `""` escape collapsing to three characters,
and the two adjacent spans in `"schema"."table"`), that spans survive a leading
comment and a trailing `;`, that blanked literals and comments contribute none, and
that the welded forms are still refused. test_neutralize_preserves_token_structure
compares `.text` over its existing six cases: a deliberate contract change, recorded
in the spec's Decisions, with the property it pins untouched.
Spec: ACE-039
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`execute_guarded` classifies a driver error at the chokepoint and returns a `Failure`. In-process and over HTTP the caller gets that object. On the DEFAULT stdio surface the tool edge forks, and `main` collapses the kind to an exit code the parent reads back through `EXIT_TO_FAILURE_KIND` — so a kind with no code of its own is a kind the fork silently loses. `column_not_found`, `table_not_found`, `permission` and `network` had no codes. All four fell to the `other` default, and the parent reported `other` while the identical error in-process reported the real kind. Nothing failed: a test that exercised only the in-process path saw the correct classification, and the wrong one shipped on the transport almost every caller actually uses. `_child_failure_message` compounds it. It relays the child's already-sanitized line only for a code present in the table, so an unmapped code ALSO discarded the message and substituted the generic unexpected-failure text. One missing table entry lost the kind and the message together. Codes 7-10 added, the docstring table extended, and the `FAILURE_KIND_TO_EXIT` comment corrected: the default now covers exactly one kind, `timeout`, which the subprocess supervisor mints at the tool edge when a child never returns and which therefore never reaches `main` to be encoded. That is a gap by construction rather than omission — a supervisor stopping an unresponsive child cannot attribute the kill to the statement, which is why it is a failure and not a `resource_limit` refusal (guardrail contract §3). Deliberately behaviour-neutral. Nothing mints the four kinds until the sanitization slice, so this lands green and alone, and that slice's fork-parity criterion becomes a pure assertion instead of an assertion plus a fix. `tools._classify_exit` and `_child_failure_message` both read this table rather than keeping a copy, so both inherit with no edit. Asserted rather than assumed — a re-introduced local copy would pass every other test here. Tests: new tests/test_ace039_exit_codes.py. Includes a test that parses the code table out of the module docstring and compares it to the dict beside it, which makes the module's "owns the contract because it documents it here" claim checkable; that docstring is what the two out-of-repo consumers are written against. Spec: ACE-039 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eft to the model (S3) Even under read-only plus object scope, a statement could call `version()`, `current_user`, `has_table_privilege(...)` or `'x'::regclass` to fingerprint the server or probe for objects. A semantic model declares tables and columns and never declares functions, so object scope had nothing to reject and the model had nothing to say. Verified before the fix: `SELECT o.id, version() FROM orders o` passed every gate. FUNCTIONS ONLY. No schema names, no relation names, no relation-name prefixes, no system variables. Catalog RELATIONS are the model's to refuse: `check_table_scope` already rejects `pg_class` and `information_schema.tables` as tables the model does not declare, so the relation half was redundant where that gate runs — and where it does not, it was harmful. Matching bare schema names on every engine refused a datasource whose `sys` schema holds ordinary user tables. A schema the model declares is a schema the caller may query, and there is now a test that says so. Residual, stated rather than hidden: on the vendored plugin layout `semantic_model.runtime` is absent by construction (sqlglot and pydantic; the mirror is stdlib-only), so catalog relations have no gate there at all. Recon denial protects an operator from a caller who is not them, and on that layout the user owns the machine, the credentials and the role. Three things the reference implementation did not have: 1. The paren matcher is built from the call names UNION the niladic keywords. Without it `SELECT "current_schema"()` is skipped by the niladic matcher (a quoted span) and missed by the paren matcher (`current_schemas` needs its trailing `s`) — a hole the false-positive fix would otherwise open. This is the one place the FP rule and the deny-list interact, and it wants a reviewer's eye. 2. `has_\w+_privilege` is enumerated to the fourteen real builtins, so `has_active_privilege(...)` stops matching. Over-refusal is the failure mode this list has already produced in real use. 3. Register-type casts, both spellings. `::regclass` anchors on the type name because the neutralizer blanks the literal before it; `CAST(x AS regclass)` requires the closing paren so a column aliased `AS regclass` is not caught. `_neutralize`'s spans have exactly one consumer, `_recon_niladic_hit`, and containment must be FULL — a partial overlap means the span and the match disagree about where the token is, and the safe reading of a disagreement is not to skip. Ordering is fixed and now asserted. A name on both this list and the dangerous-function list refuses as `read_only`, because that gate runs first and owns what it names (principle 9). Nine such collisions are pinned as `read_only` so the label cannot drift. An unreadable statement refuses `undetermined`/`unparseable`, not `recon`: it fails closed either way, but calling it `recon` told the caller it had tried to fingerprint the server when we simply could not read the statement. `REASON_FOR_RULE[RULE_RECON] = "unsafe"` — pinned by the slice that produces it, which is why ACE-035 left it out. `unsafe` rather than `out_of_scope` because there is no in-scope spelling of `version()` for an out-of-scope refusal to point toward. Three shipped ACE-035 tests change, deliberately and recorded here: the unpinned-rule set and the `refuse()` KeyError parametrize both lose `recon`, and the pinned table gains it. `_NO_VECTOR[RULE_UNPARSEABLE]` is AMENDED rather than removed — it now has a producer, but no route reaches it, because `check_read_only` runs the same neutralizer first. The enumeration sentinel gains a recon vector (16 cells to 20) and the audit matrix a recon row, so the new refusal is proven to leak nothing on all four routes and to write exactly one audit row. Spec: ACE-039 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…of it (S4)
The failure channel was the last place the model could leak. A guardrail refusal is
built from static prose and echoes only what the caller sent; `failure.message` was
relayed from the driver verbatim, and PostgreSQL volunteers declared column names.
Driven on the base commit, not argued:
failure.kind -> syntax
message -> 'Postgres execution error: column "amount" does not exist
HINT: Perhaps you meant to reference the column
"orders.internal_ref".'
`orders.internal_ref` is DECLARED and the caller never sent it. The kind was wrong
too: nothing read the text, so every code-5 error was labelled `syntax`.
After: kind `column_not_found`, message a fixed value-free sentence. Classifying FROM
driver text is not returning it — the output is one of a closed set of labels, so the
caller still learns enough to act while learning nothing about the schema.
WHOSE TEXT IS IT. Codes 2 and 3 are authored by this module (the credential
remediation naming DATASOURCE_URL, the chmod-600 fix, the `pip install` line): they
name an operator action, contain nothing the database said, and are still relayed
verbatim. Codes 4 and 5 are the twenty f-string sites that interpolate the driver's
own exception. The module docstring's table already drew that line, so the
discriminator is documented rather than invented — and it is the exit code rather
than a flag an adapter opts into, so an adapter doing nothing unusual is still
sanitized.
One site had to move to make that literally true: the BigQuery service-account load
raised code 2 while interpolating a google-auth exception, whose text can carry the
absolute path of the key file. Authored prose out, the original to the log. It was
the only such site in the thirty-nine.
Cancellation is ceded to ACE-038 — but NOT by deleting the arm. Deleting it leaves
these mis-classified rather than unclassified: the text falls through to `network`
via a "timed out" needle, or to the exit-5 prior and out as `syntax`. An
unattributable server-side cancellation is honestly `other`, so it gets an explicit
arm that says so. Relatedly, "timed out" is deliberately NOT a `network` needle:
adding it would silently move every driver connect timeout off `auth`.
Fixed a needle the reference had wrong: MySQL 2013 reads "Lost connection to MySQL
server DURING QUERY", which "lost connection during query" never matched. Kept
specific, because a bare "lost connection" also covers the initial-packet failure,
which is a real network error and not a cancellation.
Three shipped assertions change, deliberately and recorded: two in
test_ah012_executor_seam.py that asserted the driver's text reached the caller and
`main`'s stderr, and one in test_ace035_envelope.py whose `no such column` vector was
labelled `syntax` by the exit-code prior and is genuinely `column_not_found`.
The `xfail(strict=True)` in test_ace035_no_enumeration.py flips and its marker is
removed in the same change — a strict xfail that passes is a CI error, which is the
alarm it existed to raise. The file header is corrected from "not yet fixed" to what
is now true.
Fork parity is asserted against a real subprocess: the same missing column through
both routes yields the same kind and the same message. Before S2's exit codes this
reported `column_not_found` in-process and `other` forked.
Spec: ACE-039
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d it (S5)
Sanitizing the failure message without capturing the original would leave an
operator debugging a real customer failure with nothing to read. Migration 016 adds
`error_detail` to `query_executions`.
A COLUMN, NOT A SECOND TABLE — 014's argument, unchanged: the detail is 1:1 with the
execution and `Envelope.audit_id` IS `query_executions.id`, so a second table would
be a join that can only ever match one row, keyed on an id that already lives here.
NULL IS A CLAIM. It means the chokepoint holding the raw text and the recorder
writing the row were not in one process. In-process and over HTTP they are, and the
column is populated. On the forked stdio surface the child classifies and sanitizes,
the parent records, and the child writes no audit row by design — so the raw text
never crosses and the column is NULL, which is what that NULL says.
The text is carried out-of-band in a ContextVar rather than on the Envelope, because
`Failure` is `{kind, message}` and a raw field on the contract is a raw field
somebody eventually serializes. Bounded at 2,000 chars: a driver error with a full
HINT / CONTEXT / parameter dump is unbounded, and 015's "a failed statement must not
become a way to grow the store" applies verbatim.
TWO BUGS FOUND WHILE BUILDING THIS, both silent.
1. The child's raw-detail log was itself a leak path. This module never calls
`basicConfig`, so a record on the module logger falls through to
`logging.lastResort` and is written to STDERR — and `_child_failure_message`
relays the child's whole stderr into `failure.message` for any classified exit
code. Logging the raw text there would have handed it straight back to the caller
on the DEFAULT surface, in the code meant to protect it. A dedicated `_RAW_LOG`,
silenced by `main` for the CLI lifetime, keeps the child silent while the
in-process and hosted paths propagate to the server's root logger as intended.
2. The carrier went stale across the fork. `execute_guarded` clears it on entry, but
the PARENT never calls `execute_guarded` — it spawns a child and records the row
itself. So a forked call following an in-process failure in the same server
process read the earlier call's driver text and wrote it onto the later call's
audit row: an operator would debug a statement against an error it never produced.
The reset moved to the tool edge, where both paths begin. Caught only because a
test happened to order two calls one way; it now has a regression test that orders
them that way on purpose.
Both are asserted against a REAL subprocess, not a stub, because both are transport
properties: the child's whole stderr is checked, since anything on it reaches the
caller.
Spec: ACE-039
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ow goes (S6)
`db_error_classifier.md` serves the LOCAL skill surface — agami-connect,
agami-query, agami-save-correction — where the raw error is already in front of the
user and an actionable remediation is the point. `execute_sql`'s failure crosses the
LLM boundary and carries `{kind, message}` with no remediation and no value text.
Those are different presentations of the same classification, not a conflict, so the
detection rules stay one home and the vocabulary is now cited from
`guardrail.FailureKind` rather than declared a second time.
The `timeout` row is deleted. Every one of its detections — psycopg2 `QueryCanceled`,
MySQL 2013, Snowflake `query was canceled`, explicit `statement_timeout` — is a
cancellation signature, and a deadline is classified from the watchdog signal that
fired it rather than from whatever string the driver returned. Its remediation was
ACE-038's, verbatim, in a document that is not ACE-038's.
Deleting it silently would have been worse than leaving it: those errors would fall
through to `network` on the "timed out" wording, or to `syntax` on the exit-code
prior, and both read as a diagnosis this table cannot make. So the removal is stated
and an unattributable server-side cancellation classifies as `other`.
agami-query/SKILL.md loses its `timeout` retry row for the same reason and says what
replaced it: a statement stopped for taking too long is a `resource_limit` REFUSAL,
which carries its own remediation because it is a decision the server made.
Verified unchanged, no edit needed: agami-serve/SKILL.md's "exit code 3" is still
correct; `semantic_model/cli.py` and `introspect.py` relay `proc.stderr` with no code
branching, so despite the spec naming `semantic_model.cli` as a consumer to update
there was nothing to update.
Also verified that functions-only does not regress `/agami-connect`: introspection
reads `information_schema` and `pg_class` as RELATIONS and casts with `::bigint`,
which is not a register type. All pass the recon gate.
Spec: ACE-039
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found these; all four were reproduced before being fixed. **1. The fork relayed the model's own notices to the caller (security).** `_child_failure_message` returned the child's WHOLE stderr for any classified code. But stderr is a SHARED stream and `_model_safety` writes to it before execution — `[agami] applied default_filters: ["orders.tenant_id = '…'"]` — with the sanitized sentence appended after. So the caller received a declared row-level tenancy predicate it never sent, on the DEFAULT transport, while the in-process path returned the clean sentence. The fork/in-process parity this work exists to establish was false exactly where it mattered, and the traceback guard only ever caught the two `exc_info=True` sites, so any library logging to stderr had the same reach. The parent now REBUILDS the message from the exit code for the sanitized band. The child derives that sentence from the kind, so the code is all the parent needs — which makes the stream irrelevant to the answer rather than filtered. Codes 2 and 3 are still relayed: that text is authored here and is the remediation a misconfigured operator needs. **2. An unterminated `"` suppressed the keyword inside it (security).** The scan recorded a span whether or not it consumed a closing delimiter, so a runaway identifier produced one span over the whole tail and the niladic matcher skipped every keyword in it — the under-refusal direction. Reachable rather than theoretical: MySQL's default sql_mode treats `"` as a string delimiter with backslash escapes, so `SELECT "a\"b" , current_user FROM t` is a statement MySQL runs and answers with CURRENT_USER() while the neutralizer reads a runaway identifier. No closing delimiter now means no span, which is the safe direction the surrounding code already argues for. **3. Schema-qualified register casts walked through.** `'secret_table'::pg_catalog.regclass` is valid PostgreSQL, is the same object-existence oracle, and names no relation for the model gate to bite on. Both cast arms anchored the type name immediately after `::` / `AS`. They now accept an optional qualifier, which also covers `::"pg_catalog"."regclass"` because the neutralizer has unwrapped the quotes by then. **4. Six of ten engines were mis-classified.** SQL Server, Trino, Databricks and BigQuery not-found errors fell to the exit-5 prior and read as `syntax` — telling the caller to re-run the identical failing statement instead of to re-introspect a drifted model. And a bare "access denied" needle in the `auth` arm swallowed BigQuery, Trino, SQL Server and MySQL 1044 authorization failures, so the operator was told to re-credential when the fix is a GRANT, and on `syntax` the skill auto-retried the same query twice. The not-found and permission arms now carry one needle per engine, and `auth` narrows to genuine authentication wording. Two shipped assertions invert as a consequence of (1), deliberately: exit 4 is no longer "the child's own diagnostic" but the sanitized band, and this slice's own codes 7-10 relay test becomes a rebuild test. Spec: ACE-039 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebasing onto ACE-038 (#174, `961c6c3`) surfaced two interactions no unit test on either branch could see alone. Both are correct-to-change; the enumeration-matrix conflict resolved by keeping BOTH vectors (16 -> 24 cells). `test_the_budget_has_exactly_one_configuration_surface` asserted `execute_sql` holds exactly one ContextVar. The hazard it names is a second INPUT to the budget that cannot cross the fork: a parent deriving the supervisor's bound from an override would compute a bound below what the child enforces, and the ordered family would invert. `_last_error_detail` is not that — it carries raw driver text OUT of an already-failed call for the audit row, is written after the budget is resolved and spent, and is never read to compute a bound. Excluded by name so the guard keeps its shape rather than being loosened to "at most two". `test_an_executor_error_keeps_its_type_across_the_worker` asserted `syntax` and the driver's text verbatim for "no such column: nope". Both change: the kind was the exit-5 prior showing through rather than a read of the text, and the message is now the classified sentence. What the test is actually about is untouched — the exception kept its TYPE across the worker thread, which is exactly what `except ExecutorError` matching proves; a re-wrap would have produced `other`. Spec: ACE-039 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sandeep-agami
force-pushed
the
ACE-039-recon-error-hardening-port
branch
from
August 1, 2026 18:36
063d0ae to
481044a
Compare
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
Two leakage channels at the SQL executor, both crossing the LLM boundary.
Recon. Under read-only plus object scope a statement could still call
version(),current_user,has_table_privilege(…)or'x'::regclassto fingerprint the server or probe for objects. A model declares tables and columns and never declares functions, so object scope had nothing to reject.Driver text.
failure.messagewas relayed from the driver verbatim, and PostgreSQL volunteers declared column names in aHINT. Measured on9d3fd8dbefore any code:orders.internal_refis declared and the caller never sent it. After: kindcolumn_not_found, message a fixed value-free sentence.tests/test_ace035_no_enumeration.pycarried this as anxfail(strict=True); it flips green here and the marker is removed in the same commit.Ported from #120, which is superseded and closes with this. That branch predates the rewritten contract by eleven review deltas and has no mergeable base.
Changes
Seven commits, each a green checkpoint.
S1
_neutralizereports its quoted spans, in one coordinate frame. The recon gate needs to tellSELECT "current_user" FROM audit_log(a column) fromSELECT current_user(the keyword), and the neutralizer drops quote delimiters on purpose. Returns a stdlibNamedTupleof already-stripped text plus spans in that text's coordinates, so nothing reconciles frames. A span the strip disturbed is dropped, never clamped — dropping means the matcher does not skip, so the gate over-refuses.S2 four exit codes. The child classifies at the chokepoint and
maincollapses the kind to an exit code;column_not_found,table_not_found,permissionandnetworkhad none, so all four becameotheracross the fork while in-process reported the truth. Behaviour-neutral on its own.S3 the recon gate, functions only. Catalog relations stay
check_table_scope's job. Three things the reference did not have: the paren matcher is the union of call names and niladic keywords (without itSELECT "current_schema"()slips both matchers);has_\w+_privilegenarrows to the fourteen real builtins; register casts are covered in both spellings.REASON_FOR_RULE[RULE_RECON] = "unsafe".S4 the classifier. Codes 2 and 3 are authored here and still relayed; codes 4 and 5 carry the driver's exception and are classified from it, never returned. Cancellation is ceded to ACE-038 but the signatures are kept as an explicit
→ otherarm, because deleting them mis-classifies rather than declassifies.S5 migration 016 +
error_detail. A column onquery_executions, not a second table — 014's argument, andaudit_idisquery_executions.id.S6 the two consuming documents.
S7 four defects from review, below.
Decisions worth a reviewer's eye
_model_safetywrites[agami] applied default_filters: […]to it before execution, so the caller received a declared row-level tenancy predicate it never sent — on the default transport, while in-process returned the clean sentence. Since the child derives its message from the kind, the exit code alone rebuilds it, which makes the stream irrelevant rather than filtered. This one most wants a nod."records no span. It previously produced one span over the whole tail and the niladic matcher skipped every keyword inside it. Reachable: MySQL's defaultsql_modetreats"as a string delimiter with backslash escapes, soSELECT "a\"b" , current_user FROM tis a statement MySQL runs and answers withCURRENT_USER()._RAW_LOGis silenced for the CLI lifetime, so the child logs nothing and the column is NULL. This narrows the spec's earlier decision that the child would log server-side — doing so put the text back on stderr, which the parent relayed. An accepted gap, stated rather than claimed._neutralize's equality test compares.text. A deliberate contract change; the six cases and the property are untouched.Verification
uv run dev.py checkgreen — 2263 passed, 1 skipped, 0 xfailed (the strict xfail is gone with the gap it measured). From 2064 at the ACE-035 base.Every criterion measured on
9d3fd8dfirst, so the delta is shown rather than claimed:refuse('recon')raisedKeyError;check_no_recondid not exist;EXIT_TO_FAILURE_KIND.get(7)wasNoneandtools._classify_exit(7)returnedother;query_executionshad 12 columns;_neutralizereturnedstr; and the HINT leak was driven end to end.c42f96ccorpus is re-asserted in the same commit that changes_neutralize. Security review additionally fuzzed_neutralize(sql).texton HEAD against_neutralize(sql).strip()on main over 647,988 inputs: 0 divergences.Checklist
uv run dev.py checkgreen (ruff · pytest · gitleaks · lib-drift)## DecisionsVerified against real PostgreSQL 16.12
Run after the rebase, via
context/local-e2e-guardrail-testbed.md— real database, the shippedread-only grant recipe applied verbatim, the shipped synthetic sample.
The oracle is real, and the engine cannot stop it. As
agami_rothrough plainpsql, app guardentirely out of the loop:
Exists and does-not-exist are distinguishable, and the least-privilege role reads
version()andcurrent_userfreely while the engine does blockpg_read_file. That is F9's Q1 finding measuredrather than asserted, and it is the whole reason recon must be denied app-side.
Through the guard, on that same engine: all six recon vectors refuse as
recon, including'salaries'::pg_catalog.regclass— the qualified-cast gap review found, closed and proven where theoracle demonstrably works. Both welded forms from
c42f96crefuse asread_only; stacked statementsand a CTE-wrapped
DELETEtoo;pg_shadowrefuses astable_scope, which is functions-only workingas designed. A legitimate aggregate returns real rows.
A genuine PostgreSQL HINT, closed:
A real
UndefinedColumnreturns exit 7 with the identifier gone. That code exists only because ofS2, so the fork encoding is verified against a real engine rather than a canned string.
No
/agami-connectregression — introspection ran clean under the read-only role with the gate inplace. Also smoked on the packaged plugin layout under stock Python 3.9.6 with
-S, where a realdeclared column named
current_userreturns rows (the false-positive fix, end to end).Notes for the reviewer
main@961c6c3(ACE-038 ACE-038: the per-statement timeout — one budget, four ordered bounds, one boolean #174 has merged). The predicted_MATRIXconflictresolved by keeping both vectors, 16 -> 24 cells. The rebase then surfaced two ACE-038/ACE-039
interactions no unit test on either branch could see alone — see S8; both are correct-to-change and
neither weakens what its test was about.
Databricks, BigQuery, Oracle and the rest are exercised only by driver text in the corpus. That is
the honest edge of this verification.
are +1002, the vendored mirror +518 (machine-generated by
sync-lib), ~330 comments anddocstrings. Had it been split, the cut is the spec's own "two independent hardenings": S1+S3
(recon) and S2+S4+S5+S6 (sanitization); they share no code.
fixed in S7, two of them leaks. No human has read the diff yet.
Spec: ACE-039