Repository navigation
docs(agents): record the dual-reading invariant, correct the EXPLAIN tx claim - #63
Merged
Merged
Conversation
…tx claim Two gaps in the guidance file, both security-relevant. The redaction paragraph still described Redact as handling "single-quoted string + numeric literals". It now also covers backslash escapes, Postgres dollar-quoted strings including non-ASCII tags, and hex/binary numerics, and it deliberately leaves double-quoted runs as identifiers (issue #62). More importantly, nothing recorded that Redact and IsMultiStatement scan under more than one dialect reading, or why neither may be collapsed to a single pass. AGENTS.md already protects driver.go's optional interfaces with an explicit "do not simplify these away"; the same protection was missing here, and three separate bypasses were introduced during PR #61 by exactly that kind of simplification. The new paragraph states both fail-safe directions, names a payload each reading is blind to, and points at the two tests that pin them. The EXPLAIN paragraph claimed every plan runs in BeginTx(ReadOnly:true) for "Postgres and MySQL". explain.go:243 is ReadOnly:!dml — MySQL and MariaDB reject every statement inside a read-only transaction (error 1792), so the --allow-dml path is read-write. That predates PR #61, but it is the fact the separator check's whole rationale depends on: an agent trusting the old wording would conclude a missed separator is harmless. Corrected, with the consequence spelled out, and the trailing "read-only tx" in the same paragraph fixed to match. Claims verified against analyzer/redact.go and explain/explain.go.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: KARTIKrocks/sqlguard/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ariant AGENTS.md said both Redact and IsMultiStatement scan under multiple readings of both the backslash and the "$$" question. They do not: literalSpans calls scanDollarQuoted unconditionally, so both of Redact's passes recognise dollar quotes and differ only on backslash escapes, while blankLiterals' flag is dollarQuotes and IsMultiStatement never honors backslash escapes at all. Each function scans twice over a *different* axis. The text now says which, and why Redact needs no second "$$" reading: honoring one can only add a redact span, so that ambiguity has no leak direction. The bot configs had a scope blind spot that matches where the bugs actually landed. Both .greptile/config.json and .coderabbit.yaml protect the multi-statement guard under explain/**, but IsMultiStatement lives in analyzer/redact.go and its blanker in analyzer/fallback.go — all three bypasses in PR #61 were edits to analyzer/, outside the scope of the rule meant to catch them. Added a dual-reading-lexer rule scoped to those two files, extended the coderabbit analyzer/** instructions to match, and wrote the three bugs up as a worked example in .greptile/rules.md, which exists for exactly that. Worth noting both bot configs already documented BeginTx(ReadOnly: !dml) correctly — AGENTS.md was the only file that had it wrong, which the previous commit fixed.
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.
Summary
What does this PR change, and why?
Two gaps in the guidance file, both security-relevant.
The redaction paragraph still described Redact as handling "single-quoted string + numeric literals". It now also covers backslash escapes, Postgres dollar-quoted strings including non-ASCII tags, and hex/binary numerics, and it deliberately leaves double-quoted runs as identifiers (issue #62).
More importantly, nothing recorded that Redact and IsMultiStatement scan under more than one dialect reading, or why neither may be collapsed to a single pass. AGENTS.md already protects driver.go's optional interfaces with an explicit "do not simplify these away"; the same protection was missing here, and three separate bypasses were introduced during PR #61 by exactly that kind of simplification. The new paragraph states both fail-safe directions, names a payload each reading is blind to, and points at the two tests that pin them.
The EXPLAIN paragraph claimed every plan runs in BeginTx(ReadOnly:true) for "Postgres and MySQL". explain.go:243 is ReadOnly:!dml — MySQL and MariaDB reject every statement inside a read-only transaction (error 1792), so the --allow-dml path is read-write. That predates PR #61, but it is the fact the separator check's whole rationale depends on: an agent trusting the old wording would conclude a missed separator is harmless. Corrected, with the consequence spelled out, and the trailing "read-only tx" in the same paragraph fixed to match.
Claims verified against analyzer/redact.go and explain/explain.go.
Closes #
Type of change
Checklist
make cipasses (fmt-check, vet, lint, vuln, test-race, lint-docs) across all moduleswebsite/docs/with a version marker for anything new (_0.3+_,_Added in 0.3._,// 0.3+) — neverwebsite/versioned_docs/AGENTS.md/.sqlguard.example.ymlif a convention or config key changed## [Unreleased]inCHANGELOG.mdanalyzer/middleware/reporterResult)Notes for reviewers
Anything reviewers should focus on — tricky areas, trade-offs, follow-ups.