feat(cli): port supabase inspect report to native TypeScript#5565
feat(cli): port supabase inspect report to native TypeScript#5565Coly010 wants to merge 4 commits into
Conversation
Promotes the last `inspect` leaf from a Phase 0 Go proxy to a native TS port (CLI-1317). The command runs every inspect query via server-side `COPY (...) TO STDOUT WITH CSV HEADER`, writes 14 CSVs under `<output-dir>/<YYYY-MM-DD>/`, then renders a Go-parity Glamour rules summary table validating those CSVs. - Extend the shared `LegacyDbConnection` driver with `copyToCsv`, which reuses a single raw `pg` connection (via `pg-copy-streams`) for all copies against the winning dial target, matching Go's single `pgconn`. - Add a bounded csvq-subset evaluator (`report.csvq.ts`) replicating csvq value/type-comparison semantics for the inspect rule grammar. - Support custom `[experimental.inspect.rules]` from config.toml with `env(VAR)` expansion; default to the 7 embedded rules otherwise. - Hoist the shared inspect base layer into `inspect/inspect.layers.ts`. - Add `legacy/output/legacy-bold.ts` (lipgloss Bold parity). - json/stream-json output modes are a TS-only addition (Go is text-only).
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase@5565Preview package for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2850b07d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // plain column reference | ||
| const col = this.parseColRef(); | ||
| return { column: col }; |
There was a problem hiding this comment.
When a project has custom [experimental.inspect.rules] using normal csvq SELECT expressions (for example SELECT COALESCE(name, '-') FROM ... or other functions/operators), the Go implementation passes r.Query directly to the csvq driver and scans the first column, but this parser only accepts an aggregate call or a bare column reference. Those valid custom rules now render a parser error in the STATUS cell instead of evaluating to pass/fail, so existing report rules outside the built-in subset regress.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The parity description is accurate: Go hands r.Query straight to the real csvq driver (apps/cli-go/internal/inspect/report.go), so a custom rule using full csvq syntax — COALESCE, CASE, joins, subqueries, etc. — evaluates to a real pass/fail there. This evaluator implements only a bounded subset (SELECT <agg|bare-column> FROM \csv` [WHERE ...]), so an out-of-subset custom rule surfaces a parser-error STATUS cell instead of a result. The 7 built-in default rules (templates/rules.toml`) are all inside the supported grammar and test-covered, so there is no regression for them; custom rules outside the subset do diverge from Go.
This is a deliberate, documented limitation (see the header of report.csvq.ts): there is no JS/TS port of mithrandie/csvq, and neither DuckDB nor alasql accepts csvq's dialect, so closing the gap fully means reimplementing the csvq expression dialect in TS — a large piece of work, not a quick follow-up. Flagging this as a scope decision for a maintainer rather than silently absorbing the divergence or pulling a full csvq reimplementation into this PR; leaving the thread open. Options:
- (a) accept the bounded subset as the documented limit and track full csvq support as a follow-up issue;
- (b) detect out-of-subset queries and emit a clearer "unsupported csvq expression" STATUS rather than a raw parser error;
- (c) treat full custom-rule parity as in-scope here (large reimplementation; own PR).
Promoting `inspect report` from a Go proxy to a native TS handler exposed two e2e mock gaps (the parity tests previously compared Go-vs-Go and passed trivially): - The native driver eagerly forces its lazy `pg` connection with a simple `SELECT 1` probe; the pg-mock rejected it in the empty state, so the command failed to connect. A real Postgres always answers `SELECT 1` — make the mock do so too, regardless of fixture state. - The rules summary is computed by csvq over COPY CSV content the mock cannot emit (it returns an empty, header-less COPY for every query). On those empty CSVs Go's csvq panics with an "index out of range" fatal error printed into each STATUS cell, while the native TS evaluator reports a clean "unknown column" — so stdout is not faithfully comparable. Add a documented `compareStdout` opt-out to runParity and use it for the report parity test; exit code, stderr, request log, and the 14 written CSVs are still compared, and the rules-table rendering is covered by the apps/cli unit + integration tests against real fixtures.
…ules Go loads `[experimental.inspect].rules` via `viper.UnmarshalExact` without disabling `WeaklyTypedInput` (config.go:579-584), so mapstructure coerces scalar rule fields to strings (123 → "123", true → "1") and aborts only when an array entry is not a table or a field is a non-coercible type. The TS reader was coercing non-strings to "" and silently skipping non-table entries, so a broken custom rule could replace the defaults and let the command succeed with empty rules. Coerce scalar fields the same way Go does, and fail with LegacyDbConfigLoadError on a non-table entry or a non-coercible field — matching Go's "expected a map or struct" / "expected type string" decode errors. (review: report.config rule validation)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32524fe2eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const chunks: Array<Buffer> = []; | ||
| stream.on("data", (chunk: Buffer) => chunks.push(chunk)); | ||
| stream.on("error", (error: Error) => | ||
| resume( | ||
| Effect.fail(new LegacyDbCopyError({ message: `failed to copy output: ${error}` })), | ||
| ), | ||
| ); | ||
| stream.on("end", () => resume(Effect.succeed(new Uint8Array(Buffer.concat(chunks))))); |
There was a problem hiding this comment.
For databases with many tables/indexes, report CSVs such as table or index stats can become large, but this accumulates every COPY chunk in memory, concatenates the full buffer, and the caller then keeps those bytes for rule evaluation. The Go implementation streams CopyTo directly into the output file, so this port can exhaust the Bun process heap or stall on a large report where the Go CLI would complete with bounded memory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right that Go streams CopyTo straight into the file (internal/inspect/report.go:64-77) and re-reads the CSVs from disk for the rules check (sql.Open("csvq", outDir), report.go:97), so its peak memory is bounded, whereas this port buffers each COPY via Buffer.concat and pins the CSVs in an in-memory map for rule evaluation.
This is purely a resource/robustness difference, not an output one — the CSV bytes, the rules table, and the exit code are byte-identical to Go on every observable dimension (the server serializes the CSV; the TS side never touches the values), and the report runs a fixed set of catalog-stat queries rather than streaming row data, so the OOM risk is confined to pathologically large databases. Matching Go's streaming means reshaping the copyToCsv driver seam (take an output path / return a write sink) and switching the rule evaluator to read CSVs back from disk — and that seam has already changed a couple of times this review cycle. I'd rather land the byte-identical behavior now and do the streaming rework as a focused follow-up with its own tests; tracking it rather than folding it in here. Leaving this thread open for that follow-up.
Three parity gaps in how custom `[experimental.inspect.rules]` are loaded, all
grounded in Go's `UnmarshalExact` (config.go:579) under viper's default
`WeaklyTypedInput: true` and its `PersistentPreRunE` config load:
- Validate the rule config BEFORE connecting or writing CSVs. Go loads/validates
the whole config in PersistentPreRunE (via ParseDatabaseConfig → LoadConfig),
so a malformed config aborts with zero side effects. The reader now runs at the
top of the handler, before mkdir/connect/COPY (rules are still applied in the
summary step). (review: validate-before-COPY)
- Reject a non-array `rules` value the way Go's decodeSlice does under weak
typing: a scalar (`rules = "foo"`) aborts ("expected a map or struct"); a single
inline table is wrapped into one rule; an empty table yields no custom rules.
Previously any non-array silently fell through to the defaults. (review: non-array rules)
- Reject unknown/misspelled keys in a rule table. UnmarshalExact sets mapstructure
ErrorUnused per-struct, so `fails = "bad"` aborts the load; the reader now fails
with LegacyDbConfigLoadError instead of silently ignoring extra keys. (review: unknown keys)
Adds unit coverage (unknown key, single inline table, scalar rules) and an
integration test asserting a malformed config aborts before any connection or CSV.
1 participant
Promotes
inspect report— the lastinspectleaf still on the Phase 0 Go proxy — to a native TypeScript port in the legacy shell (CLI-1317). All 13 activeinspect dbsubcommands + 12 deprecated aliases were already native (#5554); this finishes theinspecttree.What it does
Runs every inspect query against the target Postgres database via server-side
COPY (<query>) TO STDOUT WITH CSV HEADER, writes one CSV per query into<output-dir>/<YYYY-MM-DD>/(14 files), then renders a Go-parity GlamourRULE | STATUS | MATCHESsummary table validating those CSVs.How it's built
copyToCsvon the shared driver (legacy-db-connection.sql-pg.layer.ts):@effect/sql-pgdoesn't expose the COPY protocol, so the session opens one rawpgconnection (viapg-copy-streams) lazily on the first copy and reuses it for all queries — against the same dial target the primary connection won (TLS/fallback/DoH/step-down parity preserved), and closed by a scope finalizer. This mirrors Go running every copy on a singlepgconn. CSVs are byte-identical by construction (the server serializes the values, never the TS side).report.csvq.ts): there is no JS port of csvq and neither DuckDB nor alasql accept its dialect, so the rule queries are evaluated by a hand-written tokenizer + recursive-descent parser + RFC4180 CSV reader that replicates csvq's value/type-comparison semantics (numeric-vs-string promotion, three-valued logic,LISTAGG, aggregates). Unsupported grammar / unknown column → the rule's STATUS cell, not a command failure (matching Go).[experimental.inspect.rules]fromconfig.toml(withenv(VAR)expansion) replace the 7 embedded defaults when present.inspect/inspect.layers.ts(now used by bothdbleaves andreport); addslegacy/output/legacy-bold.tsfor lipglossBoldparity.Reviewer context
s.tbl, whichvacuum_statsemits asname— a pre-existing quirk in Go'srules.tomlthat surfaces as an unknown-column STATUS cell. It's preserved verbatim (documented inreport.rules.ts+SIDE_EFFECTS.md); tests assert this Go-faithful behavior rather than "fixing" it.$2database literal is not escaped, matching Go'sfmt.Sprintf("'%s'", database).json/stream-jsonoutput modes are a TS-only addition (Go is text-only); CSVs are still written.pg+pg-copy-streamsare needed for the COPY protocol; verified to bundle underbun build --compile.--project-refflag that does not exist on the Goinspecttree — the surface is--db-url/--linked/--local/--output-dir, matching Go.Closes CLI-1317