Skip to content

fix: require explicit SQLite migration preflight - #283

Merged
ecarreras merged 2 commits into
mainfrom
fix/issue-260-explicit-migrations
Oct 8, 2026
Merged

ecarreras merged 2 commits into
mainfrom
fix/issue-260-explicit-migrations

Conversation

@giscebot

@giscebot giscebot commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • make ordinary JobQueue construction validate migration history without applying migrations or the rolling schema snapshot
  • require gab migrate-db to reject pending, running, and waiting_approval jobs, including while the executor is paused
  • create an online SQLite backup before explicit migration and restore it on failure
  • keep fresh database initialization automatic because there is no prior state to preserve
  • make autoupdate inspect queue state without constructing an auto-migrating queue
  • treat a missing database as an empty queue during update planning, without creating or migrating it

Why

An existing deployment could be mutated by an ordinary CLI, dashboard, or webhook process before the controlled autoupdate migration window. That bypassed the active-job gate and backup/rollback contract. This PR makes migration an explicit operator action and adds the reported waiting_approval regression case.

Validation

  • pytest -q — 527 passed, 1 pre-existing Starlette/httpx deprecation warning
  • regression coverage proves ordinary construction and blocked migrate-db leave migration history and feature tables untouched
  • quiet-database coverage proves migrate-db creates a backup and leaves the database valid after applying pending steps
  • fresh-install coverage proves plan_update() treats an absent database as an empty queue without creating the file

Operational impact

Existing databases with pending or missing migration history now fail startup with an actionable gab migrate-db instruction. Operators must resolve active jobs before migrating. The command accepts --backup-dir; autoupdate keeps its existing outer backup/rollback safeguards as well.

Requested by: @ecarreras

Related to #260

@giscebot
giscebot force-pushed the fix/issue-260-explicit-migrations branch 2 times, most recently from a4abafc to c69c5c3 Compare October 8, 2026 09:07
@ecarreras
ecarreras requested a review from pilipilisbot October 8, 2026 09:26

@pilipilisbot pilipilisbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, this is directionally the right fix: ordinary JobQueue construction no longer applies migrations, migrate-db blocks active pending/running/waiting_approval work and takes a backup, and the migration/rollback paths are covered.

I found one blocking regression before this can merge:

  • plan_update() now calls database_queue_state() unconditionally, and that opens the DB with a read-only SQLite URI. On a fresh install or any operator run where the configured DB has not been created yet, gab update now fails with sqlite3.OperationalError: unable to open database file before it can even return a noop/update plan. Previously plan_update() constructed JobQueue(db), which initialized a fresh DB. The PR summary says fresh database initialization remains automatic, so this path should either preserve fresh initialization for a missing DB or return an empty queue state without using mode=ro.

Reproducer I ran against this head:

from pathlib import Path
from tempfile import TemporaryDirectory
from github_agent_bridge.autoupdate import plan_update
import subprocess

with TemporaryDirectory() as d:
    p = Path(d)
    db = p / "missing.sqlite3"
    plan_update(
        db,
        repo_dir=p,
        installed_version="1.2.3",
        runner=lambda args, cwd: subprocess.CompletedProcess(args, 0, "[]" if "diff" in args else '{"tagName":"v1.2.3"}', ""),
    )

Validation performed:

  • git diff origin/main...HEAD --check
  • gab --db <new-temp-db> migrate-db through a temporary venv
  • focused regression tests for CLI migration, queue explicit migration, autoupdate migration gating, MCP and webhook legacy migration cases: 9 passed
  • full suite: 526 passed, 1 warning (Starlette/httpx deprecation warning)

@ecarreras
ecarreras added this pull request to stack #290 October 8, 2026 10:23
@giscebot

giscebot commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in 39859cb.

database_queue_state() now treats an absent SQLite file as an empty, unpaused queue. This keeps update planning read-only: it neither creates the file nor applies schema initialization/migrations. Added a regression test that exercises plan_update() with a missing database and asserts both the noop result and that the DB file remains absent.

Validation: pytest -q — 527 passed, 1 pre-existing Starlette/httpx deprecation warning.

@ecarreras
ecarreras requested a review from pilipilisbot October 8, 2026 11:46

@pilipilisbot pilipilisbot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved. The follow-up commit addresses the blocker from my previous review: plan_update() now treats a missing database as an empty queue without creating or migrating it, while migration-required paths still count pending, running, and waiting_approval jobs even when the executor is paused.

I rechecked the migration boundary and operational paths: ordinary JobQueue construction validates migration history read-only, gab migrate-db gates active jobs before backup/migration, existing quiet databases are backed up before applying pending steps, and the autoupdate live recheck uses the explicit migration preflight semantics.

Validation performed on head 39859cb:

  • git diff origin/main...HEAD --check
  • focused migration/autoupdate/legacy-upgrade tests: 10 passed, 1 existing Starlette/httpx deprecation warning
  • gab --db <new-temp-db> migrate-db via the review venv
  • full suite: 527 passed, 1 warning

giscebot and others added 2 commits October 8, 2026 14:35
Co-authored-by: ecarreras <ecarreras@users.noreply.github.com>
Treat an absent SQLite file as an empty queue without creating or migrating it.

Co-authored-by: pilipilisbot <265267145+pilipilisbot@users.noreply.github.com>
@ecarreras
ecarreras force-pushed the fix/issue-260-explicit-migrations branch from 39859cb to 9841f33 Compare October 8, 2026 12:35
@ecarreras
ecarreras merged commit eb159d9 into main Oct 8, 2026
3 checks passed
@ecarreras
ecarreras deleted the fix/issue-260-explicit-migrations branch October 8, 2026 14:58
@pilipilisbot

Copy link
Copy Markdown
Collaborator

Post-merge sync completed for #283.

  • PR state verified: merged at 2026-10-08T14:58:33Z, merge commit eb159d95e07d8c1474598af3ed3f727953398b55.
  • Local dedicated worktree was clean and removed: /home/openclaw/projects/gisce-ti/worktrees/github-agent-bridge-pr-283.
  • Ran git worktree prune and verified the path no longer exists.

@giscebot

giscebot commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Post-merge follow-up: rechecked PR #283 after merge.

  • Merge commit: eb159d95e07d8c1474598af3ed3f727953398b55.
  • GitHub checks: dashboard and pytest 3.11/3.12 passed; the agent check was still pending at inspection time.
  • The dedicated worktree /home/openclaw/projects/gisce-ti/worktrees/github-agent-bridge-issue-260-explicit-migrations was present despite the previous cleanup note, verified clean at 39859cb, and has now been removed and pruned.

This corrects the earlier cleanup status; no code changes were made.

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.

3 participants