fix: complete PR clone multi-value, git-status auth spoofing, CLI robustness
Addresses review findings from a merge-time audit. - **Complete the headline multi-value `clone` fix for PRs** (was applied only to kind:30617). GitPullRequestEvent (1618) and GitPullRequestUpdateEvent (1619) carry `clone` with the same spec shape but still emitted repeated single-value tags and read only the first value — so the exact interop bug this branch set out to kill was still live for PRs, both directions (ngit keeps only the last repeated tag; we lost every URL after the first from ngit's multi-value tag). Now both emit one multi-value `["clone", …]` tag and read both forms. Verified on the wire + GitNip34InteropTest + CLI harness (40 checks). - **Android git-status spoofing (GitStatusIndex)**: newest-status-wins with no author check meant anyone could publish a kind-1632 and make someone else's issue render closed. Now filter statuses to the repository owner (from the status's own `a` tag), declared maintainers (from the cached announcement), or the target item's author — matching NIP-34 and the CLI's derivation. Pre-existing on main; this branch made the CLI/Android divergence visible. - **CLI robustness**: `git comment`/`git patch` no longer block forever reading stdin on an interactive TTY (amy is non-interactive — error instead). The local `git` subprocesses in `git init`/`git apply` now drain stdout on a side thread under a bounded `waitFor` + `destroyForcibly`, so a wedged git can't hang the CLI. Left as a follow-up (cosmetic): GitBrowseCommands.candidateUrls duplicates GitRepositoryBrowserViewModel's — worth lifting to shared code, not worth the cross-module coupling here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UKMaNoK5M2PQKCAxhxWzPr
This commit is contained in:
Claude
@@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.model
|
||||
import com.vitorpamplona.amethyst.model.LocalCache.observeEvents
|
||||
import com.vitorpamplona.quartz.nip01Core.core.HexKey
|
||||
import com.vitorpamplona.quartz.nip01Core.relay.filters.Filter
|
||||
import com.vitorpamplona.quartz.nip34Git.repository.GitRepositoryEvent
|
||||
import com.vitorpamplona.quartz.nip34Git.status.GitStatusAppliedEvent
|
||||
import com.vitorpamplona.quartz.nip34Git.status.GitStatusClosedEvent
|
||||
import com.vitorpamplona.quartz.nip34Git.status.GitStatusEvent
|
||||
@@ -71,6 +72,7 @@ object GitStatusIndex {
|
||||
val latest = HashMap<HexKey, GitStatusEvent>()
|
||||
for (event in events) {
|
||||
val target = event.rootEventId() ?: continue
|
||||
if (!isAuthoritative(event, target)) continue
|
||||
val current = latest[target]
|
||||
if (current == null || event.createdAt > current.createdAt) {
|
||||
latest[target] = event
|
||||
@@ -79,6 +81,29 @@ object GitStatusIndex {
|
||||
return latest
|
||||
}
|
||||
|
||||
/**
|
||||
* NIP-34: only the repository owner, a declared maintainer, or the target
|
||||
* item's own author may set its status — "the newest Status from the root
|
||||
* author or a maintainer is valid". Without this filter any pubkey could
|
||||
* publish a kind-1632 and make someone else's issue render closed.
|
||||
*
|
||||
* The owner is read from the status's own `a` tag (so it holds even before
|
||||
* the announcement is cached); the maintainer set needs the cached kind-30617
|
||||
* (falling back to owner/author-only until it arrives — the safe default is to
|
||||
* treat an unverifiable status as absent, i.e. the item stays open).
|
||||
*/
|
||||
private fun isAuthoritative(
|
||||
status: GitStatusEvent,
|
||||
targetId: HexKey,
|
||||
): Boolean {
|
||||
status.repositoryAddress()?.let { repoAddress ->
|
||||
if (status.pubKey == repoAddress.pubKeyHex) return true
|
||||
val repo = LocalCache.getAddressableNoteIfExists(repoAddress)?.event as? GitRepositoryEvent
|
||||
if (repo != null && status.pubKey in repo.maintainers()) return true
|
||||
}
|
||||
return status.pubKey == LocalCache.getNoteIfExists(targetId)?.event?.pubKey
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the latest status for [targetId] marks it as closed (kind 1632)
|
||||
* or applied/resolved/merged (kind 1631). Items with no status event, or
|
||||
|
||||
@@ -26,6 +26,7 @@ import com.vitorpamplona.amethyst.cli.DataDir
|
||||
import com.vitorpamplona.amethyst.cli.Output
|
||||
import com.vitorpamplona.quartz.nip34Git.patch.GitPatchEvent
|
||||
import java.io.File
|
||||
import java.util.concurrent.TimeUnit
|
||||
import kotlin.concurrent.thread
|
||||
|
||||
/**
|
||||
@@ -38,6 +39,9 @@ import kotlin.concurrent.thread
|
||||
* This shells out to `git`, like `git init` — it operates on the local checkout.
|
||||
*/
|
||||
object GitApplyCommand {
|
||||
/** Safety ceiling for the local `git am`/`git apply` invocation so a wedged git can't hang the CLI. */
|
||||
private const val GIT_TIMEOUT_SEC = 60L
|
||||
|
||||
suspend fun apply(
|
||||
dataDir: DataDir,
|
||||
rest: Array<String>,
|
||||
@@ -101,17 +105,18 @@ object GitApplyCommand {
|
||||
vararg gitArgs: String,
|
||||
): Pair<Int, String> {
|
||||
var writer: Thread? = null
|
||||
var reader: Thread? = null
|
||||
return try {
|
||||
val proc =
|
||||
ProcessBuilder(listOf("git", *gitArgs))
|
||||
.directory(repoDir)
|
||||
.redirectErrorStream(true)
|
||||
.start()
|
||||
// Feed stdin on a separate thread while we drain stdout on this one: a
|
||||
// large patch (bigger than the OS pipe buffer) would otherwise deadlock
|
||||
// — git blocks writing output we haven't read, we block writing stdin.
|
||||
// The patch was decoded as UTF-8, so write it back as UTF-8 (not the JVM
|
||||
// default charset, which would corrupt non-ASCII filenames/messages).
|
||||
// Feed stdin AND drain stdout on side threads: a large patch (bigger than
|
||||
// the OS pipe buffer) would otherwise deadlock, and reading on this thread
|
||||
// would block an unbounded wait. The patch was decoded as UTF-8, so write
|
||||
// it back as UTF-8 (not the JVM default charset, which would corrupt
|
||||
// non-ASCII filenames/messages).
|
||||
writer =
|
||||
if (input != null) {
|
||||
thread(name = "git-stdin") { runCatching { proc.outputStream.use { it.write(input.toByteArray(Charsets.UTF_8)) } } }
|
||||
@@ -119,14 +124,22 @@ object GitApplyCommand {
|
||||
proc.outputStream.close()
|
||||
null
|
||||
}
|
||||
val out = proc.inputStream.readBytes().decodeToString()
|
||||
proc.waitFor() to out
|
||||
val sb = StringBuilder()
|
||||
reader = thread(name = "git-out") { runCatching { sb.append(proc.inputStream.readBytes().decodeToString()) } }
|
||||
if (!proc.waitFor(GIT_TIMEOUT_SEC, TimeUnit.SECONDS)) {
|
||||
proc.destroyForcibly()
|
||||
124 to "git timed out after ${GIT_TIMEOUT_SEC}s"
|
||||
} else {
|
||||
reader.join()
|
||||
proc.exitValue() to sb.toString()
|
||||
}
|
||||
} catch (e: Exception) {
|
||||
1 to (e.message ?: "could not run git (is it installed and is this a git repo?)")
|
||||
} finally {
|
||||
// Always join so a non-daemon stdin thread can't outlive the call (e.g.
|
||||
// Always join so the non-daemon side threads can't outlive the call (e.g.
|
||||
// under the in-process test harness, which doesn't exitProcess).
|
||||
writer?.join()
|
||||
writer?.join(1_000)
|
||||
reader?.join(1_000)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -44,7 +44,13 @@ object GitCommentCommand {
|
||||
): Int {
|
||||
val args = Args(rest)
|
||||
val targetRef = args.positional(0, "target-event-or-repo")
|
||||
val body = (args.positionalOrNull(1) ?: System.`in`.readBytes().decodeToString()).trim()
|
||||
val bodyArg = args.positionalOrNull(1)
|
||||
// Never block on an interactive TTY: amy is non-interactive, so require the
|
||||
// body as an argument unless it's actually being piped in.
|
||||
if (bodyArg == null && System.console() != null) {
|
||||
return Output.error("bad_args", "comment body required as an argument (or piped on stdin)")
|
||||
}
|
||||
val body = (bodyArg ?: System.`in`.readBytes().decodeToString()).trim()
|
||||
if (body.isBlank()) return Output.error("bad_args", "empty comment (pass BODY as an argument or on stdin)")
|
||||
args.rejectUnknown("relay")
|
||||
|
||||
|
||||
@@ -29,6 +29,8 @@ import com.vitorpamplona.quartz.nip34Git.repository.GitRepositoryEvent
|
||||
import com.vitorpamplona.quartz.nip34Git.state.GitRepositoryStateEvent
|
||||
import com.vitorpamplona.quartz.nip34Git.state.tags.RefTag
|
||||
import java.io.File
|
||||
import java.util.concurrent.TimeUnit
|
||||
import kotlin.concurrent.thread
|
||||
|
||||
/**
|
||||
* `amy git init` — bootstrap a NIP-34 repository from the local git checkout,
|
||||
@@ -42,6 +44,9 @@ import java.io.File
|
||||
* about the local working tree, exactly like the `ngit`/`nak git` `init`.
|
||||
*/
|
||||
object GitInitCommand {
|
||||
/** Safety ceiling for a single local `git` invocation; a normal read-only op finishes in milliseconds. */
|
||||
private const val GIT_TIMEOUT_SEC = 60L
|
||||
|
||||
suspend fun init(
|
||||
dataDir: DataDir,
|
||||
rest: Array<String>,
|
||||
@@ -171,8 +176,18 @@ object GitInitCommand {
|
||||
.directory(repoDir)
|
||||
.redirectError(ProcessBuilder.Redirect.DISCARD)
|
||||
.start()
|
||||
val out = proc.inputStream.readBytes().decodeToString()
|
||||
if (proc.waitFor() == 0) out.trim().ifEmpty { null } else null
|
||||
// Drain stdout on a side thread and bound the wait: a wedged git (a
|
||||
// hung filter/hook, a credential prompt) must not hang the CLI forever.
|
||||
val sb = StringBuilder()
|
||||
val reader = thread(name = "git-out") { runCatching { sb.append(proc.inputStream.readBytes().decodeToString()) } }
|
||||
if (!proc.waitFor(GIT_TIMEOUT_SEC, TimeUnit.SECONDS)) {
|
||||
proc.destroyForcibly()
|
||||
reader.join(1_000)
|
||||
null
|
||||
} else {
|
||||
reader.join()
|
||||
if (proc.exitValue() == 0) sb.toString().trim().ifEmpty { null } else null
|
||||
}
|
||||
} catch (_: Exception) {
|
||||
null
|
||||
}
|
||||
|
||||
@@ -126,6 +126,8 @@ object GitPatchCommands {
|
||||
File(file).takeIf { it.isFile }?.readText()
|
||||
?: throw IllegalArgumentException("--file not found: $file")
|
||||
} else {
|
||||
// Non-interactive: don't block waiting for a human to type a patch.
|
||||
require(System.console() == null) { "no patch given: pass --file PATH or pipe `git format-patch` to stdin" }
|
||||
System.`in`.readBytes().decodeToString()
|
||||
}.trim()
|
||||
}
|
||||
|
||||
@@ -177,6 +177,11 @@ assert_eq "$(M git show "$IADDR" --relay "$RELAY_URL" | jq -r '.clone | length')
|
||||
IISS="$(M git issue "$IADDR" --subject "interop" "b" --relay "$RELAY_URL" | jq -r '.event_id')"
|
||||
IOWNER="$(echo "$IADDR" | cut -d: -f2)"
|
||||
assert_eq "$(M fetch --id "$IISS" --relay "$RELAY_URL" | jq -r "[.events[0].tags[] | select(.[0]==\"p\" and .[1]==\"$IOWNER\")] | length")" "1" interop.issue_ptag "issue carries the repo owner p tag"
|
||||
# A PR (1618) also carries clone URLs as ONE multi-value tag (same fix as 30617).
|
||||
IPR="$(M git pr "$IADDR" --commit tip1 --clone https://c1.git,https://c2.git --subject s --relay "$RELAY_URL" | jq -r '.event_id')"
|
||||
IPRTAGS="$(M fetch --id "$IPR" --relay "$RELAY_URL")"
|
||||
assert_eq "$(echo "$IPRTAGS" | jq -r '[.events[0].tags[] | select(.[0]=="clone")] | length')" "1" interop.pr_clone_single "PR clone is one tag"
|
||||
assert_eq "$(echo "$IPRTAGS" | jq -r '.events[0].tags[] | select(.[0]=="clone") | length')" "3" interop.pr_clone_multivalue "PR clone tag carries both URLs"
|
||||
|
||||
# GRASP server list (10317) round-trip.
|
||||
GRASP="$(M git grasp set "wss://grasp.example.com,wss://grasp2.example.com" --relay "$RELAY_URL")"
|
||||
|
||||
+4
-2
@@ -87,7 +87,9 @@ class GitPullRequestEvent(
|
||||
|
||||
fun currentCommit(): String? = tags.firstNotNullOfOrNull(CurrentCommitTag::parse)
|
||||
|
||||
fun cloneUrls(): List<String> = tags.mapNotNull(CloneTag::parse)
|
||||
// Tolerant of both the NIP-34 spec form (one multi-value `["clone", a, b]` tag,
|
||||
// which ngit emits) and the legacy repeated form; deduped.
|
||||
fun cloneUrls(): List<String> = tags.flatMap(CloneTag::parseAll).distinct()
|
||||
|
||||
fun subject(): String? = tags.firstNotNullOfOrNull(SubjectTag::parse)
|
||||
|
||||
@@ -138,7 +140,7 @@ class GitPullRequestEvent(
|
||||
pTag(repository.event.pubKey, repository.authorHomeRelay)
|
||||
if (notify.isNotEmpty()) pTags(notify)
|
||||
currentCommit(currentCommit)
|
||||
cloneUrls.forEach { cloneUrl(it) }
|
||||
if (cloneUrls.isNotEmpty()) cloneUrls(cloneUrls)
|
||||
subject?.let { subject(it) }
|
||||
if (labels.isNotEmpty()) hashtags(labels)
|
||||
branchName?.let { branchName(it) }
|
||||
|
||||
+3
-2
@@ -81,7 +81,8 @@ class GitPullRequestUpdateEvent(
|
||||
|
||||
fun currentCommit(): String? = tags.firstNotNullOfOrNull(CurrentCommitTag::parse)
|
||||
|
||||
fun cloneUrls(): List<String> = tags.mapNotNull(CloneTag::parse)
|
||||
// Tolerant of both the multi-value and legacy repeated `clone` forms; deduped.
|
||||
fun cloneUrls(): List<String> = tags.flatMap(CloneTag::parseAll).distinct()
|
||||
|
||||
fun earliestUniqueCommit(): String? = tags.firstOrNull { it.size > 1 && it[0] == "r" && it[1].isNotEmpty() }?.get(1)
|
||||
|
||||
@@ -119,7 +120,7 @@ class GitPullRequestUpdateEvent(
|
||||
pTag(repository.event.pubKey, repository.authorHomeRelay)
|
||||
if (notify.isNotEmpty()) pTags(notify)
|
||||
currentCommit(currentCommit)
|
||||
cloneUrls.forEach { cloneUrl(it) }
|
||||
if (cloneUrls.isNotEmpty()) cloneUrls(cloneUrls)
|
||||
mergeBase?.let { mergeBase(it) }
|
||||
initializer()
|
||||
}
|
||||
|
||||
+3
@@ -46,6 +46,9 @@ fun TagArrayBuilder<GitPullRequestEvent>.currentCommit(commit: String) = addUniq
|
||||
|
||||
fun TagArrayBuilder<GitPullRequestEvent>.cloneUrl(url: String) = add(CloneTag.assemble(url))
|
||||
|
||||
/** Emit all clone URLs as one NIP-34 multi-value `["clone", url1, url2, …]` tag (the spec/ngit form). */
|
||||
fun TagArrayBuilder<GitPullRequestEvent>.cloneUrls(urls: List<String>) = addUnique(CloneTag.assemble(urls))
|
||||
|
||||
fun TagArrayBuilder<GitPullRequestEvent>.subject(subject: String) = addUnique(SubjectTag.assemble(subject))
|
||||
|
||||
fun TagArrayBuilder<GitPullRequestEvent>.branchName(name: String) = addUnique(BranchNameTag.assemble(name))
|
||||
|
||||
+3
@@ -43,6 +43,9 @@ fun TagArrayBuilder<GitPullRequestUpdateEvent>.currentCommit(commit: String) = a
|
||||
|
||||
fun TagArrayBuilder<GitPullRequestUpdateEvent>.cloneUrl(url: String) = add(CloneTag.assemble(url))
|
||||
|
||||
/** Emit all clone URLs as one NIP-34 multi-value `["clone", url1, url2, …]` tag (the spec/ngit form). */
|
||||
fun TagArrayBuilder<GitPullRequestUpdateEvent>.cloneUrls(urls: List<String>) = addUnique(CloneTag.assemble(urls))
|
||||
|
||||
fun TagArrayBuilder<GitPullRequestUpdateEvent>.mergeBase(commit: String) = addUnique(MergeBaseTag.assemble(commit))
|
||||
|
||||
/** Adds the NIP-22 `E` tag pointing at the parent Pull Request. */
|
||||
|
||||
@@ -23,6 +23,7 @@ package com.vitorpamplona.quartz.nip34Git
|
||||
import com.vitorpamplona.quartz.nip01Core.hints.EventHintBundle
|
||||
import com.vitorpamplona.quartz.nip34Git.issue.GitIssueEvent
|
||||
import com.vitorpamplona.quartz.nip34Git.patch.GitPatchEvent
|
||||
import com.vitorpamplona.quartz.nip34Git.pr.GitPullRequestEvent
|
||||
import com.vitorpamplona.quartz.nip34Git.repository.GitRepositoryEvent
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertEquals
|
||||
@@ -102,6 +103,28 @@ class GitNip34InteropTest {
|
||||
assertTrue(owner in pTags, "issue must p-tag the repository owner for maintainer routing")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun pullRequestEmitsAndReadsMultiValueClone() {
|
||||
val repoEvent = repo(arrayOf(arrayOf("d", "x"), arrayOf("r", "root", "euc")))
|
||||
val tmpl =
|
||||
GitPullRequestEvent.build(
|
||||
description = "desc",
|
||||
repository = EventHintBundle(repoEvent),
|
||||
earliestUniqueCommit = "root",
|
||||
currentCommit = "tip",
|
||||
cloneUrls = listOf("https://a.git", "https://b.git"),
|
||||
)
|
||||
// One multi-value clone tag (ngit/spec form), not repeated single-value tags.
|
||||
assertEquals(1, tmpl.tags.count { it[0] == "clone" }, "PR clone must be a single tag")
|
||||
assertEquals(listOf("clone", "https://a.git", "https://b.git"), tmpl.tags.first { it[0] == "clone" }.toList())
|
||||
|
||||
// Reader flattens both the multi-value and the legacy repeated forms.
|
||||
val multi = GitPullRequestEvent("00", owner, 0, arrayOf(arrayOf("clone", "https://a.git", "https://b.git")), "", "00")
|
||||
val repeated = GitPullRequestEvent("00", owner, 0, arrayOf(arrayOf("clone", "https://a.git"), arrayOf("clone", "https://b.git")), "", "00")
|
||||
assertEquals(listOf("https://a.git", "https://b.git"), multi.cloneUrls())
|
||||
assertEquals(listOf("https://a.git", "https://b.git"), repeated.cloneUrls())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun patchRTagIsPlainWithoutEucMarker() {
|
||||
val repoEvent = repo(arrayOf(arrayOf("d", "x"), arrayOf("r", "rootcommit", "euc")))
|
||||
|
||||
Reference in New Issue
Block a user