feat(screenshot): skip screenshot when preview URL returns non-2xx (#289)

* feat(screenshot): skip screenshot when preview URL returns non-2xx

Closes #287.

Before: the processor only checked Page.navigate's errorText. An HTTP
4xx/5xx response or a 3xx redirect that doesn't resolve cleanly
navigates 'successfully' from CDP's perspective, and a 404 / 503 / etc.
PNG gets posted as if it were the deployed app.

After: enable the Network CDP domain before navigating; tap
ws.on('message') for Network.responseReceived events; record the latest
type==='Document' response status for the navigated frame. After
Page.loadEventFired + the 2s settle wait, throw if the captured status
is non-2xx. The processor's existing catch logs and skips the comment
post cleanly.

Auth walls that return 200 (e.g. Vercel deployment protection) are out
of scope — caller-side fix.

Tests: 6 new in agentcore-browser.test.ts:
- 200 → captures as before
- 404 / 503 → throw with status in message
- 301 redirect → throw (defensive)
- non-Document responses (Stylesheet etc.) ignored
- no Network events fire → falls through (pre-#287 behaviour)

Architecture-notes update + Starlight mirror sync. Test file is new on
this branch; PR #275 (the broader screenshot test PR) doesn't touch
this file.

* test(screenshot): cover the screenshot pipeline

Closes #97.

Adds 53 jest tests across the four screenshot files that landed with
PR #241 + #273 with no existing coverage:

- github-webhook-verify.test.ts (14): SHA256 sign/verify, sm cache TTL +
  forceRefresh, ResourceNotFound, transparent re-fetch on signature
  mismatch / null fresh.
- github-webhook.test.ts (15): missing body/sig, ping ack, non-deploy
  events ignored, malformed JSON, state/environment filters,
  SCREENSHOT_TARGET_ENVIRONMENT override, missing fields, dedup hit,
  happy path, rollback-on-invoke-failure, non-condition DDB error.
- linear-issue-lookup.test.ts (18): regex covers extract / multi /
  bounds / case-sensitivity, prefix-routing happy path, case-insensitive
  prefix match, fallback for legacy rows + post-prefix-miss, null token
  skip, fuzzy-match guard, GraphQL errors / non-2xx / network failure.
- github-webhook-processor.test.ts (15): empty / malformed body, missing
  fields, token resolve failure, PR retry exhaustion, OPEN-only filter,
  happy path with CloudFront-host URL assertion, screenshot/S3/comment
  failure modes (each non-fatal where appropriate), Linear branch fires
  / falls back to body / skips on no-id / no-resolve / non-fatal post.
- agentcore-browser.test.ts (6): StartBrowserSession failures, full CDP
  exchange (Target.getTargets -> attach -> enable -> navigate ->
  loadEventFired -> captureScreenshot) returning PNG bytes, Stop
  invoked in finally even on CDP error, Stop's own failure logged not
  thrown, 403 unexpected-response surfaced, navigate errorText raised.

All tests use jest mocks for AWS SDK clients + an in-test FakeWebSocket
for the CDP stream so they run hermetically without real AWS or
network. Existing 286/286 handler tests still pass.

* test(screenshot): adapt cherry-picked pipeline tests to post-#240 API

Updates captureScreenshot budget-arg + high-entropy S3 key assertions
to match main's #240 versions, plus eslint formatting.

---------

Co-authored-by: Alain Krok <alkrok@amazon.com>

Author: isadeks

cdk/src/handlers/shared/agentcore-browser.ts

@@ -262,6 +262,14 @@ async function runCdpScreenshot(wssUrl: string, url: string, timeoutMs: number):
     });
   }
 
+  // Track the main-document HTTP response so we can fail fast on 4xx/5xx
+  // (404 / 503 / auth wall pages) instead of capturing what looks like the
+  // app but isn't. Captured in a Network.responseReceived listener below;
+  // checked after Page.loadEventFired but before Page.captureScreenshot.
+  // (Auth walls that return 200 are out of scope — see issue #287.)
+  let mainDocumentStatus: number | null = null;
+  let mainDocumentFrameId: string | null = null;
+
   try {
     // 1. List existing targets, find the default about:blank page.
     const targetsResp = await cdpSend('Target.getTargets');
@@ -281,9 +289,37 @@ async function runCdpScreenshot(wssUrl: string, url: string, timeoutMs: number):
       throw new Error('Target.attachToTarget did not return a sessionId');
     }
 
-    // 3. Enable Page domain so we get the `Page.loadEventFired` event
-    //    we wait on below.
+    // 3. Enable Page + Network so we get the `Page.loadEventFired` event
+    //    we wait on below AND the main-document response status. Network
+    //    has to be enabled BEFORE Page.navigate, or the response event
+    //    fires before our listener is wired and we miss the status.
     await cdpSend('Page.enable', {}, pageSessionId);
+    await cdpSend('Network.enable', {}, pageSessionId);
+
+    // Tap the raw message stream for Network.responseReceived events —
+    // we want a multi-fire listener (Document responses can appear for
+    // redirect chains), not the one-shot waiter pattern that
+    // eventWaiters / waitForEvent use. Records the latest matching
+    // status; the post-load check below acts on whatever was captured.
+    ws.on('message', (raw: RawData) => {
+      let msg: CdpMessage;
+      try {
+        msg = JSON.parse(raw.toString()) as CdpMessage;
+      } catch {
+        return;
+      }
+      if (msg.method !== 'Network.responseReceived') return;
+      const params = msg.params as
+        | { type?: string; frameId?: string; response?: { status?: number } }
+        | undefined;
+      // CDP's `Network.responseReceived` fires for every resource (HTML,
+      // JS, CSS, images, XHR, …). Only the type==='Document' event for
+      // the navigated frame is the main-document response we care about.
+      if (!params || params.type !== 'Document') return;
+      if (mainDocumentFrameId && params.frameId !== mainDocumentFrameId) return;
+      const status = params.response?.status;
+      if (typeof status === 'number') mainDocumentStatus = status;
+    });
 
     // 4. Navigate. The response includes a `frameId`; we wait on the
     //    `Page.loadEventFired` event below (more reliable than
@@ -294,6 +330,7 @@ async function runCdpScreenshot(wssUrl: string, url: string, timeoutMs: number):
     if (navError) {
       throw new Error(`Page.navigate failed: ${navError}`);
     }
+    mainDocumentFrameId = (navResp.result?.frameId as string | undefined) ?? null;
 
     // 5. Wait for the page load event. SPA-style apps may continue
     //    fetching after this fires, so add a 2s settle wait. For
@@ -302,7 +339,20 @@ async function runCdpScreenshot(wssUrl: string, url: string, timeoutMs: number):
     await waitForEvent('Page.loadEventFired');
     await new Promise((r) => setTimeout(r, 2000));
 
-    // 6. Take the screenshot.
+    // 6. Reject non-2xx main-document statuses before screenshotting.
+    //    A 404 / 503 / auth wall renders a "successful" page from CDP's
+    //    perspective; the user sees a confidently-wrong screenshot of an
+    //    error page posted as the deploy preview. Throw → processor's
+    //    catch logs and skips the PR/Linear comment cleanly.
+    //    If we never captured a status (Network.responseReceived was
+    //    queued but predicate didn't match — e.g. a redirect chain that
+    //    doesn't expose the final frame), fall through and capture
+    //    optimistically; that's the pre-#287 behaviour.
+    if (mainDocumentStatus !== null && (mainDocumentStatus < 200 || mainDocumentStatus >= 300)) {
+      throw new Error(`Preview URL returned HTTP ${mainDocumentStatus}; skipping screenshot`);
+    }
+
+    // 7. Take the screenshot.
     const shotResp = await cdpSend('Page.captureScreenshot', {
       format: 'png',
       captureBeyondViewport: true,

cdk/test/handlers/github-webhook-processor.test.ts

@@ -0,0 +1,300 @@
+/**
+ *  MIT No Attribution
+ *
+ *  Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ *  Permission is hereby granted, free of charge, to any person obtaining a copy of
+ *  the Software without restriction, including without limitation the rights to
+ *  use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of
+ *  the Software, and to permit persons to whom the Software is furnished to do so.
+ *
+ *  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ *  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ *  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ *  AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ *  LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ *  OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ *  SOFTWARE.
+ */
+
+const s3Send = jest.fn();
+jest.mock('@aws-sdk/client-s3', () => ({
+  S3Client: jest.fn(() => ({ send: s3Send })),
+  PutObjectCommand: jest.fn((input: unknown) => ({ _type: 'Put', input })),
+}));
+
+const captureScreenshotMock = jest.fn();
+jest.mock('../../src/handlers/shared/agentcore-browser', () => ({
+  captureScreenshot: (...args: unknown[]) => captureScreenshotMock(...args),
+}));
+
+const resolveGitHubTokenMock = jest.fn();
+jest.mock('../../src/handlers/shared/context-hydration', () => ({
+  resolveGitHubToken: (...args: unknown[]) => resolveGitHubTokenMock(...args),
+}));
+
+const upsertTaskCommentMock = jest.fn();
+jest.mock('../../src/handlers/shared/github-comment', () => ({
+  upsertTaskComment: (...args: unknown[]) => upsertTaskCommentMock(...args),
+}));
+
+const postIssueCommentMock = jest.fn();
+jest.mock('../../src/handlers/shared/linear-feedback', () => ({
+  postIssueComment: (...args: unknown[]) => postIssueCommentMock(...args),
+}));
+
+const findLinearIssueMock = jest.fn();
+const extractLinearIdentifierMock = jest.fn();
+jest.mock('../../src/handlers/shared/linear-issue-lookup', () => ({
+  findLinearIssueByIdentifier: (...args: unknown[]) => findLinearIssueMock(...args),
+  extractLinearIdentifier: (...args: unknown[]) => extractLinearIdentifierMock(...args),
+}));
+
+process.env.SCREENSHOT_BUCKET_NAME = 'screenshot-bucket';
+process.env.SCREENSHOT_PUBLIC_HOST = 'd1.cloudfront.net';
+process.env.GITHUB_TOKEN_SECRET_ARN = 'arn:aws:secretsmanager:us-east-1:123:secret:gh-token';
+process.env.LINEAR_WORKSPACE_REGISTRY_TABLE_NAME = 'LinearWorkspaceRegistry';
+
+import { handler } from '../../src/handlers/github-webhook-processor';
+
+function payload(overrides: Record<string, unknown> = {}): { raw_body: string } {
+  const body = {
+    deployment_status: {
+      id: 99,
+      state: 'success',
+      environment_url: 'https://preview.example.com',
+    },
+    deployment: { id: 42, sha: 'abc1234', environment: 'Preview' },
+    repository: { full_name: 'owner/repo' },
+    ...overrides,
+  };
+  return { raw_body: JSON.stringify(body) };
+}
+
+function fetchOk(jsonValue: unknown, status = 200): jest.SpyInstance {
+  return jest.spyOn(global, 'fetch').mockResolvedValueOnce({
+    ok: status >= 200 && status < 300,
+    status,
+    json: async () => jsonValue,
+  } as unknown as Response);
+}
+
+describe('github-webhook-processor handler', () => {
+  beforeEach(() => {
+    s3Send.mockReset();
+    captureScreenshotMock.mockReset();
+    resolveGitHubTokenMock.mockReset();
+    upsertTaskCommentMock.mockReset();
+    postIssueCommentMock.mockReset();
+    findLinearIssueMock.mockReset();
+    extractLinearIdentifierMock.mockReset();
+    jest.restoreAllMocks();
+  });
+
+  test('returns silently when raw_body is empty', async () => {
+    await handler({ raw_body: '' });
+    expect(resolveGitHubTokenMock).not.toHaveBeenCalled();
+  });
+
+  test('returns silently when raw_body is malformed JSON', async () => {
+    await handler({ raw_body: 'not-json{' });
+    expect(resolveGitHubTokenMock).not.toHaveBeenCalled();
+  });
+
+  test('returns when payload missing repo/sha/preview_url', async () => {
+    await handler({ raw_body: JSON.stringify({ deployment: { id: 42 } }) });
+    expect(resolveGitHubTokenMock).not.toHaveBeenCalled();
+  });
+
+  test('returns when GitHub token cannot be resolved', async () => {
+    resolveGitHubTokenMock.mockRejectedValueOnce(new Error('SM unavailable'));
+    await handler(payload());
+    expect(captureScreenshotMock).not.toHaveBeenCalled();
+  });
+
+  test('returns when no open PR is associated with the SHA after retries', async () => {
+    jest.useFakeTimers();
+    try {
+      resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+      // Four calls (delays = [0, 5s, 10s, 20s]) all return empty list.
+      fetchOk([]);
+      fetchOk([]);
+      fetchOk([]);
+      fetchOk([]);
+      const promise = handler(payload());
+      await jest.runAllTimersAsync();
+      await promise;
+      expect(captureScreenshotMock).not.toHaveBeenCalled();
+    } finally {
+      jest.useRealTimers();
+    }
+  });
+
+  test('only OPEN PRs are accepted (closed/merged are filtered)', async () => {
+    jest.useFakeTimers();
+    try {
+      resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+      fetchOk([{ number: 1, state: 'closed', title: 'old', body: '' }]);
+      fetchOk([{ number: 1, state: 'closed', title: 'old', body: '' }]);
+      fetchOk([{ number: 1, state: 'closed', title: 'old', body: '' }]);
+      fetchOk([{ number: 1, state: 'closed', title: 'old', body: '' }]);
+      const promise = handler(payload());
+      await jest.runAllTimersAsync();
+      await promise;
+      expect(captureScreenshotMock).not.toHaveBeenCalled();
+    } finally {
+      jest.useRealTimers();
+    }
+  });
+
+  test('happy path: PR found → screenshot → S3 → PR comment posted', async () => {
+    resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+    fetchOk([{ number: 17, state: 'open', title: 'feat: add x', body: 'body' }]);
+    captureScreenshotMock.mockResolvedValueOnce(new Uint8Array([1, 2, 3]));
+    s3Send.mockResolvedValueOnce({});
+    upsertTaskCommentMock.mockResolvedValueOnce({ commentId: 'cmt-1' });
+
+    await handler(payload());
+
+    // captureScreenshot now receives a deadline-aware budget (PR-241 B1).
+    expect(captureScreenshotMock).toHaveBeenCalledWith(
+      'https://preview.example.com',
+      expect.objectContaining({ timeoutMs: expect.any(Number) }),
+    );
+    expect(s3Send).toHaveBeenCalledTimes(1);
+    const putArg = (s3Send.mock.calls[0][0] as { input: { Key: string; ContentType: string } }).input;
+    // Key carries the high-entropy suffix added in PR-241 (key entropy).
+    expect(putArg.Key).toMatch(/^screenshots\/owner_repo\/abc1234-42-[0-9a-f]{16}\.png$/);
+    expect(putArg.ContentType).toBe('image/png');
+    expect(upsertTaskCommentMock).toHaveBeenCalledTimes(1);
+    const commentArg = upsertTaskCommentMock.mock.calls[0][0] as { repo: string; issueOrPrNumber: number; body: string };
+    expect(commentArg.repo).toBe('owner/repo');
+    expect(commentArg.issueOrPrNumber).toBe(17);
+    expect(commentArg.body).toMatch(/https:\/\/d1\.cloudfront\.net\/screenshots\/owner_repo\/abc1234-42-[0-9a-f]{16}\.png/);
+  });
+
+  test('aborts when screenshot capture throws', async () => {
+    resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+    fetchOk([{ number: 17, state: 'open', title: 't', body: '' }]);
+    captureScreenshotMock.mockRejectedValueOnce(new Error('CDP failed'));
+
+    await handler(payload());
+
+    expect(s3Send).not.toHaveBeenCalled();
+    expect(upsertTaskCommentMock).not.toHaveBeenCalled();
+  });
+
+  test('aborts when S3 PutObject throws', async () => {
+    resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+    fetchOk([{ number: 17, state: 'open', title: 't', body: '' }]);
+    captureScreenshotMock.mockResolvedValueOnce(new Uint8Array([1]));
+    s3Send.mockRejectedValueOnce(new Error('S3 throttled'));
+
+    await handler(payload());
+
+    expect(upsertTaskCommentMock).not.toHaveBeenCalled();
+  });
+
+  test('PR comment failure is non-fatal (log + continue)', async () => {
+    resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+    fetchOk([{ number: 17, state: 'open', title: 't', body: '' }]);
+    captureScreenshotMock.mockResolvedValueOnce(new Uint8Array([1]));
+    s3Send.mockResolvedValueOnce({});
+    upsertTaskCommentMock.mockRejectedValueOnce(new Error('GitHub 502'));
+
+    // Should not throw — the handler is best-effort.
+    await expect(handler(payload())).resolves.toBeUndefined();
+  });
+
+  test('Linear branch fires when registry table set + identifier in PR title', async () => {
+    resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+    fetchOk([{ number: 17, state: 'open', title: 'ABCA-42 fix login', body: 'body' }]);
+    captureScreenshotMock.mockResolvedValueOnce(new Uint8Array([1]));
+    s3Send.mockResolvedValueOnce({});
+    upsertTaskCommentMock.mockResolvedValueOnce({ commentId: 'cmt-1' });
+    extractLinearIdentifierMock.mockReturnValueOnce('ABCA-42');
+    findLinearIssueMock.mockResolvedValueOnce({
+      issueId: 'issue-uuid',
+      linearWorkspaceId: 'ws-1',
+      workspaceSlug: 'abca',
+    });
+    postIssueCommentMock.mockResolvedValueOnce(true);
+
+    await handler(payload());
+
+    expect(extractLinearIdentifierMock).toHaveBeenCalledWith('ABCA-42 fix login');
+    expect(findLinearIssueMock).toHaveBeenCalledWith('ABCA-42', 'LinearWorkspaceRegistry');
+    expect(postIssueCommentMock).toHaveBeenCalledTimes(1);
+    const linearArg = postIssueCommentMock.mock.calls[0];
+    expect(linearArg[1]).toBe('issue-uuid');
+    expect(linearArg[2]).toMatch(/https:\/\/d1\.cloudfront\.net\/screenshots\/owner_repo\/abc1234-42-[0-9a-f]{16}\.png/);
+  });
+
+  test('falls back to extractor on PR body when title yields no identifier', async () => {
+    resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+    fetchOk([{ number: 17, state: 'open', title: 'feat: add foo', body: 'closes ABCA-42' }]);
+    captureScreenshotMock.mockResolvedValueOnce(new Uint8Array([1]));
+    s3Send.mockResolvedValueOnce({});
+    upsertTaskCommentMock.mockResolvedValueOnce({ commentId: 'cmt-1' });
+    extractLinearIdentifierMock
+      .mockReturnValueOnce(null) // title produces no match
+      .mockReturnValueOnce('ABCA-42'); // body does
+    findLinearIssueMock.mockResolvedValueOnce({
+      issueId: 'issue-uuid',
+      linearWorkspaceId: 'ws-1',
+      workspaceSlug: 'abca',
+    });
+    postIssueCommentMock.mockResolvedValueOnce(true);
+
+    await handler(payload());
+
+    expect(extractLinearIdentifierMock).toHaveBeenCalledTimes(2);
+    expect(postIssueCommentMock).toHaveBeenCalledTimes(1);
+  });
+
+  test('skips Linear when no identifier extracted', async () => {
+    resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+    fetchOk([{ number: 17, state: 'open', title: 'no id', body: 'no id' }]);
+    captureScreenshotMock.mockResolvedValueOnce(new Uint8Array([1]));
+    s3Send.mockResolvedValueOnce({});
+    upsertTaskCommentMock.mockResolvedValueOnce({ commentId: 'cmt-1' });
+    extractLinearIdentifierMock.mockReturnValue(null);
+
+    await handler(payload());
+
+    expect(findLinearIssueMock).not.toHaveBeenCalled();
+    expect(postIssueCommentMock).not.toHaveBeenCalled();
+  });
+
+  test('skips Linear post when identifier does not resolve to an issue', async () => {
+    resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+    fetchOk([{ number: 17, state: 'open', title: 'ABCA-42 stale', body: '' }]);
+    captureScreenshotMock.mockResolvedValueOnce(new Uint8Array([1]));
+    s3Send.mockResolvedValueOnce({});
+    upsertTaskCommentMock.mockResolvedValueOnce({ commentId: 'cmt-1' });
+    extractLinearIdentifierMock.mockReturnValueOnce('ABCA-42');
+    findLinearIssueMock.mockResolvedValueOnce(null);
+
+    await handler(payload());
+
+    expect(postIssueCommentMock).not.toHaveBeenCalled();
+  });
+
+  test('Linear comment failure does not propagate (best-effort)', async () => {
+    resolveGitHubTokenMock.mockResolvedValue('gh-tok');
+    fetchOk([{ number: 17, state: 'open', title: 'ABCA-42 fix', body: '' }]);
+    captureScreenshotMock.mockResolvedValueOnce(new Uint8Array([1]));
+    s3Send.mockResolvedValueOnce({});
+    upsertTaskCommentMock.mockResolvedValueOnce({ commentId: 'cmt-1' });
+    extractLinearIdentifierMock.mockReturnValueOnce('ABCA-42');
+    findLinearIssueMock.mockResolvedValueOnce({
+      issueId: 'issue-uuid',
+      linearWorkspaceId: 'ws-1',
+      workspaceSlug: 'abca',
+    });
+    postIssueCommentMock.mockResolvedValueOnce(false);
+
+    // No throw — postIssueComment returning false is just logged.
+    await expect(handler(payload())).resolves.toBeUndefined();
+  });
+});