mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-07-30 19:46:17 +00:00
Merge pull request #3798 from davotoula/fix/code-review-followups
Address buzz CLI review findings; pin the wrapper copies and the amy arg surface
This commit is contained in:
Vitor Pamplona
@@ -29,6 +29,17 @@ tasks.withType<ProcessResources>().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>("test") {
|
||||
inputs
|
||||
.dir(rootProject.layout.projectDirectory.dir("tools/buzz-agent"))
|
||||
.withPropertyName("buzzAgentWrapperReference")
|
||||
.withPathSensitivity(PathSensitivity.RELATIVE)
|
||||
}
|
||||
|
||||
dependencies {
|
||||
implementation(project(":quartz"))
|
||||
implementation(project(":commons"))
|
||||
|
||||
@@ -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<String> =
|
||||
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(
|
||||
|
||||
@@ -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}")
|
||||
}
|
||||
@@ -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"]}"""))
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
Executable
+108
@@ -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 ]]
|
||||
Reference in New Issue
Block a user