diff --git a/README.md b/README.md index 20d4148..4634e2f 100644 --- a/README.md +++ b/README.md @@ -5,16 +5,16 @@ isolated git worktree, and opens a **draft** pull request for you to review. It anything. ``` -gh search issues --label agent-ready - │ - ▼ - claim (SQLite lease, one issue per repo at a time) - │ - ▼ +gh search issues --label agent-ready gh search prs --author + │ │ + ▼ ▼ + claim (SQLite lease, one issue per repo at a time, shared with the PR path) + │ │ + ▼ ▼ git worktree ──▶ claude -p --output-format stream-json - │ - ▼ - run the repo's tests ──▶ commit ──▶ push ──▶ gh pr create --draft + │ │ + ▼ ▼ + run the repo's tests ──▶ commit ──▶ push ──▶ gh pr create --draft react 👀 ──▶ push ──▶ react 👍 ``` ## Table of contents @@ -26,6 +26,7 @@ gh search issues --label agent-ready - [CLI flags](#cli-flags) - [How work is selected](#how-work-is-selected) - [Lifecycle of one issue](#lifecycle-of-one-issue) +- [Responding to PR comments](#responding-to-pr-comments) - [Configuration reference](#configuration-reference) - [config.json](#configjson) - [models.json](#modelsjson) @@ -239,6 +240,39 @@ issue is worked at all: remove `agent-ready` to stop the retries. A usage limit hit mid-run is recorded as `deferred` — neither an attempt nor a failure, so it neither extends the back-off nor drops the issue down the ladder. +## Responding to PR comments + +Once a draft PR is open, a reviewer can hand feedback back to the agent without re-labelling +anything: comment on the PR mentioning the handle in `github.pr_comments.mention` (`@coding-agent` +by default), and the daemon picks it up on its next poll. + +1. **Discover** — `gh search prs --author ` scoped to `github.owners`, run + before issue discovery in every tick and drawing from the same per-repo concurrency budget: a + reviewer waiting on a reply outranks starting a new issue. Only PRs the daemon itself opened, on a + branch starting with `workspace.branch_prefix`, are ever considered — a hard rule, not a config + knob, so the daemon only ever pushes to branches it created itself. +2. **Match** — every conversation comment and inline review comment is checked for the mention. + Quoted lines (`>`) and fenced code blocks don't count, so a comment that merely quotes or shows a + previous mention can't re-trigger the agent, and the daemon skips its own comments and marker- + tagged replies. Only commenters whose `author_association` is `OWNER`, `MEMBER`, or `COLLABORATOR` + (or who appear in `github.pr_comments.allowed_authors`) can trigger a run — review summary bodies + are shown to the model as context but can never trigger one, since GitHub's REST API has no + reactions endpoint for a review as a whole. +3. **Acknowledge** — every matching comment gets `github.pr_comments.ack_reaction` (👀 by default) + immediately, before any cloning: that's the visible promise that it was seen. +4. **Address** — the PR's own branch is checked out as-is (not reset against the default branch), and + Claude is given the PR and its triggering comments, with the same no-git/no-GitHub-mutation + contract as the issue flow, reframed around a branch and PR that already exist. If the feedback is + a question rather than a change request, the agent answers it in its final summary instead of + editing code — that still counts as addressed. +5. **Deliver** — if there's a code change, it's committed, verified, and pushed to the PR's branch; + either way a reply is posted summarising what was done, and each comment gets + `github.pr_comments.done_reaction` (👍 by default). +6. **Retry** — a failed attempt leaves the 👀 in place (the comment was seen) and retries after the + same exponential back-off as a failed issue, tracked per comment so one stuck comment doesn't hold + up others on the same PR. A daemon restart between the 👀 and the reply is not stranded: the task + is recorded as soon as the reaction goes out, and a subsequent pass retries it. + ## Configuration reference ### config.json @@ -261,7 +295,17 @@ This repository's own `config.json` is also **compiled into the binary** at buil "exclude_repos": [], "search_limit": 50, "poll_interval": "5m", - "binary": "gh" + "binary": "gh", + "pr_comments": { + "enabled": true, + "mention": "@coding-agent", + "search_limit": 30, + "max_age": "168h", + "ack_reaction": "eyes", + "done_reaction": "+1", + "allowed_authors": [], + "allowed_associations": ["OWNER", "MEMBER", "COLLABORATOR"] + } }, "workspace": { "root": "~/.agent-loop/work", @@ -315,6 +359,13 @@ This repository's own `config.json` is also **compiled into the binary** at buil | `github.exclude_repos` | `owner/name` repos to never touch, even if labelled | | `github.search_limit` | max issues fetched per discovery pass | | `github.poll_interval` | how often discovery runs | +| `github.pr_comments.enabled` | watch the daemon's own open PRs for `@`-mentions and act on them (see [Responding to PR comments](#responding-to-pr-comments)) | +| `github.pr_comments.mention` | handle a comment must contain to trigger a response; **must start with `@`** | +| `github.pr_comments.search_limit` | max of the daemon's own open PRs checked per pass | +| `github.pr_comments.max_age` | ignore comments older than this; `0` disables the limit | +| `github.pr_comments.ack_reaction` / `done_reaction` | GitHub reaction content applied on pickup / once addressed; one of `+1 -1 laugh confused heart hooray rocket eyes` | +| `github.pr_comments.allowed_authors` | explicit login allowlist for who may trigger the agent; empty falls back to `allowed_associations` | +| `github.pr_comments.allowed_associations` | `author_association` values permitted to trigger the agent (`OWNER`, `MEMBER`, `COLLABORATOR`, ...) when `allowed_authors` is empty | | `workspace.root` | where per-issue worktrees live | | `workspace.repos_root` | where the one-per-repo checkout-less clones live | | `workspace.logs_root` | where JSONL run transcripts are written | @@ -448,7 +499,7 @@ Loopback-only by default. It can pause and cancel work, so do not expose it. | ---------------------------- | ------------------------------------------------------------------- | | `GET /healthz` | liveness | | `GET /status` | gate state, in-flight runs, claims, model cooldowns, usage snapshot | -| `GET /runs?limit=&repo=` | recent runs with outcome, model, cost, PR link, created/started/ended timestamps | +| `GET /runs?limit=&repo=` | recent runs with outcome, model, cost, PR link, created/started/ended timestamps; `kind` distinguishes an issue run from a PR-comment run | | `GET /runs/{id}` | one run plus its event timeline | | `GET /runs/{id}/log` | the raw JSONL transcript of the Claude run | | `GET /sessions?repo=&issue=&limit=` | Claude session IDs recorded per repo/issue, newest first | @@ -521,6 +572,13 @@ user can. What constrains it: - **Nothing is ever merged**, and every PR is a draft. - Child processes run in their own process group and are killed as a group on timeout, so a runaway grandchild (a stray build/test process) can't outlive the run. +- **PR comments only ever act on PRs the daemon itself opened, on a branch under + `workspace.branch_prefix`.** A mention on any other pull request is ignored outright. +- **Only permitted commenters can trigger a run from a PR comment** — `author_association` in + `OWNER`/`MEMBER`/`COLLABORATOR`, or an explicit `github.pr_comments.allowed_authors` entry — + otherwise an arbitrary commenter on a public repo could drive a `bypassPermissions` Claude run. +- `--dry-run` suppresses PR-comment reactions and replies exactly like it does labels, comments, and + pushes on the issue path. `--install` below scopes the systemd unit's filesystem access to `/opt/coding-agent-loop` and its own `~/.agent-loop` regardless of which account it runs as (see diff --git a/config.example.json b/config.example.json index 160ffd3..f27a234 100644 --- a/config.example.json +++ b/config.example.json @@ -11,7 +11,17 @@ "exclude_repos": [], "search_limit": 50, "poll_interval": "5m", - "binary": "gh" + "binary": "gh", + "pr_comments": { + "enabled": true, + "mention": "@coding-agent", + "search_limit": 30, + "max_age": "168h", + "ack_reaction": "eyes", + "done_reaction": "+1", + "allowed_authors": [], + "allowed_associations": ["OWNER", "MEMBER", "COLLABORATOR"] + } }, "workspace": { "root": "~/.agent-loop/work", diff --git a/internal/config/config.go b/internal/config/config.go index 88c5086..a33bdbd 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -74,6 +74,34 @@ type GitHubConfig struct { PollInterval Duration `json:"poll_interval"` // Binary is the gh executable; overridable for tests. Binary string `json:"binary"` + // PRComments controls responding to @-mentions on the daemon's own pull requests. + PRComments PRCommentsConfig `json:"pr_comments"` +} + +// PRCommentsConfig controls watching the daemon's own pull requests for +// review comments that @-mention it, and acting on them. +type PRCommentsConfig struct { + Enabled bool `json:"enabled"` + // Mention is the handle a comment must contain to trigger a response, + // e.g. "@coding-agent". + Mention string `json:"mention"` + // SearchLimit caps how many of the daemon's own open PRs one pass checks. + SearchLimit int `json:"search_limit"` + // MaxAge bounds how old a comment may be and still trigger a response. 0 + // means no limit. + MaxAge Duration `json:"max_age"` + // AckReaction/DoneReaction are GitHub reaction content values (one of + // "+1 -1 laugh confused heart hooray rocket eyes") applied to a triggering + // comment when it is picked up and when it has been addressed. + AckReaction string `json:"ack_reaction"` + DoneReaction string `json:"done_reaction"` + // AllowedAuthors is an explicit login allowlist of commenters who may + // trigger the agent. Empty falls back to AllowedAssociations. + AllowedAuthors []string `json:"allowed_authors"` + // AllowedAssociations lists the author_association values (OWNER, MEMBER, + // COLLABORATOR, ...) permitted to trigger the agent when AllowedAuthors is + // empty. + AllowedAssociations []string `json:"allowed_associations"` } type WorkspaceConfig struct { @@ -165,6 +193,15 @@ func Default() Config { SearchLimit: 50, PollInterval: Duration(5 * time.Minute), Binary: "gh", + PRComments: PRCommentsConfig{ + Enabled: true, + Mention: "@coding-agent", + SearchLimit: 30, + MaxAge: Duration(168 * time.Hour), + AckReaction: "eyes", + DoneReaction: "+1", + AllowedAssociations: []string{"OWNER", "MEMBER", "COLLABORATOR"}, + }, }, Workspace: WorkspaceConfig{ Root: "~/.agent-loop/work", @@ -289,9 +326,32 @@ func (c *Config) Validate() error { if c.Discord.Enabled && c.Discord.WebhookURL == "" { return fmt.Errorf("discord.webhook_url must be set when discord.enabled is true") } + if c.GitHub.PRComments.Enabled { + pc := c.GitHub.PRComments + if !strings.HasPrefix(pc.Mention, "@") { + return fmt.Errorf("github.pr_comments.mention must start with '@', got %q", pc.Mention) + } + if pc.SearchLimit < 1 { + return fmt.Errorf("github.pr_comments.search_limit must be >= 1, got %d", pc.SearchLimit) + } + if !validReaction(pc.AckReaction) { + return fmt.Errorf("github.pr_comments.ack_reaction %q is not a valid GitHub reaction", pc.AckReaction) + } + if !validReaction(pc.DoneReaction) { + return fmt.Errorf("github.pr_comments.done_reaction %q is not a valid GitHub reaction", pc.DoneReaction) + } + } return nil } +// validReactions are the only content values GitHub's reactions API accepts. +var validReactions = map[string]bool{ + "+1": true, "-1": true, "laugh": true, "confused": true, + "heart": true, "hooray": true, "rocket": true, "eyes": true, +} + +func validReaction(r string) bool { return validReactions[r] } + // validOwners returns the entries of owners that are non-blank after trimming. func validOwners(owners []string) []string { out := make([]string, 0, len(owners)) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index a8101ab..f079bbb 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -174,3 +174,49 @@ func TestRetryBackoffMaxMustNotBeBelowTheBase(t *testing.T) { t.Fatalf("want a backoff validation error, got %v", err) } } + +func TestPRCommentsDefaults(t *testing.T) { + cfg, err := Load(writeConfig(t, `{"github":{"owners":["acme"]}}`), false) + if err != nil { + t.Fatal(err) + } + pc := cfg.GitHub.PRComments + if !pc.Enabled { + t.Fatal("pr_comments should be enabled by default") + } + if pc.Mention != "@coding-agent" { + t.Fatalf("unexpected default mention %q", pc.Mention) + } + if pc.AckReaction != "eyes" || pc.DoneReaction != "+1" { + t.Fatalf("unexpected default reactions: ack=%q done=%q", pc.AckReaction, pc.DoneReaction) + } + if len(pc.AllowedAssociations) == 0 { + t.Fatal("default allowed associations should not be empty") + } +} + +// A config file written before this feature existed has no pr_comments block +// at all; it must still load using the defaults. +func TestConfigWithoutPRCommentsBlockStillLoads(t *testing.T) { + cfg, err := Load(writeConfig(t, `{"github":{"owners":["acme"],"label":"agent-ready"}}`), false) + if err != nil { + t.Fatalf("a config predating pr_comments must still load: %v", err) + } + if !cfg.GitHub.PRComments.Enabled { + t.Fatal("defaults should still populate pr_comments") + } +} + +func TestPRCommentsRejectsInvalidReaction(t *testing.T) { + _, err := Load(writeConfig(t, `{"github":{"owners":["acme"],"pr_comments":{"enabled":true,"mention":"@coding-agent","search_limit":30,"ack_reaction":"nope","done_reaction":"+1"}}}`), false) + if err == nil || !strings.Contains(err.Error(), "ack_reaction") { + t.Fatalf("want an invalid reaction error, got %v", err) + } +} + +func TestPRCommentsRejectsMentionWithoutAt(t *testing.T) { + _, err := Load(writeConfig(t, `{"github":{"owners":["acme"],"pr_comments":{"enabled":true,"mention":"coding-agent","search_limit":30,"ack_reaction":"eyes","done_reaction":"+1"}}}`), false) + if err == nil || !strings.Contains(err.Error(), "mention") { + t.Fatalf("want a mention validation error, got %v", err) + } +} diff --git a/internal/discord/notifier.go b/internal/discord/notifier.go index 4d5d00f..72e8d64 100644 --- a/internal/discord/notifier.go +++ b/internal/discord/notifier.go @@ -304,6 +304,28 @@ func (n *Notifier) PlanPosted(r RunRef, res *claude.Result, elapsed time.Duratio }) } +// PRCommentsAddressed reports that review feedback on a pull request was +// acted on: code pushed (or not, when the feedback needed only a reply) and +// verified. +func (n *Notifier) PRCommentsAddressed(r RunRef, handled int, res *claude.Result, v verify.Result, elapsed time.Duration) { + model, cost := "", 0.0 + if res != nil { + model, cost = res.PrimaryModel(), res.TotalCostUSD + } + n.post(embed{ + Title: r.title("PR comments addressed"), + Description: r.description(), + Color: colorGreen, + Fields: append(r.fields(), + embedField{Name: "Comments handled", Value: fmt.Sprintf("%d", handled), Inline: true}, + embedField{Name: "Model", Value: orNone(model), Inline: true}, + embedField{Name: "Cost", Value: money(cost), Inline: true}, + embedField{Name: "Verification", Value: orNone(v.Status), Inline: true}, + embedField{Name: "Duration", Value: humanDuration(elapsed), Inline: true}, + ), + }) +} + // RunCanceled reports a run stopped from outside — a daemon shutdown or an // operator cancelling it. It is deliberately not styled as a failure. func (n *Notifier) RunCanceled(r RunRef, reason string) { diff --git a/internal/gh/gh.go b/internal/gh/gh.go index 55f16dd..8e2133c 100644 --- a/internal/gh/gh.go +++ b/internal/gh/gh.go @@ -13,6 +13,7 @@ import ( "errors" "fmt" "os/exec" + "sort" "strconv" "strings" "time" @@ -160,14 +161,15 @@ func (i Issue) HasLabel(name string) bool { return false } -// PullRequest is a PR the harness may have opened, used to avoid opening a -// second one for an issue. +// PullRequest is an open PR, used to avoid opening a second one for an issue, +// and to view a PR the daemon is watching for review comments. type PullRequest struct { Number int `json:"number"` URL string `json:"url"` Body string `json:"body"` Title string `json:"title"` HeadRefName string `json:"headRefName"` + BaseRefName string `json:"baseRefName"` State string `json:"state"` IsDraft bool `json:"isDraft"` MergedAt *time.Time `json:"mergedAt"` @@ -178,6 +180,32 @@ func (p PullRequest) Merged() bool { return p.MergedAt != nil && !p.MergedAt.IsZero() } +// Comment kinds a PRComment can come from. The kind decides which REST path +// segment (issues or pulls) a reaction or reply goes through. +const ( + CommentKindIssue = "issue" + CommentKindReview = "review" +) + +// PRComment is one reactable comment on a pull request: either a plain +// conversation comment or an inline review comment. +type PRComment struct { + // ID is the REST numeric id the reactions endpoint keys on. + ID int64 + Kind string + Author string + Association string + Body string + URL string + CreatedAt time.Time + // Path, DiffHunk, and Line are set only for review comments (Kind == + // CommentKindReview), giving the prompt the anchor a plain issue comment + // does not have. + Path string + DiffHunk string + Line int +} + // --- read operations -------------------------------------------------------- // AuthStatus returns an error when gh is not authenticated. @@ -303,6 +331,165 @@ func (c *Client) CloneURL(ctx context.Context, repo string) (string, error) { return out.URL + ".git", nil } +// PRSearchResult is one hit from `gh search prs`. +type PRSearchResult struct { + Number int `json:"number"` + Title string `json:"title"` + URL string `json:"url"` + Repository struct { + NameWithOwner string `json:"nameWithOwner"` + } `json:"repository"` + UpdatedAt time.Time `json:"updatedAt"` +} + +// CurrentLogin returns the login of the authenticated gh account, i.e. the +// daemon's own account. Callers should cache it: it never changes within a +// process's lifetime. +func (c *Client) CurrentLogin(ctx context.Context) (string, error) { + out, err := c.run(ctx, "", "api", "user", "--jq", ".login") + if err != nil { + return "", err + } + login := strings.TrimSpace(string(out)) + if login == "" { + return "", fmt.Errorf("gh api user reported no login") + } + return login, nil +} + +// SearchPRs finds open pull requests authored by author, restricted to owners +// when given. +func (c *Client) SearchPRs(ctx context.Context, author string, owners []string, limit int) ([]PRSearchResult, error) { + if author == "" { + return nil, fmt.Errorf("search requires an author") + } + if limit <= 0 { + limit = 30 + } + args := []string{"search", "prs", + "--author", author, + "--state", "open", + "--limit", strconv.Itoa(limit), + "--json", "number,title,url,repository,updatedAt", + } + for _, o := range owners { + if o = strings.TrimSpace(o); o != "" { + args = append(args, "--owner", o) + } + } + + var results []PRSearchResult + if err := c.runJSON(ctx, &results, args...); err != nil { + return nil, err + } + // Defensive, mirroring SearchIssues: a result naming any other owner must + // never reach the caller. + filtered := results[:0] + for _, r := range results { + if len(owners) > 0 && !ownedBy(r.Repository.NameWithOwner, owners) { + c.logf("dropping PR search result for %s: repository owner is not in %v", r.Repository.NameWithOwner, owners) + continue + } + filtered = append(filtered, r) + } + return filtered, nil +} + +// ViewPR fetches one pull request's detail. +func (c *Client) ViewPR(ctx context.Context, repo string, number int) (PullRequest, error) { + var pr PullRequest + err := c.runJSON(ctx, &pr, + "pr", "view", strconv.Itoa(number), + "--repo", repo, + "--json", "number,title,body,url,state,isDraft,headRefName,baseRefName") + if err != nil { + return PullRequest{}, err + } + return pr, nil +} + +// rawIssueComment/rawReviewComment decode the shape `gh api` returns for the +// two comment endpoints, ahead of being reduced to the common PRComment type. +type rawIssueComment struct { + ID int64 `json:"id"` + Body string `json:"body"` + URL string `json:"html_url"` + User User `json:"user"` + Assoc string `json:"author_association"` + At string `json:"created_at"` +} + +type rawReviewComment struct { + ID int64 `json:"id"` + Body string `json:"body"` + URL string `json:"html_url"` + User User `json:"user"` + Assoc string `json:"author_association"` + At string `json:"created_at"` + Path string `json:"path"` + DiffHunk string `json:"diff_hunk"` + Line int `json:"line"` +} + +// PRComments returns every reactable comment on a pull request: plain +// conversation comments and inline review comments, merged and sorted oldest +// first. Review *summary* bodies are not included here — see PRReviewBodies — +// because REST has no reactions endpoint for a review as a whole. +func (c *Client) PRComments(ctx context.Context, repo string, number int) ([]PRComment, error) { + var issueRaw []rawIssueComment + if err := c.runJSON(ctx, &issueRaw, "api", "--paginate", + fmt.Sprintf("repos/%s/issues/%d/comments", repo, number)); err != nil { + return nil, err + } + var reviewRaw []rawReviewComment + if err := c.runJSON(ctx, &reviewRaw, "api", "--paginate", + fmt.Sprintf("repos/%s/pulls/%d/comments", repo, number)); err != nil { + return nil, err + } + + out := make([]PRComment, 0, len(issueRaw)+len(reviewRaw)) + for _, r := range issueRaw { + out = append(out, PRComment{ + ID: r.ID, Kind: CommentKindIssue, Author: r.User.Login, Association: r.Assoc, + Body: r.Body, URL: r.URL, CreatedAt: parseGHTime(r.At), + }) + } + for _, r := range reviewRaw { + out = append(out, PRComment{ + ID: r.ID, Kind: CommentKindReview, Author: r.User.Login, Association: r.Assoc, + Body: r.Body, URL: r.URL, CreatedAt: parseGHTime(r.At), + Path: r.Path, DiffHunk: r.DiffHunk, Line: r.Line, + }) + } + sort.SliceStable(out, func(i, j int) bool { return out[i].CreatedAt.Before(out[j].CreatedAt) }) + return out, nil +} + +// PRReviewBodies returns the summary body of every review left on a pull +// request, context only: they cannot be reacted to over REST, so they can +// never trigger the agent. +func (c *Client) PRReviewBodies(ctx context.Context, repo string, number int) ([]string, error) { + var raw []struct { + Body string `json:"body"` + } + if err := c.runJSON(ctx, &raw, "api", "--paginate", + fmt.Sprintf("repos/%s/pulls/%d/reviews", repo, number)); err != nil { + return nil, err + } + out := make([]string, 0, len(raw)) + for _, r := range raw { + if strings.TrimSpace(r.Body) != "" { + out = append(out, r.Body) + } + } + return out, nil +} + +func parseGHTime(s string) time.Time { + t, _ := time.Parse(time.RFC3339, s) + return t +} + // ListPRs returns the repo's pull requests in the given state ("open", // "closed", "merged", or "all"). func (c *Client) ListPRs(ctx context.Context, repo, state string, limit int) ([]PullRequest, error) { @@ -578,6 +765,30 @@ func (c *Client) Comment(ctx context.Context, repo string, number int, body stri return err } +// CommentOnPR posts a comment on a pull request. A PR is an issue as far as +// the comments endpoint is concerned, so this is just Comment under a name +// that reads correctly at its call sites. +func (c *Client) CommentOnPR(ctx context.Context, repo string, number int, body string) error { + return c.Comment(ctx, repo, number, body) +} + +// React adds a reaction to a PR comment. The path segment is chosen by the +// comment's kind: plain conversation comments react through the issues +// endpoint, inline review comments through the pulls endpoint. +func (c *Client) React(ctx context.Context, repo string, comment PRComment, content string) error { + kind := "issues" + if comment.Kind == CommentKindReview { + kind = "pulls" + } + path := fmt.Sprintf("repos/%s/%s/comments/%d/reactions", repo, kind, comment.ID) + if c.DryRun { + c.logf("dry-run: would react %q to %s comment %d on %s", content, comment.Kind, comment.ID, repo) + return nil + } + _, err := c.run(ctx, "", "api", "--method", "POST", path, "-f", "content="+content) + return err +} + // LinkPRToIssue makes a PR close its issue on merge, by prepending a "Closes // #" line to its body. It is a no-op when the body already links the issue. // diff --git a/internal/gh/gh_test.go b/internal/gh/gh_test.go index 801b0ae..f40dd42 100644 --- a/internal/gh/gh_test.go +++ b/internal/gh/gh_test.go @@ -347,6 +347,101 @@ func TestEditLabelsNoopWhenNothingToDo(t *testing.T) { } } +func TestSearchPRsOwnerScoping(t *testing.T) { + out := `[ + {"number":9,"title":"Fix thing","url":"https://github.com/acme/widgets/pull/9", + "repository":{"nameWithOwner":"acme/widgets"},"updatedAt":"2024-01-01T00:00:00Z"}, + {"number":10,"title":"Wrong owner","url":"x","repository":{"nameWithOwner":"someoneelse/other"}, + "updatedAt":"2024-01-01T00:00:00Z"} + ]` + bin, argsFile, _ := stubGH(t, out) + c := New(bin, false) + + results, err := c.SearchPRs(context.Background(), "coding-agent-bot", []string{"acme"}, 10) + if err != nil { + t.Fatalf("SearchPRs: %v", err) + } + if len(results) != 1 || results[0].Repository.NameWithOwner != "acme/widgets" { + t.Fatalf("expected only the acme/widgets PR, got %+v", results) + } + + args := readFile(t, argsFile) + for _, want := range []string{"search", "prs", "--author", "coding-agent-bot", "--state", "open", "--owner", "acme"} { + if !strings.Contains(args, want) { + t.Errorf("command missing %q:\n%s", want, args) + } + } +} + +func TestPRCommentsMergesAndSortsBothKinds(t *testing.T) { + dir := t.TempDir() + bin := filepath.Join(dir, "gh-stub.sh") + script := "#!/bin/sh\n" + + "case \"$*\" in\n" + + " *issues/9/comments*) cat <<'EOF'\n" + + `[{"id":1,"body":"first","html_url":"u1","user":{"login":"alice"},"author_association":"OWNER","created_at":"2024-01-02T00:00:00Z"}]` + "\n" + + "EOF\n" + + " ;;\n" + + " *pulls/9/comments*) cat <<'EOF'\n" + + `[{"id":2,"body":"second","html_url":"u2","user":{"login":"bob"},"author_association":"MEMBER","created_at":"2024-01-01T00:00:00Z","path":"main.go","diff_hunk":"@@ -1 +1 @@","line":5}]` + "\n" + + "EOF\n" + + " ;;\n" + + "esac\n" + if err := os.WriteFile(bin, []byte(script), 0o755); err != nil { + t.Fatal(err) + } + + comments, err := New(bin, false).PRComments(context.Background(), "acme/widgets", 9) + if err != nil { + t.Fatalf("PRComments: %v", err) + } + if len(comments) != 2 { + t.Fatalf("expected 2 comments, got %+v", comments) + } + // The review comment (Jan 1) is older than the issue comment (Jan 2). + if comments[0].Kind != CommentKindReview || comments[0].Path != "main.go" || comments[0].Line != 5 { + t.Fatalf("comments not sorted or review fields not decoded: %+v", comments[0]) + } + if comments[1].Kind != CommentKindIssue || comments[1].Author != "alice" { + t.Fatalf("issue comment not decoded: %+v", comments[1]) + } +} + +func TestReactUsesKindSpecificPathAndHonoursDryRun(t *testing.T) { + bin, argsFile, _ := stubGH(t, "") + c := New(bin, false) + + if err := c.React(context.Background(), "acme/widgets", + PRComment{ID: 42, Kind: CommentKindIssue}, "eyes"); err != nil { + t.Fatal(err) + } + args := readFile(t, argsFile) + if !strings.Contains(args, "repos/acme/widgets/issues/comments/42/reactions") { + t.Errorf("expected the issues reactions path, got:\n%s", args) + } + if !strings.Contains(args, "content=eyes") { + t.Errorf("expected content=eyes, got:\n%s", args) + } + + bin2, argsFile2, _ := stubGH(t, "") + if err := New(bin2, false).React(context.Background(), "acme/widgets", + PRComment{ID: 7, Kind: CommentKindReview}, "+1"); err != nil { + t.Fatal(err) + } + if args := readFile(t, argsFile2); !strings.Contains(args, "repos/acme/widgets/pulls/comments/7/reactions") { + t.Errorf("expected the pulls reactions path, got:\n%s", args) + } + + bin3, argsFile3, _ := stubGH(t, "") + if err := New(bin3, true).React(context.Background(), "acme/widgets", + PRComment{ID: 7, Kind: CommentKindReview}, "+1"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(argsFile3); !os.IsNotExist(err) { + t.Fatal("dry-run must not invoke gh at all") + } +} + func TestCmdErrorIncludesStderr(t *testing.T) { dir := t.TempDir() bin := filepath.Join(dir, "gh-fail.sh") diff --git a/internal/orchestrator/loop.go b/internal/orchestrator/loop.go index bbbe6c0..6ec2b7a 100644 --- a/internal/orchestrator/loop.go +++ b/internal/orchestrator/loop.go @@ -65,6 +65,9 @@ type Orchestrator struct { activeRepos map[string]bool cancels map[string]context.CancelFunc repoInfo map[string]repoMeta + // botLogin caches the daemon's own GitHub login, used to scope PR + // discovery and to make sure the daemon never reacts to its own comments. + botLogin string } type repoMeta struct { @@ -172,6 +175,13 @@ func (o *Orchestrator) tick(ctx context.Context) { return } + // A reviewer waiting on a reply outranks starting a new issue, and both + // draw from the same per-repo concurrency budget. + capacity = o.tickPRComments(ctx, capacity) + if capacity <= 0 { + return + } + results, err := o.opts.GH.SearchIssues(ctx, o.opts.Config.GitHub.Label, o.opts.Config.GitHub.Owners, o.opts.Config.GitHub.SearchLimit) if err != nil { o.log.Error("issue discovery failed", "error", err) diff --git a/internal/orchestrator/phase.go b/internal/orchestrator/phase.go index b4b40c3..9a0d804 100644 --- a/internal/orchestrator/phase.go +++ b/internal/orchestrator/phase.go @@ -28,9 +28,10 @@ const approvalKeyword = "implement" const markerPrefix = "" - markerPR = markerPrefix + "pr -->" - markerFailure = markerPrefix + "failure -->" + markerPlan = markerPrefix + "plan -->" + markerPR = markerPrefix + "pr -->" + markerFailure = markerPrefix + "failure -->" + markerPRComment = markerPrefix + "pr-comment -->" ) func isAgentComment(body string) bool { diff --git a/internal/orchestrator/prcomments.go b/internal/orchestrator/prcomments.go new file mode 100644 index 0000000..da04ada --- /dev/null +++ b/internal/orchestrator/prcomments.go @@ -0,0 +1,609 @@ +package orchestrator + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "path/filepath" + "strings" + "sync" + "time" + + "github.com/google/uuid" + + "github.com/ableinc/coding-agent-loop/internal/claude" + "github.com/ableinc/coding-agent-loop/internal/config" + "github.com/ableinc/coding-agent-loop/internal/gate" + "github.com/ableinc/coding-agent-loop/internal/gh" + "github.com/ableinc/coding-agent-loop/internal/models" + "github.com/ableinc/coding-agent-loop/internal/store" + "github.com/ableinc/coding-agent-loop/internal/verify" +) + +// mentionsAgent reports whether body addresses the agent by handle. The match +// is case-insensitive and must sit on a word boundary on its trailing edge, so +// "@coding-agent-loop" is not read as a mention of "@coding-agent". Quoted +// lines (leading ">") and fenced code blocks are ignored, so a comment that +// merely quotes a previous mention, or shows one in an example, cannot +// re-trigger the agent. +func mentionsAgent(body, handle string) bool { + handle = strings.ToLower(strings.TrimSpace(handle)) + if handle == "" { + return false + } + inFence := false + for _, line := range strings.Split(body, "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "```") { + inFence = !inFence + continue + } + if inFence || strings.HasPrefix(trimmed, ">") { + continue + } + if lineMentions(strings.ToLower(line), handle) { + return true + } + } + return false +} + +func lineMentions(lowerLine, lowerHandle string) bool { + from := 0 + for { + i := strings.Index(lowerLine[from:], lowerHandle) + if i < 0 { + return false + } + pos := from + i + end := pos + len(lowerHandle) + if end >= len(lowerLine) || !isMentionChar(lowerLine[end]) { + return true + } + from = pos + 1 + } +} + +func isMentionChar(b byte) bool { + return b == '-' || b == '_' || + (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || (b >= '0' && b <= '9') +} + +// authorAllowed reports whether a comment's author may trigger the agent. An +// explicit login allowlist takes precedence over the association list; when +// neither matches, an arbitrary commenter on a public repo could otherwise +// drive a bypassPermissions Claude run. +func authorAllowed(author, association string, cfg config.PRCommentsConfig) bool { + if len(cfg.AllowedAuthors) > 0 { + for _, a := range cfg.AllowedAuthors { + if strings.EqualFold(strings.TrimSpace(a), author) { + return true + } + } + return false + } + for _, a := range cfg.AllowedAssociations { + if strings.EqualFold(strings.TrimSpace(a), association) { + return true + } + } + return false +} + +// prCommentTaskKey identifies one triggering comment in the tasks map. +func prCommentTaskKey(kind string, id int64) string { + return fmt.Sprintf("%s/%d", kind, id) +} + +// pendingMentions filters a PR's comments to the ones this run should act on: +// mention present, author permitted, not older than maxAge, not authored by +// the daemon itself, not one of the daemon's own marker comments, and either +// unseen, previously acked (a crash between the ack and the run, since a live +// run holds the repo's claim and is never re-entered), or failed with its +// per-comment back-off elapsed. +func pendingMentions(comments []gh.PRComment, cfg config.PRCommentsConfig, botLogin string, + tasks []store.PRCommentTask, now time.Time, retryBase, retryMax time.Duration) []gh.PRComment { + + byKey := make(map[string]store.PRCommentTask, len(tasks)) + for _, t := range tasks { + byKey[prCommentTaskKey(t.CommentKind, t.CommentID)] = t + } + + maxAge := cfg.MaxAge.D() + var out []gh.PRComment + for _, c := range comments { + if isAgentComment(c.Body) { + continue + } + if botLogin != "" && strings.EqualFold(c.Author, botLogin) { + continue + } + if !mentionsAgent(c.Body, cfg.Mention) { + continue + } + if !authorAllowed(c.Author, c.Association, cfg) { + continue + } + if maxAge > 0 && !c.CreatedAt.IsZero() && now.Sub(c.CreatedAt) > maxAge { + continue + } + if task, ok := byKey[prCommentTaskKey(c.Kind, c.ID)]; ok { + if task.Status == store.PRCommentDone { + continue + } + if task.Status == store.PRCommentFailed { + next := task.LastAttemptAt.Add(retryDelay(task.Attempts, retryBase, retryMax)) + if now.Before(next) { + continue + } + } + // PRCommentAcked: either genuinely in flight (the repo's claim + // already excludes this PR from being picked up again) or + // stranded by a crash between the ack and the run, in which case + // it must be retried rather than left behind forever. + } + out = append(out, c) + } + return out +} + +// tickPRComments is one discovery pass over the daemon's own open pull +// requests, looking for @-mentions to act on. It runs before issue discovery +// in each tick and shares the same capacity budget: a reviewer waiting on a +// reply outranks starting a new issue. +func (o *Orchestrator) tickPRComments(ctx context.Context, capacity int) int { + cfg := o.opts.Config.GitHub.PRComments + if !cfg.Enabled || capacity <= 0 || ctx.Err() != nil { + return capacity + } + + login, err := o.currentLogin(ctx) + if err != nil { + o.log.Error("pr comments: could not resolve the daemon's own github login", "error", err) + return capacity + } + + results, err := o.opts.GH.SearchPRs(ctx, login, o.opts.Config.GitHub.Owners, cfg.SearchLimit) + if err != nil { + o.log.Error("pr discovery failed", "error", err) + return capacity + } + o.log.Debug("pr comment discovery pass", "candidates", len(results), "capacity", capacity) + + for _, r := range results { + if ctx.Err() != nil || capacity <= 0 { + return capacity + } + repo := r.Repository.NameWithOwner + if repo == "" || r.Number == 0 { + continue + } + if !o.opts.Config.GitHub.Owned(repo) { + o.log.Error("pr discovery returned a repo outside github.owners; refusing to touch it", + "repo", repo, "pr", r.Number, "owners", strings.Join(o.opts.Config.GitHub.Owners, ",")) + continue + } + if o.opts.Config.GitHub.Excluded(repo) { + continue + } + + o.mu.Lock() + busy := o.activeRepos[repo] + o.mu.Unlock() + if busy { + continue + } + if busy, err := o.opts.Store.RepoBusy(ctx, repo); err != nil { + o.log.Error("repo busy check failed", "repo", repo, "error", err) + continue + } else if busy { + continue + } + + pr, err := o.opts.GH.ViewPR(ctx, repo, r.Number) + if err != nil { + o.log.Error("pr view failed", "repo", repo, "pr", r.Number, "error", err) + continue + } + if !strings.EqualFold(pr.State, "OPEN") { + continue + } + // Hard safety rule, not a config knob: only ever push to a branch this + // daemon created itself. + if !strings.HasPrefix(pr.HeadRefName, o.opts.Config.Workspace.BranchPrefix) { + continue + } + + comments, err := o.opts.GH.PRComments(ctx, repo, r.Number) + if err != nil { + o.log.Error("pr comments fetch failed", "repo", repo, "pr", r.Number, "error", err) + continue + } + tasks, err := o.opts.Store.PRCommentTasks(ctx, repo, r.Number) + if err != nil { + o.log.Error("pr comment task lookup failed", "repo", repo, "pr", r.Number, "error", err) + continue + } + pending := pendingMentions(comments, cfg, login, tasks, time.Now(), + o.opts.Config.Run.RetryBackoff.D(), o.opts.Config.Run.RetryBackoffMax.D()) + if len(pending) == 0 { + continue + } + + if !o.reserveRepo(repo) { + continue + } + capacity-- + + cand := candidate{repo: repo, number: r.Number, title: pr.Title, url: pr.URL} + o.wg.Add(1) + go func() { + defer o.wg.Done() + defer o.releaseRepo(cand.repo) + o.workPRComments(ctx, cand, pr, pending) + }() + } + return capacity +} + +// currentLogin returns and caches the daemon's own GitHub login, used both to +// scope SearchPRs and to make sure the daemon never reacts to its own +// comments. +func (o *Orchestrator) currentLogin(ctx context.Context) (string, error) { + o.mu.Lock() + login := o.botLogin + o.mu.Unlock() + if login != "" { + return login, nil + } + login, err := o.opts.GH.CurrentLogin(ctx) + if err != nil { + return "", err + } + o.mu.Lock() + o.botLogin = login + o.mu.Unlock() + return login, nil +} + +// workPRComments is the full lifecycle for one batch of triggering comments on +// one pull request. +func (o *Orchestrator) workPRComments(ctx context.Context, cand candidate, pr gh.PullRequest, pending []gh.PRComment) { + cfg := o.opts.Config + pcCfg := cfg.GitHub.PRComments + runID := uuid.NewString() + log := o.log.With("run", runID, "repo", cand.repo, "pr", cand.number) + + claimed, err := o.opts.Store.TryClaim(ctx, cand.repo, cand.number, runID, o.opts.WorkerID, cfg.Run.Lease.D()) + if err != nil { + log.Error("claim failed", "error", err) + return + } + if !claimed { + log.Debug("pull request claimed by another worker") + return + } + defer func() { + if err := o.opts.Store.ReleaseClaim(context.WithoutCancel(ctx), cand.repo, cand.number, runID); err != nil { + log.Error("release claim failed", "error", err) + } + }() + + runCtx, cancel := context.WithCancel(ctx) + defer cancel() + o.mu.Lock() + o.cancels[runID] = cancel + o.mu.Unlock() + defer func() { + o.mu.Lock() + delete(o.cancels, runID) + o.mu.Unlock() + }() + + stopRenew := o.renewLease(runCtx, cand, runID) + defer stopRenew() + + logPath := filepath.Join(cfg.Workspace.LogsRoot, runID+".jsonl") + + run := store.Run{ + ID: runID, Repo: cand.repo, Issue: cand.number, Attempt: 1, + Branch: pr.HeadRefName, Status: store.StatusClaimed, Kind: store.RunKindPRComment, + StartedAt: time.Now(), LogPath: logPath, + } + if err := o.opts.Store.CreateRun(ctx, run); err != nil { + log.Error("create run failed", "error", err) + return + } + ref := cand.ref(runID, 1) + o.event(ctx, runID, "claimed", fmt.Sprintf("%d review comment(s) as worker %s", len(pending), o.opts.WorkerID)) + o.opts.Discord.RunClaimed(ref, 0) + + // Ack every comment immediately, before any cloning: the 👀 is the + // user-visible promise that the comment was seen, and it must not wait on + // a slow clone or worktree setup. + for _, c := range pending { + if err := o.opts.GH.React(runCtx, cand.repo, c, pcCfg.AckReaction); err != nil { + log.Warn("could not ack comment", "comment", c.ID, "error", err) + } + if err := o.opts.Store.MarkPRCommentAcked(runCtx, cand.repo, cand.number, c.Kind, c.ID, runID); err != nil { + log.Warn("could not record ack", "comment", c.ID, "error", err) + } + } + + if err := o.executePRComments(runCtx, log, cand, pr, pending, runID, logPath); err != nil { + o.handlePRCommentFailure(ctx, log, cand, pending, runID, err) + return + } +} + +// executePRComments is the happy path for one PR-comment run. +func (o *Orchestrator) executePRComments(ctx context.Context, log *slog.Logger, cand candidate, pr gh.PullRequest, + pending []gh.PRComment, runID, logPath string) error { + cfg := o.opts.Config + ref := cand.ref(runID, 1) + started := time.Now() + + meta, err := o.repoMetadata(ctx, cand.repo) + if err != nil { + return fmt.Errorf("repo metadata: %w", err) + } + + repoPath, err := o.opts.Git.EnsureRepo(ctx, cand.repo, meta.cloneURL) + if err != nil { + return fmt.Errorf("prepare clone: %w", err) + } + if err := o.opts.Git.AssertRemote(ctx, repoPath, cand.repo); err != nil { + return err + } + + worktree := o.opts.Git.WorktreePath(cand.repo, cand.number) + // Both branch and base are the PR's own head branch: this checks out the + // PR head as-is rather than resetting it against the default branch, and + // a retry after a human pushed more commits to it picks those up too. + if err := o.opts.Git.AddWorktree(ctx, repoPath, worktree, pr.HeadRefName, pr.HeadRefName); err != nil { + return fmt.Errorf("create worktree: %w", err) + } + o.event(ctx, runID, "worktree", worktree) + + cooled, err := o.opts.Store.CooledDownModels(ctx) + if err != nil { + log.Warn("cooldown lookup failed, using full ladder", "error", err) + cooled = nil + } + ladder := o.opts.Registry.Ladder(models.RoleImplement, cooled) + if len(ladder) > 0 { + if drop := maxAttempts(ctx, o.opts.Store, cand.repo, cand.number, pending) % len(ladder); drop > 0 { + ladder = ladder[drop:] + } + } + head, fallbacks, err := models.Head(ladder) + if err != nil { + return fmt.Errorf("select model: %w", err) + } + if err := o.opts.Store.RecordUsage(ctx, runID, head.ID, "", 0, 0, 0, 0); err != nil { + log.Warn("could not pre-record model", "error", err) + } + if err := o.opts.Store.SetRunStatus(ctx, runID, store.StatusWorking); err != nil { + log.Warn("status update failed", "error", err) + } + o.event(ctx, runID, "model", fmt.Sprintf("%s (fallbacks: %s)", head.ID, orNone(fallbacks))) + log.Info("starting claude", "model", head.ID, "branch", pr.HeadRefName) + + reviews, err := o.opts.GH.PRReviewBodies(ctx, cand.repo, cand.number) + if err != nil { + log.Warn("could not fetch review summaries, continuing without them", "error", err) + } + + prompt := prCommentTaskPrompt(cand.repo, pr, pending, reviews) + sysPrompt := prCommentSystemPrompt(cand.repo, pr.HeadRefName, worktree) + + var sessionOnce sync.Once + result, runErr := o.opts.Runner.Run(ctx, claude.Options{ + Binary: cfg.Claude.Binary, + Prompt: prompt, + SystemPrompt: sysPrompt, + Model: head.Ref(), + Fallbacks: fallbacks, + PermissionMode: cfg.Claude.PermissionMode, + WorkDir: worktree, + ExtraArgs: cfg.Claude.ExtraArgs, + LogPath: logPath, + Timeout: cfg.Run.Timeout.D(), + OnEvent: func(_ string, raw json.RawMessage) { + var probe struct { + SessionID string `json:"session_id"` + } + if err := json.Unmarshal(raw, &probe); err != nil || probe.SessionID == "" { + return + } + sessionOnce.Do(func() { + o.recordSession(ctx, log, cand, runID, probe.SessionID, head.ID) + o.event(ctx, runID, "session", probe.SessionID) + log.Info("claude session started", "session", probe.SessionID) + }) + }, + }) + + usedModel := head.ID + if result != nil { + if m := result.PrimaryModel(); m != "" { + usedModel = m + } + if err := o.opts.Store.RecordUsage(ctx, runID, usedModel, result.SessionID, + result.TotalCostUSD, result.TokensIn(), result.TokensOut(), result.NumTurns); err != nil { + log.Warn("usage record failed", "error", err) + } + o.recordSession(ctx, log, cand, runID, result.SessionID, usedModel) + } + + if runErr != nil { + if hit, limited := gate.DetectLimit(result, runErr); limited { + until, gerr := o.opts.Gate.RecordLimit(ctx, hit) + if gerr != nil { + log.Error("could not record usage limit", "error", gerr) + } + if err := o.opts.Gate.CoolDownModel(ctx, head.ID, modelCooldown, hit.Reason); err != nil { + log.Warn("could not cool down model", "error", err) + } else { + o.opts.Discord.ModelCooledDown(head.ID, time.Now().Add(modelCooldown), hit.Reason) + } + o.event(ctx, runID, "usage_limit", hit.Reason) + o.opts.Discord.GateClosed(hit.Reason, until) + return errRetryable{fmt.Errorf("usage limit reached, paused until %s: %s", + until.Format(time.RFC3339), hit.Reason)} + } + if err := o.opts.Gate.CoolDownModel(ctx, head.ID, modelCooldown, "run failed"); err != nil { + log.Warn("could not cool down model", "error", err) + } else { + o.opts.Discord.ModelCooledDown(head.ID, time.Now().Add(modelCooldown), "run failed") + } + return fmt.Errorf("claude run failed: %w", runErr) + } + + if err := o.opts.Gate.RecordSuccess(ctx); err != nil { + log.Warn("could not clear usage gate", "error", err) + } + o.opts.Discord.GateCleared() + o.event(ctx, runID, "claude_done", fmt.Sprintf("turns=%d cost=$%.4f", result.NumTurns, result.TotalCostUSD)) + + hasWork, err := o.opts.Git.HasWork(ctx, worktree, pr.HeadRefName) + if err != nil { + return fmt.Errorf("inspect worktree: %w", err) + } + + pushed := false + var vres verify.Result + if hasWork { + commitMsg := fmt.Sprintf("Address review feedback on #%d\n\nGenerated by coding-agent-loop run %s.", + cand.number, runID) + if committed, err := o.opts.Git.CommitAll(ctx, worktree, commitMsg); err != nil { + return fmt.Errorf("commit changes: %w", err) + } else if committed { + o.event(ctx, runID, "committed", "harness committed the agent's working tree") + } + + if err := o.opts.Store.SetRunStatus(ctx, runID, store.StatusVerifying); err != nil { + log.Warn("status update failed", "error", err) + } + vres = o.opts.Verify.Run(ctx, cand.repo, worktree) + if err := o.opts.Store.SetVerifyStatus(ctx, runID, vres.Status); err != nil { + log.Warn("verify status update failed", "error", err) + } + o.event(ctx, runID, "verify", fmt.Sprintf("%s (%s)", vres.Status, orNone(vres.Command))) + log.Info("verification finished", "status", vres.Status, "command", vres.Command) + + if err := o.opts.Git.Push(ctx, worktree, pr.HeadRefName, cand.repo); err != nil { + return fmt.Errorf("push branch: %w", err) + } + if err := o.opts.Store.SetRunStatus(ctx, runID, store.StatusPushed); err != nil { + log.Warn("status update failed", "error", err) + } + o.event(ctx, runID, "pushed", pr.HeadRefName) + pushed = true + } else { + // No code changes is a legitimate outcome here: a question answered in + // prose is not a failure. + o.event(ctx, runID, "no_work", "the agent answered in its summary without changing the worktree") + } + + if err := o.opts.GH.CommentOnPR(ctx, cand.repo, cand.number, + prCommentComment(pending, result.Result, runID, vres, pushed)); err != nil { + log.Warn("could not comment on pull request", "error", err) + } + for _, c := range pending { + if err := o.opts.GH.React(ctx, cand.repo, c, o.opts.Config.GitHub.PRComments.DoneReaction); err != nil { + log.Warn("could not react to comment as done", "comment", c.ID, "error", err) + } + if err := o.opts.Store.MarkPRCommentDone(ctx, c.Kind, c.ID, runID); err != nil { + log.Warn("could not record comment as done", "comment", c.ID, "error", err) + } + } + + if err := o.opts.Store.SetRunStatus(ctx, runID, store.StatusAddressed); err != nil { + log.Warn("status update failed", "error", err) + } + o.event(ctx, runID, "addressed", fmt.Sprintf("%d comment(s)", len(pending))) + o.opts.Discord.PRCommentsAddressed(ref, len(pending), result, vres, time.Since(started)) + log.Info("pr review feedback addressed", "comments", len(pending), "pushed", pushed, "cost_usd", result.TotalCostUSD) + + o.cleanup(ctx, log, repoPath, worktree, true) + return nil +} + +// maxAttempts is the highest attempt count among the tasks backing pending, +// used to demote the model ladder the same way a repeatedly-failing issue +// does. +func maxAttempts(ctx context.Context, st *store.Store, repo string, pr int, pending []gh.PRComment) int { + tasks, err := st.PRCommentTasks(ctx, repo, pr) + if err != nil { + return 0 + } + byKey := make(map[string]store.PRCommentTask, len(tasks)) + for _, t := range tasks { + byKey[prCommentTaskKey(t.CommentKind, t.CommentID)] = t + } + max := 0 + for _, c := range pending { + if t, ok := byKey[prCommentTaskKey(c.Kind, c.ID)]; ok && t.Attempts > max { + max = t.Attempts + } + } + return max +} + +// handlePRCommentFailure records a failed PR-comment run. The ack reaction +// stays: the comment was seen, and the per-comment back-off decides when it +// is retried. +func (o *Orchestrator) handlePRCommentFailure(ctx context.Context, log *slog.Logger, cand candidate, + pending []gh.PRComment, runID string, cause error) { + ctx = context.WithoutCancel(ctx) + cfg := o.opts.Config.Run + + var retryable errRetryable + if errors.As(cause, &retryable) { + log.Warn("pr comment run deferred by usage limit", "error", cause) + if err := o.opts.Store.FailRun(ctx, runID, store.StatusDeferred, cause.Error()); err != nil { + log.Error("could not record deferral", "error", err) + } + o.event(ctx, runID, "deferred", cause.Error()) + o.opts.Discord.RunDeferred(cand.ref(runID, 1), cause.Error()) + return + } + + log.Error("pr comment run failed", "error", cause) + if err := o.opts.Store.FailRun(ctx, runID, store.StatusFailed, cause.Error()); err != nil { + log.Error("could not record failure", "error", err) + } + o.event(ctx, runID, "failed", cause.Error()) + + var nextAttempt time.Time + for _, c := range pending { + if err := o.opts.Store.MarkPRCommentFailed(ctx, c.Kind, c.ID, runID); err != nil { + log.Warn("could not record comment failure", "comment", c.ID, "error", err) + continue + } + tasks, err := o.opts.Store.PRCommentTasks(ctx, cand.repo, cand.number) + if err != nil { + continue + } + for _, t := range tasks { + if t.CommentKind != c.Kind || t.CommentID != c.ID { + continue + } + next := t.LastAttemptAt.Add(retryDelay(t.Attempts, cfg.RetryBackoff.D(), cfg.RetryBackoffMax.D())) + if nextAttempt.IsZero() || next.Before(nextAttempt) { + nextAttempt = next + } + } + } + + if err := o.opts.GH.CommentOnPR(ctx, cand.repo, cand.number, + prCommentFailureComment(runID, cause.Error(), nextAttempt)); err != nil { + log.Warn("could not comment on pull request", "error", err) + } + o.opts.Discord.RunFailed(cand.ref(runID, 1), cause.Error(), nextAttempt) + + o.finishCleanup(ctx, log, cand) +} diff --git a/internal/orchestrator/prcomments_test.go b/internal/orchestrator/prcomments_test.go new file mode 100644 index 0000000..3bfa1a6 --- /dev/null +++ b/internal/orchestrator/prcomments_test.go @@ -0,0 +1,146 @@ +package orchestrator + +import ( + "testing" + "time" + + "github.com/ableinc/coding-agent-loop/internal/config" + "github.com/ableinc/coding-agent-loop/internal/gh" + "github.com/ableinc/coding-agent-loop/internal/store" +) + +func TestMentionsAgent(t *testing.T) { + const handle = "@coding-agent" + tests := []struct { + name string + body string + want bool + }{ + {"plain", "@coding-agent please rename X to Y", true}, + {"mid-sentence", "hey @coding-agent can you take a look at this?", true}, + {"case-insensitive", "@Coding-Agent could you fix this", true}, + {"different-handle-non-match", "please check the @coding-agent-loop repository settings", false}, + {"quoted-line-ignored", "> @coding-agent do the thing\nI disagree with this quote", false}, + {"fenced-code-block-ignored", "example usage:\n```\n@coding-agent fix this\n```\nno request here", false}, + {"absent", "this comment does not mention anyone", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := mentionsAgent(tc.body, handle); got != tc.want { + t.Errorf("mentionsAgent(%q) = %v, want %v", tc.body, got, tc.want) + } + }) + } +} + +func defaultPRCommentsConfig() config.PRCommentsConfig { + return config.PRCommentsConfig{ + Enabled: true, + Mention: "@coding-agent", + SearchLimit: 30, + MaxAge: config.Duration(7 * 24 * time.Hour), + AckReaction: "eyes", + DoneReaction: "+1", + AllowedAssociations: []string{"OWNER", "MEMBER", "COLLABORATOR"}, + } +} + +func TestPendingMentions(t *testing.T) { + now := time.Now() + cfg := defaultPRCommentsConfig() + base, max := 15*time.Minute, 24*time.Hour + + mk := func(id int64, author, assoc, body string, age time.Duration) gh.PRComment { + return gh.PRComment{ + ID: id, Kind: gh.CommentKindIssue, Author: author, Association: assoc, + Body: body, CreatedAt: now.Add(-age), + } + } + + comments := []gh.PRComment{ + mk(1, "alice", "OWNER", "@coding-agent please fix the typo", time.Minute), + mk(2, "mallory", "NONE", "@coding-agent do something", time.Minute), // disallowed association + mk(3, "alice", "OWNER", "@coding-agent this one is old", 200*time.Hour), // too old + mk(4, "alice", "OWNER", "no mention here", time.Minute), + mk(5, "alice", "OWNER", markerPRComment+"\nalready ours", time.Minute), // our own marker + mk(6, "bot-login", "OWNER", "@coding-agent from myself", time.Minute), // the daemon's own comment + mk(7, "alice", "OWNER", "@coding-agent already done", time.Minute), + mk(8, "alice", "OWNER", "@coding-agent failed recently", time.Minute), + mk(9, "alice", "OWNER", "@coding-agent failed a while ago", time.Minute), + } + + tasks := []store.PRCommentTask{ + {CommentKind: gh.CommentKindIssue, CommentID: 7, Status: store.PRCommentDone}, + {CommentKind: gh.CommentKindIssue, CommentID: 8, Status: store.PRCommentFailed, Attempts: 1, LastAttemptAt: now.Add(-time.Minute)}, + {CommentKind: gh.CommentKindIssue, CommentID: 9, Status: store.PRCommentFailed, Attempts: 1, LastAttemptAt: now.Add(-time.Hour)}, + } + + got := pendingMentions(comments, cfg, "bot-login", tasks, now, base, max) + + ids := map[int64]bool{} + for _, c := range got { + ids[c.ID] = true + } + + if !ids[1] { + t.Error("an unseen, allowed, fresh mention should be pending") + } + if ids[2] { + t.Error("a disallowed association must not be pending") + } + if ids[3] { + t.Error("a comment older than max_age must not be pending") + } + if ids[4] { + t.Error("a comment without a mention must not be pending") + } + if ids[5] { + t.Error("the daemon's own marker comment must never be read back as a mention") + } + if ids[6] { + t.Error("the daemon's own comment must never trigger itself") + } + if ids[7] { + t.Error("a comment already marked done must not be pending again") + } + if ids[8] { + t.Error("a failed comment still inside its back-off must not be pending") + } + if !ids[9] { + t.Error("a failed comment whose back-off elapsed should be pending again") + } +} + +func TestPendingMentionsRetriesStrandedAck(t *testing.T) { + now := time.Now() + cfg := defaultPRCommentsConfig() + comments := []gh.PRComment{ + {ID: 1, Kind: gh.CommentKindIssue, Author: "alice", Association: "OWNER", + Body: "@coding-agent please fix this", CreatedAt: now.Add(-time.Minute)}, + } + tasks := []store.PRCommentTask{ + {CommentKind: gh.CommentKindIssue, CommentID: 1, Status: store.PRCommentAcked}, + } + got := pendingMentions(comments, cfg, "bot-login", tasks, now, 15*time.Minute, 24*time.Hour) + if len(got) != 1 { + t.Fatalf("a comment stranded in 'acked' by a crash must be retried, got %+v", got) + } +} + +func TestAuthorAllowed(t *testing.T) { + cfg := defaultPRCommentsConfig() + if !authorAllowed("alice", "OWNER", cfg) { + t.Error("OWNER should be allowed by default") + } + if authorAllowed("mallory", "NONE", cfg) { + t.Error("NONE should not be allowed by default") + } + + cfg.AllowedAuthors = []string{"bob"} + if authorAllowed("alice", "OWNER", cfg) { + t.Error("an explicit author allowlist should override the association fallback") + } + if !authorAllowed("bob", "NONE", cfg) { + t.Error("an allowlisted author should be allowed regardless of association") + } +} diff --git a/internal/orchestrator/prompt.go b/internal/orchestrator/prompt.go index f10739f..b0af202 100644 --- a/internal/orchestrator/prompt.go +++ b/internal/orchestrator/prompt.go @@ -90,6 +90,98 @@ final message should be the plan itself, headed by a short one-line summary of t change.`, repo, worktree) } +// prCommentSystemPrompt states the harness contract for addressing review +// feedback on an already-open pull request: the same rules as systemPrompt, +// reframed around a branch and PR that already exist rather than one about +// to be created. +func prCommentSystemPrompt(repo, branch, worktree string) string { + return fmt.Sprintf(`You are running unattended as an automated coding agent. + +Working context: +- Repository: %s +- Branch: %s (already created and checked out for you; an open pull request already exists from it) +- Worktree: %s + +You are addressing reviewer feedback left as comments on that open pull request. Keep +the change to what the feedback below asks for; do not re-litigate the rest of the PR. + +The harness, not you, owns version control and GitHub. Specifically: +- Do NOT run git push, git rebase, git reset --hard, or any force operation. +- Do NOT create branches, tags, pull requests, or comments. +- Do NOT amend or rewrite any commit that already exists. +- You MAY commit your work locally. If you do not, the harness commits it for you. + +Scope rules: +- Confine all edits to the worktree above. Do not modify files elsewhere on this machine. +- Do not modify CI configuration, deployment manifests, or anything holding credentials. +- Address every comment listed below, at the scope it asks for. Do not opportunistically + refactor, reformat, or "clean up" code the comments did not ask you to touch. +- Follow the conventions already present in the repository. + +Autonomy: +- Nobody is watching and nobody can answer a question mid-task. Do not ask for + confirmation and do not end your turn with a proposal you did not carry out. +- If a comment is a question rather than a change request, answer it in your final + summary instead of editing code for it. +- If you conclude a requested change should not be made, make no edits and explain why + in your final summary. + +Finish with a short summary addressing each comment in turn: what you changed (or why +you didn't), and anything a reviewer should look at closely. This summary is posted +back to the pull request.`, repo, branch, worktree) +} + +// prCommentTaskPrompt renders the PR and its triggering comments into the +// instruction for the review-feedback pass. reviews are review summary +// bodies, included as context only: they cannot be reacted to, so they never +// appear in comments. +func prCommentTaskPrompt(repo string, pr gh.PullRequest, comments []gh.PRComment, reviews []string) string { + var b strings.Builder + + fmt.Fprintf(&b, "Address reviewer feedback on pull request #%d in %s.\n\n", pr.Number, repo) + fmt.Fprintf(&b, "## Pull request #%d: %s\n\n", pr.Number, pr.Title) + if pr.URL != "" { + fmt.Fprintf(&b, "<%s>\n\n", pr.URL) + } + body := strings.TrimSpace(pr.Body) + if body != "" { + fmt.Fprintf(&b, "%s\n\n", truncate(body, maxBodyChars)) + } + + b.WriteString("### Comments to address\n\n") + for _, c := range comments { + author := c.Author + if author == "" { + author = "unknown" + } + fmt.Fprintf(&b, "**@%s**: %s\n\n", author, truncate(strings.TrimSpace(c.Body), maxCommentChars)) + if c.Path != "" { + fmt.Fprintf(&b, "On `%s`", c.Path) + if c.Line > 0 { + fmt.Fprintf(&b, " (line %d)", c.Line) + } + b.WriteString(":\n\n") + if c.DiffHunk != "" { + b.WriteString("```diff\n") + b.WriteString(truncate(c.DiffHunk, maxCommentChars)) + b.WriteString("\n```\n\n") + } + } + } + + if len(reviews) > 0 { + b.WriteString("### Review summaries (context only)\n\n") + for _, r := range reviews { + fmt.Fprintf(&b, "%s\n\n", truncate(strings.TrimSpace(r), maxCommentChars)) + } + } + + b.WriteString("\nAddress every comment listed above. If one is a question rather than a change " + + "request, answer it in your final summary instead of editing code for it. Read the relevant " + + "parts of the repository before editing, then make the change.\n") + return b.String() +} + // issueContext renders the issue itself: title, labels, description, and // recent discussion. Shared by the plan and implement prompts so the two // phases see the same view of the issue. diff --git a/internal/orchestrator/prompt_test.go b/internal/orchestrator/prompt_test.go index 00af875..652359d 100644 --- a/internal/orchestrator/prompt_test.go +++ b/internal/orchestrator/prompt_test.go @@ -60,3 +60,39 @@ func TestPlanSystemPromptForbidsEditing(t *testing.T) { } } } + +func TestPRCommentTaskPromptIncludesEveryCommentAndDiffHunk(t *testing.T) { + pr := gh.PullRequest{Number: 12, Title: "Add retry logic", URL: "https://github.com/acme/widgets/pull/12"} + comments := []gh.PRComment{ + {Kind: gh.CommentKindIssue, Author: "alice", Body: "please rename Foo to Bar"}, + {Kind: gh.CommentKindReview, Author: "bob", Body: "this is unsafe", + Path: "main.go", Line: 42, DiffHunk: "@@ -1,3 +1,3 @@\n-old\n+new"}, + } + p := prCommentTaskPrompt("acme/widgets", pr, comments, []string{"looks good overall"}) + + for _, want := range []string{ + "#12", "Add retry logic", "please rename Foo to Bar", "@bob", "this is unsafe", + "main.go", "42", "@@ -1,3 +1,3 @@", "looks good overall", + } { + if !strings.Contains(p, want) { + t.Errorf("pr comment task prompt missing %q:\n%s", want, p) + } + } +} + +func TestPRCommentTaskPromptWithoutReviews(t *testing.T) { + pr := gh.PullRequest{Number: 12, Title: "Add retry logic"} + p := prCommentTaskPrompt("acme/widgets", pr, []gh.PRComment{{Author: "alice", Body: "fix this"}}, nil) + if strings.Contains(p, "Review summaries") { + t.Fatalf("no review section should appear when there are no reviews:\n%s", p) + } +} + +func TestPRCommentSystemPromptForbidsCreatingPRs(t *testing.T) { + p := prCommentSystemPrompt("acme/widgets", "agent/issue-9", "/work/widgets/issue-9") + for _, want := range []string{"Do NOT create branches, tags, pull requests", "already exists", "final summary"} { + if !strings.Contains(p, want) { + t.Errorf("pr comment system prompt missing %q:\n%s", want, p) + } + } +} diff --git a/internal/orchestrator/report.go b/internal/orchestrator/report.go index 95399b9..184719b 100644 --- a/internal/orchestrator/report.go +++ b/internal/orchestrator/report.go @@ -134,6 +134,67 @@ func failureComment(runID string, attempt int, reason string, nextAttempt time.T return b.String() } +// prCommentComment is the reply posted on a pull request once its triggering +// comments have been addressed: one line per comment linking back to it, the +// agent's own summary, and the verification outcome. +func prCommentComment(handled []gh.PRComment, summary, runID string, v verify.Result, pushed bool) string { + var b strings.Builder + b.WriteString(markerPRComment) + b.WriteString("\n\n") + + b.WriteString("Addressed the following comment") + if len(handled) != 1 { + b.WriteString("s") + } + b.WriteString(":\n\n") + for _, c := range handled { + fmt.Fprintf(&b, "- %s\n", c.URL) + } + b.WriteString("\n") + + s := strings.TrimSpace(summary) + if s == "" { + s = "_The agent produced no closing summary._" + } + b.WriteString(s) + b.WriteString("\n\n") + + if pushed { + switch v.Status { + case store.VerifyPassed: + fmt.Fprintf(&b, "Tests passed (`%s`).\n", v.Command) + case store.VerifyFailed: + fmt.Fprintf(&b, "Tests failed (`%s`).\n", v.Command) + default: + b.WriteString("No test command was detected for this repository.\n") + } + } else { + b.WriteString("No code changes were needed.\n") + } + fmt.Fprintf(&b, "\ncoding-agent-loop run `%s`\n", runID) + return b.String() +} + +// prCommentFailureComment explains an unsuccessful attempt at addressing PR +// review feedback. The 👀 reaction stays regardless: the comment was seen, and +// the next pass retries it after its own back-off. +func prCommentFailureComment(runID string, reason string, nextAttempt time.Time) string { + var b strings.Builder + b.WriteString(markerPRComment) + b.WriteString("\n\n") + b.WriteString("Could not address this review feedback.\n\n") + b.WriteString("```\n") + b.WriteString(strings.TrimSpace(reason)) + b.WriteString("\n```\n\n") + if nextAttempt.IsZero() { + b.WriteString("It will try again on a later pass.\n") + } else { + fmt.Fprintf(&b, "It will try again after %s.\n", nextAttempt.UTC().Format(time.RFC1123)) + } + fmt.Fprintf(&b, "\ncoding-agent-loop run `%s`\n", runID) + return b.String() +} + // maxPlanCommentChars keeps a plan comment under GitHub's ~65536-character // comment body limit, leaving room for the marker and footer. const maxPlanCommentChars = 60000 diff --git a/internal/orchestrator/report_test.go b/internal/orchestrator/report_test.go new file mode 100644 index 0000000..6ac1ebc --- /dev/null +++ b/internal/orchestrator/report_test.go @@ -0,0 +1,60 @@ +package orchestrator + +import ( + "strings" + "testing" + "time" + + "github.com/ableinc/coding-agent-loop/internal/gh" + "github.com/ableinc/coding-agent-loop/internal/store" + "github.com/ableinc/coding-agent-loop/internal/verify" +) + +func TestPRCommentCommentCarriesMarkerAndHandledURLs(t *testing.T) { + handled := []gh.PRComment{ + {ID: 1, URL: "https://github.com/acme/widgets/pull/9#issuecomment-1"}, + {ID: 2, URL: "https://github.com/acme/widgets/pull/9#discussion_r2"}, + } + body := prCommentComment(handled, "Renamed Foo to Bar as requested.", "run-1", + verify.Result{Status: store.VerifyPassed, Command: "go test ./..."}, true) + + if !strings.Contains(body, markerPRComment) { + t.Fatalf("reply must carry the pr-comment marker so it is never read back as a mention:\n%s", body) + } + for _, c := range handled { + if !strings.Contains(body, c.URL) { + t.Errorf("reply missing handled comment URL %q:\n%s", c.URL, body) + } + } + if !strings.Contains(body, "Renamed Foo to Bar") { + t.Fatalf("reply should carry the agent's summary:\n%s", body) + } + if !strings.Contains(body, "run-1") { + t.Fatalf("reply should carry the run id:\n%s", body) + } + if !strings.Contains(body, "Tests passed") { + t.Fatalf("reply should report verification when code was pushed:\n%s", body) + } +} + +func TestPRCommentCommentWithoutAPush(t *testing.T) { + handled := []gh.PRComment{{ID: 1, URL: "u1"}} + body := prCommentComment(handled, "It already works as-is.", "run-1", verify.Result{}, false) + if !strings.Contains(body, "No code changes were needed") { + t.Fatalf("a question answered in prose should say no code changed:\n%s", body) + } +} + +func TestPRCommentFailureCommentCarriesMarkerAndReason(t *testing.T) { + next := time.Now().Add(30 * time.Minute) + body := prCommentFailureComment("run-2", "claude run failed: boom", next) + if !strings.Contains(body, markerPRComment) { + t.Fatalf("failure reply must carry the pr-comment marker:\n%s", body) + } + if !strings.Contains(body, "boom") { + t.Fatalf("failure reply should carry the reason:\n%s", body) + } + if !strings.Contains(body, "run-2") { + t.Fatalf("failure reply should carry the run id:\n%s", body) + } +} diff --git a/internal/store/store.go b/internal/store/store.go index 52db8f3..8baeff5 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -30,6 +30,23 @@ const ( StatusCanceled = "canceled" // terminal, operator cancelled StatusDeferred = "deferred" // terminal, stopped by the usage gate; not the issue's fault StatusPlanned = "planned" // terminal, a plan was posted and awaits human approval + StatusAddressed = "addressed" // terminal, success: a PR review comment was acted on +) + +// Run kinds distinguish an issue-driven run from one triggered by a PR review +// comment. Both share the runs table and the claim/lease machinery — GitHub +// numbers PRs in the same sequence as issues, so a PR number is just another +// "issue" as far as claims are concerned. +const ( + RunKindIssue = "issue" + RunKindPRComment = "pr_comment" +) + +// PR comment task statuses. +const ( + PRCommentAcked = "acked" + PRCommentDone = "done" + PRCommentFailed = "failed" ) // Verify outcomes recorded on a run. @@ -49,23 +66,25 @@ const ( // IsTerminal reports whether a run status is final. func IsTerminal(status string) bool { switch status { - case StatusPROpen, StatusFailed, StatusAbandoned, StatusCanceled, StatusDeferred, StatusPlanned: + case StatusPROpen, StatusFailed, StatusAbandoned, StatusCanceled, StatusDeferred, StatusPlanned, StatusAddressed: return true } return false } -// Run is one attempt at one issue. +// Run is one attempt at one issue, or at one PR-comment task. type Run struct { - ID string - Repo string - Issue int - CreatedAt time.Time - Attempt int - ModelID string - Branch string - PRURL string - Status string + ID string + Repo string + Issue int + CreatedAt time.Time + Attempt int + ModelID string + Branch string + PRURL string + Status string + // Kind is RunKindIssue or RunKindPRComment. + Kind string StartedAt time.Time EndedAt time.Time CostUSD float64 @@ -251,6 +270,27 @@ var migrations = []string{ created_at INTEGER NOT NULL, PRIMARY KEY (repo, issue) );`, + + // Distinguishes an issue-driven run from one triggered by a PR review + // comment, and tracks each triggering comment through ack -> done/failed + // so a restart between the 👀 and the run retries it instead of stranding + // it, and so a failed comment gets its own back-off. + `ALTER TABLE runs ADD COLUMN kind TEXT NOT NULL DEFAULT 'issue'; + + CREATE TABLE IF NOT EXISTS pr_comment_tasks ( + repo TEXT NOT NULL, + pr INTEGER NOT NULL, + comment_kind TEXT NOT NULL, + comment_id INTEGER NOT NULL, + status TEXT NOT NULL, + attempts INTEGER NOT NULL DEFAULT 0, + last_attempt_at INTEGER NOT NULL DEFAULT 0, + run_id TEXT NOT NULL DEFAULT '', + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + PRIMARY KEY (comment_kind, comment_id) + ); + CREATE INDEX IF NOT EXISTS pr_comment_tasks_pr ON pr_comment_tasks(repo, pr);`, } func (s *Store) migrate(ctx context.Context) error { @@ -367,15 +407,19 @@ func (s *Store) ActiveClaims(ctx context.Context) ([]Claim, error) { // --- runs ------------------------------------------------------------------- -// CreateRun inserts a new run row. CreatedAt defaults to now. +// CreateRun inserts a new run row. CreatedAt defaults to now, and Kind +// defaults to RunKindIssue. func (s *Store) CreateRun(ctx context.Context, r Run) error { if r.CreatedAt.IsZero() { r.CreatedAt = time.Now() } + if r.Kind == "" { + r.Kind = RunKindIssue + } _, err := s.db.ExecContext(ctx, ` - INSERT INTO runs (id, repo, issue, attempt, model_id, branch, status, created_at, started_at, log_path) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, - r.ID, r.Repo, r.Issue, r.Attempt, r.ModelID, r.Branch, r.Status, + INSERT INTO runs (id, repo, issue, attempt, model_id, branch, status, kind, created_at, started_at, log_path) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + r.ID, r.Repo, r.Issue, r.Attempt, r.ModelID, r.Branch, r.Status, r.Kind, r.CreatedAt.Unix(), r.StartedAt.Unix(), r.LogPath) if err != nil { return fmt.Errorf("create run %s: %w", r.ID, err) @@ -525,13 +569,13 @@ var errNoRows = errors.New("not found") // ErrNotFound is returned when a lookup finds nothing. var ErrNotFound = errNoRows -const runColumns = `id, repo, issue, attempt, model_id, branch, pr_url, status, created_at, started_at, ended_at, +const runColumns = `id, repo, issue, attempt, model_id, branch, pr_url, status, kind, created_at, started_at, ended_at, cost_usd, tokens_in, tokens_out, num_turns, session_id, verify_status, error, log_path` func scanRun(sc interface{ Scan(...any) error }) (Run, error) { var r Run var created, started, ended int64 - err := sc.Scan(&r.ID, &r.Repo, &r.Issue, &r.Attempt, &r.ModelID, &r.Branch, &r.PRURL, &r.Status, + err := sc.Scan(&r.ID, &r.Repo, &r.Issue, &r.Attempt, &r.ModelID, &r.Branch, &r.PRURL, &r.Status, &r.Kind, &created, &started, &ended, &r.CostUSD, &r.TokensIn, &r.TokensOut, &r.NumTurns, &r.SessionID, &r.VerifyStatus, &r.Error, &r.LogPath) if err != nil { @@ -642,11 +686,11 @@ func (s *Store) IssueHistory(ctx context.Context, repo string, issue int) (Issue COALESCE(SUM(CASE WHEN status = ? THEN 1 ELSE 0 END), 0), COALESCE(SUM(CASE WHEN status IN (?, ?) THEN 1 ELSE 0 END), 0), COALESCE(MAX(CASE WHEN status IN (?, ?) THEN ended_at ELSE 0 END), 0) - FROM runs WHERE repo = ? AND issue = ?`, + FROM runs WHERE repo = ? AND issue = ? AND kind = ?`, StatusDeferred, StatusCanceled, StatusPROpen, StatusAbandoned, StatusFailed, StatusAbandoned, StatusFailed, StatusAbandoned, - repo, issue). + repo, issue, RunKindIssue). Scan(&st.Attempts, &succeeded, &abandoned, &st.Failures, &lastFailure) if err != nil { return st, fmt.Errorf("issue history %s#%d: %w", repo, issue, err) @@ -658,8 +702,8 @@ func (s *Store) IssueHistory(ctx context.Context, repo string, issue int) (Issue } row := s.db.QueryRowContext(ctx, - `SELECT status, pr_url FROM runs WHERE repo = ? AND issue = ? ORDER BY started_at DESC, rowid DESC LIMIT 1`, - repo, issue) + `SELECT status, pr_url FROM runs WHERE repo = ? AND issue = ? AND kind = ? ORDER BY started_at DESC, rowid DESC LIMIT 1`, + repo, issue, RunKindIssue) if err := row.Scan(&st.LastStatus, &st.LastPRURL); err != nil && !errors.Is(err, sql.ErrNoRows) { return st, fmt.Errorf("issue history %s#%d: %w", repo, issue, err) } @@ -700,6 +744,100 @@ func (s *Store) LatestPlan(ctx context.Context, repo string, issue int) (string, return body, nil } +// --- pr comment tasks --------------------------------------------------- + +// PRCommentTask tracks one triggering PR comment through ack -> done/failed. +// It is the only thing that survives a crash between reacting 👀 and running +// Claude, and it gives each comment its own attempt count and back-off, +// independent of who else may have reacted to it. +type PRCommentTask struct { + Repo string + PR int + CommentKind string + CommentID int64 + Status string + Attempts int + LastAttemptAt time.Time + RunID string + CreatedAt time.Time + UpdatedAt time.Time +} + +// MarkPRCommentAcked records that a comment has been seen and 👀'd. It is +// safe to call again for a comment that failed a previous attempt: the +// PRIMARY KEY upsert takes it back to "acked" and preserves its attempt count. +func (s *Store) MarkPRCommentAcked(ctx context.Context, repo string, pr int, kind string, commentID int64, runID string) error { + now := time.Now().Unix() + _, err := s.db.ExecContext(ctx, ` + INSERT INTO pr_comment_tasks (repo, pr, comment_kind, comment_id, status, run_id, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT(comment_kind, comment_id) DO UPDATE SET + status = excluded.status, + run_id = excluded.run_id, + updated_at = excluded.updated_at`, + repo, pr, kind, commentID, PRCommentAcked, runID, now, now) + if err != nil { + return fmt.Errorf("ack pr comment %s/%d: %w", kind, commentID, err) + } + return nil +} + +// MarkPRCommentDone records that a comment has been addressed. +func (s *Store) MarkPRCommentDone(ctx context.Context, kind string, commentID int64, runID string) error { + _, err := s.db.ExecContext(ctx, ` + UPDATE pr_comment_tasks SET status = ?, run_id = ?, updated_at = ? + WHERE comment_kind = ? AND comment_id = ?`, + PRCommentDone, runID, time.Now().Unix(), kind, commentID) + if err != nil { + return fmt.Errorf("mark pr comment done %s/%d: %w", kind, commentID, err) + } + return nil +} + +// MarkPRCommentFailed records a failed attempt at a comment, bumping its +// attempt count and stamping when the attempt happened, which is what the +// per-comment back-off is measured from. +func (s *Store) MarkPRCommentFailed(ctx context.Context, kind string, commentID int64, runID string) error { + now := time.Now().Unix() + _, err := s.db.ExecContext(ctx, ` + UPDATE pr_comment_tasks SET status = ?, run_id = ?, attempts = attempts + 1, + last_attempt_at = ?, updated_at = ? + WHERE comment_kind = ? AND comment_id = ?`, + PRCommentFailed, runID, now, now, kind, commentID) + if err != nil { + return fmt.Errorf("mark pr comment failed %s/%d: %w", kind, commentID, err) + } + return nil +} + +// PRCommentTasks returns every task recorded for one pull request. +func (s *Store) PRCommentTasks(ctx context.Context, repo string, pr int) ([]PRCommentTask, error) { + rows, err := s.db.QueryContext(ctx, ` + SELECT repo, pr, comment_kind, comment_id, status, attempts, last_attempt_at, run_id, created_at, updated_at + FROM pr_comment_tasks WHERE repo = ? AND pr = ?`, repo, pr) + if err != nil { + return nil, fmt.Errorf("pr comment tasks %s#%d: %w", repo, pr, err) + } + defer rows.Close() + + var out []PRCommentTask + for rows.Next() { + var t PRCommentTask + var lastAttempt, created, updated int64 + if err := rows.Scan(&t.Repo, &t.PR, &t.CommentKind, &t.CommentID, &t.Status, &t.Attempts, + &lastAttempt, &t.RunID, &created, &updated); err != nil { + return nil, fmt.Errorf("scan pr comment task: %w", err) + } + if lastAttempt > 0 { + t.LastAttemptAt = time.Unix(lastAttempt, 0) + } + t.CreatedAt = time.Unix(created, 0) + t.UpdatedAt = time.Unix(updated, 0) + out = append(out, t) + } + return out, rows.Err() +} + // --- gate ------------------------------------------------------------------- // SetGate closes a gate until the given time. diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 9cba2a4..8062d32 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -542,6 +542,125 @@ func TestStatusPlannedIsTerminalButNotSuccessOrFailure(t *testing.T) { } } +// A run created without a Kind defaults to RunKindIssue, so existing call +// sites that predate the PR-comment feature still get a sensible value. +func TestCreateRunDefaultsKindToIssue(t *testing.T) { + ctx := context.Background() + st := testStore(t) + + if err := st.CreateRun(ctx, Run{ID: "r1", Repo: "o/r", Issue: 1, Status: StatusClaimed, StartedAt: time.Now()}); err != nil { + t.Fatal(err) + } + got, err := st.GetRun(ctx, "r1") + if err != nil { + t.Fatal(err) + } + if got.Kind != RunKindIssue { + t.Fatalf("kind = %q, want %q", got.Kind, RunKindIssue) + } +} + +// PR-comment runs must never move an issue's own retry back-off: they share +// the runs table and the issue number space, but not the history. +func TestIssueHistoryIgnoresPRCommentRuns(t *testing.T) { + ctx := context.Background() + st := testStore(t) + + if err := st.CreateRun(ctx, Run{ID: "pr1", Repo: "o/r", Issue: 9, Kind: RunKindPRComment, + Status: StatusClaimed, StartedAt: time.Now()}); err != nil { + t.Fatal(err) + } + if err := st.FailRun(ctx, "pr1", StatusFailed, "boom"); err != nil { + t.Fatal(err) + } + + hist, err := st.IssueHistory(ctx, "o/r", 9) + if err != nil { + t.Fatal(err) + } + if hist.Attempts != 0 || hist.Failures != 0 { + t.Fatalf("a pr_comment run must not count toward issue history, got %+v", hist) + } +} + +func TestPRCommentTaskLifecycle(t *testing.T) { + ctx := context.Background() + st := testStore(t) + + if err := st.MarkPRCommentAcked(ctx, "o/r", 5, "issue", 100, "run-1"); err != nil { + t.Fatalf("ack: %v", err) + } + tasks, err := st.PRCommentTasks(ctx, "o/r", 5) + if err != nil { + t.Fatal(err) + } + if len(tasks) != 1 || tasks[0].Status != PRCommentAcked || tasks[0].Attempts != 0 { + t.Fatalf("unexpected tasks after ack: %+v", tasks) + } + + if err := st.MarkPRCommentFailed(ctx, "issue", 100, "run-1"); err != nil { + t.Fatalf("fail: %v", err) + } + tasks, err = st.PRCommentTasks(ctx, "o/r", 5) + if err != nil { + t.Fatal(err) + } + if tasks[0].Status != PRCommentFailed || tasks[0].Attempts != 1 || tasks[0].LastAttemptAt.IsZero() { + t.Fatalf("failure not recorded correctly: %+v", tasks[0]) + } + + // A retry re-acks the same comment, which must not reset its attempt count. + if err := st.MarkPRCommentAcked(ctx, "o/r", 5, "issue", 100, "run-2"); err != nil { + t.Fatalf("re-ack: %v", err) + } + if err := st.MarkPRCommentDone(ctx, "issue", 100, "run-2"); err != nil { + t.Fatalf("done: %v", err) + } + tasks, err = st.PRCommentTasks(ctx, "o/r", 5) + if err != nil { + t.Fatal(err) + } + if tasks[0].Status != PRCommentDone || tasks[0].Attempts != 1 || tasks[0].RunID != "run-2" { + t.Fatalf("unexpected final task state: %+v", tasks[0]) + } +} + +// The migration that adds runs.kind and pr_comment_tasks must apply cleanly +// to a database that predates it. +func TestMigrationAddsKindAndPRCommentTasks(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "state.db") + + st, err := Open(path) + if err != nil { + t.Fatalf("open: %v", err) + } + if err := st.CreateRun(ctx, Run{ID: "r1", Repo: "o/r", Issue: 1, Status: StatusClaimed, StartedAt: time.Now()}); err != nil { + t.Fatal(err) + } + if err := st.MarkPRCommentAcked(ctx, "o/r", 2, "issue", 1, "run-x"); err != nil { + t.Fatal(err) + } + st.Close() + + // Reopening re-runs migrate(), exercising it against an already-migrated + // database, which must be a no-op rather than an error. + st2, err := Open(path) + if err != nil { + t.Fatalf("reopen: %v", err) + } + defer st2.Close() + + got, err := st2.GetRun(ctx, "r1") + if err != nil || got.Kind != RunKindIssue { + t.Fatalf("kind not preserved across reopen: %+v, err=%v", got, err) + } + tasks, err := st2.PRCommentTasks(ctx, "o/r", 2) + if err != nil || len(tasks) != 1 { + t.Fatalf("pr comment task not preserved across reopen: %+v, err=%v", tasks, err) + } +} + // A run stopped from outside is nobody's fault. Counting it would back the // issue off for hours because someone restarted the daemon. func TestCancelledRunsAreNeitherAttemptsNorFailures(t *testing.T) {