feat: Guarded SQL gateway — dry-run writes, cost gating, row caps (PGC-101)#15
Merged
Conversation
…w caps - Built: rewrote `sql_cmd.rs` as a guarded gateway. Writes now preview by default — `--allow-write` runs the statement in a transaction, reports affected count + a sample of affected rows (single-DML RETURNING wrapper), then ROLLS BACK. `--commit` (implies --allow-write) is the only apply path. Statement classification via sqlparser (DML / transactional DDL / non-transactional like CREATE INDEX CONCURRENTLY, VACUUM, REINDEX). EXPLAIN cost gate warns above a configurable threshold (`[sql] cost_warn_threshold`, default 50k; `--no-cost-check` to skip). SELECT output capped at 1000 rows with a trailer (`--limit N`, `--limit 0` uncaps), in both human and --json forms. Switched the sql path from a bare connect() to DiagnosticSession so statement/lock/connect timeouts and Ctrl+C cancellation now apply (previously: no timeout at all). - Validation: cargo fmt --check, cargo clippy --all-targets (clean for new code), cargo test --no-fail-fast — 736 passed / 0 failed. Live-verified provable rollback (dry-run leaves data unchanged; --commit changes it), cost warning via low threshold, row cap + trailer, DDL dry-run, and the non-transactional --commit-required path. - Notes: sqlparser 0.58 can't parse VACUUM/REINDEX, so those are detected by leading keyword and routed to the "requires --commit" message rather than failing with a parse error.
- Built: rewrote the Querying section (new "Writes preview themselves" subsection: --allow-write = dry-run + rollback, --commit = apply, cost warning, non-transactional caveat, row caps + timeouts), the "Why pgcrate" write bullet, the Safety conventions (--allow-write is safe to reach for; treat --commit like --yes), the frontmatter description, and the command reference table (split write into preview/apply rows). Only documents grammar verified against the built binary. - Validation: skill frontmatter embedded tests pass (name/description/---).
Critical: writes hidden inside Statement::Query / Statement::Explain were classified as Read and, under --allow-write, executed in autocommit with no transaction wrapper and no rollback — destroying data the user asked only to preview. Live-confirmed against postgres for all four: - Fixed: writable CTEs (WITH d AS (DELETE ... RETURNING *) SELECT *) now classify as writes (recursive query_writes over CTEs/set-ops/subqueries). - Fixed: leading-CTE DML (WITH x AS (...) UPDATE ...) — top-level SetExpr DML body now detected. - Fixed: SELECT ... INTO new_table now classified as a write. - Fixed: EXPLAIN ANALYZE <write> (and parenthesized EXPLAIN (ANALYZE), which parses analyze=false yet still executes) now classified as a write via inner- statement recursion. - Added: 9 classifier unit tests + 5 live integration tests asserting provable rollback (data survives --allow-write) and commit semantics for these cases. - Added: structured cost estimate to --json write output (write.cost: estimated/threshold/exceeds_threshold) so JSON consumers get the same signal humans see on stderr; cost_gate refactored to return CostEstimate. - SKILL.md: documented hidden-write coverage, the JSON cost field, and the setval/nextval static-detection limitation (unsolvable statically; read-only backstop covers the no-flag path). - Noted: side-effect functions in a SELECT (setval/nextval) can't be caught statically; under --allow-write their effect isn't rolled back (documented). - Noted: sql errors/blocked-writes exit 10 (OPERATIONAL_FAILURE), not 2 — pre-existing binary-wide behavior, out of scope. 736 baseline -> 748 tests green. fmt clean, no new clippy warnings.
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
1 participant
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.
The core safety feature of the agent-wedge phase:
pgcrate sqlwrites preview themselves by default.--allow-write= transactional dry-run: execute, report rows affected + sample, ROLLBACK.--commit(implies allow-write) applies.SELECT INTO,EXPLAIN ANALYZE <write>, nested CTEs — all caught and routed to the guarded path (review found these as live bypasses; fixed + regression-tested).default_transaction_read_only=onat the connection.[sql] cost_warn_threshold, default 50k) — stderr warning for humans, structuredwrite.costobject in JSON.--limit),truncatedfield in JSON.sqlmoved onto DiagnosticSession — statement/lock/connect timeouts now actually apply (previously a bare connect with no timeouts at all).--commitexplicitly.Validation: 748 tests green (736 baseline + 12 new, incl. provable-rollback contract and bypass regressions); every bypass live-verified fixed. Independently reviewed: approved-with-fixes.