feat(t2-4): schema_version enforcement — warn on stale, tycoon init --upgrade to migrate

[JesuFemi-O]

Summary

• load_project now emits a UserWarning when tycoon.yml has no schema_version or one older than SCHEMA_VERSION, telling users to run tycoon init --upgrade
• tycoon init --upgrade calls migrate_project and prints whether the file was updated or was already current; exits non-zero if no tycoon.yml is found

Context

Part of M2 milestone (#83). Stacked on top of T2-3 (#172).

• T2-2 (feat(project): add migrate_project and SCHEMA_VERSION (M2 T2-2) #171) introduced migrate_project and the schema_version field
• T2-3 (feat(t2-3): replace global config singleton with local TycoonConfig construction #172) replaced the global config singleton with local TycoonConfig construction
• This PR wires up the enforcement layer so existing projects are nudged to migrate

Test plan

• TestSchemaVersionWarning in tests/test_project.py: warning fires when schema_version is absent or old, no warning when current
• TestUpgrade in tests/test_init.py: --upgrade exits non-zero with no file, migrates outdated YAML, reports "up to date" when already current
• --upgrade appears in tycoon init --help
• Full suite: 699 passed

Closes #96

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[db-tycoon-stephen]

Tested this end-to-end on feat/m2-t2-4 — full suite green (700 passed, 3 skipped), and the core migration is solid: ruamel round-trip preserves comments and inline comments, version: is untouched, re-runs are correctly idempotent.

Comments left inline. The two I'd want resolved before merge are the fresh-project false alarm and the future-schema gap — together they mean the warning is loud in the case that doesn't matter and silent in the case that does. The ValueError traceback and the presentation nit are quick; the bool guard is a nit.

Separately and not inline-able: this is stacked on t2-3, so it inherits that rebase. Merging #174 brings the ruff format pass, which conflicts with t2-3/t2-4 in five files — commands/explore.py, tests/test_explore.py, tests/test_sources.py, tests/test_sync.py, tests/test_templates_e2e.py.

[db-tycoon-stephen]

Small one, but it undercuts the feature: this command nags about the very thing it's about to fix.

$ tycoon init --upgrade
UserWarning: tycoon.yml is at schema version none, current is 2. Run 'tycoon init --upgrade' to migrate.
OK tycoon.yml migrated to the current schema version.

Cause is upstream of this block: config.py builds the TycoonConfig singleton at module import, which calls load_project() before any command body runs. So the warning fires on every command whether or not it touches the project. Suppressing it for the upgrade path, or moving the check out of load_project and into the commands that act on the project, both fix it.

[JesuFemi-O]

Addressed all six points in ef9591a.

Blockers:

• Fresh project false alarm + upgrade nags itself: Warning moved out of load_project() entirely and into load_config() using the Rich console helper. load_config() is the entry point for data commands (run_all, sync, sources, explore) but is never called by tycoon init --upgrade or by the module-level singleton, so both false-alarm cases are gone. New projects are also born stamped — scaffold_blank_project and scaffold_from_template write schema_version at creation time, and save_project always stamps it on write.
• Future schema version loads silently: load_project() now raises ValueError for schema_version > SCHEMA_VERSION, mirroring the same three-way split in migrate_project.

Quick fixes:

• ValueError traceback in --upgrade: wrapped in try/except ValueError, calls error(), exits 1 cleanly.
• Raw UserWarning presentation: switched to _warn_console (Rich warn() helper) in load_config().

Nit:

• bool guard: isinstance(existing, bool) or not isinstance(existing, int) in migrate_project.

Full suite: 703 passed, 0 warnings.

[db-tycoon-stephen]

The enforcement layer is the right idea and the test coverage is better than the description sells. But the forward-compatibility gate has a failure mode that makes it worse than not having it.

A future schema_version bricks the entire CLI. load_project raising propagates through the module-level config = TycoonConfig() at config.py:143, which runs at import time. Typer imports all command modules to build the CLI, so the raise lands before argument parsing. Against a tycoon.yml containing schema_version: 99:

tycoon --help               -> rc=1  TRACEBACK
tycoon --version            -> rc=1  TRACEBACK
tycoon init --upgrade       -> rc=1  TRACEBACK
tycoon doctor               -> rc=1  TRACEBACK
tycoon data sources list    -> rc=1  TRACEBACK

The same binary one directory over is fine. So the gate meant to protect users instead makes tycoon unusable in that project — including --help, and including init --upgrade, which is the remedy the error message names. The only way out is hand-editing the YAML.

test_upgrade_future_schema_version_exits_nonzero_cleanly passes vacuously. CliRunner runs in-process, so tycoon.config was already imported in pytest's cwd and the singleton was built before the tmp_path project existed. The test can't observe this class of failure — it needs to be a subprocess invocation to mean anything.

Suggestions inline for both this and the save_project stamping.

Worth noting this is the leftover singleton from #172 (tracked in #175) graduating from tidiness issue to real failure. Pulling #175 forward may be cleaner than layering the gate on top of it — your call on sequencing, but the gate shouldn't ship in its current form either way.

Minor: the description says TestSchemaVersionWarning in tests/test_project.py and a UserWarning from load_project; it's actually TestLoadConfigSchemaWarning in tests/test_config.py emitting a console warn() from load_config. Worth correcting since the distinction matters — a console warning can't be caught by pytest.warns or silenced via warnings.filterwarnings.

[JesuFemi-O]

Addressed the two blockers from the July 31 review:

1. Future-schema gate moved to load_config()
Removed the ValueError raise from load_project — it was firing through the module-level singleton at import time, taking down --help and init --upgrade alongside everything else. The gate now lives in load_config() alongside the existing stale-version warning: errors via _error_console and raises SystemExit(1). Import side-effects are gone. init --upgrade is unaffected — it goes through migrate_project which already has its own raise for future schemas.

2. save_project preserves schema_version rather than stamping
Changed from always writing SCHEMA_VERSION to if project.schema_version is not None: data["schema_version"] = project.schema_version. Silent advancement via sources add/sources remove is gone — only migrate_project (via tycoon init --upgrade) advances the stamp.

Tests updated:

• test_future_schema_version_loads_without_raise: flipped — asserts load_project returns the project without raising
• test_save_project_preserves_schema_version + test_save_project_does_not_stamp_when_absent: assert preservation semantics
• test_errors_and_exits_when_schema_version_future (new in test_config.py): patches _error_console, asserts SystemExit(1) with the right message

706 passed, 0 warnings.

[db-tycoon-stephen]

Approving. Both blockers from the July 31 review are fixed, and I verified them against the real CLI rather than the suite.

Verified

Built 53cebfc in a clean venv and ran the actual tycoon binary against a project containing schema_version: 99:

command	rc	traceback
--help	0	none
--version	0	none
doctor	0	none
init --upgrade	1	none, clean ERROR line

The import-time brick is gone. init --upgrade — the remedy the error message names — is reachable and exits cleanly.

Migration behaviour is correct: metadata: added, schema_version: 2 stamped, the user's own version: untouched, idempotent on re-run. save_project no longer advances the stamp, and test_future_schema_version_loads_without_raise is a genuine regression guard — it fails if the raise moves back into load_project.

Suite: 695 passed / 3 skipped locally (11 errors were syrupy missing from my minimal env, not a defect — 695 + 11 = your 706). All CI green.

You were right to push back on one of my suggestions

My July 31 inline note said init --upgrade needed the future-schema check wired in directly. It doesn't — migrate_project already raises, which you pointed out and I confirmed. Adding it would have been redundant.

And I contradicted myself across the two reviews

Worth recording so the July 29 thread doesn't read as unaddressed. On July 29 I flagged it as a gap that a load+save round-trip writes v2 content but no stamp. On July 31 I argued the opposite — that save_project must not stamp, and only migrate_project should advance it. You implemented the July 31 position and test_save_project_does_not_stamp_when_absent now encodes it.

July 31 is the right call: decoupling the version marker from the migration chain is the real hazard, and the persistent warning is the intended nudge. The earlier comment is superseded, not outstanding.

Two follow-ups filed, not blocking

Both are small and tracked separately so they don't hold up the stack:

1. scaffold_from_template flattens the template YAML. shutil.copy2 preserves the file, then the yaml.safe_load/yaml.dump round-trip immediately undoes it — nyc-transit goes 41 lines to 37, all five blank-line section separators stripped, and schema_version lands at the bottom instead of next to version:. Inconsistent with migrate_project, which uses ruamel precisely to avoid this. Calling migrate_project(dst_yml.parent) after the copy fixes it and reuses tested code.

2. The new save_project lines are a no-op. model_dump(by_alias=True, exclude_none=True) already emits schema_version — I verified it. This one is my fault: my suggestion block was written against the ef9591a state where the line it replaced was an unconditional stamp. As a replacement it was correct; applied to the base it does nothing. Safe to delete.

One thing worth being explicit about

load_config() is called by four commands — sources, sync, explore, run_all. Everything else still uses the import-time singleton, so on a schema_version: 99 project tycoon doctor runs to completion at rc=0 with no mention of the mismatch, and on a stale project doctor / db / transform / history never nudge.

That's the correct trade against bricking the CLI, and it resolves when the singleton goes. But it does mean the gate currently covers 4 of ~18 command modules, which is narrower than "the enforcement layer" suggests. Filed separately for scoping rather than treated as a defect here.

Not blocking, but flagging before you merge

• The base branch is a dead end. feat/m2-t2-3 was already merged into feat/m2-t2-1 (#172), so merging this into it leaves T2-4 on a branch with no onward PR. Retargeting to feat/m2-t2-1 is safe — t2-3 is already an ancestor.
• Nothing in M2 has reached the release branch. v0.1.11 and main contain zero references to SCHEMA_VERSION or load_config; all of T2-1/T2-2/T2-3 is parked in feat/m2-t2-1, and #170 (t2-1 → v0.1.11) was closed rather than merged. Your call on how to re-establish that path.

Leaving the merge and the retarget to you.