Skip to content

feat(cli): port shell completion to native TypeScript (CLI-1965) - #6083

Draft
Coly010 wants to merge 9 commits into
developfrom
columferry/cli-1965-port-shell-completion-to-typescript-and-remove-the
Draft

feat(cli): port shell completion to native TypeScript (CLI-1965)#6083
Coly010 wants to merge 9 commits into
developfrom
columferry/cli-1965-port-shell-completion-to-typescript-and-remove-the

Conversation

@Coly010

@Coly010 Coly010 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

What changed

Replaces the Go-binary passthrough for shell tab-completion with two native TypeScript implementations, closing CLI-1965:

  1. Static scripts (legacy/commands/completion/legacy-completion-scripts.ts) — supabase completion {bash,zsh,fish,powershell} now generates the script natively instead of proxying to the Go binary. Cobra v1.10.2's completion scripts turned out to be 100% generic templates that don't bake in the command tree at all (every tab press just shells back out to supabase __complete/__completeNoDesc), so this is a byte-for-byte transcription of cobra's own genBashComp/genZshComp/genFishComp/genPowerShellComp functions, parameterized only by the program name ("supabase") and which hidden command the script calls back into. Pinned against real cobra output via 8 checked-in golden fixtures (legacy/commands/completion/__fixtures__/) generated from a real apps/cli-go build, so a future accidental edit to the hand-transcribed templates fails CI instead of shipping silently.

  2. Dynamic responder (legacy/cli/legacy-complete.ts, replacing the deleted complete-passthrough.ts) — reimplements cobra's __complete/__completeNoDesc protocol (candidates + a trailing :<directive> line) by reflecting over the live legacyRoot command tree, rather than hand-authoring a separate Go-shaped shadow model or continuing to shell out to the Go binary. Reflecting over the real tree means completion output self-corrects as the tree's own, separately-tracked content bugs (extra/missing commands, description mismatches) get fixed elsewhere.

This removes the completion command family's last dependency on the bundled Go binary — the structural blocker the milestone description calls out, since every cmd/*.go command registration was load-bearing for tab completion even where the handler itself was already dead. Unblocks the final Go binary trim.

How the cobra-output-matching question was resolved

Delegated protocol research to go-parity-auditor, which read cobra v1.10.2 source directly (available in the Go module cache) and cross-checked against apps/cli-go. Two categories of cobra behavior turned out to need different treatment:

  • Static scripts: provably independent of the command tree — a pure string-template port, verified byte-identical against a real cobra-built binary (both at generation time and end-to-end through the actual TS CLI subprocess).
  • Dynamic protocol: cobra annotations (MarkFlagRequired, MarkFlagFilename) that have no equivalent concept anywhere in this TS tree are mirrored as small, explicit, hand-verified lookup tables (matching Go's own hardcoded cmd/*.go call sites 1:1) rather than derived generically from TS flag declarations — an earlier attempt to infer "required" from whether a flag was Flag.optional-wrapped was a real, confirmed-wrong heuristic (it silently disagreed with cobra on 3 of 6 real required flags, including flags this TS port deliberately made optional at parse time for unrelated validation-ordering reasons) and was replaced with an explicit table during review.

Review findings and how they were resolved

Three independent reviewers (go-parity-auditor, engineer-reviewer, architect-reviewer) ran differential testing against a real apps/cli-go build and converged on the same set of real regressions in the first draft of the dynamic responder, all now fixed and covered by new regression tests:

  • Any global flag before the cursor (e.g. supabase --debug <TAB>) was incorrectly suppressing all subcommand-name completion.
  • A non-root command's own declared global flags (e.g. seed's --linked/--local) were invisible from anywhere in that command's subtree.
  • A command's own local flag with the same name as a global flag (e.g. db diff's local --output) was offered twice, with contradictory descriptions, instead of the local one shadowing the global one.
  • --version was offered on every command instead of the root only (cobra registers it non-persistently, root-only).
  • The --help/--version short-circuit could misfire on a subcommand's own unrelated local flag of the same name (e.g. migration squash --version <N>).
  • The completion-candidate directive was 4 (no-file-completion) too eagerly in cases cobra leaves at 0.

Deliberately left open / documented, not fixed: mutually-exclusive flag-group hiding (cobra's MarkFlagsMutuallyExclusive, ~45 call sites in apps/cli-go/cmd/) is not reproduced — there's no equivalent annotation anywhere in this TS tree to derive it from, and hand-building a ~45-entry shadow table was judged materially higher transcription-error risk than the small, stable tables this PR does maintain (4 file-extension entries, 6 required-flag entries). Deprecated-command/flag filtering is similarly not reproduced, since this TS tree has no "deprecated" concept distinct from "hidden" today. Both are called out in legacy-complete.ts's module doc comment and the completion family's SIDE_EFFECTS.md.

Testing

Unit tests for the pure command-path-resolution/flag-collection/classification/formatting logic in legacy-complete.ts (against the real legacyRoot tree, not a synthetic one) and for legacy-completion-scripts.ts (including the golden-fixture byte-exact checks); a small e2e file for each covering the real-subprocess golden paths (__complete, __completeNoDesc, completion bash/zsh); a new shared legacy-param-introspection.ts unit test covering the Param unwrap logic (hoisted out of legacy-command-instrumentation.ts, which had a private, near-identical helper).

Replaces the Go-binary passthrough for shell tab-completion with two
native implementations: a static script generator that transcribes
cobra v1.10.2's own bash/zsh/fish/powershell templates byte-for-byte
(pinned against real cobra output via checked-in golden fixtures), and
a dynamic __complete/__completeNoDesc responder that reimplements
cobra's completion protocol by reflecting over the live legacyRoot
command tree instead of shelling out to the Go binary.

Deletes complete-passthrough.ts and the four Go-proxy completion
handlers, removing the last dependency the completion command family
had on the bundled Go binary — this was the structural blocker for
trimming the Go binary (every cmd/*.go registration was load-bearing
for tab completion even where the handler itself was dead).
@Coly010

Coly010 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e92b0b2b5

ℹ️ 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".

// the root command (gated on `c.Version != ""`, and non-persistent) — it
// is never inherited by subcommands the way `--help` is.
...(commandChain.length === 1 ? [GlobalFlag.Version.flag] : []),
...ancestors.flatMap((ancestor) => legacyInternalCommand(ancestor).contextConfig.flags),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include the final command's shared flags

When completing a shared flag on the command that declares it (for example supabase db schema declarative --no-c<TAB> before selecting generate or sync), finalCommand is the declarative group, but this collector only reads contextConfig.flags from ancestors, so --no-cache is not offered until after a child command has been selected. That regresses completion for a valid Go-compatible persistent flag; include the final command's shared flags as well.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejecting this one — investigated and the described regression doesn't reproduce.

Command.withSharedFlags (effect/unstable/cli/Command.ts) merges the shared flag config into both impl.contextConfig (inherited by descendants, mergedContextConfig) and impl.config (the declaring command's own local flags, mergedConfig = mergeConfig(impl.config, sharedConfig)). legacyCollectInScopeFlags already reads legacyInternalCommand(finalCommand).config.flags unconditionally for the resolved command — so a shared flag declared on finalCommand itself is present via that merged config, independent of the ancestors-only contextConfig.flags spread this finding points at.

Verified empirically against the real command tree rather than just re-reading the source:

legacyRespondToComplete(legacyRoot, ["__complete", "db", "schema", "declarative", "--no-c"])
// => { candidates: [{ name: "--no-cache", description: "Disable catalog cache and force fresh shadow database setup." }], directive: 4 }

--no-cache is already offered on declarative itself, before generate/sync is selected — exactly the case described as regressing. No change needed here; the two existing unit tests around declarative/shared flags (legacy-complete.unit.test.ts) already cover the descendant case and continue to pass.

Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
Comment thread apps/cli/src/legacy/commands/completion/bash/bash.command.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Coly010 added 2 commits August 5, 2026 15:14
…ity (review)

Addresses three Codex review findings on PR #6083 (legacy-complete.ts):

- Replace the `as unknown as LegacyCommandInternal` assertion with a runtime
  type guard (legacyHasCommandInternals), mirroring the precedent already
  established in legacy-param-introspection.ts for the same "internal-only
  effect/unstable/cli field" problem. The repo's typing rules forbid `as`
  casts to paper over Effect/CLI typing gaps.
- Synthesize cobra's auto-registered `help` subcommand in root-level
  `__complete` output (InitDefaultHelpCmd registers it only on whichever
  command Execute() runs against, never recursively on descendants) -
  verified empirically against a real apps/cli-go build.
- Short-circuit to zero candidates with the Default directive when a
  flag-shaped token doesn't resolve to any in-scope flag, matching cobra's
  finalCmd.ParseFlags() failing outright on an unrecognized flag - a failure
  that wins even over the --help/--version short-circuit. Also verified
  empirically.

A fourth finding (shared flags via Command.withSharedFlags not visible on
the declaring command itself before a child is selected) was investigated
and rejected: Command.withSharedFlags already merges shared flags into the
declaring command's own `.config`, not just descendants' `.contextConfig`,
so the described regression does not reproduce.
…y (review)

Go's completion {bash,zsh,fish,powershell} run through cobra's normal
Execute() path (unlike __complete/__completeNoDesc, which bypass it), so
cli_command_executed fires for them today - there is no completion-specific
telemetry exemption in apps/cli-go/cmd/root.go. Since CLI-1965 replaced the
Go-proxy handler with a native one, that event stopped firing.

Pipe all four leaves through withLegacyCommandInstrumentation({ flags }) +
withJsonErrorHandling, matching every other native legacy command, and
provide commandRuntimeLayer(["completion", "<shell>"]) the same way
telemetry/enable, telemetry/status, and init do for commands with no
Management API runtime, so the recorded `command` telemetry property
reflects the real resolved path.

Adds an integration test per shell asserting cli_command_executed is
actually captured, and updates the "--no-descriptions from real argv"
test's layer to satisfy the wrapper's Analytics/ProcessControl/Stdio
requirements.
@Coly010

Coly010 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 360cce7fe5

ℹ️ 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".

Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
…-command parsing (review)

Four Go-parity gaps in the native __complete responder, each verified against
a real apps/cli-go build:

- Validate flag values (not just names) before completing, matching pflag's
  typed Value.Set failing ParseFlags for e.g. `-o not-a-format` or
  `--debug=maybe` (zero candidates, Default directive).
- Stop command-path descent at a bare `--` end-of-flags sentinel instead of
  still matching subcommands after it.
- Walk every shorthand in a boolean flag cluster (e.g. `-rj`) as changed, not
  just the last character, so `--recursive` isn't re-offered after `-rj 2`.
- Consume a value-taking flag's value during path descent even when the flag
  isn't yet in scope (e.g. `--db-url` typed before `db dump`), mirroring
  cobra's stripFlags optimistically assuming an unknown flag takes a value.

review: PRRT_kwDOErm0O86Wsq4H, PRRT_kwDOErm0O86Wsq4K, PRRT_kwDOErm0O86Wsq4P, PRRT_kwDOErm0O86Wsq4X
@Coly010

Coly010 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0024c9e66

ℹ️ 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".

Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
…oolean=value parsing (review)

Six new Codex findings from the latest __complete review round, each
confirmed against a real apps/cli-go build (rebuilt with the release
version ldflag for faithful --version behaviour) and fixed:

- A bare `--` sentinel now disables flag-name/flag-value completion
  entirely (Cases 1/2), matching cobra's flagCompletion gate, while
  legacyChangedFlagNames also stops scanning at `--` and correctly skips
  a long flag's consumed value token so a required flag after the
  sentinel is still offered.
- legacyFindUnresolvedFlagToken now resolves short-flag clusters via a
  new legacyResolveShortFlagCluster helper that walks pflag's real
  first-character-owns-the-value semantics (`-j4`, `-ojson`), instead of
  reusing legacyResolveFlagFromToken's last-character cobra heuristic,
  which remains correct for its other two use sites.
- A trailing value-taking flag with no value at all is only a hard
  parse error when toComplete is itself flag-shaped (mirroring cobra's
  checkIfFlagCompletion rescue condition); otherwise it still falls
  through to flag-VALUE completion. The Case 2 "preceding token"
  branch now also hard-stops (instead of silently falling through) when
  that token is unresolved under cobra's own last-character heuristic,
  which is what real cobra does for inputs like `-ojson ""`.
- A boolean flag with an explicit `=` (`--debug=maybe`) is flag-VALUE
  completion, not noun completion — cobra only resets to noun completion
  in the no-`=` two-token case.
- The five completion leaves (`completion`, `completion
  {bash,zsh,fish,powershell}`) now force the NoFileComp directive,
  mirroring cobra's ValidArgsFunction: NoFileCompletions registration,
  which getCompletions always applies last and unconditionally.

The one earlier "shared flags not visible" finding on this same file is
a separate, already-adjudicated rejection and is left as-is.
@Coly010

Coly010 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0cf5e840f1

ℹ️ 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".

Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
…nd per-command --output validation (review)

- legacy-complete.ts: `help <path> <toComplete>` now re-resolves the path
  after `help` from root (mirroring cobra's auto-registered help command's
  own ValidArgsFunction), instead of treating `help` as an unresolved
  leftover and returning nothing.
- legacy-complete.ts: `functions deploy --jobs`, `migration down --last`,
  and `db reset --last` are Go UintVar/UintVarP flags that reject a leading
  sign; the generic signed-Integer regex used for completion validation
  wrongly accepted negative values. Hoisted `legacyParseUintBase0` (Go's
  strconv.ParseUint(s, 0, 64), previously storage/cp-only) into
  legacy/shared/ and reused it for these three flags.
- legacy-complete.ts: the global LegacyOutputFlag's choiceKeys is the union
  of root's 5-value enum and db query's own 3-value enum, so completion
  validation accepted db query's table/csv values everywhere and the
  resource commands' env/pretty/toml/yaml values inside db query. Restored
  per-command validation.

All three verified empirically against a real apps/cli-go build (release
version ldflag set).
@Coly010

Coly010 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38c536a4e3

ℹ️ 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".

Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
…sion Changed check, and bare-dash handling in shell completion (review)

- legacyIsValidFlagValue now accepts base-0 integers (0x/0o/0b/leading-zero
  octal), and checks the uint/CSV overrides before dispatching on
  primitiveTag so they also catch string-typed flags like storage cp --jobs
  and every StringSliceVar-backed variadic flag (--domains, --schema, etc.),
  with db reset --sql-paths excluded as Go's one plain StringArrayVar.
- legacyFindUnresolvedFlagToken validates a short flag cluster's attached
  value even when the owning flag is boolean (-f=value), matching pflag.
- legacyMatchesFlagToken makes the --help/--version short-circuit match
  Changed semantics (--help=false counts, not just a bare --help).
- legacyHasUnconsumedFlagTerminator replaces a raw `includes("--")` check so
  a `--` already consumed as a preceding flag's own value doesn't disable
  flag completion for the rest of the request.
- legacyResolveCommandPath and the flag-value "preceding token" check both
  now treat a bare `-` as the positional pflag considers it, not a flag.

All five fixes verified against a real apps/cli-go build (release version
ldflag set), diffing actual __complete output before/after.
@Coly010

Coly010 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cb64005d42

ℹ️ 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".

Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
Comment thread apps/cli/src/legacy/cli/legacy-complete.ts
…sion Changed tracking, unknown-root gating, flag sort order, and global-flag description text in shell completion (review)

Fixes six fresh Codex findings from the latest review round:

- legacyIsValidFlagValue's Integer case validated backups restore
  --timestamp (an Int64VarP in Go) against the wider uint64 bound instead
  of int64's narrower, asymmetric two's-complement range, wrongly
  accepting 9223372036854775808 (one past int64 max). Hoists a shared
  base-0 digit-parsing core in legacy-parse-uint.ts and adds
  legacyIsValidBase0Int64 alongside the existing legacyParseUintBase0.

- gen types --query-timeout, gen bearer-jwt --valid-for (DurationVar), and
  --exp (TimeVar, RFC3339) are plain Flag.string in TS with no validation,
  so a bogus value fell through to the default "always valid" case. Adds
  legacyIsValidGoDuration/legacyIsValidGoRfc3339 plus per-flag override
  tables, checked the same way the existing uint/CSV overrides are.

- The --help/--version short-circuit scanned raw tokens for the literal
  strings, over-triggering past a genuine `--` terminator or when the
  token was actually consumed as a preceding flag's value. Now reads
  legacyChangedFlagNames (which already respects both) instead.

- Flag-name completion after an unmatched ROOT-level positional (e.g.
  `nosuch --d`) still offered root's flags; cobra's Command.Find ->
  legacyArgs errors in exactly that case (root itself, unresolved,
  leftover positional), ahead of the help/version short-circuit too. Adds
  that gate, filtering out the bare `-`/empty-string leftovers pflag's own
  stripFlags excludes, and exempting genuine `help ...` requests (which
  re-resolve through their own, already-correct path).

- legacyCollectInScopeFlags emitted flags in tree-declaration order;
  cobra's completion path walks InheritedFlags().VisitAll then
  NonInheritedFlags().VisitAll, each alphabetically sorted by pflag. Splits
  the collector into an inherited (ancestor) block and an own/local block,
  each sorted, with the own block excluding any name it shadows from the
  inherited one (mirroring pflag's own shadow-skip) instead of relying on
  Map re-insertion order.

- Root persistent flag descriptions in global-flags.ts were capitalized
  with trailing periods; Go's own cmd/root.go registrations are lowercase,
  no period. This only became user-visible once native completion started
  echoing these descriptions verbatim in `__complete` output.

All six verified empirically against a real apps/cli-go build (ldflag
-X .../utils.Version=1.999.0) diffing __complete output before writing the
fix, per this PR's established review methodology.
@Coly010

Coly010 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 09a2a62dca

ℹ️ 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".

Comment thread apps/cli/src/legacy/cli/legacy-complete.ts Outdated
if (visibleSubcommands.length > 0) {
directive = LegacyCompletionDirective.NoFileComp;
const subcommandCandidates: Array<LegacyCompletionCandidate> = visibleSubcommands.map(
(sub) => ({ name: sub.name, description: sub.shortDescription ?? sub.description }),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Use Go short descriptions for subcommands

For root subcommand completion such as __complete br, this now emits the existing TS short description Manage preview branches from branches.command.ts:13, while the replaced Go responder emitted Manage Supabase preview branches from apps/cli-go/cmd/branches.go:29. Because the TS command metadata is not uniformly byte-identical to Cobra's metadata, completion descriptions regress unless this path uses Go-parity text or those short descriptions are normalized.

AGENTS.md reference: apps/cli/AGENTS.md:L483-L485

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed the underlying claim — branches.command.ts does say "Manage preview branches" while apps/cli-go/cmd/branches.go:29 says "Manage Supabase preview branches" — but I don't think the fix belongs in this file or this PR.

Sampling more of the legacy command tree against Go's Short: text turns up the same drift in several other places, not just branches:

command TS withShortDescription Go Short:
config "Manage project configurations" "Manage Supabase project configurations"
inspect "Inspect project tools" "Tools to inspect your Supabase project"
projects "Manage projects" "Manage Supabase projects"
sso "Manage Single Sign-On (SSO) authentication" "…for projects"
db "Manage databases" "Manage Postgres databases"
seed "Seed a Supabase project" "Seed a Supabase project from " + utils.ConfigPath (dynamic)

There are 166 withShortDescription/withDescription call sites across apps/cli/src/legacy/commands/, set individually as each command was natively ported in its own PR over many months — none of them are touched by this PR's diff (git diff origin/develop...HEAD -- apps/cli/src/legacy/commands/branches/branches.command.ts is empty).

legacy-complete.ts deriving descriptions from the live TS command tree (rather than a hardcoded Go-text table) is the correct design here — the file's own doc comments already reject that shape of hardcoded shadow table except for the few cases (like LEGACY_COMPLETION_FLAG_FILE_EXTENSIONS, 4 entries) where there's truly no better source. A 166-entry Go-text table to work around this would be exactly the anti-pattern this file otherwise avoids, and it would still need to be kept in sync by hand forever.

The real bug is upstream: per-command short-description text drifted from Go's Short: across many already-merged ports. That's a legitimate but separate tree-wide parity audit, not something this "port the completion protocol" PR should absorb (it would need to touch ~166 unrelated command files well outside this PR's scope). Leaving this thread open rather than resolving it — flagging for a follow-up cleanup issue rather than a change here.

…n (review)

Go's time.Parse(time.RFC3339, s) independently caps the offset hour at
24 (not 23) and the offset minute at 60 (not 59), rejecting anything
above those bounds. The completion validator's regex had no bound on
the offset at all, and its doc comment claimed Go "never range-checks
the offset" — disproven empirically against a real go1.26 build
(+25:00 and +00:61 both error with "time zone offset hour/minute out
of range", while +24:00 and +00:60 parse successfully).

Capture the offset hour/minute and validate them against the same
24/60 bounds real Go uses.
@Coly010

Coly010 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8ca3966720

ℹ️ 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".

// the root command (gated on `c.Version != ""`, and non-persistent) — it
// is never inherited by subcommands the way `--help` is.
...(commandChain.length === 1 ? [GlobalFlag.Version.flag] : []),
...legacyInternalCommand(finalCommand).config.flags,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include shared flags on their declaring command

Because ownParams includes the final command's local/global flags but omits its own contextConfig.flags, a Command.withSharedFlags flag is visible only after that command becomes an ancestor. The sole current case is db schema declarative --no-cache: apps/cli-go/cmd/db_schema_declarative.go:549-552 registers it on dbDeclarativeCmd.PersistentFlags(), but __complete db schema declarative --no cannot suggest it, and placing --no-cache before generate can prevent descent to that leaf. Include the final command's shared flags in the non-inherited block.

AGENTS.md reference: apps/cli/AGENTS.md:L483-L485

Useful? React with 👍 / 👎.

Comment on lines +790 to +791
const GO_RFC3339_PATTERN =
/^(\d{4})-(\d{2})-(\d{2})T(\d{2}):(\d{2}):(\d{2})(?:\.\d+)?(?:Z|[+-](\d{2}):(\d{2}))$/;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Accept comma-separated RFC3339 fractions

When --exp uses the valid RFC3339 spelling 2024-01-02T15:04:05,5Z, this regex rejects it, so __complete gen bearer-jwt --role anon --exp 2024-01-02T15:04:05,5Z --p returns no candidates. Go's time.Parse documentation explicitly permits either a comma or decimal point before fractional seconds, and the Go command registers --exp as a TimeVar, so the previous responder accepts this value and continues with --profile/--payload completion; permit , here as well.

AGENTS.md reference: apps/cli/AGENTS.md:L483-L485

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant