diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 68be98b2..17cf7b0f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -579,6 +579,35 @@ jobs: echo "pgtest: checked $(printf '%s\n' "$pkgs" | wc -l) real-Postgres packages against the service database" exit "$rc" + - name: Put the dev shell's nixpkgs tools on PATH (for sqlc vet) + # The Real-Postgres suite above needs only `go`, so this job skips the + # moon battery's phase-two bootstrap by default. sqlc-vet (next step) + # DOES need two nixpkgs tools from the dev shell — `psql` (apply the + # migrations into the throwaway DB) and `sqlc` (run the vet) — so resolve + # them here, the same way the moon job's phase two does: the attribute + # list is the parity script's own parse of devenv.nix, so what CI installs + # and what the gate expects cannot disagree. Run at the repo root (no + # working-directory): parity.ts reads the workspace-root devenv.nix, and + # bun is already on PATH from the language-toolchains step above. + run: | + attrs=$(bun tools/toolchain/parity.ts --print-nix-attrs) + out=$(nix build --no-link --print-out-paths \ + -f tools/toolchain/gate-tools.nix env --arg attrs "$attrs") + echo "$out/bin" >>"$GITHUB_PATH" + + - name: sqlc vet (db-prepare against the service Postgres) + env: + # Same service DSN the Real-Postgres suite uses. The sqlc-vet moon task + # derives its own throwaway-DB URL from this (creates sqlcvet_$$, applies + # the migrations, points SQLC_DATABASE_URL at it, drops it on exit), so + # the job needs no extra env beyond the DSN. This is where db-prepare + # actually runs against a live schema — the moon `ci` battery has no + # Postgres service, so sqlc-vet is peeled into this env-bearing job + # (ci.yml header: the moon battery realizes no environment), the same + # pattern as the Real-Postgres suite itself. + COMPASS_TEST_DATABASE_DSN: postgres://postgres:compass-test@127.0.0.1:5432/compass?sslmode=disable + run: moon run compass-go:sqlc-vet + microvm: name: microvm runs-on: ubuntu-latest diff --git a/docs/designs/server/compass-sqlc-adoption/design.md b/docs/designs/server/compass-sqlc-adoption/design.md index 3373dd09..8e83bd9e 100644 --- a/docs/designs/server/compass-sqlc-adoption/design.md +++ b/docs/designs/server/compass-sqlc-adoption/design.md @@ -449,7 +449,7 @@ Interfaces: - [ ] T5 — migrate agent sessions/transcripts/activity/config/placements - [ ] T6 — migrate authz/tokens/secrets/issues/forge/tenant/linear remainder; allowlist down to the two permanent entries -- [ ] T7 — `sqlc vet` (db-prepare) in CI + promote advisory gate lines to +- [x] T7 — `sqlc vet` (db-prepare) in CI + promote advisory gate lines to gating ## Open Questions diff --git a/go/moon.yml b/go/moon.yml index e34b0c07..a35bdc80 100644 --- a/go/moon.yml +++ b/go/moon.yml @@ -252,13 +252,67 @@ tasks: runFromWorkspaceRoot: false inputs: *sqlc_sources + sqlc-vet: + # `sqlc vet` with the sqlc/db-prepare rule PREPAREs every generated query + # against a live, schema-loaded Postgres — the strongest check sqlc offers + # (operator/typecast validity, column existence, index-referenced-by-hint) + # beyond what static analysis sees. It needs a real database, so unlike + # sqlc-drift (pure-local, no DB, in `ci` deps) this task is NOT in the moon + # `ci` battery: the moon job realizes no service (ci.yml:27-28). It is invoked + # instead by the env-bearing `pgtest` CI job — which already runs the + # postgres:16-alpine service and exports COMPASS_TEST_DATABASE_DSN — via + # `moon run compass-go:sqlc-vet`, the same peel-into-a-service-job pattern as + # pgtest/microvm/forge/gtk4. The task provisions its own throwaway DB: + # 1. DSN-gated skip. On a dev box with no Postgres the task echoes a notice + # and exits 0 — the same posture as the pgtest suites' DSN-gated skip + # (COMPASS_TEST_DATABASE_DSN drives the real-Postgres lane; absent it, + # skip rather than red). Either COMPASS_TEST_DATABASE_DSN (the CI service + # DSN) or SQLC_DATABASE_URL satisfies the gate; the task derives its own + # per-run URL, so the pgtest job needs no extra env beyond the DSN it + # already exports. + # 2. Throwaway DB. With a DSN present it creates a uniquely-named + # (sqlcvet_$$) database on that server, applies + # internal/store/migrations/*.sql in order via psql, points + # SQLC_DATABASE_URL at it, runs the vet, and drops the database on exit + # (trap, mirroring sqlc-drift's cleanup trap). The throwaway isolates + # db-prepare from whatever else shares the service DB and leaves no + # residue. `set -e` + psql ON_ERROR_STOP make any create/migrate/vet + # failure fail the task. + # go/cmd/compass-postgres (the private-postgres wrapper) is deliberately NOT + # reused: that is the stack supervisor's process-managed instance, whereas the + # CI postgres service is simpler and already provisioned — vet only needs a + # bare server to PREPARE against, not a managed lifecycle. psql is on PATH + # from the devenv `postgresql` package. + script: | + set -e + dsn="${COMPASS_TEST_DATABASE_DSN:-${SQLC_DATABASE_URL:-}}" + if [ -z "$dsn" ]; then + echo "sqlc-vet: no COMPASS_TEST_DATABASE_DSN/SQLC_DATABASE_URL — skipping db-prepare vet (dev box without the CI Postgres service)" + exit 0 + fi + db="sqlcvet_$$" + trap 'psql "$dsn" -c "DROP DATABASE IF EXISTS $db" >/dev/null 2>&1 || true' EXIT + psql "$dsn" -v ON_ERROR_STOP=1 -q -c "CREATE DATABASE $db" + vet_url=$(printf '%s' "$dsn" | sed -E "s#/[^/?]+(\?|\$)#/${db}\1#") + for f in internal/store/migrations/*.sql; do + psql "$vet_url" -v ON_ERROR_STOP=1 -q -f "$f" + done + SQLC_DATABASE_URL="$vet_url" sqlc vet + options: + runFromWorkspaceRoot: false + inputs: *sqlc_sources + ci: # module is affected: format, vet, lint (incl. exhaustiveness), -race test, # build, the supply-chain fence, and the sqlc drift gate. `drift` is NOT a # dep here — it belongs to the schema pipeline (compass-proto) and is # scheduled through the gen tree, matching compass-proto/moon.yml's own # `ci` composition. `sqlc-drift` IS a dep: it is fully local (no - # cross-project delegation, no DB), so it belongs in ci directly. + # cross-project delegation, no DB), so it belongs in ci directly. `sqlc-vet` + # is deliberately NOT a dep: it needs a live Postgres, which the moon battery + # has no business realizing (ci.yml has no service on the moon job); it is + # invoked instead by the env-bearing `pgtest` CI job, the same way pgtest/ + # microvm/forge/gtk4 are peeled into their own service-carrying peer jobs. deps: ['fmt', 'vet', 'lint', 'nilaway', 'test', 'build', 'vuln', 'licenses', 'sqlc-drift'] options: cache: false diff --git a/go/sqlc.yaml b/go/sqlc.yaml index b9eaa08b..ee881156 100644 --- a/go/sqlc.yaml +++ b/go/sqlc.yaml @@ -9,3 +9,9 @@ sql: out: "internal/store/db" sql_package: "pgx/v5" emit_interface: true # Querier, for the store's tx/pool duality + database: + uri: "${SQLC_DATABASE_URL}" # sqlc vet's db-prepare target (CI throwaway DB); see moon sqlc-vet + analyzer: + database: false # keep `generate`/`sqlc-drift` DB-free; db-prepare (vet) still uses database.uri + rules: + - sqlc/db-prepare # PREPARE every query against the live schema — the strongest vet diff --git a/tools/inline-sql-gate/index.test.ts b/tools/inline-sql-gate/index.test.ts index f08f0e70..5c1a9a50 100644 --- a/tools/inline-sql-gate/index.test.ts +++ b/tools/inline-sql-gate/index.test.ts @@ -140,6 +140,84 @@ func f() {}`; }); }); +// --------------------------------------------------------------------------- +// Identifier-passed SQL at a pgx receiver — the T7 promotion (rule b). +// --------------------------------------------------------------------------- + +describe("identifier-passed SQL at a pgx receiver is flagged (T7)", () => { + test("pool.Query with a bare-identifier SQL arg is flagged", () => { + const src = `func f() { + rows, err := pool.Query(ctx, q, arg) + _ = rows + _ = err +}`; + const fs = scanText(STORE, src); + expect(fs.length).toBe(1); + expect(fs[0]?.snippet).toContain("pool.Query(ctx, q, arg)"); + }); + + test("conn.Exec(ctx, ddl) — the store.go migration-runner shape — is flagged", () => { + const src = `func f() { + if _, err := conn.Exec(ctx, ddl); err != nil { + return err + } +}`; + const fs = scanText(STORE, src); + expect(fs.length).toBe(1); + expect(fs[0]?.snippet).toContain("conn.Exec(ctx, ddl)"); + }); + + test("tx.Exec(ctx, m.sql) — a dotted selector SQL arg — is flagged", () => { + const src = `func f() { + if _, err := tx.Exec(ctx, m.sql); err != nil { + return err + } +}`; + const fs = scanText(STORE, src); + expect(fs.length).toBe(1); + expect(fs[0]?.snippet).toContain("tx.Exec(ctx, m.sql)"); + }); + + test("s.pool.QueryRow with a bare-identifier SQL arg is flagged", () => { + const src = `func f() { + err := s.pool.QueryRow(ctx, query, id).Scan(&v) + _ = err +}`; + const fs = scanText(STORE, src); + expect(fs.length).toBe(1); + expect(fs[0]?.snippet).toContain("s.pool.QueryRow(ctx, query, id)"); + }); + + test("a non-pgx runtime.Exec(ctx, id, spec) with an identifier SQL slot is NOT flagged", () => { + // The receiver `runtime` is not a pgx handle, so the identifier `id` in + // the slot is a container id, not hoisted SQL — the false-positive guard. + const src = `func f() { + out, err := r.runtime.Exec(ctx, id, spec) + _ = out + _ = err +}`; + expect(scanText("go/internal/runtime/agent.go", src)).toEqual([]); + }); + + test("a pgx call whose SQL slot is itself a call is NOT flagged (not a hoisted name)", () => { + const src = `func f() { + _, err := pool.Exec(ctx, buildQuery(t), arg) + _ = err +}`; + expect(scanText(STORE, src)).toEqual([]); + }); + + test("only the SQL slot is tested, never the params (a bare-identifier 2nd param does not double-flag)", () => { + const src = `func f() { + _, err := pool.Exec(ctx, q, someIdentParam) + _ = err +}`; + const fs = scanText(STORE, src); + expect(fs.length).toBe(1); + expect(fs[0]?.snippet).toContain("pool.Exec(ctx, q, someIdentParam)"); + }); +}); + // --------------------------------------------------------------------------- // isExcludedPath — generated package + test files. // --------------------------------------------------------------------------- diff --git a/tools/inline-sql-gate/index.ts b/tools/inline-sql-gate/index.ts index 64eed8a0..b9322d93 100644 --- a/tools/inline-sql-gate/index.ts +++ b/tools/inline-sql-gate/index.ts @@ -3,22 +3,34 @@ // // The rule (design record § "The inline-SQL ban"): // -// A `.Query(` / `.QueryRow(` / `.Exec(` call whose SQL argument — the first -// string-literal argument, TOKENIZED ACROSS NEWLINES because the store -// overwhelmingly puts the literal on the line AFTER the call — is a Go string -// literal (backtick or double-quoted, including `+`-concatenated literals) -// containing a SQL keyword (SELECT|INSERT|UPDATE|DELETE|WITH|CREATE|DROP) is -// banned in go/**/*.go, EXCEPT: +// A `.Query(` / `.QueryRow(` / `.Exec(` call carries banned inline SQL in +// its SQL slot (the first argument after `ctx`) when EITHER: +// (a) that argument is a Go string literal (backtick or double-quoted, +// including `+`-concatenated literals), TOKENIZED ACROSS NEWLINES +// because the store overwhelmingly puts the literal on the line AFTER +// the call, containing a SQL keyword +// (SELECT|INSERT|UPDATE|DELETE|WITH|CREATE|DROP) — flagged at ANY +// receiver, or +// (b) the call's RECEIVER is a pgx pool/tx/conn handle and that argument is +// a bare identifier or simple selector (`q`, `ddl`, `m.sql`) — SQL +// hoisted into a const/var and passed by name (the `queryAgents` shape). +// Banned in go/**/*.go, EXCEPT: // 1. go/internal/store/db/** — the sqlc-generated package, // 2. **/*_test.go — tests legitimately poke raw SQL, // 3. an explicit, checked-in allowlist of file paths (ALLOWLIST below). // // The tokenizer is load-bearing. A line-scoped grep would MISS the dominant // store shape — `s.pool.Exec(ctx,\n\t"INSERT …")` — where the literal sits on -// the line after the call. It also must NOT flag a non-pgx `Exec(ctx, id, spec)` -// (runtime/compute), whose immediate arguments are identifiers, not a SQL -// literal — so the discriminator is "the argument STARTS with a string -// delimiter", which an identifier or an expression never does. +// the line after the call. +// +// Two guards keep the identifier rule (b) from firing on non-pgx calls. First, +// it is RECEIVER-SCOPED: only a pgx handle (last receiver segment in +// {pool, tx, conn, c}) has a SQL slot, so a runtime/compute +// `r.runtime.Exec(ctx, id, spec)` / `g.client.Exec(ctx, req)` — whose receiver +// is not a pgx handle — is never a query no matter what its args look like. +// Second, only the SQL slot (arg after `ctx`) is tested, never the params, and +// only a bare identifier/selector qualifies (a call/composite/concatenation is +// not a hoisted-SQL name). // // The ratchet: the allowlist is seeded to every store file that carries inline // SQL today, so the gate is GREEN on current main while banning any NEW inline @@ -26,12 +38,11 @@ // the stale-entry check (fail-closed) then fails the gate if an allowlist entry // no longer matches any finding, so a migrated file cannot be left allowlisted. // -// Known gap (deferred to T7, per the record's residual-risk note): SQL hoisted -// into a `const`/variable and passed as an identifier (`queryAgents(ctx, sql, -// arg)`, `QueryRow(ctx, q, …)`) escapes a literal-at-callsite scan. Those files -// therefore produce NO finding here and are NOT allowlisted in T1; the record -// promotes the identifier-passed shape to gating once the migration is -// complete. +// Identifier-passed SQL is GATED (T7): once every store domain migrated, the +// only remaining identifier-passed sites are the migration runner's +// `conn.Exec(ctx, ddl)` / `tx.Exec(ctx, m.sql)` in the PERMANENTLY-allowlisted +// go/internal/store/store.go, so promoting rule (b) leaves the gate green while +// banning any NEW const-hoisted SQL at a pgx call site. // // Inputs (env): // GATE_ROOT - directory to scan (default: git toplevel). @@ -53,6 +64,23 @@ export const GO_GLOB = "go/**/*.go"; const SQL_KEYWORD_RE = /\b(?:SELECT|INSERT|UPDATE|DELETE|WITH|CREATE|DROP)\b/i; /** pgx query methods. QueryRow before Query so the longer name wins. */ const CALL_RE = /\.(?:QueryRow|Query|Exec)\(/g; +/** + * The last receiver segment names that ARE a pgx pool/tx/conn handle in this + * codebase: `s.pool`→pool, `tx`, `conn`, `c` (a *pgx.Conn). Only these carry a + * SQL slot, so a bare-identifier SQL arg (`q`, `ddl`, `m.sql`) is flagged ONLY + * at these receivers — the structural exclusion of runtime/compute + * `r.runtime.Exec(ctx, id, spec)` / `g.client.Exec(ctx, req)` (receivers + * runtime/client/engine), whose identifier args are not SQL. sqlc's own + * `s.q.GetFoo(…)` never matches CALL_RE (not Query/QueryRow/Exec). + */ +const PGX_RECEIVERS: Record = { + pool: true, + tx: true, + conn: true, + c: true, +}; +/** A bare identifier or dotted selector — `q`, `ddl`, `m.sql`, `query`. */ +const IDENT_RE = /^[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)*/; /** * The ratcheting allowlist: files permitted to carry inline SQL today. Seeded @@ -67,17 +95,14 @@ const CALL_RE = /\.(?:QueryRow|Query|Exec)\(/g; * - go/internal/pgshare/pgshare.go — the build-tagged test harness; CREATE/ * DROP SCHEMA with an interpolated, self-generated identifier. * - * NOTE (T1): agent_tree.go and presence_reads.go carry inline SQL only as - * const-hoisted identifiers passed to the call (not literals at the call site), - * so this literal-scoped gate produces no finding for them and they are - * deliberately omitted — seeding them would trip the fail-closed stale-entry - * check. They are covered by the record's T7 identifier-passed-SQL promotion. - * - * NOTE (T1): dm.go was added to the store AFTER the design record froze (§T3 - * states "There is no dm.go" and predates it); it is a genuine store domain - * file carrying inline-SQL literals, so the invariant "every currently - * inline-SQL store file is allowlisted so the gate is green on main" requires - * seeding it. It migrates (and its entry drops) in a per-domain task like the rest. + * NOTE (T7): the migration is complete — every store domain file's inline SQL + * moved to sqlc, so the allowlist is down to the two PERMANENT entries below. + * The identifier-passed-SQL promotion (rule (b) above) is now LIVE: the only + * remaining bare-identifier SQL at a pgx receiver is the migration runner's + * `conn.Exec(ctx, ddl)` / `tx.Exec(ctx, m.sql)` in store.go, which is + * permanently allowlisted — so the gate stays green while banning any NEW + * const-hoisted SQL (the former `agent_tree.go`/`presence_reads.go` shape) + * anywhere else in go/. */ export const ALLOWLIST: string[] = [ // Every store domain file's inline SQL has migrated to sqlc (T2..T6, @@ -361,6 +386,38 @@ function sourceLine(text: string, line: number, starts: number[]): string { return text.slice(start, end); } +/** + * The last receiver segment of a `.Query|.QueryRow|.Exec` call — the token + * immediately before the `.` at `dot`. For `s.pool.Exec(` returns "pool"; for + * `tx.Exec(` returns "tx"; for `r.runtime.Exec(` returns "runtime". Returns "" + * when no identifier immediately precedes the dot (e.g. a `).Exec(` chained off + * a call result), which is never a pgx handle. + */ +function receiverSegment(text: string, dot: number): string { + let i = dot - 1; + while (i >= 0 && /[A-Za-z0-9_]/.test(text.charAt(i))) i--; + return text.slice(i + 1, dot); +} + +/** + * If `s` begins with a bare identifier or dotted selector (`q`, `ddl`, `m.sql`) + * and nothing else follows it but trivia/comma/close, return that identifier's + * leading segment; otherwise null. A trailing `(` (call), `[` (index), `+` + * (concat), or `{` (composite) disqualifies it — those are expressions, not a + * hoisted-SQL name. + */ +function bareIdentifier(s: string): string | null { + const m = IDENT_RE.exec(s); + if (m === null) return null; + const rest = s.slice(m[0].length); + const nextMeaningful = firstMeaningfulIndex(rest); + if (nextMeaningful >= 0) { + const c = rest.charAt(nextMeaningful); + if (c !== "," && c !== ")") return null; + } + return m[0].split(".")[0] ?? m[0]; +} + /** * Scan ONE Go file's text for inline-SQL findings. Pure: no I/O, no allowlist, * no exit — returns every raw finding so callers can apply the allowlist and @@ -375,23 +432,46 @@ export function scanText(file: string, text: string): Finding[] { if (dot === undefined || mask[dot] !== 1) continue; const open = dot + m[0].length - 1; const args = parseArgs(text, open); - // The SQL slot is the first STRING-LITERAL argument (an identifier or an - // expression — ctx, handle.id, spec, q, sql — never starts with a - // delimiter, which is the structural exclusion of non-pgx Exec calls). - for (const arg of args) { - const fm = firstMeaningfulIndex(arg.text); - if (fm < 0) continue; - const lead = arg.text.charAt(fm); - if (lead !== '"' && lead !== "`") continue; - if (!SQL_KEYWORD_RE.test(stringContents(arg.text))) break; - const line = lineOf(arg.start + fm, starts); + const push = (index: number) => { + const line = lineOf(index, starts); findings.push({ file, line, snippet: sourceLine(text, line, starts).trim(), }); + }; + + // Rule (a): the first STRING-LITERAL argument is the SQL slot (an + // identifier or expression — ctx, handle.id, spec — never starts with a + // delimiter). Flag it iff it carries a SQL keyword. Fires at ANY receiver. + let flagged = false; + for (const arg of args) { + const fm = firstMeaningfulIndex(arg.text); + if (fm < 0) continue; + const lead = arg.text.charAt(fm); + if (lead !== '"' && lead !== "`") continue; + if (SQL_KEYWORD_RE.test(stringContents(arg.text))) { + push(arg.start + fm); + flagged = true; + } break; } + if (flagged) continue; + + // Rule (b, T7): SQL hoisted into a const/var and passed by name. Fires + // ONLY at a pgx pool/tx/conn receiver — the load-bearing guard against + // runtime/compute `r.runtime.Exec(ctx, id, spec)` false positives — and + // ONLY on the SQL slot (the arg after ctx), never a param. A bare + // identifier/selector (`q`, `ddl`, `m.sql`) there is banned; a call or + // composite expression is not a hoisted-SQL name and is left alone. + if (!PGX_RECEIVERS[receiverSegment(text, dot)]) continue; + const slot = args[1]; + if (slot === undefined) continue; + const fm = firstMeaningfulIndex(slot.text); + if (fm < 0) continue; + const ident = bareIdentifier(slot.text.slice(fm)); + if (ident === null || ident === "ctx") continue; + push(slot.start + fm); } return findings; }