diff --git a/README.md b/README.md index 20d4148..c77de25 100644 --- a/README.md +++ b/README.md @@ -474,21 +474,24 @@ To enable it: } ``` -Once enabled, every one of these posts an embed. Run notifications lead with the issue's own title, -linked, and carry the run ID and attempt number: +Once enabled, every one of these posts an embed. Every run notification's title links straight to +the GitHub issue, and carries an `Issue` field with the same link, the run ID, and attempt number — +one click reaches the issue from any message, even ones (like a failure or a cancellation) that +would otherwise show only its plain-text `owner/name#42`: - **Run claimed** — plus how many earlier attempts on that issue failed, when it is a retry. - **Claude finished** — model, session ID, turns, cost, tokens in/out, wall-clock duration. - **Verification** — passed, failed, or skipped, with the command and, on failure, the tail of the test output, so the channel says what broke without opening the PR. -- **Draft PR opened** — link, model, session ID, cost, verification status, total run duration, - diffstat. +- **Draft PR opened** — title links the PR (with the issue link kept in the `Issue` field), model, + session ID, cost, verification status, total run duration, diffstat. - **Run failed** — the cause and **when the next attempt is due** (retries are unbounded, so "when" is the useful number). - **Run abandoned** — a run that was skipped rather than attempted: the issue closed, lost its label, or is already covered by a PR. - **Run deferred** — a run the usage gate stopped. Neither an attempt nor a failure. -- **Run cancelled** — `POST /runs/{id}/cancel`. +- **Run cancelled** — `POST /runs/{id}/cancel`, linking the issue when the run's repo/issue could + be looked up. - **Label update failed** — the labels on GitHub now disagree with the store, and which edit was refused. Otherwise invisible. - **Model cooled down** — which model, until when, and why, so a run served from lower on the ladder diff --git a/internal/discord/notifier.go b/internal/discord/notifier.go index 4d5d00f..96bec49 100644 --- a/internal/discord/notifier.go +++ b/internal/discord/notifier.go @@ -82,6 +82,7 @@ func (n *Notifier) Close(drain time.Duration) { type embed struct { Title string `json:"title"` Description string `json:"description,omitempty"` + URL string `json:"url,omitempty"` Color int `json:"color"` Fields []embedField `json:"fields,omitempty"` Timestamp string `json:"timestamp"` @@ -139,6 +140,22 @@ func (n *Notifier) post(e embed) { }() } +// postRun sends e to the webhook after making sure it carries a clickable +// link to the run's issue: the title becomes a link (unless e.URL is already +// set, e.g. to a PR) and an "Issue" field is always added. A RunRef with no +// repo/issue (e.g. a run row that could not be read) posts e unchanged. +func (n *Notifier) postRun(r RunRef, e embed) { + if u := r.issueURL(); u != "" { + if e.URL == "" { + e.URL = u + } + e.Fields = append(e.Fields, embedField{ + Name: "Issue", Value: fmt.Sprintf("[%s#%d](%s)", r.Repo, r.Issue, u), + }) + } + n.post(e) +} + func truncate(s string, max int) string { if len(s) <= max { return s @@ -180,6 +197,35 @@ func (r RunRef) description() string { } } +// issueURL is the link a notification should point at: the issue's own URL +// when known, otherwise one derived from repo and issue number. Derivation +// assumes github.com, which is correct everywhere this codebase talks to +// GitHub today. +func (r RunRef) issueURL() string { + if r.URL != "" { + return r.URL + } + if r.Repo != "" && r.Issue > 0 { + return fmt.Sprintf("https://github.com/%s/issues/%d", r.Repo, r.Issue) + } + return "" +} + +// describe combines the issue's own description with a notification's own +// message, so a notification that overwrites Description for a cause or +// reason does not lose the linked issue title in the process. +func (r RunRef) describe(body string) string { + d := r.description() + switch { + case body == "": + return d + case d == "": + return body + default: + return d + "\n" + body + } +} + func (r RunRef) fields() []embedField { return []embedField{ {Name: "Run ID", Value: r.RunID, Inline: true}, @@ -197,7 +243,7 @@ func (n *Notifier) RunClaimed(r RunRef, failures int) { Name: "Previous failures", Value: fmt.Sprintf("%d", failures), Inline: true, }) } - n.post(embed{ + n.postRun(r, embed{ Title: r.title("Run claimed"), Description: r.description(), Color: colorBlurple, @@ -212,7 +258,7 @@ func (n *Notifier) ClaudeFinished(r RunRef, res *claude.Result, elapsed time.Dur if res == nil { return } - n.post(embed{ + n.postRun(r, embed{ Title: r.title("Claude finished"), Description: r.description(), Color: colorGray, @@ -247,7 +293,7 @@ func (n *Notifier) VerifyResult(r RunRef, v verify.Result) { fields = append(fields, embedField{Name: "Output (tail)", Value: codeBlock(tail(out, 900)), Inline: false}) } } - n.post(embed{Title: r.title(what), Description: r.description(), Color: color, Fields: fields}) + n.postRun(r, embed{Title: r.title(what), Description: r.description(), Color: color, Fields: fields}) } // PROpened reports that a draft pull request was opened. @@ -256,11 +302,13 @@ func (n *Notifier) PROpened(r RunRef, prURL string, res *claude.Result, v verify if res != nil { model, session, cost = res.PrimaryModel(), res.SessionID, res.TotalCostUSD } - n.post(embed{ + n.postRun(r, embed{ Title: r.title("Draft PR opened"), - Description: prURL, + Description: r.describe(prURL), + URL: prURL, Color: colorGreen, Fields: append(r.fields(), + embedField{Name: "Pull request", Value: prURL, Inline: false}, 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}, @@ -292,9 +340,9 @@ func (n *Notifier) PlanPosted(r RunRef, res *claude.Result, elapsed time.Duratio if res != nil { model, cost = res.PrimaryModel(), res.TotalCostUSD } - n.post(embed{ + n.postRun(r, embed{ Title: r.title("Plan posted, awaiting approval"), - Description: "Reply `implement` on the issue to start the change.", + Description: r.describe("Reply `implement` on the issue to start the change."), Color: colorBlurple, Fields: append(r.fields(), embedField{Name: "Model", Value: orNone(model), Inline: true}, @@ -320,9 +368,9 @@ func (n *Notifier) RunCanceled(r RunRef, reason string) { // RunFailed reports a failed run and when it will be tried again. Retries are // unbounded, so "when" is the useful number, not "how many are left". func (n *Notifier) RunFailed(r RunRef, cause string, nextAttempt time.Time) { - n.post(embed{ + n.postRun(r, embed{ Title: r.title("Run failed"), - Description: truncate(cause, 500), + Description: r.describe(truncate(cause, 500)), Color: colorOrange, Fields: append(r.fields(), embedField{Name: "Next attempt", Value: when(nextAttempt), Inline: true}, @@ -333,9 +381,9 @@ func (n *Notifier) RunFailed(r RunRef, cause string, nextAttempt time.Time) { // RunAbandoned reports a run that was skipped rather than attempted — the // issue closed, lost its label, or is already covered by a pull request. func (n *Notifier) RunAbandoned(r RunRef, reason string, nextAttempt time.Time) { - n.post(embed{ + n.postRun(r, embed{ Title: r.title("Run abandoned"), - Description: truncate(reason, 500), + Description: r.describe(truncate(reason, 500)), Color: colorRed, Fields: append(r.fields(), embedField{Name: "Next attempt", Value: when(nextAttempt), Inline: true}, @@ -346,9 +394,9 @@ func (n *Notifier) RunAbandoned(r RunRef, reason string, nextAttempt time.Time) // RunDeferred reports a run the usage gate stopped. It is neither an attempt // nor a failure, so the issue keeps its place in the queue. func (n *Notifier) RunDeferred(r RunRef, reason string) { - n.post(embed{ + n.postRun(r, embed{ Title: r.title("Run deferred"), - Description: truncate(reason, 500), + Description: r.describe(truncate(reason, 500)), Color: colorYellow, Fields: r.fields(), }) @@ -356,16 +404,15 @@ func (n *Notifier) RunDeferred(r RunRef, reason string) { // LabelUpdateFailed reports that the issue's labels could not be brought in // line with the run's state, so what GitHub shows now disagrees with the store. -func (n *Notifier) LabelUpdateFailed(repo string, issue int, runID string, add, remove []string, err error) { - n.post(embed{ - Title: fmt.Sprintf("Label update failed: %s#%d", repo, issue), - Description: truncate(err.Error(), 500), +func (n *Notifier) LabelUpdateFailed(r RunRef, add, remove []string, err error) { + n.postRun(r, embed{ + Title: r.title("Label update failed"), + Description: r.describe(truncate(err.Error(), 500)), Color: colorYellow, - Fields: []embedField{ - {Name: "Add", Value: labelList(add), Inline: true}, - {Name: "Remove", Value: labelList(remove), Inline: true}, - {Name: "Run ID", Value: runID, Inline: true}, - }, + Fields: append(r.fields(), + embedField{Name: "Add", Value: labelList(add), Inline: true}, + embedField{Name: "Remove", Value: labelList(remove), Inline: true}, + ), }) } diff --git a/internal/discord/notifier_test.go b/internal/discord/notifier_test.go index 0bcdc21..a8fab77 100644 --- a/internal/discord/notifier_test.go +++ b/internal/discord/notifier_test.go @@ -192,13 +192,112 @@ func TestRunFailedStatesTheNextAttempt(t *testing.T) { } } +// Every run-scoped notification must carry a clickable link to the issue — +// a linked title plus an "Issue" field — so a future method that forgets it +// fails this test. +func TestEveryRunScopedNotificationLinksTheIssue(t *testing.T) { + const wantIssueURL = "https://github.com/acme/widgets/issues/42" + + res := &claude.Result{SessionID: "sess-abc", NumTurns: 1, TotalCostUSD: 0.1} + vres := verify.Result{Status: store.VerifyPassed} + + cases := map[string]func(n *Notifier){ + "RunClaimed": func(n *Notifier) { n.RunClaimed(testRef(), 0) }, + "ClaudeFinished": func(n *Notifier) { n.ClaudeFinished(testRef(), res, time.Second) }, + "VerifyResult": func(n *Notifier) { n.VerifyResult(testRef(), vres) }, + "PROpened": func(n *Notifier) { + n.PROpened(testRef(), "https://github.com/acme/widgets/pull/7", res, vres, "+1 -1", time.Second) + }, + "PlanPosted": func(n *Notifier) { n.PlanPosted(testRef(), res, time.Second) }, + "RunFailed": func(n *Notifier) { n.RunFailed(testRef(), "boom", time.Time{}) }, + "RunAbandoned": func(n *Notifier) { n.RunAbandoned(testRef(), "issue closed", time.Time{}) }, + "RunDeferred": func(n *Notifier) { n.RunDeferred(testRef(), "usage limit") }, + "LabelUpdateFailed": func(n *Notifier) { + n.LabelUpdateFailed(testRef(), []string{"a"}, []string{"b"}, errors.New("HTTP 404")) + }, + } + + for name, call := range cases { + t.Run(name, func(t *testing.T) { + url, bodies := stubWebhook(t) + n := New(true, url, nil) + + call(n) + n.Close(2 * time.Second) + + e := decodeEmbed(t, waitForCount(t, bodies, 1)[0]) + if e.URL == "" { + t.Errorf("%s: expected a non-empty embed URL", name) + } + issue, ok := field(e, "Issue") + if !ok || !strings.Contains(issue, wantIssueURL) { + t.Errorf("%s: expected an Issue field linking %s, got %+v", name, wantIssueURL, e.Fields) + } + }) + } +} + +// A RunRef with no explicit URL still links the issue, derived from repo and +// issue number, because the store does not carry an issue-URL column. +func TestRunRefDerivesTheIssueURLWhenNotSet(t *testing.T) { + url, bodies := stubWebhook(t) + n := New(true, url, nil) + + ref := testRef() + ref.URL = "" + n.RunClaimed(ref, 0) + n.Close(2 * time.Second) + + e := decodeEmbed(t, waitForCount(t, bodies, 1)[0]) + if e.URL != "https://github.com/acme/widgets/issues/42" { + t.Errorf("expected a derived issue URL, got %q", e.URL) + } +} + +// PROpened's title links the PR, not the issue, but the issue is still +// reachable from the Issue field. +func TestPROpenedLinksThePRAndTheIssue(t *testing.T) { + url, bodies := stubWebhook(t) + n := New(true, url, nil) + + prURL := "https://github.com/acme/widgets/pull/7" + n.PROpened(testRef(), prURL, &claude.Result{TotalCostUSD: 0.1}, verify.Result{Status: store.VerifyPassed}, "+1 -1", time.Second) + n.Close(2 * time.Second) + + e := decodeEmbed(t, waitForCount(t, bodies, 1)[0]) + if e.URL != prURL { + t.Errorf("expected the embed url to be the PR, got %q", e.URL) + } + if issue, ok := field(e, "Issue"); !ok || !strings.Contains(issue, "issues/42") { + t.Errorf("expected an Issue field linking the issue, got %+v", e.Fields) + } +} + +// RunFailed must not lose the issue link just because Description is +// overwritten with the cause. +func TestRunFailedDescriptionKeepsTheIssueLink(t *testing.T) { + url, bodies := stubWebhook(t) + n := New(true, url, nil) + + n.RunFailed(testRef(), "claude run failed", time.Time{}) + n.Close(2 * time.Second) + + e := decodeEmbed(t, waitForCount(t, bodies, 1)[0]) + if !strings.Contains(e.Description, "claude run failed") { + t.Errorf("expected the cause in the description, got %q", e.Description) + } + if !strings.Contains(e.Description, "issues/42") { + t.Errorf("expected the issue link in the description, got %q", e.Description) + } +} + // A label edit that failed leaves GitHub disagreeing with the store, which is // invisible unless it is reported. func TestLabelUpdateFailedNamesTheLabels(t *testing.T) { url, bodies := stubWebhook(t) n := New(true, url, nil) - n.LabelUpdateFailed("acme/widgets", 42, "run-1", + n.LabelUpdateFailed(testRef(), []string{"agent-failed"}, []string{"agent-working"}, errors.New("HTTP 404")) n.Close(2 * time.Second) diff --git a/internal/orchestrator/loop.go b/internal/orchestrator/loop.go index bbbe6c0..fccb599 100644 --- a/internal/orchestrator/loop.go +++ b/internal/orchestrator/loop.go @@ -642,7 +642,7 @@ func (o *Orchestrator) execute(ctx context.Context, log *slog.Logger, cand candi if phase == phasePlan { removeLabels = append(removeLabels, cfg.GitHub.PlanLabel) } - o.setLabels(ctx, log, cand, runID, []string{cfg.GitHub.WorkingLabel}, removeLabels) + o.setLabels(ctx, log, ref, []string{cfg.GitHub.WorkingLabel}, removeLabels) o.event(ctx, runID, "model", fmt.Sprintf("%s (fallbacks: %s)", head.ID, orNone(fallbacks))) log.Info("starting claude", "phase", phase, "model", head.ID, "branch", branch, "attempt", attempt) @@ -768,7 +768,7 @@ func (o *Orchestrator) execute(ctx context.Context, log *slog.Logger, cand candi if err := o.opts.Store.SavePlan(ctx, cand.repo, cand.number, runID, result.Result); err != nil { log.Warn("could not save plan", "error", err) } - o.setLabels(ctx, log, cand, runID, + o.setLabels(ctx, log, ref, []string{cfg.GitHub.PlanLabel}, []string{cfg.GitHub.WorkingLabel}) if err := o.opts.Store.SetRunStatus(ctx, runID, store.StatusPlanned); err != nil { log.Warn("status update failed", "error", err) @@ -844,7 +844,7 @@ func (o *Orchestrator) execute(ctx context.Context, log *slog.Logger, cand candi if err := o.opts.GH.Comment(ctx, cand.repo, cand.number, issueComment(prURL, runID, vres)); err != nil { log.Warn("could not comment on issue", "error", err) } - o.setLabels(ctx, log, cand, runID, + o.setLabels(ctx, log, ref, []string{cfg.GitHub.DoneLabel}, []string{cfg.GitHub.WorkingLabel, cfg.GitHub.Label, cfg.GitHub.FailedLabel, cfg.GitHub.PlanLabel}) @@ -902,7 +902,7 @@ func (o *Orchestrator) adoptPR(ctx context.Context, log *slog.Logger, cand candi } } - o.setLabels(ctx, log, cand, runID, + o.setLabels(ctx, log, cand.ref(runID, attempt), []string{cfg.GitHub.DoneLabel}, []string{cfg.GitHub.WorkingLabel, cfg.GitHub.Label, cfg.GitHub.FailedLabel, cfg.GitHub.PlanLabel}) @@ -957,6 +957,7 @@ func (o *Orchestrator) handleFailure(ctx context.Context, log *slog.Logger, cand // context.WithoutCancel: the run's context may already be cancelled, but // the bookkeeping still has to be written. ctx = context.WithoutCancel(ctx) + ref := cand.ref(runID, attempt) var skip errSkip if errors.As(cause, &skip) { @@ -964,9 +965,9 @@ func (o *Orchestrator) handleFailure(ctx context.Context, log *slog.Logger, cand if err := o.opts.Store.FailRun(ctx, runID, store.StatusAbandoned, "skipped: "+skip.reason); err != nil { log.Error("could not record skip", "error", err) } - o.opts.Discord.RunAbandoned(cand.ref(runID, attempt), "skipped: "+skip.reason, + o.opts.Discord.RunAbandoned(ref, "skipped: "+skip.reason, o.scheduleRetry(ctx, log, cand, runID)) - o.setLabels(ctx, log, cand, runID, nil, []string{cfg.GitHub.WorkingLabel}) + o.setLabels(ctx, log, ref, nil, []string{cfg.GitHub.WorkingLabel}) o.finishCleanup(ctx, log, cand) return } @@ -982,8 +983,8 @@ func (o *Orchestrator) handleFailure(ctx context.Context, log *slog.Logger, cand log.Error("could not record cancellation", "error", err) } o.event(ctx, runID, "canceled", cause.Error()) - o.opts.Discord.RunCanceled(cand.ref(runID, attempt), cause.Error()) - o.setLabels(ctx, log, cand, runID, nil, []string{cfg.GitHub.WorkingLabel}) + o.opts.Discord.RunCanceled(ref, cause.Error()) + o.setLabels(ctx, log, ref, nil, []string{cfg.GitHub.WorkingLabel}) o.finishCleanup(ctx, log, cand) return } @@ -999,8 +1000,8 @@ func (o *Orchestrator) handleFailure(ctx context.Context, log *slog.Logger, cand log.Error("could not record deferral", "error", err) } o.event(ctx, runID, "deferred", cause.Error()) - o.opts.Discord.RunDeferred(cand.ref(runID, attempt), cause.Error()) - o.setLabels(ctx, log, cand, runID, nil, []string{cfg.GitHub.WorkingLabel}) + o.opts.Discord.RunDeferred(ref, cause.Error()) + o.setLabels(ctx, log, ref, nil, []string{cfg.GitHub.WorkingLabel}) o.finishCleanup(ctx, log, cand) return } @@ -1019,12 +1020,12 @@ func (o *Orchestrator) handleFailure(ctx context.Context, log *slog.Logger, cand o.event(ctx, runID, "failed", cause.Error()) nextAttempt := o.scheduleRetry(ctx, log, cand, runID) - o.opts.Discord.RunFailed(cand.ref(runID, attempt), cause.Error(), nextAttempt) + o.opts.Discord.RunFailed(ref, cause.Error(), nextAttempt) // The trigger label always stays put: it, and only it, decides whether the // issue is worked. agent-failed mirrors the outcome until the next attempt // clears it. - o.setLabels(ctx, log, cand, runID, + o.setLabels(ctx, log, ref, []string{cfg.GitHub.FailedLabel}, []string{cfg.GitHub.WorkingLabel}) if err := o.opts.GH.Comment(ctx, cand.repo, cand.number, failureComment(runID, attempt, cause.Error(), nextAttempt, cfg.GitHub.Label)); err != nil { @@ -1075,15 +1076,15 @@ func (o *Orchestrator) recordSession(ctx context.Context, log *slog.Logger, cand // run, but it is also never silently dropped: a failure that goes unrecorded // leaves an issue whose labels disagree with the store, which is precisely // what a human reading the issue would be misled by. -func (o *Orchestrator) setLabels(ctx context.Context, log *slog.Logger, cand candidate, runID string, add, remove []string) { +func (o *Orchestrator) setLabels(ctx context.Context, log *slog.Logger, ref discord.RunRef, add, remove []string) { ctx = context.WithoutCancel(ctx) - if err := o.opts.GH.EditLabels(ctx, cand.repo, cand.number, add, remove); err != nil { + if err := o.opts.GH.EditLabels(ctx, ref.Repo, ref.Issue, add, remove); err != nil { log.Warn("could not update labels", "add", add, "remove", remove, "error", err) - o.event(ctx, runID, "labels_failed", fmt.Sprintf("add %v remove %v: %v", add, remove, err)) - o.opts.Discord.LabelUpdateFailed(cand.repo, cand.number, runID, add, remove, err) + o.event(ctx, ref.RunID, "labels_failed", fmt.Sprintf("add %v remove %v: %v", add, remove, err)) + o.opts.Discord.LabelUpdateFailed(ref, add, remove, err) return } - o.event(ctx, runID, "labels", fmt.Sprintf("add %v remove %v", add, remove)) + o.event(ctx, ref.RunID, "labels", fmt.Sprintf("add %v remove %v", add, remove)) } func (o *Orchestrator) finishCleanup(ctx context.Context, log *slog.Logger, cand candidate) { diff --git a/internal/server/server.go b/internal/server/server.go index a57e900..df45648 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -262,6 +262,8 @@ func (s *Server) cancelRun(c fiber.Ctx) error { } // The run's own outcome notification follows from the orchestrator, and // carries the repo, issue, and attempt that this handler does not know. + // The run's own outcome notification follows from the orchestrator, and + // carries the repo, issue, and attempt that this handler does not know. s.log.Info("run cancelled by operator", "run", id) return c.JSON(fiber.Map{"cancelled": true, "run": id}) } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 9d88310..4888e28 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -219,6 +219,22 @@ func TestCancelRun(t *testing.T) { } } +// A run the controller cancels may not have a row in the store (e.g. it +// hasn't been recorded yet); the notification lookup must not affect the +// HTTP result or panic. +func TestCancelRunWithNoStoreRowStillSucceeds(t *testing.T) { + s, _, ctrl := testServer(t) + ctrl.cancelOK = true + + code, body := do(t, s, http.MethodPost, "/runs/unknown-run/cancel", nil) + if code != http.StatusOK { + t.Fatalf("cancel = %d", code) + } + if body["cancelled"] != true { + t.Fatalf("unexpected body: %v", body) + } +} + // fiberTimeout gives handlers room on a loaded machine; the default is 1s. var fiberTimeout = fiber.TestConfig{Timeout: 10 * time.Second, FailOnTimeout: true}