diff --git a/cdk/package.json b/cdk/package.json index 38dc196be..acbb53b4d 100644 --- a/cdk/package.json +++ b/cdk/package.json @@ -36,7 +36,7 @@ "cdk-nag": "^2.38.2", "constructs": "^10.6.0", "js-yaml": "^4.1.1", - "pdf-parse": "^2.4.5", + "pdf-parse": "2.4.5", "ulid": "^3.0.2", "ws": "^8.21.0" }, @@ -49,7 +49,6 @@ "@types/jest": "^30.0.0", "@types/js-yaml": "^4.0.9", "@types/node": "^26", - "@types/pdf-parse": "^1.1.5", "@types/ws": "^8.18.1", "@typescript-eslint/eslint-plugin": "^8", "@typescript-eslint/parser": "^8", diff --git a/cdk/src/constructs/github-screenshot-integration.ts b/cdk/src/constructs/github-screenshot-integration.ts index b48c70864..6005afe38 100644 --- a/cdk/src/constructs/github-screenshot-integration.ts +++ b/cdk/src/constructs/github-screenshot-integration.ts @@ -61,12 +61,21 @@ export interface GitHubScreenshotIntegrationProps { /** * Optional — when provided, the processor also tries to post the * screenshot to a linked Linear issue. Resolved from the GitHub PR - * title/body via a Linear-identifier regex (e.g. `ABCA-42`), then + * title/body via a Linear-identifier regex (e.g. `ENG-42`), then * looked up across all `status='active'` workspaces in the registry * via Linear's `issueVcsBranchSearch` GraphQL. */ readonly linearWorkspaceRegistryTable?: dynamodb.ITable; + /** + * Optional — when provided, the processor persists the captured + * screenshot's public URL onto the deploy task's TaskRecord (keyed by the + * taskId in the deploy branch), so the orchestration reconciler can + * embed the integration node's combined preview in the parent epic panel. + * Unset → persistence is skipped (the PR + Linear comments still post). + */ + readonly taskTable?: dynamodb.ITable; + /** * Removal policy for the dedup table + screenshot bucket. Defaults * to DESTROY so dev stacks don't accumulate orphans on `cdk destroy`. @@ -192,6 +201,9 @@ export class GitHubScreenshotIntegration extends Construct { ...(props.linearWorkspaceRegistryTable && { LINEAR_WORKSPACE_REGISTRY_TABLE_NAME: props.linearWorkspaceRegistryTable.tableName, }), + ...(props.taskTable && { + TASK_TABLE_NAME: props.taskTable.tableName, + }), }, bundling: commonBundling, }); @@ -247,6 +259,49 @@ export class GitHubScreenshotIntegration extends Construct { })); } + // Write access so the processor can persist screenshot_url onto the + // deploy task's TaskRecord (conditional UpdateItem). grantWriteData covers + // the UpdateItem; the handler's update is guarded by attribute_exists. + if (props.taskTable) { + props.taskTable.grantWriteData(this.webhookProcessorFn); + // iteration-UX: on an iteration re-deploy the processor resolves the + // issue's most-recent maturing-reply id via a Query on LinearIssueIndex + // (to append the `· [preview]` link to that reply). grantWriteData does + // NOT include dynamodb:Query nor the index ARN, so grant it narrowly — + // Query on just that one GSI, not blanket grantReadData on the table. + // + // findIterationReplyId then GetItems each candidate's `head_sha` on the + // BASE table to attribute the deploy to the right iteration when several + // iterations overlap. That GetItem read needs dynamodb:GetItem on the + // base-table ARN — the Query GSI grant does NOT cover it. Without this, + // the GetItem throws AccessDenied, is swallowed non-fatally, and the + // preview is captured + posted to the PR but never appended to the Linear + // iteration reply (observed in practice — the head_sha refinement was + // added without extending its IAM grant). + this.webhookProcessorFn.addToRolePolicy(new iam.PolicyStatement({ + actions: ['dynamodb:Query'], + resources: [ + Stack.of(this).formatArn({ + service: 'dynamodb', + resource: 'table', + resourceName: `${props.taskTable.tableName}/index/LinearIssueIndex`, + arnFormat: ArnFormat.SLASH_RESOURCE_NAME, + }), + ], + })); + this.webhookProcessorFn.addToRolePolicy(new iam.PolicyStatement({ + actions: ['dynamodb:GetItem'], + resources: [ + Stack.of(this).formatArn({ + service: 'dynamodb', + resource: 'table', + resourceName: props.taskTable.tableName, + arnFormat: ArnFormat.SLASH_RESOURCE_NAME, + }), + ], + })); + } + // AgentCore Browser session lifecycle + automation-stream connect. // Action set scoped to the three calls the handler actually makes; // resource is `*` because Browser sessions are ephemeral and the diff --git a/cdk/src/constructs/linear-project-mapping-table.ts b/cdk/src/constructs/linear-project-mapping-table.ts index 4a0d8b072..9825795ac 100644 --- a/cdk/src/constructs/linear-project-mapping-table.ts +++ b/cdk/src/constructs/linear-project-mapping-table.ts @@ -55,6 +55,9 @@ export interface LinearProjectMappingTableProps { * - label_filter — Linear issue label that triggers a task (default `bgagent`) * - status — 'active' | 'removed' * - onboarded_at, updated_at — ISO timestamps + * + * The table is schemaless apart from the partition key, so added fields are + * additive and read with defaults — an existing row needs no migration. */ export class LinearProjectMappingTable extends Construct { /** diff --git a/cdk/src/handlers/shared/attachment-screening.ts b/cdk/src/handlers/shared/attachment-screening.ts index f8e38cb66..00e807076 100644 --- a/cdk/src/handlers/shared/attachment-screening.ts +++ b/cdk/src/handlers/shared/attachment-screening.ts @@ -248,14 +248,42 @@ const PDF_MAX_PAGES = 50; const PDF_MAX_TEXT_BYTES = 1024 * 1024; // 1 MB extracted text cap const PDF_EXTRACT_TIMEOUT_MS = 15_000; -async function extractPdfText(content: Buffer, filename: string): Promise { - // Dynamic import — pdf-parse is only used for PDF attachments. +/** + * pdf-parse v2 is built on pdfjs, which references browser DOM globals + * (`DOMMatrix`/`ImageData`/`Path2D`) that don't exist in the Node Lambda runtime. + * For TEXT extraction (our only use) these are never actually invoked — pdfjs only + * touches them on its optional canvas RENDER path. But if they're merely *undefined*, + * pdfjs tries to load the native `@napi-rs/canvas` binding to supply them, which + * fails on Lambda (the cross-platform native binary isn't bundled) and cascades to + * `DOMMatrix is not defined` → PDF screening unavailable (observed in practice). + * + * Defining them as inert no-op stubs makes pdfjs skip the native-canvas load path + * entirely and extract text headless — no native binary, host-independent. Verified: + * `getText` returns the full text with canvas absent + these three stubs present. + * Idempotent + non-clobbering (only fills genuinely-missing globals). + */ +function ensurePdfDomGlobals(): void { + const g = globalThis as Record; + if (typeof g.DOMMatrix === 'undefined') g.DOMMatrix = class { /* inert stub — text extraction never calls it */ }; + if (typeof g.ImageData === 'undefined') g.ImageData = class { /* inert stub */ }; + if (typeof g.Path2D === 'undefined') g.Path2D = class { /* inert stub */ }; +} - let pdfParseFn: (data: Buffer, options?: { max?: number }) => Promise<{ text: string }>; +async function extractPdfText(content: Buffer, filename: string): Promise { + // pdf-parse v2 (^2.4.5) exposes a `PDFParse` CLASS — `new PDFParse({ data }).getText()` — + // NOT the v1 callable default export. Three things made this fail before: + // (1) the code called the v1 `pdfParseFn(buf)` shape (undefined on v2); (2) the + // webhook processors esbuild-bundled pdf-parse instead of shipping it via `nodeModules`, + // mangling its pdfjs/native deps; and (3) pdfjs tried to load the native + // `@napi-rs/canvas` binding for its DOM globals — absent on Lambda — instead of just + // extracting text. `ensurePdfDomGlobals` fixes (3); the bundling change fixes (2). + ensurePdfDomGlobals(); + let PDFParse; try { - // pdf-parse uses a default export; handle both CJS and ESM module shapes. - const mod = await import(/* webpackIgnore: true */ 'pdf-parse'); - pdfParseFn = (mod as any).default ?? mod; + // Destructure the class from the dynamic import and let TS infer its type from + // the value — a cross-mode `typeof import('pdf-parse').PDFParse` annotation trips + // the ESM-vs-CJS dual-`.d.ts` hazard under moduleResolution:nodenext. + ({ PDFParse } = await import(/* webpackIgnore: true */ 'pdf-parse')); } catch (importErr) { logger.error('pdf-parse module could not be imported — PDF screening unavailable', { error: importErr instanceof Error ? importErr.message : String(importErr), @@ -268,22 +296,66 @@ async function extractPdfText(content: Buffer, filename: string): Promise; + // A TypedArray is preferred (pdf-parse transfers ownership to its worker, lowering + // main-thread memory). Slice to the exact PDF bytes so a pooled Buffer's backing + // ArrayBuffer isn't handed over wholesale. + const parser = new PDFParse({ data: new Uint8Array(content.buffer, content.byteOffset, content.byteLength) }); try { const timeoutPromise = new Promise((_, reject) => { timeoutId = setTimeout(() => reject(new Error('PDF extraction timed out')), PDF_EXTRACT_TIMEOUT_MS); }); + // `first: N` parses only pages 1..N (the v2 page-cap knob). We cap pages + + // extracted-text bytes to bound cost/DoS — BUT the caller stores the WHOLE PDF + // and feeds it to the agent, so screening only a prefix while delivering the + // rest is a bypass (review #1 HIGH: injection on page 51 of a 51-page PDF). + // Fail CLOSED when the document exceeds what we can screen: reject rather than + // deliver unscreened pages. `result.total` is the PDF's full page count. const result = await Promise.race([ - pdfParseFn(content, { max: PDF_MAX_PAGES }), + parser.getText({ first: PDF_MAX_PAGES }), timeoutPromise, ]); - let text: string = result.text ?? ''; + const totalPages = typeof result.total === 'number' ? result.total : undefined; + if (totalPages !== undefined && totalPages > PDF_MAX_PAGES) { + throw new AttachmentScreeningError( + `PDF "${filename}" has ${totalPages} pages, over the ${PDF_MAX_PAGES}-page limit ABCA can fully ` + + 'screen. Split it or attach only the relevant pages so the whole document can be checked.', + ); + } + const text: string = result.text ?? ''; if (Buffer.byteLength(text, 'utf-8') > PDF_MAX_TEXT_BYTES) { - text = text.slice(0, PDF_MAX_TEXT_BYTES); + // The screened pages produced more text than we screen — we'd be delivering + // bytes we didn't fully check. Fail closed rather than truncate-and-pass. + throw new AttachmentScreeningError( + `PDF "${filename}" contains more text than ABCA can fully screen (over ${PDF_MAX_TEXT_BYTES} bytes). ` + + 'Attach a smaller document so its full contents can be checked.', + ); } return text; } catch (err) { + // Our own over-limit / no-text rejections are already user-facing — don't + // re-wrap them as "corrupt PDF". + if (err instanceof AttachmentScreeningError) throw err; + // A DEPLOYMENT bug and a genuinely-bad PDF both land here, and they look + // nothing alike to an operator. pdf-parse mangled by esbuild (a Lambda that + // reaches this path but lacks `nodeModules: ['pdf-parse']`) throws with a + // pdfjs/DOM signature — `DOMMatrix is not defined`, `Cannot find native + // binding`, `@napi-rs/canvas`. Detect that and log a LOUD, actionable + // diagnostic (with the fix) so it's not misread as "user's PDF is corrupt" — + // that misdiagnosis has cost a full debug loop before. The user-facing + // message stays generic. + const msg = err instanceof Error ? err.message : String(err); + const looksLikeBundlingBug = /DOMMatrix|ImageData|Path2D|napi-rs\/canvas|Cannot find native binding|pdfjs/i.test(msg); + if (looksLikeBundlingBug) { + logger.error( + 'PDF extraction hit a pdfjs/native-binding error — this is almost certainly a BUNDLING bug, ' + + 'not a bad PDF: the Lambda screens PDFs but was esbuild-bundled without `nodeModules: [\'pdf-parse\']`. ' + + 'Add the attachment-screening bundling carve-out to this function\'s construct (see the ' + + '//:check:pdf-parse-bundling guard).', + { error: msg, filename, metric_type: 'pdf_parse_bundling_error' }, + ); + } throw new AttachmentScreeningError( `PDF "${filename}" could not be processed. It may be corrupt or use unsupported features. ` + 'Try exporting to a simpler PDF format.', @@ -291,6 +363,8 @@ async function extractPdfText(content: Buffer, filename: string): Promise { /* best-effort teardown */ }); } } diff --git a/cdk/src/handlers/shared/error-classifier.ts b/cdk/src/handlers/shared/error-classifier.ts index f68e15f46..8364a5b69 100644 --- a/cdk/src/handlers/shared/error-classifier.ts +++ b/cdk/src/handlers/shared/error-classifier.ts @@ -29,7 +29,7 @@ export const ErrorCategory = { GUARDRAIL: 'guardrail', CONFIG: 'config', TIMEOUT: 'timeout', - // Environmental blocker (#251): agent could not progress for a missing + // Environmental blocker: agent could not progress for a missing // secret, egress denial, unreachable dependency, or fail-closed policy // engine error. Distinct from AUTH/CONFIG so operators can spot the // typed, self-diagnosed faults the platform names precisely. @@ -175,10 +175,10 @@ const PATTERNS: readonly ErrorPattern[] = [ // --- Compute --- { // A task dispatched against a task-def revision that was deregistered by a - // deploy (ABCA-660/663). Transient + self-clearing on retry; the family-based - // RunTask fix prevents it going forward, but keep a precise classification so - // any historical/edge occurrence reads as "temporary, just retry", not a - // scary compute-health alarm. + // concurrent deploy. Transient + self-clearing on retry; dispatching against + // the task-definition FAMILY (rather than a pinned revision) prevents it going + // forward, but keep a precise classification so any historical/edge occurrence + // reads as "temporary, just retry", not a scary compute-health alarm. pattern: /TaskDefinition is inactive/i, classification: { category: ErrorCategory.COMPUTE, @@ -260,8 +260,8 @@ const PATTERNS: readonly ErrorPattern[] = [ // refused the binary (`OSError: [Errno 8] Exec format error: 'claude'`) or // the claude-code shim reports its platform-native binary was never placed // ("claude native binary not installed" — its postinstall silently fell - // back at image-build time). Live-caught on ABCA-659's retry: all 3 ECS runs - // died at the run_agent step this way on a freshly rebuilt image, while the + // back at image-build time). Observed in practice: three consecutive ECS runs + // all died at the run_agent step this way on a freshly rebuilt image, while the // native binary was present but unwired. This is an IMAGE/infra fault, NOT a // problem with the user's request — a fresh attempt usually lands on a host // that materializes the image cleanly; a persistent one is a bad build an @@ -303,7 +303,7 @@ const PATTERNS: readonly ErrorPattern[] = [ // ``agent/src/runner.py:515`` (the terminal-error path). // Keying on only ``agent_status=`` missed the ``subtype=`` wrapper, so a // real max-turns failure fell through to UNKNOWN → "Unexpected error" - // (live-caught on ABCA-483: a task hit the 100-turn cap but the reply + // (observed in practice: a task hit the 100-turn cap but the reply // said "Unexpected error"). Match either ``agent_status=``/``subtype=``. { // A max-turns cap is a correct, self-explanatory classification. When the @@ -347,6 +347,57 @@ const PATTERNS: readonly ErrorPattern[] = [ errorClass: ErrorClass.TRANSIENT, }, }, + { + // The build gate was KILLED by an environment fault (out of + // disk / OOM) — the code was never verified. The agent tags the verdict + // ``build_ok=infra`` so this reads as a retryable INFRA fault, not "your + // build failed" and not a bogus ✅ (a build also killed before the agent + // would otherwise look "already red → not a regression → success"). Matched + // before the generic ``Task did not succeed.*agent_status=`` catch-all. + pattern: /Task did not succeed.*build_ok=infra/i, + classification: { + category: ErrorCategory.COMPUTE, + title: 'Build couldn\'t finish — the build machine ran out of resources', + description: 'The build/verify step was stopped because the build environment ran out of disk or memory, so your changes were never actually verified — this is an infrastructure limit, not a problem with your code.', + remedy: 'Reply here to try again — a fresh run usually clears a transient resource crunch (e.g. several builds sharing a box at once). If it keeps happening on this repo, its build needs more capacity: contact your ABCA admin to raise the build task\'s disk/memory.', + retryable: true, + errorClass: ErrorClass.TRANSIENT, + }, + }, + { + // A new-work coding task reported agent-success but no commit + // reached the branch and no PR was opened — the agent's changes were LOST + // (observed cause: a stacked child edited a nested working tree that was never + // the branch's tree, so nothing committed). This is an environment/infra + // fault, not the user's code, and a fresh run usually lands the work — so + // it reads as retryable, NOT the non-retryable "your task didn't succeed". + // Ordered before the generic ``agent_status=`` catch-all so it wins. + pattern: /deliverable=lost/i, + classification: { + category: ErrorCategory.AGENT, + title: 'The change was not saved', + description: 'The agent finished, but none of its changes were committed to the branch and no pull request was opened — the work did not land in the repository. This is usually a transient workspace fault (e.g. the clone ended up in an unexpected directory), not a problem with your request.', + remedy: 'Reply here to try again — a fresh run normally saves the work correctly. If it keeps happening on this repo, share the task ID with your ABCA admin to check the agent workspace setup.', + retryable: true, + errorClass: ErrorClass.TRANSIENT, + }, + }, + { + // The sibling of the work-lost case: a commit DID land on the branch but + // the PR never opened (e.g. ``gh pr create`` failed after the push). The + // code is safe on the branch; the only missing step is opening the PR — so + // a retry (or an operator opening it by hand) recovers it. Distinct copy + // from the work-lost case so the reader knows the change is NOT gone. + pattern: /deliverable=no_pr/i, + classification: { + category: ErrorCategory.AGENT, + title: 'Change saved, but the pull request did not open', + description: 'The agent committed its changes to the branch, but the pull request could not be created (the push succeeded; opening the PR did not). Your work is safe on the branch.', + remedy: 'Reply here to try again — it will find the existing commit and open the PR. If it persists, an admin can open a PR from the task branch manually, or check the GitHub token\'s Pull requests (Read and write) scope.', + retryable: true, + errorClass: ErrorClass.TRANSIENT, + }, + }, { pattern: /Task did not succeed.*agent_status=/i, classification: { @@ -473,8 +524,8 @@ const PATTERNS: readonly ErrorPattern[] = [ ]; /** - * Canonical blocker-reason contract (#251, decision D). Matches - * ``BLOCKED[]`` and, separately, the optional `` (resource: )`` + * Canonical blocker-reason contract. Matches ``BLOCKED[]`` and, + * separately, the optional `` (resource: )`` * segment ``format_blocker_reason`` appends. This is the SINGLE source of truth * on the CDK side and must stay in lockstep with ``format_blocker_reason`` in * ``agent/src/progress_writer.py`` and the taxonomy table in @@ -500,8 +551,8 @@ export type BlockerKind = | 'unknown_environmental'; /** - * Build the canonical terminal-reason string for an orchestration-side blocker - * (#251, decision D) — the TypeScript twin of ``format_blocker_reason`` in + * Build the canonical terminal-reason string for an orchestration-side blocker — + * the TypeScript twin of ``format_blocker_reason`` in * ``agent/src/progress_writer.py``. Produces ``BLOCKED[]: `` with * `` (resource: )`` appended when ``resource`` is set, so the same * ``classifyError`` regex that handles agent-carried reasons also classifies @@ -566,7 +617,7 @@ function blockerClassification(kind: string, resource: string | undefined): Erro case 'auth_failure': { // A Secrets Manager ARN resource means the secret exists but couldn't be // read (AccessDenied / throttling) — the fix is IAM on the task role or - // blueprint wiring, NOT PAT scopes (#251 review). A non-ARN resource is a + // blueprint wiring, NOT PAT scopes. A non-ARN resource is a // runtime credential rejection where scope/validity is the right advice. const isSecretArn = res?.startsWith('arn:aws:secretsmanager:') ?? false; return { @@ -617,7 +668,7 @@ export function classifyError(errorMessage: string | undefined | null): ErrorCla return null; } - // Environmental blockers (#251) carry a canonical ``BLOCKED[]`` prefix + // Environmental blockers carry a canonical ``BLOCKED[]`` prefix // and an extractable resource — check them first so the remedy can name the // exact secret / host rather than falling through to a generic pattern. const kindMatch = BLOCKED_KIND.exec(errorMessage); diff --git a/cdk/src/handlers/shared/linear-attachments.ts b/cdk/src/handlers/shared/linear-attachments.ts new file mode 100644 index 000000000..d1188ce9f --- /dev/null +++ b/cdk/src/handlers/shared/linear-attachments.ts @@ -0,0 +1,743 @@ +/** + * 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. + */ + +/** + * Authenticated Linear attachment enrichment at task-admission time (ADR-016). + * + * ABCA runs Linear 100% deterministically — there is NO Linear MCP (see + * ADR-016 "Linear is fully deterministic"). The agent therefore cannot fetch + * `uploads.linear.app`-hosted files at runtime (that used to be + * `mcp__linear-server__extract_images`). Instead, the webhook processor fetches + * them here at admission time, AUTHENTICATED with the workspace `@bgagent` + * OAuth token, screens each through the same Bedrock Guardrail pipeline as + * every other attachment, uploads the cleaned bytes to S3, and returns `passed` + * AttachmentRecords for `createTaskCore` to persist verbatim. + * + * All platform-supported attachment types come through here, not just images: + * images (PNG/JPEG) are screened visually, files (PDF/text/csv/markdown/json/ + * log) as text — same set the inline/URL paths and `jira-attachments.ts` allow. + * An unsupported type (docx, zip, …) FAILS the task closed with a message naming + * the supported types (user deletes it + re-triggers) — not silently skipped, so + * a user who attached a spec isn't left wondering why it was ignored. + * + * This is the Linear analog of `jira-attachments.ts` — same + * select → fetch → magic-bytes → screen → upload → record shape, same + * fail-closed contract ({@link LinearAttachmentError}), same batch cleanup. The + * one Linear-specific difference is the FETCH primitive: Linear embeds uploaded + * images inline as `![alt](https://uploads.linear.app/…)` and attaches uploaded + * files as plain `[label](https://uploads.linear.app/…)` links in the issue + * description, and those signed URLs require the workspace OAuth bearer (the + * unauthenticated URL-resolver in `resolve-url-attachments.ts` deliberately + * SKIPS `uploads.linear.app` for exactly this reason — see + * `linear-webhook-processor.extractImageUrlAttachments`). Non-Linear-hosted + * markdown images (public CDNs) stay on that unauthenticated URL path; only the + * `uploads.linear.app` ones come through here. + * + * Tests: cdk/test/handlers/shared/linear-attachments.test.ts + */ + +import { createHash } from 'crypto'; +import * as dns from 'dns/promises'; +import * as net from 'net'; +import { PutObjectCommand, DeleteObjectsCommand, type S3Client } from '@aws-sdk/client-s3'; +import { screenImage, screenTextFile, AttachmentScreeningError, type ScreeningConfig } from './attachment-screening'; +import { estimateImageTokensFromBuffer } from './image-tokens'; +import { logger } from './logger'; +import { createAttachmentRecord, type PassedAttachmentRecord } from './types'; +import { EXTENSION_TO_MIME, isAllowedMimeType, isValidFilename, validateMagicBytes, MAX_ATTACHMENT_SIZE_BYTES, MAX_TOTAL_ATTACHMENT_SIZE_BYTES, MAX_ATTACHMENTS_PER_TASK, SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL } from './validation'; +import { ATTACHMENT_OBJECT_KEY_PREFIX } from '../../constructs/attachments-bucket'; + +/** Per-request timeout for a single attachment download. */ +const ATTACHMENT_FETCH_TIMEOUT_MS = 10_000; + +/** Cap on `uploads.linear.app` files pulled from one issue description. */ +const MAX_LINEAR_UPLOADS_PER_ISSUE = 10; + +/** + * Upper bound on how many markdown links we'll even enumerate while scanning a + * description, so a pathological body with thousands of links can't spin. Set + * well above any real cap — overflow past the real slot budget is handled with a + * clear error by the caller, not by this scan limit. + */ +const SCAN_HARD_CAP = 100; + +/** Max length of the derived, path-safe attachment id (S3 key segment). */ +const MAX_ATTACHMENT_ID_LENGTH = 128; + +/** Chars of pathname digest appended to an upload id to keep distinct paths distinct. */ +const UPLOAD_ID_DIGEST_CHARS = 10; + +/* eslint-disable @typescript-eslint/no-magic-numbers -- file-format magic-byte signatures */ +/** Magic-byte signatures used to sniff a body when the content-type is generic. */ +const PNG_MAGIC = [0x89, 0x50, 0x4e, 0x47] as const; +const JPEG_MAGIC = [0xff, 0xd8, 0xff] as const; +const PDF_MAGIC = [0x25, 0x50, 0x44, 0x46, 0x2d] as const; // %PDF- +/* eslint-enable @typescript-eslint/no-magic-numbers */ + +/** + * Markdown reference to a `uploads.linear.app` file. Matches BOTH the image form + * `![alt](url)` AND the plain link form `[label](url)` — Linear embeds uploaded + * images inline (`!`) but attaches uploaded files (PDFs, logs, specs) as plain + * links. The leading `!` is optional so both are captured. + * + * Capture groups: **1 = label** (the `[…]` text — for a file link this IS the + * original filename, e.g. `spec.docx`, surfaced in user-facing messages), **2 = + * URL**. The URL may be wrapped in angle brackets — `[label]()` — + * which is the CommonMark autolink form Linear NORMALIZES uploaded-file links + * into (observed in practice: a plain `[f](https://…)` link round-tripped + * through Linear comes back as `()`, and the un-bracketed pattern + * silently dropped it). The `<`/`>` are optional and excluded from the captured + * URL, and `>` is excluded from the URL body so the closing bracket can't leak in. + */ +// +// The label quantifier is BOUNDED (``{0,MAX}``) rather than open-ended. Making the +// leading ``!`` optional means the engine can no longer anchor each attempt on a +// literal ``!`` — it retries the label scan from every ``[`` — so an unbounded +// ``[^\]]*`` is quadratic in the description length. Measured on a description of +// unmatched ``[``: 4 KB ≈ 7 ms, 12 KB ≈ 56 ms, 50 KB ≈ 940 ms, and ~150 KB blows +// the webhook processor's timeout. No crafting is needed — a large pasted table +// does it. A bound caps the work per start position; a real markdown label is far +// shorter than the limit, so nothing legitimate stops matching. +// A GENEROUS label bound. This is not what stops the pathological case (see +// SCAN_MAX_DESCRIPTION_CHARS below) — it only keeps a single start position from +// scanning arbitrarily far. It must sit well above any real markdown label, +// because a label longer than the bound makes the whole link stop matching and +// the attachment is then silently MISSED. A tight bound traded a slow scan for +// lost user data, which is the worse failure: 300 dropped a 350-char descriptive +// alt text with nothing logged. +const MARKDOWN_LABEL_MAX_CHARS = 4096; + +/** + * Hard cap on the description length we scan for attachment links. + * + * This is the real bound on the work. Making the leading ``!`` optional stopped + * the engine anchoring each attempt on a literal ``!``, and BOTH variable parts + * of the pattern then backtrack per start position — the label AND the URL. So + * bounding the label alone does not help: measured on `[](https://a` repeated, + * a 100 KB description takes ~820 ms with or without a label bound, and larger + * inputs exceed the webhook processor's timeout. No crafting is needed; a + * mangled paste does it. + * + * A length cap fixes it for every hostile shape at once and is honest about the + * trade: a description longer than this has its tail unscanned, which we LOG + * rather than pass over in silence. Linear descriptions are prose written by + * humans; 64 KB is far past any real one, and the attachment count is separately + * capped by {@link SCAN_HARD_CAP}. + */ +const SCAN_MAX_DESCRIPTION_CHARS = 65_536; +const MARKDOWN_LINK_OR_IMAGE_PATTERN = new RegExp( + `!?\\[([^\\]]{0,${MARKDOWN_LABEL_MAX_CHARS}})\\]\\(]+)>?\\)`, + 'g', +); + +/** + * Thrown when a Linear attachment that was SELECTED for inclusion cannot be + * safely fetched, validated, or screened. The caller treats this as a + * fail-closed signal: reject the whole task rather than let the agent run with + * missing or unscreened context. (Attachments filtered out *before* download — + * non-`uploads.linear.app`, over-cap — are silently skipped and never raise.) + */ +export class LinearAttachmentError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); + this.name = 'LinearAttachmentError'; + } +} + +/** S3 + screening dependencies for the attachment download path. */ +export interface LinearAttachmentStorage { + readonly s3Client: S3Client; + readonly bucketName: string; + readonly screeningConfig: ScreeningConfig; + /** Platform user the task is attributed to — part of the S3 key. */ + readonly userId: string; + /** Task ID minted by the caller — part of the S3 key. */ + readonly taskId: string; + /** Workspace `@bgagent` OAuth access token (already resolved by the caller). */ + readonly accessToken: string; + /** For log correlation only. */ + readonly linearWorkspaceId: string; +} + +/** A `uploads.linear.app` file selected from the description for download. */ +interface SelectedUpload { + readonly url: string; + /** Path-traversal-safe, unique filename for the S3 key + on-disk name. */ + readonly filename: string; + /** Stable id derived from the upload path (S3 key segment + on-disk dir). */ + readonly id: string; + /** + * Human-friendly name for USER-FACING messages/logs only (the markdown + * `[label]` — the original filename the user attached, e.g. `spec.docx`). + * Never used in an S3 key or on disk (that's {@link filename}, which is + * path-safe). Falls back to `filename` when the label is empty. + */ + readonly displayName: string; +} + +/** Is this a Linear-hosted upload URL (needs the OAuth bearer to fetch)? */ +export function isLinearUploadsUrl(url: string): boolean { + try { + const host = new URL(url).hostname.toLowerCase(); + return host === 'uploads.linear.app' || host.endsWith('.uploads.linear.app'); + } catch { + return false; + } +} + +// Private/reserved IPv4 first-octet (and second-octet range) constants, named to +// document the RFC each blocks and to satisfy no-magic-numbers. +const OCTET_10_PRIVATE = 10; // 10.0.0.0/8 (RFC 1918) +const OCTET_127_LOOPBACK = 127; // 127.0.0.0/8 +const OCTET_0_ANY = 0; // 0.0.0.0/8 +const OCTET_172_PRIVATE = 172; // 172.16.0.0/12 (RFC 1918) +const OCTET_172_LO = 16; +const OCTET_172_HI = 31; +const OCTET_192_PRIVATE = 192; // 192.168.0.0/16 (RFC 1918) +const OCTET_192_SECOND = 168; +const OCTET_169_LINKLOCAL = 169; // 169.254.0.0/16 (link-local) +const OCTET_169_SECOND = 254; +const OCTET_100_CGN = 100; // 100.64.0.0/10 (carrier-grade NAT) +const OCTET_100_LO = 64; +const OCTET_100_HI = 127; +// IPv6 link-local fe80::/10 = first hextet in [0xfe80, 0xfebf]. +const IPV6_LINKLOCAL_LO = 0xfe80; +const IPV6_LINKLOCAL_HI = 0xfebf; + +/** Reject private / internal IPs (basic SSRF guard for the resolved host). */ +function isPrivateIp(ip: string): boolean { + if (net.isIPv4(ip)) { + const [a, b] = ip.split('.').map(Number); + if (a === OCTET_10_PRIVATE || a === OCTET_127_LOOPBACK || a === OCTET_0_ANY) return true; + if (a === OCTET_172_PRIVATE && b >= OCTET_172_LO && b <= OCTET_172_HI) return true; + if (a === OCTET_192_PRIVATE && b === OCTET_192_SECOND) return true; + if (a === OCTET_169_LINKLOCAL && b === OCTET_169_SECOND) return true; + if (a === OCTET_100_CGN && b >= OCTET_100_LO && b <= OCTET_100_HI) return true; + return false; + } + const lower = ip.toLowerCase(); + // IPv4-mapped / -compatible IPv6 (::ffff:169.254.169.254, ::ffff:10.0.0.1, …): + // extract the trailing dotted-quad and re-check it as IPv4, so an attacker + // can't reach the metadata endpoint or an RFC-1918 host via the v6 wrapper + // (review #8). Matches `::ffff:1.2.3.4` and `::1.2.3.4` forms. + const mapped = lower.match(/(?:::ffff:|::)(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/); + if (mapped && net.isIPv4(mapped[1])) return isPrivateIp(mapped[1]); + // IPv6 loopback + all-zeros. + if (lower === '::1' || lower === '::') return true; + // Unique-local fc00::/7 (fc.. / fd..). + if (lower.startsWith('fc') || lower.startsWith('fd')) return true; + // Link-local is the FULL fe80::/10 range = fe80..febf (not just the fe80 + // prefix). The first hextet's high 10 bits are fixed: 0xfe80–0xfebf. + const firstHextet = parseInt(lower.split(':')[0] || '0', 16); + if (firstHextet >= IPV6_LINKLOCAL_LO && firstHextet <= IPV6_LINKLOCAL_HI) return true; + return false; +} + +/** + * Derive a stable, path-traversal-safe id + filename from a `uploads.linear.app` + * URL. The id becomes an S3 key segment AND the agent-side on-disk directory + * name, so it must never traverse. Linear upload URLs look like + * `https://uploads.linear.app///?signature=…`; we key on the + * path (minus query) and sanitize the trailing name. + */ +function deriveUploadIdentity(url: string, index: number): { id: string; filename: string } { + let pathname = ''; + let lastSegment = ''; + try { + const u = new URL(url); + pathname = u.pathname; + lastSegment = decodeURIComponent(pathname.split('/').filter(Boolean).pop() ?? ''); + } catch { + pathname = url; + } + // Stable id: the path with unsafe chars collapsed to '-' (query dropped so a + // re-signed URL for the same object maps to the same id), plus a short digest + // of the FULL pathname. + // + // The digest is load-bearing, not decoration. Collapsing every unsafe char to + // '-' and then squashing runs maps '.', '-' and '/' onto the same character, so + // ``design.v1.png`` and ``design-v1.png`` produced an identical id — and both + // de-dupe sites resolve a collision by DISCARDING the second file. A user who + // attached both got one, silently, with nothing logged and no warning. The + // digest restores injectivity for practical purposes while keeping the readable + // stem for logs. + const slug = pathname.replace(/[^A-Za-z0-9_-]/g, '-').replace(/-+/g, '-').replace(/^-|-$/g, ''); + const digest = createHash('sha256').update(pathname).digest('hex').slice(0, UPLOAD_ID_DIGEST_CHARS); + const stem = (slug || `upload-${index}`).slice(0, MAX_ATTACHMENT_ID_LENGTH - UPLOAD_ID_DIGEST_CHARS - 1); + const id = `${stem}-${digest}`; + const sanitized = lastSegment.replace(/[^a-zA-Z0-9._-]/g, '_'); + // No .png default: a link-form upload may be a PDF/log. A generic fallback name + // keeps the extension out of the type decision (content-type/magic-bytes win). + const filename = isValidFilename(sanitized) ? sanitized : `linear-upload-${index}`; + return { id, filename }; +} + +/** Lowercased file extension (no dot) of a filename, or '' if none. */ +function extensionOf(filename: string): string { + const dot = filename.lastIndexOf('.'); + return dot > 0 ? filename.slice(dot + 1).toLowerCase() : ''; +} + +/** + * Collect the UNIQUE `uploads.linear.app` markdown files from a description (both + * image `![](url)` and link `[](url)` forms). Non-Linear-hosted URLs are ignored + * here (the unauthenticated URL path in `resolve-url-attachments.ts` handles + * public images). De-dupes by id. Does NOT apply the slot cap — the caller + * decides whether the count fits (so overflow is a loud error, not a silent + * truncation; review finding #2). Scanning is bounded by {@link SCAN_HARD_CAP} + * so a pathological description with thousands of links can't run unbounded. + */ +function collectLinearUploads(description: string | undefined): SelectedUpload[] { + if (!description) return []; + // Bound the scan input, not just the pattern. Truncation is announced: an + // unscanned tail could hide an attachment, and a silent miss is worse than a + // slow scan. + let scanned = description; + if (scanned.length > SCAN_MAX_DESCRIPTION_CHARS) { + logger.warn('Issue description longer than the attachment-scan cap — tail not scanned', { + description_chars: scanned.length, scan_cap: SCAN_MAX_DESCRIPTION_CHARS, + }); + scanned = scanned.slice(0, SCAN_MAX_DESCRIPTION_CHARS); + } + const selected: SelectedUpload[] = []; + const seenIds = new Set(); + let index = 0; + let match: RegExpExecArray | null; + MARKDOWN_LINK_OR_IMAGE_PATTERN.lastIndex = 0; + while ((match = MARKDOWN_LINK_OR_IMAGE_PATTERN.exec(scanned)) !== null) { + if (selected.length >= SCAN_HARD_CAP) break; + const label = (match[1] ?? '').trim(); + const url = match[2]; + if (!isLinearUploadsUrl(url)) continue; // public CDN URLs go via the URL path + const { id, filename } = deriveUploadIdentity(url, index++); + if (seenIds.has(id)) continue; + seenIds.add(id); + // The markdown label is the original filename for a file link (Linear sets + // it to the upload's name). Use it for user-facing messages only; fall back + // to the path-safe filename when the link had no/empty label. + selected.push({ url, filename, id, displayName: label || filename }); + } + return selected; +} + +/** + * GET the raw bytes of one `uploads.linear.app` URL with the OAuth bearer, + * enforcing the size cap while reading. SSRF-guarded: HTTPS-only, host must be a + * Linear uploads host, and the resolved IP must be public. Returns an outcome + * kind so the caller can force a token refresh on a 401/403. + */ +async function fetchUploadBytes( + accessToken: string, + url: string, +): Promise< + | { readonly kind: 'ok'; readonly content: Buffer; readonly contentType: string } + | { readonly kind: 'auth' } + | { readonly kind: 'error'; readonly message: string } +> { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + return { kind: 'error', message: 'invalid URL' }; + } + if (parsed.protocol !== 'https:') { + return { kind: 'error', message: 'non-HTTPS URL' }; + } + if (!isLinearUploadsUrl(url)) { + return { kind: 'error', message: 'not a Linear uploads host' }; + } + // SSRF: resolve the host and reject private/internal targets before connecting. + try { + const addrs = await dns.lookup(parsed.hostname, { all: true }); + if (addrs.length === 0 || addrs.some((a) => isPrivateIp(a.address))) { + return { kind: 'error', message: 'host resolves to a private/reserved address' }; + } + } catch (err) { + return { kind: 'error', message: `DNS resolution failed: ${err instanceof Error ? err.message : String(err)}` }; + } + + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), ATTACHMENT_FETCH_TIMEOUT_MS); + try { + const resp = await fetch(url, { + method: 'GET', + headers: { + Authorization: `Bearer ${accessToken}`, + Accept: '*/*', + }, + signal: controller.signal, + // SSRF hardening (review): the pre-fetch DNS check validates the ALLOWLISTED + // uploads.linear.app host, but a redirect could bounce the connection to an + // internal address the check never saw. A legitimate signed Linear upload + // URL serves the bytes directly (no redirect), so reject any redirect rather + // than blindly follow it to an unvalidated host. (Narrows the residual + // DNS-rebinding TOCTOU window too — the fetch can't be steered off-host.) + redirect: 'error', + }); + if (resp.status === 401 || resp.status === 403) { + return { kind: 'auth' }; + } + if (!resp.ok) { + return { kind: 'error', message: `HTTP ${resp.status}` }; + } + const contentType = (resp.headers.get('content-type') ?? '').split(';')[0].trim().toLowerCase(); + const reader = resp.body?.getReader(); + if (!reader) { + return { kind: 'error', message: 'empty response body' }; + } + const chunks: Buffer[] = []; + let total = 0; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + total += value.length; + if (total > MAX_ATTACHMENT_SIZE_BYTES) { + await reader.cancel(); + return { kind: 'error', message: 'exceeds size limit' }; + } + chunks.push(Buffer.from(value)); + } + return { kind: 'ok', content: Buffer.concat(chunks), contentType }; + } catch (err) { + return { kind: 'error', message: err instanceof Error ? err.message : String(err) }; + } finally { + clearTimeout(timer); + } +} + +/** A native "paperclip" attachment surfaced from the issue's `attachments` + * connection (distinct from description-embedded markdown links). */ +export interface LinearPaperclipAttachment { + /** The attachment's URL (only `uploads.linear.app` ones are hydrated here). */ + readonly url: string; + /** The attachment title — the original filename, used for messages + typing. */ + readonly title: string; +} + +/** + * Fetch, screen, and store the `uploads.linear.app` files an issue carries, + * returning `passed` AttachmentRecords for `createTaskCore` to persist verbatim. + * + * Sources BOTH kinds of Linear attachment (review finding #1): + * - files embedded in the description as markdown `[label](uploads.linear.app/…)`; + * - native paperclip attachments (the `attachments` connection) passed in via + * `paperclipAttachments` — the webhook processor reads those from the context + * probe. Only `uploads.linear.app`-hosted paperclips are hydrated; a paperclip + * that's an external link (GitHub, Figma, …) is left alone. + * De-duped across both sources by upload id. + * + * @param description the Linear issue description markdown (untrusted). + * @param remainingSlots attachment slots still free after public-URL image + * extraction (so the combined total respects the + * per-task cap of {@link MAX_ATTACHMENTS_PER_TASK}). + * @param ctx OAuth token + S3/screening storage deps. + * @param paperclipAttachments native paperclip attachments from the probe (optional). + * @throws LinearAttachmentError if a SELECTED upload cannot be safely fetched, + * validated, or screened, or if the count overflows the budget + * (fail-closed — reject the task). + */ +export async function downloadScreenAndStoreLinearAttachments( + description: string | undefined, + remainingSlots: number, + ctx: LinearAttachmentStorage, + paperclipAttachments: readonly LinearPaperclipAttachment[] = [], +): Promise { + const all = collectLinearUploads(description); + + // Merge native paperclip attachments (uploads.linear.app only), de-duped by id + // against the description-scanned set so a file that's both attached AND linked + // isn't fetched twice. + const seenIds = new Set(all.map((u) => u.id)); + let pcIndex = 0; + for (const pc of paperclipAttachments) { + if (all.length >= SCAN_HARD_CAP) break; + if (!isLinearUploadsUrl(pc.url)) continue; // external-link paperclip — not ours to fetch + const { id, filename } = deriveUploadIdentity(pc.url, pcIndex++); + if (seenIds.has(id)) continue; + seenIds.add(id); + all.push({ url: pc.url, filename, id, displayName: (pc.title || '').trim() || filename }); + } + + if (all.length === 0) return []; + + // Overflow is a LOUD error, not a silent truncation (review #2 + #6). This is + // reached AFTER collecting uploads (not short-circuited on remainingSlots<=0), + // so a Linear-hosted spec behind 10 public images is REJECTED, never silently + // dropped. Budget = the smaller of free per-task slots (public-URL images + // already consumed some) and the per-issue Linear cap. + const budget = Math.min(remainingSlots, MAX_LINEAR_UPLOADS_PER_ISSUE, MAX_ATTACHMENTS_PER_TASK); + const slotsConsumedElsewhere = remainingSlots < MAX_LINEAR_UPLOADS_PER_ISSUE; + if (all.length > budget) { + throw new LinearAttachmentError( + budget <= 0 + ? `This issue has ${all.length} Linear attachment(s), but the ${MAX_ATTACHMENTS_PER_TASK}-attachment ` + + 'limit is already used up by other images in the description. Remove some and re-apply the trigger label.' + : `This issue has ${all.length} Linear attachments, over the limit of ${budget} that can be processed ` + + `for one task${slotsConsumedElsewhere ? ' (some slots are used by other images in the description)' : ''}. ` + + 'Remove some attachments and re-apply the trigger label.', + ); + } + const selected = all; + + const records: PassedAttachmentRecord[] = []; + // Keys uploaded so far this batch — deleted on any failure so a + // partially-successful batch doesn't orphan S3 objects. + const uploadedKeys: string[] = []; + // Running total of REAL downloaded bytes (declared sizes don't exist for a + // signed URL, so this is the only ceiling). + let totalBytes = 0; + + try { + for (const upload of selected) { + let outcome = await fetchUploadBytes(ctx.accessToken, upload.url); + + // uploads.linear.app auth failures are terminal here: the caller already + // resolved a fresh workspace token (resolveLinearOauthToken refreshes + // proactively within 60s of expiry), so a 401/403 means the signed URL + // itself is stale/invalid, not a refreshable token — fail closed. + if (outcome.kind === 'auth') { + throw new LinearAttachmentError( + `Attachment '${upload.displayName}' could not be downloaded: Linear rejected the credential ` + + '(the signed upload URL may have expired — re-trigger the task).', + ); + } + if (outcome.kind === 'error') { + throw new LinearAttachmentError( + `Attachment '${upload.displayName}' could not be downloaded (${outcome.message}).`, + ); + } + + const content = outcome.content; + + totalBytes += content.length; + if (totalBytes > MAX_TOTAL_ATTACHMENT_SIZE_BYTES) { + throw new LinearAttachmentError( + `Issue attachments exceed the total size limit of ${MAX_TOTAL_ATTACHMENT_SIZE_BYTES} bytes.`, + ); + } + if (content.length === 0) { + throw new LinearAttachmentError(`Attachment '${upload.displayName}' is empty (0 bytes).`); + } + + // Infer the MIME from the response content-type, the filename extension, + // then the magic bytes (in that order of trust). The extension comes from + // the markdown LABEL (displayName — the original filename like `design.pdf`), + // not the S3 filename (derived from the URL path, a UUID with no extension). + // Linear embeds uploaded images inline and attaches uploaded files (PDFs, + // logs, CSVs, JSON, text) as links — both come through here. The platform + // allowlist gates the supported set: images (PNG/JPEG) and files + // (PDF/text/csv/markdown/json/log). Anything else (docx, zip, …) fails closed. + const mimeType = inferMime(outcome.contentType, upload.displayName, content); + const isImage = mimeType.startsWith('image/'); + const attachmentType = isImage ? 'image' : 'file'; + if (!mimeType || !isAllowedMimeType(mimeType, attachmentType)) { + // Unsupported type (docx, zip, …) — there's no safe screening path, so + // fail closed and tell the user to remove it and re-trigger. (We reject + // rather than silently skip: a user who attached a spec would otherwise + // get no signal it was ignored.) The supported-extension list is derived + // from the allowlist so it never drifts. + logger.warn('Rejecting Linear task: unsupported attachment type', { + linear_workspace_id: ctx.linearWorkspaceId, + attachment_filename: upload.displayName, + content_type: outcome.contentType || 'unknown', + inferred_mime: mimeType || 'unknown', + }); + // Message states the fact + the supported set; the processor's reject + // wrapper appends the "remove and re-apply the trigger label" instruction, + // so we don't repeat it here. + throw new LinearAttachmentError( + `Attachment '${upload.displayName}' is not a supported file type ` + + `(supported: ${SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL}).`, + ); + } + // Confirm the bytes match the resolved type (blocks a masquerading/polyglot + // payload). Text types have no signature — validateMagicBytes checks for + // valid, null-free UTF-8 instead. + if (!validateMagicBytes(content, mimeType)) { + throw new LinearAttachmentError( + `Attachment '${upload.displayName}' content does not match its declared type '${mimeType}'.`, + ); + } + + // Screen through the same Bedrock Guardrail pipeline as every other + // attachment — images visually, files as text. Any block or screening + // failure is fail-closed. (Pass displayName — screening uses it only in + // its own error text, never as a path.) + let screenResult; + try { + screenResult = isImage + ? await screenImage(content, mimeType, upload.displayName, ctx.screeningConfig) + : await screenTextFile(content, mimeType, upload.displayName, ctx.screeningConfig); + } catch (err) { + if (err instanceof AttachmentScreeningError) { + throw new LinearAttachmentError( + `Attachment '${upload.displayName}' was blocked by content screening: ${err.message}`, + { cause: err }, + ); + } + throw new LinearAttachmentError( + `Attachment '${upload.displayName}' could not be screened: ${err instanceof Error ? err.message : String(err)}`, + { cause: err }, + ); + } + if (screenResult.screening.status === 'blocked') { + throw new LinearAttachmentError( + `Attachment '${upload.displayName}' was blocked by content policy: ${screenResult.screening.categories.join(', ')}`, + ); + } + + // S3 key + record filename stay path-safe (upload.filename) — the agent + // writes the attachment to disk at `/` (agent/src/attachments.py), + // so it must never carry the raw user label. + const s3Key = `${ATTACHMENT_OBJECT_KEY_PREFIX}${ctx.userId}/${ctx.taskId}/${upload.id}/${upload.filename}`; + let putResult; + try { + putResult = await ctx.s3Client.send(new PutObjectCommand({ + Bucket: ctx.bucketName, + Key: s3Key, + Body: screenResult.content, + ContentType: mimeType, + })); + } catch (s3Err) { + logger.error('S3 upload failed for Linear attachment', { + linear_workspace_id: ctx.linearWorkspaceId, + attachment_filename: upload.displayName, + s3_key: s3Key, + error: s3Err instanceof Error ? s3Err.message : String(s3Err), + metric_type: 'linear_attachment_upload_failure', + }); + throw new LinearAttachmentError( + `Attachment '${upload.displayName}' could not be stored.`, + { cause: s3Err }, + ); + } + uploadedKeys.push(s3Key); + + // Only images carry a token estimate (files aren't fed to the model as + // vision tokens). Mirrors jira-attachments. + const tokenEstimate = isImage + ? estimateImageTokensFromBuffer(screenResult.content, mimeType) + : undefined; + + records.push(createAttachmentRecord({ + attachment_id: upload.id, + type: attachmentType, + content_type: mimeType, + filename: upload.filename, + s3_key: s3Key, + s3_version_id: putResult.VersionId ?? 'unversioned', + size_bytes: screenResult.content.length, + screening: { status: 'passed', screened_at: new Date().toISOString() }, + checksum_sha256: screenResult.checksum, + ...(tokenEstimate !== undefined && { token_estimate: tokenEstimate }), + }) as PassedAttachmentRecord); + + logger.info('Linear attachment downloaded, screened, and stored', { + linear_workspace_id: ctx.linearWorkspaceId, + attachment_filename: upload.displayName, + s3_key: s3Key, + }); + } + } catch (err) { + // Fail-closed: a mid-batch failure must not orphan objects already uploaded. + await deleteS3Objects(ctx.s3Client, ctx.bucketName, uploadedKeys); + throw err; + } + + return records; +} + +/** Does `content` start with the given magic-byte signature? */ +function startsWith(content: Buffer, magic: readonly number[]): boolean { + if (content.length < magic.length) return false; + return magic.every((byte, i) => content[i] === byte); +} + +/** + * Resolve the MIME type of a downloaded upload. Trust order — and crucially, the + * filename `label` is ATTACKER-CONTROLLED (it's the markdown `[label]`), so it + * must never by itself promote bytes to a BINARY type (review finding #3): + * 1. response content-type, if it is itself a platform-allowed type; + * 2. magic-byte signature for the BINARY types (PNG/JPEG/PDF) — proven by the + * bytes, not the label; + * 3. the label extension, but ONLY when it maps to a TEXT type. Binary + * extensions are ignored here — a `[x.pdf]` label can't make a non-PDF a + * PDF; only step 2's magic bytes can. Text is safe to key on the label + * because the caller then runs validateMagicBytes, whose UTF-8 check + * rejects a binary payload wearing a `.txt`/`.csv` label. + * Returns '' if none applies (caller rejects the upload as unsupported). + */ +/** Is this an allowed TEXT-family MIME (screened as UTF-8 text, never a binary)? */ +function isAllowedTextMime(mime: string): boolean { + return (mime.startsWith('text/') || mime === 'application/json') && isAllowedMimeType(mime, 'file'); +} + +function inferMime(contentType: string, label: string, content: Buffer): string { + // BYTES ARE AUTHORITATIVE (review #2): a recognised binary magic signature wins + // over ANY content-type or label. This blocks a PDF served as `text/plain` (or + // labeled `.txt`) from skipping PDF extraction and being screened as raw text. + if (startsWith(content, PNG_MAGIC)) return 'image/png'; + if (startsWith(content, JPEG_MAGIC)) return 'image/jpeg'; + if (startsWith(content, PDF_MAGIC)) return 'application/pdf'; + // No binary signature. A content-type claiming a BINARY allowed type (pdf/png/ + // jpeg) but lacking the matching magic bytes is a mismatch → reject (don't store + // bytes lying about being a PDF/image). Only a TEXT content-type is trusted here, + // and validateMagicBytes' UTF-8 gate still confirms the bytes are really text. + if (contentType && isAllowedTextMime(contentType)) return contentType; + // Fall back to the label extension — TEXT types only. The label is + // attacker-controlled, so it can never promote bytes to a binary type (#3). + const byExt = EXTENSION_TO_MIME[extensionOf(label)]; + if (byExt && isAllowedTextMime(byExt)) return byExt; + return ''; +} + +/** + * Best-effort deletion of S3 objects by key. Never throws — cleanup failure is + * logged and left to the bucket's 90-day lifecycle. Mirrors + * `jira-attachments.deleteS3Objects`. + */ +async function deleteS3Objects(s3Client: S3Client, bucketName: string, keys: readonly string[]): Promise { + if (keys.length === 0) return; + try { + const result = await s3Client.send(new DeleteObjectsCommand({ + Bucket: bucketName, + Delete: { Objects: keys.map((Key) => ({ Key })) }, + })); + if (result.Errors && result.Errors.length > 0) { + logger.error('Partial cleanup of Linear attachment objects — some remain', { + failed_keys: result.Errors.map((e) => e.Key), + }); + } + } catch (err) { + logger.error('Cleanup of Linear attachment objects failed (90-day lifecycle is backstop)', { + keys, + error: err instanceof Error ? err.message : String(err), + }); + } +} + +/** Delete previously-stored pre-screened Linear attachment objects (for the + * processor to call when createTaskCore rejects the task after upload). */ +export async function cleanupPreScreenedAttachments( + s3Client: S3Client, + bucketName: string, + records: readonly PassedAttachmentRecord[], +): Promise { + await deleteS3Objects(s3Client, bucketName, records.map((r) => r.s3_key).filter((k): k is string => Boolean(k))); +} diff --git a/cdk/src/handlers/shared/linear-issue-context-probe.ts b/cdk/src/handlers/shared/linear-issue-context-probe.ts new file mode 100644 index 000000000..dd5e87a4a --- /dev/null +++ b/cdk/src/handlers/shared/linear-issue-context-probe.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. + */ + +import { logger } from './logger'; + +/** + * Best-effort probe for additional Linear context attached to an issue — + * paperclip attachments (linked resources) and project documents — surfaced + * as a PRESENCE SIGNAL in the task description. + * + * ADR-016: the agent runs Linear deterministically and has no Linear MCP, so + * it can't fetch these at runtime. This probe therefore just FLAGS that they + * exist (titles + counts) so the agent knows the issue may reference material + * it wasn't given, and can proceed with best judgment / note the gap rather + * than assume the description is complete. It does NOT pre-fetch bodies, screen + * content, or upload to S3 — description-embedded `uploads.linear.app` files are + * pre-hydrated separately by `linear-attachments.ts`, and recent human comments + * by `linear-feedback.fetchRecentComments`. + * + * The webhook payload itself does NOT carry attachments or project.documents, + * so we ask Linear's GraphQL API once at task-creation time. + */ + +const LINEAR_GRAPHQL_URL = 'https://api.linear.app/graphql'; +const REQUEST_TIMEOUT_MS = 5000; + +/** + * Cap on attachment titles listed inline in the task-description hint; any + * beyond this are summarized as "(+N more)" so the prepended hint stays short. + */ +const MAX_HINTED_ATTACHMENT_TITLES = 5; + +/** Cap on project documents whose CONTENT is pulled into the task context. */ +const MAX_HYDRATED_PROJECT_DOCS = 5; + +/** + * Upper bound for COUNTING project docs (review #3). The content page is capped + * at {@link MAX_HYDRATED_PROJECT_DOCS}, but the presence-hint needs the TRUE + * total so it can flag docs beyond that cap (previously the count came from the + * capped content page, so docs 6+ were invisible to the hint). A second aliased + * id-only connection counts up to this bound cheaply (no bodies fetched); a + * project with more docs than this is vanishingly rare and the hint's "+N more" + * is approximate past it anyway. + */ +const MAX_COUNTED_PROJECT_DOCS = 50; + +const ISSUE_CONTEXT_QUERY = ` +query IssueContext($id: String!, $docs: Int!, $docCount: Int!) { + issue(id: $id) { + id + attachments(first: 25) { + nodes { + id + title + url + } + } + project { + id + name + documents(first: $docs) { + nodes { id title content } + } + documentsForCount: documents(first: $docCount) { + nodes { id } + } + } + } +} +`.trim(); + +/** A native paperclip attachment (title + url) from the `attachments` connection. */ +export interface LinearProbeAttachment { + readonly title: string; + readonly url: string; +} + +/** A project wiki document's title + markdown body (ADR-016 doc pre-hydration). */ +export interface LinearProbeDocument { + readonly title: string; + readonly content: string; +} + +export interface LinearIssueContextProbe { + /** Paperclip attachment titles surfaced on the issue, if any (for the hint). */ + readonly attachmentTitles: readonly string[]; + /** + * Native paperclip attachments with their URLs — the webhook processor + * hydrates the `uploads.linear.app` ones through the attachment pipeline + * (review finding #1). Distinct from description-embedded markdown links. + */ + readonly attachments: readonly LinearProbeAttachment[]; + /** Project name (only present when the issue belongs to a project). */ + readonly projectName: string | null; + /** True when the issue's project has at least one document attached. */ + readonly projectHasDocuments: boolean; + /** + * Project wiki documents WITH their content (ADR-016: pre-hydrated at + * task-creation because the agent has no Linear MCP to fetch them). The + * webhook processor screens these through the Bedrock Guardrail and folds them + * into the task description. Capped at {@link MAX_HYDRATED_PROJECT_DOCS}. + */ + readonly projectDocuments: readonly LinearProbeDocument[]; + /** + * Whether the probe actually reached Linear and parsed a result. `false` on a + * non-2xx / GraphQL error / network failure / timeout — in which case the + * empty arrays mean "we don't KNOW", not "there is nothing". Attachment + * hydration keys on this to fail-CLOSED (reject the task) rather than run blind + * when a paperclip-only spec could be silently missing (review finding #5). The + * doc/hint consumers stay advisory (fail-open) as before. + */ + readonly ok: boolean; + /** + * Total project documents Linear reported (incl. empty-body / over-cap ones NOT + * in `projectDocuments`). Lets the hint flag "there are docs we didn't hand you" + * precisely — hydratedCount < totalCount — instead of suppressing the hint the + * moment ANY doc is hydrated (review finding #6). + */ + readonly projectDocumentCount: number; +} + +const EMPTY: LinearIssueContextProbe = { + attachmentTitles: [], + attachments: [], + projectName: null, + projectHasDocuments: false, + projectDocuments: [], + ok: true, + projectDocumentCount: 0, +}; + +/** Probe result for the FAILURE case — same empty shape but `ok:false` so a + * caller can distinguish "unknown" from "genuinely empty". */ +const PROBE_FAILED: LinearIssueContextProbe = { ...EMPTY, ok: false }; + +/** + * Issue the GraphQL query. Never throws. On failure (network, auth, GraphQL + * errors, timeout) returns a probe with `ok:false` + empty arrays; on a genuine + * empty result (2xx, issue has no attachments/docs) returns `ok:true`. The + * distinction matters: a failed probe means "we don't KNOW" (a paperclip-only + * spec could be present-but-unread), so the attachment-hydration caller + * fails-CLOSED on `ok:false` rather than treating it as "nothing here" (review + * finding #5). The doc/hint consumers stay advisory (fail-open) either way. + */ +export async function probeLinearIssueContext( + accessToken: string, + issueId: string, +): Promise { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS); + try { + const resp = await fetch(LINEAR_GRAPHQL_URL, { + method: 'POST', + headers: { + 'Authorization': `Bearer ${accessToken}`, + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ + query: ISSUE_CONTEXT_QUERY, + variables: { id: issueId, docs: MAX_HYDRATED_PROJECT_DOCS, docCount: MAX_COUNTED_PROJECT_DOCS }, + }), + signal: controller.signal, + }); + if (!resp.ok) { + logger.warn('Linear issue context probe non-2xx', { status: resp.status, issue_id: issueId }); + return PROBE_FAILED; + } + const body = (await resp.json()) as { + data?: { + issue?: { + attachments?: { nodes?: Array<{ id?: string; title?: string; url?: string }> }; + project?: { + id?: string; + name?: string; + documents?: { nodes?: Array<{ id?: string; title?: string; content?: string }> }; + documentsForCount?: { nodes?: Array<{ id?: string }> }; + } | null; + }; + }; + errors?: unknown; + }; + if (body.errors) { + logger.warn('Linear issue context probe graphql errors', { issue_id: issueId, errors: body.errors }); + return PROBE_FAILED; + } + const issue = body.data?.issue; + // A well-formed 2xx GraphQL response whose `issue` is null/absent (deleted / + // not visible) is a genuine "nothing here", not a transport failure → ok:true. + if (!issue) return EMPTY; + const attachmentNodes = issue.attachments?.nodes ?? []; + const attachmentTitles = attachmentNodes + .map((a) => (typeof a?.title === 'string' ? a.title.trim() : '')) + .filter((t): t is string => t.length > 0); + const attachments = attachmentNodes + .filter((a): a is { title?: string; url: string } => typeof a?.url === 'string' && a.url.length > 0) + .map((a) => ({ title: typeof a.title === 'string' ? a.title.trim() : '', url: a.url })); + const project = issue.project ?? null; + const projectName = typeof project?.name === 'string' && project.name.trim() ? project.name.trim() : null; + const documentNodes = project?.documents?.nodes ?? []; + const projectHasDocuments = documentNodes.length > 0; + // Keep docs that actually have body text (an empty wiki page adds nothing but + // noise + a guardrail round-trip). Title defaults to "Untitled document". + const projectDocuments = documentNodes + .filter((d): d is { title?: string; content: string } => typeof d?.content === 'string' && d.content.trim().length > 0) + .map((d) => ({ title: typeof d.title === 'string' && d.title.trim() ? d.title.trim() : 'Untitled document', content: d.content })); + // Review #3: the TRUE doc total from the id-only count connection (up to + // MAX_COUNTED_PROJECT_DOCS), NOT the capped content page — so the presence + // hint can flag docs beyond the hydration cap. Fall back to the content page + // length if the count connection is absent (older API shape / test mock). + const countNodes = project?.documentsForCount?.nodes; + const projectDocumentCount = Array.isArray(countNodes) ? countNodes.length : documentNodes.length; + return { + attachmentTitles, + attachments, + projectName, + projectHasDocuments: projectHasDocuments || projectDocumentCount > 0, + projectDocuments, + ok: true, + projectDocumentCount, + }; + } catch (err) { + logger.warn('Linear issue context probe request failed', { + issue_id: issueId, + error: err instanceof Error ? err.message : String(err), + }); + return PROBE_FAILED; + } finally { + clearTimeout(timer); + } +} + +/** + * Render a one-paragraph hint the webhook processor prepends to the task + * description when the probe surfaced anything worth flagging. Returns + * an empty string when there's nothing to hint about — the processor + * skips the prepend in that case. + * + * ADR-016: the agent has no Linear MCP, so this is a PRESENCE SIGNAL for the + * material we could NOT hand it (a non-uploads paperclip like a Figma/GitHub + * link; a project doc whose body was empty or beyond the hydration cap). Project + * documents WITH content are pre-hydrated into the description separately + * (see the processor's project-docs section), so they are NOT flagged here — + * flagging included content would wrongly tell the agent to go find it. Names + * what's missing WITHOUT pointing at any (now non-existent) fetch tool. + */ +export function renderIssueContextHint(probe: LinearIssueContextProbe): string { + const bits: string[] = []; + if (probe.attachmentTitles.length > 0) { + const titles = probe.attachmentTitles + .slice(0, MAX_HINTED_ATTACHMENT_TITLES).map((t) => `"${t}"`).join(', '); + const more = probe.attachmentTitles.length > MAX_HINTED_ATTACHMENT_TITLES + ? ` (+${probe.attachmentTitles.length - MAX_HINTED_ATTACHMENT_TITLES} more)` : ''; + bits.push(`paperclip attachments — ${titles}${more}`); + } + // Flag docs we did NOT hydrate content for — empty body, or beyond the + // hydration cap. Hydrated docs are in the description already, so don't tell + // the agent to hunt for them; but if SOME docs were left out (hydrated < total) + // still flag the remainder (review finding #6 — a single hydrated doc used to + // suppress the hint for all the others). Default arrays defensively — a + // hand-built probe object in a test may omit the newer fields. + const hydratedDocCount = (probe.projectDocuments ?? []).length; + // Prefer the exact count; when a probe object omits it (older shape / test + // mock), infer: hydrated>0 → assume those are all (no extra); else if docs are + // known to exist → treat as ≥1 unhydrated so the presence hint still fires. + const totalDocCount = typeof probe.projectDocumentCount === 'number' + ? probe.projectDocumentCount + : (hydratedDocCount > 0 ? hydratedDocCount : (probe.projectHasDocuments ? 1 : 0)); + const unhydratedDocCount = Math.max(0, totalDocCount - hydratedDocCount); + if (unhydratedDocCount > 0) { + const noun = unhydratedDocCount === 1 ? 'wiki document' : `${unhydratedDocCount} wiki documents`; + const qualifier = hydratedDocCount > 0 ? ' (empty or beyond the included set)' : ''; + if (probe.projectName) { + bits.push(`project "${probe.projectName}" has ${noun}${qualifier} not included here`); + } else { + bits.push(`the project has ${noun}${qualifier} not included here`); + } + } + if (bits.length === 0) return ''; + return ( + `The Linear issue references additional context not included here: ${bits.join('; ')}. ` + + 'These live in Linear and are not attached to this task — work from the description and ' + + 'attachments you were given, and if one of these turns out to be essential, say so in the PR rather than guessing.' + ); +} diff --git a/cdk/src/handlers/shared/linear-oauth-resolver.ts b/cdk/src/handlers/shared/linear-oauth-resolver.ts index 48cc6f895..4efb0fdf8 100644 --- a/cdk/src/handlers/shared/linear-oauth-resolver.ts +++ b/cdk/src/handlers/shared/linear-oauth-resolver.ts @@ -23,7 +23,7 @@ import { PutSecretValueCommand, SecretsManagerClient, } from '@aws-sdk/client-secrets-manager'; -import { DynamoDBDocumentClient, GetCommand } from '@aws-sdk/lib-dynamodb'; +import { DynamoDBDocumentClient, GetCommand, UpdateCommand } from '@aws-sdk/lib-dynamodb'; import { logger } from './logger'; /** @@ -64,6 +64,13 @@ export interface RegistryRow { readonly workspace_slug: string; readonly oauth_secret_arn: string; readonly status: RegistryRowStatus; + /** + * When the CURRENT authorization was installed. Rewritten by every + * (re-)authorization, which is what makes it usable as an installation + * identity: a diagnosis about one grant must not be applied to its successor. + * Optional — rows written before it was recorded have none. + */ + readonly installed_at?: string; } export interface StoredOauthToken { @@ -101,6 +108,13 @@ export interface ResolverOptions { readonly dynamoDbClient?: DynamoDBDocumentClient; /** Override fetch for token-endpoint refresh in tests. */ readonly fetchImpl?: typeof fetch; + /** + * Called once the authorization is known dead (the refresh token was rejected + * and no concurrent caller had rotated it). Injected rather than written + * inline so this module keeps doing one job — resolving a token — and callers + * with registry write access opt in. Must not throw; the caller wraps it. + */ + readonly onAuthorizationRevoked?: (linearWorkspaceId: string) => Promise; } interface CacheEntry { @@ -188,6 +202,17 @@ export async function resolveLinearOauthToken( // ─── Step 3: Refresh if expiring ───────────────────────────────── if (isTokenExpiring(token.expires_at)) { + // The revoked-marker is OPT-IN, not defaulted. + // + // Every Lambda that resolves a token holds READ-ONLY access to the registry + // table, and no stack grants it write. Defaulting the marker on therefore + // meant the write ran and failed AccessDenied on every revoked refresh, and + // the failure was swallowed — so the feature read as working while being + // permanently inert, which is worse than being visibly absent. + // + // A caller that genuinely holds registry write (or supplies its own recorder) + // passes ``onAuthorizationRevoked`` explicitly. When the grant lands, flip the + // default here in the same change — not before. const refreshed = await refreshLinearToken(token, sm, row.oauth_secret_arn, options); if (!refreshed) { // Refresh failed — return null so the caller can fall back to @@ -222,6 +247,75 @@ export async function resolveLinearOauthToken( * task). Mixing the two contracts in one function silently fails open; * splitting them keeps each call site honest. */ +/** + * Mark a workspace's registry row as ``revoked``, so the dead authorization is + * discoverable instead of living only in a log line. The resolver already + * refuses a non-active row, so this also stops the pointless + * refresh-then-fail work on every subsequent event. + * + * NOT YET EFFECTIVE IN PRODUCTION: every Lambda that resolves a token currently + * has READ-ONLY access to the registry table, so this write fails AccessDenied + * and is swallowed (deliberately — recording the diagnosis must never break token + * resolution). Granting the write is deferred; until then the operator-facing + * signal is the indeterminate state from `bgagent platform doctor`, which reports + * that the workspace could not be confirmed rather than claiming it is fine. + * Tracked in the backlog under the Linear auth-revocation item. + * + * Scoped to the installation it actually diagnosed. ``status = active`` alone is + * not enough: a re-authorization writes ``active`` again, so a straggler holding + * the OLD token — a queued event, a retry, another Lambda mid-flight — would find + * the condition satisfied and revoke the working grant the operator had just + * installed, taking the workspace down again with a stale verdict. Conditioning + * on ``installed_at`` (rewritten by every re-authorization) makes the write apply + * only while the row still describes the same installation. ``expectedInstalledAt`` + * is passed by the caller rather than re-read here, because a re-read would race + * the same way. + * + * When the caller has no ``installed_at`` to name (a row written before it was + * recorded), the write falls back to requiring the attribute to still be absent — + * so a re-authorization, which adds it, likewise takes the row out of scope. + */ +export async function markWorkspaceRevoked( + ddb: DynamoDBDocumentClient, + tableName: string, + linearWorkspaceId: string, + expectedInstalledAt?: string, + now: string = new Date().toISOString(), +): Promise { + try { + await ddb.send(new UpdateCommand({ + TableName: tableName, + Key: { linear_workspace_id: linearWorkspaceId }, + UpdateExpression: 'SET #s = :revoked, revoked_at = :now, revoked_reason = :reason', + ConditionExpression: expectedInstalledAt === undefined + ? '#s = :active AND attribute_not_exists(installed_at)' + : '#s = :active AND installed_at = :installed', + ExpressionAttributeNames: { '#s': 'status' }, + ExpressionAttributeValues: { + ':revoked': 'revoked', + ':active': 'active', + ':now': now, + ':reason': 'refresh_token_rejected', + ...(expectedInstalledAt !== undefined && { ':installed': expectedInstalledAt }), + }, + })); + logger.warn('Marked Linear workspace as revoked — re-authorization required', { + linear_workspace_id: linearWorkspaceId, + }); + } catch (err) { + if ((err as { name?: string })?.name === 'ConditionalCheckFailedException') { + // Already marked, or re-authorized since this diagnosis was made — either + // way the verdict no longer describes the row, so leave it alone. + logger.info('Skipped the revoked marker — the registry row is no longer the installation diagnosed', { + linear_workspace_id: linearWorkspaceId, + }); + return; + } + throw err; + } + registryCache.delete(linearWorkspaceId); +} + export async function getRegistryRowStrict( ddb: DynamoDBDocumentClient, tableName: string, @@ -300,6 +394,7 @@ function parseRegistryRow(rawItem: unknown, linearWorkspaceId: string): Registry workspace_slug: item.workspace_slug, oauth_secret_arn: item.oauth_secret_arn, status, + ...(typeof item.installed_at === 'string' && { installed_at: item.installed_at }), }; registryCache.set(linearWorkspaceId, { value: row, expiresAt: Date.now() + REGISTRY_CACHE_TTL_MS }); return row; @@ -439,6 +534,24 @@ async function refreshLinearToken( secret_arn: secretArn, workspace_id: current.workspace_id, }); + // RECORD the verdict, don't just log it. This is the only moment the + // platform knows the authorization is dead: from here on every event for + // this workspace is dropped, and without a durable marker the sole evidence + // is this log line — so an operator sees their trigger label do nothing and + // has no way to find out why (live-caught 2026-07-25, silent for over an + // hour). Marking the registry row makes `bgagent platform doctor` able to + // report it and name the remedy. Best-effort: a failed write must not turn a + // feedback outage into a thrown handler. + if (options.onAuthorizationRevoked) { + try { + await options.onAuthorizationRevoked(current.workspace_id); + } catch (err) { + logger.warn('Could not mark the Linear workspace as revoked (non-fatal)', { + workspace_id: current.workspace_id, + error: err instanceof Error ? err.message : String(err), + }); + } + } invalidateLinearOauthCache(current.workspace_id, secretArn); return null; } diff --git a/cdk/src/handlers/shared/screenshot-url.ts b/cdk/src/handlers/shared/screenshot-url.ts index a5b4f16e0..2a1f0581e 100644 --- a/cdk/src/handlers/shared/screenshot-url.ts +++ b/cdk/src/handlers/shared/screenshot-url.ts @@ -124,3 +124,23 @@ export function buildScreenshotKey(repo: string, sha: string, deploymentId?: num export function encodeMarkdownUrl(rawUrl: string): string { return rawUrl.replaceAll('(', '%28').replaceAll(')', '%29'); } + +/** + * Pull the ABCA ``taskId`` out of a deploy PR's head branch (#247 — parent + * panel combined screenshot). ABCA names every task branch + * ``bgagent/{taskId}/{slug}`` (see ``generateBranchName``), so the task id is + * always the SECOND path segment. Returns null for any branch that doesn't + * match the ABCA shape (a human-created branch, a fork default, etc.) so the + * screenshot pipeline simply skips persistence for non-ABCA deploys. + */ +// ``bgagent`` / ``{taskId}`` / ``{slug…}`` — the ABCA branch shape needs at +// least these three segments before a task id can be extracted. +const MIN_ABCA_BRANCH_SEGMENTS = 3; + +export function extractTaskIdFromBranch(branchName: string | null | undefined): string | null { + if (!branchName) return null; + const parts = branchName.split('/'); + if (parts.length < MIN_ABCA_BRANCH_SEGMENTS || parts[0] !== 'bgagent') return null; + const taskId = parts[1]; + return taskId && taskId.length > 0 ? taskId : null; +} diff --git a/cdk/src/types/pdf-parse.d.ts b/cdk/src/types/pdf-parse.d.ts deleted file mode 100644 index 82ec15deb..000000000 --- a/cdk/src/types/pdf-parse.d.ts +++ /dev/null @@ -1,9 +0,0 @@ -declare module 'pdf-parse' { - interface PdfParseResult { - text: string; - numpages: number; - info: Record; - } - function pdfParse(data: Buffer, options?: { max?: number }): Promise; - export = pdfParse; -} diff --git a/cdk/test/constructs/github-screenshot-integration.test.ts b/cdk/test/constructs/github-screenshot-integration.test.ts index 3e415c87a..6653b1322 100644 --- a/cdk/test/constructs/github-screenshot-integration.test.ts +++ b/cdk/test/constructs/github-screenshot-integration.test.ts @@ -20,6 +20,7 @@ import { App, Stack } from 'aws-cdk-lib'; import { Match, Template } from 'aws-cdk-lib/assertions'; import * as apigw from 'aws-cdk-lib/aws-apigateway'; +import * as dynamodb from 'aws-cdk-lib/aws-dynamodb'; import * as secretsmanager from 'aws-cdk-lib/aws-secretsmanager'; import { GitHubScreenshotIntegration } from '../../src/constructs/github-screenshot-integration'; @@ -128,3 +129,75 @@ describe('GitHubScreenshotIntegration construct', () => { }); }); }); + +describe('GitHubScreenshotIntegration — task-table grants (iteration-UX)', () => { + let template: Template; + + beforeAll(() => { + const app = new App(); + const stack = new Stack(app, 'TaskTableStack'); + const api = new apigw.RestApi(stack, 'TestApi'); + const githubTokenSecret = new secretsmanager.Secret(stack, 'GitHubToken'); + // A table carrying the LinearIssueIndex GSI the processor must Query to + // find the iteration's maturing reply (to append the `· [preview]` link). + const taskTable = new dynamodb.Table(stack, 'TaskTable', { + partitionKey: { name: 'task_id', type: dynamodb.AttributeType.STRING }, + }); + taskTable.addGlobalSecondaryIndex({ + indexName: 'LinearIssueIndex', + partitionKey: { name: 'linear_issue_id', type: dynamodb.AttributeType.STRING }, + sortKey: { name: 'created_at', type: dynamodb.AttributeType.STRING }, + }); + + new GitHubScreenshotIntegration(stack, 'Screenshot', { api, githubTokenSecret, taskTable }); + template = Template.fromStack(stack); + }); + + test('grants dynamodb:Query scoped to the LinearIssueIndex GSI (not a blanket read)', () => { + // REGRESSION: the preview-link append (findIterationReplyId) Queries + // LinearIssueIndex, but grantWriteData covers only UpdateItem — without an + // explicit Query grant the processor hit AccessDenied at runtime and + // silently logged "no reply id found" (unit-mocked ddb never caught it). + // Pin a Query statement whose resource ARN names the index. + template.hasResourceProperties('AWS::IAM::Policy', { + PolicyDocument: { + Statement: Match.arrayWith([ + Match.objectLike({ + Effect: 'Allow', + Action: 'dynamodb:Query', + Resource: Match.objectLike({ + 'Fn::Join': Match.arrayWith([ + Match.arrayWith([Match.stringLikeRegexp('index/LinearIssueIndex')]), + ]), + }), + }), + ]), + }, + }); + }); + + test('grants dynamodb:GetItem on the TaskTable base ARN (head_sha attribution read)', () => { + // REGRESSION: findIterationReplyId Queries the GSI (granted above) then + // GetItems each candidate's head_sha on the BASE table to attribute a + // deploy to the right iteration when several iterations overlap. The + // Query GSI grant does NOT cover GetItem on the base table, so the read + // threw AccessDenied, was swallowed non-fatally, and the preview was posted + // to the PR but never appended to the Linear iteration reply. Pin a GetItem + // statement whose resource ARN is the base table (no index/ suffix). + template.hasResourceProperties('AWS::IAM::Policy', { + PolicyDocument: { + Statement: Match.arrayWith([ + Match.objectLike({ + Effect: 'Allow', + Action: 'dynamodb:GetItem', + Resource: Match.objectLike({ + 'Fn::Join': Match.arrayWith([ + Match.arrayWith([Match.stringLikeRegexp('table/')]), + ]), + }), + }), + ]), + }, + }); + }); +}); diff --git a/cdk/test/handlers/shared/attachment-screening.test.ts b/cdk/test/handlers/shared/attachment-screening.test.ts index 2f3405679..7662fb75e 100644 --- a/cdk/test/handlers/shared/attachment-screening.test.ts +++ b/cdk/test/handlers/shared/attachment-screening.test.ts @@ -20,6 +20,23 @@ import * as fs from 'fs'; import * as path from 'path'; import type { BedrockRuntimeClient } from '@aws-sdk/client-bedrock-runtime'; + +// pdf-parse v2 exposes a `PDFParse` CLASS (`new PDFParse({data}).getText()`), not +// the v1 callable default. Mock it at that shape so the PDF-screening tests drive +// the real v2 contract (see the PDF-extraction regression tests below). pdfjs runs +// headless in the nodejs24.x Lambda, but ts-jest's CJS transform can't spin its +// ESM worker under jest — so unit-mock the class and prove the wiring here; the +// real extraction is validated on the live deploy. +const pdfGetTextMock = jest.fn(); +const pdfDestroyMock = jest.fn().mockResolvedValue(undefined); +jest.mock('pdf-parse', () => ({ + __esModule: true, + PDFParse: jest.fn().mockImplementation(() => ({ + getText: (...args: unknown[]) => pdfGetTextMock(...args), + destroy: (...args: unknown[]) => pdfDestroyMock(...args), + })), +})); + import { assertImageUploadBytes, AttachmentScreeningError, @@ -318,6 +335,19 @@ describe('screenImage', () => { }); describe('screenTextFile', () => { + // jest config sets clearMocks:true, which wipes the PDFParse constructor's + // implementation + pdfDestroyMock's resolved value before each test. Re-establish + // them so `new PDFParse({data})` keeps returning the getText/destroy stubs. + beforeEach(() => { + (pdfDestroyMock as jest.Mock).mockResolvedValue(undefined); + // eslint-disable-next-line @typescript-eslint/no-require-imports -- re-arm the mocked ctor after clearMocks + const { PDFParse } = require('pdf-parse') as { PDFParse: jest.Mock }; + PDFParse.mockImplementation(() => ({ + getText: (...args: unknown[]) => pdfGetTextMock(...args), + destroy: (...args: unknown[]) => pdfDestroyMock(...args), + })); + }); + test('screens plain text content', async () => { const config = { bedrockClient: mockBedrockPass(), @@ -360,27 +390,83 @@ describe('screenTextFile', () => { } }); - test('throws for PDF with no extractable text', async () => { - // Mock pdf-parse to return empty text - jest.mock('pdf-parse', () => ({ - __esModule: true, - default: jest.fn().mockResolvedValue({ text: '' }), - }), { virtual: true }); + // pdf-parse v2 is mocked at the PDFParse-class level (see the jest.mock at the + // top of this file). The prior code called the v1 callable shape + // (`pdfParseFn(buf)`), undefined on v2, so every PDF fail-closed as + // "processing unavailable". These assert the v2 contract: + // `new PDFParse({data}).getText()` → `{ text }`. Real end-to-end PDF extraction + // is validated on the live deploy (pdfjs runs headless in nodejs24.x; ts-jest's + // CJS transform can't drive its ESM worker, so a class mock is the right unit). + test('extracts and screens a PDF via the v2 PDFParse class', async () => { + pdfGetTextMock.mockResolvedValueOnce({ text: 'Design spec: add CONTRIBUTORS', total: 1, pages: [] }); const config = { bedrockClient: mockBedrockPass(), guardrailId: 'test-guardrail', guardrailVersion: '1', }; + const result = await screenTextFile(Buffer.from('%PDF-1.4 real'), 'application/pdf', 'design.pdf', config); + expect(result.screening.status).toBe('passed'); + // The v2 API was actually driven: constructed with { data } + getText called. + expect(pdfGetTextMock).toHaveBeenCalledTimes(1); + expect(pdfDestroyMock).toHaveBeenCalledTimes(1); // worker released + }); - // A minimal PDF-like buffer (pdf-parse is mocked so content doesn't matter) - const content = Buffer.from('%PDF-1.4 empty'); - + test('throws for PDF with no extractable text', async () => { + pdfGetTextMock.mockResolvedValueOnce({ text: '', total: 1, pages: [] }); + const config = { + bedrockClient: mockBedrockPass(), + guardrailId: 'test-guardrail', + guardrailVersion: '1', + }; await expect( - screenTextFile(content, 'application/pdf', 'empty.pdf', config), + screenTextFile(Buffer.from('%PDF-1.4 empty'), 'application/pdf', 'empty.pdf', config), ).rejects.toThrow(/no extractable text/); }); + test('FAILS CLOSED on a PDF with more pages than can be screened (finding #1 — no partial-screen-full-deliver)', async () => { + // 51-page PDF but we only screen the first 50 → the whole file would be + // delivered to the agent with page 51 unscreened. Reject instead. + pdfGetTextMock.mockResolvedValueOnce({ text: 'benign pages 1-50', total: 51, pages: [] }); + const config = { bedrockClient: mockBedrockPass(), guardrailId: 'g', guardrailVersion: '1' }; + await expect( + screenTextFile(Buffer.from('%PDF-1.4 big'), 'application/pdf', 'big.pdf', config), + ).rejects.toThrow(/over the .*page limit|fully screen/i); + }); + + test('FAILS CLOSED when extracted text exceeds the screened byte cap (finding #1)', async () => { + pdfGetTextMock.mockResolvedValueOnce({ text: 'x'.repeat(2 * 1024 * 1024), total: 3, pages: [] }); + const config = { bedrockClient: mockBedrockPass(), guardrailId: 'g', guardrailVersion: '1' }; + await expect( + screenTextFile(Buffer.from('%PDF-1.4 verbose'), 'application/pdf', 'verbose.pdf', config), + ).rejects.toThrow(/more text than .*can fully screen/i); + }); + + test('a within-limits PDF (≤50 pages) still screens + passes', async () => { + pdfGetTextMock.mockResolvedValueOnce({ text: 'short spec', total: 3, pages: [] }); + const config = { bedrockClient: mockBedrockPass(), guardrailId: 'g', guardrailVersion: '1' }; + const result = await screenTextFile(Buffer.from('%PDF-1.4 ok'), 'application/pdf', 'ok.pdf', config); + expect(result.screening.status).toBe('passed'); + }); + + test('a pdfjs/DOMMatrix parse error logs a BUNDLING diagnostic, not just "corrupt PDF"', async () => { + // A Lambda that reaches this path but lacks the pdf-parse bundling carve-out + // throws a pdfjs/native-binding error. Detect the signature + log an + // actionable diagnostic so it's not misread as a bad user PDF. + const { logger } = jest.requireActual('../../../src/handlers/shared/logger') as { logger: { error: (...a: unknown[]) => void } }; + const errSpy = jest.spyOn(logger, 'error').mockImplementation(() => {}); + pdfGetTextMock.mockRejectedValueOnce(new Error('DOMMatrix is not defined')); + const config = { bedrockClient: mockBedrockPass(), guardrailId: 'g', guardrailVersion: '1' }; + await expect( + screenTextFile(Buffer.from('%PDF-1.4 real'), 'application/pdf', 'spec.pdf', config), + ).rejects.toBeInstanceOf(AttachmentScreeningError); + // The loud diagnostic names the bundling cause + the fix. + const logged = errSpy.mock.calls.map((c) => String(c[0])).join('\n'); + expect(logged).toMatch(/BUNDLING bug/); + expect(logged).toMatch(/nodeModules/); + errSpy.mockRestore(); + }); + test('retries on transient Bedrock errors for text screening', async () => { const send = jest.fn() .mockRejectedValueOnce({ $metadata: { httpStatusCode: 503 }, message: 'service unavailable' }) diff --git a/cdk/test/handlers/shared/error-classifier.test.ts b/cdk/test/handlers/shared/error-classifier.test.ts index 43d0a315d..9649b3502 100644 --- a/cdk/test/handlers/shared/error-classifier.test.ts +++ b/cdk/test/handlers/shared/error-classifier.test.ts @@ -157,7 +157,7 @@ describe('classifyError', () => { expect(result!.retryable).toBe(true); }); - test('classifies claude Exec-format / broken-shim as a transient image issue (ABCA-659, not "Unexpected error")', () => { + test('classifies claude Exec-format / broken-shim as a transient image issue, not "Unexpected error"', () => { // The raw run_agent failure the broken agent image produced. const result = classifyError( "Workflow run_agent step failed: OSError: [Errno 8] Exec format error: 'claude'", @@ -244,6 +244,54 @@ describe('classifyError', () => { expect(result!.retryable).toBe(false); }); + test('build_ok=infra is a retryable COMPUTE fault, not "did not succeed"/build-failed', () => { + // A build killed by ENOSPC/OOM never verified the code — must read as a + // transient infra fault (retry / more capacity), NOT the generic + // agent-did-not-succeed or a bogus build failure. Ordered before the + // agent_status catch-all so it wins. + const result = classifyError( + "Task did not succeed (agent_status='success', build_ok=infra)", + ); + expect(result!.category).toBe(ErrorCategory.COMPUTE); + expect(result!.title).toMatch(/ran out of resources/i); + expect(result!.retryable).toBe(true); + expect(result!.errorClass).toBe(ErrorClass.TRANSIENT); + expect(result!.remedy).toMatch(/try again|capacity|admin/i); + }); + + test('deliverable=lost is a retryable AGENT fault (work not saved), not the generic did-not-succeed', () => { + // A new-work task reported agent-success but no commit reached the branch + // and no PR opened — the agent's changes were LOST (nested-clone workspace + // fault). Must read as retryable/transient with "not saved" copy, NOT the + // non-retryable "Agent task did not succeed". Ordered before the + // agent_status catch-all so it wins. + const result = classifyError( + 'Task did not succeed (agent_status=success, deliverable=lost): the coding ' + + 'task reported success but no commit reached the branch and no PR was opened ' + + "— the agent's changes did not land in the task's repository.", + ); + expect(result!.category).toBe(ErrorCategory.AGENT); + expect(result!.title).toMatch(/not saved/i); + expect(result!.retryable).toBe(true); + expect(result!.errorClass).toBe(ErrorClass.TRANSIENT); + expect(result!.remedy).toMatch(/try again/i); + }); + + test('deliverable=no_pr says the work is SAFE on the branch (the pull request just did not open)', () => { + // A commit DID land but the PR never opened — recoverable, and the copy + // must reassure the change is not gone. Distinct from deliverable=lost. + const result = classifyError( + 'Task did not succeed (agent_status=success, deliverable=no_pr): a commit ' + + 'reached the branch but no PR was opened — the change is on the branch but ' + + 'was not delivered.', + ); + expect(result!.category).toBe(ErrorCategory.AGENT); + expect(result!.title).toMatch(/pull request did not open/i); + expect(result!.retryable).toBe(true); + expect(result!.errorClass).toBe(ErrorClass.TRANSIENT); + expect(result!.description).toMatch(/safe on the branch/i); + }); + test('classifies error_max_turns as TIMEOUT with specific title (ordered before generic catch-all)', () => { // Regression guard: pre-fix, the agent's specific // ``agent_status='error_max_turns'`` signal was swallowed by the @@ -258,14 +306,15 @@ describe('classifyError', () => { expect(result!.remedy).toMatch(/--max-turns/); }); - test('ABCA-662: max_turns with an observed repeated failure stays "Exceeded max turns" and makes NO causal claim', () => { + test('max_turns with an observed repeated failure stays "Exceeded max turns" and makes NO causal claim', () => { // When the agent capped out with the last several calls being the same // repeated failure, the pipeline appends a NEUTRAL observation ("last tool // calls repeated: …"). The classification must NOT re-title the failure as // "retrying a failing step" or assert more turns wouldn't help — the window // (last few calls) can't tell a hard blocker from a long task that hit a - // recoverable snag late (662: siblings pushed fine → transient). It stays the - // plain max_turns bucket; the observed detail rides along in the message. + // recoverable snag late (observed: sibling tasks pushed fine, so the same + // repeated push failure was transient after all). It stays the plain + // max_turns bucket; the observed detail rides along in the message. const result = classifyError( "Agent session error (subtype='error_max_turns') — last tool calls repeated: " + '`git push --force-with-lease` — remote: invalid credentials fatal: exit 128', @@ -299,12 +348,13 @@ describe('classifyError', () => { expect(result!.retryable).toBe(true); }); - test('classifies the runner.py "Agent session error (subtype=...)" wrapper, not just agent_status= (K5, live-caught ABCA-483)', () => { + test('classifies the runner.py "Agent session error (subtype=...)" wrapper, not just agent_status=', () => { // runner.py:515 emits ``Agent session error (subtype='error_max_turns')`` - // — a DIFFERENT wrapper from pipeline.py's ``agent_status=``. Pre-K5 this - // fell through to UNKNOWN → "Unexpected error" even though the task hit the - // 100-turn cap (live: a 1-line README task burned 101 turns, reply said - // "Unexpected error"). The pattern must match the subtype= wrapper too. + // — a DIFFERENT wrapper from pipeline.py's ``agent_status=``. Matching only + // the ``agent_status=`` form let this fall through to UNKNOWN → "Unexpected + // error" even though the task hit the 100-turn cap (observed: a 1-line + // README task burned 101 turns and the reply said "Unexpected error"). + // The pattern must match the subtype= wrapper too. const turns = classifyError("Agent session error (subtype='error_max_turns')"); expect(turns!.title).toBe('Exceeded max turns'); expect(turns!.category).toBe(ErrorCategory.TIMEOUT); @@ -408,8 +458,8 @@ describe('classifyError', () => { expect(result!.retryable).toBe(false); }); - test('classifies a build/verify command TIMEOUT distinctly from a crash (ABCA-667 live-caught)', () => { - // The fork's full `mise run build` exceeded the 600s cap → Python + test('classifies a build/verify command TIMEOUT distinctly from a crash', () => { + // A repo's full `mise run build` exceeded the 600s cap → Python // TimeoutExpired. Before this pattern it fell to "Unexpected error"; now it // reads as a build-time-out (user-actionable: retry / raise the cap), not a // mysterious crash. @@ -420,12 +470,13 @@ describe('classifyError', () => { expect(result!.title).toMatch(/didn't finish in time|timed out/i); // A timeout is user-actionable (retry / raise the cap), not a hard failure. expect(result!.retryable).toBe(true); + expect(result!.errorClass).toBe(ErrorClass.USER); // Must NOT fall through to the generic Unexpected error. expect(result!.title).not.toMatch(/Unexpected error/i); }); }); - // --- Environmental blockers (#251) --- + // --- Environmental blockers --- describe('blocker errors (canonical BLOCKED[] prefix)', () => { test('classifies missing_secret and extracts the secret name', () => { @@ -471,7 +522,7 @@ describe('classifyError', () => { expect(result!.remedy).toContain('scopes'); }); - test('auth_failure with a Secrets Manager ARN gives IAM remedy, not PAT scopes (#251 review)', () => { + test('auth_failure with a Secrets Manager ARN gives IAM remedy, not PAT scopes', () => { const arn = 'arn:aws:secretsmanager:us-east-1:123456789012:secret:gh-token-abc'; const result = classifyError(`BLOCKED[auth_failure]: the required GitHub token secret could not be read (resource: ${arn})`); expect(result!.category).toBe(ErrorCategory.BLOCKED); @@ -616,10 +667,10 @@ describe('classifyError', () => { expect(g).toMatch(/edit the request/i); }); - // #599 N3: pin the two USER fall-through branches so the #247 failure-renderer - // contract can't rot silently. Built as explicit classifications (the exact - // category/errorClass/retryable each branch keys on) rather than relying on a - // sample string that might reclassify later. + // Pin the two USER fall-through branches so the orchestration + // failure-renderer contract can't rot silently. Built as explicit + // classifications (the exact category/errorClass/retryable each branch keys + // on) rather than relying on a sample string that might reclassify later. test('retryGuidance: retryable USER (non-guardrail) → "reply here with any extra guidance"', () => { const cls: ErrorClassification = { category: ErrorCategory.AGENT, @@ -757,7 +808,7 @@ describe('classifyError', () => { expect(detail.turns_completed).toBeNull(); }); - // Compile-time regression for Finding #10 — ``ChannelSource`` is a + // Compile-time regression guard — ``ChannelSource`` is a // literal union, not ``string``. The ``satisfies`` assertions below // exercise the valid members; the ``@ts-expect-error`` comments pin // the narrowing — if someone widens ``ChannelSource`` to ``string`` diff --git a/cdk/test/handlers/shared/linear-attachments.test.ts b/cdk/test/handlers/shared/linear-attachments.test.ts new file mode 100644 index 000000000..e9d4e6f3d --- /dev/null +++ b/cdk/test/handlers/shared/linear-attachments.test.ts @@ -0,0 +1,534 @@ +/** + * 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 screenImageMock = jest.fn(); +const screenTextFileMock = jest.fn(); +jest.mock('../../../src/handlers/shared/attachment-screening', () => { + const actual = jest.requireActual('../../../src/handlers/shared/attachment-screening'); + return { + ...actual, + screenImage: (...args: unknown[]) => screenImageMock(...args), + screenTextFile: (...args: unknown[]) => screenTextFileMock(...args), + }; +}); + +// dns.lookup is used for the SSRF guard — resolve to a public IP by default. +const dnsLookupMock = jest.fn(); +jest.mock('dns/promises', () => ({ + lookup: (...args: unknown[]) => dnsLookupMock(...args), +})); + +import { AttachmentScreeningError, type ScreeningConfig } from '../../../src/handlers/shared/attachment-screening'; +import { + downloadScreenAndStoreLinearAttachments, + isLinearUploadsUrl, + LinearAttachmentError, +} from '../../../src/handlers/shared/linear-attachments'; +import { logger } from '../../../src/handlers/shared/logger'; +import { MAX_ATTACHMENT_SIZE_BYTES } from '../../../src/handlers/shared/validation'; + +const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00, 0x00]); +const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0x00, 0x10]); +const PDF_BYTES = Buffer.from('%PDF-1.7\n1 0 obj\n<<>>\nendobj\n'); +const TEXT_BYTES = Buffer.from('log line one\nlog line two\n'); + +const putSendMock = jest.fn(); +const s3Client = { send: putSendMock } as unknown as import('@aws-sdk/client-s3').S3Client; + +const screeningConfig: ScreeningConfig = { + guardrailId: 'gr-1', + guardrailVersion: '1', + bedrockClient: {} as never, +}; + +function storageCtx() { + return { + s3Client, + bucketName: 'attachments-bucket', + screeningConfig, + userId: 'user-1', + taskId: 'task-1', + accessToken: 'lin_oauth_at', + linearWorkspaceId: 'ws-1', + }; +} + +const UPLOAD_URL = 'https://uploads.linear.app/aaaa-1111/bbbb-2222/screenshot.png?signature=abc'; +function desc(...urls: string[]): string { + return `Some issue text\n\n${urls.map((u, i) => `![img${i}](${u})`).join('\n')}\n\nmore text`; +} +/** Description with plain-link (file) markdown `[label](url)` rather than image `![]()`. */ +function fileDesc(...urls: string[]): string { + return `Some issue text\n\n${urls.map((u, i) => `[file${i}](${u})`).join('\n')}\n\nmore text`; +} +/** Description with an explicit markdown link label (the original filename). */ +function labeledDesc(label: string, url: string): string { + return `Some issue text\n\n[${label}](${url})\n\nmore text`; +} + +/** A fetch Response-like object whose body streams the given buffer once. */ +function bytesResponse(buf: Buffer, status = 200, contentType = 'image/png'): Response { + let sent = false; + return { + ok: status >= 200 && status < 300, + status, + headers: { get: (h: string) => (h.toLowerCase() === 'content-type' ? contentType : null) }, + body: { + getReader() { + return { + read() { + if (sent) return Promise.resolve({ done: true, value: undefined }); + sent = true; + return Promise.resolve({ done: false, value: new Uint8Array(buf) }); + }, + cancel() { return Promise.resolve(); }, + }; + }, + }, + } as unknown as Response; +} + +beforeEach(() => { + screenImageMock.mockReset(); + screenImageMock.mockImplementation((content: Buffer) => Promise.resolve({ + content, + checksum: 'sha256:abc', + screening: { status: 'passed', screened_at: '2026-07-22T00:00:00Z' }, + })); + screenTextFileMock.mockReset(); + screenTextFileMock.mockImplementation((content: Buffer) => Promise.resolve({ + content, + checksum: 'sha256:def', + screening: { status: 'passed', screened_at: '2026-07-22T00:00:00Z' }, + })); + putSendMock.mockReset(); + putSendMock.mockResolvedValue({ VersionId: 'v1' }); + dnsLookupMock.mockReset(); + dnsLookupMock.mockResolvedValue([{ address: '203.0.113.7', family: 4 }]); + (global.fetch as unknown) = jest.fn(); +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +describe('isLinearUploadsUrl', () => { + test('matches uploads.linear.app and subdomains, rejects others', () => { + expect(isLinearUploadsUrl('https://uploads.linear.app/x/y/z.png')).toBe(true); + expect(isLinearUploadsUrl('https://eu.uploads.linear.app/x/y/z.png')).toBe(true); + expect(isLinearUploadsUrl('https://cdn.example.com/z.png')).toBe(false); + expect(isLinearUploadsUrl('not a url')).toBe(false); + }); +}); + +describe('downloadScreenAndStoreLinearAttachments', () => { + test('happy path: fetches uploads.linear.app image with the OAuth bearer, screens, uploads, returns a passed record', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(PNG_BYTES)); + const records = await downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()); + + expect(records).toHaveLength(1); + expect(records[0].type).toBe('image'); + expect(records[0].content_type).toBe('image/png'); + expect(records[0].screening.status).toBe('passed'); + expect(screenImageMock).toHaveBeenCalled(); + expect(putSendMock).toHaveBeenCalledTimes(1); + // Bearer header carried the workspace token. + const init = (global.fetch as jest.Mock).mock.calls[0][1] as { headers: Record }; + expect(init.headers.Authorization).toBe('Bearer lin_oauth_at'); + }); + + test('two uploads whose names differ only by . vs - are BOTH kept', async () => { + // The id collapsed '.', '-' and '/' to the same char, so design.v1.png and + // design-v1.png produced an identical id — and both de-dupe sites resolve a + // collision by DISCARDING the second file. A user who attached both silently + // got one, with nothing logged. Distinct paths must stay distinct. + (global.fetch as jest.Mock).mockImplementation(() => Promise.resolve(bytesResponse(PNG_BYTES))); + const records = await downloadScreenAndStoreLinearAttachments( + desc('https://uploads.linear.app/u/p/design.v1.png', 'https://uploads.linear.app/u/p/design-v1.png'), + 10, + storageCtx(), + ); + expect(records).toHaveLength(2); + expect(new Set(records.map((r) => r.attachment_id)).size).toBe(2); + }); + + test('a long descriptive label does NOT silently drop the attachment', async () => { + // A tight label bound made a link with long alt text stop matching, so the + // file vanished with nothing logged — trading a slow scan for lost user data. + (global.fetch as jest.Mock).mockImplementation(() => Promise.resolve(bytesResponse(PNG_BYTES))); + const longLabel = 'a detailed description of this diagram '.repeat(20); // ~780 chars + const records = await downloadScreenAndStoreLinearAttachments( + `![${longLabel}](https://uploads.linear.app/u/p/spec.png)`, 10, storageCtx(), + ); + expect(records).toHaveLength(1); + }); + + test('a hostile URL-shaped description is bounded too, not just a bracket run', async () => { + // BOTH variable parts of the pattern backtrack per start position. Bounding + // the label alone left `[](https://a` repeated just as slow as before, which + // is the shape a mangled paste produces. + // + // Asserts the SCAN INPUT is bounded rather than a wall-clock budget: a timing + // assertion in a parallel suite measures CPU contention as much as the code, + // and this one failed under full-suite load while passing alone. The warning + // is the observable proof the cap engaged — and that the truncation is + // announced rather than silent. + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => undefined); + try { + const hostile = '[](https://a'.repeat(20_000); // ~240 KB, far past the cap + const records = await downloadScreenAndStoreLinearAttachments(hostile, 10, storageCtx()); + expect(records).toEqual([]); + const capped = warn.mock.calls.some(([msg]) => String(msg).includes('attachment-scan cap')); + expect(capped).toBe(true); + } finally { + warn.mockRestore(); + } + }); + + test('a large bracket-heavy description scans in bounded time, not quadratic', async () => { + // Making the leading '!' optional stopped the engine anchoring on a literal + // '!', so an unbounded label quantifier retried from every '[' — 50 KB of + // unmatched brackets took ~940 ms and ~150 KB blew the webhook timeout. No + // crafting needed; a big pasted table does it. + // Asserts the scan INPUT is bounded, not a wall-clock budget: a timing + // assertion in a parallel suite measures CPU contention as much as the code, + // and this one passed alone at ~690 ms while failing under full-suite load. + const warn = jest.spyOn(logger, 'warn').mockImplementation(() => undefined); + try { + const hostile = '['.repeat(200_000); + const records = await downloadScreenAndStoreLinearAttachments(hostile, 10, storageCtx()); + expect(records).toEqual([]); + expect(warn.mock.calls.some(([m]) => String(m).includes('attachment-scan cap'))).toBe(true); + } finally { + warn.mockRestore(); + } + }); + + test('ignores non-uploads.linear.app images (public CDN images go via the URL path)', async () => { + const records = await downloadScreenAndStoreLinearAttachments( + desc('https://cdn.example.com/pic.png'), 10, storageCtx(), + ); + expect(records).toHaveLength(0); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + test('no-op when description is empty or has no uploads', async () => { + expect(await downloadScreenAndStoreLinearAttachments(undefined, 10, storageCtx())).toEqual([]); + expect(await downloadScreenAndStoreLinearAttachments('plain text', 10, storageCtx())).toEqual([]); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + test('overflow past remainingSlots throws (loud, not a silent truncation) — finding #2', async () => { + (global.fetch as jest.Mock).mockImplementation(() => Promise.resolve(bytesResponse(PNG_BYTES))); + const urls = Array.from({ length: 5 }, (_, i) => `https://uploads.linear.app/u/${i}/p${i}.png`); + // 5 uploads but only 2 slots free → reject the task rather than drop 3 silently. + await expect( + downloadScreenAndStoreLinearAttachments(desc(...urls), 2, storageCtx()), + ).rejects.toThrow(/over the limit of 2|Remove some attachments/i); + // Nothing stored — rejected before the download loop. + expect(putSendMock).not.toHaveBeenCalled(); + }); + + test('exactly filling the slot budget is fine (no overflow error)', async () => { + (global.fetch as jest.Mock).mockImplementation(() => Promise.resolve(bytesResponse(PNG_BYTES))); + const urls = Array.from({ length: 2 }, (_, i) => `https://uploads.linear.app/u/${i}/p${i}.png`); + const records = await downloadScreenAndStoreLinearAttachments(desc(...urls), 2, storageCtx()); + expect(records).toHaveLength(2); + }); + + test('de-dupes the same upload referenced twice', async () => { + (global.fetch as jest.Mock).mockImplementation(() => Promise.resolve(bytesResponse(PNG_BYTES))); + const records = await downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL, UPLOAD_URL), 10, storageCtx()); + expect(records).toHaveLength(1); + }); + + test('zero remainingSlots WITH a Linear upload present → REJECTS (no silent drop, finding #6)', async () => { + // Regression: this used to silently return [] (the spec was dropped while the + // task ran). Now a Linear upload with no free slots fails loud. + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 0, storageCtx()), + ).rejects.toThrow(/already used up|limit/i); + expect(global.fetch).not.toHaveBeenCalled(); // rejected before fetching + }); + + test('zero remainingSlots with NO uploads → clean no-op', async () => { + const records = await downloadScreenAndStoreLinearAttachments('plain text, no uploads', 0, storageCtx()); + expect(records).toEqual([]); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + test('401 on download → LinearAttachmentError (signed URL stale; fail-closed, no refresh loop)', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(Buffer.alloc(0), 401)); + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()), + ).rejects.toBeInstanceOf(LinearAttachmentError); + expect((global.fetch as jest.Mock)).toHaveBeenCalledTimes(1); // no retry loop for a stale signed URL + }); + + test('zero-byte body → LinearAttachmentError (fail-closed)', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(Buffer.alloc(0))); + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()), + ).rejects.toBeInstanceOf(LinearAttachmentError); + expect(putSendMock).not.toHaveBeenCalled(); + }); + + test('magic-byte mismatch → LinearAttachmentError (fail-closed)', async () => { + // content-type says png but bytes are junk + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(Buffer.from([0x00, 0x01, 0x02]), 200, 'image/png')); + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()), + ).rejects.toBeInstanceOf(LinearAttachmentError); + expect(screenImageMock).not.toHaveBeenCalled(); + }); + + test('fetches a PDF file link, screens it as text, returns a file record', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(PDF_BYTES, 200, 'application/pdf')); + const records = await downloadScreenAndStoreLinearAttachments( + fileDesc('https://uploads.linear.app/u/p/design.pdf'), 10, storageCtx(), + ); + expect(records).toHaveLength(1); + expect(records[0].type).toBe('file'); + expect(records[0].content_type).toBe('application/pdf'); + expect(records[0].token_estimate).toBeUndefined(); // files don't carry a vision-token estimate + expect(screenTextFileMock).toHaveBeenCalled(); + expect(screenImageMock).not.toHaveBeenCalled(); + expect(putSendMock).toHaveBeenCalledTimes(1); + }); + + test('matches the angle-bracket autolink URL form Linear normalizes links into', async () => { + // Linear round-trips `[f](https://…)` into `[f]()`. The un-bracketed + // pattern dropped it silently (observed in practice) — the attachment + // never reached S3 and the task ran without it. + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(PDF_BYTES, 200, 'application/pdf')); + const autolinkDesc = 'See [design.pdf]() attached.'; + const records = await downloadScreenAndStoreLinearAttachments(autolinkDesc, 10, storageCtx()); + expect(records).toHaveLength(1); + expect(records[0].type).toBe('file'); + expect(records[0].content_type).toBe('application/pdf'); + // The captured URL must NOT include the trailing '>' (the fetch must hit the real URL). + const fetchedUrl = (global.fetch as jest.Mock).mock.calls[0][0] as string; + expect(fetchedUrl).toBe('https://uploads.linear.app/u/p/design.pdf'); + }); + + test('types a generic octet-stream response by the .log extension in its markdown label', async () => { + // The extension comes from the markdown LABEL (the original filename), since + // the uploads.linear.app URL path is a UUID with no extension. + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(TEXT_BYTES, 200, 'application/octet-stream')); + const records = await downloadScreenAndStoreLinearAttachments( + labeledDesc('output.log', 'https://uploads.linear.app/u/l/9c8b-uuid'), 10, storageCtx(), + ); + expect(records).toHaveLength(1); + expect(records[0].type).toBe('file'); + expect(records[0].content_type).toBe('text/x-log'); + expect(screenTextFileMock).toHaveBeenCalled(); + }); + + test('REJECTS an unsupported type fail-closed, naming the FRIENDLY filename + supported types', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce( + bytesResponse(Buffer.from([0x50, 0x4b, 0x03, 0x04]), 200, 'application/zip'), + ); + // The URL path is a UUID; the friendly name comes from the markdown label. + await expect( + downloadScreenAndStoreLinearAttachments( + labeledDesc('spec.docx', 'https://uploads.linear.app/u/z/9a8b7c6d-uuid'), 10, storageCtx(), + ), + ).rejects.toThrow(/Attachment 'spec\.docx' is not a supported file type.*PDF/i); + // Never screened or stored — rejected before that. + expect(screenImageMock).not.toHaveBeenCalled(); + expect(screenTextFileMock).not.toHaveBeenCalled(); + expect(putSendMock).not.toHaveBeenCalled(); + }); + + test('surfaces the friendly filename (markdown label) in a screening-block error, not the UUID', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(PDF_BYTES, 200, 'application/pdf')); + screenTextFileMock.mockRejectedValueOnce(new AttachmentScreeningError('blocked: prompt attack')); + await expect( + downloadScreenAndStoreLinearAttachments( + labeledDesc('design.pdf', 'https://uploads.linear.app/u/p/deadbeef-uuid'), 10, storageCtx(), + ), + ).rejects.toThrow(/Attachment 'design\.pdf' was blocked by content screening/); + }); + + test('falls back to the path-safe filename when the markdown label is empty', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce( + bytesResponse(Buffer.from([0x50, 0x4b, 0x03, 0x04]), 200, 'application/zip'), + ); + // Empty label `[]( … )` → displayName falls back to the derived filename (no crash, still rejects). + await expect( + downloadScreenAndStoreLinearAttachments( + labeledDesc('', 'https://uploads.linear.app/u/z/bundle-uuid'), 10, storageCtx(), + ), + ).rejects.toThrow(/not a supported file type/i); + }); + + test('a .txt-LABELED binary payload is REJECTED, not stored as text (finding #3)', async () => { + // Attacker labels a binary octet-stream `[diagram.txt]`. The label must NOT + // promote it to text/plain past the UTF-8 gate — bytes with invalid UTF-8 / + // nulls fail validateMagicBytes, so it's rejected as unsupported. + const binary = Buffer.from([0xff, 0xfe, 0x00, 0x01, 0x80, 0x81]); // invalid UTF-8 + null + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(binary, 200, 'application/octet-stream')); + await expect( + downloadScreenAndStoreLinearAttachments( + labeledDesc('diagram.txt', 'https://uploads.linear.app/u/x/evil-uuid'), 10, storageCtx(), + ), + ).rejects.toThrow(/not a supported file type|does not match its declared type/i); + expect(screenTextFileMock).not.toHaveBeenCalled(); + expect(putSendMock).not.toHaveBeenCalled(); + }); + + test('a .pdf-LABELED non-PDF is NOT promoted to application/pdf by the label (finding #3)', async () => { + // Only magic bytes can vouch for a binary type. A `[x.pdf]` label over + // non-PDF octet-stream bytes must not be treated as a PDF. + (global.fetch as jest.Mock).mockResolvedValueOnce( + bytesResponse(Buffer.from([0x00, 0x01, 0x02, 0x03]), 200, 'application/octet-stream'), + ); + await expect( + downloadScreenAndStoreLinearAttachments( + labeledDesc('notreally.pdf', 'https://uploads.linear.app/u/x/fake-uuid'), 10, storageCtx(), + ), + ).rejects.toThrow(/not a supported file type/i); + }); + + test('hydrates a native paperclip attachment (uploads.linear.app) — finding #1', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(PDF_BYTES, 200, 'application/pdf')); + // No description link; the file arrives via the paperclip list (4th arg). + const records = await downloadScreenAndStoreLinearAttachments( + 'Implement the attached spec.', 10, storageCtx(), + [{ title: 'spec.pdf', url: 'https://uploads.linear.app/u/pc/paperclip-uuid' }], + ); + expect(records).toHaveLength(1); + expect(records[0].type).toBe('file'); + expect(records[0].content_type).toBe('application/pdf'); + expect(screenTextFileMock).toHaveBeenCalled(); + }); + + test('ignores an external-link paperclip (not uploads.linear.app)', async () => { + const records = await downloadScreenAndStoreLinearAttachments( + 'See the design.', 10, storageCtx(), + [{ title: 'Figma', url: 'https://figma.com/file/abc' }], + ); + expect(records).toHaveLength(0); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + test('de-dupes a file that is BOTH a paperclip and a description link', async () => { + (global.fetch as jest.Mock).mockImplementation(() => Promise.resolve(bytesResponse(PDF_BYTES, 200, 'application/pdf'))); + const url = 'https://uploads.linear.app/u/dup/same-uuid'; + const records = await downloadScreenAndStoreLinearAttachments( + labeledDesc('spec.pdf', url), 10, storageCtx(), + [{ title: 'spec.pdf', url }], + ); + expect(records).toHaveLength(1); // fetched once, not twice + }); + + test('sniffs JPEG when content-type is generic but bytes are a JPEG', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(JPEG_BYTES, 200, 'application/octet-stream')); + const records = await downloadScreenAndStoreLinearAttachments( + desc('https://uploads.linear.app/u/j/photo.jpg'), 10, storageCtx(), + ); + expect(records).toHaveLength(1); + expect(records[0].content_type).toBe('image/jpeg'); + }); + + test('a PDF served as text/plain is typed as PDF by magic bytes, NOT screened as raw text (finding #2)', async () => { + // Magic bytes are authoritative: %PDF- wins over a text/plain content-type, + // so it goes through PDF extraction (screenTextFile w/ application/pdf), never + // content.toString(utf-8) on binary bytes. + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(PDF_BYTES, 200, 'text/plain')); + const records = await downloadScreenAndStoreLinearAttachments( + labeledDesc('sneaky.txt', 'https://uploads.linear.app/u/p/sneaky-uuid'), 10, storageCtx(), + ); + expect(records).toHaveLength(1); + expect(records[0].content_type).toBe('application/pdf'); // typed by bytes, not the text/plain header + expect(screenTextFileMock).toHaveBeenCalledWith(expect.anything(), 'application/pdf', expect.anything(), expect.anything()); + }); + + test('SSRF: an IPv4-mapped IPv6 metadata address (::ffff:169.254.169.254) is rejected (finding #8)', async () => { + dnsLookupMock.mockResolvedValueOnce([{ address: '::ffff:169.254.169.254', family: 6 }]); + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()), + ).rejects.toBeInstanceOf(LinearAttachmentError); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + test('SSRF: an fe80::/10 link-local address beyond the fe80 prefix (fea9::1) is rejected (finding #8)', async () => { + dnsLookupMock.mockResolvedValueOnce([{ address: 'fea9::1', family: 6 }]); + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()), + ).rejects.toBeInstanceOf(LinearAttachmentError); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + test('screening block → LinearAttachmentError (fail-closed)', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(PNG_BYTES)); + screenImageMock.mockRejectedValueOnce(new AttachmentScreeningError('blocked: prompt attack')); + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()), + ).rejects.toBeInstanceOf(LinearAttachmentError); + expect(putSendMock).not.toHaveBeenCalled(); + }); + + test('screening returns blocked status → LinearAttachmentError', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(PNG_BYTES)); + screenImageMock.mockResolvedValueOnce({ + content: PNG_BYTES, + checksum: 'sha256:abc', + screening: { status: 'blocked', categories: ['VIOLENCE'], screened_at: '2026-07-22T00:00:00Z' }, + }); + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()), + ).rejects.toBeInstanceOf(LinearAttachmentError); + }); + + test('SSRF: host resolving to a private IP → LinearAttachmentError, no fetch', async () => { + dnsLookupMock.mockResolvedValueOnce([{ address: '10.0.0.5', family: 4 }]); + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()), + ).rejects.toBeInstanceOf(LinearAttachmentError); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + test('body exceeding size limit while streaming → LinearAttachmentError', async () => { + // Stream a body larger than the cap in one chunk. + const tooBig = Buffer.concat([PNG_BYTES, Buffer.alloc(MAX_ATTACHMENT_SIZE_BYTES + 1)]); + (global.fetch as jest.Mock).mockResolvedValueOnce(bytesResponse(tooBig)); + await expect( + downloadScreenAndStoreLinearAttachments(desc(UPLOAD_URL), 10, storageCtx()), + ).rejects.toBeInstanceOf(LinearAttachmentError); + }); + + test('deletes already-uploaded objects when a later attachment fails (no orphans)', async () => { + const deleteSendMock = putSendMock; // same client.send + (global.fetch as jest.Mock) + .mockResolvedValueOnce(bytesResponse(PNG_BYTES)) // #1 ok + .mockResolvedValueOnce(bytesResponse(Buffer.alloc(0), 401)); // #2 auth-fail → throw + await expect( + downloadScreenAndStoreLinearAttachments( + desc('https://uploads.linear.app/u/1/a.png', 'https://uploads.linear.app/u/2/b.png'), + 10, storageCtx(), + ), + ).rejects.toBeInstanceOf(LinearAttachmentError); + // A DeleteObjectsCommand was sent to clean up the one uploaded object. + const sentDeletes = deleteSendMock.mock.calls.filter( + (c) => c[0]?.constructor?.name === 'DeleteObjectsCommand', + ); + expect(sentDeletes.length).toBeGreaterThanOrEqual(1); + }); +}); diff --git a/cdk/test/handlers/shared/linear-oauth-resolver.test.ts b/cdk/test/handlers/shared/linear-oauth-resolver.test.ts index 34fe749a2..768369d19 100644 --- a/cdk/test/handlers/shared/linear-oauth-resolver.test.ts +++ b/cdk/test/handlers/shared/linear-oauth-resolver.test.ts @@ -21,6 +21,7 @@ import { _resetCachesForTesting, invalidateLinearOauthCache, isTokenExpiring, + markWorkspaceRevoked, resolveLinearOauthToken, type StoredOauthToken, } from '../../../src/handlers/shared/linear-oauth-resolver'; @@ -356,6 +357,87 @@ describe('resolveLinearOauthToken', () => { expect(fetchImpl).toHaveBeenCalledTimes(1); }); + test('a permanently-rejected refresh records the revocation when a recorder is SUPPLIED', async () => { + // The failure this guards: when the authorization dies, every event for the + // workspace is dropped and the ONLY evidence was a log line, so an operator + // saw their trigger label do nothing with no way to find out why. Marking + // the row is what makes `bgagent platform doctor` able to say so. + // + // The recorder is OPT-IN: every Lambda that resolves a token holds read-only + // registry access, so defaulting it on meant the write failed AccessDenied + // and was swallowed on every revoked refresh — inert while reading as + // working. This passes it explicitly, the way a caller with the grant would. + const expiringSoon = new Date(Date.now() + 10 * 1000).toISOString(); + const stale = makeStoredToken({ refresh_token: 'rt-dead', expires_at: expiringSoon }); + + const smSend = jest.fn().mockImplementation((command: { constructor: { name: string } }) => { + if (command.constructor.name === 'GetSecretValueCommand') { + return { SecretString: JSON.stringify(stale) }; + } + return {}; + }); + const ddbSend = jest.fn().mockImplementation((command: { constructor: { name: string } }) => { + if (command.constructor.name === 'UpdateCommand') return {}; + return { Item: { workspace_slug: 'acme', oauth_secret_arn: 'arn:secret:acme', status: 'active' } }; + }); + const fetchImpl = jest.fn().mockResolvedValueOnce({ + ok: false, status: 400, json: async () => ({ error: 'invalid_grant' }), + }); + + type Opts = NonNullable[2]>; + const ddbClient = { send: ddbSend } as unknown as Opts['dynamoDbClient']; + const result = await resolveLinearOauthToken('ws-uuid-revoke', REGISTRY_TABLE, { + dynamoDbClient: ddbClient, + secretsManagerClient: { send: smSend } as unknown as Opts['secretsManagerClient'], + fetchImpl: fetchImpl as unknown as typeof fetch, + onAuthorizationRevoked: (workspaceId) => markWorkspaceRevoked( + ddbClient as never, REGISTRY_TABLE, workspaceId, + ), + }); + expect(result).toBeNull(); + + const update = ddbSend.mock.calls + .map((c) => c[0] as { constructor: { name: string }; input?: Record }) + .find((c) => c.constructor.name === 'UpdateCommand'); + expect(update).toBeDefined(); + const input = update!.input as { + ExpressionAttributeValues: Record; + ConditionExpression: string; + }; + expect(input.ExpressionAttributeValues[':revoked']).toBe('revoked'); + expect(input.ExpressionAttributeValues[':reason']).toBe('refresh_token_rejected'); + // Conditional on still being active, so a late straggler can't clobber a + // workspace an operator has already re-authorized. + expect(input.ConditionExpression).toContain(':active'); + }); + + test('a marker write failure does NOT break token resolution', async () => { + // Recording the diagnosis is strictly a bonus; if the registry write fails + // the caller must still get its clean null rather than a thrown handler. + const expiringSoon = new Date(Date.now() + 10 * 1000).toISOString(); + const stale = makeStoredToken({ refresh_token: 'rt-dead2', expires_at: expiringSoon }); + const smSend = jest.fn().mockImplementation((command: { constructor: { name: string } }) => { + if (command.constructor.name === 'GetSecretValueCommand') { + return { SecretString: JSON.stringify(stale) }; + } + return {}; + }); + const ddbSend = jest.fn().mockImplementation((command: { constructor: { name: string } }) => { + if (command.constructor.name === 'UpdateCommand') throw new Error('AccessDenied'); + return { Item: { workspace_slug: 'acme', oauth_secret_arn: 'arn:secret:acme', status: 'active' } }; + }); + const fetchImpl = jest.fn().mockResolvedValueOnce({ + ok: false, status: 400, json: async () => ({ error: 'invalid_grant' }), + }); + + type Opts = NonNullable[2]>; + await expect(resolveLinearOauthToken('ws-uuid-revoke-2', REGISTRY_TABLE, { + dynamoDbClient: { send: ddbSend } as unknown as Opts['dynamoDbClient'], + secretsManagerClient: { send: smSend } as unknown as Opts['secretsManagerClient'], + fetchImpl: fetchImpl as unknown as typeof fetch, + })).resolves.toBeNull(); + }); + test('cache invalidation on network failure: next call re-reads SM instead of looping on stale token', async () => { const expiringSoon = new Date(Date.now() + 10 * 1000).toISOString(); const stale = makeStoredToken({ expires_at: expiringSoon }); @@ -404,3 +486,130 @@ describe('resolveLinearOauthToken', () => { expect(getSecretCalls.length).toBeGreaterThanOrEqual(2); }); }); + +describe('markWorkspaceRevoked — the verdict must not outlive the grant it judged', () => { + const WS = 'ws-uuid-1'; + const INSTALLED = '2026-07-22T10:00:00.000Z'; + + /** The condition + values of the Nth Update. */ + function updateOf(send: jest.Mock, call = 0): { condition: string; values: Record } { + const input = (send.mock.calls[call][0] as { + input: { ConditionExpression: string; ExpressionAttributeValues: Record }; + }).input; + return { condition: input.ConditionExpression, values: input.ExpressionAttributeValues }; + } + + beforeEach(() => _resetCachesForTesting()); + + test('scopes the write to the installation it diagnosed, not merely to "active"', async () => { + // status = active alone is not enough: a re-authorization writes active + // again, so a straggler holding the OLD token would satisfy that condition + // and revoke the working grant the operator just installed. + const send = jest.fn().mockResolvedValue({}); + await markWorkspaceRevoked({ send } as never, REGISTRY_TABLE, WS, INSTALLED); + + const { condition, values } = updateOf(send); + expect(condition).toContain('installed_at = :installed'); + expect(values[':installed']).toBe(INSTALLED); + expect(condition).toContain('#s = :active'); + }); + + test('a row re-authorized since the diagnosis is left alone, and says so', async () => { + const conditional = new Error('The conditional request failed'); + (conditional as { name?: string }).name = 'ConditionalCheckFailedException'; + const send = jest.fn().mockRejectedValue(conditional); + + // Never throws — recording a diagnosis must not break token resolution. + await expect(markWorkspaceRevoked({ send } as never, REGISTRY_TABLE, WS, INSTALLED)) + .resolves.toBeUndefined(); + }); + + test('with no installation to name, it requires the attribute to still be ABSENT', async () => { + // A row predating installed_at: a re-authorization adds the attribute, so + // requiring its absence likewise takes the row out of scope. + const send = jest.fn().mockResolvedValue({}); + await markWorkspaceRevoked({ send } as never, REGISTRY_TABLE, WS); + + const { condition, values } = updateOf(send); + expect(condition).toContain('attribute_not_exists(installed_at)'); + expect(values).not.toHaveProperty(':installed'); + }); + + test('a non-conditional failure propagates — a silent AccessDenied is what kept this dormant', async () => { + const send = jest.fn().mockRejectedValue(new Error('AccessDeniedException')); + await expect(markWorkspaceRevoked({ send } as never, REGISTRY_TABLE, WS, INSTALLED)) + .rejects.toThrow('AccessDenied'); + }); + + test('the resolver writes NOTHING to the registry when no recorder is supplied', async () => { + // Guards the inert-but-looks-working state: the resolver used to default the + // marker on, so on every revoked refresh it issued a registry write that the + // caller's read-only role rejected, and the AccessDenied was swallowed. No + // stack in the arc grants registry write, so the correct behaviour with no + // recorder is to not attempt the write at all. + const stored = makeStoredToken({ expires_at: new Date(Date.now() + 60 * 1000).toISOString() }); + const clients = makeFakeClients({ + registryItem: { + workspace_slug: 'acme', + oauth_secret_arn: 'arn:secret:acme', + status: 'active', + installed_at: INSTALLED, + } as never, + storedToken: stored, + }); + const fetchImpl = jest.fn().mockResolvedValue({ + ok: false, + status: 400, + json: async () => ({ error: 'invalid_grant', error_description: 'refresh token revoked' }), + }); + + // No onAuthorizationRevoked supplied → no registry write is attempted. + await resolveLinearOauthToken(WS, REGISTRY_TABLE, { + ...clients, + fetchImpl: fetchImpl as unknown as typeof fetch, + }); + + const updates = clients.ddbSend.mock.calls + .map((c) => c[0] as { constructor: { name: string }; input?: Record }) + .filter((cmd) => cmd.constructor.name === 'UpdateCommand'); + expect(updates).toHaveLength(0); + }); + + test('a SUPPLIED recorder carries the installation from the row the resolver read', async () => { + // The value must come from the read that drove this refresh attempt. Re-reading + // it inside the recorder would race a concurrent re-authorization exactly as + // the status-only condition did, so this asserts the wiring, not just that + // some marker fired. + const stored = makeStoredToken({ expires_at: new Date(Date.now() + 60 * 1000).toISOString() }); + const clients = makeFakeClients({ + registryItem: { + workspace_slug: 'acme', + oauth_secret_arn: 'arn:secret:acme', + status: 'active', + installed_at: INSTALLED, + } as never, + storedToken: stored, + }); + const fetchImpl = jest.fn().mockResolvedValue({ + ok: false, + status: 400, + json: async () => ({ error: 'invalid_grant', error_description: 'refresh token revoked' }), + }); + + await resolveLinearOauthToken(WS, REGISTRY_TABLE, { + ...clients, + fetchImpl: fetchImpl as unknown as typeof fetch, + onAuthorizationRevoked: (workspaceId) => markWorkspaceRevoked( + clients.dynamoDbClient as never, REGISTRY_TABLE, workspaceId, INSTALLED, + ), + }); + + const updates = clients.ddbSend.mock.calls + .map((c) => c[0] as { constructor: { name: string }; input?: Record }) + .filter((cmd) => cmd.constructor.name === 'UpdateCommand'); + expect(updates).toHaveLength(1); + expect(updates[0].input!.ConditionExpression).toContain('installed_at = :installed'); + expect((updates[0].input!.ExpressionAttributeValues as Record)[':installed']) + .toBe(INSTALLED); + }); +}); diff --git a/cdk/test/handlers/shared/screenshot-url.test.ts b/cdk/test/handlers/shared/screenshot-url.test.ts index a02d4f1dc..3e6461423 100644 --- a/cdk/test/handlers/shared/screenshot-url.test.ts +++ b/cdk/test/handlers/shared/screenshot-url.test.ts @@ -17,7 +17,28 @@ * SOFTWARE. */ -import { buildScreenshotKey, encodeMarkdownUrl, isAllowedScreenshotUrl } from '../../../src/handlers/shared/screenshot-url'; +import { buildScreenshotKey, encodeMarkdownUrl, extractTaskIdFromBranch, isAllowedScreenshotUrl } from '../../../src/handlers/shared/screenshot-url'; + +describe('extractTaskIdFromBranch (#247 — screenshot → parent panel)', () => { + test('pulls the taskId from a standard ABCA branch (2nd segment)', () => { + expect(extractTaskIdFromBranch('bgagent/01TASKID123/abca-300-book-with-points')) + .toBe('01TASKID123'); + }); + test('tolerates extra trailing segments (taskId is always 2nd)', () => { + expect(extractTaskIdFromBranch('bgagent/01TASKID123/abca-300/extra')).toBe('01TASKID123'); + }); + test('null for a non-ABCA branch (human / fork default / too few segments)', () => { + expect(extractTaskIdFromBranch('main')).toBeNull(); + expect(extractTaskIdFromBranch('feature/foo')).toBeNull(); + expect(extractTaskIdFromBranch('bgagent')).toBeNull(); + expect(extractTaskIdFromBranch('bgagent//slug')).toBeNull(); // empty taskId + }); + test('null for empty / nullish', () => { + expect(extractTaskIdFromBranch('')).toBeNull(); + expect(extractTaskIdFromBranch(undefined)).toBeNull(); + expect(extractTaskIdFromBranch(null)).toBeNull(); + }); +}); describe('buildScreenshotKey', () => { test('produces a screenshots/_/--.png shape', () => { diff --git a/cdk/test/handlers/shared/validation.test.ts b/cdk/test/handlers/shared/validation.test.ts index fc4bd8952..2b82b6b96 100644 --- a/cdk/test/handlers/shared/validation.test.ts +++ b/cdk/test/handlers/shared/validation.test.ts @@ -35,6 +35,9 @@ import { parseBody, parseLimit, parseStatusFilter, + EXTENSION_TO_MIME, + MIME_TO_EXTENSION, + SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL, validateAttachments, validateMagicBytes, validateMaxBudgetUsd, @@ -699,16 +702,27 @@ describe('validateMagicBytes', () => { expect(validateMagicBytes(binary, 'text/plain')).toBe(false); }); - test('rejects a text type whose bytes are not decodable UTF-8', () => { - // 0xC3 starts a 2-byte sequence but 0x28 is not a valid continuation byte, and - // 0xFF never appears in UTF-8 at all. Neither carries a NUL, so the null-byte - // scan below would pass them — only the decode catches this. - expect(validateMagicBytes(Buffer.from([0x48, 0xC3, 0x28]), 'text/plain')).toBe(false); - expect(validateMagicBytes(Buffer.from([0xFF, 0xFE, 0x41]), 'application/json')).toBe(false); + test('rejects INVALID UTF-8 masquerading as text (finding #3)', () => { + // A lone 0xFF / 0x80 continuation byte is not valid UTF-8 — the old null-only + // check let these through; the real decoder now rejects them. + expect(validateMagicBytes(Buffer.from([0xff, 0xfe, 0x80, 0x81]), 'text/plain')).toBe(false); + expect(validateMagicBytes(Buffer.from([0xc3, 0x28]), 'application/json')).toBe(false); // invalid 2-byte seq + }); + + test('accepts valid multi-byte UTF-8 text (emoji / accents)', () => { + expect(validateMagicBytes(Buffer.from('café — 日本語 ✅', 'utf-8'), 'text/markdown')).toBe(true); + }); + + test('does not false-reject a multi-byte char that straddles the old 8 KB cutoff', () => { + // 8191 ASCII bytes + a 3-byte char crossing what used to be the 8192-byte + // cutoff. Decoding the whole buffer sees the complete character, so it must + // pass; a prefix-only decode would have cut it in half and rejected it. + const buf = Buffer.concat([Buffer.alloc(8191, 0x61), Buffer.from('本', 'utf-8')]); + expect(validateMagicBytes(buf, 'text/plain')).toBe(true); }); test('a long ASCII preamble does not launder trailing binary', () => { - // The check used to look at only the first 8 KB, so a benign preamble longer + // The check once looked at only the first 8 KB, so a benign preamble longer // than that let arbitrary bytes through behind it. Validate the whole buffer. const payload = Buffer.concat([ Buffer.from('A'.repeat(9000)), @@ -717,11 +731,6 @@ describe('validateMagicBytes', () => { expect(validateMagicBytes(payload, 'text/plain')).toBe(false); }); - test('accepts multi-byte UTF-8 text (not just ASCII)', () => { - // The decode must not reject legitimate non-ASCII text. - expect(validateMagicBytes(Buffer.from('héllo — 世界 🎉', 'utf8'), 'text/plain')).toBe(true); - }); - test('rejects mismatched signatures', () => { const jpeg = Buffer.from([0xFF, 0xD8, 0xFF, 0xE0]); expect(validateMagicBytes(jpeg, 'image/png')).toBe(false); @@ -812,3 +821,36 @@ describe('createAttachmentRecord', () => { expect(record.token_estimate).toBeUndefined(); }); }); + +describe('attachment type maps (derived, single source of truth)', () => { + test('EXTENSION_TO_MIME is the reverse of the allowlist + covers the .jpeg alias', () => { + expect(EXTENSION_TO_MIME.pdf).toBe('application/pdf'); + expect(EXTENSION_TO_MIME.png).toBe('image/png'); + expect(EXTENSION_TO_MIME.jpg).toBe('image/jpeg'); + expect(EXTENSION_TO_MIME.jpeg).toBe('image/jpeg'); // alias + expect(EXTENSION_TO_MIME.log).toBe('text/x-log'); + // Unsupported extensions resolve to nothing. + expect(EXTENSION_TO_MIME.docx).toBeUndefined(); + expect(EXTENSION_TO_MIME.zip).toBeUndefined(); + }); + + test('SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL is a human list derived from the allowlist (incl. JPEG alias)', () => { + const label = SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL; + // JPEG alias is listed (design nit): a user with a .jpeg file sees it's supported. + for (const ext of ['PNG', 'JPG', 'JPEG', 'TXT', 'CSV', 'MD', 'JSON', 'PDF', 'LOG']) { + expect(label).toContain(ext); + } + // Derived + upper-cased, comma-separated, no unsupported types leak in. + expect(label).not.toMatch(/docx|zip/i); + }); + + test('every MIME in MIME_TO_EXTENSION is actually allowed by isAllowedMimeType (design concern)', () => { + // Guards against the allowlist + the extension map drifting: the rejection + // message (derived from the map) must never advertise a type that + // isAllowedMimeType still rejects. + for (const mime of Object.keys(MIME_TO_EXTENSION)) { + const kind = mime.startsWith('image/') ? 'image' : 'file'; + expect(isAllowedMimeType(mime, kind)).toBe(true); + } + }); +}); diff --git a/cli/src/commands/linear.ts b/cli/src/commands/linear.ts index aac4fe0a6..57e4d0ac3 100644 --- a/cli/src/commands/linear.ts +++ b/cli/src/commands/linear.ts @@ -113,12 +113,18 @@ export function renderLinearAppTemplate(opts: LinearAppTemplateOptions = {}): st '', 'Click Save, copy the Client ID and Client Secret, then return here.', '', - 'Why these specific fields:', - ' • GitHub username with [bot] suffix gates the actor=app agent flow.', - ' Without it, Linear surfaces a misleading "Invalid redirect_uri" error.', + 'Non-obvious gotchas (Linear explains the fields themselves inline):', + ' • GitHub username is REQUIRED for actor=app — leaving it blank surfaces a', + ' misleading "Invalid redirect_uri" error, not a "missing username" one.', ' • Webhooks toggle must be ON for the same reason; the URL value is unused', ' by the OAuth dance and can be a placeholder.', ' • Wildcard callback URLs are not accepted by Linear; list each URL fully.', + ' • Do NOT enable Linear "agent" / app-notification events on this app. ABCA', + ' is a COMMENT-based integration (it replies + reacts on ordinary comments).', + ' With agent events on, Linear renders an @mention of the app as its', + ' interactive agent-activity surface instead of a comment thread, which', + ' breaks the reply/reaction UX. Leave agent/app events OFF; the trigger comes', + ' from the workspace webhook (Issues + Comments), configured separately next.', bar, ].join('\n'); } @@ -373,7 +379,9 @@ export function makeLinearCommand(): Command { console.log('In Linear → Settings → API → Webhooks → + New webhook, paste:'); console.log(); console.log(` URL: ${webhookUrl}`); - console.log(' Resource types: Issues'); + console.log(' Resource types: Issues, Comments'); + console.log(' (Issues = label-triggered tasks + epic orchestration;'); + console.log(' Comments = @bgagent re-iteration on a sub-issue PR)'); console.log(' Team: (whichever team owns the projects you map)'); console.log(); console.log('Save, then open the webhook detail page and copy the signing secret'); @@ -446,6 +454,7 @@ export function makeLinearCommand(): Command { .option('--client-secret ', 'Linear OAuth app Client Secret (else prompted; prefer interactive)') .option('--no-browser', 'Print the authorization URL instead of opening a browser (for SSH/headless)') .option('--no-actor-app', 'Drop actor=app from the OAuth flow (diagnostic: isolates whether agent-install is blocking)') + .option('--no-force-consent', 'Omit prompt=consent (diagnostic: restores the pre-fix behaviour that dead-ends on an already-installed app)') .action(async (slug: string, opts) => { if (!SLUG_RE.test(slug)) { throw new CliError( @@ -524,6 +533,11 @@ export function makeLinearCommand(): Command { state, codeChallenge: pkce.codeChallenge, actorApp: useActorApp, + // Always force the consent screen. A FRESH install shows it anyway, so + // this only changes the already-installed case — which without it + // dead-ends on "already installed" and returns no code, making a + // revoked-but-installed workspace unrecoverable by this command. + forceConsent: opts.forceConsent !== false, }); if (!useActorApp) { console.log(' ⚠ --no-actor-app: dropping actor=app for diagnosis. Token will not be agent-scoped.'); @@ -610,13 +624,14 @@ export function makeLinearCommand(): Command { // first) — that silently breaks signature verification (401 "Invalid // signature") for every workspace after the first in a multi-workspace // deployment. Rotation stays the job of `update-webhook-secret`. - // Fail CLOSED (#612 review B1): read this workspace's existing signing - // secret before the overwrite. Only ResourceNotFoundException is a clean - // first-install; any other SM error (AccessDenied, KMSAccessDenied, + // Fail CLOSED: read this workspace's existing signing secret before the + // overwrite. Only ResourceNotFoundException is a clean first-install; + // any other Secrets Manager error (AccessDenied, KMSAccessDenied, // Throttling, network) — or a corrupt bundle — must surface, NOT default // to undefined (which would mirror the stack-wide secret over a working - // per-workspace one → the #611 401 clobber, silently, behind a green - // "Setup complete"). Extracted to readExistingWebhookSecret + unit-tested. + // per-workspace one, producing a silent 401 on every webhook delivery + // behind a green "Setup complete"). Extracted to + // readExistingWebhookSecret + unit-tested. let existingWebhookSecret: string | undefined; try { existingWebhookSecret = await readExistingWebhookSecret( @@ -653,8 +668,8 @@ export function makeLinearCommand(): Command { installed_at: now, updated_at: now, installed_by_platform_user_id: cognitoSub, - // Fold the preserved secret into the INITIAL bundle (#612 review N2): - // the OAuth-secret write below then lands the correct bundle in ONE + // Fold the preserved secret into the INITIAL bundle so that + // the OAuth-secret write below lands the correct bundle in ONE // PutSecretValue, and the preserve path skips the later re-write. This // also closes a narrow window — if any fallible step between the two // writes threw, the bundle was left persisted WITHOUT the secret (401 @@ -676,8 +691,8 @@ export function makeLinearCommand(): Command { const ddb = DynamoDBDocumentClient.from(new DynamoDBClient({ region })); // Best-effort: fetch team keys so the screenshot processor can - // prefix-route Linear issue lookups (e.g. ABCA-42 → workspace - // owning ABCA) instead of scanning every active workspace. + // 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 ${tokenResponse.access_token}`); await ddb.send(new PutCommand({ TableName: workspaceRegistryTable!, @@ -728,9 +743,10 @@ export function makeLinearCommand(): Command { // • preserve — this workspace ALREADY has its own `lin_wh_` // secret (a re-run). Keep it; do NOT overwrite from // the stack-wide fallback, which is a DIFFERENT - // workspace's secret once >1 is installed. This is - // the #611 clobber fix — the stack-wide value is NOT - // necessarily this workspace's. + // workspace's secret once >1 is installed. The + // stack-wide value is NOT necessarily this + // workspace's, so overwriting from it breaks + // signature verification for this workspace. // • mirror-stackwide — no per-workspace secret yet but stack-wide is // set: mirror it (correct for the first/only // workspace; warn for an additional one). @@ -851,6 +867,7 @@ export function makeLinearCommand(): Command { .option('--stack-name ', 'CloudFormation stack name', 'backgroundagent-dev') .option('--no-browser', 'Print the authorization URL instead of opening a browser (for SSH/headless)') .option('--no-actor-app', 'Drop actor=app from the OAuth flow (diagnostic)') + .option('--no-force-consent', 'Omit prompt=consent (diagnostic)') .action(async (slug: string, opts) => { if (!SLUG_RE.test(slug)) { throw new CliError( @@ -948,6 +965,11 @@ export function makeLinearCommand(): Command { state, codeChallenge: pkce.codeChallenge, actorApp: useActorApp, + // Always force the consent screen. A FRESH install shows it anyway, so + // this only changes the already-installed case — which without it + // dead-ends on "already installed" and returns no code, making a + // revoked-but-installed workspace unrecoverable by this command. + forceConsent: opts.forceConsent !== false, }); if (!useActorApp) { console.log(' ⚠ --no-actor-app: dropping actor=app for diagnosis. Token will not be agent-scoped.'); diff --git a/cli/src/commands/platform.ts b/cli/src/commands/platform.ts index 7fe622eb3..eecd4e225 100644 --- a/cli/src/commands/platform.ts +++ b/cli/src/commands/platform.ts @@ -19,6 +19,7 @@ import { Command } from 'commander'; import { CliError } from '../errors'; +import { makeLinearRefreshVerifier } from '../linear-auth-health'; import { DEFAULT_STACK_NAME, resolveOperatorContext } from '../operator-context'; import { doctorChecksPassed, runPlatformDoctor } from '../platform-doctor'; import { listStackOutputs } from '../stack-outputs'; @@ -90,9 +91,19 @@ export function makePlatformCommand(): Command { .option('--region ', 'AWS region (defaults to configured region or AWS_REGION)') .option('--stack-name ', 'CloudFormation stack name', DEFAULT_STACK_NAME) .option('--output ', 'Output format: text or json', 'text') + .option( + '--verify-refresh', + 'Settle any INDETERMINATE Linear workspace by attempting its token refresh. ' + + 'Rotates and re-saves that workspace\'s token (what the platform does on every event), ' + + 'so it is safe but state-changing — hence opt-in.', + ) .action(async (opts) => { const { region, stackName } = resolveOperatorContext(opts); - const results = await runPlatformDoctor({ region, stackName }); + const results = await runPlatformDoctor({ + region, + stackName, + ...(opts.verifyRefresh && { linearVerifyRefresh: makeLinearRefreshVerifier(region) }), + }); const passed = doctorChecksPassed(results); if (opts.output === 'json') { diff --git a/cli/src/linear-auth-health.ts b/cli/src/linear-auth-health.ts new file mode 100644 index 000000000..2c84d8cc2 --- /dev/null +++ b/cli/src/linear-auth-health.ts @@ -0,0 +1,372 @@ +/** + * 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. + */ + +/** + * Is each onboarded Linear workspace's OAuth authorization still usable? + * + * Why this needs its own check: when a workspace's authorization dies, the + * platform goes SILENT rather than loud. The webhook processor can't resolve a + * token, logs "workspace not resolvable — dropping event", and returns — so a + * user applies the trigger label and gets nothing at all: no comment, no + * reaction, no state change. The only evidence is a CloudWatch line nobody is + * watching. Observed 2026-07-25: a workspace's authorization was revoked + * upstream and every event silently dropped for over an hour. + * + * The distinction this check exists to make: an EXPIRED access token is normal + * and self-healing (the resolver refreshes it on the next call), whereas a + * REVOKED authorization needs a human to re-authorize. Both look like "auth + * broken" from the outside, so reporting "expired" as a failure would cry wolf + * on every idle workspace, and reporting "revoked" as fine would hide a total + * outage. So we ask the surface, and only treat a rejected REFRESH as a failure. + * + * Read-only: this never consumes a refresh token (which would rotate it and + * disrupt the very thing being diagnosed). It probes with the stored access + * token, and only when that is rejected does it distinguish expiry from + * revocation using the locally-known expiry. + */ + +import { + GetSecretValueCommand, + PutSecretValueCommand, + SecretsManagerClient, +} from '@aws-sdk/client-secrets-manager'; +import { ScanCommand } from '@aws-sdk/lib-dynamodb'; +import { documentClient } from './dynamo-clients'; +import { verifyLinearRefreshAndPersist } from './linear-oauth'; + +/** Linear's GraphQL endpoint — a cheap authenticated probe target. */ +const LINEAR_GRAPHQL_ENDPOINT = 'https://api.linear.app/graphql'; + +/** Smallest authenticated query that proves a token is live. */ +const VIEWER_PROBE_QUERY = '{ viewer { id } }'; + +/** How each workspace's authorization looks right now. */ +export type LinearAuthState = + /** Access token accepted — the workspace is fully working. */ + | 'active' + /** + * INDETERMINATE, and deliberately never reported as healthy. The access token + * is rejected and expired and a refresh token is stored — which is BOTH the + * normal idle-workspace shape AND the exact shape of a workspace whose refresh + * token has been revoked (live-caught 2026-07-25: the access token had expired + * ~48 minutes earlier and the refresh token was dead, and this check reported + * it as fine). + * + * The shallow probe cannot separate those two, so it must not pretend to. Pass + * ``verifyRefresh`` to resolve it definitively — see + * {@link CheckLinearAuthOptions.verifyRefresh}. + */ + | 'expired_indeterminate' + /** + * Access token rejected while NOT expired, or no refresh token stored. The + * authorization itself is gone (revoked upstream, app uninstalled) — events + * are being dropped until someone re-authorizes. + */ + | 'revoked' + /** Registry row marked inactive by an operator — dropping events is intended. */ + | 'disabled' + /** Couldn't determine (secret unreadable, network failure). Not a verdict. */ + | 'unknown'; + +export interface LinearWorkspaceAuthHealth { + readonly workspaceId: string; + readonly workspaceSlug: string; + readonly state: LinearAuthState; + /** Operator-facing explanation, including the remedy when there is one. */ + readonly detail: string; +} + +/** Registry row fields this check reads. */ +interface RegistryRow { + readonly linear_workspace_id?: string; + readonly workspace_slug?: string; + readonly oauth_secret_arn?: string; + readonly status?: string; + /** Stamped by the platform when it detected the authorization was dead. */ + readonly revoked_at?: string; + readonly revoked_reason?: string; +} + +/** The stored-secret fields this check reads. Deliberately narrow: the token + * values are used only to make one probe call and are never returned. */ +interface StoredToken { + readonly access_token?: string; + readonly refresh_token?: string; + readonly expires_at?: string; + readonly workspace_slug?: string; +} + +/** Probe outcome, kept separate from the verdict so the mapping is testable. */ +export type ProbeResult = 'accepted' | 'rejected' | 'error'; + +/** + * Outcome of ATTEMPTING the refresh — the only way to settle + * ``expired_indeterminate``. + * + * ``refreshed`` means the grant is alive AND the rotated token was persisted. + * That persistence is not optional: Linear rotates the refresh token on every + * use, so a verifier that refreshes without saving the result destroys the + * workspace's token chain — which is exactly how an ad-hoc probe stranded a + * healthy workspace on 2026-07-25. A verifier that cannot persist must not + * refresh at all. + */ +export type RefreshVerifyResult = 'refreshed' | 'rejected' | 'error'; + +/** + * Attempt the refresh for one workspace and persist the rotation. Injected + * rather than implemented here so this module keeps doing one job (reporting), + * the destructive-if-done-wrong half is written once beside the other OAuth + * write paths, and tests can supply a fake. + */ +export type LinearRefreshVerifier = (workspace: { + readonly workspaceId: string; + readonly oauthSecretArn: string; +}) => Promise; + +/** Ask the surface whether an access token is still accepted. Injectable so + * tests don't reach the network. */ +export type LinearProbe = (accessToken: string) => Promise; + +/** Default probe: a minimal authenticated GraphQL query. */ +export const probeLinearAccessToken: LinearProbe = async (accessToken) => { + try { + const response = await fetch(LINEAR_GRAPHQL_ENDPOINT, { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'Authorization': `Bearer ${accessToken}` }, + body: JSON.stringify({ query: VIEWER_PROBE_QUERY }), + }); + if (response.status === 401 || response.status === 403) return 'rejected'; + if (!response.ok) return 'error'; + // Linear reports auth failures as 200 + an errors array as well as via 401, + // so a body check is needed — treating a 200 as proof would report a dead + // token as healthy. + const body = await response.json() as { data?: { viewer?: { id?: string } } }; + return body?.data?.viewer?.id ? 'accepted' : 'rejected'; + } catch { + return 'error'; + } +}; + +/** True when ``expiresAt`` is in the past (or unparseable, which we treat as + * expired — the resolver does the same, preferring an extra refresh). */ +export function isExpired(expiresAt: string | undefined, now: Date = new Date()): boolean { + if (!expiresAt) return true; + const ts = new Date(expiresAt).getTime(); + if (Number.isNaN(ts)) return true; + return now.getTime() >= ts; +} + +/** + * Map a probe result + stored-token facts to a verdict. Pure, so the + * expired-vs-revoked distinction — the whole point of the check — is testable + * without a network or a live workspace. + */ +export function classifyAuthState( + probe: ProbeResult, + token: { hasRefreshToken: boolean; expiresAt?: string }, + now: Date = new Date(), +): LinearAuthState { + if (probe === 'accepted') return 'active'; + if (probe === 'error') return 'unknown'; + // Rejected. An expired token with a refresh token in hand is the normal + // idle-workspace case and heals itself on the next resolve. + if (isExpired(token.expiresAt, now) && token.hasRefreshToken) return 'expired_indeterminate'; + return 'revoked'; +} + +export interface CheckLinearAuthOptions { + readonly region: string; + readonly registryTableName: string; + /** Injectable for tests. */ + readonly probe?: LinearProbe; + /** + * Supply this to RESOLVE ``expired_indeterminate`` instead of reporting it. + * Omitted by default because it performs a real token rotation: correct and + * non-destructive (it persists the new token, exactly as the platform's own + * refresh does), but a state-changing action an operator should opt into. + */ + readonly verifyRefresh?: LinearRefreshVerifier; + readonly now?: Date; +} + +/** + * Report the authorization health of every workspace in the registry. + * Never throws: a workspace that can't be assessed comes back ``unknown`` so + * one broken row doesn't hide the others. + */ +export async function checkLinearWorkspaceAuth( + options: CheckLinearAuthOptions, +): Promise { + const { region, registryTableName } = options; + const probe = options.probe ?? probeLinearAccessToken; + const now = options.now ?? new Date(); + + const ddb = documentClient(region); + // Paginate. A single Scan page stops at DynamoDB's 1MB limit, so a registry + // large enough to span pages would silently omit workspaces — and an omitted + // REVOKED workspace is the one case that must never be missed, because the + // report would then read as clean. Any read failure propagates to the caller, + // which reports it as a warn rather than a pass (a partial scan is not a + // clean bill of health). + const rows: RegistryRow[] = []; + let exclusiveStartKey: Record | undefined; + do { + const page = await ddb.send(new ScanCommand({ + TableName: registryTableName, + ...(exclusiveStartKey && { ExclusiveStartKey: exclusiveStartKey }), + })); + rows.push(...((page.Items ?? []) as RegistryRow[])); + exclusiveStartKey = page.LastEvaluatedKey; + } while (exclusiveStartKey); + + const sm = new SecretsManagerClient({ region }); + const out: LinearWorkspaceAuthHealth[] = []; + + for (const row of rows) { + const workspaceId = row.linear_workspace_id ?? '(unknown)'; + const slug = row.workspace_slug ?? workspaceId; + + if (row.status === 'revoked') { + // The platform itself recorded this when a refresh was rejected, which is + // the authoritative signal — more reliable than anything this check can + // infer from a token, and it carries when it happened. + out.push({ + workspaceId, + workspaceSlug: slug, + state: 'revoked', + detail: describeState('revoked', undefined, row.revoked_at), + }); + continue; + } + if (row.status && row.status !== 'active') { + out.push({ + workspaceId, + workspaceSlug: slug, + state: 'disabled', + detail: `Registry row status is '${row.status}' — events for this workspace are dropped by design.`, + }); + continue; + } + if (!row.oauth_secret_arn) { + out.push({ + workspaceId, + workspaceSlug: slug, + state: 'revoked', + detail: 'Registry row has no oauth_secret_arn. Re-run `bgagent linear setup` for this workspace.', + }); + continue; + } + + let stored: StoredToken | null = null; + try { + const secret = await sm.send(new GetSecretValueCommand({ SecretId: row.oauth_secret_arn })); + stored = secret.SecretString ? JSON.parse(secret.SecretString) as StoredToken : null; + } catch (err) { + out.push({ + workspaceId, + workspaceSlug: slug, + state: 'unknown', + detail: `Could not read the OAuth secret: ${err instanceof Error ? err.message : String(err)}`, + }); + continue; + } + + if (!stored?.access_token) { + out.push({ + workspaceId, + workspaceSlug: slug, + state: 'revoked', + detail: 'Stored OAuth secret has no access token. Re-run `bgagent linear setup` for this workspace.', + }); + continue; + } + + let state = classifyAuthState( + await probe(stored.access_token), + { hasRefreshToken: Boolean(stored.refresh_token), expiresAt: stored.expires_at }, + now, + ); + + // Resolve the one state the shallow probe cannot decide. Only attempted when + // the operator opted in, and only for that state — never for a workspace + // already known active or revoked, so a healthy grant is never rotated just + // to produce a report. + if (state === 'expired_indeterminate' && options.verifyRefresh) { + const verified = await options.verifyRefresh({ + workspaceId, + oauthSecretArn: row.oauth_secret_arn, + }); + // A verifier error leaves the state indeterminate rather than guessing: it + // must not turn a transient network failure into a "revoked" verdict that + // sends an operator to re-authorize a working workspace. + if (verified === 'refreshed') state = 'active'; + else if (verified === 'rejected') state = 'revoked'; + } + + out.push({ + workspaceId, + workspaceSlug: stored.workspace_slug ?? slug, + state, + detail: describeState(state, stored.expires_at), + }); + } + + return out; +} + +/** Operator-facing wording per state, remedy included where one applies. */ +function describeState(state: LinearAuthState, expiresAt?: string, revokedAt?: string): string { + switch (state) { + case 'active': + return `Authorization is live (access token valid until ${expiresAt ?? 'unknown'}).`; + case 'expired_indeterminate': + return 'INDETERMINATE — the access token has expired. This is what a healthy idle workspace looks ' + + 'like AND what a revoked one looks like; the two are indistinguishable without attempting the ' + + 'refresh. Re-run with `--verify-refresh` for a definitive answer (it performs the same refresh ' + + 'the platform would, and persists the rotated token).'; + case 'revoked': + return `Authorization was REVOKED${revokedAt ? ` at ${revokedAt}` : ''} — the platform cannot post ` + + 'to this workspace and is dropping its events. Re-authorize with `bgagent linear setup` ' + + '(or `bgagent linear add-workspace` for an additional workspace).'; + case 'disabled': + return 'Workspace is disabled in the registry.'; + case 'unknown': + default: + return 'Could not determine authorization state.'; + } +} + +/** + * The production {@link LinearRefreshVerifier}: binds Secrets Manager to + * {@link verifyLinearRefreshAndPersist}, which owns the refresh-and-save + * sequencing (and the rule that a rotation which wasn't persisted is reported as + * an error, never as health). + */ +export function makeLinearRefreshVerifier(region: string): LinearRefreshVerifier { + const sm = new SecretsManagerClient({ region }); + return async ({ oauthSecretArn }) => verifyLinearRefreshAndPersist({ + readSecret: async () => { + const res = await sm.send(new GetSecretValueCommand({ SecretId: oauthSecretArn })); + return res.SecretString; + }, + writeSecret: async (secretString) => { + await sm.send(new PutSecretValueCommand({ SecretId: oauthSecretArn, SecretString: secretString })); + }, + }); +} diff --git a/cli/src/linear-oauth.ts b/cli/src/linear-oauth.ts index 30a77c942..27906fba3 100644 --- a/cli/src/linear-oauth.ts +++ b/cli/src/linear-oauth.ts @@ -159,6 +159,14 @@ export function generatePkce(): { codeVerifier: string; codeChallenge: string } /** * Build the Linear authorization URL the CLI opens in the browser. * `actorApp: true` adds `actor=app` (the Agent install variant). + * + * ``forceConsent`` adds ``prompt=consent``, which Linear documents as showing + * the consent screen "every time, even if all scopes were previously granted". + * That is what makes RE-authorization possible: without it, an app already + * installed in the workspace short-circuits with "already installed" and never + * returns an authorization code — so the one command meant to recover a revoked + * authorization could not recover it (live-caught 2026-07-25, where a workspace's + * grant was revoked while the app install stayed active). */ export function buildAuthorizationUrl(opts: { clientId: string; @@ -167,6 +175,7 @@ export function buildAuthorizationUrl(opts: { codeChallenge: string; scopes?: readonly string[]; actorApp?: boolean; + forceConsent?: boolean; }): string { const params = new URLSearchParams({ client_id: opts.clientId, @@ -183,6 +192,9 @@ export function buildAuthorizationUrl(opts: { if (opts.actorApp ?? true) { params.set('actor', 'app'); } + if (opts.forceConsent) { + params.set('prompt', 'consent'); + } return `${LINEAR_AUTHORIZE_ENDPOINT}?${params.toString()}`; } @@ -378,3 +390,77 @@ export async function readExistingWebhookSecret( ? bundle.webhook_signing_secret : undefined; } + +/** + * Attempt a workspace's refresh and PERSIST the rotated token — the only way to + * settle "is this expired token's grant still alive?" definitively. + * + * Persistence is the whole safety property, not a convenience. Linear rotates the + * refresh token on every use, so a caller that refreshes and discards the result + * has silently consumed the workspace's only key: the stored token is now the + * spent one, and the workspace is stranded exactly as if it had been revoked. + * (That is not hypothetical — an ad-hoc probe did it to a healthy workspace on + * 2026-07-25 and it had to be repaired by hand.) So this function refreshes and + * writes in one step, and reports `error` rather than a verdict if the write + * fails, because a rotation that happened but wasn't saved is the dangerous case + * and must not be reported as health. + * + * `readSecret` / `writeSecret` are injected so this stays testable without + * Secrets Manager and so the caller owns client construction. + */ +export async function verifyLinearRefreshAndPersist(args: { + readSecret: () => Promise; + writeSecret: (secretString: string) => Promise; + fetchImpl?: typeof fetch; + now?: Date; +}): Promise<'refreshed' | 'rejected' | 'error'> { + let stored: StoredLinearOauthToken; + try { + const raw = await args.readSecret(); + if (!raw) return 'error'; + stored = JSON.parse(raw) as StoredLinearOauthToken; + } catch { + return 'error'; + } + if (!stored.refresh_token || !stored.client_id || !stored.client_secret) { + // Nothing to attempt: without these the grant cannot be renewed by anyone, + // which is a real dead end rather than an inconclusive probe. + return 'rejected'; + } + + let refreshed; + try { + refreshed = await refreshAccessToken({ + refreshToken: stored.refresh_token, + clientId: stored.client_id, + clientSecret: stored.client_secret, + ...(args.fetchImpl && { fetchImpl: args.fetchImpl }), + }); + } catch (err) { + // Only Linear's own `invalid_grant` proves the grant is dead. A network + // blip, a 5xx, or a malformed body must NOT be reported as revoked — that + // would send an operator to re-authorize a working workspace. + const message = err instanceof Error ? err.message : String(err); + return message.includes('invalid_grant') ? 'rejected' : 'error'; + } + + const now = args.now ?? new Date(); + const next: StoredLinearOauthToken = { + ...stored, + access_token: refreshed.access_token, + // Persist the ROTATED refresh token; re-using the old one always fails. + refresh_token: refreshed.refresh_token ?? stored.refresh_token, + expires_at: computeExpiresAt(refreshed.expires_in, now), + scope: refreshed.scope, + updated_at: now.toISOString(), + }; + try { + await args.writeSecret(JSON.stringify(next)); + } catch { + // The rotation HAPPENED but wasn't saved, so the stored token is now spent. + // Report `error`, never `refreshed`: the operator must see that this needs + // attention rather than a green tick over a workspace we just stranded. + return 'error'; + } + return 'refreshed'; +} diff --git a/cli/src/platform-doctor.ts b/cli/src/platform-doctor.ts index 2d9611601..f7fd80cd7 100644 --- a/cli/src/platform-doctor.ts +++ b/cli/src/platform-doctor.ts @@ -24,6 +24,7 @@ import { DescribeUserPoolCommand, } from '@aws-sdk/client-cognito-identity-provider'; import { isGithubTokenConfigured } from './github-token'; +import { checkLinearWorkspaceAuth, type LinearProbe, type LinearRefreshVerifier } from './linear-auth-health'; import { PLATFORM_REPO_DEFAULTS } from './repo-display'; import { countActiveRepos } from './repo-lookup'; import { getStackOutput } from './stack-outputs'; @@ -51,6 +52,14 @@ export interface DoctorCheckResult { export interface RunPlatformDoctorOptions { readonly region: string; readonly stackName: string; + /** Injectable Linear auth probe (tests supply a fake; production uses the default). */ + readonly linearProbe?: LinearProbe; + /** + * Opt-in resolver for the indeterminate auth state. Absent by default because + * it rotates a real token (safely — it persists the rotation), which an + * operator should choose rather than have a read-only-looking command do. + */ + readonly linearVerifyRefresh?: LinearRefreshVerifier; } /** Smoke-check deployed platform readiness (operator AWS credentials). */ @@ -64,12 +73,14 @@ export async function runPlatformDoctor( appClientId, githubTokenSecretArn, repoTableName, + linearRegistryTableName, ] = await Promise.all([ getStackOutput(region, stackName, 'ApiUrl'), getStackOutput(region, stackName, 'UserPoolId'), getStackOutput(region, stackName, 'AppClientId'), getStackOutput(region, stackName, 'GitHubTokenSecretArn'), getStackOutput(region, stackName, 'RepoTableName'), + getStackOutput(region, stackName, 'LinearWorkspaceRegistryTableName'), ]); const checks: DoctorCheckResult[] = []; @@ -79,6 +90,9 @@ export async function runPlatformDoctor( checks.push(await checkGithubToken(region, githubTokenSecretArn)); checks.push(await checkActiveRepos(region, repoTableName)); checks.push(await checkBedrockModel(region, DEFAULT_BEDROCK_MODEL_ID)); + checks.push(await checkLinearAuth( + region, linearRegistryTableName, options.linearProbe, options.linearVerifyRefresh, + )); return checks; } @@ -232,6 +246,90 @@ async function checkBedrockModel(region: string, modelId: string): Promise { + const id = 'linear_workspace_auth'; + const label = 'Linear workspace authorizations live'; + if (!registryTableName) { + // Linear is optional — a stack with no Linear integration is not broken. + return { id, label, status: 'pass', detail: 'No Linear workspace registry on this stack (integration not deployed).' }; + } + + try { + const health = await checkLinearWorkspaceAuth({ + region, + registryTableName, + ...(probe && { probe }), + ...(verifyRefresh && { verifyRefresh }), + }); + if (health.length === 0) { + return { id, label, status: 'pass', detail: 'No Linear workspaces onboarded yet.' }; + } + + const revoked = health.filter((w) => w.state === 'revoked'); + // Indeterminate is NOT healthy. It is the exact shape of the workspace whose + // authorization died on 2026-07-25 — expired access token, refresh token + // present but dead — and reporting it as a pass is how that outage stayed + // invisible for over an hour. It is a warn rather than a fail because the + // same shape is also a perfectly healthy idle workspace, and failing every + // quiet workspace would train operators to ignore the check. + const indeterminate = health.filter((w) => w.state === 'expired_indeterminate'); + const unknown = health.filter((w) => w.state === 'unknown'); + const summary = health + .map((w) => `${w.workspaceSlug}=${w.state}`) + .join(', '); + + if (revoked.length > 0) { + const remedies = revoked.map((w) => ` ${w.workspaceSlug}: ${w.detail}`).join('\n'); + return { + id, + label, + status: 'fail', + detail: `${revoked.length} of ${health.length} workspace(s) have a REVOKED authorization — their ` + + `Linear events are being dropped silently.\n${remedies}\n (${summary})`, + }; + } + if (indeterminate.length > 0) { + return { + id, + label, + status: 'warn', + detail: `${indeterminate.length} of ${health.length} workspace(s) could NOT be confirmed ` + + 'authorized — an expired access token looks identical whether the refresh token behind it ' + + 'is alive or revoked. Re-run `bgagent platform doctor --verify-refresh` to settle it ' + + `(that performs the refresh the platform would, and persists the rotated token).\n (${summary})`, + }; + } + if (unknown.length > 0) { + return { + id, + label, + status: 'warn', + detail: `Could not assess ${unknown.length} of ${health.length} workspace(s): ${summary}`, + }; + } + return { id, label, status: 'pass', detail: `${health.length} workspace(s) authorized: ${summary}` }; + } catch (err) { + return { + id, + label, + status: 'warn', + detail: `Could not read the Linear workspace registry: ${err instanceof Error ? err.message : String(err)}`, + }; + } +} + /** True when every check passed (warnings are acceptable). */ export function doctorChecksPassed(results: readonly DoctorCheckResult[]): boolean { return results.every((r) => r.status === 'pass' || r.status === 'warn'); diff --git a/cli/test/commands/linear.test.ts b/cli/test/commands/linear.test.ts index db0cf3de0..5bd40d878 100644 --- a/cli/test/commands/linear.test.ts +++ b/cli/test/commands/linear.test.ts @@ -162,6 +162,15 @@ describe('renderLinearAppTemplate', () => { expect(out).toContain('REQUIRED for actor=app'); }); + test('warns against enabling Linear agent / app-notification events (breaks comment-thread UX)', () => { + // ABCA is a comment-based integration; an OAuth app in Linear "agent" mode + // makes @mentions render as interactive agent activity instead of comment + // threads. The template must steer operators away from that toggle. + const out = renderLinearAppTemplate(); + expect(out.toLowerCase()).toContain('agent'); + expect(out).toMatch(/do not enable .*agent/i); + }); + test('defaults the callback URL to the localhost endpoint that setup listens on', () => { // Phase 2.0b-O2 (shipped) uses an ephemeral localhost server during // `bgagent linear setup`. Printing the right URL by default diff --git a/cli/test/linear-auth-health.test.ts b/cli/test/linear-auth-health.test.ts new file mode 100644 index 000000000..073b9dee0 --- /dev/null +++ b/cli/test/linear-auth-health.test.ts @@ -0,0 +1,216 @@ +/** + * 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 ddbSend = jest.fn(); +jest.mock('../src/dynamo-clients', () => ({ + documentClient: () => ({ send: (...a: unknown[]) => ddbSend(...a) }), +})); + +const smSend = jest.fn(); +jest.mock('@aws-sdk/client-secrets-manager', () => ({ + SecretsManagerClient: jest.fn(() => ({ send: (...a: unknown[]) => smSend(...a) })), + GetSecretValueCommand: jest.fn((input: unknown) => ({ _type: 'GetSecret', input })), + PutSecretValueCommand: jest.fn((input: unknown) => ({ _type: 'PutSecret', input })), +})); + +jest.mock('@aws-sdk/lib-dynamodb', () => ({ + ScanCommand: jest.fn((input: unknown) => ({ _type: 'Scan', input })), +})); + +import { checkLinearWorkspaceAuth, classifyAuthState, isExpired } from '../src/linear-auth-health'; + +const NOW = new Date('2026-07-25T13:00:00.000Z'); +const FUTURE = '2026-07-25T14:00:00.000Z'; +const PAST = '2026-07-25T12:00:00.000Z'; + +describe('isExpired', () => { + test('a future expiry is not expired', () => { + expect(isExpired(FUTURE, NOW)).toBe(false); + }); + + test('a past expiry is expired', () => { + expect(isExpired(PAST, NOW)).toBe(true); + }); + + test('absent or unparseable expiry counts as expired', () => { + // Matches the platform resolver: prefer an unnecessary refresh over + // assuming a token nobody can date is still good. + expect(isExpired(undefined, NOW)).toBe(true); + expect(isExpired('not-a-date', NOW)).toBe(true); + }); +}); + +describe('classifyAuthState — expired vs revoked is the point of this check', () => { + test('an accepted token is active', () => { + expect(classifyAuthState('accepted', { hasRefreshToken: true, expiresAt: FUTURE }, NOW)).toBe('active'); + }); + + test('rejected + already expired + a refresh token = INDETERMINATE, never healthy', () => { + // This shape is genuinely ambiguous: it is what a quiet-but-healthy workspace + // looks like AND what a revoked one looks like. The classifier must refuse to + // guess — failing every idle workspace would train operators to ignore the + // check, and passing them hides a real outage. + expect(classifyAuthState('rejected', { hasRefreshToken: true, expiresAt: PAST }, NOW)) + .toBe('expired_indeterminate'); + }); + + test('rejected while NOT expired = the authorization itself is gone', () => { + // A token that the surface refuses even though it should still be valid + // means the grant was revoked upstream — every event is being dropped. + expect(classifyAuthState('rejected', { hasRefreshToken: true, expiresAt: FUTURE }, NOW)) + .toBe('revoked'); + }); + + test('rejected with NO refresh token = revoked, since nothing can recover it', () => { + // Even if it merely expired, without a refresh token there is no path back + // without a human, so it must not be reported as self-healing. + expect(classifyAuthState('rejected', { hasRefreshToken: false, expiresAt: PAST }, NOW)) + .toBe('revoked'); + }); + + test('a probe that could not complete is unknown, never a verdict', () => { + // A network blip must not be reported as a revoked authorization — that + // would send an operator to re-authorize a healthy workspace. + expect(classifyAuthState('error', { hasRefreshToken: true, expiresAt: FUTURE }, NOW)).toBe('unknown'); + expect(classifyAuthState('error', { hasRefreshToken: false, expiresAt: PAST }, NOW)).toBe('unknown'); + }); + + test('the live 2026-07-25 incident is INDETERMINATE from the shallow probe alone', () => { + // The REAL shape, not a convenient variant: the maguireb workspace had a + // refresh token present and only ~25h old, and an access token that had + // expired ~48 minutes earlier. Linear rejected both, but the shallow probe + // only sees the access token, so it CANNOT tell this from a healthy idle + // workspace. Pinning it as `revoked` here (by passing hasRefreshToken:false) + // is what let the real bug hide: the check reported that workspace as needing + // no action while every one of its events was being dropped. The honest + // classification is indeterminate — and `verifyRefresh` is what settles it. + const liveIncident = { hasRefreshToken: true, expiresAt: '2026-07-25T12:12:48.012Z' }; + expect(classifyAuthState('rejected', liveIncident, NOW)).toBe('expired_indeterminate'); + // And critically: indeterminate must NOT be treated as healthy by the caller. + expect(classifyAuthState('rejected', liveIncident, NOW)).not.toBe('active'); + }); +}); + +describe('checkLinearWorkspaceAuth — the real function, end to end', () => { + /** The live-incident secret: refresh token present, access token long expired. */ + const INDETERMINATE_SECRET = JSON.stringify({ + access_token: 'lin_dead', + refresh_token: 'rt-present', + expires_at: '2026-07-25T12:12:48.012Z', + workspace_slug: 'maguireb', + client_id: 'cid', + client_secret: 'sec', + }); + + const row = (extra: Record = {}) => ({ + linear_workspace_id: 'ws-1', + workspace_slug: 'maguireb', + oauth_secret_arn: 'arn:secret:maguireb', + status: 'active', + ...extra, + }); + + beforeEach(() => { + jest.clearAllMocks(); + smSend.mockResolvedValue({ SecretString: INDETERMINATE_SECRET }); + }); + + test('WITHOUT a verifier the live-incident workspace is indeterminate, not active', async () => { + ddbSend.mockResolvedValue({ Items: [row()] }); + const health = await checkLinearWorkspaceAuth({ + region: 'us-east-1', + registryTableName: 'Reg', + probe: async () => 'rejected', + }); + expect(health).toHaveLength(1); + expect(health[0].state).toBe('expired_indeterminate'); + expect(health[0].detail).toMatch(/INDETERMINATE/); + }); + + test('WITH a verifier that rejects, the same workspace is reported REVOKED', async () => { + // This is the bug the review caught: previously this workspace read as + // "no action needed" while every one of its events was being dropped. + ddbSend.mockResolvedValue({ Items: [row()] }); + const health = await checkLinearWorkspaceAuth({ + region: 'us-east-1', + registryTableName: 'Reg', + probe: async () => 'rejected', + verifyRefresh: async () => 'rejected', + }); + expect(health[0].state).toBe('revoked'); + expect(health[0].detail).toMatch(/REVOKED/); + }); + + test('WITH a verifier that refreshes, it is reported active', async () => { + ddbSend.mockResolvedValue({ Items: [row()] }); + const health = await checkLinearWorkspaceAuth({ + region: 'us-east-1', + registryTableName: 'Reg', + probe: async () => 'rejected', + verifyRefresh: async () => 'refreshed', + }); + expect(health[0].state).toBe('active'); + }); + + test('a verifier ERROR leaves it indeterminate — never a revoked verdict', async () => { + ddbSend.mockResolvedValue({ Items: [row()] }); + const health = await checkLinearWorkspaceAuth({ + region: 'us-east-1', + registryTableName: 'Reg', + probe: async () => 'rejected', + verifyRefresh: async () => 'error', + }); + expect(health[0].state).toBe('expired_indeterminate'); + }); + + test('the verifier is NOT invoked for a workspace already known healthy', async () => { + // Rotating a live workspace's token merely to produce a report would be a + // side-effect nobody asked for. + ddbSend.mockResolvedValue({ Items: [row()] }); + const verify = jest.fn, unknown[]>().mockResolvedValue('refreshed'); + const health = await checkLinearWorkspaceAuth({ + region: 'us-east-1', + registryTableName: 'Reg', + probe: async () => 'accepted', + verifyRefresh: verify as never, + }); + expect(health[0].state).toBe('active'); + expect(verify).not.toHaveBeenCalled(); + }); + + test('the registry scan is PAGINATED — a revoked workspace on page 2 is still reported', async () => { + // Past DynamoDB's 1MB page a single Scan silently truncates, and an omitted + // revoked workspace would make the whole report read as clean. + ddbSend + .mockResolvedValueOnce({ Items: [row({ linear_workspace_id: 'ws-page1' })], LastEvaluatedKey: { k: 'next' } }) + .mockResolvedValueOnce({ Items: [row({ linear_workspace_id: 'ws-page2', status: 'revoked' })] }); + const health = await checkLinearWorkspaceAuth({ + region: 'us-east-1', + registryTableName: 'Reg', + probe: async () => 'accepted', + }); + // Asserted on the workspace id, not the slug: for a live workspace the slug + // is taken from the stored secret, so it would not distinguish the two rows. + expect(health.map((w) => w.workspaceId)).toEqual(['ws-page1', 'ws-page2']); + expect(health[1].state).toBe('revoked'); + // The second Scan must carry the continuation key. + expect((ddbSend.mock.calls[1][0] as { input: Record }).input.ExclusiveStartKey) + .toEqual({ k: 'next' }); + }); +}); diff --git a/cli/test/linear-oauth.test.ts b/cli/test/linear-oauth.test.ts index c3a682b3b..52780b702 100644 --- a/cli/test/linear-oauth.test.ts +++ b/cli/test/linear-oauth.test.ts @@ -31,6 +31,7 @@ import { readExistingWebhookSecret, refreshAccessToken, resolveWebhookSecretAction, + verifyLinearRefreshAndPersist, } from '../src/linear-oauth'; describe('linearOauthSecretName', () => { @@ -106,6 +107,46 @@ describe('buildAuthorizationUrl', () => { const parsed = new URL(url); expect(parsed.searchParams.has('actor')).toBe(false); }); + + test('forceConsent adds prompt=consent so an already-installed app can RE-authorize', () => { + // Without this, Linear short-circuits an installed app with "already + // installed" and returns no authorization code — so `linear setup`, the + // documented remedy for a revoked authorization, could not actually recover + // one (live-caught 2026-07-25). + const url = buildAuthorizationUrl({ + clientId: 'cid', + redirectUri: 'https://localhost:8443/oauth/callback', + state: 'state-uuid', + codeChallenge: 'challenge', + forceConsent: true, + }); + expect(new URL(url).searchParams.get('prompt')).toBe('consent'); + }); + + test('prompt is omitted unless asked, so the param set stays minimal by default', () => { + const url = buildAuthorizationUrl({ + clientId: 'cid', + redirectUri: 'https://localhost:8443/oauth/callback', + state: 'state-uuid', + codeChallenge: 'challenge', + }); + expect(new URL(url).searchParams.has('prompt')).toBe(false); + }); + + test('forceConsent composes with actor=app — the combination the install path needs', () => { + // The failing case is specifically an actor=app re-install, so both params + // must survive together. + const parsed = new URL(buildAuthorizationUrl({ + clientId: 'cid', + redirectUri: 'https://localhost:8443/oauth/callback', + state: 'state-uuid', + codeChallenge: 'challenge', + actorApp: true, + forceConsent: true, + })); + expect(parsed.searchParams.get('actor')).toBe('app'); + expect(parsed.searchParams.get('prompt')).toBe('consent'); + }); }); describe('isAccessTokenExpiring', () => { @@ -284,6 +325,121 @@ describe('refreshAccessToken', () => { }); }); +describe('verifyLinearRefreshAndPersist', () => { + // This is the only code that can settle "is this workspace's authorization + // actually alive?", and it does so DESTRUCTIVELY: Linear rotates the refresh + // token on every use, so an attempt that isn't persisted spends the stored + // token and strands the workspace. Both halves are pinned here. + + const STORED = JSON.stringify({ + access_token: 'lin_old', + refresh_token: 'lin_refresh_old', + client_id: 'cid', + client_secret: 'csec', + expires_at: '2026-07-25T12:00:00.000Z', + workspace_slug: 'acme', + }); + + const refreshOk = () => jest.fn().mockResolvedValueOnce(mockResponse(200, { + access_token: 'lin_new', + token_type: 'Bearer', + expires_in: 86400, + refresh_token: 'lin_refresh_rotated', + scope: 'read write', + })); + + test('a live grant is refreshed AND the rotated token is persisted', async () => { + const writeSecret = jest.fn().mockResolvedValue(undefined); + const result = await verifyLinearRefreshAndPersist({ + readSecret: async () => STORED, + writeSecret, + fetchImpl: refreshOk() as unknown as typeof fetch, + now: new Date('2026-07-26T10:00:00.000Z'), + }); + + expect(result).toBe('refreshed'); + const saved = JSON.parse(writeSecret.mock.calls[0][0]); + // The rotated refresh token is what makes the NEXT refresh possible; saving + // the old one back would work once and then fail forever. + expect(saved.refresh_token).toBe('lin_refresh_rotated'); + expect(saved.access_token).toBe('lin_new'); + expect(saved.expires_at).toBe('2026-07-27T10:00:00.000Z'); + // Fields it does not own are carried through untouched — the webhook secret + // and client credentials live in this same bundle. + expect(saved.client_secret).toBe('csec'); + expect(saved.workspace_slug).toBe('acme'); + }); + + test('a rotation that could not be saved reports error, never health', async () => { + // The token has already been spent at this point, so a green tick here would + // hide a workspace this very check just broke. + const result = await verifyLinearRefreshAndPersist({ + readSecret: async () => STORED, + writeSecret: async () => { throw new Error('AccessDeniedException on PutSecretValue'); }, + fetchImpl: refreshOk() as unknown as typeof fetch, + }); + expect(result).toBe('error'); + }); + + test("only Linear's invalid_grant is read as revoked", async () => { + const fetchImpl = jest.fn().mockResolvedValueOnce(mockResponse(400, { + error: 'invalid_grant', error_description: 'refresh token was revoked', + })); + const result = await verifyLinearRefreshAndPersist({ + readSecret: async () => STORED, + writeSecret: async () => { throw new Error('must not be called'); }, + fetchImpl: fetchImpl as unknown as typeof fetch, + }); + expect(result).toBe('rejected'); + }); + + test('a 5xx or network failure is error — NOT revoked', async () => { + // Reporting revoked here would send an operator to re-authorize a perfectly + // healthy workspace because Linear had a bad minute. + for (const failing of [ + jest.fn().mockResolvedValueOnce(mockResponse(503, { error: 'service_unavailable' })), + jest.fn().mockRejectedValueOnce(new Error('ECONNRESET')), + ]) { + const result = await verifyLinearRefreshAndPersist({ + readSecret: async () => STORED, + writeSecret: async () => { throw new Error('must not be called'); }, + fetchImpl: failing as unknown as typeof fetch, + }); + expect(result).toBe('error'); + } + }); + + test('a bundle with no refresh token is rejected without any network call', async () => { + // Nothing can renew this grant, which is a genuine dead end rather than an + // inconclusive probe. + const fetchImpl = jest.fn(); + const result = await verifyLinearRefreshAndPersist({ + readSecret: async () => JSON.stringify({ access_token: 'lin_old', client_id: 'cid', client_secret: 'csec' }), + writeSecret: async () => { throw new Error('must not be called'); }, + fetchImpl: fetchImpl as unknown as typeof fetch, + }); + expect(result).toBe('rejected'); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + + test('an unreadable or malformed secret is error, and nothing is spent', async () => { + const fetchImpl = jest.fn(); + for (const readSecret of [ + async () => { throw new Error('AccessDenied'); }, + async () => undefined, + async () => 'not json', + ]) { + const result = await verifyLinearRefreshAndPersist({ + readSecret: readSecret as () => Promise, + writeSecret: async () => { throw new Error('must not be called'); }, + fetchImpl: fetchImpl as unknown as typeof fetch, + }); + expect(result).toBe('error'); + } + expect(fetchImpl).not.toHaveBeenCalled(); + }); +}); + describe('resolveWebhookSecretAction', () => { it('PRESERVES an existing per-workspace secret over the stack-wide one (multi-workspace re-run — the bug)', () => { // The regression: re-running `setup` on an already-installed workspace must diff --git a/cli/test/platform-doctor.test.ts b/cli/test/platform-doctor.test.ts new file mode 100644 index 000000000..e13386765 --- /dev/null +++ b/cli/test/platform-doctor.test.ts @@ -0,0 +1,128 @@ +/** + * 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. + */ + +// Scope: how doctor TURNS workspace auth health into an operator verdict. That +// mapping is where a real outage hid — a workspace whose events were all being +// dropped was reported as needing no action — so each state gets a pinned +// verdict here. The individual health states are classified (and tested) in +// linear-auth-health; this file only covers the pass/warn/fail decision. + +const healthMock = jest.fn(); +jest.mock('../src/linear-auth-health', () => ({ + checkLinearWorkspaceAuth: (...args: unknown[]) => healthMock(...args), +})); + +// Only the Linear registry output is wanted; every other check short-circuits on +// a missing stack output rather than reaching AWS. +const stackOutputMock = jest.fn(); +jest.mock('../src/stack-outputs', () => ({ + getStackOutput: (...args: unknown[]) => stackOutputMock(...args), +})); + +jest.mock('@aws-sdk/client-bedrock', () => ({ + BedrockClient: jest.fn(() => ({ send: jest.fn().mockRejectedValue(new Error('not under test')) })), + GetFoundationModelCommand: jest.fn(), +})); + +import { runPlatformDoctor, type DoctorCheckResult } from '../src/platform-doctor'; + +const REGISTRY = 'LinearWorkspaceRegistry'; + +/** One health entry, defaulting to the shape the live incident had. */ +function workspace(state: string, slug = 'maguireb') { + return { workspaceId: `ws-${slug}`, workspaceSlug: slug, state, detail: `${state} detail` }; +} + +async function linearCheck(): Promise { + const checks = await runPlatformDoctor({ region: 'us-east-1', stackName: 'Abca' }); + const check = checks.find((c) => c.id === 'linear_workspace_auth'); + if (!check) throw new Error('doctor no longer reports a Linear auth check'); + return check; +} + +beforeEach(() => { + jest.clearAllMocks(); + stackOutputMock.mockImplementation(async (_region: string, _stack: string, output: string) => + (output === 'LinearWorkspaceRegistryTableName' ? REGISTRY : null)); +}); + +describe('doctor verdict for Linear workspace auth', () => { + test('an indeterminate workspace does NOT pass — it warns, with the remedy', async () => { + // The regression this pins: reporting indeterminate as a pass is how a fully + // broken workspace read as healthy while every event it produced was dropped. + healthMock.mockResolvedValue([workspace('expired_indeterminate')]); + + const check = await linearCheck(); + expect(check.status).toBe('warn'); + expect(check.status).not.toBe('pass'); + expect(check.detail).toContain('--verify-refresh'); + }); + + test('a revoked workspace fails, and the detail carries its re-authorize remedy', async () => { + healthMock.mockResolvedValue([workspace('revoked')]); + + const check = await linearCheck(); + expect(check.status).toBe('fail'); + expect(check.detail).toContain('revoked detail'); + }); + + test('revoked outranks indeterminate — the actionable outage is the headline', async () => { + healthMock.mockResolvedValue([workspace('expired_indeterminate', 'quiet'), workspace('revoked', 'dead')]); + + const check = await linearCheck(); + expect(check.status).toBe('fail'); + // Both still appear in the summary, so the warn isn't lost behind the fail. + expect(check.detail).toContain('quiet=expired_indeterminate'); + expect(check.detail).toContain('dead=revoked'); + }); + + test('all-active passes', async () => { + healthMock.mockResolvedValue([workspace('active')]); + expect((await linearCheck()).status).toBe('pass'); + }); + + test('an unassessable workspace warns rather than passing', async () => { + healthMock.mockResolvedValue([workspace('unknown')]); + expect((await linearCheck()).status).toBe('warn'); + }); + + test('a stack with no Linear registry passes — the integration is optional', async () => { + stackOutputMock.mockResolvedValue(null); + expect((await linearCheck()).status).toBe('pass'); + expect(healthMock).not.toHaveBeenCalled(); + }); + + test('a registry read failure warns — a partial answer is not a clean bill of health', async () => { + healthMock.mockRejectedValue(new Error('AccessDeniedException on Scan')); + const check = await linearCheck(); + expect(check.status).toBe('warn'); + expect(check.detail).toContain('AccessDeniedException'); + }); + + test('the refresh verifier reaches the health check only when the operator opts in', async () => { + healthMock.mockResolvedValue([workspace('active')]); + const verify = jest.fn(); + + await runPlatformDoctor({ region: 'us-east-1', stackName: 'Abca' }); + expect(healthMock.mock.calls[0][0]).not.toHaveProperty('verifyRefresh'); + + await runPlatformDoctor({ region: 'us-east-1', stackName: 'Abca', linearVerifyRefresh: verify as never }); + expect(healthMock.mock.calls[1][0]).toHaveProperty('verifyRefresh', verify); + }); +}); diff --git a/yarn.lock b/yarn.lock index f87bb4e67..6ea6dda69 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3006,13 +3006,6 @@ dependencies: undici-types "~7.18.0" -"@types/pdf-parse@^1.1.5": - version "1.1.5" - resolved "https://registry.yarnpkg.com/@types/pdf-parse/-/pdf-parse-1.1.5.tgz#a0959022604457169177622b512ed03b975f10e2" - integrity sha512-kBfrSXsloMnUJOKi25s3+hRmkycHfLK6A09eRGqF/N8BkQoPUmaCr+q8Cli5FnfohEz/rsv82zAiPz/LXtOGhA== - dependencies: - "@types/node" "*" - "@types/sax@^1.2.1": version "1.2.7" resolved "https://registry.yarnpkg.com/@types/sax/-/sax-1.2.7.tgz#ba5fe7df9aa9c89b6dff7688a19023dd2963091d" @@ -7222,7 +7215,7 @@ path-scurry@^1.11.1: lru-cache "^10.2.0" minipass "^5.0.0 || ^6.0.2 || ^7.0.0" -pdf-parse@^2.4.5: +pdf-parse@2.4.5: version "2.4.5" resolved "https://registry.yarnpkg.com/pdf-parse/-/pdf-parse-2.4.5.tgz#fcbf9774d985a7f573e899c22618ab53a508a9f3" integrity sha512-mHU89HGh7v+4u2ubfnevJ03lmPgQ5WU4CxAVmTSh/sxVTEDYd1er/dKS/A6vg77NX47KTEoihq8jZBLr8Cxuwg==