Merge pull request 'Fix non-zero exit on error and debounce transient mergeable=false' (#2) from benvin/followup-fixes into main

Reviewed-on: #2

AGENTS.md

@@ -53,7 +53,7 @@ Config via env (all have defaults):
 | Variable | Default | Purpose |
 |---|---|---|
 | `VAULT_ADDR` | `https://vault.service.consul:8200` | Vault/OpenBao address |
-| `AGENT_APPROLE_ROLE_ID` | `ababbcd3-9c77-5c6a-be2d-287fce9214a6` | AppRole role_id |
+| `AGENT_APPROLE_ROLE_ID` | built-in default | AppRole role_id (overridable) |
 | `GITEA_URL` | `https://git.unkin.net` | Gitea base URL |
 | `AGENT_LOGIN` | `unkin-agent` | login whose comments watchpr ignores |
 

README.md

@@ -20,7 +20,7 @@ Everything is configured by environment variables, all with defaults:
 | Variable | Default | Purpose |
 |---|---|---|
 | `VAULT_ADDR` | `https://vault.service.consul:8200` | Vault/OpenBao address |
-| `AGENT_APPROLE_ROLE_ID` | `ababbcd3-9c77-5c6a-be2d-287fce9214a6` | AppRole role_id |
+| `AGENT_APPROLE_ROLE_ID` | built-in default | AppRole role_id (overridable) |
 | `GITEA_URL` | `https://git.unkin.net` | Gitea base URL |
 | `AGENT_LOGIN` | `unkin-agent` | login whose comments `watchpr` ignores |
 

cmd/agentpr/main.go

@@ -20,6 +20,17 @@ import (
 var version = "dev"
 
 func main() {
+	// cobra prints the error itself (SilenceErrors stays off); we only need to
+	// turn any command error into a non-zero exit.
+	if err := newRootCmd().Execute(); err != nil {
+		os.Exit(1)
+	}
+}
+
+// newRootCmd builds the agentpr command tree. It is separated from main so
+// tests can invoke Execute and assert the exit behaviour without spawning a
+// process.
+func newRootCmd() *cobra.Command {
 	root := &cobra.Command{
 		Use:          "agentpr",
 		Short:        "Manage Gitea PRs and comments as the unkin-agent user.",
@@ -30,10 +41,7 @@ func main() {
 	root.SetVersionTemplate("{{.Version}}\n")
 
 	root.AddCommand(newPRCmd(), newWhoamiCmd(), newVersionCmd())
-
-	if err := root.Execute(); err != nil {
-		os.Exit(1)
-	}
+	return root
 }
 
 // client mints a Gitea token via Vault and returns a ready client.

cmd/agentpr/main_test.go

@@ -0,0 +1,18 @@
+package main
+
+import (
+	"io"
+	"testing"
+)
+
+// A malformed --repo must fail the command (so main exits non-zero). ParseRepo
+// rejects it before any Vault/Gitea call, so this stays hermetic.
+func TestExecuteBadRepoErrors(t *testing.T) {
+	cmd := newRootCmd()
+	cmd.SetArgs([]string{"pr", "create", "--repo", "not-a-repo", "--base", "main", "--head", "x", "--title", "t"})
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	if err := cmd.Execute(); err == nil {
+		t.Fatal("Execute() = nil, want error for a malformed --repo")
+	}
+}

cmd/watchpr/main.go

@@ -23,6 +23,17 @@ import (
 var version = "dev"
 
 func main() {
+	// cobra prints the error itself (SilenceErrors stays off); we only need to
+	// turn any command error into a non-zero exit.
+	if err := newRootCmd().Execute(); err != nil {
+		os.Exit(1)
+	}
+}
+
+// newRootCmd builds the watchpr command tree. It is separated from main so
+// tests can invoke Execute and assert the exit behaviour without spawning a
+// process.
+func newRootCmd() *cobra.Command {
 	var interval time.Duration
 	var once, jsonMode bool
 
@@ -70,10 +81,7 @@ func main() {
 		Run:          func(cmd *cobra.Command, args []string) { fmt.Println(version) },
 		SilenceUsage: true,
 	})
-
-	if err := root.Execute(); err != nil {
-		os.Exit(1)
-	}
+	return root
 }
 
 func clientFor() (*agent.GiteaClient, error) {

cmd/watchpr/main_test.go

@@ -0,0 +1,30 @@
+package main
+
+import (
+	"io"
+	"testing"
+)
+
+// A bad PR reference must fail the command (so main exits non-zero) rather than
+// return nil. Parsing rejects the ref before any Vault/Gitea call, so this stays
+// hermetic.
+func TestExecuteBadRefErrors(t *testing.T) {
+	cmd := newRootCmd()
+	cmd.SetArgs([]string{"--once", "not-a-ref"})
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	if err := cmd.Execute(); err == nil {
+		t.Fatal("Execute() = nil, want error for a bad PR reference")
+	}
+}
+
+// No arguments is also an error (nothing to watch).
+func TestExecuteNoArgsErrors(t *testing.T) {
+	cmd := newRootCmd()
+	cmd.SetArgs(nil)
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	if err := cmd.Execute(); err == nil {
+		t.Fatal("Execute() = nil, want error when no PR references are given")
+	}
+}

internal/agent/watch.go

@@ -67,7 +67,7 @@ func isFailedCI(state string) bool {
 //   - the PR closed without merging
 //   - a new comment from someone other than the agent
 //   - CI transitioned into failure/error
-//   - the PR lost mergeability (a conflict appeared)
+//   - the PR lost mergeability (a conflict appeared) for two consecutive polls
 func MeaningfulChange(prev, cur PRState) (bool, string) {
 	if !prev.Merged && cur.Merged {
 		return true, "PR merged"
@@ -82,7 +82,11 @@ func MeaningfulChange(prev, cur PRState) (bool, string) {
 	if isFailedCI(cur.CIStatus) && !isFailedCI(prev.CIStatus) {
 		return true, "CI failed (" + cur.CIStatus + ")"
 	}
-	if prev.Mergeable && !cur.Mergeable && cur.State == "open" {
+	// Gitea computes mergeability asynchronously, so a PR can briefly report
+	// mergeable=false right after a push. Require the loss to persist across two
+	// consecutive polls (both prev and cur false, still open) before treating it
+	// as a real conflict; a single false poll is debounced.
+	if !prev.Mergeable && !cur.Mergeable && cur.State == "open" {
 		return true, "PR lost mergeability (conflict)"
 	}
 	return false, ""

internal/agent/watch_test.go

@@ -17,6 +17,7 @@ func base() PRState {
 func TestMeaningfulChange(t *testing.T) {
 	tests := []struct {
 		name       string
+		mutatePrev func(s *PRState)
 		mutate     func(s *PRState)
 		wantChange bool
 	}{
@@ -56,10 +57,27 @@ func TestMeaningfulChange(t *testing.T) {
 			wantChange: true,
 		},
 		{
-			name:       "lost mergeability alerts",
+			// A single mergeable=false poll is debounced: Gitea often reports
+			// this transiently right after a push.
+			name:       "mergeable true to false for one poll is benign",
+			mutate:     func(s *PRState) { s.Mergeable = false },
+			wantChange: false,
+		},
+		{
+			// mergeable=false persisting into a second consecutive poll is a
+			// real conflict and alerts.
+			name:       "mergeable false persisting a second poll alerts",
+			mutatePrev: func(s *PRState) { s.Mergeable = false },
 			mutate:     func(s *PRState) { s.Mergeable = false },
 			wantChange: true,
 		},
+		{
+			// mergeable recovered (false then true) must not alert.
+			name:       "mergeable recovered false to true is benign",
+			mutatePrev: func(s *PRState) { s.Mergeable = false },
+			mutate:     func(s *PRState) {},
+			wantChange: false,
+		},
 		{
 			name:       "new head sha alone is benign",
 			mutate:     func(s *PRState) { s.HeadSHA = "def456" },
@@ -69,6 +87,9 @@ func TestMeaningfulChange(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			prev := base()
+			if tt.mutatePrev != nil {
+				tt.mutatePrev(&prev)
+			}
 			cur := base()
 			tt.mutate(&cur)
 			got, reason := MeaningfulChange(prev, cur)