From 42bf2a43d8f20451efb0a22d6f41640c35d413bd Mon Sep 17 00:00:00 2001 From: davotoula Date: Tue, 28 Jul 2026 21:32:28 +0200 Subject: [PATCH] fix: address code-review findings on the buzz CLI + wrapper scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the review of the Sonar literal-extraction pass. None were introduced by that pass — it renamed or sat next to each one. no_relays told the wrong story. `relaysFor` returns an empty *non-null* set when --relays was given but every entry failed `normalizeGroupRelay`, so the elvis to `outboxRelays()` never fires and the user was told to pass the flag they had just passed. The message now names the real problem and echoes the rejected value; the `no_relays` error code is unchanged. `participants` was string-munged. `arr.toString().trim('[',']').split(",")` turns any shape other than a flat string array into plausible-looking fake pubkeys, and splits a comma-bearing string into two entries. Now decoded as a JsonArray, with non-string elements skipped rather than stringified. Extracted to `participantsOf` so it is testable; BuzzParticipantsTest covers the shapes and was confirmed to fail (3 of 5 cases) against the old implementation. Nothing kept the two copies of the buzz-agent wrappers in sync. They exist as byte-identical trees under cli/src/main/resources/buzz-agent (what `agent up` extracts) and tools/buzz-agent (what the README tells people to use), with no build step or test pinning them — I had to hand-apply the same patch twice this week. BuzzAgentWrapperSyncTest now asserts it. The tools/ tree is declared as a `:cli:test` input, without which Gradle calls the task up-to-date after a tools/-only edit and misses exactly the drift being guarded (verified both ways). --- cli/build.gradle.kts | 11 +++ .../amethyst/cli/commands/BuzzCommands.kt | 58 ++++++++++------ .../amethyst/cli/BuzzAgentWrapperSyncTest.kt | 68 +++++++++++++++++++ .../amethyst/cli/BuzzParticipantsTest.kt | 67 ++++++++++++++++++ 4 files changed, 182 insertions(+), 22 deletions(-) create mode 100644 cli/src/test/kotlin/com/vitorpamplona/amethyst/cli/BuzzAgentWrapperSyncTest.kt create mode 100644 cli/src/test/kotlin/com/vitorpamplona/amethyst/cli/BuzzParticipantsTest.kt diff --git a/cli/build.gradle.kts b/cli/build.gradle.kts index 0da6af4bdb..d92227363e 100644 --- a/cli/build.gradle.kts +++ b/cli/build.gradle.kts @@ -29,6 +29,17 @@ tasks.withType().configureEach { duplicatesStrategy = DuplicatesStrategy.INCLUDE } +// BuzzAgentWrapperSyncTest compares the bundled buzz-agent wrappers against their +// tools/ reference copies. The reference tree lives outside this module, so without +// declaring it Gradle calls :cli:test up-to-date after a tools/-only edit — exactly the +// drift the test exists to catch. +tasks.named("test") { + inputs + .dir(rootProject.layout.projectDirectory.dir("tools/buzz-agent")) + .withPropertyName("buzzAgentWrapperReference") + .withPathSensitivity(PathSensitivity.RELATIVE) +} + dependencies { implementation(project(":quartz")) implementation(project(":commons")) diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/BuzzCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/BuzzCommands.kt index ab66424c51..43dcd861c0 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/BuzzCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/BuzzCommands.kt @@ -46,7 +46,10 @@ import com.vitorpamplona.quartz.utils.TimeUtils import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import kotlinx.serialization.json.Json +import kotlinx.serialization.json.JsonPrimitive import kotlinx.serialization.json.buildJsonObject +import kotlinx.serialization.json.contentOrNull +import kotlinx.serialization.json.jsonArray import kotlinx.serialization.json.jsonObject import kotlinx.serialization.json.jsonPrimitive import kotlinx.serialization.json.put @@ -147,7 +150,7 @@ object BuzzCommands { ctx.prepare() val me = ctx.identity.pubKeyHex val relays = relaysFor(ctx, relaysFlag) - if (relays.isEmpty()) return Output.error("no_relays", NO_RELAYS_MSG) + if (relays.isEmpty()) return Output.error("no_relays", noRelaysMsg(relaysFlag)) // The deployed Buzz relay does NOT emit kind-41001; instead it (a) confirms a DM's // channel id synchronously in the open OK, and (b) addresses each member a kind-44100 @@ -177,25 +180,9 @@ object BuzzCommands { .sortedByDescending { it.createdAt } .take(limit) .map { sys -> - // The relay's dm_created content carries a `participants` array our - // SystemMessagePayload model drops; read it from the raw content. - val participants = - runCatching { - jsonParser - .parseToJsonElement(sys.content) - .jsonObject["participants"] - ?.let { arr -> - arr - .toString() - .trim('[', ']') - .split(",") - .map { it.trim().trim('"') } - .filter { it.isNotBlank() } - } - }.getOrNull().orEmpty() mapOf( "dm_id" to sys.channel(), - "participants" to participants, + "participants" to participantsOf(sys.content), "created_at" to sys.createdAt, ) } @@ -518,7 +505,7 @@ object BuzzCommands { ctx.prepare() val me = ctx.identity.pubKeyHex val relays = relaysFor(ctx, relaysFlag) - if (relays.isEmpty()) return Output.error("no_relays", NO_RELAYS_MSG) + if (relays.isEmpty()) return Output.error("no_relays", noRelaysMsg(relaysFlag)) val filter = Filter(kinds = listOf(AgentTurnMetricEvent.KIND), tags = mapOf("p" to listOf(me))) val decrypted = @@ -572,7 +559,7 @@ object BuzzCommands { ctx.prepare() val me = ctx.identity.pubKeyHex val relays = relaysFor(ctx, relaysFlag) - if (relays.isEmpty()) return Output.error("no_relays", NO_RELAYS_MSG) + if (relays.isEmpty()) return Output.error("no_relays", noRelaysMsg(relaysFlag)) val filter = Filter(kinds = listOf(PersonaEvent.KIND), authors = listOf(me)) val personas = @@ -600,8 +587,35 @@ object BuzzCommands { } } - /** Message for the `no_relays` error: neither `--relays` nor the account's outbox had one. */ - private const val NO_RELAYS_MSG = "no relays: pass --relays ws://…" + /** + * The relay's `dm_created` content carries a `participants` array our [SystemMessageEvent] + * payload model drops, so read it off the raw content. + * + * Decoded as JSON, not string-munged: anything that isn't a flat array of non-blank strings is + * dropped rather than stringified into a plausible-looking pubkey. A pubkey is hex so it can't + * itself contain a comma, but a nested object or a non-array `participants` used to come back + * as garbage entries the caller had no way to tell from real ones. + */ + internal fun participantsOf(content: String): List = + runCatching { + jsonParser + .parseToJsonElement(content) + .jsonObject["participants"] + ?.jsonArray + ?.mapNotNull { (it as? JsonPrimitive)?.contentOrNull?.takeIf(String::isNotBlank) } + }.getOrNull().orEmpty() + + /** + * The `no_relays` detail. Passing `--relays` and having every entry rejected also lands here + * (the flag wins, so the outbox is never consulted), and telling that user to pass the flag + * they just passed is useless — name the real problem instead. + */ + private fun noRelaysMsg(relaysFlag: String?) = + if (relaysFlag == null) { + "no relays: pass --relays ws://…" + } else { + "no usable relays in --relays: expected comma-separated ws:// or wss:// urls, got '$relaysFlag'" + } /** The `--relays` set if given, else the account's outbox relays. */ private suspend fun relaysFor( diff --git a/cli/src/test/kotlin/com/vitorpamplona/amethyst/cli/BuzzAgentWrapperSyncTest.kt b/cli/src/test/kotlin/com/vitorpamplona/amethyst/cli/BuzzAgentWrapperSyncTest.kt new file mode 100644 index 0000000000..44598405db --- /dev/null +++ b/cli/src/test/kotlin/com/vitorpamplona/amethyst/cli/BuzzAgentWrapperSyncTest.kt @@ -0,0 +1,68 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.cli + +import java.io.File +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * The gated-runner wrappers exist twice: `cli/src/main/resources/buzz-agent/` is what + * `amy buzz agent up` extracts into `~/.amy/buzz-agent`, and `tools/buzz-agent/` is the + * readable reference `tools/buzz-agent/README.md` tells people to point `--exec` at. + * + * Nothing in the build copies one to the other, so a fix applied to only one half ships a + * runner that behaves differently from its own documentation. This pins them together. + */ +class BuzzAgentWrapperSyncTest { + @Test + fun bundledWrappersMatchTheToolsReference() { + val bundled = repoRoot.resolve("cli/src/main/resources/buzz-agent") + val reference = repoRoot.resolve("tools/buzz-agent") + + val scripts = + bundled + .listFiles() + ?.filter { it.isFile } + .orEmpty() + .sortedBy { it.name } + assertTrue(scripts.isNotEmpty(), "no bundled wrappers found in $bundled") + + scripts.forEach { script -> + val twin = reference.resolve(script.name) + assertTrue(twin.isFile, "${script.name} has no counterpart in tools/buzz-agent — add one") + assertEquals( + script.readText(), + twin.readText(), + "${script.name} drifted between cli/src/main/resources/buzz-agent and tools/buzz-agent — " + + "apply the change to both copies", + ) + } + } + + /** Walks up from the test's working directory to the checkout root (the dir holding both trees). */ + private val repoRoot: File + get() = + generateSequence(File(".").absoluteFile) { it.parentFile } + .firstOrNull { File(it, "tools/buzz-agent").isDirectory && File(it, "cli/src/main/resources/buzz-agent").isDirectory } + ?: error("could not locate the repo root from ${File(".").absolutePath}") +} diff --git a/cli/src/test/kotlin/com/vitorpamplona/amethyst/cli/BuzzParticipantsTest.kt b/cli/src/test/kotlin/com/vitorpamplona/amethyst/cli/BuzzParticipantsTest.kt new file mode 100644 index 0000000000..59abf37963 --- /dev/null +++ b/cli/src/test/kotlin/com/vitorpamplona/amethyst/cli/BuzzParticipantsTest.kt @@ -0,0 +1,67 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.cli + +import com.vitorpamplona.amethyst.cli.commands.BuzzCommands +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * `buzz dm list` reads `participants` off the raw `dm_created` content. It used to do that by + * stringifying the element and splitting on commas, which turned every non-string shape into + * plausible-looking-but-fake pubkeys. These cases pin the decoded behaviour. + */ +class BuzzParticipantsTest { + private val a = "a".repeat(64) + private val b = "b".repeat(64) + + @Test + fun readsAFlatArrayOfStrings() { + assertEquals(listOf(a, b), BuzzCommands.participantsOf("""{"participants":["$a","$b"]}""")) + } + + @Test + fun emptyWhenAbsentOrEmptyOrUnparseable() { + assertEquals(emptyList(), BuzzCommands.participantsOf("""{"type":"dm_created"}""")) + assertEquals(emptyList(), BuzzCommands.participantsOf("""{"participants":[]}""")) + assertEquals(emptyList(), BuzzCommands.participantsOf("not json at all")) + assertEquals(emptyList(), BuzzCommands.participantsOf("")) + } + + @Test + fun rejectsNonArrayShapesInsteadOfStringifyingThem() { + // The old split-on-comma parse yielded ["nope"] and ["x":1}] respectively. + assertEquals(emptyList(), BuzzCommands.participantsOf("""{"participants":"nope"}""")) + assertEquals(emptyList(), BuzzCommands.participantsOf("""{"participants":{"x":1}}""")) + } + + @Test + fun skipsNonStringElementsButKeepsTheRest() { + assertEquals(listOf(b), BuzzCommands.participantsOf("""{"participants":[{"nested":"$a"},"$b"]}""")) + assertEquals(listOf(a), BuzzCommands.participantsOf("""{"participants":["$a",null,""," "]}""")) + } + + @Test + fun doesNotSplitAStringThatContainsACommaIntoTwoEntries() { + // The regression the old `arr.toString().split(",")` parse produced. + assertEquals(listOf("$a,$b"), BuzzCommands.participantsOf("""{"participants":["$a,$b"]}""")) + } +}