[bassilkhilo-ag2]

Closes #955.

What

Phase 1 of the migration framework from RFC #906 — infra only, no first real migration yet.

Changes

File	What
migrations/README.md	Per-script contract: idempotent, NNNN-* naming, exit-0 = applied, exit-non-zero = abort
src/run_migrations.py	Runner: reads state/schema-version.json, walks migrations/ numerically, runs pending scripts, atomic write on success, stops loudly on failure. CLI: python3 src/run_migrations.py [--dry-run]
src/startup.sh	Invokes runner before services launch; aborts startup if runner exits non-zero
tests/migrations/conftest.py	Tempdir fixtures + make_migration() helper
tests/migrations/test_runner_no_migrations_yet.py	9 tests: fresh state, apply sh/py scripts, stop-on-failure, dry-run no-op, idempotent rerun
tests/migrations/test_no_collision.py	CI guard: duplicate numbers → fail, gaps in sequence → fail
.gitignore	Unblock tests/migrations/ subdirectory

Acceptance

• python3 src/run_migrations.py exits 0 on fresh checkout (no migrations, no state file)
• All 11 tests in tests/migrations/ pass
• bash src/startup.sh integration: runs runner before service launch, aborts on non-zero

Out of scope (Phase 2)

• Migration 0001 (PRIVATE_DIR → MEMORY_DIR backfill) — separate PR
• Migration 0002 (workspace contract A/B) — separate PR, owner-direction needed first

🤖 Generated with Claude Code

[sonichi]

LGTM — solid Phase 1 migration infra. Atomic state writes, clean per-script contract, 11 tests covering the right axes (fresh state / apply / failure / dry-run / idempotency / collision-detection).

Strengths:

• Atomic state writes: tmp + os.replace on state/schema-version.json — crash-safe, no partial-write torn state.
• Stop-loudly-on-failure: runner returns 1 on first failed migration with last 10 stderr lines printed; startup.sh aborts. Clean fail-fast contract that matches the "don't boot a half-migrated system" invariant.
• CI guard tests (test_no_collision.py): catches duplicate numbers + sequence gaps. Authoring mistakes that would otherwise silently land are caught at PR time.
• Per-script contract spelled out in migrations/README.md: idempotent, NNNN-* naming, exit-0 = applied. Future migration authors have a clear rubric.
• bootstrap-on-import + CLI symmetry: importable for testing, runnable from startup.sh, fallback when workspace_default is unavailable (test envs).

Five observations (not blockers, queueing as follow-up candidates):

1. _bootstrap_on_import swallows ALL exceptions silently (except Exception: pass). A migration failure during a manual from src.run_migrations import ... would leave the user with no signal — silent failure mode in the one path where the user is most likely to be debugging. Suggest at-minimum a print(..., file=sys.stderr) for the failure case.

2. stderr[-10:] truncation: only the last 10 stderr lines surface in the runner's failure output. A migration that prints diagnostic context BEFORE the actual error gets cut off. Either bump the slice or print all-lines-if-fewer-than-50 (cheap, more debuggable).

3. len(parts[0]) != 4 filter is brittle: a 5-digit migration (00100-foo.sh) gets silently skipped from discovery. If someone hits 10000 migrations decades from now this matters; more immediately, a typo (001-foo.sh) would be invisible. Worth a stderr warning on filtered files OR widening the constraint to >=4 and isdigit.

4. No --force for re-applying a broken-applied migration: if migration 0003 has a latent bug discovered post-apply, there's no clean way to re-run without manually editing state/schema-version.json. Phase 2 candidate.

5. No test for _bootstrap_on_import: the import-time path is the most fragile (silent fallback, sentinel-gating). Adding a single test that imports the module twice + asserts sentinel-gate works would cheaply close that gap.

6. subprocess env={**os.environ, "SUTANDO_WORKSPACE": ..., "DRY_RUN": "0"}: clean enough; no env-stripping or PATH issues observed. Note for future migration authors: anything that needs additional env vars (e.g. SUTANDO_MEMORY_DIR) has to be inherited from the parent process. Worth documenting in migrations/README.md.

Phase 1 = pure infra, no actual migration script yet — so true behavioral confidence depends on the eventual Phase 2 migration (PRIVATE_DIR → MEMORY_DIR per PR body). Worth being conservative on the merge: maybe land Phase 1 + the first real migration (Phase 2 #1) together, so the runner exercises real state on day one.

Owner merge call.

[liususan091219]

NON-BLOCKING

Scope-clean Phase 1 framework — 7 files all in migrations/ + runner + startup + tests. No build_log.md leak (unlike the sibling Bassil PRs). Migration contract (NNNN-* naming, idempotent, exit-0 = applied) reads well in the README.

A few small things, none blocking:

1. Schema-version write atomicity — src/run_migrations.py writes state/schema-version.json after each migration. If the runner is interrupted between applying migration N and writing N to the version file, the next run will re-apply migration N. The migration contract says "idempotent" so that's safe by construction, but worth a comment in the README so future authors don't accidentally write non-idempotent migrations relying on the runner to skip them.

2. startup.sh abort behavior — confirmed: "aborts startup if runner exits non-zero". Good — a half-applied migration shouldn't be papered over by booting services anyway.

3. No-collision test (test_no_collision.py) — gap-detection is a nice future-proofing. Question: do you want the CI guard to also reject mixed-extension collisions (e.g. 0003-foo.sh + 0003-bar.py)? The numbered-name contract suggests yes, but the README doesn't say.

LGTM aside from those; deferring to Susan for the merge call.

[VasiliyRad]

LGTM. Clean Phase 1 migration infra — well-scoped to just the framework with no actual migrations yet.

What works well: Atomic state writes via tmp+replace, fail-fast on first broken migration (startup.sh aborts), per-migration state persistence so crash-recovery re-applies at most one script (idempotent by contract), CI collision guard catching duplicate/gap numbering errors at PR time. 11 tests cover the right axes.

One note (non-blocking): _bootstrap_on_import() is defined but never called at module level — it's effectively dead code until a future change wires it. Fine for Phase 1, just flagging so it doesn't get forgotten.

Agree with sonichi's and liususan's non-blocking observations (silent exception swallowing in bootstrap, mixed-extension collision, stderr truncation) as Phase 2 candidates.

[sonichi]

@bassilkhilo Pause recommendation: #1169 ([@sonichi] greenlit option B — no auto-migrate on resolve_workspace(), explicit sutando-migrate CLI instead) supersedes the Phase 1 framework's destructive-move semantic. Recommend marking draft until that direction is consolidated. Closing isn't suggested — your framework work may shape the CLI's per-step structure once the design settles.