Repository navigation
DO NOT MERGE — merge train: 3973,3972,3974,3283 - #4005
Closed
trinity-ability wants to merge 24 commits into
Closed
trinity-ability wants to merge 24 commits into
trinity-ability wants to merge 24 commits into
Conversation
…egistered function is one (#3395) run_all_migrations runs only what MIGRATIONS lists, so a migration function that is written but never registered ships green today and never runs on an existing install. A data-only migration (an UPDATE backfill) is invisible to any schema diff. test_fan_out_id_migration_present pinned this class for one name; this test pins it for every name, in both directions. The functions are discovered from the loaded module (inspect, filtered to _migrate* defined in db/migrations.py) and compared by identity with the MIGRATIONS captured at import. Non-vacuity: the discovered set must be non-empty and equal the top-level `def _migrate*` names parsed from the file's source, so an empty or partial discovery cannot pass. Mutations, each applied to db/migrations.py from a cp snapshot and restored: - M1, delete ("a2a_caller_identity", _migrate_a2a_caller_identity) from MIGRATIONS: AssertionError: _migrate* functions defined in db/migrations.py but not registered in MIGRATIONS (existing installs never run them): ['_migrate_a2a_caller_identity'] - D1, add an unregistered data-only _migrate_probe_backfill (UPDATE, no DDL): AssertionError: _migrate* functions defined in db/migrations.py but not registered in MIGRATIONS (existing installs never run them): ['_migrate_probe_backfill'] - V1, register probe_backfill, which is not a _migrate* function: AssertionError: MIGRATIONS entries whose function is not a _migrate* function defined in db/migrations.py: ['probe_backfill -> migrations.probe_backfill'] - Vacuity, point the test's file filter at a non-existent split.py: AssertionError: no _migrate* functions discovered in .../src/backend/db/migrations.py M0 (no mutation): green, 170 defined = 170 registered. The plan's size floor (discovered >= len(MIGRATIONS)) was replaced: under V1 it fired first and reported "discovery is broken" instead of naming the foreign entry. The source-parse check is the floor instead. Part of #3395.
… directions (#3395) test_schema_parity.py asked only `init_database() - init_schema()`: an object a migration creates that schema.py lacks. The reverse went unchecked, so an object schema.py declares that a fresh install never ends up with shipped green. Two fresh-install shapes produce that: an old creating migration's CREATE TABLE losing a column (init_schema's IF NOT EXISTS never adds it back), and a rebuild migration on the second pass dropping indexes or triggers (_atomic_rebuild defaults to indexes=()). TestReverseSchemaParity mirrors the four forward assertions over the same two fixtures (schema - production: tables, columns, indexes, triggers), each naming the object and the fix. The module docstring now describes both directions, cites the lifecycle as database.py::init_database instead of stale line numbers, and states the limit: both fixtures are the fresh-install shape, so a column whose migration was never registered on a table only init_schema creates stays invisible here (the upgrade guard covers it). Mutation (forward-only = the four pre-existing assertions): - M3, `signal_query TEXT,` removed from _migrate_canary_violations_table's CREATE TABLE: forward-only 4 passed; full file 1 failed, test_no_schema_only_columns names canary_violations.signal_query (TEXT). - M7, a registered chat_messages rebuild that skips on pass 1 and runs on pass 2 with indexes=(): forward-only 4 passed; full file 1 failed, test_no_schema_only_indexes names the 5 idx_chat_messages_* indexes. - M8, a pass-2 DROP TRIGGER audit_log_no_update: test_no_schema_only_triggers red. M9, a pass-2 DROP TABLE agent_tags: test_no_schema_only_tables (and _indexes) red. Forward-only green on both.
…der two entries (#3395) /review finding: the set comparison is a bijection on functions only, so one _migrate* function registered under two MIGRATIONS names passed while running twice on every fresh database. The test now names every function with more than one entry. Mutation, register _migrate_a2a_caller_identity a second time as "a2a_caller_identity_again": AssertionError: functions registered under more than one MIGRATIONS entry: ["_migrate_a2a_caller_identity <- ['a2a_caller_identity', 'a2a_caller_identity_again']"] M0, M1, D1 and V1 re-run after this change: green, then red with the same messages as in the first commit. Part of #3395.
…hout citing an unmerged guard (#3395) Review finding: the limit paragraph said the existing-install shape "is covered by the merge-base upgrade guard", which does not exist on dev until its own PR lands. It now states only what this file cannot see and why.
…e PR, must end at schema.py (#3395) test_schema_parity.py models a fresh install only. On that path a migration whose target table does not exist yet raises `no such table`, which the runner skips as a fresh install, and init_schema then creates the table with every column schema.py lists. So a schema.py column whose migration was never registered (or never written) is present on every fresh install and missing on every existing one, and no fresh-install comparison can see it. 213 of the 306 ADD COLUMN sites in db/migrations.py target a table only init_schema creates (schedule_executions, users, chat_messages, ...), so a reverse diff on the fresh path, as the issue proposes, is blind on 70% of them. scripts/ci/check_sqlite_upgrade_parity.py models the existing install: it resolves the merge-base of the PR's base and head, git-archives the merge-base's src/backend, runs that tree's lifecycle (migrations, init_schema, migrations) on a temp file DB in its own interpreter, then runs the checkout's lifecycle on the same DB in a second interpreter, and requires every table, column, index and trigger the checkout's init_schema declares to exist, with migration_health at (n, n, None). Pure stdlib. Exit 0 pass, 1 gap (each object named; a head lifecycle that raises on the upgraded install counts), 2 could not run (no base/head, no merge-base, the base tree failing to build its install) - never a silent pass. tests/unit/test_sqlite_upgrade_parity_guard.py drives the comparison core over synthetic two-file backends (no git, no backend deps), so it runs in the unit tier: registered migration passes; written-but-unregistered and no-migration-at-all are gaps naming t.b; a new init_schema-only table is no false red; an unbuildable base exits 2. Plus head-lifecycle-raises, unrecorded-migration, the git fail-closed paths, and main() over a throwaway git repo. Registered in tests/registry.json. Mutation (the issue's exact class): replay PR #3913 (17adf5a against 17adf5a^) with `("a2a_caller_identity", _migrate_a2a_caller_identity),` deleted from MIGRATIONS while the function and the schema.py column stay: exit 1, "column schedule_executions.a2a_caller_identity: ... never gets it on upgrade". With the function deleted too (R1'): exit 1, same column. As shipped (R0): exit 0, 1 migration newly recorded, (170, 170, None). dev against its own merge-base: exit 0. A PR adding an init_schema-only table plus index on dev (N1): exit 0, 93 tables / 199 indexes.
…a-relevant PR (#3395) One step in the existing schema-parity job, after the dependency install (the merge-base's lifecycle runs under them) and behind the job's self-skip, on pull_request only (the merge-base comes from the PR's base/head SHAs; the checkout already uses fetch-depth: 0). Same dummy env as the parity pytest. The script and its self-test join the change-detection regex and push.paths, the way check_alembic_parity.py is listed. The job id, and so the required check name `schema-parity`, is unchanged. Header comment gains one paragraph. Mutation: see the previous commit - the PR #3913 replay with its MIGRATIONS line deleted exits 1 naming schedule_executions.a2a_caller_identity.
…does and states its limits (#3395) Review fixes on the guard and its self-test. Guard: - The lifecycle connection uses row_factory = sqlite3.Row, as db/connection.py::get_db_connection does, so a migration that reads a row by column name is not reported as a boot failure. - A checkout whose db/schema.py or db/migrations.py does not import is "could not run" (exit 2), not a schema gap. - Paths are resolved, tar extraction errors exit 2, and a single positional argument is a usage error rather than being silently ignored. - The base-failure message says what to do (land a dependency bump on its own; update an old branch). - The docstring states the limits it had left implicit: the install it builds has no rows (init_database's seeding steps are not run, so backfills, rebuild copies and constraints on existing rows are not exercised); only missing objects are reported; the ledger check is stricter than /health on purpose (a renamed migration re-runs, a removed one keeps being counted). The 213-of-306 count names its method. Self-test (11 -> 22 tests): an index, a trigger and a table dropped by a migration after init_schema; a renamed migration; a duplicate registration; the restated order (migrations before init_schema, the second pass); rows by name; a head that does not import; relative paths; a one-argument call. Twelve guard mutations (each finding loop, both ledger checks, the first_pending check, the second pass, the order, base failure -> pass, merge-base -> head, head import -> gap, no row_factory) each turn at least one test red; before this commit seven of the first ten survived.
…iggered by (#3395) The self-test was in push.paths and the self-skip regex but not in the parity pytest command, so a PR touching only it ran the job without running it. It now sits next to test_alembic_parity_guard.py on that line. The test also runs in backend-unit-test, as before.
…it (#3395) When the merge-base's own lifecycle raised, the upgrade-parity guard exited 2, so the PR that repairs a broken dev (whose merge-base is that broken commit) was blocked by a required check, with branch protection the only way out. The guard now walks back the merge-base's first-parent ancestors, at most MAX_BASE_WALKBACK (20), to the newest one that builds, upgrades from it, and warns naming every commit skipped. Not fail-open: the head still has to upgrade over the longer range. Exit 2 stays the answer when the bound is exhausted. Also from review: the stated limits and workflow header say the guard runs the open-source runner only, never the enterprise one; database.md and Invariant #3 name the guard; the script is executable (ruff EXE001); the exhausted-walk message no longer suggests the circular "land the bump first" order.
merge-train: the maintainer asked for the setting in the user docs (discussion #3282); agent-chat.md still listed PDF as unsupported. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Integration surface for #3973, #3972, #3974, #3283. Never merged; members merge individually once green.
🤖 Generated with Claude Code