Skip to content

fix(setup): run migrations during onboard when DATABASE_URL preset (#846) - #2309

Merged
ilblackdragon merged 6 commits into
stagingfrom
fix/846-onboard-migration-order
Apr 20, 2026
Merged

fix(setup): run migrations during onboard when DATABASE_URL preset (#846)#2309
ilblackdragon merged 6 commits into
stagingfrom
fix/846-onboard-migration-order

Conversation

@ilblackdragon

@ilblackdragon ilblackdragon commented Apr 11, 2026

Copy link
Copy Markdown
Member

Summary

Fixes #846. The auto_setup_database() PostgreSQL early-return paths skipped test_database_connection_postgres() and run_migrations_postgres() when DATABASE_URL was already set, leaving self.db_pool = None and causing "Failed to save settings to database" at the final onboard step.

  • Both early-return PostgreSQL paths now establish the connection and run migrations
  • Added test_auto_setup_database_runs_migrations_with_existing_env regression test (libsql variant)

Test plan

  • cargo fmt, cargo clippy --all-features zero warnings
  • cargo test --lib -- setup bootstrap (125 tests pass)
  • Manual: set DATABASE_URL, run ironclaw onboard, verify it completes

🤖 Generated with Claude Code

@gemini-code-assist

gemini-code-assist Bot commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions github-actions Bot added scope: agent Agent core (agent loop, router, scheduler) scope: channel Channel infrastructure scope: channel/cli TUI / CLI channel scope: channel/web Web gateway channel scope: channel/wasm WASM channel runtime scope: tool Tool infrastructure scope: tool/builtin Built-in tools scope: tool/wasm WASM tool sandbox scope: tool/builder Dynamic tool builder scope: db Database trait / abstraction scope: db/postgres PostgreSQL backend scope: db/libsql libSQL / Turso backend scope: llm LLM integration scope: orchestrator Container orchestrator scope: worker Container worker scope: config Configuration scope: setup Onboarding / setup scope: sandbox Docker sandbox scope: ci CI/CD workflows scope: docs Documentation size: XL 500+ changed lines risk: high Safety, secrets, auth, or critical infrastructure contributor: core 20+ merged PRs labels Apr 11, 2026
ilblackdragon
ilblackdragon commented Apr 11, 2026
View reviewed changes

@ilblackdragon ilblackdragon left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Blocking feedback, posted as a comment because GitHub does not allow requesting changes on your own PR.

The fix itself is directionally right, but the regression coverage does not test the branch that changed.

The bug was in the PostgreSQL early-return path in auto_setup_database() when DATABASE_URL was already set. The added test only exercises the libSQL branch, so it would not catch a future regression in the Postgres wiring. That violates the repo rule to test through the real caller rather than only proving a similar helper/path.

I want a caller-level regression test that drives auto_setup_database() with DATABASE_URL preset and proves the wizard ends up with a live DB handle and successful persistence on the PostgreSQL path.

ilblackdragon added a commit that referenced this pull request Apr 11, 2026
@ilblackdragon @claude
test(setup): caller-level regression test for Postgres auto_setup_dat…
ebe8a80 
…abase path

Addresses PR #2309 review feedback: prior test only covered the libSQL
branch while the original bug was in the PostgreSQL early-return path.

Drives auto_setup_database() with a preset DATABASE_URL against a real
pgvector-enabled Postgres container (testcontainers), asserts the
wizard ends up with a live db_pool, proves migrations ran by invoking
persist_settings(), and does a round-trip read-back via Store::get_setting
to confirm actual persistence — not just an in-memory Ok. Skips
gracefully when Docker is unavailable, matching the workspace_integration
pattern. Gated behind the integration feature.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ilblackdragon

ilblackdragon commented Apr 11, 2026

Copy link
Copy Markdown
Member Author

Addressed in ebe8a80: added caller-level integration test driving auto_setup_database() with a preset Postgres DATABASE_URL, exercising the early-return branch and asserting the wizard ends with a live DB handle + successful persistence (including a round-trip read-back via Store::get_setting). Uses testcontainers-modules with the pgvector/pgvector:pg16 image so the migrated schema (which requires pgvector) can actually be applied. Gated behind the integration feature; skips gracefully when Docker is unavailable.

ilblackdragon and others added 2 commits April 13, 2026 03:50
@ilblackdragon @claude
fix(setup): run migrations in auto_setup_database for existing Postgr…
51b644f 
…eSQL config (#846)

When DATABASE_URL was already set before running `ironclaw onboard`,
auto_setup_database() skipped both connection testing and migrations,
leaving self.db_pool as None. This caused save_and_summarize() to fail
with "Failed to save settings to database" because persist_settings()
had no DB handle to write to. Now both postgres early-return paths call
test_database_connection_postgres() and run_migrations_postgres() before
returning, matching the existing libsql behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ilblackdragon @claude
test(setup): caller-level regression test for Postgres auto_setup_dat…
e70f5de 
…abase path

Addresses PR #2309 review feedback: prior test only covered the libSQL
branch while the original bug was in the PostgreSQL early-return path.

Drives auto_setup_database() with a preset DATABASE_URL against a real
pgvector-enabled Postgres container (testcontainers), asserts the
wizard ends up with a live db_pool, proves migrations ran by invoking
persist_settings(), and does a round-trip read-back via Store::get_setting
to confirm actual persistence — not just an in-memory Ok. Skips
gracefully when Docker is unavailable, matching the workspace_integration
pattern. Gated behind the integration feature.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ilblackdragon
ilblackdragon force-pushed the fix/846-onboard-migration-order branch from ebe8a80 to e70f5de Compare April 13, 2026 03:50
This was referenced Apr 22, 2026
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
ljanke-agents added a commit to ljanke/ironclaw that referenced this pull request Apr 27, 2026
@ljanke-agents @claude
fix(setup): run migrations on existing-URL onboarding path
e239fac 
When DATABASE_URL is preset and the user accepts "Use this database?"
in the interactive wizard, route through finish_postgres_auto_setup so
connection-test + migrations both run instead of returning Ok(())
without running migrations.

Same bug shape as PR nearai#2309 / issue nearai#846 fixed in auto_setup_database;
the interactive existing-URL branch was the remaining sibling site.

Host: ironclaw-dev Model: claude-opus-4-7

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 27, 2026
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
@ilblackdragon @claude
fix(setup): run migrations during onboard when DATABASE_URL preset (n…
1a554eb 
…earai#846) (nearai#2309)

* fix(setup): run migrations in auto_setup_database for existing PostgreSQL config (nearai#846)

When DATABASE_URL was already set before running `ironclaw onboard`,
auto_setup_database() skipped both connection testing and migrations,
leaving self.db_pool as None. This caused save_and_summarize() to fail
with "Failed to save settings to database" because persist_settings()
had no DB handle to write to. Now both postgres early-return paths call
test_database_connection_postgres() and run_migrations_postgres() before
returning, matching the existing libsql behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(setup): caller-level regression test for Postgres auto_setup_database path

Addresses PR nearai#2309 review feedback: prior test only covered the libSQL
branch while the original bug was in the PostgreSQL early-return path.

Drives auto_setup_database() with a preset DATABASE_URL against a real
pgvector-enabled Postgres container (testcontainers), asserts the
wizard ends up with a live db_pool, proves migrations ran by invoking
persist_settings(), and does a round-trip read-back via Store::get_setting
to confirm actual persistence — not just an in-memory Ok. Skips
gracefully when Docker is unavailable, matching the workspace_integration
pattern. Gated behind the integration feature.

[skip-regression-check]

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(setup): case-insensitive backend comparison + migration coupling assert

Addresses PR nearai#2309 review: DATABASE_BACKEND comparison is now
case-insensitive, debug_assert guards the db_pool coupling between
connection test and migration, and a new test covers the
DATABASE_BACKEND=postgres early-return path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor(setup): typed DatabaseBackend parsing, extract finish_postgres_auto_setup

Address review feedback on nearai#846:

- Parse DATABASE_BACKEND via DatabaseBackend::FromStr instead of stringly
  comparing "postgres"/"postgresql". Aligns with .claude/rules/types.md
  and gains the "pg" alias for free.
- Extract the duplicated body of both postgres early-return branches
  (connection test + debug_assert + migration + settings record) into a
  shared finish_postgres_auto_setup helper so the two call sites cannot
  drift.
- Move the "Using existing PostgreSQL configuration" banner to after the
  connection test succeeds, so a failing connection no longer prints a
  misleading success-toned message first.
- Factor postgres-container setup and round-trip assertions out of the
  two integration tests into start_pg_container and
  assert_auto_setup_postgres_persisted helpers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(setup): make run_migrations_postgres error on missing pool

Address PR nearai#2309 review comments from serrrfirat and Copilot:

- run_migrations_postgres() previously returned Ok(()) silently when
  db_pool was None. That made the correctness of the onboarding path
  depend on a side-effect-only ordering contract with
  test_database_connection_postgres(), flagged as fragile in the review.
  Convert the silent no-op into an explicit SetupError so a future
  regression cannot re-introduce the original nearai#846 failure mode in
  release builds (debug_assert only fires in debug).
- Drop the now-redundant debug_assert in finish_postgres_auto_setup —
  the runtime check in the callee supersedes it.
- Replace the misleading tests/workspace_integration.rs reference in
  start_pg_container's docstring (that file uses env-PG skip, not
  testcontainers) with a self-contained description.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serrrfirat serrrfirat serrrfirat left review comments
Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

contributor: core 20+ merged PRs risk: high Safety, secrets, auth, or critical infrastructure scope: agent Agent core (agent loop, router, scheduler) scope: channel/cli TUI / CLI channel scope: channel/wasm WASM channel runtime scope: channel/web Web gateway channel scope: channel Channel infrastructure scope: ci CI/CD workflows scope: config Configuration scope: db/libsql libSQL / Turso backend scope: db/postgres PostgreSQL backend scope: db Database trait / abstraction scope: docs Documentation scope: llm LLM integration scope: orchestrator Container orchestrator scope: sandbox Docker sandbox scope: setup Onboarding / setup scope: tool/builder Dynamic tool builder scope: tool/builtin Built-in tools scope: tool/wasm WASM tool sandbox scope: tool Tool infrastructure scope: worker Container worker size: L 200-499 changed lines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

onboard fails with "Failed to save settings to database", but ironclaw starts successfully and applies migrations

3 participants

@ilblackdragon @serrrfirat