diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index c270a6f..2e8a9a6 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -13,10 +13,11 @@ repos: rev: v0.5.1 hooks: - id: go-fmt - - id: go-unit-tests - # go-vet at the module level (dnephin's go-vet runs at repo root, which has no - # .go files here since both tools live under cmd/). The CI pre-commit image + # go vet and go test at the module level (dnephin's run at repo root, which has + # no .go files here since both tools live under cmd/, and its go-unit-tests + # caps every package at 30s and re-runs the whole module once per file batch — + # the git-fixture tests outgrew both). The CI pre-commit image # (almalinux9-gobuilder) has go installed. - repo: local hooks: @@ -26,3 +27,9 @@ repos: language: system types: [go] pass_filenames: false + - id: go-test-mod + name: go test (module) + entry: go test ./... + language: system + types: [go] + pass_filenames: false diff --git a/AGENTS.md b/AGENTS.md index 17c86b7..8506a86 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,9 +8,10 @@ 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 post PR comments, as - `unkin-agent` (fixes the "tea posts as Ben" attribution problem). - Subcommands: `pr create`, `pr comment`, `pr edit`, `whoami`. +- **`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`. - **`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 @@ -28,7 +29,7 @@ parsing, watch-state comparison, git worktree helpers). ## Structure ``` -cmd/agentpr/main.go # agentpr CLI (pr create / pr comment / pr edit / whoami) +cmd/agentpr/main.go # agentpr CLI (pr + issue create/comment/edit, 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) @@ -36,7 +37,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 create/edit/get, comments, status, whoami) + gitea.go # Gitea REST client (PR/issue create/edit/get, 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) @@ -129,8 +130,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 create / -comment / whoami / status). No live Vault/Gitea access is required for tests. +against `httptest` servers (fake AppRole login + gitea creds + PR/issue create ++ edit / comment / whoami / status). No live Vault/Gitea access is required for tests. ## agentvault seed-outpost @@ -219,3 +220,6 @@ wrapped per stage (login / read denied / write denied) via `ErrVaultDenied`. worktree root. - CI "combined status" comes from `/commits/{sha}/status`; an empty head SHA yields an empty state without an API call. +- 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`). diff --git a/README.md b/README.md index 6a80e59..1fe1607 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 post PR comments as the - agent user. +- **`agentpr`** — create and edit pull requests and 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 @@ -56,6 +56,18 @@ agentpr pr edit --repo unkin/argocd-apps --pr 42 --body "Adds the ServiceAccount agentpr pr edit --repo unkin/argocd-apps --pr 42 --title "Add woodpecker SA" # prints: # +# File an issue (--body optional) +agentpr issue create --repo unkin/argocd-apps \ + --title "Woodpecker SA missing" --body "The pipeline fails with ..." +# prints: # + +# Comment on an issue (the same Gitea endpoint `pr comment` posts to) +agentpr issue comment --repo unkin/argocd-apps --issue 43 --body "Fixed in #44." + +# Edit an issue's title and/or body; an omitted flag is left unchanged +agentpr issue edit --repo unkin/argocd-apps --issue 43 --body "The pipeline fails with ..." +# prints: # + agentpr --version agentpr --help ``` @@ -116,10 +128,10 @@ checkout too; the worktrees themselves live under the **worktree root** ```bash # Clone unkin/argocd-apps into ~/src/prodenv if missing, then add a worktree for -# a new branch off the remote default branch. Prints the worktree path. +# the branch. Prints the worktree path. agentws new argocd-apps --branch benvin/my-change -# Branch off a specific base instead of the remote default +# Branch off a specific base instead of the remote default (new branches only) agentws new argocd-apps --branch benvin/hotfix --from release-1.2 # List managed worktrees (repo, branch, path) @@ -143,6 +155,20 @@ agentws clean agentws token ``` +`agentws new` fetches first, then takes one of two paths and names the one it +took on its last output line. A branch that **already exists on origin** is +checked out at `origin/` and set to track it, so the worktree starts on +the branch's own commits (`branch (tracking origin/ at )`); a local +branch left from an earlier run is fast-forwarded onto it. A branch origin does +**not** have is created from `--from`, or from the remote's default branch when +`--from` is absent (`branch (new, from origin/)`) — the default is read +from `origin/HEAD`, so a repo on `master` forks from `master`. `--from` is +ignored, with a note, when the branch is already on origin. + +The one case the worktree does not land on `origin/` is a local branch +carrying commits origin has never seen. Those commits exist nowhere else, so the +checkout is left on them and the output says how many. + ### prune `agentws prune` finds worktrees two ways and merges the results: the managed diff --git a/cmd/agentpr/main.go b/cmd/agentpr/main.go index f5faf40..34327fd 100644 --- a/cmd/agentpr/main.go +++ b/cmd/agentpr/main.go @@ -6,6 +6,9 @@ // agentpr pr create --repo owner/repo --base main --head feature --title T --body B // agentpr pr comment --repo owner/repo --pr 12 --body "..." // agentpr pr edit --repo owner/repo --pr 12 --title T --body B +// 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 whoami package main @@ -34,14 +37,14 @@ func main() { func newRootCmd() *cobra.Command { root := &cobra.Command{ Use: "agentpr", - Short: "Manage Gitea PRs and comments as an agent user.", - Long: "agentpr manages Gitea pull requests and comments as an agent user, using a\nGitea token minted from Vault (AppRole login + gitea/creds/).\nSet AGENT_LOGIN to act as another agent identity, or GITEA_CREDS_PATH to name\nthe Vault creds path outright.", + Short: "Manage Gitea PRs, issues and comments as an agent user.", + Long: "agentpr manages Gitea pull requests, issues and comments as an agent user,\nusing a Gitea token minted from Vault (AppRole login + gitea/creds/).\nSet AGENT_LOGIN to act as another agent identity, or GITEA_CREDS_PATH to name\nthe Vault creds path outright.", Version: version, SilenceUsage: true, } root.SetVersionTemplate("{{.Version}}\n") - root.AddCommand(newPRCmd(), newWhoamiCmd(), newVersionCmd()) + root.AddCommand(newPRCmd(), newIssueCmd(), newWhoamiCmd(), newVersionCmd()) return root } @@ -59,7 +62,16 @@ func newPRCmd() *cobra.Command { Use: "pr", Short: "Create and edit PRs, and post PR comments", } - cmd.AddCommand(newPRCreateCmd(), newPRCommentCmd(), newPREditCmd()) + cmd.AddCommand(newPRCreateCmd(), newCommentCmd("pr", "PR", "Post a comment on a pull request"), newPREditCmd()) + return cmd +} + +func newIssueCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "issue", + Short: "File and edit issues, and post issue comments", + } + cmd.AddCommand(newIssueCreateCmd(), newCommentCmd("issue", "issue", "Post a comment on an issue"), newIssueEditCmd()) return cmd } @@ -104,20 +116,24 @@ func newPRCreateCmd() *cobra.Command { return cmd } -func newPRCommentCmd() *cobra.Command { +// newCommentCmd builds a comment command whose number flag is named numFlag. +// Gitea backs every PR with an issue of the same number and serves comments +// from the issue endpoint, so `pr comment` and `issue comment` are one command +// under two flag names rather than two implementations that could drift. +func newCommentCmd(numFlag, noun, short string) *cobra.Command { var repo, body string - var pr int + var number int cmd := &cobra.Command{ Use: "comment", - Short: "Post a comment on a pull request", + Short: short, SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { owner, name, err := agent.ParseRepo(repo) if err != nil { return err } - if pr <= 0 { - return fmt.Errorf("--pr must be a positive PR number") + if number <= 0 { + return fmt.Errorf("--%s must be a positive %s number", numFlag, noun) } if body == "" { return fmt.Errorf("--body is required") @@ -126,20 +142,20 @@ func newPRCommentCmd() *cobra.Command { if err != nil { return err } - cm, err := c.CreateComment(owner+"/"+name, pr, body) + cm, err := c.CreateComment(owner+"/"+name, number, body) if err != nil { return err } - fmt.Printf("comment %d posted on %s/%s#%d\n", cm.ID, owner, name, pr) + fmt.Printf("comment %d posted on %s/%s#%d\n", cm.ID, owner, name, number) return nil }, } f := cmd.Flags() f.StringVar(&repo, "repo", "", "Repository as owner/repo (required)") - f.IntVar(&pr, "pr", 0, "PR number (required)") + f.IntVar(&number, numFlag, 0, noun+" number (required)") f.StringVar(&body, "body", "", "Comment body (required)") _ = cmd.MarkFlagRequired("repo") - _ = cmd.MarkFlagRequired("pr") + _ = cmd.MarkFlagRequired(numFlag) _ = cmd.MarkFlagRequired("body") return cmd } @@ -159,22 +175,9 @@ func newPREditCmd() *cobra.Command { if pr <= 0 { return fmt.Errorf("--pr must be a positive PR number") } - // Only the flags actually given are sent: omitting --title must - // leave the title as it is, not blank it. - var opts agent.EditPROptions - if cmd.Flags().Changed("title") { - // Gitea ignores an empty title, so sending one would report - // success while changing nothing. - if title == "" { - return fmt.Errorf("--title cannot be empty: a title can be set but not cleared") - } - opts.Title = &title - } - if cmd.Flags().Changed("body") { - opts.Body = &body - } - if opts.Title == nil && opts.Body == nil { - return fmt.Errorf("at least one of --title or --body is required") + opts, err := editOptions(cmd, title, body) + if err != nil { + return err } c, err := client() if err != nil { @@ -198,6 +201,106 @@ func newPREditCmd() *cobra.Command { return cmd } +// editOptions turns the --title/--body flags actually given into an edit +// payload. Only the flags present are sent: omitting --title must leave the +// title as it is, not blank it. +func editOptions(cmd *cobra.Command, title, body string) (agent.EditOptions, error) { + var opts agent.EditOptions + if cmd.Flags().Changed("title") { + // Gitea ignores an empty title, so sending one would report success + // while changing nothing. + if title == "" { + return opts, fmt.Errorf("--title cannot be empty: a title can be set but not cleared") + } + opts.Title = &title + } + if cmd.Flags().Changed("body") { + opts.Body = &body + } + if opts.Title == nil && opts.Body == nil { + return opts, fmt.Errorf("at least one of --title or --body is required") + } + return opts, nil +} + +func newIssueCreateCmd() *cobra.Command { + var repo, title, body string + cmd := &cobra.Command{ + Use: "create", + Short: "File an issue", + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { + owner, name, err := agent.ParseRepo(repo) + if err != nil { + return err + } + if title == "" { + return fmt.Errorf("--title is required") + } + c, err := client() + if err != nil { + return err + } + issue, err := c.CreateIssue(owner+"/"+name, agent.CreateIssueOptions{ + Title: title, + Body: body, + }) + if err != nil { + return err + } + fmt.Printf("#%d %s\n", issue.Number, issue.HTMLURL) + return nil + }, + } + f := cmd.Flags() + f.StringVar(&repo, "repo", "", "Repository as owner/repo (required)") + f.StringVar(&title, "title", "", "Issue title (required)") + f.StringVar(&body, "body", "", "Issue body") + _ = cmd.MarkFlagRequired("repo") + return cmd +} + +func newIssueEditCmd() *cobra.Command { + var repo, title, body string + var issue int + cmd := &cobra.Command{ + Use: "edit", + Short: "Edit an issue's title and/or body", + 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") + } + opts, err := editOptions(cmd, title, body) + if err != nil { + return err + } + c, err := client() + if err != nil { + return err + } + updated, err := c.EditIssue(owner+"/"+name, issue, opts) + if err != nil { + return err + } + fmt.Printf("#%d %s\n", updated.Number, updated.HTMLURL) + return nil + }, + } + f := cmd.Flags() + f.StringVar(&repo, "repo", "", "Repository as owner/repo (required)") + f.IntVar(&issue, "issue", 0, "Issue number (required)") + f.StringVar(&title, "title", "", "New issue title (unchanged when omitted)") + f.StringVar(&body, "body", "", "New issue body (unchanged when omitted)") + _ = 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 b3faecf..444b3b1 100644 --- a/cmd/agentpr/main_test.go +++ b/cmd/agentpr/main_test.go @@ -4,6 +4,8 @@ import ( "io" "strings" "testing" + + "github.com/spf13/cobra" ) // A malformed --repo must fail the command (so main exits non-zero). ParseRepo @@ -49,3 +51,101 @@ func TestPREditRejectsEmptyTitle(t *testing.T) { t.Errorf("error = %q, want it to reject the empty title", err) } } + +// execute runs the command tree with args, discarding output, so tests assert +// on the error alone. Every case here fails before any Vault/Gitea call. +func execute(args ...string) error { + cmd := newRootCmd() + cmd.SetArgs(args) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + return cmd.Execute() +} + +// An issue needs a title; Gitea rejects an empty one, so the command must too. +func TestIssueCreateRequiresTitle(t *testing.T) { + err := execute("issue", "create", "--repo", "unkin/repo", "--body", "b") + if err == nil { + t.Fatal("Execute() = nil, want an error when --title is missing") + } + if !strings.Contains(err.Error(), "--title is required") { + t.Errorf("error = %q, want it to name the missing flag", err) + } +} + +// --repo is required, and cobra must reject its absence before anything reaches +// for a token. +func TestIssueCreateRequiresRepo(t *testing.T) { + err := execute("issue", "create", "--title", "t") + 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 flag", err) + } +} + +func TestIssueCreateBadRepoErrors(t *testing.T) { + if err := execute("issue", "create", "--repo", "not-a-repo", "--title", "t"); err == nil { + t.Fatal("Execute() = nil, want error for a malformed --repo") + } +} + +// `issue comment` addresses the issue by --issue, not --pr, and needs it. +func TestIssueCommentRequiresIssueNumber(t *testing.T) { + err := execute("issue", "comment", "--repo", "unkin/repo", "--body", "hi") + 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) + } +} + +func TestIssueEditRequiresTitleOrBody(t *testing.T) { + err := execute("issue", "edit", "--repo", "unkin/repo", "--issue", "12") + if err == nil { + t.Fatal("Execute() = nil, want an error when neither --title nor --body is given") + } + if !strings.Contains(err.Error(), "--title or --body") { + t.Errorf("error = %q, want it to name the missing flags", err) + } +} + +func TestIssueEditRejectsEmptyTitle(t *testing.T) { + err := execute("issue", "edit", "--repo", "unkin/repo", "--issue", "12", "--title", "") + if err == nil { + t.Fatal("Execute() = nil, want an error for an empty --title") + } + if !strings.Contains(err.Error(), "--title cannot be empty") { + t.Errorf("error = %q, want it to reject the empty title", err) + } +} + +// PRs and issues share Gitea's comment endpoint, so both comment commands are +// built from one constructor: they must stay identical apart from the flag +// naming the number. +func TestCommentCommandsStayInStep(t *testing.T) { + find := func(group string) *cobra.Command { + t.Helper() + cmd, _, err := newRootCmd().Find([]string{group, "comment"}) + if err != nil || cmd.Name() != "comment" { + t.Fatalf("%s comment not found: %v", group, err) + } + return cmd + } + has := func(cmd *cobra.Command, name string) bool { return cmd.Flags().Lookup(name) != nil } + + prCmd, issueCmd := find("pr"), find("issue") + for _, name := range []string{"repo", "body"} { + if !has(prCmd, name) || !has(issueCmd, name) { + t.Errorf("--%s missing: pr=%t issue=%t", name, has(prCmd, name), has(issueCmd, name)) + } + } + if !has(prCmd, "pr") || has(prCmd, "issue") { + t.Error("pr comment must take --pr and only --pr") + } + if !has(issueCmd, "issue") || has(issueCmd, "pr") { + t.Error("issue comment must take --issue and only --issue") + } +} diff --git a/cmd/agentws/main.go b/cmd/agentws/main.go index cbfeead..f15bd5f 100644 --- a/cmd/agentws/main.go +++ b/cmd/agentws/main.go @@ -161,13 +161,24 @@ func newNewCmd() *cobra.Command { return err } - // c. Base branch: --from or the remote default. + // c. A branch origin already has is work in progress and must be + // checked out as it stands; only a branch origin does not have is + // forked from a base. + onRemote := agent.GitRemoteBranchExists(srcDir, "origin", branch) + startPoint := "origin/" + branch base := from - if base == "" { - base, err = agent.GitRemoteDefaultBranch(srcDir, "origin") - if err != nil { - return err + if onRemote { + if from != "" { + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "note: --from %s ignored, origin/%s already exists\n", from, branch) } + } else { + if base == "" { + base, err = agent.GitRemoteDefaultBranch(srcDir, "origin") + if err != nil { + return err + } + } + startPoint = "origin/" + base } // d. Create the worktree FROM the source checkout so the branch is @@ -176,7 +187,7 @@ func newNewCmd() *cobra.Command { if _, statErr := os.Stat(wtPath); statErr == nil { return fmt.Errorf("worktree already exists at %s", wtPath) } - if err := agent.GitWorktreeAdd(srcDir, wtPath, branch, "origin/"+base); err != nil { + if err := agent.GitWorktreeAdd(srcDir, wtPath, branch, startPoint); err != nil { return err } @@ -199,9 +210,19 @@ func newNewCmd() *cobra.Command { return err } - // f. Report the worktree path and branch. + // f. A local branch left from an earlier run may sit behind origin, + // so reusing it is not enough on its own. + summary := fmt.Sprintf("branch %s (new, from origin/%s)", branch, base) + if onRemote { + summary, err = alignToRemote(cmd.OutOrStdout(), wtPath, branch) + if err != nil { + return err + } + } + + // g. Report the worktree path and which of the two paths was taken. _, _ = fmt.Fprintf(cmd.OutOrStdout(), "%s\n", wtPath) - _, _ = fmt.Fprintf(cmd.OutOrStdout(), "branch %s (from origin/%s)\n", branch, base) + _, _ = fmt.Fprintf(cmd.OutOrStdout(), "%s\n", summary) return nil }, } @@ -212,6 +233,47 @@ func newNewCmd() *cobra.Command { return cmd } +// alignToRemote puts the worktree on origin/ and reports what that took. +// A reused local branch can be stale, and a fast-forward is the only move that +// adds no commit and drops none; a local branch carrying commits origin does not +// have is left where it stands, because those commits exist nowhere else. +func alignToRemote(out io.Writer, wtPath, branch string) (string, error) { + remoteRef := "origin/" + branch + want, err := agent.GitRevParse(wtPath, remoteRef) + if err != nil { + return "", err + } + head, err := agent.GitRevParse(wtPath, "HEAD") + if err != nil { + return "", err + } + if head != want { + ahead, err := agent.GitAheadCount(wtPath, remoteRef, "HEAD") + if err != nil { + return "", err + } + if ahead > 0 { + return fmt.Sprintf("branch %s (local, %s not on %s, left at %s)", + branch, commitCount(ahead), remoteRef, shortSHA(head)), nil + } + if err := agent.GitMergeFFOnly(wtPath, remoteRef); err != nil { + return "", err + } + _, _ = fmt.Fprintf(out, "fast-forwarded stale %s to %s\n", branch, remoteRef) + } + if err := agent.GitSetUpstream(wtPath, branch, remoteRef); err != nil { + return "", err + } + return fmt.Sprintf("branch %s (tracking %s at %s)", branch, remoteRef, shortSHA(want)), nil +} + +func shortSHA(sha string) string { + if len(sha) > 7 { + return sha[:7] + } + return sha +} + // --- list ----------------------------------------------------------------- func newListCmd() *cobra.Command { diff --git a/cmd/agentws/new_test.go b/cmd/agentws/new_test.go new file mode 100644 index 0000000..440d078 --- /dev/null +++ b/cmd/agentws/new_test.go @@ -0,0 +1,192 @@ +package main + +import ( + "bytes" + "path/filepath" + "strings" + "testing" + + "git.unkin.net/unkin/agent-tools/internal/agent" +) + +// newFixtureOn builds the same origin/source/worktree-root layout as the prune +// fixture but with a chosen default branch, so `new` can be tested against a +// repo whose default is not "main". +func newFixtureOn(t *testing.T, defBranch string) *fixture { + t.Helper() + root := t.TempDir() + f := &fixture{ + root: root, + bare: filepath.Join(root, "origin.git"), + srcDir: filepath.Join(root, "src", "repo"), + wtRoot: filepath.Join(root, "worktrees"), + } + git(t, root, "init", "--bare", "-b", defBranch, f.bare) + + seed := filepath.Join(root, "seed") + git(t, root, "init", "-b", defBranch, seed) + identity(t, seed) + writeCommit(t, seed, "README.md", "hi\n", "init") + git(t, seed, "remote", "add", "origin", f.bare) + git(t, seed, "push", "-u", "origin", defBranch) + + git(t, root, "clone", f.bare, f.srcDir) + identity(t, f.srcDir) + + t.Setenv("AGENTWS_ROOT", f.wtRoot) + t.Setenv("AGENTWS_SRC_ROOT", filepath.Join(root, "src")) + t.Setenv("AGENTWS_OWNER", "unkin") + return f +} + +// runNew invokes `agentws new repo --branch ` and returns its output. +func runNew(t *testing.T, branch string, extra ...string) string { + t.Helper() + var out bytes.Buffer + cmd := newRootCmd() + cmd.SetArgs(append([]string{"new", "repo", "--branch", branch}, extra...)) + cmd.SetOut(&out) + cmd.SetErr(&out) + if err := cmd.Execute(); err != nil { + t.Fatalf("new %s: %v (output %q)", branch, err, out.String()) + } + return out.String() +} + +// pushBranch creates branch on origin carrying one commit and returns its SHA. +func pushBranch(t *testing.T, f *fixture, branch, file, content string) string { + t.Helper() + seed := filepath.Join(f.root, "seed") + git(t, seed, "checkout", "-b", branch) + writeCommit(t, seed, file, content, "work on "+branch) + git(t, seed, "push", "origin", branch) + return git(t, seed, "rev-parse", "HEAD") +} + +func wtPathFor(f *fixture, branch string) string { + return filepath.Join(f.wtRoot, agent.WorktreeDirName("repo", branch)) +} + +// The regression: a branch that already exists on origin must be checked out at +// origin's tip, not forked from the default branch. +func TestNewChecksOutExistingRemoteBranch(t *testing.T) { + f := newFixtureOn(t, "main") + want := pushBranch(t, f, "benvin/existing", "a.txt", "a\n") + + out := runNew(t, "benvin/existing") + + path := wtPathFor(f, "benvin/existing") + if got := git(t, path, "rev-parse", "HEAD"); got != want { + t.Errorf("worktree HEAD = %s, want origin/benvin/existing %s", got, want) + } + if upstream := git(t, path, "rev-parse", "--abbrev-ref", "HEAD@{upstream}"); upstream != "origin/benvin/existing" { + t.Errorf("upstream = %q, want origin/benvin/existing", upstream) + } + if !contains(out, "tracking origin/benvin/existing") { + t.Errorf("output %q does not say the remote branch was checked out", out) + } +} + +// A branch origin does not have is still forked from the default branch, and the +// output must say so rather than leaving the caller to guess. +func TestNewForksBranchMissingFromRemote(t *testing.T) { + f := newFixtureOn(t, "main") + want := git(t, f.srcDir, "rev-parse", "origin/main") + + out := runNew(t, "benvin/fresh") + + path := wtPathFor(f, "benvin/fresh") + if got := git(t, path, "rev-parse", "HEAD"); got != want { + t.Errorf("worktree HEAD = %s, want origin/main %s", got, want) + } + if !contains(out, "branch benvin/fresh (new, from origin/main)") { + t.Errorf("output %q does not report a new branch", out) + } +} + +// The base is the remote's own default branch, so a repo defaulting to master +// forks from master. +func TestNewForksFromMasterDefaultBranch(t *testing.T) { + f := newFixtureOn(t, "master") + want := git(t, f.srcDir, "rev-parse", "origin/master") + + out := runNew(t, "benvin/on-master") + + path := wtPathFor(f, "benvin/on-master") + if got := git(t, path, "rev-parse", "HEAD"); got != want { + t.Errorf("worktree HEAD = %s, want origin/master %s", got, want) + } + if !contains(out, "from origin/master") { + t.Errorf("output %q does not name origin/master as the base", out) + } +} + +// An existing remote branch beats --from: the flag is ignored and the caller is +// told, rather than the branch being silently re-forked. +func TestNewIgnoresFromWhenBranchIsOnRemote(t *testing.T) { + f := newFixtureOn(t, "main") + want := pushBranch(t, f, "benvin/with-from", "a.txt", "a\n") + + out := runNew(t, "benvin/with-from", "--from", "main") + + path := wtPathFor(f, "benvin/with-from") + if got := git(t, path, "rev-parse", "HEAD"); got != want { + t.Errorf("worktree HEAD = %s, want origin/benvin/with-from %s", got, want) + } + if !contains(out, "--from main ignored") { + t.Errorf("output %q does not report the ignored --from", out) + } +} + +// A local branch left behind by an earlier run must not pin the worktree to a +// commit origin has moved past. +func TestNewFastForwardsStaleLocalBranch(t *testing.T) { + f := newFixtureOn(t, "main") + stale := pushBranch(t, f, "benvin/stale", "a.txt", "a\n") + git(t, f.srcDir, "fetch", "origin") + git(t, f.srcDir, "branch", "benvin/stale", "origin/benvin/stale") + + seed := filepath.Join(f.root, "seed") + writeCommit(t, seed, "b.txt", "b\n", "more work") + git(t, seed, "push", "origin", "benvin/stale") + want := git(t, seed, "rev-parse", "HEAD") + if want == stale { + t.Fatal("fixture did not move origin/benvin/stale on") + } + + out := runNew(t, "benvin/stale") + + path := wtPathFor(f, "benvin/stale") + if got := git(t, path, "rev-parse", "HEAD"); got != want { + t.Errorf("worktree HEAD = %s, want origin/benvin/stale %s", got, want) + } + if !contains(out, "fast-forwarded stale benvin/stale") { + t.Errorf("output %q does not report the fast-forward", out) + } +} + +// A local branch carrying commits origin does not have keeps them: they exist +// nowhere else, so the worktree stays put and the output says so. +func TestNewKeepsLocalCommitsAheadOfRemote(t *testing.T) { + f := newFixtureOn(t, "main") + pushBranch(t, f, "benvin/ahead", "a.txt", "a\n") + git(t, f.srcDir, "fetch", "origin") + git(t, f.srcDir, "checkout", "-b", "benvin/ahead", "origin/benvin/ahead") + writeCommit(t, f.srcDir, "local.txt", "local\n", "local only") + want := git(t, f.srcDir, "rev-parse", "HEAD") + git(t, f.srcDir, "checkout", "main") + + out := runNew(t, "benvin/ahead") + + path := wtPathFor(f, "benvin/ahead") + if got := git(t, path, "rev-parse", "HEAD"); got != want { + t.Errorf("worktree HEAD = %s, want the local tip %s", got, want) + } + if !contains(out, "1 commit not on origin/benvin/ahead") { + t.Errorf("output %q does not report the unpushed commit", out) + } +} + +func contains(haystack, needle string) bool { + return strings.Contains(haystack, needle) +} diff --git a/internal/agent/client_test.go b/internal/agent/client_test.go index 182541f..88297e3 100644 --- a/internal/agent/client_test.go +++ b/internal/agent/client_test.go @@ -105,13 +105,13 @@ func TestEditPRSendsOnlySuppliedFields(t *testing.T) { title, body, empty := "new title", "new body", "" tests := []struct { name string - opts EditPROptions + opts EditOptions want map[string]any }{ - {"body only", EditPROptions{Body: &body}, map[string]any{"body": "new body"}}, - {"title only", EditPROptions{Title: &title}, map[string]any{"title": "new title"}}, - {"both", EditPROptions{Title: &title, Body: &body}, map[string]any{"title": "new title", "body": "new body"}}, - {"explicit empty body is sent", EditPROptions{Body: &empty}, map[string]any{"body": ""}}, + {"body only", EditOptions{Body: &body}, map[string]any{"body": "new body"}}, + {"title only", EditOptions{Title: &title}, map[string]any{"title": "new title"}}, + {"both", EditOptions{Title: &title, Body: &body}, map[string]any{"title": "new title", "body": "new body"}}, + {"explicit empty body is sent", EditOptions{Body: &empty}, map[string]any{"body": ""}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -164,7 +164,7 @@ func TestEditPRAPIError(t *testing.T) { title := "new title" c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client()} - _, err := c.EditPR("unkin/repo", 7, EditPROptions{Title: &title}) + _, err := c.EditPR("unkin/repo", 7, EditOptions{Title: &title}) if err == nil { t.Fatal("expected error on 404") } @@ -810,3 +810,139 @@ func TestWatchAbortsWhenTokenExpiresMidWatch(t *testing.T) { t.Errorf("auth failure logged as a warning %d time(s); it must abort", warned) } } + +func TestCreateIssueRequestBody(t *testing.T) { + var gotPath, gotMethod, gotAuth string + var gotBody CreateIssueOptions + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/issues", func(w http.ResponseWriter, r *http.Request) { + gotPath, gotMethod = r.URL.Path, r.Method + gotAuth = r.Header.Get("Authorization") + _ = json.NewDecoder(r.Body).Decode(&gotBody) + _, _ = 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: "gitea-abc", HTTP: srv.Client()} + issue, err := c.CreateIssue("unkin/repo", CreateIssueOptions{Title: "T", Body: "B"}) + if err != nil { + t.Fatalf("CreateIssue: %v", err) + } + if gotMethod != http.MethodPost || gotPath != "/api/v1/repos/unkin/repo/issues" { + t.Errorf("request = %s %s, want POST /api/v1/repos/unkin/repo/issues", gotMethod, gotPath) + } + if gotAuth != "token gitea-abc" { + t.Errorf("auth header = %q, want 'token gitea-abc'", gotAuth) + } + if gotBody.Title != "T" || gotBody.Body != "B" { + t.Errorf("request body = %+v", gotBody) + } + if issue.Number != 12 || issue.HTMLURL != "https://git.unkin.net/unkin/repo/issues/12" { + t.Errorf("parsed issue = %+v", issue) + } +} + +// Filing against a repo that does not exist (or that the token may not see) +// gets Gitea's 404, which must surface as a not-found error carrying the API's +// own message rather than a bare status. +func TestCreateIssueRepoNotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = io.WriteString(w, `{"errors":null,"message":"user redirect does not exist [name: ghost]","url":"https://git.unkin.net/api/swagger"}`) + })) + defer srv.Close() + + c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client()} + _, err := c.CreateIssue("ghost/repo", CreateIssueOptions{Title: "T"}) + if err == nil { + t.Fatal("expected error for a repo that does not exist") + } + if !IsNotFound(err) { + t.Errorf("IsNotFound(%v) = false, want true", err) + } + if !strings.Contains(err.Error(), "user redirect does not exist") { + t.Errorf("error %q should carry the API message", err) + } +} + +// Any other non-2xx is a plain API failure: reported, not retried, and not +// mistaken for a missing repo. +func TestCreateIssueAPIError(t *testing.T) { + requests := 0 + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/issues", func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = io.WriteString(w, `{"errors":null,"message":"Validation Error: title is empty","url":"https://git.unkin.net/api/swagger"}`) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + c := &GiteaClient{BaseURL: srv.URL, Token: "t", HTTP: srv.Client()} + _, err := c.CreateIssue("unkin/repo", CreateIssueOptions{Title: "T"}) + if err == nil { + t.Fatal("expected error on 422") + } + if IsNotFound(err) { + t.Errorf("a 422 must not read as not-found: %v", err) + } + if !strings.Contains(err.Error(), "Validation Error") { + t.Errorf("error %q should carry the API message", err) + } + if requests != 1 { + t.Errorf("requests = %d, want 1 (a 422 is not retried)", requests) + } +} + +// An issue edit sends only the fields it was given, for the same reason a PR +// edit does: Gitea overwrites whatever key it receives. +func TestEditIssueSendsOnlySuppliedFields(t *testing.T) { + title, body := "new title", "new body" + tests := []struct { + name string + opts EditOptions + want map[string]any + }{ + {"body only", EditOptions{Body: &body}, map[string]any{"body": "new body"}}, + {"title only", EditOptions{Title: &title}, map[string]any{"title": "new title"}}, + {"both", EditOptions{Title: &title, Body: &body}, map[string]any{"title": "new title", "body": "new body"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var gotBody map[string]any + var gotMethod, gotPath string + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/repos/unkin/repo/issues/12", func(w http.ResponseWriter, r *http.Request) { + gotMethod, gotPath = r.Method, r.URL.Path + _ = json.NewDecoder(r.Body).Decode(&gotBody) + _, _ = io.WriteString(w, `{"number":12,"title":"new title","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()} + issue, err := c.EditIssue("unkin/repo", 12, tt.opts) + if err != nil { + t.Fatalf("EditIssue: %v", err) + } + if gotMethod != http.MethodPatch { + t.Errorf("method = %s, want PATCH", gotMethod) + } + if gotPath != "/api/v1/repos/unkin/repo/issues/12" { + t.Errorf("path = %q", gotPath) + } + if len(gotBody) != len(tt.want) { + t.Errorf("payload = %v, want exactly the supplied fields %v", gotBody, tt.want) + } + for k, v := range tt.want { + if gotBody[k] != v { + t.Errorf("payload[%q] = %v, want %v", k, gotBody[k], v) + } + } + if issue.Number != 12 || issue.HTMLURL == "" { + t.Errorf("parsed issue = %+v", issue) + } + }) + } +} diff --git a/internal/agent/git.go b/internal/agent/git.go index 54a1de6..675a4fa 100644 --- a/internal/agent/git.go +++ b/internal/agent/git.go @@ -109,6 +109,37 @@ func GitRemoteBranchExists(repoDir, remote, branch string) bool { return err == nil } +// GitRevParse resolves ref to a full object id in repoDir. +func GitRevParse(repoDir, ref string) (string, error) { + return runGit(repoDir, "rev-parse", ref) +} + +// GitAheadCount counts commits reachable from head that upstream does not hold. +func GitAheadCount(repoDir, upstream, head string) (int, error) { + out, err := runGit(repoDir, "rev-list", "--count", upstream+".."+head) + if err != nil { + return 0, err + } + n, err := strconv.Atoi(strings.TrimSpace(out)) + if err != nil { + return 0, fmt.Errorf("parse rev-list count %q: %w", out, err) + } + return n, nil +} + +// GitMergeFFOnly advances the branch checked out at dir to ref, failing rather +// than writing a merge commit when the move is not a fast-forward. +func GitMergeFFOnly(dir, ref string) error { + _, err := runGit(dir, "merge", "--ff-only", ref) + return err +} + +// GitSetUpstream points branch at the remote-tracking ref upstream. +func GitSetUpstream(repoDir, branch, upstream string) error { + _, err := runGit(repoDir, "branch", "--set-upstream-to="+upstream, branch) + return err +} + // GitIsDirty reports whether the checkout at dir has uncommitted or untracked // changes. func GitIsDirty(dir string) (bool, error) { diff --git a/internal/agent/gitea.go b/internal/agent/gitea.go index d075580..b88eac8 100644 --- a/internal/agent/gitea.go +++ b/internal/agent/gitea.go @@ -273,19 +273,20 @@ func (c *GiteaClient) CreatePR(repoPath string, opts CreatePROptions) (PullReque return pr, err } -// EditPROptions are the fields an edit may change. Pointers so an unset field -// is omitted from the payload entirely, leaving that field as it is. The two -// fields are not symmetric: Gitea only applies a title when it is non-empty, -// so Title can be set but never cleared and a "" title is a silent no-op, -// while a pointer to "" Body really does blank the body. -type EditPROptions struct { +// EditOptions are the fields an edit may change, for a pull request or an +// issue alike. Pointers so an unset field is omitted from the payload +// entirely, leaving that field as it is. The two fields are not symmetric: +// Gitea only applies a title when it is non-empty, so Title can be set but +// never cleared and a "" title is a silent no-op, while a pointer to "" Body +// really does blank the body. +type EditOptions struct { Title *string `json:"title,omitempty"` Body *string `json:"body,omitempty"` } // EditPR updates a pull request's title and/or body // (PATCH /api/v1/repos/{owner}/{repo}/pulls/{index}). -func (c *GiteaClient) EditPR(repoPath string, number int, opts EditPROptions) (PullRequest, error) { +func (c *GiteaClient) EditPR(repoPath string, number int, opts EditOptions) (PullRequest, error) { var pr PullRequest err := c.do(http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/pulls/%d", repoPath, number), opts, &pr) return pr, err @@ -298,6 +299,36 @@ func (c *GiteaClient) GetPR(repoPath string, number int) (PullRequest, error) { return pr, err } +// Issue is the subset of Gitea's issue object we track. Gitea numbers issues +// and pull requests in one sequence, so Number is comparable to a PR number. +type Issue struct { + Number int `json:"number"` + State string `json:"state"` + Title string `json:"title"` + HTMLURL string `json:"html_url"` +} + +// CreateIssueOptions are the fields for filing an issue. +type CreateIssueOptions struct { + Title string `json:"title"` + Body string `json:"body"` +} + +// CreateIssue files an issue (POST /api/v1/repos/{owner}/{repo}/issues). +func (c *GiteaClient) CreateIssue(repoPath string, opts CreateIssueOptions) (Issue, error) { + var issue Issue + err := c.do(http.MethodPost, "/api/v1/repos/"+repoPath+"/issues", opts, &issue) + return issue, err +} + +// EditIssue updates an issue's title and/or body +// (PATCH /api/v1/repos/{owner}/{repo}/issues/{index}). +func (c *GiteaClient) EditIssue(repoPath string, number int, opts EditOptions) (Issue, error) { + var issue Issue + err := c.do(http.MethodPatch, fmt.Sprintf("/api/v1/repos/%s/issues/%d", repoPath, number), opts, &issue) + return issue, err +} + // Comment is the subset of an issue comment we track. type Comment struct { ID int64 `json:"id"` @@ -305,8 +336,10 @@ type Comment struct { Body string `json:"body"` } -// CreateComment posts a comment on the PR's issue thread -// (POST /api/v1/repos/{owner}/{repo}/issues/{n}/comments). +// CreateComment posts a comment on an issue thread +// (POST /api/v1/repos/{owner}/{repo}/issues/{n}/comments). Gitea backs a pull +// request with an issue of the same number, so this is the single path for +// both. func (c *GiteaClient) CreateComment(repoPath string, number int, body string) (Comment, error) { var cm Comment payload := map[string]string{"body": body}