From 43040f0f786ba8c275d77bf7ccf1d2848fe6f626 Mon Sep 17 00:00:00 2001 From: jellologic <31935831+jellologic@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:24:21 -0400 Subject: [PATCH] Stamp a literal version in fromV3, not the moving constant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fromV3` stamped `version: SUPPORTED_VERSION`. `migrate()` dispatches on that number, so a rung stamping the CURRENT version makes the ladder skip every step above it: the draft claims to be current while still carrying the older shape. `fromV1` and `fromV2` both stamp literals and both carry a comment warning about exactly this, the second of which says "the moment SUPPORTED_VERSION became 4 this was the bug it predicted." It then happened one rung further up. Latent today and invisible for a specific reason: while SUPPORTED_VERSION is 4, `SUPPORTED_VERSION` and `4` produce identical output. It becomes wrong the moment a v5 exists, at which point a v3 config is marked v5 while holding v4 shape — and `fs-config-store` would write that back. That same property means a behavioural test cannot fail on it today. The added tests are therefore two kinds: - Per-rung assertions that each `fromVN` returns N+1, written against literals rather than SUPPORTED_VERSION. Against the constant they would have passed while the bug was present, which is how it survived review. These are future guards and cannot fail now. - A source-level guard, in the spirit of test/architecture.test.ts, asserting no rung stamps a non-literal. This one does fail on the unfixed code, verified by reintroducing the bug. Each rung is fed its own minimal input rather than the previous rung's output, so the tests need no widening cast. Refs #37 Signed-off-by: jellologic <31935831+jellologic@users.noreply.github.com> --- src/core/migrate.ts | 9 +++- test/core/migrate.test.ts | 87 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/src/core/migrate.ts b/src/core/migrate.ts index b2a10d1..9abd3bd 100644 --- a/src/core/migrate.ts +++ b/src/core/migrate.ts @@ -302,7 +302,14 @@ export function fromV3(raw: Record): V4Draft { const { agentProfiles: _dropped, ...rest } = raw return { ...rest, - version: SUPPORTED_VERSION, + // LITERAL 4, not SUPPORTED_VERSION — the third time this warning has been + // written and the first time it was needed. `fromV1` and `fromV2` both stamp + // their own output version because `migrate` dispatches on the number: a rung + // that stamps the CURRENT version makes the ladder skip every step above it. + // This one stamped the constant, so a v3 config would have been marked v5 + // while still carrying v4 shape the moment a v5 existed. It was invisible + // only because SUPPORTED_VERSION happened to equal 4. + version: 4, providerAccounts: isPlainObject(raw.providerAccounts) ? (raw.providerAccounts as Record>) : {}, diff --git a/test/core/migrate.test.ts b/test/core/migrate.test.ts index ab385bd..60736b5 100644 --- a/test/core/migrate.test.ts +++ b/test/core/migrate.test.ts @@ -1,15 +1,19 @@ import test from 'node:test' +import { readFileSync } from 'node:fs' import assert from 'node:assert/strict' import { SUPPORTED_VERSION, emptyState, fromV1, + fromV2, + fromV3, isV1, migrate, normalize, validateProfileName, } from '../../src/core/migrate.ts' import { makeProfile } from '../support/fixtures.ts' +import type { ConfigV2 } from '../../src/ports/config-store.ts' /** Exactly the shape swisscode 0.1.0 writes. */ const V1_FULL = { @@ -293,3 +297,86 @@ test('profile names: grammar, reserved words and the common-word guard', () => { assert.ok(!validateProfileName('fix').ok) assert.ok(validateProfileName('fix', { force: true }).ok) }) + +/** + * Each rung stamps its OWN output version, never the moving constant. + * + * `migrate` dispatches on the version number, so a rung that stamps + * SUPPORTED_VERSION makes the ladder skip every step above it: the draft claims + * to be current while still carrying the older shape. `fromV1` and `fromV2` + * carried a comment warning about this; `fromV3` did it anyway, and it was + * invisible only because SUPPORTED_VERSION happened to equal its own output. + * + * These assert LITERALS deliberately. Written against SUPPORTED_VERSION they + * would have passed while the bug was present, which is how it survived. + * + * Each rung is fed its own minimal input rather than the previous rung's + * output, so no widening cast is needed to express the test. + */ +const V2_MIN: ConfigV2 = { + version: 2, + profiles: { work: { provider: 'zai', apiKey: 'k' } }, + defaultProfile: 'work', + bindings: {}, + settings: {}, +} + +const V3_MIN: Record = { + version: 3, + providerAccounts: { work: { provider: 'zai', apiKey: 'k' } }, + agentProfiles: { work: { models: { opus: 'glm-5.2' } } }, + profiles: { work: { agentProfile: 'work', accounts: ['work'] } }, + defaultProfile: 'work', + bindings: {}, + settings: {}, +} + +test('fromV1 stamps version 2, not the current version', () => { + assert.equal(fromV1(V1_FULL).version, 2) +}) + +test('fromV2 stamps version 3, not the current version', () => { + assert.equal(fromV2(V2_MIN).version, 3) +}) + +test('fromV3 stamps version 4, not the current version', () => { + assert.equal(fromV3(V3_MIN).version, 4) +}) + +/** + * The guard that can actually fail today. + * + * The behavioural tests above cannot: while SUPPORTED_VERSION is 4, the buggy + * `version: SUPPORTED_VERSION` and the correct `version: 4` produce identical + * output. That is precisely why the bug survived review and sat latent — it has + * no observable behaviour until a v5 exists, at which point it silently marks + * v3 configs as v5. + * + * So this reads the source instead, in the spirit of test/architecture.test.ts: + * a rung may not stamp the moving constant, whatever that constant happens to + * equal right now. + */ +test('no migration rung stamps the moving version constant', () => { + const src = readFileSync(new URL('../../src/core/migrate.ts', import.meta.url), 'utf8') + // Each rung's body runs to the next top-level export. + for (const match of src.matchAll(/export function (fromV\d+)\b/g)) { + const start = match.index ?? 0 + const next = src.indexOf('\nexport ', start + 1) + const body = src.slice(start, next === -1 ? src.length : next) + const stamp = body.match(/^\s*version: (.+),$/m) + assert.ok(stamp, `${match[1]} has no version stamp`) + assert.match( + stamp[1] ?? '', + /^\d+$/, + `${match[1]} must stamp a literal version, not ${stamp[1]} — ` + + 'migrate() dispatches on this number and a moving constant makes the ladder skip rungs', + ) + } +}) + +test('a v3 config migrates to exactly v4, one rung at a time', () => { + const result = migrate(V3_MIN) + assert.equal(result.migratedFrom, 3) + assert.equal(result.state.version, 4) + assert.equal(result.readOnly, false) +})