diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/GitStatusIndex.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/GitStatusIndex.kt index a19dce396f..67c4935592 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/GitStatusIndex.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/GitStatusIndex.kt @@ -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() 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 diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitApplyCommand.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitApplyCommand.kt index fb8ab81219..99a0f2acc9 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitApplyCommand.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitApplyCommand.kt @@ -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, @@ -101,17 +105,18 @@ object GitApplyCommand { vararg gitArgs: String, ): Pair { 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) } } } diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitCommentCommand.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitCommentCommand.kt index 48ac8de712..79194c946e 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitCommentCommand.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitCommentCommand.kt @@ -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") diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitInitCommand.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitInitCommand.kt index 2b7d9f3664..f0c528b3ae 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitInitCommand.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitInitCommand.kt @@ -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, @@ -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 } diff --git a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitPatchCommands.kt b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitPatchCommands.kt index 8f5a8f5d3a..11ac0dd2aa 100644 --- a/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitPatchCommands.kt +++ b/cli/src/main/kotlin/com/vitorpamplona/amethyst/cli/commands/GitPatchCommands.kt @@ -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() } diff --git a/cli/tests/git/git-nip34-headless.sh b/cli/tests/git/git-nip34-headless.sh index ca72bb154a..e97b04930e 100755 --- a/cli/tests/git/git-nip34-headless.sh +++ b/cli/tests/git/git-nip34-headless.sh @@ -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")" diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/GitPullRequestEvent.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/GitPullRequestEvent.kt index c9d67b6398..2b2696415b 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/GitPullRequestEvent.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/GitPullRequestEvent.kt @@ -87,7 +87,9 @@ class GitPullRequestEvent( fun currentCommit(): String? = tags.firstNotNullOfOrNull(CurrentCommitTag::parse) - fun cloneUrls(): List = 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 = 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) } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/GitPullRequestUpdateEvent.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/GitPullRequestUpdateEvent.kt index 898187ae25..4df780f32a 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/GitPullRequestUpdateEvent.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/GitPullRequestUpdateEvent.kt @@ -81,7 +81,8 @@ class GitPullRequestUpdateEvent( fun currentCommit(): String? = tags.firstNotNullOfOrNull(CurrentCommitTag::parse) - fun cloneUrls(): List = tags.mapNotNull(CloneTag::parse) + // Tolerant of both the multi-value and legacy repeated `clone` forms; deduped. + fun cloneUrls(): List = 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() } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/TagArrayBuilderExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/TagArrayBuilderExt.kt index ca384e8108..cc50024081 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/TagArrayBuilderExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/TagArrayBuilderExt.kt @@ -46,6 +46,9 @@ fun TagArrayBuilder.currentCommit(commit: String) = addUniq fun TagArrayBuilder.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.cloneUrls(urls: List) = addUnique(CloneTag.assemble(urls)) + fun TagArrayBuilder.subject(subject: String) = addUnique(SubjectTag.assemble(subject)) fun TagArrayBuilder.branchName(name: String) = addUnique(BranchNameTag.assemble(name)) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/UpdateTagArrayBuilderExt.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/UpdateTagArrayBuilderExt.kt index db9c1e84e9..4de994479f 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/UpdateTagArrayBuilderExt.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip34Git/pr/UpdateTagArrayBuilderExt.kt @@ -43,6 +43,9 @@ fun TagArrayBuilder.currentCommit(commit: String) = a fun TagArrayBuilder.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.cloneUrls(urls: List) = addUnique(CloneTag.assemble(urls)) + fun TagArrayBuilder.mergeBase(commit: String) = addUnique(MergeBaseTag.assemble(commit)) /** Adds the NIP-22 `E` tag pointing at the parent Pull Request. */ diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip34Git/GitNip34InteropTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip34Git/GitNip34InteropTest.kt index f2fd10155e..0c4249e63e 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip34Git/GitNip34InteropTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip34Git/GitNip34InteropTest.kt @@ -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")))