test(session): pin migration catalogue content (append-only enforcement)

[dgageot]

On PR #2646 reviewers flagged the dangerous pattern of editing an
already-shipped migration:

"while it looks ok in this context, it's always dangerous to update
an existing migration. A new migration would be cheap."

Once a migration has shipped, its (ID, Name, Description, UpSQL, DownSQL) tuple is part of the on-disk schema contract. Editing any
of those bytes after the fact means an already-deployed database may
run the new SQL even though the row in schema_migrations still
records the old name — leaving the schema in a state no version of
the code has ever produced.

This PR adds two tests in pkg/session/migrations_pinned_test.go,
neither of which depends on git history:

• TestMigrationCatalogIsContentPinned SHA-256s every migration's
  textual fields and compares against a pinned constant. Editing an
  existing entry — or inserting one ahead of an existing entry —
  flips the digest, and the test fails with a "did you mean to add
  migration N+1?" message that quotes the new digest the contributor
  must paste in. Bumping the catalogue is a two-line diff (append the
  migration + update the constant), and the audit trail lives in the
  commit history.

• TestMigrationIDsAreSequential is the construction-side
  companion: IDs must be 1..N with no gaps, and each Name must
  start with the zero-padded ID prefix the catalogue already uses.

UpFunc is intentionally excluded from the digest — function-pointer
identity is not stable across builds. Data migrations defined in Go
fall back to change-review as the safety net.

Mutation-tested

Appending " (edited)" to migration 001's Description fails the
test with the expected message and prints the new digest.

No behavioural change

Pure test addition; the runtime migration runner is untouched.

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

Two findings in the new test code — one confirmed logic gap and one documented design limitation.

[docker-agent]

[MEDIUM] wantPrefix is derived from m.ID (actual) instead of wantID (expected), creating a false-negative

The prefix check is meant to verify that each migration's Name matches its expected sequential position — but it derives the prefix from m.ID, not from wantID:

wantID := i + 1
if m.ID != wantID {
    t.Errorf("migration[%d]: ID = %d, want %d …", i, m.ID, wantID)
}
wantPrefix := fmt.Sprintf("%03d_", m.ID)   // ← should be wantID
if !strings.HasPrefix(m.Name, wantPrefix) {
    t.Errorf(…)
}

Scenario that slips through: A developer deletes migration 006_... from the middle of the slice. Now migration[5] has m.ID = 7 and wantID = 6. The ID check fires (good), but execution continues because t.Errorf is non-fatal. The prefix check then forms "007_" from m.ID = 7 and happily matches a Name of "007_…" — silently passing even though the slot expected ID 6 and an "006_" prefix.

The fix is one character: fmt.Sprintf("%03d_", wantID). This makes the two checks independent guards rather than correlated ones.

[dgageot]

Good catch — fixed. Changed wantPrefix := fmt.Sprintf("%03d_", m.ID) to use wantID instead, and added a comment explaining why the two checks need to stay independent so a deleted-middle scenario can't slip through.

[docker-agent]

Perfect — the fix is spot-on and the inline comment makes the reasoning crystal clear for future readers. Thanks for adding that explanation about the independence of the two checks.

[docker-agent]

[LOW] UpFunc exclusion means Go data-migration changes bypass the append-only enforcement

The digest intentionally omits UpFunc — and the comment explains why (function-pointer identity is not stable across builds). However, migration 015_migrate_messages_to_session_items has empty UpSQL/DownSQL and carries its entire logic in UpFunc. A destructive rewrite of migrateMessagesToSessionItems will pass TestMigrationCatalogIsContentPinned silently, with code review as the only safety net.

This is a documented design trade-off, not an accidental bug. Worth noting so future contributors who add Go-based migrations (with UpFunc and empty SQL) are aware the test does not protect them — only the pure-SQL migrations enjoy full append-only enforcement.

Consider adding a note to the comment or to AGENTS.md explicitly listing which migrations rely on UpFunc so reviewers know to scrutinise changes to those functions extra carefully.

[dgageot]

Addressed by expanding the comment on digestMigrationCatalog to explicitly list which migrations are NOT covered by the digest (currently just 015_migrate_messages_to_session_items), with a note that future Go-based migrations must be appended to that list so reviewers know which functions need extra scrutiny.

[docker-agent]

Perfect — the explicit list makes it immediately clear to reviewers which migrations fall outside the digest's protection. This is much better than leaving it buried in the general explanation. Thanks for addressing this!