From 008d9bc0d3b68629e958a39674927e771225d255 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Mon, 27 Jul 2026 12:34:50 +0100 Subject: [PATCH 1/4] =?UTF-8?q?feat(carve=20S4):=20Linear=20issue-context?= =?UTF-8?q?=20surface=20=E2=80=94=20attachments,=20PDF/image=20screening,?= =?UTF-8?q?=20auth=20health?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring the Linear surface's issue-context and operator tooling onto main. All of this is reachable today through the existing Linear webhook path; none of it depends on the sub-issue orchestration arc, so it stands alone. - linear-attachments.ts / linear-issue-context-probe.ts: pull an issue's file attachments and recent comments into the task context, so the agent sees the screenshots and discussion a human would. - attachment-screening.ts: screen fetched attachments (images and PDFs) before they reach the model. This carries the pdf-parse v2 upgrade as one unit — the source uses the v2 `PDFParse` class, which requires pinning the dependency and dropping the hand-written module declaration that described the v1 function export. Splitting any of the three breaks the build, so they travel together. - error-classifier.ts: distinguish transient compute faults from user-visible build failures, and report whether work was preserved when a run does not produce a pull request. - screenshot-url.ts + github-screenshot-integration.ts: resolve screenshot URLs for the review path. - CLI: `linear-auth-health.ts` + `platform-doctor.ts` surface an expired or revoked Linear authorization as a diagnosable condition instead of silently dropped events; `linear.ts` / `platform.ts` expose it. The comments and test names in these files are rewritten to explain the what and why directly rather than pointing at internal tracker ids, and two CLI strings that printed an internal issue number to user-facing output are reworded. Stacked on the agent-runtime slice. Full CDK suite 2598 passing, CLI 675 passing, both type-checks clean. --- cdk/package.json | 3 +- .../github-screenshot-integration.ts | 57 +- .../linear-project-mapping-table.ts | 9 + .../handlers/shared/attachment-screening.ts | 92 ++- cdk/src/handlers/shared/error-classifier.ts | 79 +- cdk/src/handlers/shared/linear-attachments.ts | 678 ++++++++++++++++++ .../shared/linear-issue-context-probe.ts | 300 ++++++++ .../handlers/shared/linear-oauth-resolver.ts | 117 ++- cdk/src/handlers/shared/screenshot-url.ts | 20 + cdk/src/types/pdf-parse.d.ts | 9 - .../github-screenshot-integration.test.ts | 73 ++ .../shared/attachment-screening.test.ts | 106 ++- .../handlers/shared/error-classifier.test.ts | 87 ++- .../shared/linear-attachments.test.ts | 466 ++++++++++++ .../shared/linear-oauth-resolver.test.ts | 164 +++++ .../handlers/shared/screenshot-url.test.ts | 23 +- cdk/test/handlers/shared/validation.test.ts | 66 +- cli/src/commands/linear.ts | 90 ++- cli/src/commands/platform.ts | 13 +- cli/src/linear-auth-health.ts | 372 ++++++++++ cli/src/linear-oauth.ts | 86 +++ cli/src/platform-doctor.ts | 98 +++ cli/test/commands/linear.test.ts | 9 + cli/test/linear-auth-health.test.ts | 216 ++++++ cli/test/linear-oauth.test.ts | 156 ++++ cli/test/platform-doctor.test.ts | 128 ++++ yarn.lock | 9 +- 27 files changed, 3423 insertions(+), 103 deletions(-) create mode 100644 cdk/src/handlers/shared/linear-attachments.ts create mode 100644 cdk/src/handlers/shared/linear-issue-context-probe.ts delete mode 100644 cdk/src/types/pdf-parse.d.ts create mode 100644 cdk/test/handlers/shared/linear-attachments.test.ts create mode 100644 cli/src/linear-auth-health.ts create mode 100644 cli/test/linear-auth-health.test.ts create mode 100644 cli/test/platform-doctor.test.ts 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..c46a11b7c 100644 --- a/cdk/src/constructs/linear-project-mapping-table.ts +++ b/cdk/src/constructs/linear-project-mapping-table.ts @@ -55,6 +55,15 @@ export interface LinearProjectMappingTableProps { * - label_filter — Linear issue label that triggers a task (default `bgagent`) * - status — 'active' | 'removed' * - onboarded_at, updated_at — ISO timestamps + * - decompose_allowed — #299 Mode B: enable `bgagent:decompose`/`bgagent:auto` + * auto-decomposition for this project (absent → false/off) + * - max_sub_issues — #299: cap on an auto-decomposed plan's sub-issue count + * (absent → default 8) + * - max_parent_budget_usd — #299: cap on a plan's worst-case cost + * (Σ child budgets, USD; absent → unbounded) + * + * The table is schemaless apart from the partition key; the #299 fields are + * additive and read with defaults, so pre-#299 rows need 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..ced5eae8d --- /dev/null +++ b/cdk/src/handlers/shared/linear-attachments.ts @@ -0,0 +1,678 @@ +/** + * 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 * 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; + +/* 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. + */ +const MARKDOWN_LINK_OR_IMAGE_PATTERN = /!?\[([^\]]*)\]\(]+)>?\)/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). Bounded length. + const id = (pathname.replace(/[^A-Za-z0-9_-]/g, '-').replace(/-+/g, '-').replace(/^-|-$/g, '') || `upload-${index}`).slice(0, MAX_ATTACHMENT_ID_LENGTH); + 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 []; + 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(description)) !== 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..67cdb44a2 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,7 +202,18 @@ export async function resolveLinearOauthToken( // ─── Step 3: Refresh if expiring ───────────────────────────────── if (isTokenExpiring(token.expires_at)) { - const refreshed = await refreshLinearToken(token, sm, row.oauth_secret_arn, options); + // Default the revoked-marker to a registry write, so every caller records a + // dead authorization without having to remember to opt in — the whole point + // is that the failure stops being invisible. A caller can override (or pass + // a no-op) if it lacks registry write access. + const withMarker: ResolverOptions = { + ...options, + onAuthorizationRevoked: options.onAuthorizationRevoked + // Pass the installation this resolve is acting on, so a verdict reached + // about THIS grant can't be applied to a successor installed meanwhile. + ?? ((workspaceId) => markWorkspaceRevoked(ddb, registryTableName, workspaceId, row.installed_at)), + }; + const refreshed = await refreshLinearToken(token, sm, row.oauth_secret_arn, withMarker); if (!refreshed) { // Refresh failed — return null so the caller can fall back to // best-effort behaviour. Cache is already invalidated. @@ -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..f49acc7ae --- /dev/null +++ b/cdk/test/handlers/shared/linear-attachments.test.ts @@ -0,0 +1,466 @@ +/** + * 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 { 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('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..48d3f35f9 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,78 @@ describe('resolveLinearOauthToken', () => { expect(fetchImpl).toHaveBeenCalledTimes(1); }); + test('a permanently-rejected refresh RECORDS the revocation on the registry row', 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. + 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 result = await resolveLinearOauthToken('ws-uuid-revoke', 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, + }); + 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 +477,94 @@ 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's DEFAULT marker carries the installation from the row it read", async () => { + // The value must come from the read that drove this refresh attempt. Re-reading + // it inside the marker 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' }), + }); + + // No onAuthorizationRevoked override: the built-in registry write runs. + 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(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..701ee42e0 100644 --- a/cli/src/commands/linear.ts +++ b/cli/src/commands/linear.ts @@ -53,6 +53,9 @@ import { promptSecret } from '../prompt-secret'; /** Default label that triggers an ABCA task when applied to a Linear issue. */ const DEFAULT_LABEL_FILTER = 'bgagent'; +/** Auto-decomposition: default sub-issue cap shown when --max-sub-issues is omitted (matches the handler default). */ +const DEFAULT_MAX_SUB_ISSUES = 8; + /** Standard RFC 4122 UUID — Linear's `projects.nodes[].id` matches this shape. */ const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; @@ -113,12 +116,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 +382,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 +457,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 +536,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 +627,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 +671,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 +694,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 +746,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 +870,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 +968,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.'); @@ -1383,6 +1408,9 @@ export function makeLinearCommand(): Command { .requiredOption('--repo ', 'GitHub repository the mapped project should route tasks to') .option('--label