Skip to content

Commit 08d73de

Browse files
isadeksisadeks
committed
fix(orchestration): integration node no longer floods the parent epic + panel preview deep-links (aws-samples#247 UX.16 + UX.17)
Live-caught on the ABCA-301 fan-out epic: the synthetic integration node has NO Linear sub-issue of its own, so its work spilled THREE standalone comments onto the PARENT epic — '🤖 Starting integration…', '🔗 PR opened: aws-samples#191', '🖼️ Preview screenshot' — a comment stream that violates the locked 'one maturing panel, no comment stream' spec and 100% duplicates what the panel already shows (Integration row ✅ + Combined PR + Combined preview). Separately, the panel's combined preview embedded the image but had no clickable link to the running combined deploy. UX.16 — stop the flood (two emitters): - agent: prompt_builder._channel_prompt_addendum now returns '' when channel_metadata has no linear_issue_id. The integration node is the only Linear task without one (orchestration-release omits it on purpose), so the agent no longer gets the 'post Starting/PR-opened to Linear' instructions → no groping onto the parent. Real sub-issues are unaffected (they have linear_issue_id). - screenshot pipeline: persistScreenshotUrl now returns whether the deploy task is an integration node (read from the UpdateItem ALL_NEW channel_metadata.orchestration_sub_issue_id — no extra Get), and the processor SKIPS the standalone Linear screenshot comment for it. The GitHub PR comment still posts (load-bearing on the PR); the panel embed is the only Linear surface for the combined result. UX.17 — panel preview deep-link: - persist screenshot_preview_url (the live Vercel/Netlify deploy URL the shot was captured from) alongside screenshot_url; thread it through resolveCombinedScreenshotUrl → upsertEpicPanel → renderEpicPanel. - the panel's combined preview is now a clickable linked image ([![combined preview](png)](preview)) + an 'Open the combined preview' link. Falls back to the plain image when no preview URL is known. The preview URL is payload-derived → parens percent-encoded (markdown breakout defense, same as the screenshot comment renderers). Tests: agent test_linear_integration_node_gets_no_addendum (+ existing linear test now carries linear_issue_id); processor 'persists BOTH urls' + 'integration node deploy persists but posts NO standalone Linear comment'; renderEpicPanel deep-link + paren-encode + fallback; reconciler #57 test extended to assert the deep-link. Also refreshed 8 stale postRollup mocks to the merged LinearPostResult {ok} contract (merge left them returning bare booleans → 5 pre-existing reds, now green). cdk:compile clean; agent suite 1116 green.
1 parent a5d00a6 commit 08d73de

9 files changed

Lines changed: 537 additions & 156 deletions

‎agent/src/prompt_builder.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,15 @@ def _channel_prompt_addendum(config: TaskConfig) -> str:
144144
"""
145145
if config.channel_source != "linear":
146146
return ""
147+
# #247 UX.16: a synthetic orchestration integration node has NO real Linear
148+
# sub-issue — `linear_issue_id` is intentionally omitted from its
149+
# channel_metadata (see orchestration-release.ts). Without a target issue
150+
# the agent would grope via the MCP and post its "Starting"/"PR opened"
151+
# comments onto the PARENT epic, cluttering the maturing panel (which
152+
# already shows the integration row + combined PR + preview). Skip the
153+
# progress addendum entirely for these nodes — the panel is the surface.
154+
if not config.channel_metadata.get("linear_issue_id"):
155+
return ""
147156
issue_identifier = config.channel_metadata.get("linear_issue_identifier") or ""
148157
issue_ref = f" (`{issue_identifier}`)" if issue_identifier else ""
149158
return (

‎agent/tests/test_prompts.py‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,34 @@ def test_linear_channel_includes_linear_tools(self):
3535
addendum = _channel_prompt_addendum(
3636
_config(
3737
channel_source="linear",
38-
channel_metadata={"linear_issue_identifier": "ABC-42"},
38+
channel_metadata={
39+
"linear_issue_id": "issue-uuid-1",
40+
"linear_issue_identifier": "ABC-42",
41+
},
3942
)
4043
)
4144
assert "Linear issue progress updates" in addendum
4245
assert "mcp__linear-server__save_comment" in addendum
4346
assert "ABC-42" in addendum
4447

48+
def test_linear_integration_node_gets_no_addendum(self):
49+
# #247 UX.16: the synthetic orchestration integration node is a Linear
50+
# task but has NO real sub-issue — channel_metadata omits
51+
# linear_issue_id. Without a target issue the agent would grope via the
52+
# MCP and post its "Starting"/"PR opened" comments onto the PARENT epic,
53+
# cluttering the maturing panel. No issue id → no progress addendum.
54+
addendum = _channel_prompt_addendum(
55+
_config(
56+
channel_source="linear",
57+
channel_metadata={
58+
"orchestration_id": "orch_abc",
59+
"orchestration_sub_issue_id": "orch_abc__integration",
60+
"parent_linear_issue_id": "parent-uuid",
61+
},
62+
)
63+
)
64+
assert addendum == ""
65+
4566
def test_jira_channel_gets_no_addendum(self):
4667
# Jira comments are posted out-of-band by jira_reactions (REST shim);
4768
# the Atlassian MCP can't load in a headless agent, so instructing the

‎cdk/src/handlers/github-webhook-processor.ts‎

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919

2020
import { DynamoDBClient } from '@aws-sdk/client-dynamodb';
2121
import { PutObjectCommand, S3Client } from '@aws-sdk/client-s3';
22-
import { DynamoDBDocumentClient, UpdateCommand } from '@aws-sdk/lib-dynamodb';
22+
import { DynamoDBDocumentClient, GetCommand, UpdateCommand } from '@aws-sdk/lib-dynamodb';
2323
import { captureScreenshot } from './shared/agentcore-browser';
2424
import { resolveGitHubToken } from './shared/context-hydration';
2525
import { upsertTaskComment } from './shared/github-comment';
@@ -34,6 +34,7 @@ import {
3434
findLinearIssueByIdentifier,
3535
} from './shared/linear-issue-lookup';
3636
import { logger } from './shared/logger';
37+
import { isIntegrationNode } from './shared/orchestration-integration-node';
3738
import { buildScreenshotKey, encodeMarkdownUrl, extractTaskIdFromBranch, isAllowedScreenshotUrl } from './shared/screenshot-url';
3839

3940
const s3 = new S3Client({});
@@ -262,11 +263,18 @@ export async function handler(event: ProcessorEvent): Promise<void> {
262263
const publicUrl = `https://${SCREENSHOT_PUBLIC_HOST}/${key}`;
263264
const commentBody = renderCommentBody(publicUrl, previewUrl);
264265

265-
// #247: persist the screenshot URL on the deploy task's record (keyed by the
266-
// taskId in the branch) so the orchestration reconciler can embed the
267-
// integration node's combined preview in the parent epic panel. Best-effort,
268-
// before the comment posts so a comment-post failure doesn't skip it.
269-
await persistScreenshotUrl(pr.headRefName, publicUrl);
266+
// #247: persist the screenshot + preview URLs on the deploy task's record
267+
// (keyed by the taskId in the branch) so the orchestration reconciler can
268+
// embed the integration node's combined preview in the parent epic panel.
269+
// Best-effort, before the comment posts so a comment-post failure doesn't
270+
// skip it. The return tells us whether this is the synthetic integration
271+
// node — whose screenshot belongs in the panel only, never as a standalone
272+
// Linear comment on the parent epic (#247 UX.16).
273+
const { isIntegrationNode: isIntegrationDeploy } = await persistScreenshotUrl(
274+
pr.headRefName,
275+
publicUrl,
276+
previewUrl,
277+
);
270278

271279
try {
272280
const result = await upsertTaskComment({
@@ -305,7 +313,14 @@ export async function handler(event: ProcessorEvent): Promise<void> {
305313
// load-bearing artifact; the Linear comment is bonus surface for
306314
// reviewers who live in Linear. Only fires when the registry table
307315
// is configured AND the PR carries a Linear identifier.
308-
if (LINEAR_WORKSPACE_REGISTRY_TABLE) {
316+
//
317+
// #247 UX.16: the synthetic integration node has no Linear sub-issue of its
318+
// own, so a Linear post here would resolve the parent-epic identifier from
319+
// the PR title and land a "🖼️ Preview screenshot" comment ON THE PARENT —
320+
// cluttering the maturing panel (which already embeds the combined preview
321+
// via the persisted screenshot_url). Skip the Linear post for the integration
322+
// node; the panel is the only Linear surface for the combined result.
323+
if (LINEAR_WORKSPACE_REGISTRY_TABLE && !isIntegrationDeploy) {
309324
// Branch-name first — it deterministically encodes this PR's own
310325
// issue (`bgagent/{taskId}/abca-151-...`). Title/body are ambiguous
311326
// fallbacks: in a stacked #247 orchestration the body often names a
@@ -517,19 +532,37 @@ async function findPullRequestForSha(
517532
* artifacts. Conditional on ``attribute_exists`` so we never resurrect a
518533
* TTL-reaped row.
519534
*/
520-
async function persistScreenshotUrl(branchName: string, publicUrl: string): Promise<void> {
521-
if (!TASK_TABLE) return;
535+
async function persistScreenshotUrl(
536+
branchName: string,
537+
publicUrl: string,
538+
previewUrl: string,
539+
): Promise<{ isIntegrationNode: boolean }> {
540+
const result = { isIntegrationNode: false };
541+
if (!TASK_TABLE) return result;
522542
const taskId = extractTaskIdFromBranch(branchName);
523-
if (!taskId) return;
543+
if (!taskId) return result;
524544
try {
525-
await ddb.send(new UpdateCommand({
545+
// Persist BOTH the captured image URL and the live preview-deploy URL so
546+
// the reconciler can render a clickable combined-preview deep-link in the
547+
// panel (#247 UX.17). Return-on-values so we learn whether this deploy task
548+
// is a synthetic integration node WITHOUT a second Get (#247 UX.16): the
549+
// integration node's screenshot belongs in the PANEL only — it must NOT
550+
// also post a standalone Linear comment on the parent epic.
551+
const upd = await ddb.send(new UpdateCommand({
526552
TableName: TASK_TABLE,
527553
Key: { task_id: taskId },
528-
UpdateExpression: 'SET screenshot_url = :u',
554+
UpdateExpression: 'SET screenshot_url = :u, screenshot_preview_url = :p',
529555
ConditionExpression: 'attribute_exists(task_id)',
530-
ExpressionAttributeValues: { ':u': publicUrl },
556+
ExpressionAttributeValues: { ':u': publicUrl, ':p': previewUrl },
557+
ReturnValues: 'ALL_NEW',
531558
}));
532-
logger.info('Persisted screenshot_url on task record', { task_id: taskId, public_url: publicUrl });
559+
const subIssueId = upd.Attributes?.channel_metadata?.orchestration_sub_issue_id;
560+
result.isIntegrationNode = typeof subIssueId === 'string' && isIntegrationNode(subIssueId);
561+
logger.info('Persisted screenshot_url on task record', {
562+
task_id: taskId,
563+
public_url: publicUrl,
564+
is_integration_node: result.isIntegrationNode,
565+
});
533566
} catch (err) {
534567
// ConditionalCheckFailed = the task row is gone (TTL); anything else is a
535568
// transient DDB error. Either way the comments still posted — log + move on.
@@ -539,6 +572,7 @@ async function persistScreenshotUrl(branchName: string, publicUrl: string): Prom
539572
error: err instanceof Error ? err.message : String(err),
540573
});
541574
}
575+
return result;
542576
}
543577

544578
function renderCommentBody(publicUrl: string, previewUrl: string): string {

‎cdk/src/handlers/orchestration-reconciler.ts‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,10 @@ import {
4646
} from '@aws-sdk/lib-dynamodb';
4747
import type { DynamoDBRecord, DynamoDBStreamEvent } from 'aws-lambda';
4848
import { createTaskCore } from './shared/create-task-core';
49+
import { renderFailureReply } from './shared/failure-reply';
50+
import { postIssueComment, replyToComment } from './shared/linear-feedback';
4951
import { logger } from './shared/logger';
52+
import { isIntegrationNode } from './shared/orchestration-integration-node';
5053
import { ORCH_LOG } from './shared/orchestration-log-events';
5154
import {
5255
computeReconcilePlan,
@@ -56,9 +59,6 @@ import {
5659
import { readConcurrencyBudget, releaseReadyChildren } from './shared/orchestration-release';
5760
import { planDirectRestack, type RestackStep } from './shared/orchestration-restack';
5861
import { cascadeNodeLabel, upsertEpicPanel } from './shared/orchestration-rollup';
59-
import { postIssueComment, replyToComment } from './shared/linear-feedback';
60-
import { renderFailureReply } from './shared/failure-reply';
61-
import { isIntegrationNode } from './shared/orchestration-integration-node';
6262
import {
6363
claimRollup,
6464
clearRollupClaim,
@@ -247,16 +247,25 @@ async function resolveChildPrUrls(
247247
* no preview deployed yet, or the read fails. Only the integration node is
248248
* read (one Get), since that's the only node whose preview is "combined".
249249
*/
250-
async function resolveCombinedScreenshotUrl(taskId?: string): Promise<string | null> {
250+
async function resolveCombinedScreenshotUrl(
251+
taskId?: string,
252+
): Promise<{ url: string; previewUrl?: string } | null> {
251253
if (!taskId) return null;
252254
try {
253255
const res = await ddb.send(new GetCommand({
254256
TableName: TASK_TABLE,
255257
Key: { task_id: taskId },
256-
ProjectionExpression: 'screenshot_url',
258+
ProjectionExpression: 'screenshot_url, screenshot_preview_url',
257259
}));
258260
const url = res.Item?.screenshot_url;
259-
return typeof url === 'string' && url.length > 0 ? url : null;
261+
if (typeof url !== 'string' || url.length === 0) return null;
262+
const previewUrl = res.Item?.screenshot_preview_url;
263+
// #247 UX.17: the live preview-deploy URL makes the panel's combined
264+
// preview a clickable deep-link to the running combined site.
265+
return {
266+
url,
267+
...(typeof previewUrl === 'string' && previewUrl.length > 0 && { previewUrl }),
268+
};
260269
} catch (err) {
261270
logger.warn('Combined screenshot read failed (non-fatal) — panel posts without it', {
262271
task_id: taskId, error: err instanceof Error ? err.message : String(err),
@@ -418,7 +427,7 @@ async function refreshPanelAndSettle(
418427
// the panel when the epic is complete. Only read it on the all-terminal
419428
// settle (the integration node has deployed by then); skip the extra Get on
420429
// every in-flight edit.
421-
const combinedScreenshotUrl = (allTerminal && integration)
430+
const combinedScreenshot = (allTerminal && integration)
422431
? await resolveCombinedScreenshotUrl(integration.child_task_id)
423432
: null;
424433

@@ -445,7 +454,8 @@ async function refreshPanelAndSettle(
445454
children,
446455
prUrls,
447456
...(combinedPrUrl !== undefined && { combinedPrUrl }),
448-
...(combinedScreenshotUrl !== null && { combinedScreenshotUrl }),
457+
...(combinedScreenshot !== null && { combinedScreenshotUrl: combinedScreenshot.url }),
458+
...(combinedScreenshot?.previewUrl !== undefined && { combinedPreviewUrl: combinedScreenshot.previewUrl }),
449459
inProgress: !allTerminal,
450460
mirrorParentState: allTerminal ? won : false,
451461
...(meta.release_context.channel_source !== undefined && {

‎cdk/src/handlers/shared/orchestration-rollup.ts‎

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,10 @@ import {
3838
upsertStatusComment,
3939
} from './linear-feedback';
4040
import { logger } from './logger';
41-
import { ORCH_LOG } from './orchestration-log-events';
4241
import { isIntegrationNode } from './orchestration-integration-node';
42+
import { ORCH_LOG } from './orchestration-log-events';
4343
import type { OrchestrationChildRow } from './orchestration-store';
44+
import { encodeMarkdownUrl } from './screenshot-url';
4445
import type { ChannelSource } from './types';
4546

4647
/** Which rollup we're posting — drives the heading + emoji. */
@@ -198,6 +199,13 @@ export interface EpicPanelParams {
198199
readonly combinedPrUrl?: string;
199200
/** Combined preview screenshot url, embedded in the panel (auto-refreshes; no separate comment). */
200201
readonly combinedScreenshotUrl?: string;
202+
/**
203+
* Live deploy-preview URL the combined screenshot was captured from (#247
204+
* UX.17). When present, the embedded combined preview becomes a clickable
205+
* deep-link to the running combined site. Ignored unless
206+
* ``combinedScreenshotUrl`` is also set.
207+
*/
208+
readonly combinedPreviewUrl?: string;
201209
}
202210

203211
const PANEL_FOOTER = '_One live panel — updates in place as the epic progresses; no comment stream._';
@@ -248,7 +256,7 @@ function panelLabel(row: EpicPanelRow): string {
248256
* - Combined PR callout + embedded combined screenshot when present.
249257
*/
250258
export function renderEpicPanel(params: EpicPanelParams): string {
251-
const { rows, inProgress, combinedPrUrl, combinedScreenshotUrl } = params;
259+
const { rows, inProgress, combinedPrUrl, combinedScreenshotUrl, combinedPreviewUrl } = params;
252260
const terminal = (s: string) => s === 'succeeded' || s === 'failed' || s === 'skipped';
253261
// "done" counts settled rows that are NOT mid-update (an updating row is back in flight).
254262
const done = rows.filter((r) => terminal(r.child_status) && !r.updatingReason).length;
@@ -283,9 +291,29 @@ export function renderEpicPanel(params: EpicPanelParams): string {
283291
const callout = combinedPrUrl
284292
? ['', `🔗 **Combined PR (all sub-issues merged):** [${combinedPrUrl}](${combinedPrUrl})`]
285293
: [];
286-
const shot = combinedScreenshotUrl
287-
? ['', `🖼️ **Combined preview**`, '', `![combined preview](${combinedScreenshotUrl})`]
288-
: [];
294+
// #247 UX.17: when we know the live preview-deploy URL, render the embedded
295+
// screenshot as a clickable linked image + a plain "Open the combined
296+
// preview" link, so a reviewer can open the running combined site, not just
297+
// see a static PNG. The preview URL is payload-derived (came from the deploy
298+
// webhook) — percent-encode its parens so a crafted path can't break out of
299+
// the markdown link. The CloudFront screenshot URL is our own key (no
300+
// parens) so it's interpolated as-is.
301+
let shot: string[] = [];
302+
if (combinedScreenshotUrl) {
303+
if (combinedPreviewUrl) {
304+
const safePreview = encodeMarkdownUrl(combinedPreviewUrl);
305+
shot = [
306+
'',
307+
'🖼️ **Combined preview**',
308+
'',
309+
`[![combined preview](${combinedScreenshotUrl})](${safePreview})`,
310+
'',
311+
`[Open the combined preview](${safePreview})`,
312+
];
313+
} else {
314+
shot = ['', '🖼️ **Combined preview**', '', `![combined preview](${combinedScreenshotUrl})`];
315+
}
316+
}
289317

290318
return [heading, '', ...lines, ...callout, ...shot, '', PANEL_FOOTER].join('\n');
291319
}
@@ -333,6 +361,8 @@ export interface UpsertEpicPanelParams {
333361
readonly updating?: Readonly<Record<string, string>>;
334362
readonly combinedPrUrl?: string;
335363
readonly combinedScreenshotUrl?: string;
364+
/** Live preview-deploy URL the combined screenshot was captured from (#247 UX.17). */
365+
readonly combinedPreviewUrl?: string;
336366
/**
337367
* Whether the epic is in progress. When omitted, derived: in progress iff any
338368
* child is non-terminal OR any row has an updating reason. Pass explicitly to
@@ -383,6 +413,7 @@ export async function upsertEpicPanel(params: UpsertEpicPanelParams): Promise<st
383413
inProgress,
384414
...(params.combinedPrUrl !== undefined && { combinedPrUrl: params.combinedPrUrl }),
385415
...(params.combinedScreenshotUrl !== undefined && { combinedScreenshotUrl: params.combinedScreenshotUrl }),
416+
...(params.combinedPreviewUrl !== undefined && { combinedPreviewUrl: params.combinedPreviewUrl }),
386417
});
387418

388419
let commentId: string | null;

‎cdk/src/handlers/shared/types.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,15 @@ export interface TaskRecord {
104104
* tasks with no UI to screenshot).
105105
*/
106106
readonly screenshot_url?: string;
107+
/**
108+
* Live deploy-preview URL the {@link screenshot_url} image was captured from
109+
* (e.g. the Vercel/Netlify preview deploy). Persisted alongside
110+
* ``screenshot_url`` so the orchestration reconciler can make the INTEGRATION
111+
* node's combined preview in the parent epic panel a clickable deep-link to
112+
* the running combined site, not just a static image (#247 UX.17). Absent
113+
* when no preview deployed.
114+
*/
115+
readonly screenshot_preview_url?: string;
107116
readonly error_message?: string;
108117
readonly idempotency_key?: string;
109118
readonly channel_source: ChannelSource;

0 commit comments

Comments
 (0)