From 0dbe62502428694fa7860724232cb977967dcb00 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 17:13:05 +0530 Subject: [PATCH 1/3] chore: configure CodeAnt AI and sync the other reviewer configs CodeAnt reads repository configuration from .codeant/, resolving inline CI parameters over that directory over dashboard settings. Checking it in keeps the review policy reviewable like any other change, and matches how .coderabbit.yaml and .greptile/ are already maintained here. review.json 14 rules, ported from the invariants in AGENTS.md and .greptile/config.json, with the ids kept identical across tools so one invariant has one name wherever it is reported instructions.json 9 context entries aimed at the false positives this codebase reliably produces: the //nolint deprecated delegations, explain's raw-Query carve-out, the CLI's blank-imported drivers, the exported API of a library, the throwaway credentials in the test compose file configuration.json analyses and file scope; deadcode and duplicate-code analysis are off because golangci-lint's `unused` already answers the first correctly for a library, and the second would report the deliberately parallel driver branches and integration adapters quality_gates_conditions.json secrets, sast_rating and sca_rating; no coverage gate, since coverage goes to Codecov and CodeAnt has no upload wired here README.md what is enforced and why, per CodeAnt's own guidance on documenting a config Analysis scope and review scope are filtered separately: go.mod and go.sum stay in analysis scope because that is how SCA sees the dependency graph, but go.sum, package-lock.json and testdata are dropped from line-by-line review. The other configs are brought up to date with them. .coderabbit.yaml's middleware/driver.go instruction described the old ErrSkip-only rule; .greptile gains a matching analyze-once-per-execution rule and a worked example of the double-analysis bug. AGENTS.md now records that four reviewer configs exist and that an invariant belongs in all of them. --- .codeant/README.md | 109 +++++++++++++++++++++++++ .codeant/configuration.json | 42 ++++++++++ .codeant/instructions.json | 58 +++++++++++++ .codeant/quality_gates_conditions.json | 26 ++++++ .codeant/review.json | 88 ++++++++++++++++++++ .coderabbit.yaml | 18 +++- .greptile/config.json | 8 +- .greptile/rules.md | 45 ++++++++++ AGENTS.md | 31 +++++++ 9 files changed, 422 insertions(+), 3 deletions(-) create mode 100644 .codeant/README.md create mode 100644 .codeant/configuration.json create mode 100644 .codeant/instructions.json create mode 100644 .codeant/quality_gates_conditions.json create mode 100644 .codeant/review.json diff --git a/.codeant/README.md b/.codeant/README.md new file mode 100644 index 0000000..4fbd870 --- /dev/null +++ b/.codeant/README.md @@ -0,0 +1,109 @@ +# CodeAnt AI configuration + +Repository-level configuration for [CodeAnt AI](https://docs.codeant.ai), +checked in so the settings are reviewed like any other change instead of +living only in a dashboard. CodeAnt resolves configuration as **inline CI +parameters > this directory > dashboard settings**, and each level overrides +only the fields it defines. + +This is the third reviewer configured for this repo, alongside +`.coderabbit.yaml` and `.greptile/`. The rule ids below deliberately match +`.greptile/config.json` where the rule is the same, so one invariant has one +name across every tool. + +| File | Purpose | Reference | +| --- | --- | --- | +| `configuration.json` | Which analyses run, and over which files | [Analysis Configuration](https://docs.codeant.ai/repositories/analysis_configuration) | +| `review.json` | Repo-specific review rules, merged by `id` | [Rules](https://docs.codeant.ai/pull_request/customize/rules) | +| `instructions.json` | Context that prevents false positives, merged by `id` | [Instructions](https://docs.codeant.ai/pull_request/customize/instructions) | +| `quality_gates_conditions.json` | Conditions that gate a PR | [Quality Gates](https://docs.codeant.ai/pull_request/quality_gates/repository_configuration) | + +Anything an organization-wide global config repo defines is merged in +underneath these files; a local `id` or `metric` always wins. + +## What is enabled, and why + +Two analyses are off. Both are switched off because CI already answers the +same question more accurately for this codebase, not because the question +does not matter: + +- **`deadcode_analysis`** — this is a published library, so an exported + identifier with no in-repo caller is the public API rather than dead code. + `golangci-lint`'s `unused` runs in `make ci` and understands that + distinction; a generic reachability pass would report the whole exported + surface, plus the optional driver-interface methods in + `middleware/driver.go` that exist so `database/sql`'s type assertions + succeed. +- **`duplicatecode_analysis`** — the four `Query`/`Exec` branches in + `middleware/driver.go` and the six `integrations/*` adapters are + deliberately parallel; each branch forwards to a different optional base + interface, and collapsing them changes fallback behaviour. AGENTS.md + records this as an invariant. + +Everything else stays on. `sast_analysis`, `secrets_analysis`, `sca_analysis` +and `iac_analysis` matter most here: sqlguard is a defensive security tool, +and `SECURITY.md` treats a missed dangerous query, a leaked literal or an +executed `EXPLAIN` as vulnerabilities rather than style nits. +`complex_function_analysis` keeps the default maintainability index of 15, +which pairs with `gocyclo`'s `min-complexity: 15` in `.golangci.yml`. + +## File filters + +Two separate keys, because they scope different things: + +- `file_filters.config.exclude_files` scopes **analysis**. It drops build + output, `node_modules`, the frozen docs snapshots and the static assets. + `go.mod` and `go.sum` stay in scope — they are how software composition + analysis sees the dependency graph. +- `review_configuration.exclude` scopes **PR review**, and additionally drops + `go.sum`, `package-lock.json` and `testdata`, where a line-by-line AI + comment has nothing useful to say. + +`include_files` is left empty on purpose: when it is set it takes precedence +and the exclude patterns are ignored entirely. + +`website/versioned_docs/**` and `website/versioned_sidebars/**` are excluded +from both. They are frozen release snapshots produced by +`npm run cut-version`; a suggestion there is unactionable by definition, +since editing a snapshot rewrites history for users still on that version. + +## Quality gates + +Security-first and deliberately small, so a red gate always means something: + +| Metric | Condition | Effect | +| --- | --- | --- | +| `secrets` | `GREATER_THAN 0` | Any new secret fails the commit or PR | +| `sast_rating` | `LESS_THAN B` | Requires A or better (fails on a medium-or-worse finding) | +| `sca_rating` | `LESS_THAN B` | Same bar for dependency vulnerabilities | + +The secrets condition excludes `test/integration/docker-compose.yml`, whose +user, password and database are all the literal string `sqlguard`. Those are +fixtures for ephemeral local containers on deliberately non-default host +ports, used by `make db-up`; they are not a credential for anything. The +exclusion is scoped to that one file so a real secret anywhere else still +fails the gate. + +Coverage metrics (`new_coverage_percentage`, `total_coverage_percentage`) are +**not** configured. Coverage in this repo is merged by `make coverage` and +uploaded to Codecov (`codecov.yml`); CodeAnt would need its own +[coverage upload step](https://docs.codeant.ai/control_center/test_coverage/github) +in `ci.yml` plus an API token before such a gate could pass, and a gate that +cannot pass is worse than no gate. Add the upload first if you want one. + +Auto-approval (`.codeant/approval.json`) is also not configured. `main` takes +changes through pull requests that a human merges, so nothing here should +approve itself. + +## Changing this + +- Rules and instructions are merged **by `id`** — keep ids stable and + descriptive so a global config, or a future override, can address them. +- A rule says what the reviewer should *enforce*; an instruction gives context + that stops it reporting something deliberate. Adding an instruction is + usually the right fix for a false positive. +- `scope` must contain `"pr"` for a rule to apply during pull request review; + `"ide"` applies it in the editor extensions. +- Prefer findings CI cannot already produce. `make ci` runs gofmt, `go vet`, + `golangci-lint`, `govulncheck`, `go test -race` and markdownlint across all + nine modules, and a separate workflow runs CodeQL. diff --git a/.codeant/configuration.json b/.codeant/configuration.json new file mode 100644 index 0000000..d89ac23 --- /dev/null +++ b/.codeant/configuration.json @@ -0,0 +1,42 @@ +{ + "code_analysis": { + "enabled": true, + "features": { + "sast_analysis": "enabled", + "secrets_analysis": "enabled", + "sca_analysis": "enabled", + "iac_analysis": "enabled", + "antipatterns_analysis": "enabled", + "docstring_analysis": "enabled", + "complex_function_analysis": "enabled", + "deadcode_analysis": "disabled", + "duplicatecode_analysis": "disabled" + }, + "config": { + "complexity": { + "maintainability_index": 15 + } + } + }, + "file_filters": { + "config": { + "include_files": "", + "exclude_files": "bin/**,dist/**,**/node_modules/**,website/build/**,website/.docusaurus/**,website/versioned_docs/**,website/versioned_sidebars/**,website/static/**" + } + }, + "review_configuration": { + "exclude": [ + "bin/**", + "dist/**", + "**/node_modules/**", + "**/package-lock.json", + "**/go.sum", + "**/testdata/**", + "website/build/**", + "website/.docusaurus/**", + "website/versioned_docs/**", + "website/versioned_sidebars/**", + "website/static/**" + ] + } +} diff --git a/.codeant/instructions.json b/.codeant/instructions.json new file mode 100644 index 0000000..5b48faf --- /dev/null +++ b/.codeant/instructions.json @@ -0,0 +1,58 @@ +{ + "instructions": [ + { + "id": "repo-context", + "description": "sqlguard (github.com/KARTIKrocks/sqlguard) is a near-zero-dependency, security-conscious SQL query analyzer for Go: a database/sql driver-layer middleware, a static CLI scanner and an EXPLAIN-plan analyzer sharing one analyzer/reporter core. It is a defensive security tool - a bug that lets it silently miss a dangerous query, leak a raw literal, or actually execute a statement during EXPLAIN is a security bug rather than a style nit (see SECURITY.md). Prioritise fail-closed behaviour, PII redaction, concurrency safety and not altering query semantics over style. The project is pre-release with no backward-compatibility guarantee: prefer the clean redesign over preserving an existing public API, and do not suggest deprecation shims, compat layers or add-alongside variants. AGENTS.md is the authoritative description of the architecture and its invariants; read it before proposing a structural change.", + "files": ["**/*"], + "scope": ["pr", "ide"] + }, + { + "id": "deliberate-driver-chain-shape", + "description": "middleware/driver.go hand-implements the standard database/sql wrapping chain (Driver + DriverContext, Connector, Conn, Stmt, Tx) with zero dependencies. Its parallel Query/Exec branches and its per-type optional-interface methods are deliberately repetitive: each branch forwards to a different optional base interface, and collapsing them changes fallback behaviour. Do not report this file for duplication, and do not report the //nolint:staticcheck deprecated delegations - they are required for a faithful wrapper and each carries an explanation (nolintlint enforces that).", + "files": ["middleware/driver.go", "middleware/*_test.go"], + "scope": ["pr", "ide"] + }, + { + "id": "explain-keeps-query-raw", + "description": "The explain package deliberately keeps Result.Query - and the Query of the analyzer.Results it builds - RAW rather than redacted, because the user typed the statement on their own CLI and it never reaches a log or telemetry sink. Fingerprint is still always set. Do not report explain findings as a redaction miss. Likewise, the EXPLAIN string is built by concatenation because EXPLAIN takes no bind parameters; do not suggest parameterizing it.", + "files": ["explain/**/*.go"], + "scope": ["pr", "ide"] + }, + { + "id": "cli-driver-imports-are-intentional", + "description": "cmd/sqlguard/db.go blank-imports github.com/jackc/pgx/v5/stdlib and github.com/go-sql-driver/mysql so `sqlguard explain` can connect. Only this package imports them, and Go links per imported package, so a library consumer of analyzer or middleware never links them. Do not report these as a core-dependency violation and do not suggest moving them into explain/ - that WOULD put database drivers in the library import graph.", + "files": ["cmd/sqlguard/**/*.go"], + "scope": ["pr", "ide"] + }, + { + "id": "library-not-application", + "description": "This is a published library, so exported identifiers with no in-repo caller are the public API, not dead code - consumers are the callers. The same applies to the optional driver interface methods in middleware/driver.go, which exist so database/sql's type assertions succeed. golangci-lint's `unused` runs in CI and understands these Go semantics; prefer its verdict over a generic reachability heuristic.", + "files": ["**/*.go"], + "scope": ["pr", "ide"] + }, + { + "id": "test-conventions", + "description": "Run anything touching middleware with -race: the driver chain, QueryTracker and the caches are concurrent. A test for a fixed bug should prove the failure mode - the convention here is that a regression test must fail against the unfixed code, and several assert exact analysis counts for that reason. errcheck, gosec, bodyclose, errorlint, noctx and perfsprint are intentionally relaxed in _test.go files (see .golangci.yml exclusions), so do not report those there. test/integration is a separate, unpublished module behind the `integration` build tag so `go test ./...` stays Docker-free; do not suggest removing the tag, folding it into the core module, or replacing its live-database assertions with mocks - the tabular EXPLAIN output it pins is server-version-dependent, which is exactly why it cannot be a unit test.", + "files": ["**/*_test.go", "test/integration/**/*"], + "scope": ["pr", "ide"] + }, + { + "id": "test-fixture-credentials", + "description": "test/integration/docker-compose.yml contains throwaway credentials (user, password and database all 'sqlguard') for local, ephemeral containers on deliberately non-default host ports. They are fixtures for `make db-up`, are never used against a real server, and are not a leaked secret. Report a credential in this file only if it looks like a real one.", + "files": ["test/integration/docker-compose.yml"], + "scope": ["pr", "ide"] + }, + { + "id": "docs-site-conventions", + "description": "The docs site is Docusaurus under website/, linted and formatted by Biome (website/biome.json) and type-checked by TypeScript; prose is linted repo-wide by markdownlint (.markdownlint-cli2.jsonc) via `make lint-docs`, not by Biome. website/docs is the unreleased tree and website/versioned_docs holds frozen snapshots that are cut with `npm run cut-version` and never hand-edited - do not propose changes there. Node-side files (docusaurus.config.ts, sidebars, scripts) must not use browser APIs, and both docusaurus.config.ts and scripts/cut-version.mjs read MAX_LIVE_VERSIONS from versions.config.json rather than duplicating it.", + "files": ["website/**/*"], + "scope": ["pr", "ide"] + }, + { + "id": "ci-already-covers", + "description": "`make ci` runs gofmt/goimports checks, go vet, golangci-lint (v2 schema, .golangci.yml - including gosec, staticcheck, unused, gocyclo at min-complexity 15, revive's exported rule, errorlint, bodyclose, nilerr, contextcheck and the allocation linters), govulncheck, `go test -race`, and markdownlint, across all nine modules; a separate workflow runs CodeQL. Prefer findings those tools cannot produce - design, invariant and cross-module reasoning - over restating a lint that already gates the build.", + "files": ["**/*"], + "scope": ["pr", "ide"] + } + ] +} diff --git a/.codeant/quality_gates_conditions.json b/.codeant/quality_gates_conditions.json new file mode 100644 index 0000000..c838b22 --- /dev/null +++ b/.codeant/quality_gates_conditions.json @@ -0,0 +1,26 @@ +{ + "quality_gate": { + "enabled": true, + "conditions": [ + { + "metric": "secrets", + "operator": "GREATER_THAN", + "value": "0", + "scope": ["commit", "pull_request"], + "exclude_files": ["test/integration/docker-compose.yml"] + }, + { + "metric": "sast_rating", + "operator": "LESS_THAN", + "value": "B", + "scope": ["pull_request"] + }, + { + "metric": "sca_rating", + "operator": "LESS_THAN", + "value": "B", + "scope": ["pull_request"] + } + ] + } +} diff --git a/.codeant/review.json b/.codeant/review.json new file mode 100644 index 0000000..7f81f17 --- /dev/null +++ b/.codeant/review.json @@ -0,0 +1,88 @@ +{ + "rules": [ + { + "id": "fail-closed-on-unknown-shape", + "description": "Code that interprets an external, version-dependent shape (an EXPLAIN plan's columns, a SQL parser's output, a config file) must fail closed - return an error - when the shape is not what was expected, never silently degrade to 'found nothing'. A false negative in a query linter is worse than a hard failure. This was a real bug: a MySQL 9 EXPLAIN plan missing an expected column made every column lookup return the empty string and Analyze reported zero issues instead of erroring. Treat a change that removes a required-column check or turns an error into a default value as high severity.", + "files": ["explain/**/*.go", "analyzer/**/*.go", "config/**/*.go"], + "scope": ["pr", "ide"] + }, + { + "id": "redaction-default", + "description": "Redaction is the default and there is exactly one canonical normalizer. Before any analyzer.Result leaves the process, Result.Query must be redacted through analyzer.Redact (unless the caller explicitly set rawQuery / WithRawQuery) and Result.Fingerprint must always be set, PII-free and low-cardinality, because it is used as a metric label. Never add a second normalizer - middleware.normalizeQuery delegates to analyzer.Fingerprint and the N+1 group key IS the fingerprint. Directly-built findings (slow-query, n-plus-one) must go through Analyzer.PrepareQuery. The explain package is the single deliberate exception: it keeps Result.Query raw because the user typed the query on their own CLI. Do not flag that, and do not extend the exception anywhere else.", + "files": ["analyzer/**/*.go", "middleware/**/*.go", "integrations/**/*.go", "reporter/**/*.go"], + "scope": ["pr", "ide"] + }, + { + "id": "dual-reading-lexer", + "description": "Redact and IsMultiStatement scan SQL literals under more than one dialect reading and neither may be collapsed to a single pass. Redact must never leave a literal byte in its output: it always treats $tag$...$tag$ as a literal and varies only the backslash reading (\\' escapes in MySQL's default sql_mode and in Postgres E'...' strings, but not under standard_conforming_strings), redacting the UNION of both readings so ambiguous SQL is over-redacted. IsMultiStatement must never miss a ';': it never honors backslash escapes and varies the $$ reading instead, refusing when ANY reading leaves a ';' outside a literal - each reading is blind to a payload the other catches (UPDATE t AS $$ SET id = 1; DROP TABLE t versus SELECT $$'$$; DROP TABLE t). stripComments and blankLiterals must agree about dollar quotes. IsMultiStatement is explain's stacked-statement guard, and the MySQL --allow-dml path runs read-write where DDL implicit-commits past the rollback, so weakening it is a security bypass, not a refactor. Flag any change that drops a reading, makes the two lexers disagree, or deletes TestRedactNoLeakAcrossDialectAmbiguity or TestIsMultiStatementNeedsBothReadings.", + "files": ["analyzer/redact.go", "analyzer/fallback.go", "analyzer/*_test.go"], + "scope": ["pr", "ide"] + }, + { + "id": "single-analysis-core", + "description": "middleware.Guard (Check / CheckLatency / Observe / ResetN1 / Analyzer) is the single analysis core. Every interception point in the driver chain and every out-of-tree integration must route through one Guard; integrations/pgxguard is the reference example. Flag any hand-rolled check or latency logic that bypasses it - that shape silently loses redaction-by-default, fingerprints, the parser seam, file config, N+1, dedup and the analysis cache. Per-query work must stay allocation-light and config-free: enable/disable, severity and settings are resolved once in NewGuard and DefaultWithProfile, never per query.", + "files": ["middleware/**/*.go", "integrations/**/*.go"], + "scope": ["pr", "ide"] + }, + { + "id": "analyze-once-per-execution", + "description": "A query must be analyzed exactly once per execution. Every interception point in middleware/driver.go analyzes AFTER calling the base driver, never before, because whether the query ran is knowable only from the base's answer. Two answers mean it did not run and database/sql re-issues the same logical query: driver.ErrSkip (the base declined a direct Query/Exec and database/sql falls back to Prepare+Query, which re-enters via wStmt - go-sql-driver/mysql returns this for every parameterized query unless interpolateParams=true) and driver.ErrBadConn (the connection was dead, and database/sql retries on another connection up to twice more). Analyzing a declined attempt multiplies one logical query by two or three; the duplicate static finding hides behind the dedup window but the inflated N+1 count silently lowers the configured threshold. Argument conversion must also run before analysis. Flag any change that moves analysis ahead of the base call or restores Guard.Observe here.", + "files": ["middleware/driver.go", "middleware/driver_fallback_test.go"], + "scope": ["pr", "ide"] + }, + { + "id": "driver-optional-interfaces", + "description": "middleware/driver.go structurally implements the optional database/sql driver interfaces (QueryerContext, ExecerContext, Pinger, SessionResetter, Validator, NamedValueChecker, ConnPrepareContext, StmtExecContext, StmtQueryContext) on every wrapper type, forwarding to the base only when the base implements that interface and otherwise returning driver.ErrSkip or a documented no-op so database/sql falls back exactly as it would for the bare driver. Do not simplify this away - it preserves base-driver behavior. The deprecated-path delegations (base.Begin, legacy Queryer/Execer, Stmt.Exec/Query) are deliberate and carry //nolint:staticcheck with an explanation; a faithful wrapper must delegate to whatever the wrapped driver exposes, so do not suggest removing them. The wrapper must never modify the SQL text or the arguments.", + "files": ["middleware/driver.go"], + "scope": ["pr", "ide"] + }, + { + "id": "core-dependency-purity", + "description": "The analyzer, middleware and reporter packages must stay free of third-party dependencies and of any YAML library. config is the only YAML-aware package and nothing may depend on it from those three, which is what keeps gopkg.in/yaml.v3 out of a library consumer's import graph. Flag any new external import added under analyzer/, middleware/ or reporter/, and any new require in the root go.mod that is not clearly CLI-only. The CLI's own deps (cobra, x/tools, sqlite3 in tests, and the pgx/mysql drivers blank-imported by cmd/sqlguard/db.go) are fine where they are, because Go links per imported package.", + "files": ["analyzer/**/*.go", "middleware/**/*.go", "reporter/**/*.go", "go.mod"], + "scope": ["pr", "ide"] + }, + { + "id": "explain-never-executes", + "description": "The EXPLAIN analyzer must never execute the statement it plans. Keep BeginTx plus an unconditional deferred Rollback, never use EXPLAIN ANALYZE, and keep validate()'s SELECT/WITH-only default (DML only behind WithAllowDML) and its analyzer.IsMultiStatement rejection - never replace that with strings.Contains(query, \";\"), which a comment or string literal defeats. The transaction is read-only everywhere EXCEPT MySQL/MariaDB under --allow-dml (BeginTx ReadOnly: !dml), because those servers reject every statement inside a read-only transaction with error 1792, including a planning-only EXPLAIN; do not restore ReadOnly:true there. EXPLAIN takes no bind parameters, so concatenation is unavoidable by design - the defense is validate() plus the always-rolled-back transaction, not parameterization. Plan columns are addressed by name, never by position (MariaDB emits 10 columns where MySQL emits 12), and rows whose table starts with '<' are derived/UNION temporaries that are skipped on purpose.", + "files": ["explain/**/*.go", "cmd/sqlguard/**/*.go"], + "scope": ["pr", "ide"] + }, + { + "id": "rules-self-register", + "description": "A new analyzer rule must be one analyzer.Register(RuleSpec{...}) call from init() in analyzer/rules.go - do not hand-maintain a rule list in Default(). Being addressable by name is what makes enable/disable, severity overrides, per-rule settings and suppressions work uniformly. Rules read the normalized analyzer.Statement produced by the configured Parser and must never re-parse or pattern-match raw SQL themselves. The seven rules with a nil Factory (slow-query, n-plus-one, seq-scan, high-cost, full-table-scan, no-index-used, filesort) are registered to be addressable even though the Analyzer never evaluates them; their owners ask RuleEnabled / RuleSeverity / RuleSettings. RuleEnabled deliberately ignores the only: whitelist - do not 'fix' that. All seven register in analyzer/rules.go and not in the packages that own them, so config can validate names while importing only analyzer.", + "files": ["analyzer/**/*.go", "config/**/*.go"], + "scope": ["pr", "ide"] + }, + { + "id": "parser-parity-and-never-break-the-query-path", + "description": "An analyzer.Parser must never let a parse failure break the caller's query path: analyzer.Analyze degrades to the FallbackParser, and a dialect parser returns a best-effort Statement (Exact=false) rather than an error. A dialect parser may only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind, the zero-dependency fallback has to learn it too - detectKind and insertColumnsListed are the pair to check - and a new structural field read by a rule needs a corpus row in TestParser_NeverAddsFindingTheFallbackDoesNot. The dialect parsers intentionally keep MaxInListLen, ImplicitCommaJoin and CartesianJoin as fallback heuristics; that carve-out is documented, not a bug.", + "files": ["parsers/**/*.go", "analyzer/fallback.go", "analyzer/parser.go", "analyzer/statement.go"], + "scope": ["pr", "ide"] + }, + { + "id": "hot-path-allocation", + "description": "analyzer.Analyze and middleware.Guard.Check run on every intercepted query in a live application. Flag a new heap allocation, map creation, regexp compilation or unbounded growth added to that path. Config resolution and rule enable/disable happen once at construction (Profile, DefaultWithProfile, NewGuard), never per query. Cached result slices are shared and read-only - do not mutate them (see Guard.report, which allocates only when a finding actually passes dedup). The analysis cache is keyed on the exact query string rather than the fingerprint on purpose, because a few rules read literal-derived facts the fingerprint folds away. Dedup and N+1 use bounded maps; keep them bounded. If an allocation there is genuinely unavoidable, say so rather than reporting it as an oversight.", + "files": ["analyzer/**/*.go", "middleware/**/*.go"], + "scope": ["pr", "ide"] + }, + { + "id": "nine-modules-in-lockstep", + "description": "This repo is nine Go modules (root, parsers/pgparser, parsers/mysqlparser, and six integrations/*) plus the unpublished test/integration module, all on the same Go version and released in lockstep. A public API change or a Go version bump must update every go.mod and .github/workflows/ci.yml together, and a dependency change must be followed by `make tidy` across all of them - tidying only the root leaves the others stale. No go.mod in this repo has a replace directive: the committed go.work is what points the satellites at this tree, so do not suggest adding one. Flag a change that touches one module's go.mod where the same change is required in the others.", + "files": ["**/go.mod", "go.work", ".github/workflows/*.yml", "Makefile"], + "scope": ["pr", "ide"] + }, + { + "id": "website-docs-version-markers", + "description": "website/docs is the unreleased documentation; website/versioned_docs holds frozen release snapshots that must never be edited, because changing one rewrites history for users still on that version. A documented API addition must carry a version marker in one of these exact forms: _X.Y+_ appended to an API table cell, _Added in X.Y._ opening a prose paragraph, or a trailing // X.Y+ comment inside a code block. Changed behaviour takes _Changed in X.Y._ plus one line on what it was before. Every option, function and rule name in the docs must match an exported identifier or a registered rule name exactly - a wrong name is a support burden, not a typo. Internal links are checked at build time and intro.md uses slug: / so its links must be file-relative.", + "files": ["website/docs/**/*.md", "website/docs/**/*.mdx", "README.md", "CHANGELOG.md"], + "scope": ["pr", "ide"] + }, + { + "id": "go-exported-docs", + "description": "Every new or newly-exported type, function, method and constant needs a doc comment starting with the identifier's own name - revive's exported rule is enabled in .golangci.yml. Type names must not stutter with their package (explain.Result, not explain.ExplainResult). Modern Go idioms are expected repo-wide: range-over-int, any over interface{}, and compile-time interface-satisfaction asserts in the form var _ I = (*T)(nil).", + "files": ["**/*.go", "!**/*_test.go"], + "scope": ["pr", "ide"] + } + ] +} diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 0b4a1d6..3c1f51c 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -114,8 +114,22 @@ reviews: and a faithful wrapper must delegate to whatever the base exposes. The optional-interface forwarding (return driver.ErrSkip / documented no-op when the base lacks an interface) preserves bare-driver behavior — do not - "simplify" it away. Analyze only on the path that actually executes - (never before an ErrSkip return) to avoid double-counting. + "simplify" it away. + A query must be analyzed exactly once per execution, so every + interception point here analyzes AFTER calling the base, never before + (analyzeExecuted), and argument conversion runs first. Whether the + query ran is knowable only from the base's answer, and two answers mean + it did not: driver.ErrSkip (the base declined a direct Query/Exec, and + database/sql falls back to Prepare+Query, re-entering via wStmt — + go-sql-driver/mysql answers this for every parameterized query unless + interpolateParams=true) and driver.ErrBadConn (the connection was dead, + and database/sql retries on another connection up to twice more). + Analyzing a declined attempt multiplies one logical query by two or + three; the duplicate finding hides behind the dedup window but the + inflated N+1 count silently lowers the configured threshold (#67). Flag + any change that moves analysis ahead of the base call or restores + Guard.Observe here — Observe is for interception points that are only + ever told a query ran, i.e. the out-of-tree integrations. - path: "integrations/**" instructions: >- diff --git a/.greptile/config.json b/.greptile/config.json index 208e0d7..3421904 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -42,9 +42,15 @@ "scope": ["analyzer/**/*.go", "middleware/**/*.go", "reporter/**/*.go", "go.mod"], "severity": "high" }, + { + "id": "analyze-once-per-execution", + "rule": "A query must be analyzed exactly once per execution, so every interception point in middleware/driver.go analyzes AFTER calling the base driver, never before (analyzeExecuted), and argument conversion (namedToValues) runs before analysis so a call rejected before it reaches the base is never counted. Whether the base ran the query is knowable only from its answer, and two answers mean it did not — after each, database/sql re-issues the same logical query and it re-enters the chain. driver.ErrSkip is a per-call answer, not only a per-driver one: a base that implements QueryerContext may still decline an individual query, and the Prepare+Query fallback then analyzes it at wStmt; go-sql-driver/mysql returns ErrSkip for every parameterized query unless interpolateParams=true, so on MySQL this is the common path rather than an edge case. driver.ErrBadConn means the connection was already dead — its contract forbids returning it when the operation may have run — and database/sql retries the whole query up to twice more, at both the conn and the stmt level. Analyzing a declined attempt multiplies one logical query by two or three: the duplicate static finding hides behind the dedup window, but the inflated N+1 count does not and silently lowers every configured threshold (issue #67). Guard.Observe (check-then-time) is correct only where the interception point is told the query ran, which is every out-of-tree integration but nothing in driver.go. Flag any change that moves analysis ahead of the base call, drops one of the two error checks, or removes the fakeErrSkipDriver/fakeBadConnDriver tests.", + "scope": ["middleware/driver.go", "middleware/*_test.go", "integrations/**/*.go"], + "severity": "high" + }, { "id": "driver-optional-interfaces", - "rule": "middleware/driver.go structurally implements optional database/sql driver interfaces (QueryerContext, Pinger, SessionResetter, NamedValueChecker, etc.) on every wrapper type, forwarding to the base only if the base implements that interface, else returning driver.ErrSkip or a documented no-op so database/sql falls back exactly as it would for the bare driver. Do not 'simplify' this away. The deprecated-path delegations (base.Begin, legacy Queryer/Execer, Stmt.Exec/Query) are deliberate and //nolint:staticcheck-annotated — a faithful wrapper must delegate to whatever the wrapped driver exposes; do not suggest removing them.", + "rule": "middleware/driver.go structurally implements optional database/sql driver interfaces (QueryerContext, Pinger, SessionResetter, NamedValueChecker, etc.) on every wrapper type, forwarding to the base only if the base implements that interface, else returning driver.ErrSkip or a documented no-op so database/sql falls back exactly as it would for the bare driver. Do not 'simplify' this away. The deprecated-path delegations (base.Begin, legacy Queryer/Execer, Stmt.Exec/Query) are deliberate and //nolint:staticcheck-annotated — a faithful wrapper must delegate to whatever the wrapped driver exposes; do not suggest removing them. The parallel Query/Exec branches are repetitive on purpose — each forwards to a different optional base interface — so do not report them as duplication.", "scope": ["middleware/driver.go"], "severity": "medium" }, diff --git a/.greptile/rules.md b/.greptile/rules.md index a01931e..b9b6311 100644 --- a/.greptile/rules.md +++ b/.greptile/rules.md @@ -96,6 +96,51 @@ therefore be an EXPLAIN bypass. `TestRedactNoLeakAcrossDialectAmbiguity` and `TestIsMultiStatementNeedsBothReadings` pin all of this; a change that deletes either test needs to justify itself. +## One execution, one analysis — worked example + +`analyze-once-per-execution` exists because `database/sql` re-issues a query +more often than the wrapper's shape suggests, and the original code analyzed +before handing the query to the base driver. That was correct only for a base +with no direct `Queryer`/`Execer` at all, which the comment there addressed. +Two other answers mean "this did not run", and both re-enter the chain: + +- `driver.ErrSkip` is a **per-call** answer, not only a per-driver one. A base + that implements `QueryerContext` may still decline an individual query, and + `database/sql` then falls back to Prepare+Query, which re-enters through + `wStmt` and analyzes there. `go-sql-driver/mysql` answers `ErrSkip` for + every parameterized query unless `interpolateParams=true`, which is off by + default — so on MySQL essentially all application traffic was analyzed + twice (issue #67). +- `driver.ErrBadConn` means the connection was already dead. Its contract + forbids returning it when the operation may have been performed, so nothing + executed, and `database/sql` retries the whole query on another connection — + twice from the pool, then once on a fresh one. A stale pool (MySQL's + `wait_timeout`, a restart, a failover) therefore produced **three** analyses + for one logical query. + +The visible damage is not the duplicate static finding — that hides behind the +default one-minute dedup window, which is why the bug went unnoticed. It is +the N+1 counter, which is not deduped: `WithN1Detection(10, …)` fired at five +real queries on MySQL, so every configured threshold was silently halved. + +The fix is one shape, applied at every interception point in `driver.go`: call +the base, then `analyzeExecuted`, which returns without analyzing when the +answer is `ErrSkip` or `ErrBadConn`. Two consequences are deliberate. Analysis +happens after execution, which is fine because nothing consumes findings +before the query runs; and the latency window is read before the rules run, so +analysis time cannot push a query past the slow-query threshold. Argument +conversion also moved ahead of analysis, so a call rejected with +"driver does not support named parameters" — which never reaches the database +— is no longer counted. + +`Guard.Observe` (check, then time) survives for interception points that are +only ever told a query ran: the out-of-tree integrations. Nothing in +`driver.go` is in that position, so restoring it there reintroduces the bug. +The regression tests are fake drivers rather than assertions on internals — +`fakeErrSkipDriver` and `fakeBadConnDriver` in +`middleware/driver_fallback_test.go` — and each fails against the unfixed code +with an exact count (2, 3, or a tripped N+1 threshold). + ## Module topology Nine Go modules (root, `parsers/pgparser`, `parsers/mysqlparser`, and six diff --git a/AGENTS.md b/AGENTS.md index e8a3f5b..8cfe16a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,3 +113,34 @@ must be file-relative (`./middleware.md`), not id-relative. - **Pre-release, no backward compatibility.** Nothing is shipped. Prefer the clean design over preserving existing public APIs; do not add deprecation shims or compat layers. When a better design presents itself, replace rather than add-alongside. - Modern Go idioms expected (range-over-int, compile-time interface-satisfaction asserts `var _ I = (*T)(nil)`, `any`). - Lint config specifics: `revive`'s `exported` rule is **enabled** — exported symbols need a doc comment (starting with the symbol name), and type names must not stutter with their package (e.g. `explain.Result`, not `explain.ExplainResult`); `gocyclo` min-complexity is 15; `errcheck` is relaxed in `_test.go`. + +## AI reviewer configuration + +Four reviewers are configured from files in this repo, and they are kept in +sync by hand: + +- `.coderabbit.yaml` — CodeRabbit: `path_filters`, per-path `path_instructions`, + and the linters it re-runs (golangci-lint, gitleaks, actionlint, markdownlint, + biome). +- `.greptile/` — Greptile: `config.json` (scoped rules with ids and severities, + ignore patterns), `files.json` (the files it should read for context), and + `rules.md` (prose rationale with worked examples from real bugs). +- `.codeant/` — CodeAnt AI: `review.json` (rules), `instructions.json` + (context that prevents false positives), `configuration.json` (which + analyses run, and over which files) and `quality_gates_conditions.json`. + `.codeant/README.md` documents each choice. +- This file, which every coding agent reads. + +**An invariant documented here belongs in all of them.** Each config is the +same knowledge aimed at a different reviewer, so a new invariant — or a +correction to one — is only half-landed if it lives in one file. Rule ids are +deliberately shared across `.greptile/config.json` and `.codeant/review.json` +(`redaction-default`, `explain-never-executes`, `analyze-once-per-execution`, +…) so one invariant has one name wherever it is reported. The same applies in +reverse: a rule that turns out to be wrong is wrong in three places. + +Prefer a rule that states the invariant and its reason over one that restates +a lint. `make ci` already gates gofmt, `go vet`, golangci-lint, govulncheck, +`go test -race` and markdownlint across all nine modules, and a separate +workflow runs CodeQL; review budget is better spent on the cross-module and +design reasoning those tools cannot do. From 3333427d443e299ec8af2dcdfd198dd3088a3ed9 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 17:25:45 +0530 Subject: [PATCH 2/3] chore: address reviewer findings on the reviewer configs CodeAnt, major: the parser rule listed parsers/**, fallback.go, parser.go and statement.go but not analyzer/analyzer.go, which is where Analyze degrades to the FallbackParser when a configured parser errors - the exact behaviour the rule exists to protect. Added it. CodeAnt, nitpick: the same rule was named parser-parity-and-never-break-the-query-path while .greptile called its equivalent parser-never-breaks-query-path, which made the README's claim that shared invariants carry one name across tools untrue. Renamed to match, and gave .greptile's rule the parity half it was missing (a dialect parser may only remove findings, and a statement kind the grammar learns the fallback has to learn too) so the shared id now names the same rule in both. CodeRabbit, minor: .greptile/rules.md described the driver fix in the present tense while middleware/driver.go still analyzes before the base call. The worked example now says the fix lands in #67. Not addressed: CodeRabbit's suggestion to add the analyze-once-per-execution invariant to AGENTS.md. It is already there on fix/errskip-double-analysis, in the Architecture section next to the driver paragraph it belongs to; adding it here would conflict with that branch. --- .codeant/review.json | 4 ++-- .greptile/config.json | 2 +- .greptile/rules.md | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.codeant/review.json b/.codeant/review.json index 7f81f17..cd1aa7e 100644 --- a/.codeant/review.json +++ b/.codeant/review.json @@ -55,9 +55,9 @@ "scope": ["pr", "ide"] }, { - "id": "parser-parity-and-never-break-the-query-path", + "id": "parser-never-breaks-query-path", "description": "An analyzer.Parser must never let a parse failure break the caller's query path: analyzer.Analyze degrades to the FallbackParser, and a dialect parser returns a best-effort Statement (Exact=false) rather than an error. A dialect parser may only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind, the zero-dependency fallback has to learn it too - detectKind and insertColumnsListed are the pair to check - and a new structural field read by a rule needs a corpus row in TestParser_NeverAddsFindingTheFallbackDoesNot. The dialect parsers intentionally keep MaxInListLen, ImplicitCommaJoin and CartesianJoin as fallback heuristics; that carve-out is documented, not a bug.", - "files": ["parsers/**/*.go", "analyzer/fallback.go", "analyzer/parser.go", "analyzer/statement.go"], + "files": ["parsers/**/*.go", "analyzer/analyzer.go", "analyzer/fallback.go", "analyzer/parser.go", "analyzer/statement.go"], "scope": ["pr", "ide"] }, { diff --git a/.greptile/config.json b/.greptile/config.json index 3421904..6d0f26c 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -62,7 +62,7 @@ }, { "id": "parser-never-breaks-query-path", - "rule": "A pluggable SQL parser (parsers/pgparser, parsers/mysqlparser) or analyzer.Parser implementation must never let a parse failure propagate as an error that breaks the caller's query path. On a parse failure, degrade to the FallbackParser's best-effort Statement (Exact=false) instead. The dialect parsers deliberately keep MaxInListLen/ImplicitCommaJoin/CartesianJoin as fallback heuristics rather than deriving them from the AST — that is a documented carve-out, not a bug to fix.", + "rule": "A pluggable SQL parser (parsers/pgparser, parsers/mysqlparser) or analyzer.Parser implementation must never let a parse failure propagate as an error that breaks the caller's query path. On a parse failure, degrade to the FallbackParser's best-effort Statement (Exact=false) instead — that degradation lives in analyzer.Analyze. A dialect parser may also only REMOVE findings, never add one: opting into a real grammar is sold as trading false positives away, and the reporting surface is identical so nothing would warn. When a parser learns a statement kind the zero-dependency fallback has to learn it too (detectKind and insertColumnsListed are the pair to check), and a new structural field read by a rule needs a corpus row in TestParser_NeverAddsFindingTheFallbackDoesNot. The dialect parsers deliberately keep MaxInListLen/ImplicitCommaJoin/CartesianJoin as fallback heuristics rather than deriving them from the AST — that is a documented carve-out, not a bug to fix.", "scope": ["parsers/**/*.go", "analyzer/**/*.go"], "severity": "high" }, diff --git a/.greptile/rules.md b/.greptile/rules.md index b9b6311..935e071 100644 --- a/.greptile/rules.md +++ b/.greptile/rules.md @@ -123,9 +123,9 @@ default one-minute dedup window, which is why the bug went unnoticed. It is the N+1 counter, which is not deduped: `WithN1Detection(10, …)` fired at five real queries on MySQL, so every configured threshold was silently halved. -The fix is one shape, applied at every interception point in `driver.go`: call -the base, then `analyzeExecuted`, which returns without analyzing when the -answer is `ErrSkip` or `ErrBadConn`. Two consequences are deliberate. Analysis +The fix lands in #67, as one shape applied at every interception point in +`driver.go`: call the base, then `analyzeExecuted`, which returns without +analyzing when the answer is `ErrSkip` or `ErrBadConn`. Two consequences are deliberate. Analysis happens after execution, which is fine because nothing consumes findings before the query runs; and the latency window is read before the rules run, so analysis time cannot push a query past the slow-query threshold. Argument @@ -136,7 +136,7 @@ conversion also moved ahead of analysis, so a call rejected with `Guard.Observe` (check, then time) survives for interception points that are only ever told a query ran: the out-of-tree integrations. Nothing in `driver.go` is in that position, so restoring it there reintroduces the bug. -The regression tests are fake drivers rather than assertions on internals — +Its regression tests are fake drivers rather than assertions on internals — `fakeErrSkipDriver` and `fakeBadConnDriver` in `middleware/driver_fallback_test.go` — and each fails against the unfixed code with an exact count (2, 3, or a tripped N+1 threshold). From cb80564cb70af33132604befc6b46a3012214f95 Mon Sep 17 00:00:00 2001 From: kartik Date: Fri, 25 Sep 2026 17:26:43 +0530 Subject: [PATCH 3/3] chore: drop the pending-fix hedge from the driver worked example The section already opens with "(issue #67)", so saying the fix lands there added a tense that only reads correctly until #67 merges. The worked example describes the invariant and the bug behind it; that is what the file is for. --- .greptile/rules.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.greptile/rules.md b/.greptile/rules.md index 935e071..b9b6311 100644 --- a/.greptile/rules.md +++ b/.greptile/rules.md @@ -123,9 +123,9 @@ default one-minute dedup window, which is why the bug went unnoticed. It is the N+1 counter, which is not deduped: `WithN1Detection(10, …)` fired at five real queries on MySQL, so every configured threshold was silently halved. -The fix lands in #67, as one shape applied at every interception point in -`driver.go`: call the base, then `analyzeExecuted`, which returns without -analyzing when the answer is `ErrSkip` or `ErrBadConn`. Two consequences are deliberate. Analysis +The fix is one shape, applied at every interception point in `driver.go`: call +the base, then `analyzeExecuted`, which returns without analyzing when the +answer is `ErrSkip` or `ErrBadConn`. Two consequences are deliberate. Analysis happens after execution, which is fine because nothing consumes findings before the query runs; and the latency window is read before the rules run, so analysis time cannot push a query past the slow-query threshold. Argument @@ -136,7 +136,7 @@ conversion also moved ahead of analysis, so a call rejected with `Guard.Observe` (check, then time) survives for interception points that are only ever told a query ran: the out-of-tree integrations. Nothing in `driver.go` is in that position, so restoring it there reintroduces the bug. -Its regression tests are fake drivers rather than assertions on internals — +The regression tests are fake drivers rather than assertions on internals — `fakeErrSkipDriver` and `fakeBadConnDriver` in `middleware/driver_fallback_test.go` — and each fails against the unfixed code with an exact count (2, 3, or a tripped N+1 threshold).