diff --git a/cdk/src/handlers/github-webhook-processor.ts b/cdk/src/handlers/github-webhook-processor.ts index 39f60cac..73b4fd2a 100644 --- a/cdk/src/handlers/github-webhook-processor.ts +++ b/cdk/src/handlers/github-webhook-processor.ts @@ -34,6 +34,7 @@ import { findLinearIssueByIdentifier, } from './shared/linear-issue-lookup'; import { logger } from './shared/logger'; +import { type LookupResult, LOOKUP_ABSENT, lookupFailed, lookupFound, lookupValueOr } from './shared/lookup-result'; import { isIntegrationNode } from './shared/orchestration-integration-node'; import { buildScreenshotKey, encodeMarkdownUrl, extractTaskIdFromBranch, isAllowedScreenshotUrl } from './shared/screenshot-url'; import { makeClient, makeDocClient } from './shared/ua'; @@ -344,7 +345,7 @@ export async function handler(event: ProcessorEvent): Promise { // link to that reply now (in place). Find the most-recent iteration // reply id for this issue and edit it; idempotent via the [preview] // marker so a webhook redelivery won't double-append. - const iter = await findIterationReplyId(linearIssue.issueId, sha); + const iter = lookupValueOr(await findIterationReplyId(linearIssue.issueId, sha), null); if (iter) { // (1) Durably persist the screenshot onto the ITERATION task so the // terminal-settle renders the thumbnail from a strongly-consistent @@ -412,8 +413,8 @@ export async function handler(event: ProcessorEvent): Promise { async function findIterationReplyId( linearIssueId: string, deploySha?: string, -): Promise<{ replyId: string; taskId: string } | null> { - if (!TASK_TABLE) return null; +): Promise> { + if (!TASK_TABLE) return LOOKUP_ABSENT; try { const res = await ddb.send(new QueryCommand({ TableName: TASK_TABLE, @@ -426,7 +427,7 @@ async function findIterationReplyId( .map((item) => ({ taskId: item.task_id, replyId: item.channel_metadata?.iteration_reply_comment_id })) .filter((c): c is { taskId: string; replyId: string } => typeof c.taskId === 'string' && typeof c.replyId === 'string' && c.replyId.length > 0); - if (candidates.length === 0) return null; + if (candidates.length === 0) return LOOKUP_ABSENT; // Prefer the task whose pushed head_sha matches this deploy's commit (correct // attribution under overlapping iterations). Walk newest-first; GetItem the @@ -436,16 +437,16 @@ async function findIterationReplyId( const got = await ddb.send(new GetCommand({ TableName: TASK_TABLE, Key: { task_id: c.taskId }, ProjectionExpression: 'head_sha', })); - if (got.Item?.head_sha === deploySha) return { replyId: c.replyId, taskId: c.taskId }; + if (got.Item?.head_sha === deploySha) return lookupFound({ replyId: c.replyId, taskId: c.taskId }); } } // No SHA match (pre-fix task / non-PR deploy) → newest reply-bearing task. - return { replyId: candidates[0].replyId, taskId: candidates[0].taskId }; + return lookupFound({ replyId: candidates[0].replyId, taskId: candidates[0].taskId }); } catch (err) { logger.warn('findIterationReplyId query failed (non-fatal)', { linear_issue_id: linearIssueId, error: err instanceof Error ? err.message : String(err), }); - return null; + return lookupFailed(err); } } diff --git a/cdk/src/handlers/jira-webhook-processor.ts b/cdk/src/handlers/jira-webhook-processor.ts index 6111bc1a..ba885b9b 100644 --- a/cdk/src/handlers/jira-webhook-processor.ts +++ b/cdk/src/handlers/jira-webhook-processor.ts @@ -48,6 +48,7 @@ import { import { resolveSoleActiveJiraTenant } from './shared/jira-tenant-registry'; import type { SubIssueNode } from './shared/linear-subissue-fetch'; import { logger } from './shared/logger'; +import { lookupValueOr } from './shared/lookup-result'; import type { CommentRef } from './shared/orchestration-channel'; import { makeJiraChannel } from './shared/orchestration-channel-jira'; import { parseRetryIntent } from './shared/orchestration-comment-trigger'; @@ -634,7 +635,7 @@ export async function handler(event: ProcessorEvent): Promise { discovery.orchestrationId, ); if (fresh) { - const commentId = await upsertEpicPanel({ + const commentId = lookupValueOr(await upsertEpicPanel({ channel: makeJiraChannel(WORKSPACE_REGISTRY_TABLE), parent: { issueId: issue.key, @@ -646,7 +647,7 @@ export async function handler(event: ProcessorEvent): Promise { }, children: fresh.children, labelFilter, - }); + }), null); if (commentId) { await setStatusCommentId( ddb, @@ -718,7 +719,7 @@ export async function handler(event: ProcessorEvent): Promise { try { // Unlike seed, extension already has a durable snapshot. Refresh the // panel from it even if a post-release read is temporarily unavailable. - const commentId = await upsertEpicPanel({ + const commentId = lookupValueOr(await upsertEpicPanel({ channel: makeJiraChannel(WORKSPACE_REGISTRY_TABLE), parent: { issueId: issue.key, @@ -738,7 +739,7 @@ export async function handler(event: ProcessorEvent): Promise { children: panelSnapshot.children, inProgress: true, labelFilter, - }); + }), null); if (commentId && !panelSnapshot.meta.status_comment_id) { await setStatusCommentId( ddb, @@ -1184,7 +1185,7 @@ async function handleJiraEpicRetry( if (WORKSPACE_REGISTRY_TABLE) { const refreshed = await loadOrchestration(ddb, ORCHESTRATION_TABLE, orchestrationId); if (refreshed) { - const panelId = await upsertEpicPanel({ + const panelId = lookupValueOr(await upsertEpicPanel({ channel: makeJiraChannel(WORKSPACE_REGISTRY_TABLE), parent: { issueId: parentIssueKey, @@ -1202,7 +1203,7 @@ async function handleJiraEpicRetry( statusCommentId: refreshed.meta.status_comment_id, inProgress: true, labelFilter: refreshed.meta.release_context.trigger_label, - }); + }), null); if (panelId) { await setStatusCommentId(ddb, ORCHESTRATION_TABLE, orchestrationId, panelId); } diff --git a/cdk/src/handlers/linear-webhook-processor.ts b/cdk/src/handlers/linear-webhook-processor.ts index 66a770ac..20fedbad 100644 --- a/cdk/src/handlers/linear-webhook-processor.ts +++ b/cdk/src/handlers/linear-webhook-processor.ts @@ -50,6 +50,7 @@ import { resolveLinearOauthToken } from './shared/linear-oauth-resolver'; import { fetchIssueParentId } from './shared/linear-subissue-fetch'; import { lookupTaskByLinearIssue, prNumberFromTask } from './shared/linear-task-by-issue'; import { logger } from './shared/logger'; +import { type LookupResult, LOOKUP_ABSENT, isLookupFailure, lookupFailed, lookupFound, lookupValueOr } from './shared/lookup-result'; import { type Channel, type IssueRef } from './shared/orchestration-channel'; import { makeLinearChannel } from './shared/orchestration-channel-linear'; import { @@ -74,6 +75,7 @@ import { computeEpicRetryPlan } from './shared/orchestration-reconcile'; import { applyTerminalCreateFailures, readConcurrencyBudget, releaseReadyChildren } from './shared/orchestration-release'; import { upsertEpicPanel } from './shared/orchestration-rollup'; import { claimCommentAck, clearRollupClaim, deriveOrchestrationId, loadOrchestration, setChildOwnAttachments, setRetryCommentId, setStatusCommentId, type OrchestrationChildRow, type OrchestrationReleaseContext } from './shared/orchestration-store'; +import { readTaskPrNumber } from './shared/task-pr-number'; import { DEFAULT_LABEL_FILTER, hasHelpLabel, HELP_SUFFIX } from './shared/trigger-label'; import type { Attachment, PassedAttachmentRecord } from './shared/types'; import { makeClient, makeDocClient } from './shared/ua'; @@ -515,7 +517,7 @@ async function postIterationAck( registryTableName: string, issueId: string, replyTargetId: string, -): Promise { +): Promise> { try { const ref = await channelFor(registryTableName).upsertThreadedReply?.( issueRef(issueId, workspaceId), @@ -524,12 +526,12 @@ async function postIterationAck( ); // An empty id means the surface posted but can't address the reply later — // report "no reply to mature" rather than stamping a blank id on the task. - return ref?.commentId || null; + return ref?.commentId ? lookupFound(ref.commentId) : LOOKUP_ABSENT; } catch (err) { logger.warn('Iteration ack reply failed (non-fatal)', { issue_id: issueId, error: err instanceof Error ? err.message : String(err), }); - return null; + return lookupFailed(err); } } @@ -1173,14 +1175,14 @@ export async function handler(event: ProcessorEvent): Promise { const settled = seedHadTerminalFailure && postReleaseSnapshot.children.every( (c) => c.child_status === 'succeeded' || c.child_status === 'failed' || c.child_status === 'skipped', ); - const commentId = await upsertEpicPanel({ + const commentId = lookupValueOr(await upsertEpicPanel({ channel: channelFor(WORKSPACE_REGISTRY_TABLE), parent: issueRef(issue.id, workspaceId), children: postReleaseSnapshot.children, ...seedFailureReasons(postReleaseSnapshot.children), inProgress: !settled, mirrorParentState: true, - }); + }), null); if (commentId) { await setStatusCommentId(ddb, ORCHESTRATION_TABLE, discovery.orchestrationId, commentId); } @@ -1283,13 +1285,13 @@ export async function handler(event: ProcessorEvent): Promise { const fresh = await loadOrchestration(ddb, ORCHESTRATION_TABLE, discovery.orchestrationId); const children = fresh?.children ?? snapshot.children; const meta = (fresh ?? snapshot).meta; - const newId = await upsertEpicPanel({ + const newId = lookupValueOr(await upsertEpicPanel({ channel: channelFor(WORKSPACE_REGISTRY_TABLE), parent: issueRef(issue.id, workspaceId), ...(meta.status_comment_id !== undefined && { statusCommentId: meta.status_comment_id }), children, inProgress: true, // the extend re-opened the epic - }); + }), null); if (newId && meta.status_comment_id === undefined) { await setStatusCommentId(ddb, ORCHESTRATION_TABLE, discovery.orchestrationId, newId); } @@ -1679,13 +1681,13 @@ async function maybeRetryTerminalEpic( ); } // Post the panel FRESH (no statusCommentId → new comment, below the note). - const newPanelId = await upsertEpicPanel({ + const newPanelId = lookupValueOr(await upsertEpicPanel({ channel, parent: parentRef, children, inProgress: true, mirrorParentState: true, - }); + }), null); if (newPanelId) { await setStatusCommentId(ddb, ORCHESTRATION_TABLE, orchestrationId, newPanelId); } @@ -1929,7 +1931,18 @@ async function handleCommentTrigger(payload: LinearCommentEvent): Promise // the issue may still be a plain (non-orchestration) issue that ABCA opened // a PR for — fall through to the standalone path, which iterates // on that PR with the same 👀/reply ack but no dependency cascade. - const parentId = await fetchIssueParentId(resolved.accessToken, commentedIssueId); + const parentResult = await fetchIssueParentId(resolved.accessToken, commentedIssueId); + if (isLookupFailure(parentResult)) { + // The parent lookup broke (Linear outage / GraphQL error). Do NOT fall + // through to the standalone path — that would silently downgrade an + // orchestration child. Defer; a stream replay re-drives once Linear recovers. + logger.warn('Comment trigger: issue-parent lookup failed — deferring (not downgrading to standalone)', { + issue_id: commentedIssueId, + error: parentResult.error instanceof Error ? parentResult.error.message : String(parentResult.error), + }); + return; + } + const parentId = lookupValueOr(parentResult, null); const orchestrationId = parentId ? deriveOrchestrationId(parentId) : null; const snapshot = orchestrationId ? await loadOrchestration(ddb, ORCHESTRATION_TABLE, orchestrationId) @@ -2083,8 +2096,19 @@ async function handleParentEpicCommentTrigger(args: { return; } - const prNumber = await resolveChildPrNumber(target.child_task_id); - if (prNumber === null) { + const prNumberResult = await readTaskPrNumber(ddb, process.env.TASK_TABLE_NAME!, target.child_task_id); + if (isLookupFailure(prNumberResult)) { + // The read broke — don't tell the user "no PR yet" (a distinct, misleading + // state). Leave the 👀 in place so a stream replay can re-drive it. + logger.warn('Comment trigger (parent epic): sub-issue PR read failed — deferring', { + orchestration_id: orchestrationId, + sub_issue_id: target.sub_issue_id, + child_task_id: target.child_task_id, + error: prNumberResult.error instanceof Error ? prNumberResult.error.message : String(prNumberResult.error), + }); + return; + } + if (!prNumberResult.ok) { // Matched a node but it has no PR to iterate. If that node FAILED, the user // named it to fix it — there's nothing to iterate (no PR), so point them // straight at retry instead of the generic disambiguation. Observed in @@ -2106,6 +2130,7 @@ async function handleParentEpicCommentTrigger(args: { }); return; } + const prNumber = prNumberResult.value; // Resolve the FULL child row (the matcher returns a trimmed view without // ``repo``) so the iteration carries the sub-issue's repo. @@ -2173,12 +2198,32 @@ async function iterateOrchestrationChild(args: { const subIssueId = child.sub_issue_id; const triggerCommentIssueId = args.triggerCommentIssueId ?? subIssueId; - const prNumber = args.prNumber ?? (child.child_task_id ? await resolveChildPrNumber(child.child_task_id) : null); - if (prNumber === null || prNumber === undefined) { - logger.warn('Comment trigger: sub-issue has no resolvable PR — cannot iterate', { - orchestration_id: orchestrationId, sub_issue_id: subIssueId, child_task_id: child.child_task_id, - }); - return; + let prNumber: number; + if (args.prNumber !== undefined) { + prNumber = args.prNumber; + } else { + const prNumberResult = child.child_task_id + ? await readTaskPrNumber(ddb, process.env.TASK_TABLE_NAME!, child.child_task_id) + : LOOKUP_ABSENT; + if (!prNumberResult.ok) { + // Can't iterate without a PR either way, but log an outage distinctly + // from a genuinely-absent PR so it isn't misread as "nothing to do". + logger.warn( + isLookupFailure(prNumberResult) + ? 'Comment trigger: sub-issue PR read failed — cannot iterate' + : 'Comment trigger: sub-issue has no resolvable PR — cannot iterate', + { + orchestration_id: orchestrationId, + sub_issue_id: subIssueId, + child_task_id: child.child_task_id, + ...(isLookupFailure(prNumberResult) && { + error: prNumberResult.error instanceof Error ? prNumberResult.error.message : String(prNumberResult.error), + }), + }, + ); + return; + } + prNumber = prNumberResult.value; } // Attribute to the orchestration's release user (the comment author may not @@ -2198,7 +2243,7 @@ async function iterateOrchestrationChild(args: { // silence) and persist its id so the fanout dispatcher matures THIS reply // (🔄→✅/💬) instead of posting new top-level comments. The reply threads under // the conversation root (replyTargetId) on the issue the comment lives on. - const iterationReplyId = await postIterationAck(workspaceId, registryTableName, triggerCommentIssueId, replyTargetId); + const iterationReplyId = lookupValueOr(await postIterationAck(workspaceId, registryTableName, triggerCommentIssueId, replyTargetId), null); // Idempotency: one iteration per (sub-issue, comment). The comment id is // unique per comment, so a webhook retry of the same comment dedups. @@ -2433,7 +2478,7 @@ async function handleStandaloneCommentTrigger(args: { await channel.reactToComment?.({ commentId }, target, 'started'); // Immediate "👀 On it" threaded reply + persist its id so the fanout dispatcher // matures THIS reply instead of posting new comments. - const iterationReplyId = await postIterationAck(workspaceId, registryTableName, issueId, replyTargetId); + const iterationReplyId = lookupValueOr(await postIterationAck(workspaceId, registryTableName, issueId, replyTargetId), null); const idempotencyKey = `iterate_${issueId}_${commentId}`.replace(/[^A-Za-z0-9_-]/g, '').slice(0, MAX_IDEMPOTENCY_KEY_LENGTH); const channelMetadata: Record = { @@ -2600,7 +2645,7 @@ async function maybeStartStandaloneNewWork(args: { // ACK immediately (👀 reaction + threaded "On it"), same as the iteration and // clarify-resume paths. await channel.reactToComment?.({ commentId }, target, 'started'); - const iterationReplyId = await postIterationAck(workspaceId, registryTableName, issueId, replyTargetId); + const iterationReplyId = lookupValueOr(await postIterationAck(workspaceId, registryTableName, issueId, replyTargetId), null); // Idempotency: key on (issue, comment) so a webhook redelivery of the SAME // comment doesn't spawn a second task. Distinct prefix from iterate_/clarify_. @@ -2729,7 +2774,7 @@ async function maybeResumeClarifyHold(args: { const channel = channelFor(registryTableName); const target = issueRef(issueId, workspaceId); await channel.reactToComment?.({ commentId }, target, 'started'); - const iterationReplyId = await postIterationAck(workspaceId, registryTableName, issueId, replyTargetId); + const iterationReplyId = lookupValueOr(await postIterationAck(workspaceId, registryTableName, issueId, replyTargetId), null); const resumeDescription = buildClarifyResumeDescription( typeof row.task_description === 'string' ? row.task_description : undefined, @@ -2802,27 +2847,6 @@ async function maybeResumeClarifyHold(args: { return true; } -/** Read a child task's PR number (numeric pr_number, else parse pr_url). Null if neither. */ -async function resolveChildPrNumber(taskId: string): Promise { - try { - const res = await ddb.send(new GetCommand({ TableName: process.env.TASK_TABLE_NAME!, Key: { task_id: taskId } })); - const pr = res.Item?.pr_number; - if (typeof pr === 'number') return pr; - const url = res.Item?.pr_url; - if (typeof url === 'string') { - const m = url.match(/\/pull\/(\d+)\b/); - if (m) return Number(m[1]); - } - return null; - } catch (err) { - logger.warn('Comment trigger: failed to read sub-issue task record for PR number', { - task_id: taskId, - error: err instanceof Error ? err.message : String(err), - }); - return null; - } -} - /** * Decide whether a Linear Issue event should trigger a task. * diff --git a/cdk/src/handlers/orchestration-reconciler.ts b/cdk/src/handlers/orchestration-reconciler.ts index 19d7a938..9ac0228c 100644 --- a/cdk/src/handlers/orchestration-reconciler.ts +++ b/cdk/src/handlers/orchestration-reconciler.ts @@ -59,6 +59,7 @@ import { renderJiraFinishedPointer, } from './shared/jira-status-comment'; import { logger } from './shared/logger'; +import { type LookupResult, LOOKUP_ABSENT, isLookupFailure, lookupFailed, lookupFound, lookupValueOr } from './shared/lookup-result'; import type { Channel, CommentRef, IssueRef } from './shared/orchestration-channel'; import { channelForSource, type ChannelRegistryTables } from './shared/orchestration-channel-factory'; import { computeLeaves, isIntegrationNode } from './shared/orchestration-integration-node'; @@ -81,6 +82,7 @@ import { type OrchestrationChildRow, } from './shared/orchestration-store'; import { encodeMarkdownUrl } from './shared/screenshot-url'; +import { readTaskPrNumber } from './shared/task-pr-number'; import { makeDocClient } from './shared/ua'; import { OrchestrationTable } from '../constructs/orchestration-table'; import { TaskStatus, TERMINAL_STATUSES, type TaskStatusType } from '../constructs/task-status'; @@ -447,8 +449,8 @@ function soleLeafChild( */ async function resolveCombinedScreenshotUrl( taskId?: string, -): Promise<{ url: string; previewUrl?: string } | null> { - if (!taskId) return null; +): Promise> { + if (!taskId) return LOOKUP_ABSENT; try { const res = await ddb.send(new GetCommand({ TableName: TASK_TABLE, @@ -456,19 +458,19 @@ async function resolveCombinedScreenshotUrl( ProjectionExpression: 'screenshot_url, screenshot_preview_url', })); const url = res.Item?.screenshot_url; - if (typeof url !== 'string' || url.length === 0) return null; + if (typeof url !== 'string' || url.length === 0) return LOOKUP_ABSENT; const previewUrl = res.Item?.screenshot_preview_url; // The live preview-deploy URL makes the panel's combined // preview a clickable deep-link to the running combined site. - return { + return lookupFound({ url, ...(typeof previewUrl === 'string' && previewUrl.length > 0 && { previewUrl }), - }; + }); } catch (err) { logger.warn('Combined screenshot read failed (non-fatal) — panel posts without it', { task_id: taskId, error: err instanceof Error ? err.message : String(err), }); - return null; + return lookupFailed(err); } } @@ -779,7 +781,7 @@ export async function refreshPanelAndSettle( // Only read on the all-terminal settle (the node has deployed by then); skip // the extra Get on every in-flight panel edit. const combinedScreenshot = (allTerminal && previewNode) - ? await resolveCombinedScreenshotUrl(previewNode.child_task_id) + ? lookupValueOr(await resolveCombinedScreenshotUrl(previewNode.child_task_id), null) : null; if (allTerminal) { @@ -798,7 +800,7 @@ export async function refreshPanelAndSettle( // all-terminal caller. The panel BODY edit is naturally idempotent. const won = !allTerminal || await claimRollup(ddb, ORCHESTRATION_TABLE, orchestrationId, now); - const newId = await upsertEpicPanel({ + const newId = lookupValueOr(await upsertEpicPanel({ channel, parent: issueRef(meta.parent_issue_ref, meta.credentials_ref, meta.release_context), ...(meta.status_comment_id !== undefined && { statusCommentId: meta.status_comment_id }), @@ -812,7 +814,7 @@ export async function refreshPanelAndSettle( mirrorParentState: allTerminal ? won : false, ...(meta.release_context?.trigger_label !== undefined && { labelFilter: meta.release_context.trigger_label }), - }); + }), null); // Persist a freshly-created panel comment id so later edits reuse it. if (newId && !meta.status_comment_id) { try { @@ -1169,8 +1171,15 @@ async function replyToIterationComment( // Mature the settle reply (👀→✅/💬) with cost + running total, // editing the trigger-time reply when its id was captured. A failure keeps the // standard failure reply (which a human can reply to, to retry). - const prNumber = await resolvePrNumber(evt.taskId); - const prUrl = await resolvePrUrl(evt.taskId); + const prNumberResult = await readTaskPrNumber(ddb, TASK_TABLE, evt.taskId); + if (isLookupFailure(prNumberResult)) { + logger.warn('Settle reply: PR number read failed (non-fatal) — number omitted', { + task_id: evt.taskId, + error: prNumberResult.error instanceof Error ? prNumberResult.error.message : String(prNumberResult.error), + }); + } + const prNumber = lookupValueOr(prNumberResult, null); + const prUrl = lookupValueOr(await resolvePrUrl(evt.taskId), null); const { total: runningTotalUsd, partial: runningTotalPartial } = await sumIterationCostForIssue({ ddb, taskTableName: TASK_TABLE, @@ -1377,15 +1386,28 @@ async function spawnRestackTask( changedSubIssueId: string, ): Promise<'created' | 'exists' | 'failed'> { const child = step.child; - const prNumber = await resolvePrNumber(child.child_task_id); - if (prNumber === null) { - logger.warn('Restack cascade: dependent has no resolvable PR number — skipping', { - orchestration_id: child.orchestration_id, - sub_issue_id: child.sub_issue_id, - child_task_id: child.child_task_id, - }); + const prNumberResult = child.child_task_id + ? await readTaskPrNumber(ddb, TASK_TABLE, child.child_task_id) + : LOOKUP_ABSENT; + if (!prNumberResult.ok) { + // Both variants can't restack (no PR to re-stack onto), but log them + // distinctly: an outage ("read failed") is actionable, "no PR yet" is not. + logger.warn( + isLookupFailure(prNumberResult) + ? 'Restack cascade: dependent TaskRecord read failed — cannot restack' + : 'Restack cascade: dependent has no resolvable PR number — skipping', + { + orchestration_id: child.orchestration_id, + sub_issue_id: child.sub_issue_id, + child_task_id: child.child_task_id, + ...(isLookupFailure(prNumberResult) && { + error: prNumberResult.error instanceof Error ? prNumberResult.error.message : String(prNumberResult.error), + }), + }, + ); return 'failed'; } + const prNumber = prNumberResult.value; // Idempotency keyed on the SOURCE task id: this exact completion re-stacks // a given dependent at most once. Within [A-Za-z0-9_-], ≤128 chars. @@ -1438,42 +1460,22 @@ async function spawnRestackTask( } } -/** - * Read a dependent's PR number from its TaskRecord. Prefers numeric - * ``pr_number``; orchestration child tasks commonly persist only ``pr_url`` - * (``.../pull/N``) with ``pr_number`` null — fall back to parsing it. - */ -/** The dependent's PR URL (for a clickable reply link). Null when absent. */ -async function resolvePrUrl(taskId?: string): Promise { - if (!taskId) return null; +/** The dependent's PR URL (for a clickable reply link). */ +async function resolvePrUrl(taskId: string): Promise> { try { const res = await ddb.send(new GetCommand({ TableName: TASK_TABLE, Key: { task_id: taskId }, ProjectionExpression: 'pr_url', })); - return typeof res.Item?.pr_url === 'string' ? res.Item.pr_url : null; - } catch { - return null; - } -} - -async function resolvePrNumber(taskId?: string): Promise { - if (!taskId) return null; - try { - const res = await ddb.send(new GetCommand({ TableName: TASK_TABLE, Key: { task_id: taskId } })); - const pr = res.Item?.pr_number; - if (typeof pr === 'number') return pr; const url = res.Item?.pr_url; - if (typeof url === 'string') { - const m = url.match(/\/pull\/(\d+)\b/); - if (m) return Number(m[1]); - } - return null; + return typeof url === 'string' ? lookupFound(url) : LOOKUP_ABSENT; } catch (err) { - logger.warn('Restack cascade: failed to read dependent TaskRecord for PR number', { - task_id: taskId, - error: err instanceof Error ? err.message : String(err), + // Was a bare `catch { return null; }` with no logging (#756 Cat 2): a + // failed read was indistinguishable from "task has no PR". Surface both — + // the settle reply still degrades to omitting the link, but observably. + logger.warn('Settle reply: PR URL read failed (non-fatal) — link omitted', { + task_id: taskId, error: err instanceof Error ? err.message : String(err), }); - return null; + return lookupFailed(err); } } diff --git a/cdk/src/handlers/shared/jira-feedback.ts b/cdk/src/handlers/shared/jira-feedback.ts index 817584be..f7bbb70c 100644 --- a/cdk/src/handlers/shared/jira-feedback.ts +++ b/cdk/src/handlers/shared/jira-feedback.ts @@ -23,6 +23,7 @@ import { type ResolvedJiraOutboundAuth, } from './jira-oauth-resolver'; import { logger } from './logger'; +import { type LookupResult, isLookupFailure, lookupFailed, lookupFound } from './lookup-result'; import type { StateIntent, TransitionOptions } from './orchestration-channel'; /** @@ -331,11 +332,17 @@ interface JiraTransitionSnapshot { readonly transitions?: unknown; } +// The snapshot always exists for a real issue, so there is no genuine "absent" +// state — the read either loads the snapshot or it failed. Returning a +// {@link LookupResult} lets the caller log a Jira outage distinctly from a +// legitimate "no matching transition" no-op instead of collapsing both into a +// bare `null` (#756 Cat 2). ``:395`` in particular masked invalid JSON from +// Jira, a distinct class from a network timeout. async function readTransitionSnapshot( ctx: JiraFeedbackContext, issueIdOrKey: string, auth: ResolvedJiraOutboundAuth, -): Promise { +): Promise> { let status: number; let body: string; if (auth.kind === 'app') { @@ -345,7 +352,13 @@ async function readTransitionSnapshot( cloud_id: ctx.cloudId, issue_key: issueIdOrKey, }); - if (!result.ok) return null; + if (!result.ok) { + logger.warn('Jira transition lookup (app actor) failed', { + jira_cloud_id: ctx.cloudId, + issue_id_or_key: issueIdOrKey, + }); + return lookupFailed(new Error('Jira app-actor get_transitions returned not-ok')); + } status = result.status; body = result.body; } else { @@ -369,7 +382,7 @@ async function readTransitionSnapshot( issue_id_or_key: issueIdOrKey, error: err instanceof Error ? err.message : String(err), }); - return null; + return lookupFailed(err); } finally { clearTimeout(timer); } @@ -380,19 +393,19 @@ async function readTransitionSnapshot( issue_id_or_key: issueIdOrKey, status, }); - return null; + return lookupFailed(new Error(`Jira transition lookup returned HTTP ${status}`)); } try { const parsed = JSON.parse(body) as unknown; return parsed && typeof parsed === 'object' && !Array.isArray(parsed) - ? parsed as JiraTransitionSnapshot - : null; + ? lookupFound(parsed as JiraTransitionSnapshot) + : lookupFailed(new Error('Jira transition lookup returned a non-object body')); } catch (err) { logger.warn('Jira transition lookup returned invalid JSON', { issue_id_or_key: issueIdOrKey, error: err instanceof Error ? err.message : String(err), }); - return null; + return lookupFailed(err); } } @@ -486,8 +499,20 @@ export async function transitionIssueState( ): Promise { const auth = await resolveTenantAuth(ctx); if (!auth) return false; - const snapshot = await readTransitionSnapshot(ctx, issueIdOrKey, auth); - if (!snapshot) return false; + const snapshotResult = await readTransitionSnapshot(ctx, issueIdOrKey, auth); + if (!snapshotResult.ok) { + // A lookup failure is NOT the same as "transition not allowed" — log the + // outage distinctly, but stay best-effort (callers proceed regardless). + logger.warn('Jira transition: snapshot lookup failed — not transitioning', { + jira_cloud_id: ctx.cloudId, + issue_id_or_key: issueIdOrKey, + ...(isLookupFailure(snapshotResult) && { + error: snapshotResult.error instanceof Error ? snapshotResult.error.message : String(snapshotResult.error), + }), + }); + return false; + } + const snapshot = snapshotResult.value; const currentCategory = typeof snapshot.fields?.status?.statusCategory?.key === 'string' ? snapshot.fields.status.statusCategory.key diff --git a/cdk/src/handlers/shared/linear-feedback.ts b/cdk/src/handlers/shared/linear-feedback.ts index 022a496b..83de57b4 100644 --- a/cdk/src/handlers/shared/linear-feedback.ts +++ b/cdk/src/handlers/shared/linear-feedback.ts @@ -20,6 +20,7 @@ import { isTerminalMaturingReply, preservePreviewSuffix } from './iteration-reply'; import { resolveLinearOauthToken } from './linear-oauth-resolver'; import { logger } from './logger'; +import { type LookupResult, LOOKUP_ABSENT, lookupFailed, lookupFound, lookupValueOr } from './lookup-result'; import { isBotAuthoredComment } from './orchestration-comment-trigger'; /** @@ -249,11 +250,24 @@ interface TeamState { readonly position: number; } +/** + * Read side of the Linear GraphQL API (the write side is {@link graphqlRequest}). + * Returns a {@link LookupResult}: ``found`` with the ``data`` payload, ``absent`` + * when the call succeeded but carried no ``data`` (an anomalous but non-error + * shape), or ``failed`` on any error layer (non-2xx, GraphQL errors, a thrown + * fetch/timeout). The failure used to collapse into the same ``null`` as a + * genuinely-empty payload (#756 Cat 2); encoding it in the type lets a caller + * that cares distinguish the two. The many best-effort/fail-open callers here + * deliberately collapse it back with ``lookupValueOr(result, null)`` — an + * explicit choice at the call site, not a swallowed catch — because their + * documented contract is to proceed on any failure (advisory context, cosmetic + * cleanup, a skipped transition). Never throws. + */ async function graphqlData( accessToken: string, query: string, variables: Record, -): Promise | null> { +): Promise>> { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS); try { @@ -265,19 +279,19 @@ async function graphqlData( }); if (!resp.ok) { logger.warn('Linear feedback GraphQL non-2xx', { status: resp.status }); - return null; + return lookupFailed(new Error(`Linear feedback GraphQL returned HTTP ${resp.status}`)); } const body = (await resp.json()) as { data?: Record; errors?: unknown }; if (body.errors) { logger.warn('Linear feedback GraphQL errors', { errors: body.errors }); - return null; + return lookupFailed(body.errors); } - return body.data ?? null; + return body.data ? lookupFound(body.data) : LOOKUP_ABSENT; } catch (err) { logger.warn('Linear feedback request failed', { error: err instanceof Error ? err.message : String(err), }); - return null; + return lookupFailed(err); } finally { clearTimeout(timer); } @@ -419,7 +433,7 @@ export async function upsertStatusComment( return ok ? existingCommentId : null; } - const data = await graphqlData(token, COMMENT_CREATE_RETURNING_ID_MUTATION, { issueId, body }); + const data = lookupValueOr(await graphqlData(token, COMMENT_CREATE_RETURNING_ID_MUTATION, { issueId, body }), null); const created = data?.commentCreate as { success?: boolean; comment?: { id?: string } } | undefined; return created?.success && created.comment?.id ? created.comment.id : null; } @@ -473,7 +487,7 @@ export async function sweepTransientNotes( ): Promise { const token = await resolveToken(ctx); if (!token) return 0; - const data = await graphqlData(token, ISSUE_COMMENTS_QUERY, { issueId }); + const data = lookupValueOr(await graphqlData(token, ISSUE_COMMENTS_QUERY, { issueId }), null); const issue = data?.issue as { comments?: { nodes?: Array<{ id?: string; body?: string }> } } | undefined; const nodes = issue?.comments?.nodes ?? []; let deleted = 0; @@ -532,7 +546,7 @@ export async function fetchRecentComments( ): Promise { const token = await resolveToken(ctx); if (!token) return []; - const data = await graphqlData(token, RECENT_COMMENTS_QUERY, { issueId }); + const data = lookupValueOr(await graphqlData(token, RECENT_COMMENTS_QUERY, { issueId }), null); const issue = data?.issue as { comments?: { nodes?: RawLinearComment[] } } | undefined; const nodes = issue?.comments?.nodes ?? []; @@ -622,9 +636,9 @@ export async function replyToComment( ): Promise { const token = await resolveToken(ctx); if (!token) return null; - const data = await graphqlData(token, COMMENT_REPLY_RETURNING_ID_MUTATION, { + const data = lookupValueOr(await graphqlData(token, COMMENT_REPLY_RETURNING_ID_MUTATION, { issueId, parentId: parentCommentId, body, - }); + }), null); const created = data?.commentCreate as { success?: boolean; comment?: { id?: string } } | undefined; return created?.success && created.comment?.id ? created.comment.id : null; } @@ -658,7 +672,7 @@ export async function upsertThreadedReply( // Both options below need the CURRENT body, so read it once. let current: string | undefined; if (options?.preservePreview || options?.skipIfSettled) { - const data = await graphqlData(token, COMMENT_BODY_QUERY, { commentId: existingReplyId }); + const data = lookupValueOr(await graphqlData(token, COMMENT_BODY_QUERY, { commentId: existingReplyId }), null); current = (data?.comment as { body?: string } | undefined)?.body; } // A PROGRESS edit must never overwrite an outcome. The terminal settle and the @@ -698,9 +712,9 @@ export async function upsertThreadedReply( return existingReplyId; } - const data = await graphqlData(token, COMMENT_REPLY_RETURNING_ID_MUTATION, { + const data = lookupValueOr(await graphqlData(token, COMMENT_REPLY_RETURNING_ID_MUTATION, { issueId, parentId: parentCommentId, body, - }); + }), null); const created = data?.commentCreate as { success?: boolean; comment?: { id?: string } } | undefined; return created?.success && created.comment?.id ? created.comment.id : null; } @@ -730,7 +744,7 @@ async function repairOverwrittenOutcome( outcomeBody: string, ): Promise { if (!isTerminalMaturingReply(outcomeBody)) return; // only outcomes are worth defending - const data = await graphqlData(accessToken, COMMENT_BODY_QUERY, { commentId }); + const data = lookupValueOr(await graphqlData(accessToken, COMMENT_BODY_QUERY, { commentId }), null); const current = (data?.comment as { body?: string } | undefined)?.body; // Unreadable → leave it alone: acting on an unknown body could overwrite // something newer, and the reply most likely still holds the outcome. @@ -771,7 +785,7 @@ export async function appendOnceToComment( ): Promise { const token = await resolveToken(ctx); if (!token) return false; - const data = await graphqlData(token, COMMENT_BODY_QUERY, { commentId }); + const data = lookupValueOr(await graphqlData(token, COMMENT_BODY_QUERY, { commentId }), null); const current = (data?.comment as { body?: string } | undefined)?.body; if (typeof current !== 'string') return false; if (current.includes(marker)) return false; // already appended (idempotent) @@ -821,7 +835,7 @@ export async function swapIssueReaction( const token = await resolveToken(ctx); if (!token) return false; - const data = await graphqlData(token, ISSUE_REACTIONS_QUERY, { issueId }); + const data = lookupValueOr(await graphqlData(token, ISSUE_REACTIONS_QUERY, { issueId }), null); const reactions = ((data?.issue as { reactions?: Array<{ id: string; emoji: string }> } | undefined)?.reactions) ?? []; // Delete our stale markers (any bgagent emoji that isn't the target). @@ -870,7 +884,7 @@ export async function swapCommentReaction( const token = await resolveToken(ctx); if (!token) return false; - const data = await graphqlData(token, COMMENT_REACTIONS_QUERY, { commentId }); + const data = lookupValueOr(await graphqlData(token, COMMENT_REACTIONS_QUERY, { commentId }), null); const reactions = ((data?.comment as { reactions?: Array<{ id: string; emoji: string }> } | undefined)?.reactions) ?? []; let targetPresent = false; @@ -966,7 +980,7 @@ export async function transitionIssueState( const token = await resolveToken(ctx); if (!token) return false; - const data = await graphqlData(token, ISSUE_TEAM_STATES_QUERY, { issueId }); + const data = lookupValueOr(await graphqlData(token, ISSUE_TEAM_STATES_QUERY, { issueId }), null); const issue = data?.issue as | { state?: TeamState; team?: { states?: { nodes?: TeamState[] } } } | undefined; @@ -1052,7 +1066,7 @@ export async function revertIssueToNotStarted( const token = await resolveToken(ctx); if (!token) return false; - const data = await graphqlData(token, ISSUE_TEAM_STATES_QUERY, { issueId }); + const data = lookupValueOr(await graphqlData(token, ISSUE_TEAM_STATES_QUERY, { issueId }), null); const issue = data?.issue as | { state?: TeamState; team?: { states?: { nodes?: TeamState[] } } } | undefined; diff --git a/cdk/src/handlers/shared/linear-subissue-fetch.ts b/cdk/src/handlers/shared/linear-subissue-fetch.ts index f6745e9c..ab843743 100644 --- a/cdk/src/handlers/shared/linear-subissue-fetch.ts +++ b/cdk/src/handlers/shared/linear-subissue-fetch.ts @@ -36,6 +36,7 @@ */ import { logger } from './logger'; +import { type LookupResult, LOOKUP_ABSENT, lookupFailed, lookupFound } from './lookup-result'; import type { DagNode } from './orchestration-dag'; const LINEAR_GRAPHQL_URL = 'https://api.linear.app/graphql'; @@ -296,15 +297,20 @@ query IssueParent($issueId: String!) { /** * Fetch a sub-issue's parent issue id, for the comment trigger. A Linear * comment names the issue it is on (the sub-issue); to find its orchestration - * we need the PARENT (orchestration_id is derived from the parent). Returns the - * parent id, or null when the issue has no parent (a top-level issue — not part - * of any orchestration) or on any fetch/auth/GraphQL failure. Never throws. + * we need the PARENT (orchestration_id is derived from the parent). + * + * Returns a {@link LookupResult}: ``found`` with the parent id, ``absent`` when + * the issue genuinely has no parent (a top-level issue — not part of any + * orchestration), or a failure when the fetch/auth/GraphQL call breaks. The + * distinction matters: a failure previously collapsed to the same ``null`` as + * "no parent", silently downgrading an orchestration child to the standalone + * path (#756 Cat 2). Never throws. */ export async function fetchIssueParentId( accessToken: string, issueId: string, options: FetchSubIssueGraphOptions = {}, -): Promise { +): Promise> { const fetchImpl = options.fetchImpl ?? fetch; const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS); @@ -317,20 +323,21 @@ export async function fetchIssueParentId( }); if (!resp.ok) { logger.warn('Linear issue-parent fetch non-2xx', { status: resp.status, issue_id: issueId }); - return null; + return lookupFailed(new Error(`Linear issue-parent fetch returned HTTP ${resp.status}`)); } const raw = (await resp.json()) as { data?: { issue?: { parent?: { id?: string } } }; errors?: unknown }; if (raw.errors) { logger.warn('Linear issue-parent fetch GraphQL errors', { issue_id: issueId, errors: raw.errors }); - return null; + return lookupFailed(raw.errors); } - return raw.data?.issue?.parent?.id ?? null; + const parentId = raw.data?.issue?.parent?.id; + return typeof parentId === 'string' ? lookupFound(parentId) : LOOKUP_ABSENT; } catch (err) { logger.warn('Linear issue-parent fetch failed', { issue_id: issueId, error: err instanceof Error ? err.message : String(err), }); - return null; + return lookupFailed(err); } finally { clearTimeout(timer); } diff --git a/cdk/src/handlers/shared/lookup-result.ts b/cdk/src/handlers/shared/lookup-result.ts new file mode 100644 index 00000000..a5bdaa48 --- /dev/null +++ b/cdk/src/handlers/shared/lookup-result.ts @@ -0,0 +1,63 @@ +/** + * 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. + */ + +/** + * Outcome of a best-effort lookup (a network/DDB read that may legitimately + * find nothing). It exists to stop the silent-success-masking class (#756 Cat 2, + * AI004): returning a bare `null`/`[]` from a `catch` collapses "genuinely + * absent" into "the lookup broke", so the caller — and any orchestration + * control flow keyed off it — cannot tell a real empty from an outage. + * + * Three states, deliberately distinct: + * - `{ ok: true, value }` — the lookup ran and found a value. + * - `{ ok: false, absent: true }` — the lookup ran and there is genuinely nothing. + * - `{ ok: false, error }` — the lookup itself failed (throw / non-2xx / bad body). + * + * Callers that must route differently on failure (e.g. avoid posting a + * duplicate comment, or escalate a retry) branch on {@link isLookupFailure}. + * Purely best-effort callers that legitimately treat absent === failed can + * collapse with {@link lookupValueOr} — but the failure is now still + * *observable* at the source, which is the point. + */ +export type LookupResult = + | { readonly ok: true; readonly value: T } + | { readonly ok: false; readonly absent: true } + | { readonly ok: false; readonly error: unknown }; + +/** The lookup ran and found `value`. */ +export const lookupFound = (value: T): LookupResult => ({ ok: true, value }); + +/** The lookup ran and there is genuinely nothing to find (not an error). */ +export const LOOKUP_ABSENT = { ok: false, absent: true } as const; + +/** The lookup itself failed — carries the cause for logging/escalation. */ +export const lookupFailed = (error: unknown): LookupResult => ({ ok: false, error }); + +/** True only for the genuine-failure variant (not the absent variant). */ +export const isLookupFailure = ( + r: LookupResult, +): r is { readonly ok: false; readonly error: unknown } => !r.ok && 'error' in r; + +/** + * Collapse to the found value, or `fallback` when absent OR failed. For purely + * best-effort callers that legitimately treat both the same; the failure was + * already surfaced (logged) at the lookup site, so nothing is masked here. + */ +export const lookupValueOr = (r: LookupResult, fallback: F): T | F => + r.ok ? r.value : fallback; diff --git a/cdk/src/handlers/shared/orchestration-channel-slack.ts b/cdk/src/handlers/shared/orchestration-channel-slack.ts index a0e90236..1d90e2d9 100644 --- a/cdk/src/handlers/shared/orchestration-channel-slack.ts +++ b/cdk/src/handlers/shared/orchestration-channel-slack.ts @@ -47,6 +47,7 @@ */ import { logger } from './logger'; +import { lookupValueOr } from './lookup-result'; import { type Channel, type IssueRef, @@ -126,11 +127,11 @@ export function makeSlackChannel(secretPrefix: string = SLACK_SECRET_PREFIX): Ch async postComment(issue, body) { const ctx = await contextFor(issue); if (!ctx) return null; - const ts = await slackFetchTs(ctx.token, 'chat.postMessage', { + const ts = lookupValueOr(await slackFetchTs(ctx.token, 'chat.postMessage', { channel: ctx.channel, thread_ts: ctx.threadTs, text: body, - }); + }), null); return ts ? { commentId: ts } : null; }, @@ -140,18 +141,18 @@ export function makeSlackChannel(secretPrefix: string = SLACK_SECRET_PREFIX): Ch if (existing?.commentId) { // Edit in place — this is what makes the maturing panel one message // rather than a stream. chat.update echoes the ts it edited. - const ts = await slackFetchTs(ctx.token, 'chat.update', { + const ts = lookupValueOr(await slackFetchTs(ctx.token, 'chat.update', { channel: ctx.channel, ts: existing.commentId, text: body, - }); + }), null); return ts ? { commentId: ts } : null; } - const ts = await slackFetchTs(ctx.token, 'chat.postMessage', { + const ts = lookupValueOr(await slackFetchTs(ctx.token, 'chat.postMessage', { channel: ctx.channel, thread_ts: ctx.threadTs, text: body, - }); + }), null); return ts ? { commentId: ts } : null; }, @@ -233,11 +234,11 @@ export function makeSlackChannel(secretPrefix: string = SLACK_SECRET_PREFIX): Ch // Slack threads are one level deep: a reply goes to the thread the parent // belongs to. Using the parent's own ts as thread_ts starts a thread on it // when the parent is a root, and stays in-thread otherwise. - const ts = await slackFetchTs(ctx.token, 'chat.postMessage', { + const ts = lookupValueOr(await slackFetchTs(ctx.token, 'chat.postMessage', { channel: ctx.channel, thread_ts: parent.commentId, text: body, - }); + }), null); return ts ? { commentId: ts } : null; }, @@ -253,11 +254,11 @@ export function makeSlackChannel(secretPrefix: string = SLACK_SECRET_PREFIX): Ch const ctx = await contextFor(issue); if (!ctx) return null; if (existing?.commentId) { - const ts = await slackFetchTs(ctx.token, 'chat.update', { + const ts = lookupValueOr(await slackFetchTs(ctx.token, 'chat.update', { channel: ctx.channel, ts: existing.commentId, text: body, - }); + }), null); return ts ? { commentId: ts } : null; } return this.postThreadedReply!(issue, parent, body); diff --git a/cdk/src/handlers/shared/orchestration-rollup.ts b/cdk/src/handlers/shared/orchestration-rollup.ts index 261bc556..e789b93d 100644 --- a/cdk/src/handlers/shared/orchestration-rollup.ts +++ b/cdk/src/handlers/shared/orchestration-rollup.ts @@ -29,6 +29,7 @@ */ import { logger } from './logger'; +import { type LookupResult, LOOKUP_ABSENT, lookupFailed, lookupFound } from './lookup-result'; import type { Channel, IssueRef } from './orchestration-channel'; import { isIntegrationNode } from './orchestration-integration-node'; import { ORCH_LOG } from './orchestration-log-events'; @@ -444,7 +445,15 @@ export interface UpsertEpicPanelParams { /** * Render + upsert the single maturing epic panel, and (optionally) mirror the * outcome on the parent issue's state + reaction. The ONE place the parent panel - * is written. Returns the panel comment id (new or existing), or null on failure. + * is written. + * + * Returns a {@link LookupResult}: ``found`` with the panel comment id (new or + * existing), ``absent`` when the surface accepted the upsert but handed back no + * usable id (nothing to persist — the next edit would address a comment that + * doesn't exist), or ``failed`` when the upsert threw. The failure used to + * collapse into the same ``null`` as "no id" (#756 Cat 2); callers persist only a + * ``found`` id, so they collapse this with ``lookupValueOr(result, null)`` — the + * failure is already logged at source and never overwrites a stored id. * * - Edits ``statusCommentId`` in place when given; else posts a fresh comment. * - Header/rows via {@link renderEpicPanel}; ``inProgress`` derived if omitted. @@ -456,7 +465,7 @@ export interface UpsertEpicPanelParams { * transition silently no-op'd and left the epic stuck. * Best-effort: a surface hiccup never throws out of the reconcile. */ -export async function upsertEpicPanel(params: UpsertEpicPanelParams): Promise { +export async function upsertEpicPanel(params: UpsertEpicPanelParams): Promise> { const { channel, parent } = params; const rows = buildPanelRows(params.children, params.prUrls ?? {}, params.updating ?? {}, params.failureReasons ?? {}); const terminal = (s: string) => s === 'succeeded' || s === 'failed' || s === 'skipped'; @@ -487,7 +496,7 @@ export async function upsertEpicPanel(params: UpsertEpicPanelParams): Promise, -): Promise { +): Promise> { try { const response = await fetch(`https://slack.com/api/${method}`, { method: 'POST', @@ -95,20 +104,22 @@ export async function slackFetchTs( }); if (!response.ok) { logger.warn('Slack API returned non-2xx', { method, status: response.status }); - return null; + return lookupFailed(new Error(`Slack API ${method} returned HTTP ${response.status}`)); } const result = await response.json() as { ok: boolean; error?: string; ts?: string }; if (!result.ok) { logger.warn('Slack API returned error', { method, error: result.error }); - return null; + return lookupFailed(new Error(`Slack API ${method} error: ${result.error ?? 'unknown'}`)); } // chat.update echoes the ts it edited; chat.postMessage returns the new one. - return result.ts ?? null; + if (typeof result.ts === 'string') return lookupFound(result.ts); + logger.warn('Slack API returned ok without a ts', { method }); + return lookupFailed(new Error(`Slack API ${method} succeeded but returned no ts`)); } catch (err) { logger.warn('Slack API fetch threw', { method, error: err instanceof Error ? err.message : String(err), }); - return null; + return lookupFailed(err); } } diff --git a/cdk/src/handlers/shared/task-pr-number.ts b/cdk/src/handlers/shared/task-pr-number.ts new file mode 100644 index 00000000..99cb196c --- /dev/null +++ b/cdk/src/handlers/shared/task-pr-number.ts @@ -0,0 +1,53 @@ +/** + * 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. + */ + +import { GetCommand, type DynamoDBDocumentClient } from '@aws-sdk/lib-dynamodb'; +import { type LookupResult, LOOKUP_ABSENT, lookupFailed, lookupFound } from './lookup-result'; + +/** + * Read a task's PR number from its TaskRecord. Prefers the numeric + * ``pr_number``; orchestration child tasks commonly persist only ``pr_url`` + * (``.../pull/N``) with ``pr_number`` null — fall back to parsing it. + * + * Deduplicated from the byte-for-byte-equivalent copies that previously lived + * in orchestration-reconciler (``resolvePrNumber``) and linear-webhook-processor + * (``resolveChildPrNumber``) — both swallowed the read failure into a bare + * ``null`` (#756 Cat 2). Returns a {@link LookupResult} so callers can tell + * "dependent has no PR yet" (absent) from "the TaskRecord read broke" (error): + * the restack cascade must not misreport an outage as "no PR — skipping". + */ +export async function readTaskPrNumber( + ddb: DynamoDBDocumentClient, + tableName: string, + taskId: string, +): Promise> { + try { + const res = await ddb.send(new GetCommand({ TableName: tableName, Key: { task_id: taskId } })); + const pr = res.Item?.pr_number; + if (typeof pr === 'number') return lookupFound(pr); + const url = res.Item?.pr_url; + if (typeof url === 'string') { + const m = url.match(/\/pull\/(\d+)\b/); + if (m) return lookupFound(Number(m[1])); + } + return LOOKUP_ABSENT; + } catch (err) { + return lookupFailed(err); + } +} diff --git a/cdk/test/handlers/jira-webhook-processor-orchestration.test.ts b/cdk/test/handlers/jira-webhook-processor-orchestration.test.ts index 7d821263..221b4045 100644 --- a/cdk/test/handlers/jira-webhook-processor-orchestration.test.ts +++ b/cdk/test/handlers/jira-webhook-processor-orchestration.test.ts @@ -102,6 +102,7 @@ process.env.USER_CONCURRENCY_TABLE_NAME = 'Concurrency'; process.env.MAX_CONCURRENT_TASKS_PER_USER = '10'; import { handler } from '../../src/handlers/jira-webhook-processor'; +import { LOOKUP_ABSENT, lookupFound } from '../../src/handlers/shared/lookup-result'; const oauth = { accessToken: 'jira-token', @@ -246,7 +247,7 @@ describe('jira-webhook-processor orchestration adapter', () => { applyTerminalCreateFailuresMock.mockResolvedValue(snapshot.children); readConcurrencyBudgetMock.mockReset().mockResolvedValue(7); upsertEpicPanelMock.mockReset(); - upsertEpicPanelMock.mockResolvedValue(null); + upsertEpicPanelMock.mockResolvedValue(LOOKUP_ABSENT); setStatusCommentIdMock.mockReset(); claimCommentAckMock.mockReset().mockResolvedValue(true); clearRollupClaimMock.mockReset().mockResolvedValue(undefined); @@ -634,7 +635,7 @@ describe('jira-webhook-processor orchestration adapter', () => { loadOrchestrationMock .mockResolvedValueOnce(snapshot) .mockResolvedValueOnce(extendedSnapshot); - upsertEpicPanelMock.mockResolvedValueOnce('new-panel'); + upsertEpicPanelMock.mockResolvedValueOnce(lookupFound('new-panel')); const extensionEvent = event(); const payload = JSON.parse(extensionEvent.raw_body); diff --git a/cdk/test/handlers/linear-webhook-processor-orchestration.test.ts b/cdk/test/handlers/linear-webhook-processor-orchestration.test.ts index 2b14f090..53c8fbf8 100644 --- a/cdk/test/handlers/linear-webhook-processor-orchestration.test.ts +++ b/cdk/test/handlers/linear-webhook-processor-orchestration.test.ts @@ -124,6 +124,7 @@ process.env.TASK_TABLE_NAME = 'TaskTable'; process.env.ORCHESTRATION_TABLE_NAME = 'OrchestrationTable'; import { handler } from '../../src/handlers/linear-webhook-processor'; +import { LOOKUP_ABSENT, lookupFound } from '../../src/handlers/shared/lookup-result'; function eventWith(payload: Record): { raw_body: string } { return { raw_body: JSON.stringify(payload) }; @@ -720,7 +721,7 @@ describe('linear-webhook-processor — @bgagent comment trigger', () => { /** Mock for a PLAIN (non-orchestration) issue: no parent, no orchestration snapshot, only the GSI hit. */ function mockStandaloneOnly(standalone: { task_id: string; user_id?: string; repo?: string; pr_url?: string; pr_number?: number; status?: string } | null): void { - fetchIssueParentIdMock.mockResolvedValue(null); // no parent ⇒ not a sub-issue + fetchIssueParentIdMock.mockResolvedValue(LOOKUP_ABSENT); // no parent ⇒ not a sub-issue ddbSend.mockImplementation(async (cmd: { _type: string; input: Record }) => { if (cmd._type === 'Query' && cmd.input.IndexName === 'LinearIssueIndex') { return { Items: standalone ? [standalone] : [] }; @@ -738,7 +739,7 @@ describe('linear-webhook-processor — @bgagent comment trigger', () => { createTaskCoreMock.mockReset().mockResolvedValue({ statusCode: 201, body: '{}' }); resolveLinearOauthTokenMock.mockReset() .mockResolvedValue({ accessToken: 'tok', oauthSecretArn: 'arn:secret', workspaceSlug: 'acme' }); - fetchIssueParentIdMock.mockReset().mockResolvedValue('PARENT'); + fetchIssueParentIdMock.mockReset().mockResolvedValue(lookupFound('PARENT')); discoverOrchestrationMock.mockReset(); reactToCommentMock.mockReset().mockResolvedValue(true); replyToCommentMock.mockReset().mockResolvedValue(true); @@ -925,7 +926,7 @@ describe('linear-webhook-processor — @bgagent comment trigger', () => { // A Query failure and a genuine miss are different facts. Collapsing them told // the user their issue is not ours, which is a guess dressed as a conclusion — // and it hides a real fault (throttling, a missing GSI) behind a silent no-op. - fetchIssueParentIdMock.mockResolvedValue(null); + fetchIssueParentIdMock.mockResolvedValue(LOOKUP_ABSENT); ddbSend.mockImplementation(async (cmd: { _type: string; input: Record }) => { // ONLY the GSI query fails — everything else (the redelivery claim, the // commenter authorization) must still work, or the nudge would be skipped @@ -948,7 +949,7 @@ describe('linear-webhook-processor — @bgagent comment trigger', () => { }); test('@bgagent on a sub-issue whose parent is not an orchestration AND no ABCA task → no task', async () => { - fetchIssueParentIdMock.mockResolvedValue('PARENT'); + fetchIssueParentIdMock.mockResolvedValue(lookupFound('PARENT')); ddbSend.mockImplementation(async (cmd: { _type: string; input: Record }) => { if (cmd._type === 'Query' && cmd.input.IndexName === 'LinearIssueIndex') return { Items: [] }; return { Items: [] }; // loadOrchestration → no snapshot @@ -967,7 +968,7 @@ describe('linear-webhook-processor — @bgagent comment trigger', () => { // Even with a fully actionable iteration target, a commenter with NO linked // platform user must not be able to start a code-pushing run billed to the // requester. The mapping Get returns nothing → the gate blocks before dispatch. - fetchIssueParentIdMock.mockResolvedValue(null); + fetchIssueParentIdMock.mockResolvedValue(LOOKUP_ABSENT); ddbSend.mockImplementation(async (cmd: { _type: string; input: Record }) => { if (cmd._type === 'Query' && cmd.input.IndexName === 'LinearIssueIndex') { return { Items: [{ task_id: 'task-solo', user_id: 'u-solo', repo: 'o/r', pr_number: 99 }] }; diff --git a/cdk/test/handlers/shared/orchestration-channel-slack.test.ts b/cdk/test/handlers/shared/orchestration-channel-slack.test.ts index edbd496a..ec54c049 100644 --- a/cdk/test/handlers/shared/orchestration-channel-slack.test.ts +++ b/cdk/test/handlers/shared/orchestration-channel-slack.test.ts @@ -36,8 +36,11 @@ jest.mock('../../../src/handlers/shared/logger', () => ({ logger: { info: jest.fn(), warn: jest.fn(), error: jest.fn() }, })); +import { lookupFailed, lookupFound } from '../../../src/handlers/shared/lookup-result'; import { type IssueRef } from '../../../src/handlers/shared/orchestration-channel'; import { makeSlackChannel, slackThreadRef } from '../../../src/handlers/shared/orchestration-channel-slack'; +// slackFetchTs now returns a LookupResult (not string|null); the transport mock +// resolves real results so the adapter's lookupValueOr collapse is exercised. /** A Slack "issue" is a thread: channel + thread_ts, keyed by team_id. */ const thread: IssueRef = { issueId: slackThreadRef('C123', '1700000000.001'), credentialsRef: 'T99' }; @@ -46,7 +49,7 @@ beforeEach(() => { jest.clearAllMocks(); getSlackSecretMock.mockResolvedValue('xoxb-token'); slackFetchMock.mockResolvedValue(true); - slackFetchTsMock.mockResolvedValue('1700000000.002'); + slackFetchTsMock.mockResolvedValue(lookupFound('1700000000.002')); }); describe('Slack channel adapter — the capability-gated surface', () => { @@ -91,7 +94,7 @@ describe('Slack channel adapter — the capability-gated surface', () => { test('upsertComment EDITS the given message in place — the maturing panel', async () => { // Without edit-in-place a Slack epic would stream a new message per // transition, which is the surface this design exists to avoid. - slackFetchTsMock.mockResolvedValue('1700000000.005'); + slackFetchTsMock.mockResolvedValue(lookupFound('1700000000.005')); const res = await ch.upsertComment(thread, 'panel v2', { commentId: '1700000000.005' }); expect(res).toEqual({ commentId: '1700000000.005' }); const [, method, body] = slackFetchTsMock.mock.calls[0]; @@ -197,7 +200,7 @@ describe('Slack channel adapter — the capability-gated surface', () => { }); test('a failed Slack call reports null rather than a bogus ref', async () => { - slackFetchTsMock.mockResolvedValue(null); + slackFetchTsMock.mockResolvedValue(lookupFailed(new Error('slack down'))); await expect(ch.postComment(thread, 'x')).resolves.toBeNull(); }); }); diff --git a/cdk/test/handlers/shared/orchestration-rollup.test.ts b/cdk/test/handlers/shared/orchestration-rollup.test.ts index fea2e389..8dd8d39b 100644 --- a/cdk/test/handlers/shared/orchestration-rollup.test.ts +++ b/cdk/test/handlers/shared/orchestration-rollup.test.ts @@ -31,6 +31,7 @@ jest.mock('../../../src/handlers/shared/slack-verify', () => ({ const loggerMock = { info: jest.fn(), warn: jest.fn(), error: jest.fn() }; jest.mock('../../../src/handlers/shared/logger', () => ({ logger: loggerMock })); +import { LOOKUP_ABSENT, isLookupFailure, lookupFound } from '../../../src/handlers/shared/lookup-result'; import type { IssueRef } from '../../../src/handlers/shared/orchestration-channel'; import { channelForSource, registerChannelFactory } from '../../../src/handlers/shared/orchestration-channel-factory'; import { makeSlackChannel, slackThreadRef } from '../../../src/handlers/shared/orchestration-channel-slack'; @@ -233,7 +234,7 @@ const row = (sub: string, status: string): OrchestrationChildRow => ({ describe('upsertEpicPanel — the maturing panel + parent-state mirror', () => { beforeEach(() => { slackFetchMock.mockReset().mockResolvedValue(true); - slackFetchTsMock.mockReset().mockResolvedValue('1700000000.002'); + slackFetchTsMock.mockReset().mockResolvedValue(lookupFound('1700000000.002')); channel = makeFakeChannel(); loggerMock.info.mockReset(); loggerMock.warn.mockReset(); @@ -243,7 +244,7 @@ describe('upsertEpicPanel — the maturing panel + parent-state mirror', () => { const id = await upsertEpicPanel({ channel, parent, statusCommentId: 'panel-1', children: [row('a', 'running')], }); - expect(id).toBe('cmt-1'); + expect(id).toEqual(lookupFound('cmt-1')); const [, , existing] = channel.upsertComment.mock.calls[0]; expect(existing).toEqual({ commentId: 'panel-1' }); }); @@ -252,7 +253,7 @@ describe('upsertEpicPanel — the maturing panel + parent-state mirror', () => { // A blank id must not be persisted — the next edit would address a comment // that doesn't exist. "No id" is the honest answer. channel.upsertComment.mockResolvedValue({ commentId: '' }); - expect(await upsertEpicPanel({ channel, parent, children: [row('a', 'running')] })).toBeNull(); + expect(await upsertEpicPanel({ channel, parent, children: [row('a', 'running')] })).toEqual(LOOKUP_ABSENT); }); test('in progress → re-opens the parent to running (regression allowed) + 👀', async () => { @@ -310,7 +311,7 @@ describe('upsertEpicPanel — the maturing panel + parent-state mirror', () => { mirrorParentState: true, // asks for a state mirror Slack cannot do }); // The panel landed... - expect(id).toBe('1700000000.002'); + expect(id).toEqual(lookupFound('1700000000.002')); expect(slackFetchTsMock).toHaveBeenCalled(); // ...the ✅ marker went on the thread root... const adds = slackFetchMock.mock.calls.filter((c) => c[1] === 'reactions.add'); @@ -357,7 +358,7 @@ describe('upsertEpicPanel — the maturing panel + parent-state mirror', () => { mirrorParentState: true, // asks for a transition this surface cannot do }); - expect(id).toBe('acme-panel-1'); + expect(id).toEqual(lookupFound('acme-panel-1')); expect(posted).toHaveLength(1); expect(posted[0].issue).toBe('acme-epic-1'); // The panel body is the engine's own rendering — the surface supplied none of it. @@ -374,12 +375,12 @@ describe('upsertEpicPanel — the maturing panel + parent-state mirror', () => { const id = await upsertEpicPanel({ channel: commentOnly, parent, children: [row('a', 'succeeded')], mirrorParentState: true, }); - expect(id).toBe('cmt-1'); + expect(id).toEqual(lookupFound('cmt-1')); }); - test('a panel-comment failure is swallowed and reported as no id', async () => { + test('a panel-comment failure is reported as a lookup failure', async () => { channel.upsertComment.mockRejectedValue(new Error('surface hiccup')); - expect(await upsertEpicPanel({ channel, parent, children: [row('a', 'running')] })).toBeNull(); + expect(isLookupFailure(await upsertEpicPanel({ channel, parent, children: [row('a', 'running')] }))).toBe(true); expect(loggerMock.warn).toHaveBeenCalled(); }); @@ -388,7 +389,7 @@ describe('upsertEpicPanel — the maturing panel + parent-state mirror', () => { const id = await upsertEpicPanel({ channel, parent, children: [row('a', 'succeeded')], inProgress: false, mirrorParentState: true, }); - expect(id).toBe('cmt-1'); + expect(id).toEqual(lookupFound('cmt-1')); }); }); diff --git a/cli/src/commands/linear.ts b/cli/src/commands/linear.ts index 4982d3f8..7f817d5e 100644 --- a/cli/src/commands/linear.ts +++ b/cli/src/commands/linear.ts @@ -1331,7 +1331,15 @@ export function makeLinearCommand(): Command { // Best-effort: fetch team keys so the screenshot processor can // prefix-route Linear issue lookups (e.g. ENG-42 → the workspace // owning the ENG team) instead of scanning every active workspace. - const teamKeys = await queryLinearTeamKeys(`Bearer ${linearAccessToken}`); + const teamKeysResult = await queryLinearTeamKeys(`Bearer ${linearAccessToken}`); + if (!teamKeysResult.ok) { + console.log( + ' ⚠ Could not read this workspace\'s Linear team keys — recording the workspace ' + + 'without them. Issue lookups will fall back to scanning every workspace; re-run ' + + 'setup later to record team keys for faster prefix-routing.', + ); + } + const teamKeys = teamKeysResult.ok ? teamKeysResult.keys : []; await ddb.send(new PutCommand({ TableName: workspaceRegistryTable!, Item: { @@ -1755,7 +1763,15 @@ export function makeLinearCommand(): Command { // ─── Persist registry + user-mapping rows ────────────────────── // Fetch team keys for prefix-routing (see same call in `setup`). - const teamKeys = await queryLinearTeamKeys(`Bearer ${tokenResponse.access_token}`); + const teamKeysResult = await queryLinearTeamKeys(`Bearer ${tokenResponse.access_token}`); + if (!teamKeysResult.ok) { + console.log( + ' ⚠ Could not read this workspace\'s Linear team keys — recording the workspace ' + + 'without them. Issue lookups will fall back to scanning every workspace; re-run ' + + 'add-workspace later to record team keys for faster prefix-routing.', + ); + } + const teamKeys = teamKeysResult.ok ? teamKeysResult.keys : []; await ddb.send(new PutCommand({ TableName: workspaceRegistryTable!, Item: { @@ -2422,15 +2438,27 @@ interface LinearWorkspaceMember { readonly email?: string; } +/** + * Outcome of a {@link queryLinearTeamKeys} call. An empty `keys` array is a + * legitimate success (a workspace with no teams), so the only distinction the + * two states carry is success-vs-failure — a network/auth/GraphQL break that + * used to collapse into the same empty array as "no teams" (#756 Cat 2). The + * caller warns on `ok: false` so prefix-routing degrading to a full-workspace + * scan is visible instead of silent. + */ +export type TeamKeysResult = + | { readonly ok: true; readonly keys: string[] } + | { readonly ok: false; readonly error: unknown }; + /** * Query the workspace's team keys (e.g. `["ABCA", "PLAT"]`). Persisted on * the registry row so the screenshot processor can prefix-route Linear * issue lookups to the owning workspace instead of scanning every - * workspace's tokens. Returns an empty array on failure — callers persist - * what they got and the lookup falls back to scanning if `team_keys` is - * absent or stale. + * workspace's tokens. On failure the caller persists the row without + * `team_keys` and the lookup falls back to scanning — but the failure is + * surfaced (see {@link TeamKeysResult}) rather than swallowed as "no teams". */ -export async function queryLinearTeamKeys(authorizationHeader: string): Promise { +export async function queryLinearTeamKeys(authorizationHeader: string): Promise { try { const res = await fetch('https://api.linear.app/graphql', { method: 'POST', @@ -2444,15 +2472,17 @@ export async function queryLinearTeamKeys(authorizationHeader: string): Promise< query: '{ teams(first: 100) { nodes { key } } }', }), }); - if (!res.ok) return []; + if (!res.ok) { + return { ok: false, error: new Error(`Linear teams query returned HTTP ${res.status}`) }; + } const body = await res.json() as { data?: { teams?: { nodes?: Array<{ key?: string }> } } }; const keys = (body.data?.teams?.nodes ?? []) .map((t) => t.key) .filter((k): k is string => typeof k === 'string' && k.length > 0) .map((k) => k.toUpperCase()); - return Array.from(new Set(keys)).sort(); - } catch { - return []; + return { ok: true, keys: Array.from(new Set(keys)).sort() }; + } catch (err) { + return { ok: false, error: err }; } } diff --git a/cli/test/commands/linear.test.ts b/cli/test/commands/linear.test.ts index d60faf89..1119907b 100644 --- a/cli/test/commands/linear.test.ts +++ b/cli/test/commands/linear.test.ts @@ -585,10 +585,12 @@ describe('generateInviteCode', () => { describe('queryLinearTeamKeys', () => { // Returned keys are persisted on the registry row at install time and // drive prefix-routing inside the screenshot processor — see #96. The - // helper intentionally swallows every failure path (returns []) so a - // transient Linear outage during `setup` doesn't abort the OAuth - // dance. Coverage verifies (a) the happy-path normalization and (b) - // every failure mode collapses to []. + // helper never throws (a transient Linear outage during `setup` must not + // abort the OAuth dance), but it distinguishes success-with-keys from a + // network/auth/GraphQL failure via a discriminated result (#756 Cat 2) so + // the caller can warn instead of silently persisting "no teams". Coverage + // verifies (a) happy-path normalization, (b) an empty-but-valid workspace + // is `ok`, and (c) every failure mode surfaces `ok: false`. const originalFetch = global.fetch; afterEach(() => { global.fetch = originalFetch; @@ -611,9 +613,9 @@ describe('queryLinearTeamKeys', () => { }), }) as unknown as typeof fetch; - const keys = await queryLinearTeamKeys('Bearer tok'); + const result = await queryLinearTeamKeys('Bearer tok'); - expect(keys).toEqual(['ABCA', 'PLAT', 'WEB']); + expect(result).toEqual({ ok: true, keys: ['ABCA', 'PLAT', 'WEB'] }); expect(global.fetch).toHaveBeenCalledWith( 'https://api.linear.app/graphql', expect.objectContaining({ @@ -640,32 +642,35 @@ describe('queryLinearTeamKeys', () => { }), }) as unknown as typeof fetch; - expect(await queryLinearTeamKeys('Bearer tok')).toEqual(['ABCA']); + expect(await queryLinearTeamKeys('Bearer tok')).toEqual({ ok: true, keys: ['ABCA'] }); }); - test('returns [] when Linear responds non-2xx', async () => { + test('fails (ok: false) when Linear responds non-2xx', async () => { global.fetch = jest.fn().mockResolvedValue({ ok: false, status: 500, json: async () => ({}), }) as unknown as typeof fetch; - expect(await queryLinearTeamKeys('Bearer tok')).toEqual([]); + const result = await queryLinearTeamKeys('Bearer tok'); + expect(result.ok).toBe(false); + expect(result.ok === false && result.error).toBeInstanceOf(Error); }); - test('returns [] when fetch itself throws (network failure)', async () => { - global.fetch = jest.fn().mockRejectedValue(new Error('ECONNRESET')) as unknown as typeof fetch; + test('fails (ok: false) when fetch itself throws (network failure)', async () => { + const err = new Error('ECONNRESET'); + global.fetch = jest.fn().mockRejectedValue(err) as unknown as typeof fetch; - expect(await queryLinearTeamKeys('Bearer tok')).toEqual([]); + expect(await queryLinearTeamKeys('Bearer tok')).toEqual({ ok: false, error: err }); }); - test('returns [] when GraphQL response shape is missing teams.nodes', async () => { + test('succeeds with no keys when GraphQL response shape is missing teams.nodes', async () => { global.fetch = jest.fn().mockResolvedValue({ ok: true, json: async () => ({ data: {} }), }) as unknown as typeof fetch; - expect(await queryLinearTeamKeys('Bearer tok')).toEqual([]); + expect(await queryLinearTeamKeys('Bearer tok')).toEqual({ ok: true, keys: [] }); }); });