feat(cli): add a quick start interactive wizard

[aparajon]

What

Add an interactive schemabot init wizard for MySQL and PostgreSQL. It connects your database, detects existing schema files, guides storage setup, and verifies the baseline before showing your next command.

Generated with Codex

[Copilot]

🟡 Changes recommended

The wizard currently prompts even when stdout is redirected (making prompts invisible) and namespace collection can accept empty entries; both are user-facing correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds an interactive “init” wizard to the SchemaBot CLI so first-time database setup can be completed without knowing every flag, while keeping the underlying initialization/verification path shared with non-interactive usage.

Changes:

• Make schemabot init accept missing flags and, when run in a terminal, prompt for the required decisions before calling the existing shared init workflow.
• Add wizard-focused unit/integration coverage and update docs with an end-to-end initialization walkthrough.
• Add scripts + assets to record and render the demo wizard/plan GIF used by the new documentation.

File summaries

File	Description
scripts/render-init-demo.cjs	Renders the recorded wizard + first plan into a GIF for docs.
scripts/record-init-demo.py	Records real CLI wizard output and a follow-up plan into a JSON “recording”.
pkg/cmd/main_test.go	Adjusts CLI parsing expectations so init no longer requires flags at parse-time.
pkg/cmd/commands/init.go	Routes init through input collection (wizard/non-interactive) before running the shared initialization workflow; adds progress reporting hooks.
pkg/cmd/commands/init_wizard.go	Implements missing-input detection and the interactive prompt flow (wizard).
pkg/cmd/commands/init_wizard_test.go	Unit tests for wizard input collection, cancellation, and non-interactive missing-input reporting.
integration/localruntime/init_test.go	Integration coverage for --non-interactive --json missing-input output.
docs/init.md	New docs page describing interactive and non-interactive initialization, with examples and demo GIF.
assets/src/init-demo.html	HTML renderer that animates the recorded wizard/diff/plan for the demo GIF.
assets/src/init-demo-recording.json	Recorded wizard session + plan output used by the demo renderer.

Review details

Suppressed comments (1)

pkg/cmd/commands/init_wizard.go:148

• Namespace parsing trims whitespace but can still append empty entries when the user types extra commas (e.g., shop,). That makes missingInputs() think scope is provided, but later validation fails (or could accidentally proceed with an empty scope). Re-prompt until at least one non-empty namespace is collected.

	if len(cmd.Namespaces) == 0 {
		fallback := cmd.Database
		if cmd.Type == "postgres" {
			fallback = "public"
		}
		selected, err := ask("Namespaces (comma-separated)", fallback)
		if err != nil {
			return err
		}
		for namespace := range strings.SplitSeq(selected, ",") {
			cmd.Namespaces = append(cmd.Namespaces, strings.TrimSpace(namespace))
		}
	}

• Files reviewed: 10/11 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1334, cb69e65.

Verdict: 7 findings — 0 blocking, 4 non-blocking, 3 suggestions. The split the PR set out to keep is genuinely kept: the wizard collects decisions and initialize still owns registration, staged verification, publication and profile persistence, so AZ-6 holds as the body claims. The findings are all in the new prompt layer — two input shapes that let a user answer every question and then hard-fail, and a coverage floor where the flag the feature depends on is never the deciding term in any test.

Non-blocking

1. The reuse prompt gates on the path existing, not on schema files existing, and declining aborts instead of importing. init_wizard.go:161 is if _, err := os.Stat(cmd.SchemaDir); err == nil && !cmd.ReuseSchema {, so a plain mkdir -p schema beforehand — with schema the flag default at init.go:33 — asks "Verify and reuse the existing schema files?" about an empty directory, and the printed default n returns setup cancelled; choose a new schema directory to import into (:167) after all nine answers are in. Answering y is no better: an empty dir snapshots to one "./" entry, so LoadCLIConfig(stage) fails with schemabot.yaml not found in /…/.schemabot-init-XXXX — a temp path the deferred cleanup has already deleted — and it arrives after localsetup.Register at init.go:118. An errors.Is(err, fs.ErrNotExist) test plus a non-empty check would make an empty or absent directory the fresh-import path it should be.

2. A trailing comma in the namespace answer discards the whole session. init_wizard.go:145-147 appends every split element unconditionally, so shop, yields ["shop", ""]; missingInputs only tests len(...) == 0 (:25), so the summary prints, consent is taken, and the first thing initialize does — onboardPullNamespaces at init.go:79 — rejects it at onboard.go:154 with namespace "" must be non-empty …. Nothing is registered and no directory is staged (both come later, at :96/:118), so the only cost is nine re-typed answers — but that is the whole point of a wizard. Skipping empty elements is a one-line fix.

3. --non-interactive is never the deciding term in any test. Deleting cmd.NonInteractive || from init_wizard.go:39, or deleting || !ui.IsTerminal(os.Stdout), leaves go test ./pkg/cmd/... green. Note it is not stdin that masks them — go test hands the binary /dev/null, which is a character device, so ui.IsTerminal(os.Stdin) is true; the masking is that init_wizard_test.go:35 sets NonInteractive: true and runs with a piped stdout, and init_test.go:53 passes --non-interactive --json through CombinedOutput. Hoisting the two IsTerminal results into an injectable field would let one table test over {NonInteractive, JSON, interactive} pin all three branches and move the missing_inputs contract out of the container-gated layer.

4. Progress output is conditional on flag completeness rather than on there being a human. cmd.progress is assigned only at init_wizard.go:54, after prompting, and collectInputs returns at :36-37 when nothing is missing — so a fully-flagged terminal run prints nothing at all across manager.Ensure's 30 s budget, the live-schema pull and the plan, since reportProgress is a nil no-op (init.go:237-239). The child server's own output goes to runtime.log, so the terminal really is silent. The sibling command already solves this unconditionally with withLoading("Pulling live schema...", true, …) at onboard.go:43.

General suggestions

5. --type now has validation in neither the grammar nor the tests. Dropping enum:"mysql,postgres" was mandatory, not sloppy — kong v1.16.1 refuses to build a scalar enum with no required and no default (tag.go:327), and with default:"" it rejects the omitted flag outright; I confirmed both against the pinned version. And nothing was lost from --help, which never rendered enum values. What remains is that the replacement check at init_wizard.go:32-33 and the re-ask loop at :104-111 have zero coverage (collapsing the loop to a single ask keeps the suite green), and that enum:",mysql,postgres" default:"" is an available if cosmetically noisy way to get the constraint back into the grammar.

6. docs/init.md is unreachable from anywhere in the repo. git grep for init.md at this head returns nothing — every other file under docs/ has at least one inbound reference even when the curated README index omits it (check-runs.md, namespaces.md), so this one is uniquely orphaned, and README.md:124 still sends new users to schemabot onboard with no mention of init. The 1.5 MB GIF is fine by local precedent (pr-demo.gif is 31 MB). Two smaller doc notes: docs/init.md:41 presents --non-interactive and --json as orthogonal when --json alone also suppresses the wizard, and render-init-demo.cjs:3 says Chrome is required after this very head made it a fallback.

7. Prompt-layer polish. An explicitly supplied --profile is re-asked (:157) while the other four fields are skipped when set (:126); Globals.Profile has no kong default and no envar binding, so == "" really does mean "not passed" (SCHEMABOT_PROFILE is read later, inside client/config.go, so a guard would still show it as the default — --schema-dir is genuinely not distinguishable and should stay). The empty-answer retry at :129-130 re-prompts silently where the engine loop prints Choose mysql or postgres.. And the two Ctrl-C paths return different shapes — bare err at :73 versus setup cancelled: %w at :85 — which the new test cannot distinguish, since ErrorIs(context.Canceled) passes on both and started is signalled by the banner write rather than by a prompt.

The one thing that could have broken, verified

ask starts a goroutine per prompt and abandons it on cancellation, over a bufio.Reader shared by all nine prompts — the classic shape for a leak, a data race, or one prompt eating the next one's line. It is safe, for three separate reasons and I checked each. ready is buffered (make(chan answer, 1)), so an abandoned sender never blocks and never leaks. Two ask calls can never overlap: the <-ctx.Done() arm returns an error that unwinds promptInputs and collectInputs immediately, so no second ask is ever issued on that reader — and the reader itself is constructed fresh per promptInputs call (:61). A race probe produced no product-code race, only one inside the probe's own buffer. The abandoned reader does swallow bytes destined for a later reader, but schemabot init exits after a cancelled wizard, so the window closes at process exit. Cancellation itself is handled by the global signal.Notify in main.go plus the bound context, not by a missing local handler.

Verified correct

• No credential can reach an error string: both connections are validated as env:VARIABLE references at init.go:74-77, and the child server's stdout/stderr go to a runtime.log rather than being inlined.
• missing_inputs and initialization_error can never both print — the ErrSilent guard at init.go:51 short-circuits, and every failure path exits 1.
• --database "" is now stricter, not looser: kong's required accepted an explicit empty value, and the new TrimSpace check classifies it as missing.
• No invocation shape hangs. TTY and docker run -it prompt; CI, | tee, heredoc stdin and untty'd docker take the missing-flags error. Gating on stdout is right, not a convention breach — the prompts are written to stdout, and pkg/ui deliberately puts colors and hyperlinks on stdout while the spinner sits on stderr.
• AZ-6 is upheld, not moved: the mismatched-profile guard, RENAME_NOREPLACE publication, the preserve-on-conflict fallback and the retains-runtime wrap are all still in init.go and the two init_publish_* files, which the *Enforced:* line still names correctly. The wizard's own contract comment says as much.
• Terminology clean (no "migration", no hyphenated "schema-change"); t.Context() and testify used correctly; promptSignalWriter's default: is the once-only-notify idiom, not a silent branch.
• Both JSON shapes and the missing field ordering match docs/init.md exactly; the documented empty-namespace and failed-setup-retains-runtime claims both check out.
• Demo assets carry no credential, hostname or internal identifier; the recorder's frame-dedup delay accumulation is correct and its magick argv is ~40× under ARG_MAX.
• go build ./..., go vet ./pkg/cmd/..., gofmt, and go test -race ./pkg/cmd/... all clean at this head; every CI check green.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Thanks for the detailed review. I checked all seven findings against the current wizard and addressed the remaining items in 66ec952, 25f49ab, and 240628e:

• Empty schema directories now take the fresh-import path. Publication uses directory-only removal followed by a no-replace rename, preserving concurrent files and rejecting symlinks. Regression tests cover these protections.
• Namespace input is validated before leaving its screen; a trailing comma stays in the wizard with an error. Namespace discovery now supplies the normal selection path.
• Terminal-mode tests independently cover --non-interactive, JSON, redirected input, and redirected output. Fully specified interactive terminal runs now receive progress output too.
• Invalid engine input has regression coverage. The picker only offers supported engines.
• The setup guide is linked from the configuration docs; it explicitly says JSON suppresses prompts, and the renderer comment now describes the browser fallback correctly.
• Profile defaults remain editable in the final review without an extra prompt. Empty input produces inline feedback, and cancellation uses the shared Bubble Tea/context path.

The full CLI command package passed with the race detector, and the required commit/push hooks passed. CI is running on the updated head.

— Codex (GPT-6)

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1334, ee5457d.

Scope: delta re-review only — cb69e650 → ee5457d5 (24 files, +1414/−330). The earlier review covered the PR up to cb69e650. Of its 7 findings, 5 are closed, 2 partially (--type still has no enum tag on the grammar; docs/init.md now has exactly one inbound link, from docs/configuration.md, and the README still routes new users to onboard).

Verdict: 8 findings — 2 blocking (a wizard dead-end on multiple explicit namespaces; ctrl+c no longer stops init at all), 5 non-blocking, the rest suggestions. The delta replaces the line-based prompt with a bubbletea wizard that now dials the database; the credential-redaction story holds, the terminal-control story does not.

Blocking

1. The wizard refuses the value it pre-filled, with no way forward but deleting a space. validate() case 5 splits on , without trimming elements, while line 60 seeds the field with strings.Join(cmd.Namespaces, ", "). Reproduced: init --namespace sales --namespace west, three shift+tab from review, Enter → namespace " west" must be non-empty and contain no leading or trailing whitespace, step stuck at 5; shift+tab then Enter returns to 5 because m.editing disables the skip loop, so the only exits are manual editing or esc. It is a pure false negative — namespaceChoices trims per element and would have accepted it; the one existing test uses a single namespace, where Join emits no separator.

2. ctrl+c and esc no longer stop init, and the two-press force-exit is gone. initializeWithUI runs bubbletea with tea.WithInput(os.Stdin) (which clears ISIG via term.MakeRaw) plus tea.WithoutSignalHandler(), so main.go's signal.Notify → os.Exit(130) escape hatch never fires; Update only sets stopping and calls cancel(). But CallPullSchemaAPI (init.go:146) and CallPlanAPI (init.go:166) take no context — they use context.Background() under a 30 s client timeout — and the only ctx.Err() check sits after both, so cancellation is inert for ~60 s while the UI shows "Finishing cleanup…". Verified under a real pty: two 0x03 presses produced byte-identical output and the process survived; the control arm without bubbletea exited on the first. client.doSlowPostIntoCtx exists for exactly this and is the fix.

Non-blocking

3. Namespace names from the remote catalog reach the terminal with escapes intact. namespaceView renders names[i] with a bare %s, and so do the single-result notice and the review screen; validateRelativePathPart accepts all control bytes, and nothing in lipgloss, bubbletea's standardRenderer, or ansi.Truncate strips them. Demonstrated end to end: CREATE SCHEMA "a<ESC>[2Jb" on a live PG 16 is returned by the discovery query, passes the reserved-namespace filter, and survives into View(). The sibling file says the quiet part out loud — "Quoting also prevents database names from injecting terminal controls" — and has a test pinning it; the picker whose whole job is a trust decision has neither.

4. Catalog discovery is now a hard gate with no manual fallback. The deleted prompt defaulted to public (postgres) or the database name (mysql); now namespaceKey accepts only enter (retry) and shift+tab when len(m.names) == 0. A role that can read its own tables but not evaluate has_schema_privilege over pg_namespace therefore cannot complete interactive setup at all, and must independently know to abandon and re-run with --namespace. docs/init.md documents the stop but not the workaround.

5. The wizard→command copy-back is executed by no test. The ten assignments at promptInputs are unpinned: making the ReuseSchema assignment unreachable leaves go test ./pkg/cmd/commands/ green (verified), and so would swapping cmd.DSN/cmd.StorageDSN — registering the application DSN as the state database. The deleted TestInitWizardCollectsInputsWithoutInitializing asserted exactly this plus the AZ-6 property that the wizard creates no ~/.schemabot; neither assertion survives.

6. A non-empty schema dir without schemabot.yaml now fails late and points at a deleted path. Line 363 sets ReuseSchema on any non-empty directory, so a stray README.md routes into stageExistingInitSchema and fails after localsetup.Register with schemabot.yaml not found in /…/.schemabot-init-XXXX — naming the staging temp dir the deferred os.RemoveAll has already removed. This is the residual half of prior finding 1: the gate correctly moved off os.Stat, but "non-empty" still is not "has schema files".

7. Two of the delta's own fixes are unpinned by their tests. Mutating initConnectionSummary's case "mysql" to fall through to default keeps the suite green — TestInitConnectionSummaryRedactsCredentials asserts only NotContains, and no test asserts host/database are actually shown for MySQL. Likewise, deleting strings.EqualFold(data.Engine, "postgres") from templates/plan.go:38 leaves it green, silently reverting commit b83e6742.

8. AZ-6's text is now false. The invariant says initialization never "replaces an existing schema directory", but publishInitSchema now rmdirs an existing empty one and renames over it. The behaviour is fine; the wording should narrow to "non-empty", and the *Enforced:* list should gain init_empty_dir_unix.go / init_publish_other.go.

General suggestions

• The review screen truncates to m.height-4, so at ≤20 rows the reuse disclosure, the state-DSN line and the "we won't change your application's schema" reassurance all scroll out while enter still confirms. The ↑/↓ scroll hint is always present, so it is signposted rather than silent — but View() clamps height only on the review step, and step 3 with an error renders 23 lines at m.height == 14.
• checkConnection overwrites m.cancelDiscovery without cancelling its predecessor and has no defer cancel(), unlike the sibling discoverNamespaces. No reachable leak today (keys are swallowed while checking), but the two should look the same.
• Three dead constructs: initWizard.check is a seam nothing assigns, Init()'s m.step == 5 branch is unreachable because skip[3]/skip[4] are never true, and cmd.interactive = true at init_wizard.go:56 re-asserts what line 33 already set.
• Update does I/O: the scroll clamp calls contentView(), which runs os.ReadDir, and steps 3/4 call initConnectionSummary — hence os.Getenv + pgx.ParseConfig, which may read PGPASSFILE — on every spinner tick.
• pkg/localsetup/check_connection.go and discover.go ship with no unit tests; their constant-string error mappings exist precisely to keep DSNs out of the UI, and a future %w would pass make test-unit.
• The PlanHeaderData.Engine change is unrelated drive-by scope, and leaves Engine silently overriding IsMySQL — the new test pins the contradictory pair {Engine: "PostgreSQL", IsMySQL: true} rather than collapsing it.

The one thing that could have broken, verified

The riskiest new mechanism is the empty-directory publish path, which rmdirs a directory the operator may own. It is fail-closed at every window: the os.Lstat symlink guard is ordered before ReadDir (so a symlinked root gets the specific refusal, and this is what the pre-existing TestPublishInitSchemaRefusesSymlinkDestination pins), unix.Rmdir returns ENOTEMPTY if anything is created between ReadDir and the call and ENOTDIR on a symlink swap, and the retried rename is RENAME_NOREPLACE/RENAME_EXCL, so a concurrently recreated target is preserved rather than clobbered. Separately confirmed that no reuse path ever deletes or overwrites a user file — ReuseSchema on the wrong directory errors at LoadCLIConfig, the database/engine check, or the namespace-set check, all before publication.

Verified correct

• Baseline green: go vet and go test ./pkg/cmd/commands/ ./pkg/localsetup/ -race -count=1 pass; all CI checks green at this SHA.
• No caller of initialize bypasses collectInputs, so cmd.interactive cannot be left unset; --json and --non-interactive both force it false and never emit escape sequences.
• Deleting the stderr progress writer regressed nothing — it was only ever installed after a successful prompt, exactly the case the progress program now covers, and reportProgress is nil-safe.
• No credential reaches an error string on the new dialing paths: mysqlconn.Open/postgresconn.Open errors are discarded rather than wrapped, and initConnectionSummary never surfaces the parser error.
• The summary cannot show a destination different from the one dialed — postgresconn.Open resolves through the same pgx.ParseConfig, including identical ambient-env fallbacks; the PGHOST/PGSERVICE concern is refuted.
• Discovery never falls back to an ambient connection, and postgres discovery correctly excludes pg_catalog, information_schema, pg_toast, the RDS schemas and the _ prefix.
• Generation guards on both message types hold in each direction — no stale accept, no fresh drop — and shift+tab bumps the generation before navigating.
• View()'s scroll slice cannot panic at any height (m.height floored at 8), m.selected is never a nil map, and configureInitWizard's skip loop always terminates.
• Untrimmed values cannot escape forward: shift+tab writes the raw input back, but every forward transition re-runs validate() and re-trims — the only leak is finding 1's false rejection.
• The new go.mod entry is atotto/clipboard, an indirect consequence of bubbles/textinput; bubbletea, bubbles and lipgloss were already direct dependencies.
• docs/init.md's completion sample matches initCompletion byte for byte, and its behavioural claims (both flags suppress prompts, connections confirmed before discovery, MySQL stays within the DSN's database, state metadata written only after review) all check out against the code.
• Prior findings 3, 4 and 7 are genuinely closed: TestInitTerminalModes is a 5-row table where each of the four conjuncts has a row that dies on its deletion; progress now gates on cmd.interactive; the profile is skip-only-editable and the two Ctrl-C shapes are distinct by construction.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Addressed the eight findings in 04fe496:

1. Namespace validation now trims comma-separated elements while preserving unchanged explicit namespace values, including names containing commas. The prefilled multi-namespace value has regression coverage.
2. Initialization now passes its context through the pull and plan HTTP requests. Tests cancel each request after it reaches a blocked server and require prompt cancellation. Ctrl+C/Escape still waits for cleanup, but no longer waits for those HTTP timeouts.
3. Catalog names are escaped for display in the picker, single-result notice, and review. The original names remain intact for selection. Tests include an ANSI clear-screen sequence.
4. Failed or empty discovery now offers m for manual namespace entry without restarting. The docs also describe --namespace as a bypass.
5. Copy-back is tested directly, including distinct application/state references, all collected fields, reuse, profile selection, confirmation, and absence of runtime initialization.
6. Non-empty directories must contain a readable SchemaBot config before review can confirm or initialization can register a runtime. Errors refer to the original directory, not a deleted staging path.
7. Tests now assert the MySQL destination and both PostgreSQL engine labels.
8. AZ-6 now specifies non-empty directories and includes the empty-directory/platform publication helpers.

I also cached connection summaries and directory disclosure outside rendering, added scrolling on short wizard screens, aligned connection-check cancellation cleanup with discovery, removed the unreachable initial-discovery branch and redundant interactive assignment, and added credential-redaction tests for both local connection helpers.

The PostgreSQL heading fix remains because the real setup demo exposed a MySQL title on a PostgreSQL plan. Bare init still validates the optional engine in the command and offers only supported engines in the picker. The initialization guide remains linked from configuration docs.

Focused tests, the build, TOC check, and required commit/push hooks passed. CI is running on the updated head.

Codex (GPT-6)

[morgo]

🤖 Review posted by Morgan's AI agent.

Approving. Big diff, but the risky surfaces are small and they're handled well.

Credential containment is structural, not just careful. init requires both DSNs as env:VARIABLE references, so InitCmd never holds a literal secret — which is what makes ExitWithJSON("initialization_error", err.Error()) safe to add on the error path. The one place a real credential does exist is CheckConnection/DiscoverNamespaces, and both deliberately drop the driver error for a generic message, with the reason written down (Driver errors can contain connection material). That's the right call for a TUI that renders whatever it's handed. initTerminalText quoting anything containing a control rune is the matching defence for catalog-supplied namespace names — a schema named with an embedded escape sequence can't repaint the wizard.

The new empty-directory publish path is not a TOCTOU. os.ReadDir reporting empty is only a hint; the actual exclusion comes from rmdir, which cannot succeed on a non-empty directory, followed by a RENAME_NOREPLACE/RENAME_EXCL that still can't clobber a concurrently created target. Both steps fail closed, and the Lstat symlink check in front means a symlinked schema/ is refused rather than resolved through. init_publish_other.go picked up the matching removeEmptyInitDir stub, so the non-Linux/macOS build stays honest.

Postgres discovery uses pg_catalog.has_schema_privilege(oid, 'USAGE') with pg_catalog.-qualified names throughout, so it can't be steered by search_path, and it lists only what the user can actually read. MySQL staying inside the DSN's database via SELECT DATABASE() (with an up-front cfg.DBName == "" refusal) matches existing pull semantics rather than inventing a second rule.

One real finding, non-blocking:

initialize computes the reuse signal and throws it away. Line 296 calls initSchemaReuse(cmd.SchemaDir) purely as a validation gate — if _, err := ... — while the wizard path at the bottom of init_wizard_tui.go does cmd.ReuseSchema = reuse from the same function. So on the flag path, pointing --schema-dir at an existing valid schema directory without --reuse-schema runs the whole workflow — start the runtime, pull the live schema, build a baseline plan — and only fails at the very end with schema directory … differs from the verified import; existing files were preserved. Nothing is damaged, but the user pays for a full import to be told something knowable before any of it started.

Since the returned bool is right there, either adopting it (cmd.ReuseSchema = reuse when the flag wasn't set) or failing fast with "this directory already has a schema; pass --reuse-schema" would close the gap. Adopting it makes the two entry points agree, which is what the type comment promises: "Both routes use the same initialization workflow."

Two smaller notes, take or leave: the swallowed driver errors in CheckConnection/DiscoverNamespaces aren't logged even at debug, so a genuinely malformed DSN leaves nothing to diagnose from — a slog.Debug with the error would keep the UI clean and still be recoverable. And the misleading-error case I raised on #1329 (every renameInitSchema failure treated as "destination exists") is still reachable here through the same fallback, so fixing it there covers this too.

CI is clean at 04fe4964.

[aparajon]

🤖 Fixed the flags/wizard mismatch in 6d528597. Both entry points now automatically verify and reuse a valid existing schema directory. The engine integration scenario covers preserving comments without --reuse-schema, still accepting the explicit flag, and rejecting unapplied schema changes. The docs reflect this behavior.

The publication error fix from #1329 is also carried through, while preserving the empty-directory and symlink safeguards. The build and focused publication/reuse tests pass.

I kept raw driver errors out of debug logs too: they can contain connection credentials, so logging them would move the exposure from the screen to a log file. More detailed diagnostics should first classify or sanitize those errors.

Codex (GPT-6)

[morgo]

🤖 Review posted by Morgan's AI agent.

Re-approving at 199e8829. Pure restack — same 30 files and line counts, and I re-read the parts I cared about most: publishInitSchema still has the Lstat symlink refusal and the rmdir-then-rename path that fails closed on a concurrent write, and CheckConnection/DiscoverNamespaces still swallow driver errors so a DSN can't reach the UI.

The finding from my last review is unchanged and still non-blocking: initialize calls initSchemaReuse(cmd.SchemaDir) at line 94 purely as a validation gate and discards the bool, while the wizard sets cmd.ReuseSchema from the same call. So the flag path still runs a full import and baseline plan before failing with "schema directory … differs from the verified import" on something knowable up front.

[aparajon]

🤖 You were right: automatic reuse in the flag path was missing from the published branch. Restored it in 765ed470. initialize now retains the initSchemaReuse result, so both flags and the wizard verify existing desired files without importing over them. Integration coverage checks automatic reuse, rejection of real schema differences, preservation of edited files, and continued support for --reuse-schema.

This also carries the parent fixes into the wizard while retaining its symlink refusal and safe handling of empty directories. The invariant references now point to the separate registration, profile, and initialization guarantees.

Codex (GPT-6)

[morgo]

🤖 Review posted by Morgan's AI agent.

Re-approving at 765ed470. I compared this PR's own patch at my last-approved head against its patch now. Setting aside the docs/invariants.md reshuffle and the withDatabaseSnapshot / lock-tag / fs.ErrExist fixes inherited from the parents, the change here is the fix to my finding.

initialize no longer discards the reuse decision:

reuse, err := initSchemaReuse(cmd.SchemaDir)
if err != nil {
    return nil, err
}
cmd.ReuseSchema = cmd.ReuseSchema || reuse

That was the divergence I raised — init_wizard_tui.go assigned cmd.ReuseSchema from the same call while initialize threw the bool away and kept only the error, so the two entry points disagreed about whether an existing schema directory was being reused.

The || is the right combinator rather than a plain assignment: an explicit --reuse-schema survives a directory where detection returns false, and detection can still turn it on when the user didn't pass the flag. That matches the documentation change in the same commit — existing directories with a valid schemabot.yaml are verified and reused automatically, with the flag still accepted — so the docs and the code now describe the same behaviour.

The things I checked previously are unchanged: pkg/localsetup/discover.go still gates on pg_catalog.has_schema_privilege(oid, 'USAGE') with fully-qualified names, and initTerminalText still quotes control runes.

41/41 real checks green; the 4 CANCELLED entries are un-expanded matrix placeholders from a superseded run.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1334, 765ed47.

Verdict: 5 findings — 0 blocking, 4 non-blocking (JSON contract, placeholder dirs, wizard promises, detached goroutine), 1 suggestion.

Non-blocking

init --json emits two incompatible error shapes. init_wizard.go:46 prints a flat {"error":"missing_inputs","missing":[...]}, while every other failure goes through client.ExitWithJSON (init.go:53) and prints the repo-standard nested {"error":{"code","message"}}. A strict decoder breaks on one or the other, and docs/init.md:85 documents only the flat, non-standard form.

A placeholder file in the schema directory blocks init entirely. init_validation.go:43 treats any non-empty directory without a loadable schemabot.yaml as fatal, so a repo with schema/.gitkeep (git cannot commit an empty directory) aborts with a nested plan/apply usage message about two unrelated commands. The new empty-directory publish path (init.go:214) only rescues a truly empty directory, so the common placeholder case has no route through setup.

The wizard review screen promises something the confirm gate then refuses. init_wizard_tui.go:87 sets hasExistingSchema from a bare non-empty check, so a directory holding only README.md renders "We’ll verify them and keep your edits." One keypress later line 222 runs initSchemaReuse, fails, and bounces back to step 6 — which was unconditionally skipped on the way forward, so this is the first and only gate.

initializeWithUI returns without waiting for the in-flight init goroutine. On a p.Run() error init_progress_tui.go:86 returns immediately; bubbletea never joins per-Cmd goroutines, so the deferred cmd.progress = nil races the still-running reportProgress, and the deferred os.RemoveAll(stage) can be skipped, orphaning a .schemabot-init-* directory. The comment two lines above claims the opposite. Requires an abnormal TTY/renderer error mid-init, so severity is low but the comment is wrong either way.

General suggestions

Shift+Tab out of the namespace picker skips the state-DB step. init_wizard_tui.go:195 forces 4 → 3 whenever explicitNamespaces is false (the normal discovery case), so backing up from the review lands on the application connection and the SchemaBot state DSN is unreachable without walking forward again. That contradicts docs/init.md's "Use Shift+Tab from the review to edit any decision"; no test covers backward navigation.

The one thing that could have broken, verified

The new empty-directory publish path in publishInitSchemaWithRename could have clobbered a file outside the staging area. It fails closed: Lstat rejects a symlink, unix.Rmdir returns ENOTDIR if the target is swapped for a symlink after the check (TOCTOU closed), and the follow-up rename is still the RENAME_NOREPLACE variant, so no existing file can be overwritten.

Verified correct

• removeEmptyInitDir build tags are complete — init_empty_dir_unix.go (darwin||linux) plus the init_publish_other.go fallback; the new branch is unreachable on other platforms.
• doPostIntoWithClient reuses the same 30s httpClient as doPostInto, so routing pull/plan through the context-aware helper adds no timeout regression.
• CallPullSchemaAPIWithContext(..., PullSchemaOptions{Namespaces: ...}) is field-for-field equivalent to the variadic call it replaced (CatalogDetail/Lint were already zero).
• CallPullSchemaAPI, CallPullSchemaAPIWithOptions, CallPlanAPI and postPlanRequest all retain live callers — no dead wrappers left behind.
• WritePlanHeader's postgres branch runs after the IsMySQL assignment and both call sites pass a real engine, so Vitess/MySQL output is unchanged.
• DiscoverNamespaces' postgres query is pg_catalog-qualified and adds a has_schema_privilege(oid,'USAGE') filter on top of the server's own reserved-namespace filter — a strict subset.
• CheckConnection/DiscoverNamespaces discard driver errors and initConnectionSummary renders only quoted host/database, so no DSN, user, or password reaches the UI.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Addressed in 17f02188:

1. Missing-input JSON now uses the same nested error object with code and message as other errors, retaining the missing-fields list. The documented payload and decoding tests match it.
2. Non-empty folders without schemabot.yaml now get setup-specific guidance naming the user's folder, without unrelated plan/apply usage. I kept the refusal to remove or overwrite placeholders automatically; the message explains how to choose an empty folder or remove the placeholder explicitly.
3. The wizard validates the folder before showing its review screen. It only promises reuse when the shared reuse check accepts it.
4. The progress UI owns and joins initialization on every exit, including terminal failures before or during startup. A race-enabled regression test checks that cleanup finishes before return.
5. Shift+Tab from the namespace picker now reaches the state-database connection. The backward path has explicit coverage.

The guide also clarifies the explicit empty-namespace marker in 02bdbaa6. Thanks for the detailed pass.

Codex (GPT-6)

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1334, f4324dc.
Re-review of the delta only: 765ed470..f4324dc2 (the range since the previous review comment). Findings from that earlier comment are not repeated here.

Verdict: 2 findings — 1 non-blocking (doc over-claims lease coverage), 1 suggestion.

Non-blocking

The rewritten OW entry over-claims that retryable-apply expiry excludes a live driver. docs/invariants.md:777 says expiry "reads the row's operation lease ... so that it excludes a live driver by that same mechanism", but undrivenOperationGate filters on claimableApplyStates(), which omits failed_retryable — the state a redispatched operation keeps for its entire drive — unlike the reaper's state-filter-free unleasedOperationGate. The code says so itself ("this gate admits them, which is the same exposure the row had before any gate existed", apply_operations.go:1968), and TestApplyStore_ExpireRetryable_ExpiresTasksWhoseOperationSettledUnderAFreshLease pins the terminalizing outcome for exactly that row shape. Failure: deployment B is redispatched (fresh lease, state = failed_retryable) while A's redispatches push applies.attempt to maxRecoveryAttempts; ExpireRetryable fires on the budget arm, the gate does not match B, and B's running/pending tasks become failed/cancelled under a live driver — the deleted paragraph existed to prevent precisely this over-claim.

General suggestions

Init work now starts before the display does, and its error guidance is dropped. init_progress_tui.go:94 launches initialize in a goroutine before p.Run(), so a terminal that fails to start no longer prevents setup — it only cancels it afterwards, and none of LoadConfig, initSchemaReuse, os.MkdirAll or localsetup.Register checks runCtx, so runtime.yaml can already be rewritten. The returned fmt.Errorf("run setup display: %w", runErr) also discards outcome.err's "initialization incomplete; runtime registration is retained for retry", so the user gets no hint about the leftover registration. Previously the work ran as a Bubble Tea Init command and never started at all when Run failed.

The one thing that could have broken, verified

undrivenOperationGate's placeholder ordering in the two task UPDATEs: SET args, then terminal-state placeholders, then applyIDs, then stringArgs(drivingStates) — matching the rendered ? order exactly, with the lease cutoff a literal (LiteralIntervalAmount) contributing no arg. A single transposition here would terminalize the wrong rows silently; it is correct in both statements.

Verified correct

• A task with apply_operation_id IS NULL is admitted by the new gate (NOT EXISTS over an empty join), matching unleasedOperationGate.
• A leased pending operation cannot exist: the claim moves pending→running and stopped→resuming atomically with the lease rotation, so omitting them from drivingStates opens no window.
• Rows the gate skips stay recoverable — reapStrandedActive and reapStrandedRetryable need only a settled+quiet parent plus a stale operation lease, which expiry establishes in the same transaction.
• localsetup.Register's new cfg.Databases == nil guard is complete; the adjacent Environments[...] write can't hit a nil map because cfg.Validate() rejects zero-environment databases first.
• UpdateConfig's retry loop is safe: LoadConfig always returns a non-nil Profiles, edit re-runs on freshly loaded bytes, and both busy/changed errors mean no write landed.
• fmt.Errorf("%w: %w", ErrConfigBusy, err) in both lock files keeps errors.Is true through SaveConfig's bare return err, so lock contention is actually retried.
• Login's edit closure only copies token/refresh/expiry and refuses when endpoint, local runtime, or OIDC changed — a concurrent configure can no longer be clobbered.

This review was generated by Claude Code (claude-opus-5).

[Kiran01bm]

🤖 Approved on Kiran's (@kmuddukrishna) behalf by the scheduled review agent — no blocking findings at f4324dc. See the review comment above; non-blocking findings and suggestions, if any, are not merge gates.

[aparajon]

🤖 Thanks for the re-review. The expiry/OW-8 findings concern #1299, which landed on main in bb15169c and entered this revision range when the stack was synchronized. They are not introduced by the onboarding changes. I verified the concern, but this stack now leaves both expiry behavior and its invariant wording unchanged from main. A coordinated lifecycle fix belongs in separate work. My attempted isolated gate change was backed out after CI exposed a regression; the subsequent invariant edit was also removed to keep this stack scoped to onboarding.

The display issue is fixed in f5d01de3. Setup is still owned and joined outside Bubble Tea, but now waits for its Init command before doing any work. A startup failure cancels the waiting worker without registering a runtime. A failure after setup starts still joins cleanup and combines the display error with initialization's retained-registration guidance instead of discarding it. Separate startup and mid-run failure tests pass with the race detector.

The follow-up is already in flight: #1346 moves expiry to a reaper sweep, defers the whole apply while operation leases are fresh, and rechecks leases under operation locks before settling parent and children together. That PR is the place to address the expiry and invariant findings.

Codex (GPT-6)

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for schemabot/pull/1334, 5047a50.
Re-review of the delta only: 6846fe7b..5047a509. Note the range does not start at my last posted comment (f4324dc2) — an intermediate review at 6846fe7b was produced but never posted because the head moved first, so f4324dc2..6846fe7b has no comment on this PR.

Verdict: 2 findings — 1 blocking (expiry writes terminal task states over a live redispatch), 1 non-blocking (OW-8 documents intent, not shipped behaviour).

Blocking

undrivenOperationGate keys on claimableApplyStates(), which omits failed_retryable — the exact state a live redispatching operation holds for its whole drive. The gate renders AND lease_holder.state IN (...) from claimableApplyStates() (apply_operations.go:1968, :1975), while the repo's own driverOccupyingOperationStates comment says "A redispatched operation keeps that state for its whole drive — the claim rotates the lease and leaves the state alone" (:782-789). So the NOT EXISTS misses and the row is admitted where the removed unleasedOperationGate protected it.

Failure: fan-out apply with deployments A and B under on_failure=continue. A's redispatches exhaust the parent's retry budget while the parent sits failed_retryable (an operation-only drive never bumps it — Heartbeat refuses), so ExpireRetryable's budget arm selects it; B's operation is mid-drive at failed_retryable, the gate admits it, and B's running task is written Failed and its pending task Cancelled while B is still copying. Per ST-4 (docs/invariants.md:564) terminal stored tasks never move back, so B copies and cuts over successfully but comes to rest permanently on a failed verdict no driver wrote. The PR also deletes the test that pinned this (TestApplyStore_ExpireRetryable_DefersTasksUnderAFreshRetryableLease) and the fan-out test's UPDATE apply_operations SET state = failed_retryable ... WHERE id = held line.

Non-blocking

OW-8 now asserts expiry "excludes a live driver by that same mechanism", which is false for the redispatch case the gate deliberately admits. docs/invariants.md:777 contradicts the gate's own doc comment — "Nothing in the row distinguishes them ... So this gate admits them, which is the same exposure the row had before any gate existed" (apply_operations.go:1957-1963) — and it replaced the deleted, accurate line about live redispatches. AGENTS.md requires entries to describe shipped behaviour and a weakened invariant to be called out in the same PR; as written, an on-call reader concludes a terminal task proves no drive was live.

The one thing that could have broken, verified

Swapping unleasedOperationGate for undrivenOperationGate changes which task rows expiry may terminalize. Enumerating the state sets: waiting_for_cutover is inside claimableApplyStates(), so a healthy successor parked at the cutover barrier is still protected (the case the operations-UPDATE comment says must survive expiry), and tasks with no operation row still fall through exactly as before. The one state that fell out is failed_retryable, which driverOccupyingOperationStates() adds back "because a driver really is occupying it" — that is the blocking finding above, and it is the only regression the substitution introduces.

Verified correct

• Positional argument order matches the SQL text in both edited UPDATEs (cancel and non-terminal-fail).
• Placeholder count matches argument count: 19 ? from placeholders(len(claimableApplyStates())), 19 args from stringArgs(drivingStates).
• freshLeaseAfter is a dialect literal (LiteralIntervalAmount), not a bound parameter, so Postgres rebinding is undisturbed.
• No slice aliasing: applyIDs is allocated at exact capacity and gate args append into two independently allocated slices.
• Sprintf verb order in undrivenOperationGate is correct — state IN (%s) gets placeholders, updated_at >= %s gets the literal.
• Transaction ordering is sound: the pending-cancel UPDATE runs first, so its rows are terminal and excluded from the fail UPDATE; both evaluate the gate identically.
• Task writes are now consistent with the unleased UPDATE apply_operations ... WHERE state = failed_retryable later in the same transaction, removing a pre-existing internal inconsistency.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Thanks for the re-review.

The expiry gate, OW-8 wording, and related storage test comments are unchanged from main in the final PR diff. The incremental range starts from an intermediate experiment that was backed out, so it shows that rollback as a change even though these PRs do not introduce the underlying behavior. The concern is real, but it belongs to the already-merged #1299 and its in-flight follow-up #1346. That PR owns the coordinated expiry/lease fix and the matching invariant and test-comment changes.

Please evaluate these findings against the net PR diff to its base rather than treating the temporary experiment as the baseline. This stack stays scoped to CLI onboarding and the wizard.

Codex (GPT-6)