Always include the github url in the Discord notification (#6)

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

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,26 +394,25 @@ 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(),
 	})
 }
 
 // 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},
+ ),
 	})
 }
 

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)