From 05594113a23681611903ef05b067baedbb54f580 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Wed, 12 Aug 2026 22:17:10 +1000 Subject: [PATCH] Fix non-zero exit on error and debounce transient mergeable=false agentpr/watchpr already propagated command errors to a non-zero exit, but that behaviour had no regression coverage and the root command was not constructible outside main(). watchpr also fired a spurious conflict alert because Gitea computes mergeability asynchronously and can briefly report mergeable=false right after a push. The docs additionally printed the AppRole role_id literal UUID. - Extract newRootCmd() in both cmd/agentpr and cmd/watchpr so main() only runs Execute and exits non-zero on error; add tests asserting Execute returns an error for a bad PR ref / malformed --repo / no args. - Debounce mergeability loss in MeaningfulChange: only alert when mergeable=false persists across two consecutive polls (both prev and cur false, still open); update the table test for one-poll-false (benign), false-persisting (alert), and recovered false->true (benign). - Refer to AGENT_APPROLE_ROLE_ID by env var in README.md/AGENTS.md without printing the literal role_id; keep the code default and env override. --- AGENTS.md | 2 +- README.md | 2 +- cmd/agentpr/main.go | 16 ++++++++++++---- cmd/agentpr/main_test.go | 18 ++++++++++++++++++ cmd/watchpr/main.go | 16 ++++++++++++---- cmd/watchpr/main_test.go | 30 ++++++++++++++++++++++++++++++ internal/agent/watch.go | 8 ++++++-- internal/agent/watch_test.go | 23 ++++++++++++++++++++++- 8 files changed, 102 insertions(+), 13 deletions(-) create mode 100644 cmd/agentpr/main_test.go create mode 100644 cmd/watchpr/main_test.go diff --git a/AGENTS.md b/AGENTS.md index 7abad7e..62437f6 100644 --- a/AGENTS.md +++ b/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 | diff --git a/README.md b/README.md index 0dbc053..897997f 100644 --- a/README.md +++ b/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 | diff --git a/cmd/agentpr/main.go b/cmd/agentpr/main.go index 63548d9..7b12c0b 100644 --- a/cmd/agentpr/main.go +++ b/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. diff --git a/cmd/agentpr/main_test.go b/cmd/agentpr/main_test.go new file mode 100644 index 0000000..899ff3a --- /dev/null +++ b/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") + } +} diff --git a/cmd/watchpr/main.go b/cmd/watchpr/main.go index f9b8cfe..68952f1 100644 --- a/cmd/watchpr/main.go +++ b/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) { diff --git a/cmd/watchpr/main_test.go b/cmd/watchpr/main_test.go new file mode 100644 index 0000000..f587be1 --- /dev/null +++ b/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") + } +} diff --git a/internal/agent/watch.go b/internal/agent/watch.go index f36181c..da320f8 100644 --- a/internal/agent/watch.go +++ b/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, "" diff --git a/internal/agent/watch_test.go b/internal/agent/watch_test.go index 1a3f273..280b732 100644 --- a/internal/agent/watch_test.go +++ b/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)