From 42bf2a43d8f20451efb0a22d6f41640c35d413bd Mon Sep 17 00:00:00 2001 From: davotoula Date: Tue, 28 Jul 2026 21:32:28 +0200 Subject: [PATCH 1/2] 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"]}""")) + } +} From 592fd8f542a0c178842a846d1ead65347e7817be Mon Sep 17 00:00:00 2001 From: davotoula Date: Wed, 29 Jul 2026 14:01:22 +0200 Subject: [PATCH 2/2] test: pin the amy argument surface the buzz-agent path depends on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flag names and error strings are the contract between amy and the wrapper scripts (and their operators), but nothing type-checks them: renaming --base-ref or rewording an error breaks callers while compiling clean. This harness pins the ones the buzz-agent path drives. 14 cases, all offline against an isolated ~/.amy via the `amy.home` seam the JVM tests use: the shared `repo-naddr-or-coordinates` positional on git browse/cat/log, `pass --channel GID` on the workflow verbs, --base-ref accepted while --base-reff is rejected on both agent up and agent serve, and the no_relays detail. Confirmed to discriminate: 14/14 on this branch, 10/14 when built against main-upstream's BuzzCommands, with the four failures printing the old misleading "no relays: pass --relays ws://…" for input the user did pass. Worth recording from writing it: a bare word like `garbage` is *accepted* as a relay (the normalizer prepends wss://), so the realistic trigger for the empty set is a pasted http:// url — which is what the cases use. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WopdqNoZ9tYoMNppJG17BL --- tools/buzz-agent/README.md | 13 ++++ tools/buzz-agent/test-amy-args.sh | 108 ++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) create mode 100755 tools/buzz-agent/test-amy-args.sh diff --git a/tools/buzz-agent/README.md b/tools/buzz-agent/README.md index 305c98912a..0379a566bb 100644 --- a/tools/buzz-agent/README.md +++ b/tools/buzz-agent/README.md @@ -112,6 +112,19 @@ empty-worktree failure mode, and `base_branch` resolution when `gh` answers oddl non-zero) — the paths that otherwise only fail on someone else's machine, unattended, via a kind-46007 whose stderr is the only clue. +### `./test-amy-args.sh` — the `amy` argument surface + +```bash +./gradlew :cli:installDist +./tools/buzz-agent/test-amy-args.sh +AMY=/path/to/amy ./tools/buzz-agent/test-amy-args.sh # or point at another build +``` + +Pins the flag names and error strings the wrappers and their operators key on — `--base-ref`, the +`repo-naddr-or-coordinates` positional, `pass --channel GID`, and the `no_relays` detail. A renamed +flag or reworded message breaks callers without breaking any compile, and none of it is covered by a +type. Runs offline against an isolated `~/.amy` (every case is rejected before a relay is dialled). + ### `agent-exec.sh` Verified end-to-end against a throwaway repo with a stubbed `gh` + agent diff --git a/tools/buzz-agent/test-amy-args.sh b/tools/buzz-agent/test-amy-args.sh new file mode 100755 index 0000000000..1e4f1dbdf8 --- /dev/null +++ b/tools/buzz-agent/test-amy-args.sh @@ -0,0 +1,108 @@ +#!/usr/bin/env bash +# +# test-amy-args.sh — pins the argument-validation surface of the `amy` verbs the +# buzz-agent path drives (`buzz agent`, `buzz workflow`, `buzz dm`, `git`). +# +# These are the strings a human or a wrapper script keys on: a flag name silently +# renamed, or a usage/error message reworded, breaks callers without breaking any +# compile. Everything here runs offline against an isolated ~/.amy — the failing +# cases are all rejected before a relay is contacted. +# +# ./gradlew :cli:installDist +# ./tools/buzz-agent/test-amy-args.sh +# AMY=/path/to/amy ./tools/buzz-agent/test-amy-args.sh # or point at another build +# +# Exits 0 when every case passes, 1 otherwise. + +set -uo pipefail + +ROOT="$(cd "$(dirname "$0")/../.." && pwd)" +AMY="${AMY:-$ROOT/cli/build/install/amy/bin/amy}" +if [[ ! -x "$AMY" ]]; then + echo "no amy binary at $AMY — build one with: ./gradlew :cli:installDist" >&2 + echo "(or set AMY=/path/to/amy)" >&2 + exit 2 +fi + +# Isolate the data dir: `amy.home` is the seam the JVM tests use too, so this never +# touches a real ~/.amy. +AMY_HOME="$(mktemp -d)" +trap 'rm -rf "$AMY_HOME"' EXIT +export JAVA_OPTS="-Damy.home=$AMY_HOME" + +pass=0 +fail=0 + +expect() { # name, expected-substring, amy args... + local name="$1" want="$2" + shift 2 + local out + out="$("$AMY" "$@" 2>&1)" + if [[ "$out" == *"$want"* ]]; then + printf ' PASS %s\n' "$name" + pass=$((pass + 1)) + else + printf ' FAIL %s\n want output containing %q\n got: %s\n' \ + "$name" "$want" "$(echo "$out" | head -3 | tr '\n' ' ')" + fail=$((fail + 1)) + fi +} + +reject() { # name, unwanted-substring, amy args... + local name="$1" unwanted="$2" + shift 2 + local out + out="$("$AMY" "$@" 2>&1)" + if [[ "$out" != *"$unwanted"* ]]; then + printf ' PASS %s\n' "$name" + pass=$((pass + 1)) + else + printf ' FAIL %s\n output must NOT contain %q\n got: %s\n' \ + "$name" "$unwanted" "$(echo "$out" | head -3 | tr '\n' ' ')" + fail=$((fail + 1)) + fi +} + +printf '\namy arg-surface harness: %s\n\n' "$AMY" + +"$AMY" --account harness init > /dev/null 2>&1 || { echo "could not create a throwaway account" >&2; exit 1; } +A=(--account harness) +DEAD_RELAY=wss://127.0.0.1:1 # closed port: connects and fails immediately + +# --- the positional name every `git` verb shares --------------------------- +for verb in browse cat log; do + expect "git $verb names its missing positional" "repo-naddr-or-coordinates" git "$verb" +done + +# --- required flags on the workflow verbs ---------------------------------- +expect "workflow trigger demands a channel" "pass --channel GID" \ + "${A[@]}" buzz workflow trigger "$DEAD_RELAY" wf1 --task "do a thing" +expect "workflow list demands a channel" "pass --channel GID" \ + "${A[@]}" buzz workflow list "$DEAD_RELAY" + +# --- the --base-ref flag name ---------------------------------------------- +# `agent up` checks --repo before parsing the rest, so point it at a real repo to +# reach rejectUnknown. +expect "agent up rejects a mistyped --base-ref" "unknown flag: --base-reff" \ + "${A[@]}" buzz agent up "$DEAD_RELAY" --repo "$ROOT" --approver npub1qqq --base-reff x --timeout 1 +reject "agent up accepts --base-ref" "unknown flag" \ + "${A[@]}" buzz agent up "$DEAD_RELAY" --repo "$ROOT" --approver npub1qqq --base-ref x --timeout 1 +expect "agent serve rejects a mistyped --base-ref" "unknown flag: --base-reff" \ + "${A[@]}" buzz agent serve "$DEAD_RELAY" --exec true --base-reff x +reject "agent serve accepts --base-ref" "unknown flag" \ + "${A[@]}" buzz agent serve "$DEAD_RELAY" --exec true --base-ref x --once --timeout 1 + +# --- the no_relays detail --------------------------------------------------- +# Passing --relays wins over the outbox, so an all-unparseable list yields an empty +# set and must NOT tell the user to pass the flag they just passed. Note a bare word +# like "garbage" is *accepted* (the normalizer prepends wss://) — a pasted http url +# is the realistic rejection. +for bad in "http://x" "ws://" "," "!!!"; do + expect "buzz dm list names the real problem for --relays '$bad'" \ + "no usable relays in --relays" "${A[@]}" buzz dm list --relays "$bad" --timeout 2 +done +reject "buzz dm list accepts a well-formed relay" "no_relays" \ + "${A[@]}" buzz dm list --relays "$DEAD_RELAY" --timeout 2 + +printf '\n %d passed, %d failed\n\n' "$pass" "$fail" +[[ "$fail" -eq 0 ]]