fix(cli): port db reset --experimental remote schema-files path to native TS (CLI-1958) - #6062
Open
Coly010 wants to merge 50 commits into
Conversation
…tive TS (CLI-1958) Ports Go's apply.MigrateAndSeed EXPERIMENTAL declarative branch (apps/cli-go/internal/migration/apply/apply.go:19,51-68) for db reset's remote (--linked / remote --db-url) path, replacing the last Go-binary delegation on that command. A versionless --experimental / SUPABASE_EXPERIMENTAL reset with pg-delta not enabled now applies [db.migrations].schema_paths files directly (legacyApplySchemaFiles) instead of replaying timestamped migrations, faithfully reproducing two undocumented Go quirks: an empty schema_paths default silently applies nothing, and a partial glob failure is swallowed once at least one pattern matches. Hoists the Glob.SQLFiles traversal (legacySqlFilesGlob) out of the seed pipeline into shared/ so both [db.seed].sql_paths and the new [db.migrations].schema_paths resolve through one port of Go's glob semantics. Exposes schema_paths from the db-config TOML reader with the same env-override/remote-block-merge handling as the sibling seed field. Removes the remaining LegacyGoProxy delegation from db reset's handler and runtime layer now that both the remote and local paths are fully native (the local path's own schema-files branch still runs behind the existing db __db-bootstrap seam, out of scope here).
Whitespace-only fix following the db reset (CLI-1958) note update — oxfmt recomputes column widths across the whole markdown table.
…ort (CLI-1958) Fixes the items three reviewers (go-parity, engineer, architect) converged on: - Correct legacyMigrateAndSeed's stale docstring: the EXPERIMENTAL schema-files branch is reachable from start's fresh-volume setup (version: ""), not just migration down, and is deliberately deferred to CLI-2040, not unreachable. - Route legacy-seed.ts's resolveSeedFiles through the shared legacySqlFilesGlob instead of a third hand-rolled glob copy, fixing a silent directory-expansion gap on the migration down/start seed path. Remove legacy-seed-ops.ts's now- redundant legacyGlobSeedFiles/LegacyGlobResult pass-through shim. - Add Go's GlobOption surface (skipEmptyGlobs/errorOnAllSkipped) to legacySqlFilesGlob for db diff's upcoming declarative path; this issue's own callers pass no options, so behavior is unchanged. - Fix legacy-sql-files-glob.ts's toSlash to only convert on win32, matching Go's filepath.ToSlash (a no-op on non-Windows) — otherwise routing legacy-seed.ts's backslash-escape patterns through the shared glob would corrupt them. - Add reset.integration.test.ts coverage for schema_paths declaration order across multiple patterns and directory-entry expansion, plus toml-read tests for SUPABASE_DB_MIGRATIONS_SCHEMA_PATHS's env-override, string, non-string- filter, and remote-suppression branches. - Correct reset.layers.ts's docstring: the local reset path still reaches LegacyGoProxy through the bootstrap seam (CLI-1955's scope), only the remote path is fully native.
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…8-port-db-reset-experimental-remote-schema-files-path-natively # Conflicts: # apps/cli/docs/go-cli-porting-status.md
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@9d1eb7a2b473c41acd33c3d7a22b2dcfb63fd309Preview 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: 8ff4485289
ℹ️ 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".
… (CLI-1958)
`legacySqlFilesGlob`/`legacyWalkSqlFiles` diverged from Go's
`config.Glob.SQLFiles` (`apps/cli-go/pkg/config/config.go:123-211`) in two
ways, both confirmed empirically against a built `apps/cli-go` probe:
- Directory expansion re-`stat`ed each child, following symlinks. Go's
`fs.WalkDir` types children from their parent's `ReadDir` entry
(Lstat-based), so a symlinked `.sql` file or subdirectory below a matched
schema/seed directory is never included or recursed into
(`io/fs/walk.go:114-115`). Detect this with `readLink` (succeeds only for
symlinks) before falling back to `stat`.
- An empty pattern (e.g. `schema_paths = [""]`) resolved via
`path.join(workdir, "")` to the workdir itself and reported a match. Go's
`Lstat("")` fails, so `fs.Glob`/`afero.Glob` always report no match for an
empty pattern. Short-circuit on `pattern.length === 0`.
Review: PR #6062 (chatgpt-codex-connector), threads on
legacy-sql-files-glob.ts:92 and :40.
…I-1958)
`[db.migrations].schema_paths` and `[db.seed].sql_paths` decode through Go's
`v.UnmarshalExact` (`apps/cli-go/pkg/config/config.go:749-756`), whose
decoder config never overrides `WeaklyTypedInput`, so viper's
`defaultDecoderConfig` default of `true` stands. mapstructure's
`decodeString` therefore weakly converts a non-string scalar array element
(bool to "1"/"0", a number to its decimal string) instead of erroring or
dropping it. The TS reader filtered non-string entries out instead, silently
dropping schemas.
Verified empirically against a built `apps/cli-go` probe: `schema_paths =
[42, true, "schemas/*.sql"]` resolves to `supabase/{42,1,schemas/*.sql}`.
Review: PR #6062 (chatgpt-codex-connector), thread on
legacy-db-config.toml-read.ts:1887.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7020d74f0f
ℹ️ 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".
…stion to exec-phase (CLI-1958)
Two Go-parity gaps in the experimental `db reset` remote schema-files path,
both verified empirically against apps/cli-go:
- legacySqlFilesGlob/legacyWalkSqlFiles silently treated a directory-read
failure during walk as an empty match. Go's fs.WalkDir propagates a ReadDir
error and walkMatchedDir wraps it as "failed to walk matched directory: ...",
which applySchemaFiles only discards when at least one OTHER file was still
found (declared non-empty); with nothing else matched, Go aborts before
applying anything. The walk failure now surfaces as a warning via
Effect.result/Result.isFailure, so the existing files.length === 0 gate
correctly turns it fatal instead of reporting silent success after schemas
are already dropped.
- legacyApplySchemaFiles attached Go's CmdSuggestion ("See schema file: ...")
to every legacyExecSqlFile failure, but Go's applySchemaFiles only sets
CmdSuggestion after ExecBatch (statement execution) fails -- a
NewMigrationFromFile (file-read) failure returns before CmdSuggestion is
ever touched. execMigrationBatch's mapError callback now carries a
"read"/"exec" phase tag so the suggestion is attached only on exec-phase
failures.
Extracted the ad hoc errMessage helper (legacy-migration-apply.ts) into
shared legacy-error-message.ts so legacy-sql-files-glob.ts can reuse it
without a circular import.
…ies (CLI-1958)
Go's config.Glob decode (UnmarshalExact, config.go:749-756) weakly coerces a
bool/number array element but hits mapstructure's UnconvertibleTypeError for
a non-scalar one (nested array/table), which aborts the ENTIRE config load
with "failed to parse config: decoding failed due to the following
error(s): ...". Verified empirically against apps/cli-go:
`schema_paths = [[]]` / `[{path = "x.sql"}]` both fail config.Load with that
exact message before any schema is dropped; multiple bad entries are
aggregated in one message.
legacyWeakCoerceGlobEntry previously filtered these elements out silently,
which on an experimental remote reset could drop remote schemas and then
apply zero files while reporting success. legacyReadDbToml now fails the
whole config load with the byte-matching mapstructure-style message instead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fa1508080
ℹ️ 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".
…1958) Go's mapstructure decode is weakly typed on the whole `[]string` field, not just on array elements: a top-level scalar (e.g. `schema_paths = 42`) is wrapped into a synthetic single-element array and decoded through the same per-element rules as a real array entry, and a non-empty table fails with the same unconvertible-type error an array element would. The native reader only applied that weak coercion to array elements, so a scalar fell through to the empty/default fallback instead of resolving (and potentially warning) like Go does. Verified empirically against apps/cli-go.
…1958) Two Go-parity gaps in the shared SQL-file globber: - A matched path that fails to stat (a broken symlink, or a file that disappears between the glob and the stat) was falling back to treating it as a regular file. Go's Glob.SQLFiles records a "failed to stat matched file" warning and skips the path instead; the fallback here let a later read of the nonexistent path turn a warned-but-otherwise-successful reset into a hard apply error. - Splitting an absolute pattern whose meta character is in the first path component (e.g. `/*.sql`, `/tmp*/*.sql`) collapsed the root directory to `""`, which the globber treats as "use the workdir". Go's real runtime glob path (afero.IOFS.Glob -> afero.Glob) explicitly special-cases a bare `/` and keeps it, so the pattern resolves against the filesystem root, not cwd. Verified empirically against apps/cli-go.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efbc9a070b
ℹ️ 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".
…review: CLI-1958)
Go's fs.WalkDir builds each child path via path.Join, which runs path.Clean
and collapses a doubled `/`. The native walkMatchedDir port instead
string-concatenated `${rel}/${name}`, so a literal schema_paths/sql_paths
directory entry ending in `/` (e.g. "/tmp/schemas/") produced
"/tmp/schemas//a.sql" instead of Go's "/tmp/schemas/a.sql" for every child.
Verified empirically: a scratch apps/cli-go probe calling
config.Glob{"<dir>/"}.SQLFiles(...) against a real trailing-slash directory
returns the single-slash path.
…IDE_EFFECTS (review: CLI-1958) The env var table omitted SUPABASE_DB_MIGRATIONS_SCHEMA_PATHS even though this change makes the native remote --experimental schema-files branch resolve [db.migrations].schema_paths through legacy-db-config.toml-read.ts's generic AutomaticEnv-override reader (LEGACY_ENV_OVERRIDABLE_KEYS), which this PR newly added for that key. No dedicated CLI flag exists for schema_paths, so the env var is the only non-config-file override surface.
Contributor
Author
|
pr-autopilot: heads up, this PR now shows Not auto-resolving this — it needs a manual rebase/merge to reconcile the two sides' table edits rather than a scripted fix. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dabdb6ade
ℹ️ 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".
…(review: CLI-1958) Go's fs.WalkDir joins child paths with path.Join, which cleans a bare `.` root away entirely. schema_paths/sql_paths entries like [".."] can resolve to exactly "." via config's own path.Join(SupabaseDirPath, ..), so joinRelChild must special-case rel === "." to match Go's foo.sql instead of ./foo.sql -- otherwise the seed_files.path hash key diverges between Go and native tooling, causing seeds to needlessly re-run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2502f3544f
ℹ️ 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".
…LI-1958) joinRelChild string-concatenated rel/name instead of running Go's path.Clean-equivalent lexical cleaning, so a directory configured with a cleanable segment (e.g. "/tmp/x/../schemas") produced a literal ".."-containing path instead of matching Go's fs.WalkDir output. Verified empirically that Node's path.join matches Go's path.Join byte-for-byte across dot-root, trailing-slash, and embedded "."/".." cases, so delegate to the injected Path service instead of special-casing more segment shapes.
…iew: CLI-1958) Go's fs.WalkDir types each child from the DirEntry its parent ReadDir already returned and never re-Stats through it, so a .sql file removed between ReadDir and the walk callback's own visit stays in Go's declared file list; only the later, real file-open fails loudly. This port's follow-up fs.stat call opens a race window Go doesn't have, and on failure silently classified the child as Unknown and dropped it with no warning - an experimental reset whose only schema file hit this race would "succeed" having applied nothing. Verified empirically with a scratch filepath.WalkDir probe that deletes a sibling .sql file between ReadDir and that file's own visit: Go still reports it IsRegular from the cached DirEntry, keeps it declared, and the later os.Open fails with "no such file or directory" - never a silent drop. Match that outcome: on a stat failure, best-effort include a '.sql'- named child anyway and let the real downstream read surface the failure.
…tFloat does (review: CLI-1958) strconv.FormatFloat special-cases the three non-finite float values before the format verb is consulted, rendering "+Inf"/"-Inf"/"NaN" — never JS's own "Infinity"/"-Infinity". A TOML schema_paths/sql_paths array entry can realistically hit this via the bare inf/-inf/nan float literals TOML v1.0 supports, which the weak mapstructure-style glob coercion must render identically to Go.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c405fc9eb8
ℹ️ 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".
…limit (review: CLI-1958) Go's bufio.Scanner can only raise "token too long" once it needs more data and the buffer is already full; a delimiter-terminated token is found in the same Scan() call that fills the buffer, before that check is reached, so it succeeds at exactly the limit. A trailing token with no delimiter never gets that chance, so it fails at exactly the limit. Track whether each split token was delimiter-terminated or emitted at EOF and compare with `>=` only for the latter.
…(review: CLI-1958) Go's strconv.FormatFloat preserves the IEEE754 sign bit on zero, so a weakly-decoded schema_paths/sql_paths entry of -0.0 renders as "-0". JS's (-0).toString() drops the sign and returns "0", so schema_paths = [-0.0] resolved to the wrong path. Detect Object.is(value, -0) before falling through to the generic toString() path.
…(review: CLI-1958) Go's down.ResetAll (resetRemote's delegate) best-effort caches the pg-delta migrations catalog right after apply.MigrateAndSeed succeeds, warning on failure rather than failing the reset. The native remote reset finished without calling the equivalent — already-ported legacyTryCacheMigrationsCatalog (wired into db push) — so downstream pg-delta tooling missed the refreshed cache and the warning line never appeared. Wire it in after the seed step, gated the same way Go gates it: skip for a versioned reset (--version/--last), since TryCacheMigrationsCatalog no-ops on any non-empty version.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b232d0805
ℹ️ 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".
…mmar (review: CLI-1958) viper.GetSizeInBytes -> cast.ToInt -> strconv.ParseInt(s, 0, 0) parses the post-multiplier-strip remainder with base 0, so hex/octal/binary literals (e.g. 0x100000) are valid Go byte counts that the previous decimal-only regex silently rejected, falling back to the 256KiB default instead of Go's parsed value. Verified against real vendored viper@v1.21.0 + cast@v1.10.0.
…view: CLI-1958) SIDE_EFFECTS.md still said the best-effort catalog-cache warning was "not ported", but reset.handler.ts already wires legacyTryCacheMigrationsCatalog unconditionally after either apply branch. Documents the cache file, its gating (no resolved version + pg-delta enabled), SUPABASE_EXPERIMENTAL_PG_DELTA, and the warn-never-fail behavior, matching db push's existing SIDE_EFFECTS wording.
…ths (review: CLI-1958) Go's UnmarshalExact decodes the whole config in one mapstructure pass and joins every field's decode error together (decodeStructFromMap never stops at the first field), so a config invalid in both db.seed.sql_paths and db.migrations.schema_paths reports both in one combined error. The reader was failing on the first field and never evaluating the second. Verified against the real apps/cli-go/pkg/config package (scratch probe via a local replace directive) with both fields invalid simultaneously.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e8393d526
ℹ️ 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".
…1958) legacyDbResetRuntimeLayer omitted LegacyPgDeltaSslProbe, LegacyEdgeRuntimeScript, and LegacyDockerRun, unlike legacyDbPushRuntimeLayer. When pg-delta caching is enabled, the post-reset catalog cache reaches those services via legacyExportCatalogPgDelta; missing them is an untyped missing-service defect the handler's Effect.catch cannot recover from, crashing the process after the remote database has already been reset instead of writing the catalog or emitting Go's best-effort warning. Compose the same three layers db push uses. Added a regression test that builds the real legacyDbResetRuntimeLayer (not a mocked service) and asserts both services are actually exposed.
…egers (review: CLI-1958) parseGoBaseZeroInt rejected underscore digit separators outright (e.g. "1_048_576"), silently falling back to the 256KiB default even though Go's strconv.ParseInt(s, 0, 64) accepts them per its integer-literal grammar. A statement between the two limits would apply in TS but Go would already have failed with "bufio.Scanner: token too long". Implemented Go's exact underscore-placement grammar, verified empirically against a real Go strconv.ParseInt(s, 0, 64): a single underscore may sit immediately after a base prefix (0x/0o/0b, or the bare leading "0" of legacy octal) or between two digits — never doubled, never leading a plain decimal literal, never trailing.
…efore (review: CLI-1958) Both db reset and db push captured the snapshot's Clock.currentTimeMillis before calling legacyTryCacheMigrationsCatalog, i.e. before the hash and the pg-delta export (a network round-trip) resolved. Go's real WriteMigrationCatalogSnapshot reads time.Now().UTC() internally, after TryCacheMigrationsCatalog has already resolved hash and snapshot, immediately before the write. The early capture could make a concurrent cache write from another process sort in the wrong order during catalog resolution/retention. Moved the clock read inside legacyTryCacheMigrationsCatalog itself, right before the write, fixing both callers at their shared root. Added a regression test that proves the timestamp reflects a real time gap the mocked export takes before resolving.
…view: CLI-1958) SIDE_EFFECTS.md labeled the migrations-catalog cache write as remote-path only. Go's start.SetupLocalDatabase (called by the local reset's PG15 recreate branch, behind this port's db __db-bootstrap seam) also calls pgcache.TryCacheMigrationsCatalog after MigrateAndSeed, with a "local" prefix — inherited automatically since the local path delegates to the real Go binary rather than being reimplemented in TS. The PG<=14 branch never calls it at all. Documented both paths and the PG14/PG15 split.
…Dir order (review: CLI-1958) legacyWalkSqlFiles iterated a directory's readDirectory() result in raw filesystem enumeration order. Go's fs.WalkDir visits entries in lexical byte order (os.ReadDir's own "sorted by filename" contract), so when a matched directory has multiple problematic children (e.g. two unreadable subdirectories), Go deterministically fails on the lexically-first one. This port's unsorted iteration could pick a different one depending on filesystem enumeration order, surfacing a different fatal/WARN message than Go. Sorted directory entries with the existing UTF-8 byte-order comparator before recursing. Added a regression test using a fake FileSystem that deliberately returns entries in reverse order to prove the fix, independent of what the real OS happens to return.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89acd6c011
ℹ️ 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".
…log export (review: CLI-1958) db push scopes legacyApplyProjectEnv(projectEnv) around its whole run so a SUPABASE_INTERNAL_IMAGE_REGISTRY/PGDELTA_NPM_REGISTRY set only in supabase/.env reaches the pg-delta edge-runtime helpers, which read process.env directly. db reset loaded the same projectEnv but never applied it, so the same override was silently ignored for its post-reset migrations-catalog export, falling back to the default registries.
…r text (review: CLI-1958)
Two follow-ups on the SUPABASE_SCANNER_BUFFER_SIZE parsing work:
- Reject a magnitude outside Go's signed int64 range (e.g.
"9223372036854775808", one over math.MaxInt64) the same way
cast.ToInt does: strconv.ParseInt(s, 0, 0) returns a range error, and
cast.ToInt discards ANY parseFn error and returns exactly 0 (falls back to
the 256KiB default), not the huge value Number.parseInt would silently
round to. Verified against the pinned spf13/cast@v1.10.0
(cast.ToInt("9223372036854775808") -> 0).
- Track the last RAW scanned token unconditionally, matching Go's
`token = scanner.Text()` (runs on every successful Scan(), before the
len(trim) > 0 append gate). A statement that trims to empty right before
an oversized one (e.g. a lone ";") must still show in the "After
statement N: ..." error text, not a blank token.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ac66092de
ℹ️ 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".
… treating as empty (review: CLI-1958)
smol-toml parses every TOML datetime variant to a TomlDate (a Date
subclass) that stores its value internally, not as an enumerable own
property, so it satisfies the same zero-enumerable-key test this reader
used to detect an empty inline table (`schema_paths = {}`). A bare
datetime therefore silently resolved to an empty pattern list instead of
failing config load.
Verified empirically against the real apps/cli-go config.Load: Go's
mapstructure decoder reports a bare datetime as unconvertible and aborts
the whole load, with a distinct Go type per TOML datetime variant
(time.Time for offset date-time, toml.LocalDateTime/LocalDate/LocalTime
for the three zone-less "local" variants). Exclude TomlDate from the
empty-table special case and teach legacyGoUnconvertibleType to name the
correct per-variant Go type, matching Go's error text exactly whether the
datetime is a top-level scalar or an array element.
…8-port-db-reset-experimental-remote-schema-files-path-natively # Conflicts: # apps/cli/docs/go-cli-porting-status.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d7e1f8ab1
ℹ️ 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".
…8-port-db-reset-experimental-remote-schema-files-path-natively # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/commands/db/reset/reset.handler.ts # apps/cli/src/legacy/shared/legacy-db-config.toml-read.ts # apps/cli/src/legacy/shared/legacy-migrate-and-seed.ts # apps/cli/src/legacy/shared/legacy-pgdelta.cache.unit.test.ts # apps/cli/src/legacy/shared/legacy-seed.ts
- legacy-sql-split.ts: match Go's unicode.IsDigit (decimal digits only,
\p{Nd}) for dollar-quote tag/identifier characters instead of \p{N}
(all Unicode numbers), which wrongly accepted non-decimal digit runes
like superscript-2 and could split schema SQL differently than Go.
- legacy-seed-ops.ts / legacy-seed.ts: enforce SUPABASE_SCANNER_BUFFER_SIZE
on seed file parsing via the shared checkScannerBufferSize, matching
Go's SeedFile.ExecBatchWithCache, which parses through the same
parseFile every other file type does.
- legacy-pgdelta.cache.ts: listJsonEntries now only swallows a genuinely
missing catalog directory and propagates every other read failure
(e.g. permission denied), matching Go's ReadDir usage after
ensureTempDir — previously every failure was treated as empty,
silently defeating catalog retention and cache resolution.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d1eb7a2b4
ℹ️ 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 content = yield* fs.readFileString( | ||
| path.isAbsolute(seed.path) ? seed.path : path.join(workdir, seed.path), | ||
| ); | ||
| yield* checkScannerBufferSize(content, (message) => new Error(message)); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier schema-file/project-env finding is that the separate seed check calls checkScannerBufferSize with its default empty project-env map. legacyApplyProjectEnv(projectEnv) only exposes the registry allowlist, so on a versionless experimental remote reset where SUPABASE_SCANNER_BUFFER_SIZE exists only in supabase/.env, an oversized seed statement is executed after the schemas are dropped, while the formerly delegated Go path loads that env value and rejects the seed; thread projectEnv through legacySeedData as is already done for legacyApplySchemaFiles.
AGENTS.md reference: apps/cli/AGENTS.md:L247-L257
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Ports
db reset's--experimentalremote schema-files path from a Go-binary delegation to native TypeScript, removing the lastLegacyGoProxydependency fromdb reset's remote branch.Trigger (matches Go's
apply.MigrateAndSeedthree-conjunct gate exactly):--experimental/SUPABASE_EXPERIMENTALset, no explicit--version/--last, and[experimental.pgdelta].enabledunset. Body: globs[db.migrations].schema_pathsand execs each matched file with no history tracking (no version row, noRESET ALLbetween files) — reproducing two undocumented Go quirks byte-for-byte:schema_paths = []makes this a silent no-op (schemas/seeds still drop and reseed, nothing gets applied).New shared primitives:
legacyApplySchemaFiles(legacy-migration-apply.ts) and a hoistedlegacy-sql-files-glob.ts(replacing three separate hand-rolled copies of Go'sGlob.SQLFilestraversal that had existed acrossdb push's seed path, this new schema-files path, andstart/migration down's seed path — the third one had already silently diverged, missing directory-entry expansion; that's fixed too).This path connects directly (no shadow database involved) — confirmed via go-parity-auditor, addressing the issue's own note about overlapping with CLI-1956's shadow-provisioning work (a separate, still-in-progress issue in another PR): there turned out to be no actual dependency.
Known, deliberately-deferred gap (tracked separately)
legacyMigrateAndSeed(shared bymigration downand nativesupabase start's fresh-volume setup) does not yet implement this same schema-files branch, sosupabase start --experimentalon a fresh volume doesn't reproduce Go's behavior. This is pre-existing (not introduced here) and out of scope fordb reset— filed as CLI-2040 with the same go-parity-auditor findings, and the relevant docstring here now points at it instead of asserting (falsely) that the gap doesn't exist.Review notes
Reviewed independently by go-parity-auditor, engineer-reviewer, and architect-reviewer — all three converged on the same two follow-ups (now fixed): a stale "unreachable" docstring papering over the CLI-2040 gap, and an incomplete hoist that left a third, silently-diverging copy of the shared glob logic in
db push/start/migration down's seed path. Consolidating that hoist also surfaced and fixed a real latent bug in the shared glob's Windows-path handling (toSlashwas applying backslash-to-slash conversion unconditionally instead of Windows-only, which would have corrupted backslash-escaped glob patterns on non-Windows once rerouted). Added test coverage for schema-file application order, directory-entry expansion, and the newSUPABASE_DB_MIGRATIONS_SCHEMA_PATHSenv-override branches.Fixes CLI-1958