Merge pull request 'agentpr: close and reopen issues' (#19) from benvin/agentpr-issue-state into main

Reviewed-on: #19

AGENTS.md

@@ -8,10 +8,11 @@ from Vault, so actions are attributed to the agent rather than to whoever runs
 the tool. Setting `AGENT_LOGIN` selects a different agent identity, so a service
 like repospawner can run these tools as itself.
 
-- **`agentpr`** — create and edit pull requests and issues, and post comments on
-  either, as `unkin-agent` (fixes the "tea posts as Ben" attribution problem).
-  Subcommands: `pr create`, `pr comment`, `pr edit`, `issue create`,
-  `issue comment`, `issue edit`, `whoami`.
+- **`agentpr`** — create and edit pull requests and issues, close and reopen
+  issues, and post comments on either, as `unkin-agent` (fixes the "tea posts
+  as Ben" attribution problem). Subcommands: `pr create`, `pr comment`,
+  `pr edit`, `issue create`, `issue comment`, `issue edit`, `issue close`,
+  `issue reopen`, `whoami`.
 - **`watchpr`** — poll one or more PRs and exit when a tracked PR changes
   meaningfully: it merges/closes, gets a new non-agent comment, its CI fails,
   or it loses mergeability. Benign transitions (CI pending→success, the agent's
@@ -29,7 +30,7 @@ parsing, watch-state comparison, git worktree helpers).
 ## Structure
 
 ```
-cmd/agentpr/main.go      # agentpr CLI (pr + issue create/comment/edit, whoami)
+cmd/agentpr/main.go      # agentpr CLI (pr + issue create/comment/edit, issue close/reopen, whoami)
 cmd/watchpr/main.go      # watchpr CLI (poll + meaningful-change exit)
 cmd/agentws/main.go      # agentws CLI (new / list / rm / clean / token / credential)
 cmd/agentws/prune.go     # agentws prune (classify worktrees, remove the safe ones)
@@ -37,7 +38,7 @@ cmd/agentvault/main.go   # agentvault CLI (seed-outpost / seed-oauth)
 internal/agent/          # shared plumbing:
   token.go               #   env config + in-process Gitea-token cache
   vault.go               #   AppRole login + read the gitea creds path
-  gitea.go               #   Gitea REST client (PR/issue create/edit/get, comments, status, whoami)
+  gitea.go               #   Gitea REST client (PR/issue create/edit/get, issue state, comments, status, whoami)
   parse.go               #   owner/repo#N and owner/repo parsing
   watch.go               #   PRState snapshot + MeaningfulChange comparison
   git.go                 #   git worktree/clone/fetch helpers (os/exec, no go-git)
@@ -131,7 +132,8 @@ make test           # go test -v -race ./...
 `internal/agent` covers PR-ref parsing, the `MeaningfulChange` table (benign vs
 alerting transitions), request-body construction, and the Vault+Gitea client
 against `httptest` servers (fake AppRole login + gitea creds + PR/issue create
-+ edit / comment / whoami / status). No live Vault/Gitea access is required for tests.
++ edit / close / reopen / comment / whoami / status). No live Vault/Gitea access
+is required for tests.
 
 ## agentvault seed-outpost
 
@@ -223,3 +225,7 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`.
 - Gitea backs every PR with an issue of the same number and serves comments from
   `/issues/{n}/comments`, so `agentpr pr comment` and `agentpr issue comment`
   are one implementation under two flag names (`--pr` / `--issue`).
+- `issue close`/`issue reopen` read the issue before the PATCH: Gitea answers a
+  no-op state change with 200, so without the read an already-closed issue would
+  report success. There is no `pr close`: closing a pull request is a human's
+  call, not an agent's.

README.md

@@ -6,8 +6,8 @@ token from Vault, so automated PRs, comments and pushes are attributed to the
 agent — not to whoever happens to run the command. Set `AGENT_LOGIN` to act as a
 different agent identity.
 
-- **`agentpr`** — create and edit pull requests and issues, and post comments on
-  either, as the agent user.
+- **`agentpr`** — create and edit pull requests and issues, close and reopen
+  issues, and post comments on either, as the agent user.
 - **`watchpr`** — poll one or more PRs and exit when one changes in a way worth
   acting on.
 - **`agentws`** — manage per-branch git worktrees for `unkin-agent`, cloning
@@ -68,6 +68,12 @@ agentpr issue comment --repo unkin/argocd-apps --issue 43 --body "Fixed in #44."
 agentpr issue edit --repo unkin/argocd-apps --issue 43 --body "The pipeline fails with ..."
 # prints: #<number> <html_url>
 
+# Close or reopen an issue; an issue already in that state is an error, not a
+# silent success
+agentpr issue close --repo unkin/argocd-apps --issue 43
+agentpr issue reopen --repo unkin/argocd-apps --issue 43
+# prints: #<number> <state> <html_url>
+
 agentpr --version
 agentpr --help
 ```

cmd/agentpr/main.go

@@ -9,6 +9,8 @@
 //	agentpr issue create --repo owner/repo --title T --body B
 //	agentpr issue comment --repo owner/repo --issue 12 --body "..."
 //	agentpr issue edit --repo owner/repo --issue 12 --title T --body B
+//	agentpr issue close --repo owner/repo --issue 12
+//	agentpr issue reopen --repo owner/repo --issue 12
 //	agentpr whoami
 package main
 
@@ -69,9 +71,15 @@ func newPRCmd() *cobra.Command {
 func newIssueCmd() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "issue",
-		Short: "File and edit issues, and post issue comments",
+		Short: "File, edit, close and reopen issues, and post issue comments",
 	}
-	cmd.AddCommand(newIssueCreateCmd(), newCommentCmd("issue", "issue", "Post a comment on an issue"), newIssueEditCmd())
+	cmd.AddCommand(
+		newIssueCreateCmd(),
+		newCommentCmd("issue", "issue", "Post a comment on an issue"),
+		newIssueEditCmd(),
+		newIssueStateCmd("close", "Close an issue", agent.IssueStateClosed),
+		newIssueStateCmd("reopen", "Reopen a closed issue", agent.IssueStateOpen),
+	)
 	return cmd
 }
 
@@ -301,6 +309,44 @@ func newIssueEditCmd() *cobra.Command {
 	return cmd
 }
 
+// newIssueStateCmd builds `issue close` and `issue reopen`, which differ only
+// in the state they ask for. An issue already in that state is an error, not a
+// silent success: Gitea answers the PATCH with 200 either way.
+func newIssueStateCmd(use, short, state string) *cobra.Command {
+	var repo string
+	var issue int
+	cmd := &cobra.Command{
+		Use:          use,
+		Short:        short,
+		SilenceUsage: true,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			owner, name, err := agent.ParseRepo(repo)
+			if err != nil {
+				return err
+			}
+			if issue <= 0 {
+				return fmt.Errorf("--issue must be a positive issue number")
+			}
+			c, err := client()
+			if err != nil {
+				return err
+			}
+			updated, err := c.SetIssueState(owner+"/"+name, issue, state)
+			if err != nil {
+				return err
+			}
+			fmt.Printf("#%d %s %s\n", updated.Number, updated.State, updated.HTMLURL)
+			return nil
+		},
+	}
+	f := cmd.Flags()
+	f.StringVar(&repo, "repo", "", "Repository as owner/repo (required)")
+	f.IntVar(&issue, "issue", 0, "Issue number (required)")
+	_ = cmd.MarkFlagRequired("repo")
+	_ = cmd.MarkFlagRequired("issue")
+	return cmd
+}
+
 func newWhoamiCmd() *cobra.Command {
 	return &cobra.Command{
 		Use:          "whoami",

cmd/agentpr/main_test.go

@@ -149,3 +149,56 @@ func TestCommentCommandsStayInStep(t *testing.T) {
 		t.Error("issue comment must take --issue and only --issue")
 	}
 }
+
+// Both state commands need a repo and an issue number; cobra and the RunE
+// guard must reject their absence before anything reaches for a token.
+func TestIssueStateCommandsRequireFlags(t *testing.T) {
+	for _, verb := range []string{"close", "reopen"} {
+		t.Run(verb+" without --issue", func(t *testing.T) {
+			err := execute("issue", verb, "--repo", "unkin/repo")
+			if err == nil {
+				t.Fatal("Execute() = nil, want an error when --issue is missing")
+			}
+			if !strings.Contains(err.Error(), "issue") {
+				t.Errorf("error = %q, want it to name the missing --issue flag", err)
+			}
+		})
+		t.Run(verb+" without --repo", func(t *testing.T) {
+			err := execute("issue", verb, "--issue", "12")
+			if err == nil {
+				t.Fatal("Execute() = nil, want an error when --repo is missing")
+			}
+			if !strings.Contains(err.Error(), "repo") {
+				t.Errorf("error = %q, want it to name the missing --repo flag", err)
+			}
+		})
+		t.Run(verb+" with a zero --issue", func(t *testing.T) {
+			err := execute("issue", verb, "--repo", "unkin/repo", "--issue", "0")
+			if err == nil {
+				t.Fatal("Execute() = nil, want an error for a non-positive issue number")
+			}
+			if !strings.Contains(err.Error(), "--issue must be a positive") {
+				t.Errorf("error = %q, want it to reject the issue number", err)
+			}
+		})
+		t.Run(verb+" with a malformed --repo", func(t *testing.T) {
+			if err := execute("issue", verb, "--repo", "not-a-repo", "--issue", "12"); err == nil {
+				t.Fatal("Execute() = nil, want error for a malformed --repo")
+			}
+		})
+	}
+}
+
+// Closing and reopening are issue-only: a PR is closed by a human, so the pr
+// group must not grow these verbs by accident.
+func TestPRHasNoStateCommands(t *testing.T) {
+	for _, verb := range []string{"close", "reopen"} {
+		if cmd, _, err := newRootCmd().Find([]string{"pr", verb}); err == nil && cmd.Name() == verb {
+			t.Errorf("pr %s exists; closing a PR is not agentpr's to do", verb)
+		}
+		cmd, _, err := newRootCmd().Find([]string{"issue", verb})
+		if err != nil || cmd.Name() != verb {
+			t.Fatalf("issue %s not found: %v", verb, err)
+		}
+	}
+}

internal/agent/client_test.go

@@ -946,3 +946,144 @@ func TestEditIssueSendsOnlySuppliedFields(t *testing.T) {
 		})
 	}
 }
+
+// Closing and reopening differ only in the state sent; both must read the
+// issue first and then PATCH the issue endpoint with that state alone.
+func TestSetIssueStateRequest(t *testing.T) {
+	tests := []struct {
+		name    string
+		current string
+		state   string
+	}{
+		{"close an open issue", IssueStateOpen, IssueStateClosed},
+		{"reopen a closed issue", IssueStateClosed, IssueStateOpen},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var methods []string
+			var gotBody map[string]any
+			var gotPath string
+			mux := http.NewServeMux()
+			mux.HandleFunc("/api/v1/repos/unkin/repo/issues/12", func(w http.ResponseWriter, r *http.Request) {
+				methods = append(methods, r.Method)
+				gotPath = r.URL.Path
+				if r.Method == http.MethodGet {
+					_, _ = io.WriteString(w, `{"number":12,"state":"`+tt.current+`","title":"T","html_url":"https://git.unkin.net/unkin/repo/issues/12"}`)
+					return
+				}
+				_ = json.NewDecoder(r.Body).Decode(&gotBody)
+				_, _ = io.WriteString(w, `{"number":12,"state":"`+tt.state+`","title":"T","html_url":"https://git.unkin.net/unkin/repo/issues/12"}`)
+			})
+			srv := httptest.NewServer(mux)
+			defer srv.Close()
+
+			c := &GiteaClient{BaseURL: srv.URL, Token: "gitea-abc", HTTP: srv.Client()}
+			issue, err := c.SetIssueState("unkin/repo", 12, tt.state)
+			if err != nil {
+				t.Fatalf("SetIssueState: %v", err)
+			}
+			if len(methods) != 2 || methods[0] != http.MethodGet || methods[1] != http.MethodPatch {
+				t.Errorf("requests = %v, want a GET then a PATCH", methods)
+			}
+			if gotPath != "/api/v1/repos/unkin/repo/issues/12" {
+				t.Errorf("path = %q", gotPath)
+			}
+			if len(gotBody) != 1 || gotBody["state"] != tt.state {
+				t.Errorf("payload = %v, want only {\"state\":%q}", gotBody, tt.state)
+			}
+			if issue.State != tt.state || issue.Number != 12 {
+				t.Errorf("parsed issue = %+v", issue)
+			}
+		})
+	}
+}
+
+// Gitea answers a no-op state change with 200, so an issue already in the
+// requested state must fail rather than report a change that never happened —
+// and no PATCH may be sent.
+func TestSetIssueStateAlreadyInState(t *testing.T) {
+	patches := 0
+	mux := http.NewServeMux()
+	mux.HandleFunc("/api/v1/repos/unkin/repo/issues/12", func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodPatch {
+			patches++
+		}
+		_, _ = io.WriteString(w, `{"number":12,"state":"closed","title":"T","html_url":"https://git.unkin.net/unkin/repo/issues/12"}`)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client()}
+	_, err := c.SetIssueState("unkin/repo", 12, IssueStateClosed)
+	if err == nil {
+		t.Fatal("expected an error closing an already-closed issue")
+	}
+	if !errors.Is(err, ErrIssueStateUnchanged) {
+		t.Errorf("errors.Is(%v, ErrIssueStateUnchanged) = false", err)
+	}
+	if !strings.Contains(err.Error(), "unkin/repo#12") {
+		t.Errorf("error %q should name the issue", err)
+	}
+	if patches != 0 {
+		t.Errorf("PATCH requests = %d, want 0", patches)
+	}
+}
+
+// A non-2xx on either leg must surface the API's own message, not a bare
+// status, and must not be mistaken for a successful change.
+func TestSetIssueStateAPIError(t *testing.T) {
+	tests := []struct {
+		name   string
+		failOn string
+		status int
+	}{
+		{"read fails", http.MethodGet, http.StatusNotFound},
+		{"write fails", http.MethodPatch, http.StatusForbidden},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mux := http.NewServeMux()
+			mux.HandleFunc("/api/v1/repos/unkin/repo/issues/12", func(w http.ResponseWriter, r *http.Request) {
+				if r.Method == tt.failOn {
+					w.WriteHeader(tt.status)
+					_, _ = io.WriteString(w, `{"message":"no dice","url":"https://git.unkin.net/api/swagger","errors":null}`)
+					return
+				}
+				_, _ = io.WriteString(w, `{"number":12,"state":"open","title":"T","html_url":"https://git.unkin.net/unkin/repo/issues/12"}`)
+			})
+			srv := httptest.NewServer(mux)
+			defer srv.Close()
+
+			c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client()}
+			_, err := c.SetIssueState("unkin/repo", 12, IssueStateClosed)
+			if err == nil {
+				t.Fatalf("expected an error when %s returns %d", tt.failOn, tt.status)
+			}
+			if !strings.Contains(err.Error(), `"message":"no dice"`) {
+				t.Errorf("error %q should carry the API message", err)
+			}
+		})
+	}
+}
+
+// Only Gitea's two states are accepted, and a bad one is rejected before any
+// request goes out.
+func TestSetIssueStateRejectsUnknownState(t *testing.T) {
+	requests := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requests++
+	}))
+	defer srv.Close()
+
+	c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client()}
+	_, err := c.SetIssueState("unkin/repo", 12, "merged")
+	if err == nil {
+		t.Fatal("expected an error for an unknown state")
+	}
+	if !strings.Contains(err.Error(), `invalid issue state "merged"`) {
+		t.Errorf("error = %q, want it to name the invalid state", err)
+	}
+	if requests != 0 {
+		t.Errorf("requests = %d, want 0", requests)
+	}
+}

internal/agent/gitea.go

@@ -329,6 +329,46 @@ func (c *GiteaClient) EditIssue(repoPath string, number int, opts EditOptions) (
 	return issue, err
 }
 
+// Issue states Gitea accepts on a state change. Gitea has no third state: an
+// issue is open or closed.
+const (
+	IssueStateOpen   = "open"
+	IssueStateClosed = "closed"
+)
+
+// ErrIssueStateUnchanged reports a state change asked for the state the issue
+// is already in. Gitea answers such a PATCH with 200 and changes nothing, so
+// without this check closing an already-closed issue would look like it worked.
+var ErrIssueStateUnchanged = errors.New("issue is already in that state")
+
+// GetIssue fetches a single issue
+// (GET /api/v1/repos/{owner}/{repo}/issues/{index}).
+func (c *GiteaClient) GetIssue(repoPath string, number int) (Issue, error) {
+	var issue Issue
+	err := c.do(http.MethodGet, fmt.Sprintf("/api/v1/repos/%s/issues/%d", repoPath, number), nil, &issue)
+	return issue, err
+}
+
+// SetIssueState closes or reopens an issue. It reads the issue first so an
+// issue already in the requested state fails with ErrIssueStateUnchanged
+// instead of reporting a change that never happened.
+func (c *GiteaClient) SetIssueState(repoPath string, number int, state string) (Issue, error) {
+	if state != IssueStateOpen && state != IssueStateClosed {
+		return Issue{}, fmt.Errorf("invalid issue state %q: want %q or %q", state, IssueStateOpen, IssueStateClosed)
+	}
+	current, err := c.GetIssue(repoPath, number)
+	if err != nil {
+		return Issue{}, err
+	}
+	if current.State == state {
+		return current, fmt.Errorf("%s#%d: %w (%s)", repoPath, number, ErrIssueStateUnchanged, state)
+	}
+	var issue Issue
+	payload := map[string]string{"state": state}
+	err = c.do(http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/issues/%d", repoPath, number), payload, &issue)
+	return issue, err
+}
+
 // Comment is the subset of an issue comment we track.
 type Comment struct {
 	ID   int64  `json:"id"`