fix(fanout): render the PR link on ✅ success, not only on ⚠️ (ABCA-584) (#601)

* fix(fanout): render the PR link on ✅ success, not only on ⚠️ (F-prlink)

The Linear completion comment rendered the PR URL ONLY on the ⚠️ "shipped a PR
but stopped early" path, on the assumption that on ✅ success the agent's own
step-2 "PR opened" comment reliably carries the link, so duplicating it is noise.

That assumption fails when the agent skips its PR-opened comment. Live-caught on
ABCA-584: a :decompose that declined to split ran as one coding task, opened PR
#395 (pr_url + build_passed on the task record), but posted NO "🔗 PR opened"
comment — so the ✅ "Task completed" comment omitted the link and it was lost
entirely; the user asked "where is the PR link?".

Fix: render `PR: <url>` whenever the task produced one, on BOTH ✅ and ⚠️. The
completion comment is the terminal, platform-owned surface, so it must carry the
link rather than depend on the agent choosing to post its own; a duplicate with
the agent's comment (when it does fire) is far cheaper than a missing PR, and
this is the one terminal comment per task so it can't spam. Test updated to
assert the link renders on ✅. Full cdk build green (2935 tests). Held.

* docs+test: address review nits N1–N3 on the PR-link render

Optional polish from the #601 review (fix itself approved, unchanged):

- N1: add a negative assertion for the branch this change now makes live —
  ✅ task_completed + prUrl null → NO `PR:` line. The relaxed `if (args.prUrl)`
  guard means this is no longer structurally guaranteed by the old ⚠️-only
  condition, so pin its absence (fanout-task-events.test.ts).
- N2: reword the "ONE terminal comment per task" comment — the single-post
  guarantee lives in the caller `dispatchToLinear` (linear_final_comment_event_id
  idempotency marker), NOT this pure formatter, which enforces nothing. Credit
  the right layer so a future non-idempotent caller isn't misled.
- N3: add a one-line note atop the function JSDoc — prUrl, when present, renders
  on ALL frames — so the "why" isn't split between the JSDoc (ABCA-91 story) and
  the render-site note (ABCA-584) 50 lines below.

No behavior change. fanout-task-events tests green (108); eslint clean.

Author: isadeks

cdk/src/handlers/fanout-task-events.ts

@@ -850,6 +850,12 @@ async function dispatchToEmail(event: FanOutEvent): Promise<void> {
  * Render the Linear final-status comment body. Inputs are already
  * coerced to native types by the caller; this function only formats.
  *
+ * ``prUrl``, when present, renders on ALL frames — the ✅ success frame as much
+ * as the ⚠️ shipped-but-stopped one. The completion comment is the terminal,
+ * platform-owned surface, so it must carry the PR link authoritatively rather
+ * than assume the agent's own "PR opened" comment did (the ABCA-584 case — see
+ * the render note below). Only the framing/header flips on the outcome.
+ *
  * The framing flips between three outcomes based on `(eventType, prUrl)`:
  *
  *   1. ``task_completed``                        → ✅ "Task completed"
@@ -906,14 +912,21 @@ export function renderLinearFinalStatusComment(args: {
     '',
     `cost: ${costStr} • turns: ${turnsStr} • duration: ${durationStr}`,
   ];
-  // Render the PR URL only on the ⚠️ "shipped a PR but stopped early"
-  // path — that's the case where the agent's own step-2 "PR opened"
-  // comment is *not* guaranteed to have fired (the agent may have
-  // crashed between opening the PR and posting the comment, e.g.
-  // ABCA-91 hitting max-turns on turn 101). On the ✅ success path the
-  // agent's step-2 comment reliably carries the PR link, so duplicating
-  // it here is just noise.
-  if (args.prUrl && shippedDespiteFailure) {
+  // Render the PR URL whenever the task produced one — on BOTH the ✅ success and
+  // the ⚠️ "shipped a PR but stopped early" paths. The prior code rendered it only
+  // on ⚠️, on the assumption that "on ✅ the agent's own step-2 'PR opened' comment
+  // reliably carries the link, so duplicating it is noise." That assumption FAILS
+  // when the agent skips its PR-opened comment — live-caught on ABCA-584, where a
+  // decompose→single task opened PR #395 (pr_url on the record) but posted no
+  // PR-opened comment, so the ✅ completion comment omitted it and the link was
+  // LOST entirely. The completion comment is the terminal, platform-owned surface;
+  // rendering pr_url here guarantees the link is never lost, and a duplicate with
+  // the agent's own comment is far cheaper than a missing PR. (Duplicate terminal
+  // comments can't spam because the CALLER, `dispatchToLinear`, posts this body at
+  // most once per task via the `linear_final_comment_event_id` idempotency marker
+  // — this formatter is pure and enforces nothing; a future caller on a
+  // non-idempotent path would need its own guard.)
+  if (args.prUrl) {
     lines.push('', `PR: ${args.prUrl}`);
   }
   lines.push('', `_task ${args.taskId}_`);

cdk/test/handlers/fanout-task-events.test.ts

@@ -1382,14 +1382,14 @@ describe('fanout-task-events: Linear dispatcher (issue #239)', () => {
     expect(body).toContain('$0.55');
     expect(body).toContain('27 / 100');
     expect(body).toContain('3m 41s');
-    // PR URL is intentionally NOT rendered on the ✅ success path —
-    // the agent's step-2 "PR opened" comment already carries it, so
-    // duplicating it here just stacks two near-identical links on the
-    // Linear issue. (Smoke-test feedback after the first dev deploy.)
-    // The ⚠️ "shipped a PR but stopped early" path DOES render it,
-    // because the agent may have crashed before its step-2 comment
-    // fired — see the ABCA-91 case test below.
-    expect(body).not.toContain('https://github.com/owner/repo/pull/13');
+    // F-prlink (ABCA-584): the PR URL IS rendered on the ✅ success path. The old
+    // behavior omitted it, assuming the agent's own step-2 "PR opened" comment
+    // always carries it — but that comment can silently not fire (live-caught: a
+    // decompose→single task opened a PR but posted no PR-opened comment, so the
+    // link was lost entirely). The terminal completion comment is the
+    // platform-owned surface, so it must carry the link; a duplicate is far
+    // cheaper than a missing PR.
+    expect(body).toContain('https://github.com/owner/repo/pull/13');
     expect(body).toContain('t-lin');
   });
 
@@ -1665,6 +1665,25 @@ describe('renderLinearFinalStatusComment', () => {
     expect(body).not.toContain('27 /');
   });
 
+  test('✅ task_completed with prUrl null → NO PR line (ABCA-584 guard)', () => {
+    // Relaxing the render guard to `if (args.prUrl)` makes "✅ completed +
+    // prUrl null → no PR: line" a LIVE branch (it was previously structurally
+    // guaranteed by the old ⚠️-only condition). Pin its absence so a future
+    // stray or `PR: null` line can't creep in unnoticed.
+    const body = renderLinearFinalStatusComment({
+      eventType: 'task_completed',
+      prUrl: null,
+      costUsd: 0.5,
+      turns: 10,
+      maxTurns: 100,
+      durationS: 60,
+      taskId: 't',
+      errorTitle: null,
+    });
+    expect(body).toContain('✅');
+    expect(body).not.toContain('PR:');
+  });
+
   test('formatDuration: under 60s → seconds only', () => {
     const body = renderLinearFinalStatusComment({
       eventType: 'task_completed',