Merge branch 'main' into feat/257-semgrep-silent-success-masking

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/src/handlers/shared/linear-issue-lookup.ts

@@ -73,14 +73,17 @@ query IssueByIdentifier($identifier: String!) {
 `.trim();
 
 /**
- * Look up a Linear issue by identifier (e.g. `ABCA-42`) by iterating
- * over every active workspace in the registry until one returns a
- * match. Returns the first hit.
+ * Look up a Linear issue by identifier (e.g. `ABCA-42`).
  *
- * For v1 this scan is cheap — typical deployments have 1-2 workspaces.
- * If a stack ever onboards many workspaces sharing identifier prefixes,
- * a followup can store team_key prefixes on the registry row and route
- * directly. Until then, linear-time iteration is fine.
+ * Routing strategy:
+ * 1. Scan active workspaces (one round-trip — typical stacks have 1–2).
+ * 2. If any row's `team_keys` contains the identifier's team key (`ABCA`),
+ *    query that workspace directly and return on hit.
+ * 3. Otherwise fall back to iterating every active workspace until one
+ *    returns a match. This handles legacy rows missing `team_keys` (the
+ *    column was added in #96 and back-fills only on next `setup` /
+ *    `add-workspace` re-run) and the rare case where a team was added in
+ *    Linear after the workspace was registered.
  *
  * @param identifier `ABCA-42`-style Linear issue identifier
  * @param registryTableName name of LinearWorkspaceRegistryTable
@@ -90,7 +93,11 @@ export async function findLinearIssueByIdentifier(
   identifier: string,
   registryTableName: string,
 ): Promise<LinearIssueLocation | null> {
-  let active: Array<{ linear_workspace_id: string; workspace_slug: string }> = [];
+  let active: Array<{
+    linear_workspace_id: string;
+    workspace_slug: string;
+    team_keys: string[] | null;
+  }> = [];
   try {
     const scanResp = await ddb.send(new ScanCommand({
       TableName: registryTableName,
@@ -101,6 +108,7 @@ export async function findLinearIssueByIdentifier(
     active = (scanResp.Items ?? []).map((item) => ({
       linear_workspace_id: item.linear_workspace_id as string,
       workspace_slug: item.workspace_slug as string,
+      team_keys: Array.isArray(item.team_keys) ? (item.team_keys as string[]) : null,
     }));
   } catch (err) {
     logger.warn('Linear issue lookup: failed to scan workspace registry', {
@@ -114,7 +122,33 @@ export async function findLinearIssueByIdentifier(
     return null;
   }
 
+  // Identifier prefix is the part before the first dash (`ABCA-42` → `ABCA`).
+  // Compare uppercase since Linear team keys are upper-case but inbound text
+  // (PR titles, branch names) is mixed-case.
+  const teamKey = identifier.split('-', 1)[0]?.toUpperCase();
+  const prefixMatch = teamKey
+    ? active.find((ws) => ws.team_keys?.some((k) => k.toUpperCase() === teamKey))
+    : undefined;
+
+  // Try the prefix-matched workspace first.
+  if (prefixMatch) {
+    const resolved = await resolveLinearOauthToken(prefixMatch.linear_workspace_id, registryTableName);
+    if (resolved) {
+      const found = await queryIssueByIdentifier(resolved.accessToken, identifier);
+      if (found) {
+        return {
+          issueId: found,
+          linearWorkspaceId: prefixMatch.linear_workspace_id,
+          workspaceSlug: prefixMatch.workspace_slug,
+        };
+      }
+    }
+  }
+
+  // Fallback: iterate workspaces NOT already tried via prefix-match.
+  // Covers legacy rows without `team_keys` and post-registration team adds.
   for (const ws of active) {
+    if (prefixMatch && ws.linear_workspace_id === prefixMatch.linear_workspace_id) continue;
     const resolved = await resolveLinearOauthToken(ws.linear_workspace_id, registryTableName);
     if (!resolved) continue;