fix(cli): port shadow database provisioning to native TS (CLI-1956) - #6027
Draft
Conversation
…0 in /apps/cli-go in the go-minor group across 1 directory (#6023) Bumps the go-minor group with 1 update in the /apps/cli-go directory: [github.com/docker/go-connections](https://github.com/docker/go-connections). Updates `github.com/docker/go-connections` from 0.7.0 to 0.8.0 <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/docker/go-connections/commit/754f9060ef9371a6e9504a82e25bd0bce0cfe406"><code>754f906</code></a> Merge pull request <a href="https://redirect.github.com/docker/go-connections/issues/158">#158</a> from thaJeztah/no_umask</li> <li><a href="https://github.com/docker/go-connections/commit/20f47a112d2119c502530055300c3ba272fa3e17"><code>20f47a1</code></a> sockets: read somaxconn from system instead of SOMAXCONN</li> <li><a href="https://github.com/docker/go-connections/commit/e195e2a4e6e63b1ac25d4e1c2511170bdb030781"><code>e195e2a</code></a> sockets: set socket permissions without umask hack</li> <li><a href="https://github.com/docker/go-connections/commit/32c72ec777e66c7f4a391097f400ea62eff7e63f"><code>32c72ec</code></a> Merge pull request <a href="https://redirect.github.com/docker/go-connections/issues/162">#162</a> from thaJeztah/abstract_sockets</li> <li><a href="https://github.com/docker/go-connections/commit/f3526e58848fc48baf375ca42aa23d0d18dafefa"><code>f3526e5</code></a> sockets: improve abstract Unix socket handling</li> <li><a href="https://github.com/docker/go-connections/commit/fd93b41aeecfdee02fe3e7be3f8799c8842f8cef"><code>fd93b41</code></a> Merge pull request <a href="https://redirect.github.com/docker/go-connections/issues/163">#163</a> from thaJeztah/rm_log</li> <li><a href="https://github.com/docker/go-connections/commit/d0c75596e3ef03a6fb9e78befc9b18d9eed28e7e"><code>d0c7559</code></a> sockets: update more tests to use tempSocketPath utility</li> <li><a href="https://github.com/docker/go-connections/commit/7106f49a36e292e9d0cad10f1842505304254cc4"><code>7106f49</code></a> Merge pull request <a href="https://redirect.github.com/docker/go-connections/issues/161">#161</a> from thaJeztah/todone</li> <li><a href="https://github.com/docker/go-connections/commit/fa1caa79d797b5a4d622aecc371dd91c677d7e32"><code>fa1caa7</code></a> sockets: fix some remaining TODOs in Windows code</li> <li><a href="https://github.com/docker/go-connections/commit/31d55b210c6596022c9dd2a2a06093d62e086dac"><code>31d55b2</code></a> Merge pull request <a href="https://redirect.github.com/docker/go-connections/issues/160">#160</a> from thaJeztah/inmemory_context</li> <li>Additional commits viewable in <a href="https://github.com/docker/go-connections/compare/v0.7.0...v0.8.0">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore <dependency name> major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore <dependency name> minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore <dependency name>` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore <dependency name>` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore <dependency name> <ignore condition>` will remove the ignore condition of the specified dependency and ignore conditions </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Ports the shadow-database provisioning used by `db diff`/`db pull` (create → health-wait → connect → setup/migrate → remove) from the hidden Go `db __shadow` seam to native TypeScript, and removes that seam from apps/cli-go/cmd/db.go. This was the last local-container orchestration `db diff`/`db pull`'s native engines still delegated to Go. New shared primitives live in legacy/shared/db-bootstrap/shadow-database.ts (create/connect/setup/migrate/remove), composed by legacy/commands/db/shared/legacy-shadow-source.ts for db diff/pull's --target-local declarative branch. legacy-pgdelta.apply.ts is a from-scratch port of Go's pgdelta.ApplyDeclarative. Hoisted a shared legacyResolveDbSetupPrelude (db-setup.ts) so fresh-db and shadow setup stop duplicating the same JWKS/image-pull resolution. Fixes CLI-1956
Contributor
Author
|
@codex review |
![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: d63bb5825f
ℹ️ 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".
…ed cast (review: #PRRT_kwDOErm0O86Vl2Wu) legacyApplyDeclarativePgDelta cast JSON.parse's output straight to LegacyPgDeltaApplyResult; a syntactically valid but non-object payload (null, an array) let parsed.status throw an unhandled TypeError instead of failing typed as LegacyDeclarativeApplyError. Validate the top-level shape with a type-guard fed by "status" in value narrowing, which also removes the `as` cast AGENTS.md prohibits in legacy/.
…gaps in shadow schema loading (review: #PRRT_kwDOErm0O86Vl2Wv, #PRRT_kwDOErm0O86Vl2Wx, #PRRT_kwDOErm0O86Vl2Wz) legacyLoadDeclaredSchemas's schema_paths walk had three Go parity gaps against pkg/config/config.go's Glob.SQLFiles/walkMatchedDir: - An unreadable matched directory (permission/I-O error) was silently treated as zero files instead of propagating Go's "failed to walk matched directory" error, which can compare a local-target diff against the wrong target or produce an incomplete migration. - An absolute Windows schema_paths entry (`C:\repo\schema.sql`) was never forward-slash-normalized before globbing, unlike Go's `fs.Glob(fsys, filepath.ToSlash(pattern))` — mirrors the same toSlash step legacy-seed-ops.ts already applies for `[db.seed] sql_paths`. - A symlinked .sql file was included via `fs.stat` (which follows symlinks), where Go's `entry.Type().IsRegular()` excludes symlinks outright — fixed with the same `readLink`-as-no-follow-detector idiom already used by cp.handler.ts/legacy-seed-buckets.ts.
…(review: #PRRT_kwDOErm0O86Vl2W0) legacyResolveDbSetupPrelude resolved JWKS whenever realtimeEnabledForSetup was true, regardless of Postgres major version. Go's initSchema (start.go:243-253) returns via InitSchema14 for MajorVersion <= 14 without ever calling initSchema15, so Config.Auth.ResolveJWKS never runs on PG13/14 even with realtime enabled. ResolveJWKS can perform live discovery/JWKS HTTP requests for configured auth.third_party providers, so the unconditional resolve could make native db diff/db pull fail on a PG14 project where Go's own shadow setup never would.
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: c7c1fb4c90
ℹ️ 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".
…ching Go's DirExists (review: #PRRT_kwDOErm0O86Vmpso) legacyLoadDeclaredSchemas and legacyPrepareShadowSource used fs.exists() for the pg-delta declarative dir and supabase/schemas checks, so a stray regular file at either path was treated as present and handed to legacyWalkSqlFilesSorted's readDirectory call, which fails trying to read a file as a directory. Go's afero.DirExists (internal/db/diff/diff.go:63,80; internal/db/diff/shadow.go:72) treats a non-directory path the same as an absent one and falls through to the next source in the priority ladder.
… use (review: #PRRT_kwDOErm0O86Vmpsn) legacyIsPgDeltaApplyResult only checked the top-level "status" field, so a pg-delta payload with a malformed nested field (e.g. errors as an object instead of an array) passed the guard and reached legacyFormatApplyFailure's `for (const issue of errors)`, throwing an unhandled TypeError defect instead of failing typed as LegacyDeclarativeApplyError. Go's json.Unmarshal rejects the whole payload the moment any of ApplyResult's own typed fields doesn't match its struct field type (apps/cli-go/internal/pgdelta/apply.go: 27-44), so the guard now checks every field ApplyResult itself declares a type for, not just status.
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: d9fb78a54b
ℹ️ 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".
…s nil pointer (review: #PRRT_kwDOErm0O86VnXRL) Go's ApplyIssue.Statement is *ApplyStatement, so a `"statement":null` entry unmarshals to nil and formatApplyIssue's `issue.Statement == nil` (apply.go:202) renders it as message-only. The TS port's guard only checked `=== undefined`, so a JSON.parse'd null fell through to issue.statement.statementClass and threw a TypeError instead of rendering the failure message.
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: 18e5b0982f
ℹ️ 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".
…matching Go's WalkDir/afero.Walk (review: #PRRT_kwDOErm0O86VnmwR)
legacyGlobDeclaredSchemaPaths and legacyWalkSqlFilesSorted used
Effect's FileSystem.readDirectory(dir, { recursive: true }), which is
backed by Node's recursive fs.readdir and follows symlinked
subdirectories. Go's fs.WalkDir/afero.Walk are both Lstat-based and
never descend into a symlinked directory, so a schema directory
symlinking outside the configured schema tree could leak external
.sql files into a local-target diff/pull that Go would never pick up.
Added a shared legacyWalkRegularSqlFilesNoFollow helper that walks
manually, one non-recursive readDirectory per level, skipping any
symlinked entry (file or directory) via the existing
readLink-before-stat idiom before recursing.
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Re-provision the shadow on pooler retries
When a linked direct-host diff fails with an IPv6 connectivity error and an IPv4 pooler is available, this retries only the diff engine against the already-provisioned shadow. In the checked Go path, apps/cli-go/internal/db/pull/pull.go's diffRemoteSchema calls diff.DiffDatabase again, so the fallback prints the creation/diff banners again and provisions and tears down a fresh shadow. Move the fallback boundary around the complete shadow preparation and diff operation so this recovery path preserves the Go command's observable output and Docker/setup side effects.
AGENTS.md reference: apps/cli/AGENTS.md:L248-L252
ℹ️ 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".
…ing Go's nil ApplyStatementLocation (review: #PRRT_kwDOErm0O86Vn2zi)
…56 removed its only caller (review: #PRRT_kwDOErm0O86Vn2zk)
Contributor
Author
|
@codex review |
…hing Go's DiffDatabase (review: pullrequestreview-4834534116) Go's diffRemoteSchema retries the whole diff.DiffDatabase call (shadow provisioning included) against the IPv4 pooler on an IPv6 failure, not just the diff step (internal/db/pull/pull.go:176-190, internal/db/diff/diff.go:211-217) — DiffDatabase prints "Creating shadow database..." and runs PrepareShadowSource before ever touching the target connection, so a pooler retry re-prints the creation/diff banners and provisions + tears down a second, fresh shadow. The native pull handler previously provisioned one shadow up front and only retried the diff engine call against it, silently reusing the shadow across the retry instead of re-provisioning it. Restructure the fallback boundary so `withPoolerFallback` retries the full prepare-shadow-then-diff operation, and strengthen the integration test to assert the double provision/teardown and repeated banners. The declarative pull path (pullDeclarativePgDelta) is untouched: Go itself provisions that shadow once via PrepareRawShadow before its own retry, so single-shadow reuse there already matches Go.
Contributor
Author
|
Re: Codex's "Re-provision the shadow on pooler retries" review (submitted on Confirmed and fixed in f278200 — this is a real parity gap, not a false positive. Verified against The native Fix: restructured the fallback boundary so One deliberate non-change: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23147569e1
ℹ️ 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".
…hing Go's per-element UnmarshalJSON (review: PRRT_kwDOErm0O86VoG_A)
legacyIsPgDeltaApplyResult only checked that errors/stuckStatements/
validationErrors/diagnostics were arrays, not that each element matched
Go's ApplyIssue/ApplyDiagnosis JSON shape - so `{"status":"success",
"errors":[123]}` passed the guard and was reported as a successful
apply. Verified empirically against Go's real struct definitions
(apps/cli-go/internal/pgdelta/apply.go): a malformed element (wrong
type, or an object field of the wrong type) fails ApplyIssue/
ApplyDiagnosis's own UnmarshalJSON, which fails the WHOLE ApplyResult
decode, not just that element - so a payload like this must be
rejected here too, not accepted as a false success.
legacyIsValidApplyIssueElement/legacyIsValidApplyDiagnosisElement
reproduce Go's exact per-element acceptance rules, including the
asymmetry between the two (ApplyIssue accepts a bare string; Diagnosis
does not) and ApplyDiagnosis's own statementId leniency (never fails
the parse for a mistyped statementId - Go silently drops it instead).
…e sort/native separators in schema walks (review: PRRT_kwDOErm0O86VoG_B, PRRT_kwDOErm0O86VoG_E, PRRT_kwDOErm0O86VoG_I, PRRT_kwDOErm0O86VoG_J) Four fixes to the shared schema-directory walkers, all in legacy-shadow-source.ts: - legacyWalkRegularSqlFilesNoFollow no longer swallows a per-entry fs.stat failure into "file absent" - both Go walkers (afero.Walk, fs.WalkDir) pass an entry stat/lstat error to their callback, which returns it and aborts the whole walk, so a permission/IO error can no longer silently build an incomplete declarative target. - Both of this module's `sort.Strings` ports (the final path sort in legacyWalkRegularSqlFilesNoFollow, and legacyGlobDeclaredSchemaPaths's per-pattern match sort) now use a UTF-8-byte comparator (legacyCompareUtf8Bytes) instead of JS's default UTF-16-code-unit sort - verified empirically that a supplementary-plane character alongside a BMP private-use character sorts in the opposite order under each scheme. - legacyWalkSqlFilesSorted (the afero.Walk-backed pg-delta-declarative- dir/supabase-schemas branches) now checks whether its own root is a symlink and returns [] without descending, matching afero.Walk's Lstat-the-root-first behavior - verified against afero's own source. legacyGlobDeclaredSchemaPaths's schema_paths branch is unchanged (Go's fs.WalkDir deliberately follows a symlinked root). - legacyWalkSqlFilesSorted now joins the declarative-dir/schemas-dir relative path with the injected Path service instead of a literal `/` template, so the result carries native separators on Windows (Go's afero.Walk never calls filepath.ToSlash on this branch, unlike the schema_paths/fs.WalkDir branch, which does) - verified that path.win32.join normalizes every `/` in the input, not just the outer join, so this is a no-op on POSIX and a one-line fix on Windows.
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: 22c923c381
ℹ️ 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".
…t UnmarshalJSON (review: PRRT_kwDOErm0O86VoYwD) Go's `json.Unmarshal` parses an int-typed field's JSON literal via strconv.ParseInt, so any number with a decimal point (e.g. 1.5) fails the whole ApplyResult decode instead of being silently accepted. The TS guard only checked `typeof === "number"`, so a malformed totalApplied/totalRounds/ totalStatements/totalSkipped/position value could slip through and print a misleading apply summary.
…obs, matching Go's fs.Glob (review: PRRT_kwDOErm0O86VoYwE, PRRT_kwDOErm0O86VoYwF) Two independent parity gaps in the declarative schema-path resolution Go compiles per-platform: filepath.Clean/ToSlash only treat `\` as a path separator on a Windows build, so on darwin/linux a backslash in a schema_paths entry is a literal filename character that must NOT be folded into `/` before comparing against the declarative dir. Gate legacyCleanSchemaPath's normalization on the host platform (verified against a real GOOS=darwin build). Separately, io/fs.Glob never matches an empty pattern (Stat(fsys, "") fails on a real OS filesystem, verified against the real config.Glob.SQLFiles), but legacyGlobPattern's literal-pattern branch resolves "" to the workdir itself, which always exists — so an empty schema_paths entry recursed into and collected every .sql file in the project instead of matching nothing. Short-circuited the empty-pattern case at this call site rather than in the shared legacyGlobPattern helper, since that helper also backs [db.seed] sql_paths and legacy-migrate-and-seed.ts, both out of scope here.
…s len() gate (review: PRRT_kwDOErm0O86VoYwG)
Go's DockerStart only overrides NetworkMode when
`len(viper.GetString("network-id")) > 0` (docker.go:379-383) — an explicitly
empty override (e.g. a shell expanding an unset var to "") falls through to
the generated supabase_network_<projectId> name. legacyBuildLocalDbContainerInputs
only checked Option.isSome, so `--network-id ""` passed the literal empty
string straight to `docker create --network`, which fails. This shared
builder backs db start/db reset in addition to CLI-1956's new db diff/db pull
shadow-provisioning callers, so the fix (and its regression test) is scoped
there rather than duplicated per caller.
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: fadb97d9d2
ℹ️ 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".
…k error causes, matching Go (review: PRRT_kwDOErm0O86VooEy, PRRT_kwDOErm0O86VooEw)
- legacyBuildShadowPostgresContainerSpec now initializes POSTGRES_PASSWORD from
the caller-resolved [db] password instead of a hardcoded "postgres" literal,
matching Go's NewContainerConfig (diff.go:140), which sources it from the
SAME utils.Config.Db.Password for both the real local container and the
shadow. legacyShadowRunInputFromLocalContainerInputs's setup.dbUrl (used only
to extract the shadow's internal setup-job password) is built from that same
resolved password instead of the regular container's own hardcoded dbUrl, so
a non-default [db] password no longer breaks every shadow setup job.
- legacyGlobDeclaredSchemaPaths now includes the underlying PlatformError's
message when a matched schema directory fails to walk, matching Go's
walkMatchedDir ("failed to walk matched directory: %w") and the sibling
legacyWalkSqlFilesSorted, instead of discarding the cause and substituting
the directory name.
Declined the same round's "preserve POSIX backslashes while globbing schema
paths" finding: verified empirically against the real apps/cli-go
config.Glob.SQLFiles on darwin that Go's own path.Match treats `\` as an
escape metacharacter on every platform (not a literal filename byte), so
preserving it here would not reproduce a Go behavior that doesn't exist —
documented inline rather than silently dropped.
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: 3a4f121445
ℹ️ 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".
…file (review: PRRT_kwDOErm0O86Vo8Yv) Go's `failed to stat matched file: %w` (apps/cli-go/pkg/config/config.go:161-165) wraps the real stat error; this branch was interpolating the matched file path instead, matching the sibling walk-failure branch two lines below.
…view: PRRT_kwDOErm0O86Vo8Yy) Go's loadDeclaredSchemas (apps/cli-go/internal/db/diff/diff.go:52-101) wraps the same afero.Walk failure with a different prefix per source: the pg-delta declarative-dir branch reports "failed to walk declarative dir: %w", while the supabase/schemas fallback reports "failed to walk dir: %w". Both branches shared legacyWalkSqlFilesSorted with one hardcoded generic prefix; it now takes an errorPrefix so each caller supplies its own, matching Go. Adds unit tests locking in both prefixes.
…Go (review: PRRT_kwDOErm0O86Vo8Ys) Go's DockerRemove (apps/cli-go/internal/utils/docker.go:439-446) is a single Docker.ContainerRemove SDK call, so any failure mode (daemon disconnected, the call erroring outright) folds into the one err it prints "Failed to remove container:" for. The TS port only reported a nonzero docker rm exit code, silently discarding a spawn or exit-code/stderr-collection failure via the outer Effect.ignore. Now catches those the same way the sibling legacyRestartSatelliteService (restart-services.ts) already does, via legacyDescribeContainerCliFailure, and reports the same warning before continuing (still best-effort).
…atching Go's encoding/json (review: PRRT_kwDOErm0O86Vo8Yw)
Verified empirically against Go's real encoding/json: unmarshaling a JSON
null into a non-pointer struct field (string/bool/int) produces no error
and leaves the zero value untouched. ApplyIssue's non-Statement fields
and ApplyDiagnosis's fields are all plain non-pointer types decoded this
way inside their UnmarshalJSON, so {"errors":[{"message":null}]} is a
valid, Go-accepted ApplyResult, not a parse failure.
legacyIsValidApplyIssueElement/legacyIsValidApplyDiagnosisElement now
tolerate null alongside each field's declared type; the downstream
formatters already default with String(x ?? "")/?? 0/=== true throughout,
so once the guard lets null through it renders the same zero-value output
Go would ("unknown pg-delta issue"/"unknown pg-delta diagnostic"). Widens
the TS interfaces to `| null` on these fields rather than accepting a
runtime value the type doesn't admit.
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: 0372627139
ℹ️ 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".
…ding/json (review: PRRT_kwDOErm0O86VpQi2)
ApplyResult has no custom UnmarshalJSON — its plain, non-pointer int
counters (totalStatements/totalRounds/totalApplied/totalSkipped) decode
via the default encoding/json, which accepts a JSON null for a non-pointer
int field with no error and leaves the zero value (verified empirically).
The prior null-scalar handling only covered issue/diagnostic elements;
extend the same "null means absent" rule to the top-level counters so a
payload like {"status":"success","totalApplied":null} is accepted rather
than rejected as a parse failure.
… (review: PRRT_kwDOErm0O86VpQi5) Go's formatStatementSQL truncates via len(normalized)/normalized[:maxLen-3], both of which count and slice raw UTF-8 bytes. The TS port truncated on JS UTF-16 code units instead, so a failed statement containing multibyte (e.g. non-ASCII identifier) characters could stay far longer than Go's 120-byte limit — or not truncate at all — producing a different, potentially much larger failure line than Go's legacy stderr contract. Slice via Buffer at the same byte offset Go's raw byte slice would use.
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: 7e1ffbc252
ℹ️ 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".
…o (review: PRRT_kwDOErm0O86VpoP7, PRRT_kwDOErm0O86VpoP-, PRRT_kwDOErm0O86VpoQE, PRRT_kwDOErm0O86VpoQH)
Four Go-parity gaps in the pg-delta declarative-apply failure formatting:
- legacyFormatStatementSql now returns raw bytes instead of decoding the
truncated SQL back into a string. Go's formatStatementSQL slices raw UTF-8
bytes with no regard for codepoint boundaries, so a multibyte character
straddling byte 117 is cut mid-sequence and written to stderr unvalidated.
Buffer#toString("utf-8") on that same fragment substitutes U+FFFD instead
of reproducing it, corrupting Go's fixed 120-byte summary (verified against
a real Go probe: the raw slice is intentionally invalid UTF-8, and the
decode/re-encode round-trip changes both content and length). The whole
formatting pipeline (legacyFormatApplyIssue/legacyFormatApplyFailure) now
carries Buffer end to end, and the real call site uses output.rawBytes.
- legacyIsPgDeltaApplyResult now accepts a JSON null for
errors/stuckStatements/validationErrors/diagnostics. Go's encoding/json
accepts null for a []T slice field with no error, leaving a nil slice —
verified empirically — so a payload like {"errors":null} must format as if
none were reported, not fail the parse.
- legacyNormalizeApplyDiagnosis now drops a diagnosis's statementId when a
nested field (e.g. filePath) is mistyped, instead of rendering the raw
object. Go's ApplyDiagnosis.UnmarshalJSON tries the object shape first, then
a bare-string fallback, and silently leaves StatementID nil if both fail —
verified empirically that {"filePath":123,...} decodes with StatementID ==
nil in Go, so the native path was showing a bogus "(123#1)" location Go
never emits.
- legacyFormatDebugJson now reformats via a byte-preserving JSON token
scanner (legacyGoJsonIndentTokens) instead of JSON.parse + JSON.stringify.
Go's json.Indent only inserts whitespace between existing tokens; decoding
a number beyond Number.MAX_SAFE_INTEGER into a JS float64 (or re-escaping
an existing string escape like \/) would corrupt the exact debug payload
users are asked to attach to bug reports. The new scanner is verified
byte-identical to Go's json.Indent across nested objects/arrays, empty
{}/[], and both edge cases above.
…tching Go (review: PRRT_kwDOErm0O86VpoQB) Go's DiffDatabase prints "Creating shadow database..." before calling PrepareShadowSource, which is where the postgres image gets resolved/pulled (via CreateShadowDatabase -> utils.DockerStart) -- and the whole thing runs again, fresh, on every pooler-retry attempt. The native db pull handler was resolving the shadow image outside runShadowDiff, before the retried closure even printed its banner, so an image-resolution failure omitted Go's "Creating shadow database..." line and the image was never re-resolved on a pooler retry. Moving the resolution inside runShadowDiff, right after the banner, matches both Go's ordering and diff.handler.ts's own (already correct) call site.
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…6-port-shadow-database-provisioning-natively-and-remove-the-db # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/shared/db-bootstrap/rollback.ts # apps/cli/src/legacy/shared/legacy-container-cli.ts
…volume-rename test refs) Merging origin/develop into this branch surfaced three post-merge breaks: legacyRollbackStart gained a debug parameter on develop that db/start's own caller (added on this branch) didn't know about, container-lifecycle's already-exists-tolerance test block silently inherited develop's pre-rename legacyEnsureStartVolume/LegacyStartVolumeCreateError references (this branch had already renamed both under its own equivalent fix for the same podman bug), and go-cli-porting-status.md needed an oxfmt table reflow after the merge's manual conflict resolution.
…t-local-recreate-natively-and-remove-the-__db' into columferry/cli-1956-port-shadow-database-provisioning-natively-and-remove-the-db # Conflicts: # apps/cli/docs/go-cli-porting-status.md # apps/cli/src/legacy/commands/db/start/start.handler.ts # apps/cli/src/legacy/commands/db/start/start.integration.test.ts # apps/cli/src/legacy/shared/db-bootstrap/db-setup.ts # apps/cli/src/legacy/shared/db-bootstrap/local-container-inputs.ts # apps/cli/src/legacy/shared/db-bootstrap/rollback.ts # apps/cli/src/legacy/shared/legacy-container-cli.ts
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 the shadow-database provisioning used by
db diff/db pull(create → health-wait → connect → setup/migrate → remove) from the hidden Godb __shadowseam to native TypeScript, and removes that seam fromapps/cli-go/cmd/db.go. This was the last local-container orchestrationdb diff/db pull's native engines still delegated to Go for.New shared primitives live in
legacy/shared/db-bootstrap/shadow-database.ts(create/connect/setup/migrate/remove — kept as separate composable pieces rather than one monolithic function, since the two known future callers need different subsets:migration squash(CLI-1969) needs create → health-wait → connect → setup only, whiledb diff --use-pgadmin(CLI-1968) needs create → health-wait → migrate).legacy/commands/db/shared/legacy-shadow-source.tscomposes these fordb diff/db pull's--target-localdeclarative branch, which also needs pg-delta.legacy-pgdelta.apply.tsis a from-scratch port of Go'spgdelta.ApplyDeclarative.Hoisted a shared
legacyResolveDbSetupPrelude(db-setup.ts) so fresh-db setup and shadow setup stop duplicating the same JWKS/image-pull resolution, per this repo's "Hoist Before You Duplicate" rule.Why
Part of the M9 milestone (Go removal) — this and the three PRs below it in the stack (#6021 CLI-1953, #6022 CLI-1954, #6026 CLI-1955) progressively remove the Go delegations that anchor the bundled Go binary. This PR removes the last one blocking
db diff/db pull's native engines.Reviewer-relevant context
shadow-<uuid>, avoiding a collision hazard the old constant"shadow"id had) and reclaimed at the shadow's own teardown (legacyRemoveShadowDatabase) rather than via the shared name-keyed sweep, since the shadow container is unnamed and that sweep can never find it.db pullno longer wires theLegacyDeclarativeSeamlayer — it has no remaining caller of it.db diffstill needs it (--use-pgadmin/--use-pg-schemastill delegate).Fixes CLI-1956