diff --git a/client/VENDORING.md b/client/VENDORING.md index c746ed0..a12ad20 100644 --- a/client/VENDORING.md +++ b/client/VENDORING.md @@ -40,7 +40,7 @@ source is the pair a bisect wants: The tree's current recorded state — sha256-of-sha256s over every git-tracked file under `client/` except this one: -**state digest:** `42c4a314d998300123ba5ff488f269cba2fc4ecc6eff2bba8814f7f7ee4a09f7` +**state digest:** `34d7639263b56a316ea2da485f6ed070bbe6822b502f900228041b443e5186ed` `packaging/check_vendoring.py` asserts it on every push, and refuses any tracked file matching a §2 never-vendor class. **Any commit that touches diff --git a/client/shared/src/commonMain/kotlin/ai/ciris/mobile/shared/models/ClientMode.kt b/client/shared/src/commonMain/kotlin/ai/ciris/mobile/shared/models/ClientMode.kt index c641297..c72af40 100644 --- a/client/shared/src/commonMain/kotlin/ai/ciris/mobile/shared/models/ClientMode.kt +++ b/client/shared/src/commonMain/kotlin/ai/ciris/mobile/shared/models/ClientMode.kt @@ -219,14 +219,101 @@ fun clientModeFrom( // unaffected: same package, same name, same `const val`. /** - * Whether [nodeVersion] differs materially from [CLIENT_VERSION] — i.e. a - * non-blocking "update recommended" banner should be shown. Compares the - * leading `major.minor.patch` (ignoring any pre-release/build suffix) and only - * flags an actual mismatch (never flags when the node version is unknown/blank). + * The oldest node this client can drive. + * + * A FLOOR, not a pin. CIRISServer 0.5.192 moves its own dependency to + * `ciris-client>=0.5.190,<0.6` (CIRISServer#497) so the client can ship for the + * agent team without a paired server cut; this is the same relaxation from the + * other side. + * + * THIS NUMBER IS NOT YET EARNED THE WAY THE SERVER EARNED ITS FLOOR. The server + * mutation-tested theirs at both ends — lower it to 0.5.186 and the id gate + * fails, lower it to 0.5.188 and the wheel gate fails — so the bound is measured + * rather than asserted. Ours is the pairing both sides are standardising on and + * nothing here proves this client cannot drive an older node. Until a gate + * installs the floor and exercises the API surface this client actually calls, + * treat it as the server team put it: an untested bound is a guess with a + * version number on it. + */ +const val MIN_NODE_VERSION: String = "0.5.190" + +/** + * Compare two `major.minor.patch` versions NUMERICALLY. Negative, zero, positive. + * + * NOT string comparison, and the difference is live at the versions this repo + * actually ships. Lexically `"0.5.9" > "0.5.190"`, because '9' > '1' — so a + * floor check written the obvious way decides a 0.5.9 node satisfies a 0.5.190 + * floor and a 0.5.191 client is below a 0.5.9 one. Three-digit patch numbers + * make this the common case here, not an edge. + * + * The old equality check was immune to this because it never ORDERED anything. + * Introducing a floor introduces ordering, and ordering is the whole trap. + * + * Missing components read as 0, so `0.5` and `0.5.0` compare equal. A component + * that is not a number sinks to 0 rather than throwing: this drives a banner, + * and a malformed version should not be able to crash the surface that reports + * it. + */ +fun compareVersions(a: String, b: String): Int { + fun parts(v: String): List = + v.trim().removePrefix("v").takeWhile { it.isDigit() || it == '.' } + .split('.').map { it.toIntOrNull() ?: 0 } + val pa = parts(a) + val pb = parts(b) + for (i in 0 until maxOf(pa.size, pb.size)) { + val d = (pa.getOrNull(i) ?: 0) - (pb.getOrNull(i) ?: 0) + if (d != 0) return if (d < 0) -1 else 1 + } + return 0 +} + +/** + * Whether the node and this client are INCOMPATIBLE — i.e. a non-blocking + * "update recommended" banner should be shown. + * + * WAS EQUALITY, WHICH ONLY WORKED BECAUSE THE VERSIONS WERE LOCKED. While the + * server pinned `ciris-client==` the two were always equal, the + * comparison was always true, and the branch was never exercised. Widening to a + * range turns that held-still assumption into a defect: a 0.5.192 node with a + * 0.5.193 client is THE POINT of decoupling, and equality would greet its + * operator with a permanent nag. Trading a release dance for a permanent nag is + * the worse trade (CIRISClient#16). + * + * TWO DIRECTIONS, WHICH EQUALITY CONFLATED INTO ONE: + * + * - is the NODE too old for this client? [MIN_NODE_VERSION], held here, + * because a node that predates the whole idea of declaring a floor cannot + * tell us anything about itself. + * - is the CLIENT too old for this node? [nodeMinClientVersion], declared + * BY THE NODE, because only the node knows which clients it supports. A + * client-held answer to this cannot ever learn that a newer node needs a + * newer client — it would call itself compatible and be wrong. + * + * Nothing declares [nodeMinClientVersion] today, so it is optional and absent + * means "the node did not say", not "the node is happy". The floor this side + * holds still applies, which keeps the real signal the nag exists for: a + * genuinely incompatible pair still says so. Replacing equality with "never + * complain" would have deleted that signal, and someone shipped a mismatched + * pair once already. + * + * Never flags on an unknown or blank node version — that is "I could not ask", + * which is not "you are broken". */ -fun isVersionMismatch(nodeVersion: String?, clientVersion: String = CLIENT_VERSION): Boolean { +fun isVersionMismatch( + nodeVersion: String?, + clientVersion: String = CLIENT_VERSION, + nodeMinClientVersion: String? = null, + minNodeVersion: String = MIN_NODE_VERSION, +): Boolean { val node = nodeVersion?.trim()?.removePrefix("v")?.takeWhile { it.isDigit() || it == '.' } if (node.isNullOrBlank()) return false val client = clientVersion.trim().removePrefix("v").takeWhile { it.isDigit() || it == '.' } - return node != client + + // The node's own claim about what it needs, when it makes one. + val declared = nodeMinClientVersion?.trim()?.removePrefix("v") + ?.takeWhile { it.isDigit() || it == '.' } + if (!declared.isNullOrBlank() && compareVersions(client, declared) < 0) return true + + // And what this client needs of the node. + return compareVersions(node, minNodeVersion) < 0 } diff --git a/client/shared/src/commonTest/kotlin/ai/ciris/mobile/shared/models/ClientModeTest.kt b/client/shared/src/commonTest/kotlin/ai/ciris/mobile/shared/models/ClientModeTest.kt index 7b7f9a3..8db092c 100644 --- a/client/shared/src/commonTest/kotlin/ai/ciris/mobile/shared/models/ClientModeTest.kt +++ b/client/shared/src/commonTest/kotlin/ai/ciris/mobile/shared/models/ClientModeTest.kt @@ -330,12 +330,24 @@ class ClientModeTest { @Test fun version_mismatch_ignores_the_v_prefix() { - assertFalse(isVersionMismatch("v0.5.176", "0.5.176")) + // Intent unchanged; the EXAMPLE had to move. It used to pair 0.5.176 + // with 0.5.176 and assert no flag, which held only because the check was + // equality. Under a compatibility floor (CIRISClient#16) a 0.5.176 node + // is genuinely too old and SHOULD flag, so the prefix is now shown to be + // ignored at versions where the answer is not about age. + assertFalse(isVersionMismatch("v0.5.191", "0.5.191")) + assertTrue(isVersionMismatch("v0.5.176", "0.5.191")) } @Test - fun version_mismatch_fires_on_a_real_difference() { + fun version_mismatch_fires_on_a_node_below_the_floor() { + // RENAMED, because the reason changed. This used to be "fires on a real + // difference" — under equality, 0.5.175 vs 0.5.176 flagged because they + // differed. It still flags, but now because 0.5.175 is below + // MIN_NODE_VERSION, and a mere difference no longer flags anything: + // that is the decoupling CIRISServer#497 asked for. assertTrue(isVersionMismatch("0.5.175", "0.5.176")) + assertFalse(isVersionMismatch("0.5.192", "0.5.193")) } // ── The runtime declares what it is (CIRISAgent#1111) ────────────────── diff --git a/client/shared/src/commonTest/kotlin/ai/ciris/mobile/shared/models/CompatibilityFloorTest.kt b/client/shared/src/commonTest/kotlin/ai/ciris/mobile/shared/models/CompatibilityFloorTest.kt new file mode 100644 index 0000000..d7a7ba2 --- /dev/null +++ b/client/shared/src/commonTest/kotlin/ai/ciris/mobile/shared/models/CompatibilityFloorTest.kt @@ -0,0 +1,84 @@ +package ai.ciris.mobile.shared.models + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * The floor, and the ordering trap underneath it. + * + * The equality check this replaces never ordered anything, so it could not be + * wrong about order. A floor is nothing BUT order. + */ +class CompatibilityFloorTest { + + // ---- the trap ---------------------------------------------------- + + @Test + fun numeric_not_lexical_at_the_versions_we_actually_ship() { + // Lexically "0.5.9" > "0.5.190" because '9' > '1'. Every one of these + // is a pair this repo could really see: three-digit patch numbers make + // it the common case, not an edge. + assertTrue(compareVersions("0.5.9", "0.5.190") < 0, "0.5.9 is BELOW 0.5.190") + assertTrue(compareVersions("0.5.191", "0.5.9") > 0, "0.5.191 is ABOVE 0.5.9") + assertTrue(compareVersions("0.5.88", "0.5.190") < 0, "0.5.88 is BELOW 0.5.190") + assertEquals(0, compareVersions("0.5.191", "v0.5.191")) + assertEquals(0, compareVersions("0.5", "0.5.0")) + } + + @Test + fun a_malformed_version_cannot_crash_the_banner() { + compareVersions("", "0.5.190") + compareVersions("not-a-version", "0.5.190") + compareVersions("0.5.x", "0.5.190") + } + + // ---- the point of decoupling ------------------------------------- + + @Test + fun a_mixed_but_compatible_pair_says_nothing() { + // THE case the range exists for: client ahead of the node, both above + // the floor. Equality nagged here, permanently, on every such pair. + assertFalse(isVersionMismatch("0.5.192", clientVersion = "0.5.193")) + assertFalse(isVersionMismatch("0.5.190", clientVersion = "0.5.199")) + assertFalse(isVersionMismatch("0.5.191", clientVersion = "0.5.191")) + } + + @Test + fun a_node_below_the_floor_still_says_so() { + // The signal the nag exists for. "Never complain" would have deleted it. + assertTrue(isVersionMismatch("0.5.188", clientVersion = "0.5.191")) + assertTrue(isVersionMismatch("0.5.186", clientVersion = "0.5.191")) + // And the lexical trap must not rescue a too-old node: "0.5.9" reads as + // greater than "0.5.190" to a string compare. + assertTrue(isVersionMismatch("0.5.9", clientVersion = "0.5.191")) + } + + // ---- the direction only the node can answer ---------------------- + + @Test + fun a_node_may_declare_a_client_floor_of_its_own() { + // A newer node needing a newer client: no client-held constant can ever + // know this, which is why the node has to be able to say it. + assertTrue(isVersionMismatch("0.6.0", clientVersion = "0.5.191", nodeMinClientVersion = "0.6.0")) + assertFalse(isVersionMismatch("0.6.0", clientVersion = "0.6.1", nodeMinClientVersion = "0.6.0")) + } + + @Test + fun a_node_that_declares_nothing_is_not_taken_as_content() { + // Absent means "did not say", not "is happy". Every node today is here, + // so this is the live path, and the floor this side holds still applies. + assertFalse(isVersionMismatch("0.5.191", clientVersion = "0.5.191", nodeMinClientVersion = null)) + assertTrue(isVersionMismatch("0.5.186", clientVersion = "0.5.191", nodeMinClientVersion = null)) + } + + // ---- unchanged behaviour ----------------------------------------- + + @Test + fun an_unknown_node_version_is_not_a_verdict() { + assertFalse(isVersionMismatch(null)) + assertFalse(isVersionMismatch("")) + assertFalse(isVersionMismatch(" ")) + } +}