Skip to content

Commit c9e5648

Browse files
committed
fix(cli): close test db Go-parity gaps from Codex review
Address the parity divergences flagged in the Codex review of the `test db` port: - Sanitize the configured `project_id` before naming the local docker network. Go runs `sanitizeProjectId` on `c.ProjectId` unconditionally (config.go:471) before `GetId` derives `supabase_network_<id>`, so a configured value like "my project" joins the same network the local stack created. The port previously only sanitized the cwd-basename fallback. - Enable TLS for remote connections. Go uses TLS for remote (`ConnectByUrl`: pgx `sslmode=prefer` with non-TLS fallbacks stripped) and disables it for local (`ConnectLocalPostgres` sets `TLSConfig=nil`). The driver now sets `ssl: { rejectUnauthorized: false }` for remote (no cert verification, matching pgx) via a new `isLocal` connect option, threaded from the resolver and the pooler temp-role probe. - Keep stdout reserved for the pg_prove TAP stream. Go writes "Connecting to {local|remote} database..." to stderr; the port used `output.task`, which renders a clack spinner on stdout (text) and emits JSON log events on stdout (stream-json), corrupting the TAP stream. Emit the diagnostic via `output.raw(..., "stderr")` instead. Updates SIDE_EFFECTS.md and the connect-path e2e comment, and adds integration coverage for the stderr diagnostic, the remote label, and project_id sanitization.
1 parent 6725866 commit c9e5648

7 files changed

Lines changed: 141 additions & 31 deletions

File tree

apps/cli-e2e/src/tests/database-core.e2e.test.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,5 +365,19 @@ describe("test db", () => {
365365
expect(result.stderr).toContain("connect");
366366
});
367367

368-
testParity(["test", "db", "--local"]);
368+
// No testParity for `test db --local`: with no local Postgres listening in the
369+
// harness, the only reachable path is the connection-failure path, and its
370+
// stderr diverges by driver in ways that aren't cosmetic and can't be
371+
// normalized away. Both now emit Go's leading diagnostic to stderr:
372+
// Connecting to local database...
373+
// but the connect-error body and trailing hint still differ by driver. Go (pgx):
374+
// failed to connect to postgres: failed to connect to `host=… user=… database=…`: dial error (dial tcp …: connect: connection refused)
375+
// Make sure your local IP is allowed in Network Restrictions and Network Bans.
376+
// http://…/project/_/database/settings
377+
// The TS port (@effect/sql-pg) prints the effect SqlError and the --debug hint:
378+
// failed to connect to postgres: effect/sql/SqlError: PgClient: Failed to connect
379+
// Try rerunning the command with --debug to troubleshoot the error.
380+
// The meaningful contract (non-zero exit + a connect error on stderr) is
381+
// covered by the behaviour test above. A real connect-path parity test would
382+
// need a live local database in the harness.
369383
});

apps/cli/src/legacy/commands/test/db/SIDE_EFFECTS.md

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ One-shot `docker run --rm supabase/pg_prove:3.36`:
3131

3232
- `-v <hostpath>:<dockerpath>:ro` for each test path
3333
- `--security-opt label:disable`
34-
- `--network supabase_network_<project_id>` (local) with env `PGHOST=db PGPORT=5432`, or `--network host` (db-url / linked) with the resolved host/port
34+
- `--network supabase_network_<project_id>` (local) with env `PGHOST=db PGPORT=5432`, or `--network host` (db-url / linked) with the resolved host/port. `<project_id>` is sanitized exactly as Go's `config.Load` does (`sanitizeProjectId`), so an invalid configured value (e.g. `"my project"`) joins the same network the local stack created
3535
- `-e PGHOST/PGPORT/PGUSER/PGPASSWORD/PGDATABASE`
3636
- cmd `pg_prove --ext .pg --ext .sql -r <paths> [--verbose]` (`--verbose` when `--debug`)
3737

@@ -70,16 +70,24 @@ stream with no structured equivalent.
7070

7171
### `--output-format text` (Go CLI compatible)
7272

73-
TAP streams to stdout; connection/enable progress shows as spinners (text mode only).
73+
TAP streams to stdout. The connection diagnostic `Connecting to {local|remote} database...`
74+
is written to **stderr** (matching Go's `ConnectByConfigStream`), never to stdout — no
75+
spinner is used, so stdout carries only the raw TAP bytes.
7476

7577
### `--output-format json` / `stream-json`
7678

77-
No machine envelope is emitted (Go has none). stdout carries the raw TAP stream and
78-
spinners are suppressed; a non-zero `pg_prove` exit still fails the command (exit 1).
79+
No machine envelope is emitted (Go has none). stdout carries the raw TAP stream only; the
80+
connection diagnostic still goes to stderr (no task JSON-log events are written to stdout,
81+
which would otherwise corrupt the TAP stream). A non-zero `pg_prove` exit still fails the
82+
command (exit 1).
7983

8084
## Notes
8185

8286
- Native TypeScript port (Phase 1+); no Go proxy. Hidden command (matches Go).
87+
- Postgres TLS matches Go (`internal/utils/connect.go`): remote (`--db-url` / `--linked`)
88+
connections use TLS without certificate verification (pgx `sslmode=prefer` with non-TLS
89+
fallbacks stripped); local connections disable TLS (`ConnectLocalPostgres` sets
90+
`cc.TLSConfig = nil`).
8391
- Postgres access uses `@effect/sql-pg`. Go detects "pgTAP already installed" via a
8492
`pgx` `OnNotice` (code 42710 `duplicate_object`) callback, which `@effect/sql-pg`
8593
does not expose; the port instead checks `pg_extension` by extension name (any

apps/cli/src/legacy/commands/test/db/db.handler.ts

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -99,17 +99,30 @@ export const legacyTestDb = Effect.fn("legacy.test.db")(function* (flags: Legacy
9999
: isLocal
100100
? yield* Effect.gen(function* () {
101101
const toml = yield* legacyReadDbToml(fs, path, cliConfig.workdir);
102-
const projectId = Option.getOrElse(toml.projectId, () =>
103-
sanitizeProjectId(nodePath.basename(cliConfig.workdir)),
102+
// Go sanitizes `c.ProjectId` unconditionally (`config.go:471`) —
103+
// whether it came from `config.toml` or the cwd-basename fallback —
104+
// before deriving the network name `supabase_network_<id>`
105+
// (`config.go:57-58`, `GetId`). A configured `project_id` like
106+
// "my project" must join the same sanitized network the local stack
107+
// created, not the literal raw value.
108+
const projectId = sanitizeProjectId(
109+
Option.getOrElse(toml.projectId, () => nodePath.basename(cliConfig.workdir)),
104110
);
105111
return { _tag: "named" as const, name: `supabase_network_${projectId}` };
106112
})
107113
: { _tag: "host" as const };
108114

109115
const exitCode = yield* Effect.scoped(
110116
Effect.gen(function* () {
111-
const connecting = yield* output.task("Connecting to database...");
112-
const session = yield* dbConn.connect(conn).pipe(Effect.tapError(() => connecting.fail()));
117+
// stdout is reserved for the pg_prove TAP stream (the docker subprocess
118+
// writes it there directly), so connection diagnostics must go to stderr —
119+
// exactly as Go does (`ConnectByConfigStream` writes "Connecting to …
120+
// database…" to `os.Stderr`, `connect.go:205-228`). A `Output.task`
121+
// spinner would corrupt the TAP stream: clack writes spinner ANSI to
122+
// stdout in text mode, and the stream-json layer emits task JSON log
123+
// events to stdout. Go has no "Running pgTAP tests…" line at all.
124+
yield* output.raw(`Connecting to ${isLocal ? "local" : "remote"} database...\n`, "stderr");
125+
const session = yield* dbConn.connect(conn, { isLocal });
113126

114127
// Detect pre-existence before enabling so the drop is skipped when pgTAP
115128
// was already installed (Go keys this off an OnNotice 42710 callback,
@@ -125,7 +138,6 @@ export const legacyTestDb = Effect.fn("legacy.test.db")(function* (flags: Legacy
125138
message: `failed to enable pgTAP: ${cause.message}`,
126139
}),
127140
),
128-
Effect.tapError(() => connecting.fail()),
129141
);
130142
if (!alreadyExists) {
131143
yield* Effect.addFinalizer(() =>
@@ -138,22 +150,16 @@ export const legacyTestDb = Effect.fn("legacy.test.db")(function* (flags: Legacy
138150
),
139151
);
140152
}
141-
yield* connecting.clear();
142153

143-
const running = yield* output.task("Running pgTAP tests...");
144-
const code = yield* docker
145-
.run({
146-
image: LEGACY_PG_PROVE_IMAGE,
147-
cmd: args.cmd,
148-
env: runEnv,
149-
binds: args.binds,
150-
workingDir: args.workingDir,
151-
securityOpt: ["label:disable"],
152-
network,
153-
})
154-
.pipe(Effect.tapError(() => running.fail()));
155-
yield* running.clear();
156-
return code;
154+
return yield* docker.run({
155+
image: LEGACY_PG_PROVE_IMAGE,
156+
cmd: args.cmd,
157+
env: runEnv,
158+
binds: args.binds,
159+
workingDir: args.workingDir,
160+
securityOpt: ["label:disable"],
161+
network,
162+
});
157163
}),
158164
);
159165

apps/cli/src/legacy/commands/test/db/db.integration.test.ts

Lines changed: 62 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { mkdirSync, writeFileSync } from "node:fs";
2+
import { join } from "node:path";
13
import { BunServices } from "@effect/platform-bun";
24
import { describe, expect, it } from "@effect/vitest";
35
import { Effect, Exit, Layer, Option } from "effect";
@@ -6,6 +8,7 @@ import { mockOutput } from "../../../../../tests/helpers/mocks.ts";
68
import {
79
mockLegacyCliConfig,
810
mockLegacyTelemetryStateTracked,
11+
useLegacyTempWorkdir,
912
} from "../../../../../tests/helpers/legacy-mocks.ts";
1013
import { LegacyDebugFlag, LegacyNetworkIdFlag } from "../../../../shared/legacy/global-flags.ts";
1114
import { RuntimeInfo } from "../../../../shared/runtime/runtime-info.service.ts";
@@ -67,19 +70,25 @@ function mockDbConnection(opts: {
6770
}),
6871
extensionExists: () => Effect.succeed(opts.existed ?? false),
6972
};
73+
const connectCalls: Array<{ cfg: LegacyPgConnInput; isLocal: boolean }> = [];
7074
const layer = Layer.succeed(LegacyDbConnection, {
71-
connect: () =>
72-
opts.connectFails === true
75+
connect: (cfg, options) => {
76+
connectCalls.push({ cfg, isLocal: options.isLocal });
77+
return opts.connectFails === true
7378
? Effect.fail(
7479
new LegacyDbConnectError({ message: "failed to connect to postgres: refused" }),
7580
)
76-
: Effect.succeed(session),
81+
: Effect.succeed(session);
82+
},
7783
});
7884
return {
7985
layer,
8086
get execCalls() {
8187
return execCalls;
8288
},
89+
get connectCalls() {
90+
return connectCalls;
91+
},
8392
};
8493
}
8594

@@ -122,6 +131,7 @@ interface SetupOpts {
122131
runFails?: boolean;
123132
debug?: boolean;
124133
networkId?: string;
134+
workdir?: string;
125135
}
126136

127137
function setup(opts: SetupOpts = {}) {
@@ -135,7 +145,7 @@ function setup(opts: SetupOpts = {}) {
135145
resolver,
136146
connection.layer,
137147
docker.layer,
138-
mockLegacyCliConfig({ workdir: "/work/project", projectId: Option.none() }),
148+
mockLegacyCliConfig({ workdir: opts.workdir ?? "/work/project", projectId: Option.none() }),
139149
telemetry.layer,
140150
runtimeInfoLayer,
141151
Layer.succeed(LegacyDebugFlag, opts.debug ?? false),
@@ -170,6 +180,9 @@ describe("legacy test db integration", () => {
170180
expect(run?.env["PGPORT"]).toBe("5432");
171181
expect(run?.securityOpt).toEqual(["label:disable"]);
172182
expect(run?.cmd.slice(0, 5)).toEqual(["pg_prove", "--ext", ".pg", "--ext", ".sql"]);
183+
// The setup connection must be told it is local so the driver disables TLS
184+
// (Go's `ConnectLocalPostgres` sets `cc.TLSConfig = nil`).
185+
expect(connection.connectCalls[0]?.isLocal).toBe(true);
173186
}).pipe(Effect.provide(layer));
174187
});
175188

@@ -222,13 +235,16 @@ describe("legacy test db integration", () => {
222235
});
223236

224237
it.live("db-url mode: uses host networking and the resolved host/port", () => {
225-
const { layer, docker } = setup({ conn: REMOTE_CONN, isLocal: false });
238+
const { layer, docker, connection } = setup({ conn: REMOTE_CONN, isLocal: false });
226239
return Effect.gen(function* () {
227240
yield* legacyTestDb(flags({ dbUrl: Option.some("postgres://x"), local: false }));
228241
const run = docker.lastOpts;
229242
expect(run?.network).toEqual({ _tag: "host" });
230243
expect(run?.env["PGHOST"]).toBe(REMOTE_CONN.host);
231244
expect(run?.env["PGPORT"]).toBe("5432");
245+
// Remote connection → driver must enable TLS (Go strips non-TLS fallbacks
246+
// in `ConnectByUrl`); the handler signals this via `isLocal: false`.
247+
expect(connection.connectCalls[0]?.isLocal).toBe(false);
232248
}).pipe(Effect.provide(layer));
233249
});
234250

@@ -322,4 +338,45 @@ describe("legacy test db integration", () => {
322338
expect(telemetry.flushed).toBe(true);
323339
}).pipe(Effect.provide(layer));
324340
});
341+
342+
it.live("writes the connection diagnostic to stderr, keeping stdout for TAP (Go parity)", () => {
343+
const { layer, out } = setup();
344+
return Effect.gen(function* () {
345+
yield* legacyTestDb(flags());
346+
// Go writes "Connecting to local database..." to os.Stderr and reserves
347+
// stdout for the pg_prove TAP stream. A spinner/task on stdout would corrupt
348+
// that stream (and stream-json task events would too), so the port must emit
349+
// this on stderr and produce no stdout bytes of its own.
350+
expect(out.stderrText).toContain("Connecting to local database...");
351+
expect(out.stdoutText).toBe("");
352+
// Go has no "Running pgTAP tests..." line and no spinner task messages.
353+
expect(out.messages).toEqual([]);
354+
}).pipe(Effect.provide(layer));
355+
});
356+
357+
it.live("labels the connection diagnostic 'remote' for non-local connections", () => {
358+
const { layer, out } = setup({ conn: REMOTE_CONN, isLocal: false });
359+
return Effect.gen(function* () {
360+
yield* legacyTestDb(flags({ dbUrl: Option.some("postgres://x"), local: false }));
361+
expect(out.stderrText).toContain("Connecting to remote database...");
362+
}).pipe(Effect.provide(layer));
363+
});
364+
365+
const tempWorkdir = useLegacyTempWorkdir();
366+
it.live("sanitizes a configured project_id when naming the local network (Go parity)", () => {
367+
const workdir = tempWorkdir.current;
368+
mkdirSync(join(workdir, "supabase"), { recursive: true });
369+
// Go auto-fixes an invalid project_id via sanitizeProjectId (config.go:471,
370+
// 803-805); the local stack network is created from the sanitized id, so
371+
// `test db --local` must join `supabase_network_My_Project`, not the raw value.
372+
writeFileSync(join(workdir, "supabase", "config.toml"), 'project_id = "My Project"\n');
373+
const { layer, docker } = setup({ workdir });
374+
return Effect.gen(function* () {
375+
yield* legacyTestDb(flags());
376+
expect(docker.lastOpts?.network).toEqual({
377+
_tag: "named",
378+
name: "supabase_network_My_Project",
379+
});
380+
}).pipe(Effect.provide(layer));
381+
});
325382
});

apps/cli/src/legacy/shared/legacy-db-config.layer.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,9 @@ export const legacyDbConfigLayer = Layer.effect(
202202
conn: LegacyPgConnInput,
203203
): Effect.Effect<void, LegacyDbConfigError, LegacyPlatformApi> => {
204204
const attempt = (n: number): Effect.Effect<void, LegacyDbConfigError, LegacyPlatformApi> =>
205-
Effect.scoped(dbConn.connect(conn).pipe(Effect.asVoid)).pipe(
205+
// The temp-role probe always targets the remote Supavisor pooler, so it
206+
// connects with TLS (Go's pooler path goes through `ConnectByUrl`).
207+
Effect.scoped(dbConn.connect(conn, { isLocal: false }).pipe(Effect.asVoid)).pipe(
206208
Effect.catch((cause) => {
207209
// Go's `backoff.WithMaxRetries(b, 8)` allows 8 retries after the
208210
// initial attempt → 9 total attempts. `n` is 1-based, so give up only

apps/cli/src/legacy/shared/legacy-db-connection.service.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,23 @@ export interface LegacyDbSession {
4545
readonly extensionExists: (name: string) => Effect.Effect<boolean, LegacyDbExecError>;
4646
}
4747

48+
/** Per-connection options the driver layer cannot infer from `cfg` alone. */
49+
export interface LegacyDbConnectOptions {
50+
/**
51+
* Whether the target is the local stack (Go's `utils.IsLocalDatabase`). Drives
52+
* TLS, mirroring Go (`apps/cli-go/internal/utils/connect.go`): local connections
53+
* set `cc.TLSConfig = nil` (`ConnectLocalPostgres`) → no TLS, while remote
54+
* connections go through `ConnectByUrl`, where pgx defaults to `sslmode=prefer`
55+
* and every non-TLS fallback is stripped → TLS is required (without certificate
56+
* verification, matching pgx's default for `prefer`/`require`).
57+
*/
58+
readonly isLocal: boolean;
59+
}
60+
4861
interface LegacyDbConnectionShape {
4962
readonly connect: (
5063
cfg: LegacyPgConnInput,
64+
options: LegacyDbConnectOptions,
5165
) => Effect.Effect<LegacyDbSession, LegacyDbConnectError, Scope.Scope>;
5266
}
5367

apps/cli/src/legacy/shared/legacy-db-connection.sql-pg.layer.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { Effect, Layer, Redacted, type Scope } from "effect";
33
import * as Reactivity from "effect/unstable/reactivity/Reactivity";
44
import { LegacyDbConnectError, LegacyDbExecError } from "./legacy-db-connection.errors.ts";
55
import {
6+
type LegacyDbConnectOptions,
67
LegacyDbConnection,
78
type LegacyDbSession,
89
type LegacyPgConnInput,
@@ -53,6 +54,7 @@ function buildConnectionUrl(cfg: LegacyPgConnInput): string {
5354
*/
5455
const connect = (
5556
cfg: LegacyPgConnInput,
57+
{ isLocal }: LegacyDbConnectOptions,
5658
): Effect.Effect<LegacyDbSession, LegacyDbConnectError, Scope.Scope> =>
5759
Effect.gen(function* () {
5860
const hasOptions = cfg.options !== undefined && cfg.options.length > 0;
@@ -69,6 +71,13 @@ const connect = (
6971
password: Redacted.make(cfg.password),
7072
database: cfg.database,
7173
}),
74+
// TLS parity with Go (`internal/utils/connect.go`): remote connections use
75+
// TLS (pgx `sslmode=prefer` with non-TLS fallbacks stripped) but do not
76+
// verify the certificate, while local connections disable TLS entirely
77+
// (`ConnectLocalPostgres` sets `cc.TLSConfig = nil`). Omitting `ssl` here
78+
// leaves the `pg` driver in plaintext mode — fine for local, but it would
79+
// break against SSL-enforcing remote Supabase databases.
80+
...(isLocal ? {} : { ssl: { rejectUnauthorized: false } }),
7281
maxConnections: 1,
7382
}).pipe(
7483
Effect.provide(Reactivity.layer),

0 commit comments

Comments
 (0)