feat(migrations): zero-touch auto-migration runner + version handshake (runtime slice)#690
Conversation
…on handshake (ops-1l18 runtime slice) Runtime slice for the zero-touch upgrade design (both specs K&S-approved): a boot-keyed migration runner (resources/migrations/*) plus the CLI<->server version handshake (src/version-handshake.ts). GitHub Actions lanes and the prefix flip are separate, later PRs. Runner: MigrationRegistry + runner.ts orchestrates detect -> one shared async pre-hash (computed after boot-ready, before any first write) -> per-migration pre-flight ladder (space check with a 90% headroom floor -> prune -> risk-scoped snapshot -> content-only export fallback -> halt with the exact reason) -> throttled batches with per-row markers -> risk-class completion gate (derived-only: count+marker; schema-additive: count+full-envelope; content-transform: count+old-row-envelope+ new-row-presence) -> post-hash -> structural-only ledger OrgEvent -> state-file update -> snapshot prune. Single-flight via an in-process mutex + a stale-tolerant file lock. Never throws - every failure path resolves to a halted/failed progress entry, never an exception out of the boot path. embedding-stamp.ts (always-active, derived-only) reuses Memory.put()'s own regen branch via a genuine admin-authenticated loopback PUT /Memory/:id - the same mechanism `flair reembed` already uses in production - rather than a bare in-process databases.flair.Memory.put() call, which was found empirically (building the integration test against real Harper) to bypass resources/Memory.ts's subclass entirely. Also required adding embeddingModel: String @indexed to schemas/memory.graphql (purely additive - the field was already written but never declared, so it wasn't queryable at all). synthetic-test-migration.ts (schema-additive) is the CI-only variant that exercises the full runner path end-to-end; registers ONLY when FLAIR_ENABLE_TEST_MIGRATIONS === "1" (exact-match opt-in, unit-tested), additionally scoped to a reserved never-real agent id. version-handshake.ts: public GET /Health now also reports `version` (resources/health.ts); every CLI command gets a cached (~60s TTL) check via a global preAction hook, gated on process.stdout.isTTY (an unconditional extra request broke test/unit/presence-set.test.ts's single-shot mock server - fixed here). `flair doctor` shows the version triple + migration state and --fix offers a restart on mismatch. Tests: 134 new unit tests (fake-table/fake-regen coverage of the full runner cycle across all three risk-class gates, lock stale-break, space headroom floor, 0700 enforcement, snapshot retention union, ledger structural-cleanliness, version-handshake caching) + 11 new integration tests against real ephemeral Harper (synthetic migration end-to-end, resume-after-mid-migration-kill via a genuine SIGTERM + restart, halt-on-blocked-space, embedding-stamp's real regen path, version handshake against a live /Health). Full existing suite (2024 unit / 276 integration) verified green before and after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tps-sherlock
left a comment
There was a problem hiding this comment.
Security review: PR #690 — zero-touch auto-migration runner + version handshake. All five gate items pass.
(1) embedding-stamp.ts admin-authenticated loopback — CLEAN.
resolveAdminAuthHeader() reads HDB_ADMIN_PASSWORD ?? FLAIR_ADMIN_PASSWORD from env, constructs a Basic admin:<pass> header in-memory, and sends it to http://127.0.0.1:<port>/Memory/<id>. The credential is:
- Never logged (no console.log, no error message interpolation)
- Never in the ledger (ledger.ts records structural metadata only — see item 3)
- Never in errors (the catch block returns
falsesilently; the error object is discarded) - Loopback-only (
127.0.0.1hardcoded inresolveSelfBaseUrl()) — never traverses a network interface - Same mechanism as
flair reembed(src/cli.ts) andauth-middleware.ts'sgetAdminPass()— no new bypass surface
The "no credential → skip and retry next boot" behavior is correct: resolveAdminAuthHeader() returns null → regenViaHttpPut returns false → the row stays in its explicit-null state (queryable, retried). No credential leakage path exists. ✅
(2) dir-safety.ts — matches the spec requirement exactly.
ensureSecureDir: mkdir(0700) → explicit chmodSync(0700) → stat-verify. The chmod-after-mkdir covers a permissive umask silently widening mkdir's mode argument. The stat-verify catches unusual filesystem/ACL edge cases where chmod doesn't stick. verifySecureDir checks (mode & 0o077) !== 0 — any group/other bit set → UnsafeDirectoryError with path + actual octal mode in the message. writeSecureFile re-verifies the directory immediately before each write (TOCTOU defense — a directory re-permissioned between creation and a later write in a long-running batch). File written at 0600. This is exactly the semantics I specified. ✅
(3) ledger.ts — structural metadata only.
buildLedgerDetail() serializes: migrationId, initiator, fromVersion, toVersion, scope, startedAt, endedAt, outcome, rowsProcessed, rowsRemaining, hashEnvelopeMatch, error. No memory IDs, no content, no agent IDs (authorId is the constant "flair-migrations"). Written via databases.flair.OrgEvent.put() with internal auth (resolveAgentAuth(undefined) → {kind: "internal"}). The detail field is a JSON string of the same structural fields — nothing else is ever written. ✅
(4) synthetic-test-migration.ts — cannot activate in prod.
Two independent gates, either sufficient alone:
- Registration:
FLAIR_ENABLE_TEST_MIGRATIONS === "1"— exact-match string comparison, not truthy/"true"/NODE_ENV=test. The env var name is explicit and unlikely to be set accidentally. - Scope: even if registered, all queries are scoped to
RESERVED_TEST_AGENT_ID = "__flair_migration_synthetic_test_agent__"— a double-underscore-prefixed reserved name that can never match a real agent. It can never see or touch genuine memories.
Belt-and-suspenders. The env gate alone is sufficient; the agentId scope is correct defense-in-depth. ✅
(5) Halt states + version handshake — no new disclosure surface.
/Health(public, anonymous): version field added. The comment explicitly acknowledges my prior verdict — fine while locally bound; if ever fronted publicly over Fabric, must omit. The richer/HealthDetailalready carried version pre-existing (auth-gated). This only adds it to the anonymous endpoint too, and the comment correctly flags the future constraint.- Halt states surface in
/HealthDetail(auth-gated) asstats.migrationswith per-migration{id, rowsDone, rowsRemaining, state, reason}and as warnings. No memory content or IDs exposed. - CLI handshake: TTY-only stderr nudge, never changes exit codes or stdout. Gated on
process.stdout.isTTY— piped/scripted/CI invocations never pay the extra round trip. No machine-consumed signal.
Additional notes:
- The
waitForEmbeddingsSettled()probe (up to ~8.5s) is correct — prevents the known gap where a row'sembeddingModelwas never set because the engine wasn't warm. Bounded timeout, proceeds regardless. - The
not_equal+equals nullOR-combined pending condition correctly handles Harper's index semantics (truly-absent properties are invisible to queries; explicit-null is queryable). The explicit-null-on-failure guarantee is load-bearing and correctly implemented. runMigrationCycleis documented to never throw — the boot-path catch inscheduleMigrationBootis defense-in-depth only. Correct per invariant II (halt-don't-brick).
No findings. Ship it. 🔥
tps-sherlock
left a comment
There was a problem hiding this comment.
Sherlock Review — APPROVED
Reviewed the full diff (22 files, +5864/-47). All five security gate items confirmed correct.
(1) embedding-stamp.ts admin-authenticated loopback — CLEAN
resolveAdminAuthHeader() reads HDB_ADMIN_PASSWORD / FLAIR_ADMIN_PASSWORD from process.env — the same resolution auth-middleware.ts's getAdminPass() uses. The credential is used to construct a Basic auth header for a loopback PUT /Memory/:id to http://127.0.0.1:<HTTP_PORT>.
No credential leakage:
- The auth header is constructed inline, used once, never stored
- The
fetchcall goes to127.0.0.1— never leaves the host - On failure (
!authHeader), the function returnsfalsewith no log message containing the credential - The error path (
catch { return false }) swallows the error silently — no credential in error messages - The module doc correctly explains WHY the loopback HTTP call is load-bearing:
databases.flair.Memoryresolves to the raw table, not theresources/Memory.tssubclass — only a genuine HTTP dispatch reaches the regen branch
No new bypass surface vs. the authorizeLocal history: this uses Basic auth over loopback, the same mechanism flair reembed already uses in production. The authorizeLocal escalation class (flair#614/#654) was about credential-LESS loopback requests being forged as super_user — this path carries a real credential, so it's not in that class.
(2) dir-safety.ts 0700 enforcement — CORRECT
ensureSecureDir(): mkdirSync(dir, { recursive: true, mode: 0o700 }) → verifySecureDir() → if that throws, chmodSync(dir, 0o700) → verifySecureDir() again (throws loudly if still not compliant). The verifySecureDir() function checks (st.mode & 0o777) & 0o077 !== 0 — any group/other bit set → UnsafeDirectoryError with the path and actual octal mode in the message.
writeSecureFile() re-verifies the containing directory is still 0700 immediately before each write — not just at creation time. This covers the "directory re-permissioned between creation and write" case.
The error message matches my wording exactly: "refusing to use : expected 0700, found (group/other-accessible) — fix permissions (chmod 700 ) or point the migration at a different directory." Correct.
(3) ledger.ts OrgEvent fields — STRUCTURAL-METADATA-ONLY ✓
buildLedgerDetail() serializes exactly: migrationId, initiator, fromVersion, toVersion, scope, startedAt, endedAt, outcome, rowsProcessed, rowsRemaining, hashEnvelopeMatch, and optionally error. No memory IDs, no content, no agent IDs beyond the fixed authorId: "flair-migrations". The scope field is "full" or "canary" — never names specific agents. The module doc explicitly states "never memory IDs or content summaries." Clean.
(4) synthetic-test-migration.ts no-ship gate — CORRECT
Three-layer defense:
- Registration conditional on
FLAIR_ENABLE_TEST_MIGRATIONS === "1"— exact-match string comparison, not a genericNODE_ENV=testcheck - Even if registered unconditionally, queries scoped to
RESERVED_TEST_AGENT_ID = "__flair_migration_synthetic_test_agent__"— a reserved, never-real agent ID - The
shouldRegisterSyntheticMigration()function is exported and unit-tested independently — the test confirms it returnsfalsewhen the env var is unset
The unit test (test/unit/migrations-synthetic.test.ts) explicitly asserts: "registers ONLY embedding-stamp when FLAIR_ENABLE_TEST_MIGRATIONS is unset — the synthetic variant never ships active." Correct.
(5) Halt states + version handshake — SAFE
Halt state: haltMigration() sets progress to state: "halted" with a reason string, writes a ledger event with outcome: "halted", and the service keeps running on the pre-migration shape. The isShortCircuited() check only fires on lastOutcome === "success" — a halted migration is retried on every boot. The pre-hash runs async after boot serves — no blocking. Correct.
Version handshake: checkServerHandshake() reads the server version from the existing health endpoint (locally-bound Harper ops port). The version is already exposed there — this PR doesn't broaden the exposure surface. The handshake is a read-only comparison (CLI version vs. server version) with a one-line stderr nudge on mismatch. No new disclosure.
Verdict
All five security gate items confirmed correct. The 0700 enforcement matches my stat-verify requirement exactly. The ledger is structural-metadata-only. The synthetic migration gate is belt-and-suspenders. The halt state is safe under additive-first. Approving.
tps-kern
left a comment
There was a problem hiding this comment.
Approved — all five ratification points verified, all five deviations adjudicated.
Ratification points:
1. Async-after-ready envelope (migration-boot.ts): ✅
scheduleMigrationBoot() uses setImmediate — server is accepting connections before any migration write. waitForTablesReady() polls for Memory/Relationship table accessors (bounded 30s). waitForEmbeddingsSettled() waits up to 8.5s for getMode()==="local" before running embedding-stamp (prevents the null-embeddingModel gap). runMigrationCycle never throws (defense-in-depth catch in the boot path). Server serves on old shape throughout — correct per the async-after-ready verdict.
2. Risk-class posture (risk-policy.ts): ✅
Three risk classes with distinct postures: derived-only (batch 50, metadata-only snapshot, count+marker gate), schema-additive (batch 50, schema+metadata snapshot, count+full-envelope gate), content-transform (batch 10, pointers+metadata snapshot, count+old-row-envelope+new-row-presence gate). Batch sizes and gate strictness scale with risk — correct.
3. Cheap detect (bounded query): ✅
embedding-stamp detect uses search with limit:1 on the pending condition (OR of not_equal + equals-null). Index-seekable on embeddingModel (now @indexed — see deviation 3). Returns true on first match — bounded, not a full scan. Synthetic migration uses the same pattern with agentId + source conditions.
4. Single-flight (lock.ts): ✅
In-process mutex (module-scope flag) + file lock (O_CREAT|O_EXCL atomic create, {pid, hostname, startedAt} in lock file). Stale detection: pid not alive OR file older than 5min. Dead holder → break and proceed. Documented as single-host only, Fabric-wide explicitly deferred — matches the v1 verdict.
5. Synthetic no-ship gate: ✅
FLAIR_ENABLE_TEST_MIGRATIONS === "1" (exact-match, not truthy/NODE_ENV). Scoped to RESERVED_TEST_AGENT_ID (double-underscore, never real). Belt-and-suspenders — either gate alone is sufficient. riskClass schema-additive (exercises the stricter envelope gate path). Correct CI payload.
Deviation adjudications:
(1) Migration snapshots = logical JSONL, not flair snapshot tar: ✅ ACCEPTED.
The physical tar (flair snapshot create) requires stopping Harper — the in-process runner cannot stop itself. The logical snapshot (manifest + risk-class-scoped JSONL, 0700 dirs, 0600 files) is the correct alternative. It satisfies invariant III's scope-by-risk-class requirement: derived-only gets metadata-only, schema-additive gets schema+metadata, content-transform gets pointers+metadata. The CI restore drill should target THIS mechanism (not the tar restore) — the drill restores from the logical snapshot and verifies row counts + pointer chains. The physical tar remains the manual/operator-level backup; the logical snapshot is the migration-specific pre-flight backup. Two mechanisms, two purposes.
(2) Embedding-stamp regen via admin-authenticated loopback PUT, not in-process table put: ✅ ACCEPTED.
Empirically verified: the raw databases.flair.Memory reference bypasses the Memory.ts subclass (which holds the regen/dedup/auth logic). A loopback PUT to /Memory/:id goes through Harper's REST dispatch → Memory.ts subclass → regen branch. This is the SAME mechanism flair reembed (src/cli.ts) uses in production. Not a new pattern — it's the existing prod pattern, correctly applied here. The admin credential is loopback-only, never logged, never in the ledger (Sherlock verified).
(3) embeddingModel String @indexed (was written but never declared): ✅ ACCEPTED.
The field existed in the schema but wasn't @indexed, meaning Harper's query engine couldn't seek on it — every condition-based query on embeddingModel was a scan. Adding @indexed makes the detect() query a seek instead of a scan, which is what makes "cheap detect" actually cheap. This is an additive schema change (adding an index to an existing field) — no data migration needed, Harper builds the index on the next write. Correct and necessary.
(4) Version-handshake preAction gated on isTTY: ✅ ACCEPTED.
The handshake nudge prints to stderr only when process.stdout.isTTY is true — piped/scripted/CI invocations skip the nudge (no noise in logs, no parseable signal for scripts). doctor still surfaces the version triple regardless of TTY. This is the right posture: interactive users get the nudge, automation gets silence. The handshake check itself (HTTP call to /health) still runs regardless of TTY — only the OUTPUT is gated. The cache is per-(rootPath, serverUrl), so multi-instance CLI use doesn't bleed.
(5) KNOWN GAP: rows whose embeddingModel was never written are invisible to bounded detect: ✅ ACCEPTED — the proposed mitigation lands in THIS PR.
The gap: Harper doesn't index truly-absent properties (a property that was never SET, vs explicitly null). A row whose embeddingModel was never written (only possible if the embeddings engine was down for the entire write) is invisible to the not_equal + equals-null OR condition.
The proposed mitigation: the pre-hash envelope pass already reads every row — have it tally rows lacking embeddingModel and surface the count in health. This is the right call. It closes the gap with zero extra passes (the envelope is already O(corpus), and the tally is a free byproduct of the row read). The count in health surfaces the issue for an operator (or for a future migration that handles truly-absent fields), without requiring a separate scan.
Lands in THIS PR, not a fast-follow — the envelope pass is already there, adding a counter is trivial, and shipping a known gap without the free mitigation would be an avoidable quality miss.
Additional notes:
- Sherlock already approved — no security findings.
- The waitForEmbeddingsSettled() race-prevention window is a real fix found during implementation. Correct: a migration that re-embeds a row while the engine is still warming up silently fails (getEmbedding returns null), leaving embeddingModel null. The 8.5s wait window (just over the probe's 8s timeout) ensures getMode() is settled before any regen attempt.
- The explicit-null-on-failure guarantee in embedding-stamp.ts is load-bearing and correctly implemented: if regen fails, the row lands back in explicit-null (queryable, retried), never truly-absent (permanently invisible).
- 2158 unit + 287 integration pass. Resume-after-kill proven on live Harper (140 rows, SIGTERM mid-flight, resumed + completed). Halt-on-space proven (server kept serving, row untouched). These are the right integration tests for the invariants.
Ship it.
…ch-delay knob + two-phase observation (CI robustness) CI failed on migrations-resume-after-kill.test.ts alone: on a slow shared runner the single 30s mid-flight window expired before the migration even reached "running" (the deferred start — tables-ready wait + embeddings settle + async pre-hash — working as designed, unbudgeted by the test), and at the 100ms prod throttle the whole running phase for the 140-row corpus is a few hundred ms, catchable only probabilistically between HTTP health polls. Test-robustness bug, not a runtime bug. - runner.ts: FLAIR_MIGRATION_TEST_BATCH_DELAY_MS, a test-only batch- throttle override following space.ts's FLAIR_MIGRATION_TEST_FREE_BYTES pattern (env var is the only injection lever into the spawned Harper child). DOUBLE-GATED: honored only when the synthetic CI-migration gate (FLAIR_ENABLE_TEST_MIGRATIONS === "1") is active, so a stray env var on a prod deployment can never alter the real 100ms throttle. Invalid/negative values ignored. - resume test: sets the knob to 2500ms (each mid-flight window now ~2.5s — the kill lands mid-flight deterministically, not probabilistically) and splits observation into two phases: wait for state=running with a 120s deadline (covers deferred start on slow runners), THEN catch mid-flight with 100ms polls. The too-fast guard is kept in both phases — if "completed" is ever seen before a mid-flight state, the test fails loudly instead of silently passing without exercising the kill. Post-restart completion deadline also widened to 120s; the resumed-work slack assertion tightened to <= killedAt.rowsRemaining (an upper bound by construction). - unit tests: pin the knob's double gate (honored with gate on, ignored with gate off, unparseable values ignored). Verified: fixed test passed 3x consecutively (~25s each); full suites green after the change (unit 2161 pass / 0 fail, integration 287 pass / 0 fail); resources typecheck clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tps-kern
left a comment
There was a problem hiding this comment.
Re-approved — the CI-robustness delta is clean.
b2effa7 delta review:
1. FLAIR_MIGRATION_TEST_BATCH_DELAY_MS (runner.ts): ✅
Double-gated correctly: resolveTestBatchDelayMs() returns undefined unless shouldRegisterSyntheticMigration(env) is true (requires FLAIR_ENABLE_TEST_MIGRATIONS === "1"). Invalid values (non-numeric, negative) ignored via Number.isFinite(n) && n >= 0. The default batchDelayMs resolution is deps.batchDelayMs ?? resolveTestBatchDelayMs() ?? 100 — prod path (no test env var) always lands at 100ms. A stray env var can never alter the prod throttle. Three unit tests pin the three cases (honored gate-on, ignored gate-off, unparseable ignored). Correct.
2. Two-phase resume-after-kill test: ✅
Phase 1: wait for state=running with 120s deadline (covers the async-deferred start from migration-boot.ts's setImmediate + waitForTablesReady + waitForEmbeddingsSettled). Phase 2: catch mid-flight via the 2.5s test knob window. Too-fast guard kept. Resumed-work slack tightened to <= killedAt.rowsRemaining (was looser before — tighter is correct, the resumed run should not process MORE rows than were remaining at kill time). The test bug was not budgeting for the async-deferred start + ~300ms running phase at prod throttle — the knob fixes this deterministically without altering runtime behavior.
3. Unit tests for the knob: ✅
Three new tests pin the double-gate invariant: gate-on + valid value → honored, gate-off + valid value → ignored (prod behavior), gate-on + unparseable → ignored. Correct coverage.
Runtime behavior beyond the gated knob is untouched. 3x local stability + 2161 unit + 287 integration green. Ship it.
…— pin the stamp so only the migration under test runs (root cause of CI failure #2, evidence in-file) + timeout self-diagnostics Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tps-kern
left a comment
There was a problem hiding this comment.
Re-approved — test isolation fix is correct. Pinning FLAIR_EMBEDDING_MODEL so embedding-stamp detects nothing (rows seeded with that exact stamp) isolates the synthetic migration under test. Runtime untouched. The ground-truth assertion proving the isolation held is the right guard. 15.5s pass, was 124s timeout. Ship it.
@/private/tmp/claude-501/-Users-squeued-agents-flint/f701f601-31bb-40d0-96dc-6bc3d51e2760/scratchpad/pr-body.md