diff --git a/AGENTS.md b/AGENTS.md index 8506a86..2ef6715 100644 --- a/AGENTS.md +++ b/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. diff --git a/README.md b/README.md index 1fe1607..a09ac35 100644 --- a/README.md +++ b/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: # +# 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: # + agentpr --version agentpr --help ``` diff --git a/cmd/agentpr/main.go b/cmd/agentpr/main.go index 34327fd..2bd4703 100644 --- a/cmd/agentpr/main.go +++ b/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", diff --git a/cmd/agentpr/main_test.go b/cmd/agentpr/main_test.go index 444b3b1..749b586 100644 --- a/cmd/agentpr/main_test.go +++ b/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) + } + } +} diff --git a/internal/agent/client_test.go b/internal/agent/client_test.go index 88297e3..696abc7 100644 --- a/internal/agent/client_test.go +++ b/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) + } +} diff --git a/internal/agent/gitea.go b/internal/agent/gitea.go index b88eac8..6cc8d73 100644 --- a/internal/agent/gitea.go +++ b/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"`