From f54148159c860a626d4df5fca1703964dc1b8ee8 Mon Sep 17 00:00:00 2001 From: bgagent Date: Wed, 13 May 2026 15:34:13 -0700 Subject: [PATCH 1/8] feat(linear): comment + react on pre-container task-creation failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the silent-drop UX gap that appeared whenever a Linear-triggered task was rejected before the agent container started — the user would apply the trigger label, see nothing happen, and have no way to know why. Reactions and progress comments are emitted by the agent container; nothing fired until that point, so all upstream rejections were invisible on the Linear side. This commit wires a best-effort GraphQL feedback path covering all six distinct rejection points: In `linear-webhook-processor.ts` (pre-`createTaskCore`): 1. Issue has no projectId → "isn't in a project" comment 2. Project not onboarded / removed → "isn't onboarded; admin can run `bgagent linear onboard-project`" comment 3. Webhook missing organization or actor → diagnostic comment 4. Linear actor has no linked platform user → "v1 only the API-token owner can submit; multi-user OAuth is on the v3 roadmap" comment 5. `createTaskCore` returns non-201 → message branched on status: guardrail/validation block surfaces the user-facing error string; 503 prompts the user to re-apply the label; other 4xx/5xx falls through to a generic message. In `orchestrate-task.ts` (post-201, in admission control): 6. User concurrency cap rejection → "concurrency limit; wait for one to finish, then re-apply the label" comment. All five processor paths and the orchestrator path call a shared helper, `reportIssueFailure(secretArn, issueId, message)`, that runs the comment and ❌ reaction in parallel via `Promise.allSettled`. The helper: - Reuses the existing 5-minute `getLinearSecret` cache from `linear-verify.ts` (no extra Secrets Manager hits on warm Lambdas). - Swallows network, auth, and GraphQL errors with WARN logs — Linear feedback is advisory and must never gate the rejection path. - Posts to Linear's hosted GraphQL endpoint; mutation shapes match `agent/src/linear_reactions.py` (`commentCreate`, `reactionCreate`). CDK plumbing: - `linear-integration.ts` — wires `LINEAR_API_TOKEN_SECRET_ARN` into the webhook processor and grants read on the existing `LinearIntegration.apiTokenSecret`. - `agent.ts` — grants the same secret to `orchestrator.fn` and populates the env var. The grant is unconditional; the orchestrator only invokes the helper when `task.channel_source === 'linear'`. The non-Linear case is a hard no-op at the call site — `notifyLinear- OnConcurrencyCap` early-returns on `channel_source !== 'linear'`, and the processor only handles Linear payloads. Slack/API/webhook tasks are unaffected. Tests (28 new; 1240 → 1268, all green): - `cdk/test/handlers/shared/linear-feedback.test.ts` (13 tests): mutation shape, auth header, error swallowing in 4 distinct failure modes (secret-resolution null, non-2xx, GraphQL `errors`, network throw), `Promise.allSettled` partial-success semantics. - `cdk/test/handlers/linear-webhook-processor.test.ts` (10 new tests in a `user-visible feedback` describe block): one assertion per rejection path + happy-path-doesn't-fire + filter-rejection-doesn't- fire (the latter is intentional UX — the processor sees many events that aren't tasks, and dropping a comment on each would be noisy). - `cdk/test/handlers/orchestrate-task-feedback.test.ts` (5 tests): new file; covers `notifyLinearOnConcurrencyCap` directly with `withDurableExecution` mocked. Asserts the linear path fires; the api/webhook/slack paths no-op; missing metadata, missing env, and undefined `channel_metadata` all no-op cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) --- cdk/src/constructs/linear-integration.ts | 2 + cdk/src/handlers/linear-webhook-processor.ts | 65 +++++++ cdk/src/handlers/orchestrate-task.ts | 39 +++++ cdk/src/handlers/shared/linear-feedback.ts | 147 ++++++++++++++++ cdk/src/stacks/agent.ts | 12 ++ .../handlers/linear-webhook-processor.test.ts | 142 +++++++++++++++ .../orchestrate-task-feedback.test.ts | 131 ++++++++++++++ .../handlers/shared/linear-feedback.test.ts | 163 ++++++++++++++++++ 8 files changed, 701 insertions(+) create mode 100644 cdk/src/handlers/shared/linear-feedback.ts create mode 100644 cdk/test/handlers/orchestrate-task-feedback.test.ts create mode 100644 cdk/test/handlers/shared/linear-feedback.test.ts diff --git a/cdk/src/constructs/linear-integration.ts b/cdk/src/constructs/linear-integration.ts index 93f08989..e50c777f 100644 --- a/cdk/src/constructs/linear-integration.ts +++ b/cdk/src/constructs/linear-integration.ts @@ -181,11 +181,13 @@ export class LinearIntegration extends Construct { ...createTaskEnv, LINEAR_PROJECT_MAPPING_TABLE_NAME: this.projectMappingTable.tableName, LINEAR_USER_MAPPING_TABLE_NAME: this.userMappingTable.tableName, + LINEAR_API_TOKEN_SECRET_ARN: this.apiTokenSecret.secretArn, }, bundling: commonBundling, }); this.projectMappingTable.grantReadData(webhookProcessorFn); this.userMappingTable.grantReadData(webhookProcessorFn); + this.apiTokenSecret.grantRead(webhookProcessorFn); props.taskTable.grantReadWriteData(webhookProcessorFn); props.taskEventsTable.grantReadWriteData(webhookProcessorFn); if (props.repoTable) { diff --git a/cdk/src/handlers/linear-webhook-processor.ts b/cdk/src/handlers/linear-webhook-processor.ts index 261a4566..4ba5f66a 100644 --- a/cdk/src/handlers/linear-webhook-processor.ts +++ b/cdk/src/handlers/linear-webhook-processor.ts @@ -21,12 +21,14 @@ import * as crypto from 'crypto'; import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; import { DynamoDBDocumentClient, GetCommand } from '@aws-sdk/lib-dynamodb'; import { createTaskCore } from './shared/create-task-core'; +import { reportIssueFailure } from './shared/linear-feedback'; import { logger } from './shared/logger'; const ddb = DynamoDBDocumentClient.from(new DynamoDBClient({})); const PROJECT_MAPPING_TABLE = process.env.LINEAR_PROJECT_MAPPING_TABLE_NAME!; const USER_MAPPING_TABLE = process.env.LINEAR_USER_MAPPING_TABLE_NAME!; +const API_TOKEN_SECRET_ARN = process.env.LINEAR_API_TOKEN_SECRET_ARN!; const DEFAULT_LABEL_FILTER = 'bgagent'; /** Shape of Linear `Issue` webhook payloads we care about. Undocumented fields are tolerated. */ @@ -100,6 +102,11 @@ export async function handler(event: ProcessorEvent): Promise { logger.info('Linear Issue has no projectId — skipping (cannot route to a repo)', { issue_id: issue.id, }); + await reportIssueFailure( + API_TOKEN_SECRET_ARN, + issue.id, + "❌ This Linear issue isn't in a project — ABCA needs a Linear project to route the task to a repo. Move the issue into a project and re-apply the trigger label.", + ); return; } @@ -113,6 +120,11 @@ export async function handler(event: ProcessorEvent): Promise { linear_project_id: projectId, issue_id: issue.id, }); + await reportIssueFailure( + API_TOKEN_SECRET_ARN, + issue.id, + "❌ This Linear project isn't onboarded to ABCA. An admin can onboard it with `bgagent linear onboard-project --repo / --label `.", + ); return; } const repo = mapping.Item.repo as string; @@ -145,6 +157,11 @@ export async function handler(event: ProcessorEvent): Promise { organization_id: workspaceId, actor_id: actorId, }); + await reportIssueFailure( + API_TOKEN_SECRET_ARN, + issue.id, + "❌ Linear webhook is missing the organization or actor field — ABCA can't attribute this task to a user. This is unusual; please report it to your ABCA admin.", + ); return; } @@ -155,6 +172,11 @@ export async function handler(event: ProcessorEvent): Promise { linear_user_id: actorId, issue_id: issue.id, }); + await reportIssueFailure( + API_TOKEN_SECRET_ARN, + issue.id, + "❌ This Linear user isn't linked to a platform user. In v1 only the API-token owner can submit tasks from Linear; multi-user OAuth support is on the v3 roadmap.", + ); return; } @@ -192,6 +214,11 @@ export async function handler(event: ProcessorEvent): Promise { body: result.body, issue_id: issue.id, }); + await reportIssueFailure( + API_TOKEN_SECRET_ARN, + issue.id, + buildCreateTaskFailureMessage(result.statusCode, result.body), + ); return; } @@ -236,6 +263,44 @@ function shouldTrigger(payload: LinearIssueEvent, labelFilter: string): boolean return false; } +/** + * Translate a `createTaskCore` non-201 response into a user-facing Linear comment. + * + * The CDK error envelope is `{ error: { code, message, request_id } }`. We surface + * the `message` because it's already user-readable (e.g. "Task description was + * blocked by content policy") and add a per-status prefix so the user can tell + * a guardrail block from a 503 from a validation error. + * + * Falls back to a generic message if the body fails to parse — best-effort, never throws. + */ +function buildCreateTaskFailureMessage(statusCode: number | undefined, rawBody: string | undefined): string { + let detail = ''; + try { + if (rawBody) { + const parsed = JSON.parse(rawBody) as { error?: { code?: string; message?: string } }; + const message = parsed.error?.message; + if (typeof message === 'string' && message.trim()) { + detail = message.trim(); + } + } + } catch { + // fall through to the generic message + } + + if (statusCode === 400 && detail) { + // Guardrail blocks and validation errors land here; the message is already + // user-readable so just prefix it. + return `❌ ABCA couldn't accept this task: ${detail}`; + } + if (statusCode === 503) { + return `❌ ABCA is temporarily unavailable (status ${statusCode}). Please re-apply the trigger label in a few minutes.`; + } + if (detail) { + return `❌ ABCA couldn't create this task (status ${statusCode ?? 'unknown'}): ${detail}`; + } + return `❌ ABCA couldn't create this task (status ${statusCode ?? 'unknown'}). Check the ABCA admin logs for details.`; +} + function buildTaskDescription(issue: LinearIssueEvent['data']): string { const parts: string[] = []; if (issue.identifier && issue.title) { diff --git a/cdk/src/handlers/orchestrate-task.ts b/cdk/src/handlers/orchestrate-task.ts index 9008c2db..1f7ed054 100644 --- a/cdk/src/handlers/orchestrate-task.ts +++ b/cdk/src/handlers/orchestrate-task.ts @@ -20,6 +20,7 @@ import { withDurableExecution, type DurableExecutionHandler } from '@aws/durable-execution-sdk-js'; import { TaskStatus, TERMINAL_STATUSES } from '../constructs/task-status'; import { resolveComputeStrategy } from './shared/compute-strategy'; +import { reportIssueFailure } from './shared/linear-feedback'; import { logger } from './shared/logger'; import { admissionControl, @@ -34,6 +35,7 @@ import { type PollState, } from './shared/orchestrator'; import { runPreflightChecks } from './shared/preflight'; +import type { TaskRecord } from './shared/types'; interface OrchestrateTaskEvent { readonly task_id: string; @@ -73,6 +75,7 @@ const durableHandler: DurableExecutionHandler = asyn if (!result) { await failTask(taskId, current.status, 'User concurrency limit reached', task.user_id, false); await emitTaskEvent(taskId, 'admission_rejected', { reason: 'concurrency_limit' }); + await notifyLinearOnConcurrencyCap(task); } return result; }); @@ -265,3 +268,39 @@ const durableHandler: DurableExecutionHandler = asyn }; export const handler = withDurableExecution(durableHandler); + +/** + * Post a Linear comment + ❌ reaction when admission control rejects a task + * for the user concurrency cap. Linear-only; silently no-ops for other + * channels. + * + * The processor side (`linear-webhook-processor.ts`) already covers + * pre-`createTaskCore` rejections (unmapped project, unlinked actor, guardrail); + * this hook covers the post-201 case where the orchestrator rejects on + * admission. Without this, the only Linear-side signal would be the 👀 + * reaction the agent never gets to add — looks like the integration silently + * dropped the request. + * + * Best-effort: errors inside `reportIssueFailure` are swallowed at the helper + * layer; we don't surface them here because Linear feedback must never block + * the rejection path. + * + * Exported for unit testing — the durable handler invokes it inline. + */ +export async function notifyLinearOnConcurrencyCap(task: TaskRecord): Promise { + if (task.channel_source !== 'linear') return; + const issueId = task.channel_metadata?.linear_issue_id; + if (!issueId) return; + const secretArn = process.env.LINEAR_API_TOKEN_SECRET_ARN; + if (!secretArn) { + logger.warn('Skipping Linear concurrency-cap feedback: LINEAR_API_TOKEN_SECRET_ARN not set', { + task_id: task.task_id, + }); + return; + } + await reportIssueFailure( + secretArn, + issueId, + '❌ ABCA hit your concurrency limit — too many tasks running for your user. Wait for one to finish, then re-apply the trigger label.', + ); +} diff --git a/cdk/src/handlers/shared/linear-feedback.ts b/cdk/src/handlers/shared/linear-feedback.ts new file mode 100644 index 00000000..958c365a --- /dev/null +++ b/cdk/src/handlers/shared/linear-feedback.ts @@ -0,0 +1,147 @@ +/** + * 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 { getLinearSecret } from './linear-verify'; +import { logger } from './logger'; + +/** + * Lambda-side helper for posting comments and reactions onto Linear issues + * via direct GraphQL. Used by the webhook processor to give users feedback + * on pre-container failures (guardrail block, concurrency cap, unmapped + * project, etc.) — paths where the agent never starts and the agent-side + * Linear MCP / `linear_reactions.py` cannot run. + * + * All calls are best-effort. Errors are logged at WARN and swallowed — + * Linear feedback is advisory and must never gate task-rejection logic. + */ + +const LINEAR_GRAPHQL_URL = 'https://api.linear.app/graphql'; + +const REQUEST_TIMEOUT_MS = 5000; + +/** Reaction emoji short-code for the failure marker. Matches `EMOJI_FAILURE` in `agent/src/linear_reactions.py`. */ +const EMOJI_FAILURE = 'x'; + +const COMMENT_CREATE_MUTATION = ` +mutation CreateComment($issueId: String!, $body: String!) { + commentCreate(input: { issueId: $issueId, body: $body }) { + success + } +} +`.trim(); + +const REACTION_CREATE_MUTATION = ` +mutation ReactIssue($issueId: String!, $emoji: String!) { + reactionCreate(input: { issueId: $issueId, emoji: $emoji }) { + success + } +} +`.trim(); + +async function graphqlRequest( + apiToken: string, + query: string, + variables: Record, +): 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': apiToken, + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ query, variables }), + signal: controller.signal, + }); + if (!resp.ok) { + logger.warn('Linear feedback GraphQL non-2xx', { status: resp.status }); + return false; + } + const body = (await resp.json()) as { errors?: unknown }; + if (body.errors) { + logger.warn('Linear feedback GraphQL errors', { errors: body.errors }); + return false; + } + return true; + } catch (err) { + logger.warn('Linear feedback request failed', { + error: err instanceof Error ? err.message : String(err), + }); + return false; + } finally { + clearTimeout(timer); + } +} + +async function resolveToken(secretArn: string): Promise { + try { + return await getLinearSecret(secretArn); + } catch (err) { + logger.warn('Linear feedback could not resolve API token', { + error: err instanceof Error ? err.message : String(err), + }); + return null; + } +} + +/** + * Post a comment onto a Linear issue. Returns true on success, false on any failure + * (network, auth, GraphQL errors). Never throws — callers proceed regardless. + */ +export async function postIssueComment( + apiTokenSecretArn: string, + issueId: string, + body: string, +): Promise { + const token = await resolveToken(apiTokenSecretArn); + if (!token) return false; + return graphqlRequest(token, COMMENT_CREATE_MUTATION, { issueId, body }); +} + +/** + * Add an emoji reaction onto a Linear issue. Defaults to ❌ — the failure marker + * the agent uses on the success/failure side. Returns true on success. + */ +export async function addIssueReaction( + apiTokenSecretArn: string, + issueId: string, + emoji: string = EMOJI_FAILURE, +): Promise { + const token = await resolveToken(apiTokenSecretArn); + if (!token) return false; + return graphqlRequest(token, REACTION_CREATE_MUTATION, { issueId, emoji }); +} + +/** + * Convenience: post a feedback comment **and** drop a ❌ reaction in one call. + * Both calls run in parallel; both are best-effort. Returns void — callers + * never branch on the result. + */ +export async function reportIssueFailure( + apiTokenSecretArn: string, + issueId: string, + message: string, +): Promise { + await Promise.allSettled([ + postIssueComment(apiTokenSecretArn, issueId, message), + addIssueReaction(apiTokenSecretArn, issueId, EMOJI_FAILURE), + ]); +} diff --git a/cdk/src/stacks/agent.ts b/cdk/src/stacks/agent.ts index 92683bf5..ba3001c0 100644 --- a/cdk/src/stacks/agent.ts +++ b/cdk/src/stacks/agent.ts @@ -604,6 +604,18 @@ export class AgentStack extends Stack { linearIntegration.apiTokenSecret.secretArn, ); + // Pipe the Linear API token secret into the orchestrator Lambda so the + // concurrency-cap rejection path can post a Linear comment + ❌ instead + // of silently dropping the task. The orchestrator only uses the secret + // when `task.channel_source === 'linear'`, but the IAM grant is + // unconditional — the secret is created lazily via Secrets Manager and + // costs nothing if unused. + linearIntegration.apiTokenSecret.grantRead(orchestrator.fn); + orchestrator.fn.addEnvironment( + 'LINEAR_API_TOKEN_SECRET_ARN', + linearIntegration.apiTokenSecret.secretArn, + ); + new CfnOutput(this, 'LinearWebhookSecretArn', { value: linearIntegration.webhookSecret.secretArn, description: 'Secrets Manager ARN for the Linear webhook signing secret — populate via `bgagent linear setup`', diff --git a/cdk/test/handlers/linear-webhook-processor.test.ts b/cdk/test/handlers/linear-webhook-processor.test.ts index e02e823c..346c4b0e 100644 --- a/cdk/test/handlers/linear-webhook-processor.test.ts +++ b/cdk/test/handlers/linear-webhook-processor.test.ts @@ -29,8 +29,14 @@ jest.mock('../../src/handlers/shared/create-task-core', () => ({ createTaskCore: (...args: unknown[]) => createTaskCoreMock(...args), })); +const reportIssueFailureMock = jest.fn(); +jest.mock('../../src/handlers/shared/linear-feedback', () => ({ + reportIssueFailure: (...args: unknown[]) => reportIssueFailureMock(...args), +})); + process.env.LINEAR_PROJECT_MAPPING_TABLE_NAME = 'LinearProjects'; process.env.LINEAR_USER_MAPPING_TABLE_NAME = 'LinearUsers'; +process.env.LINEAR_API_TOKEN_SECRET_ARN = 'arn:aws:secretsmanager:us-east-1:123:secret:bgagent/linear/api-token-XYZ'; import { handler } from '../../src/handlers/linear-webhook-processor'; @@ -61,6 +67,8 @@ describe('linear-webhook-processor handler', () => { beforeEach(() => { ddbSend.mockReset(); createTaskCoreMock.mockReset(); + reportIssueFailureMock.mockReset(); + reportIssueFailureMock.mockResolvedValue(undefined); }); test('skips missing raw_body', async () => { @@ -188,4 +196,138 @@ describe('linear-webhook-processor handler', () => { expect(createTaskCoreMock).toHaveBeenCalledTimes(1); }); + + describe('user-visible feedback on silent-failure paths', () => { + test('posts comment + ❌ when issue has no projectId', async () => { + const payload = issue(); + const data = { ...(payload.data as Record) }; + delete data.projectId; + payload.data = data; + + await handler(eventWith(payload)); + + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + const [secretArn, issueId, message] = reportIssueFailureMock.mock.calls[0]; + expect(secretArn).toBe(process.env.LINEAR_API_TOKEN_SECRET_ARN); + expect(issueId).toBe('issue-1'); + expect(message).toContain("isn't in a project"); + }); + + test('posts feedback when project is not onboarded', async () => { + ddbSend.mockResolvedValueOnce({ Item: undefined }); + + await handler(eventWith(issue())); + + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + const [, issueId, message] = reportIssueFailureMock.mock.calls[0]; + expect(issueId).toBe('issue-1'); + expect(message).toContain("isn't onboarded"); + expect(message).toContain('bgagent linear onboard-project'); + }); + + test('posts feedback when project mapping is removed', async () => { + ddbSend.mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'removed' } }); + + await handler(eventWith(issue())); + + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + }); + + test('posts feedback when actor has no linked platform user', async () => { + ddbSend + .mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }) + .mockResolvedValueOnce({ Item: undefined }); + + await handler(eventWith(issue())); + + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + const [, , message] = reportIssueFailureMock.mock.calls[0]; + expect(message).toContain("isn't linked to a platform user"); + expect(message).toContain('multi-user OAuth'); + }); + + test('posts feedback when webhook is missing organization or actor', async () => { + ddbSend + .mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }); + const payload = issue({ organizationId: '', actor: undefined }); + const data = { ...(payload.data as Record) }; + delete data.creatorId; + payload.data = data; + + await handler(eventWith(payload)); + + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + const [, , message] = reportIssueFailureMock.mock.calls[0]; + expect(message).toContain('missing the organization or actor'); + }); + + test('surfaces guardrail block message on createTaskCore 400', async () => { + ddbSend + .mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }) + .mockResolvedValueOnce({ Item: { platform_user_id: 'cognito-user-1', status: 'active' } }); + createTaskCoreMock.mockResolvedValueOnce({ + statusCode: 400, + body: JSON.stringify({ + error: { + code: 'VALIDATION_ERROR', + message: 'Task description was blocked by content policy.', + request_id: 'req-1', + }, + }), + }); + + await handler(eventWith(issue())); + + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + const [, , message] = reportIssueFailureMock.mock.calls[0]; + expect(message).toContain('blocked by content policy'); + expect(message).toContain("couldn't accept this task"); + }); + + test('surfaces 503 retry message on createTaskCore service-unavailable', async () => { + ddbSend + .mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }) + .mockResolvedValueOnce({ Item: { platform_user_id: 'cognito-user-1', status: 'active' } }); + createTaskCoreMock.mockResolvedValueOnce({ + statusCode: 503, + body: JSON.stringify({ + error: { + code: 'INTERNAL_ERROR', + message: 'Content screening is temporarily unavailable. Please try again later.', + request_id: 'req-1', + }, + }), + }); + + await handler(eventWith(issue())); + + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + const [, , message] = reportIssueFailureMock.mock.calls[0]; + expect(message).toContain('temporarily unavailable'); + expect(message).toContain('re-apply the trigger label'); + }); + + test('does NOT post feedback on the happy 201 path', async () => { + ddbSend + .mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }) + .mockResolvedValueOnce({ Item: { platform_user_id: 'cognito-user-1', status: 'active' } }); + createTaskCoreMock.mockResolvedValueOnce({ statusCode: 201, body: JSON.stringify({ data: { task_id: 'T1' } }) }); + + await handler(eventWith(issue())); + + expect(reportIssueFailureMock).not.toHaveBeenCalled(); + }); + + test('does NOT post feedback on filter-rejected events (e.g. label not present)', async () => { + ddbSend.mockResolvedValueOnce({ Item: { repo: 'org/repo', status: 'active' } }); + const payload = issue(); + (payload.data as Record).labels = [{ id: 'l2', name: 'other' }]; + + await handler(eventWith(payload)); + + // Filter rejection is intentional UX (not every Linear event triggers ABCA); + // dropping a comment/❌ here would be noisy and misleading. + expect(reportIssueFailureMock).not.toHaveBeenCalled(); + }); + }); }); diff --git a/cdk/test/handlers/orchestrate-task-feedback.test.ts b/cdk/test/handlers/orchestrate-task-feedback.test.ts new file mode 100644 index 00000000..2b7b6662 --- /dev/null +++ b/cdk/test/handlers/orchestrate-task-feedback.test.ts @@ -0,0 +1,131 @@ +/** + * 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. + */ + +// Mock the durable execution SDK before importing orchestrate-task — its +// `withDurableExecution` wraps the handler at module import time. We only +// care about `notifyLinearOnConcurrencyCap` here, which is a plain async +// function exported alongside the durable handler. +jest.mock('@aws/durable-execution-sdk-js', () => ({ + withDurableExecution: (fn: unknown) => fn, +})); + +const reportIssueFailureMock = jest.fn(); +jest.mock('../../src/handlers/shared/linear-feedback', () => ({ + reportIssueFailure: (...args: unknown[]) => reportIssueFailureMock(...args), +})); + +// Stub the unused-but-imported orchestrator helpers so module-init side +// effects don't try to talk to AWS. +jest.mock('../../src/handlers/shared/orchestrator', () => ({ + admissionControl: jest.fn(), + emitTaskEvent: jest.fn(), + failTask: jest.fn(), + finalizeTask: jest.fn(), + hydrateAndTransition: jest.fn(), + loadBlueprintConfig: jest.fn(), + loadTask: jest.fn(), + pollTaskStatus: jest.fn(), + transitionTask: jest.fn(), +})); +jest.mock('../../src/handlers/shared/preflight', () => ({ + runPreflightChecks: jest.fn(), +})); +jest.mock('../../src/handlers/shared/compute-strategy', () => ({ + resolveComputeStrategy: jest.fn(), +})); + +process.env.LINEAR_API_TOKEN_SECRET_ARN = 'arn:aws:secretsmanager:us-east-1:123:secret:bgagent/linear/api-token-XYZ'; + +import { notifyLinearOnConcurrencyCap } from '../../src/handlers/orchestrate-task'; +import type { TaskRecord } from '../../src/handlers/shared/types'; + +function task(overrides: Partial = {}): TaskRecord { + return { + task_id: 'TASK001', + user_id: 'user-123', + status: 'SUBMITTED', + repo: 'org/repo', + task_type: 'new_task', + branch_name: 'bgagent/TASK001/foo', + channel_source: 'api', + status_created_at: 'SUBMITTED#2024-01-01T00:00:00Z', + created_at: '2024-01-01T00:00:00Z', + updated_at: '2024-01-01T00:00:00Z', + ...overrides, + } as TaskRecord; +} + +describe('notifyLinearOnConcurrencyCap', () => { + beforeEach(() => { + reportIssueFailureMock.mockReset(); + reportIssueFailureMock.mockResolvedValue(undefined); + }); + + test('posts Linear comment + ❌ when channel_source is linear and issue id is set', async () => { + await notifyLinearOnConcurrencyCap(task({ + channel_source: 'linear', + channel_metadata: { linear_issue_id: 'lin-issue-1' }, + })); + + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + const [secretArn, issueId, message] = reportIssueFailureMock.mock.calls[0]; + expect(secretArn).toBe(process.env.LINEAR_API_TOKEN_SECRET_ARN); + expect(issueId).toBe('lin-issue-1'); + expect(message).toContain('concurrency limit'); + }); + + test('no-ops on non-Linear channels (api / webhook / slack)', async () => { + for (const source of ['api', 'webhook', 'slack'] as const) { + reportIssueFailureMock.mockClear(); + await notifyLinearOnConcurrencyCap(task({ + channel_source: source, + channel_metadata: { linear_issue_id: 'lin-issue-1' }, // even if metadata is set + })); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); + } + }); + + test('no-ops when channel_metadata is missing the issue id (defensive)', async () => { + await notifyLinearOnConcurrencyCap(task({ + channel_source: 'linear', + channel_metadata: {}, // no linear_issue_id + })); + + expect(reportIssueFailureMock).not.toHaveBeenCalled(); + }); + + test('no-ops when channel_metadata is undefined', async () => { + await notifyLinearOnConcurrencyCap(task({ channel_source: 'linear' })); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); + }); + + test('no-ops when LINEAR_API_TOKEN_SECRET_ARN env is not set (logs warn)', async () => { + const saved = process.env.LINEAR_API_TOKEN_SECRET_ARN; + delete process.env.LINEAR_API_TOKEN_SECRET_ARN; + try { + await notifyLinearOnConcurrencyCap(task({ + channel_source: 'linear', + channel_metadata: { linear_issue_id: 'lin-issue-1' }, + })); + expect(reportIssueFailureMock).not.toHaveBeenCalled(); + } finally { + process.env.LINEAR_API_TOKEN_SECRET_ARN = saved; + } + }); +}); diff --git a/cdk/test/handlers/shared/linear-feedback.test.ts b/cdk/test/handlers/shared/linear-feedback.test.ts new file mode 100644 index 00000000..71c436b6 --- /dev/null +++ b/cdk/test/handlers/shared/linear-feedback.test.ts @@ -0,0 +1,163 @@ +/** + * 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 getLinearSecretMock = jest.fn(); +jest.mock('../../../src/handlers/shared/linear-verify', () => ({ + getLinearSecret: (...args: unknown[]) => getLinearSecretMock(...args), +})); + +const fetchMock = jest.fn(); +// `fetch` is a global on Node 24; reassign for test isolation. +(globalThis as unknown as { fetch: jest.Mock }).fetch = fetchMock; + +import { + addIssueReaction, + postIssueComment, + reportIssueFailure, +} from '../../../src/handlers/shared/linear-feedback'; + +const SECRET_ARN = 'arn:aws:secretsmanager:us-east-1:123:secret:bgagent/linear/api-token-XYZ'; +const ISSUE_ID = 'issue-1'; +const TOKEN = 'lin_api_TESTTOKEN'; + +function jsonResponse(body: unknown, status: number = 200): Response { + return { + ok: status >= 200 && status < 300, + status, + json: async () => body, + } as unknown as Response; +} + +describe('linear-feedback', () => { + beforeEach(() => { + getLinearSecretMock.mockReset(); + fetchMock.mockReset(); + getLinearSecretMock.mockResolvedValue(TOKEN); + fetchMock.mockResolvedValue(jsonResponse({ data: { commentCreate: { success: true } } })); + }); + + describe('postIssueComment', () => { + test('POSTs the commentCreate mutation with the issue id and body', async () => { + const ok = await postIssueComment(SECRET_ARN, ISSUE_ID, '❌ blocked'); + + expect(ok).toBe(true); + expect(fetchMock).toHaveBeenCalledTimes(1); + const [url, init] = fetchMock.mock.calls[0]; + expect(url).toBe('https://api.linear.app/graphql'); + expect(init.method).toBe('POST'); + expect(init.headers).toMatchObject({ + 'Authorization': TOKEN, + 'Content-Type': 'application/json', + }); + const body = JSON.parse(init.body as string) as { query: string; variables: Record }; + expect(body.query).toContain('commentCreate'); + expect(body.variables).toEqual({ issueId: ISSUE_ID, body: '❌ blocked' }); + }); + + test('returns false (and logs warn) when the secret cannot be resolved', async () => { + getLinearSecretMock.mockResolvedValueOnce(null); + + const ok = await postIssueComment(SECRET_ARN, ISSUE_ID, 'msg'); + + expect(ok).toBe(false); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + test('returns false on non-2xx response (no throw)', async () => { + fetchMock.mockResolvedValueOnce(jsonResponse({}, 500)); + + const ok = await postIssueComment(SECRET_ARN, ISSUE_ID, 'msg'); + + expect(ok).toBe(false); + }); + + test('returns false on GraphQL errors (no throw)', async () => { + fetchMock.mockResolvedValueOnce(jsonResponse({ errors: [{ message: 'auth' }] })); + + const ok = await postIssueComment(SECRET_ARN, ISSUE_ID, 'msg'); + + expect(ok).toBe(false); + }); + + test('returns false on network failure (swallowed)', async () => { + fetchMock.mockRejectedValueOnce(new Error('ECONNRESET')); + + const ok = await postIssueComment(SECRET_ARN, ISSUE_ID, 'msg'); + + expect(ok).toBe(false); + }); + + test('returns false when getLinearSecret throws (swallowed at resolveToken layer)', async () => { + getLinearSecretMock.mockRejectedValueOnce(new Error('AccessDenied')); + + const ok = await postIssueComment(SECRET_ARN, ISSUE_ID, 'msg'); + + expect(ok).toBe(false); + expect(fetchMock).not.toHaveBeenCalled(); + }); + }); + + describe('addIssueReaction', () => { + test('defaults to ❌ (emoji short-code "x")', async () => { + await addIssueReaction(SECRET_ARN, ISSUE_ID); + + const init = fetchMock.mock.calls[0][1]; + const body = JSON.parse(init.body as string) as { query: string; variables: { emoji: string } }; + expect(body.query).toContain('reactionCreate'); + expect(body.variables.emoji).toBe('x'); + }); + + test('honours an explicit emoji argument', async () => { + await addIssueReaction(SECRET_ARN, ISSUE_ID, 'eyes'); + + const init = fetchMock.mock.calls[0][1]; + const body = JSON.parse(init.body as string) as { variables: { emoji: string } }; + expect(body.variables.emoji).toBe('eyes'); + }); + }); + + describe('reportIssueFailure', () => { + test('posts comment + ❌ in parallel via Promise.allSettled', async () => { + await reportIssueFailure(SECRET_ARN, ISSUE_ID, '❌ failed'); + + expect(fetchMock).toHaveBeenCalledTimes(2); + const queries = fetchMock.mock.calls.map((c) => { + const init = c[1]; + return JSON.parse(init.body as string).query as string; + }); + expect(queries.some((q) => q.includes('commentCreate'))).toBe(true); + expect(queries.some((q) => q.includes('reactionCreate'))).toBe(true); + }); + + test('does not throw when one leg fails (partial-success semantics)', async () => { + // First call (comment) fails; second (reaction) succeeds. + fetchMock + .mockResolvedValueOnce(jsonResponse({}, 500)) + .mockResolvedValueOnce(jsonResponse({ data: { reactionCreate: { success: true } } })); + + await expect(reportIssueFailure(SECRET_ARN, ISSUE_ID, 'msg')).resolves.toBeUndefined(); + }); + + test('does not throw when both legs fail', async () => { + fetchMock.mockRejectedValue(new Error('ECONNRESET')); + + await expect(reportIssueFailure(SECRET_ARN, ISSUE_ID, 'msg')).resolves.toBeUndefined(); + }); + }); +}); From 07bcc5b9dc49e2596ecaa7001e31d950f1ff4f08 Mon Sep 17 00:00:00 2001 From: bgagent Date: Thu, 14 May 2026 11:56:04 -0700 Subject: [PATCH 2/8] =?UTF-8?q?feat(linear):=20finish=20v1.1=20polish=20?= =?UTF-8?q?=E2=80=94=20state-on-start=20+=20Alain=20#63=20nits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wraps the v1.1 polish theme from PR #87. Five small additions, all agent-side or docs: State-on-start (the user-visible one): - prompt_builder._channel_prompt_addendum now instructs the agent to transition the originating Linear issue to `In Progress` (or `Todo` fallback) at agent-start, mirroring the existing `In Review` chain fired at PR-open. Closes the gap where the issue stayed at `Backlog` during real agent work — only the 👀 reaction and "🤖 Starting" comment signaled progress, while humans-using-Linear expect the state column to reflect "being worked." Skips if the issue is already in `In Progress` or any later state; doesn't loop on list_issue_statuses. Alain #63 review nits (4 small surgical changes): - linear_reactions.py: auth-failure circuit breaker. Track consecutive 401/403s; after 3 strikes, log ERROR once and short-circuit all later _graphql calls (return None) until the container restarts. Resets on any 2xx response. Replaces the prior behaviour where revoked tokens flooded CloudWatch with WARNs and wasted Linear API quota indefinitely. - pipeline.py: declare `linear_eyes_reaction_id: str | None = None` explicitly before the try block instead of relying on `locals().get("linear_eyes_reaction_id")` in the crash handler. Functionally identical; survives refactors and reads cleanly. - config.py::resolve_linear_api_token: narrow `except Exception` to `(BotoCoreError, ClientError)` from botocore.exceptions. Switch `print()` to `shell.log("WARN", ...)` so warnings join the structured log stream the rest of the agent uses. - LINEAR_SETUP_GUIDE.md + cli/src/commands/linear.ts: stop telling users to run `bgagent linear link ` when auto-link fails — the code generator is a v3 feature that doesn't ship in v1, so the suggestion was misleading. Replaced with explicit admin-assisted fallback (DynamoDB put-item with steps to find workspaceId, viewerId, Cognito sub) and a clear "this command exists but is non-functional in v1" note. Tests: 532 agent + 1268 cdk + 196 cli, all green. Deployed to backgroundagent-dev. Smoke-tested 👀-on-start (156ms, agent unblocked) in the prior commit; state-on-start smoke is the next manual step. Co-Authored-By: Claude Opus 4.7 (1M context) --- agent/src/config.py | 10 +- agent/src/linear_reactions.py | 206 +++++++++++++++- agent/src/pipeline.py | 12 +- agent/src/prompt_builder.py | 22 +- agent/tests/test_linear_reactions.py | 221 +++++++++++++++++- cli/src/commands/linear.ts | 8 +- docs/guides/LINEAR_SETUP_GUIDE.md | 23 +- .../content/docs/using/Linear-setup-guide.md | 23 +- 8 files changed, 499 insertions(+), 26 deletions(-) diff --git a/agent/src/config.py b/agent/src/config.py index 0e9e4958..d3c9e745 100644 --- a/agent/src/config.py +++ b/agent/src/config.py @@ -5,6 +5,7 @@ import uuid from models import TaskConfig, TaskType +from shell import log AGENT_WORKSPACE = os.environ.get("AGENT_WORKSPACE", "/workspace") @@ -58,6 +59,7 @@ def resolve_linear_api_token() -> str: return "" try: import boto3 + from botocore.exceptions import BotoCoreError, ClientError region = os.environ.get("AWS_REGION") or os.environ.get("AWS_DEFAULT_REGION") client = boto3.client("secretsmanager", region_name=region) @@ -66,10 +68,12 @@ def resolve_linear_api_token() -> str: if token: os.environ["LINEAR_API_TOKEN"] = token return token - except Exception as e: + except (BotoCoreError, ClientError) as e: # Never let a Secrets Manager outage crash the agent. The Linear MCP - # will simply fail on first call with a clear auth error. - print(f"[config] resolve_linear_api_token failed: {type(e).__name__}: {e}", flush=True) + # will simply fail on first call with a clear auth error. Narrowed + # to botocore exceptions per Alain's #63 review — broader `except` + # hid genuine bugs in the Secrets Manager call shape. + log("WARN", f"resolve_linear_api_token failed: {type(e).__name__}: {e}") return "" diff --git a/agent/src/linear_reactions.py b/agent/src/linear_reactions.py index f7144de8..f130486e 100644 --- a/agent/src/linear_reactions.py +++ b/agent/src/linear_reactions.py @@ -26,6 +26,8 @@ from __future__ import annotations import os +import threading +import time from typing import Any import requests @@ -60,6 +62,44 @@ } """.strip() +#: Fetch reactions on an issue plus each reaction's emoji + owning user id — +#: enough to filter by viewer (the API-token owner) and emoji on re-runs. +_ISSUE_REACTIONS_QUERY = """ +query IssueReactions($issueId: String!) { + issue(id: $issueId) { + reactions { + id + emoji + user { id } + } + } +} +""".strip() + +#: Resolve the API-token owner so the sweep only deletes our own reactions +#: and never touches reactions a human added. +_VIEWER_QUERY = """ +query Viewer { viewer { id } } +""".strip() + +#: Reactions we own and want to clear before a fresh run. +_BGAGENT_EMOJIS = frozenset({EMOJI_STARTED, EMOJI_SUCCESS, EMOJI_FAILURE}) + +#: Module-level cache of the API-token owner's id. Resolved once per +#: container lifetime (Linear's `viewer { id }` is stable for the token). +_viewer_id_cache: str | None = None + +#: Auth-failure circuit breaker. Linear API tokens can be revoked mid-run; +#: without a circuit breaker, every subsequent ``_graphql`` call retries +#: (within its 5s timeout) and floods CloudWatch with WARNs while wasting +#: Linear's quota. After ``_AUTH_FAILURE_THRESHOLD`` consecutive 401/403 +#: responses, ``_auth_circuit_open`` flips to True and all later calls +#: short-circuit (return None) without hitting the network. A successful +#: 2xx response resets the counter. +_AUTH_FAILURE_THRESHOLD = 3 +_consecutive_auth_failures = 0 +_auth_circuit_open = False + def _enabled(channel_source: str, channel_metadata: dict[str, str] | None) -> str | None: """Return the Linear issue id if reactions should fire, else None. @@ -79,8 +119,16 @@ def _graphql(query: str, variables: dict[str, Any]) -> dict[str, Any] | None: """POST a GraphQL query. Return parsed data on success, None on any failure. Swallows network / auth / schema errors with a WARN log — reactions are - advisory and never gate the pipeline. + advisory and never gate the pipeline. After + ``_AUTH_FAILURE_THRESHOLD`` consecutive auth failures (401/403), the + module-level circuit breaker flips open and all later calls short-circuit + without hitting the network. A successful 2xx response resets the counter. """ + global _consecutive_auth_failures, _auth_circuit_open + + if _auth_circuit_open: + return None + token = os.environ.get("LINEAR_API_TOKEN", "") if not token: log("WARN", "linear_reactions: LINEAR_API_TOKEN not set; skipping reaction") @@ -100,10 +148,29 @@ def _graphql(query: str, variables: dict[str, Any]) -> dict[str, Any] | None: log("WARN", f"linear_reactions: request failed ({type(e).__name__}): {e}") return None + if resp.status_code in (401, 403): + _consecutive_auth_failures += 1 + if _consecutive_auth_failures >= _AUTH_FAILURE_THRESHOLD and not _auth_circuit_open: + _auth_circuit_open = True + log( + "ERROR", + "linear_reactions: auth circuit OPEN after " + f"{_consecutive_auth_failures} consecutive {resp.status_code}s — " + "API token likely revoked. Suppressing further Linear calls " + "for this container.", + ) + else: + log("WARN", f"linear_reactions: HTTP {resp.status_code} from Linear (auth)") + return None + if resp.status_code != 200: log("WARN", f"linear_reactions: HTTP {resp.status_code} from Linear") return None + # Successful 2xx — reset the auth failure counter so transient blips don't + # accumulate toward the threshold. + _consecutive_auth_failures = 0 + body = resp.json() if resp.content else {} if body.get("errors"): log("WARN", f"linear_reactions: GraphQL errors: {body['errors']}") @@ -112,18 +179,151 @@ def _graphql(query: str, variables: dict[str, Any]) -> dict[str, Any] | None: return body.get("data") or {} +def _get_viewer_id() -> str | None: + """Return the API-token owner's user id, cached for the container lifetime. + + Used by ``_sweep_stale_reactions`` to scope deletes to bgagent-owned + reactions only — without this filter, a re-run would also wipe any 👀 / ✅ + / ❌ reactions a human user happened to add for unrelated reasons. + """ + global _viewer_id_cache + if _viewer_id_cache: + return _viewer_id_cache + data = _graphql(_VIEWER_QUERY, {}) + if not data: + return None + viewer_id = (data.get("viewer") or {}).get("id") + if isinstance(viewer_id, str) and viewer_id: + _viewer_id_cache = viewer_id + return viewer_id + return None + + +def _sweep_stale_reactions(issue_id: str, exclude_id: str | None = None) -> None: + """Delete bgagent-owned 👀/✅/❌ reactions on the issue. + + Called from ``react_task_started`` *after* the new 👀 is posted, so + re-runs (label removed and re-applied; or pre-container ❌ from the + orchestrator/processor followed by a successful retry) don't accumulate + stale terminal markers next to the new 👀. Running after the post + means the user-visible 👀 lands fast even if the sweep's first call + hits cold-connection latency on Linear's API. + + The just-posted 👀 must not be deleted by the sweep — pass its id as + ``exclude_id`` so the filter skips it. + + Best-effort: any failure (viewer fetch, reactions query, individual + reactionDelete) is logged and swallowed — sweep is post-👀 cleanup + and never gates the pipeline. + """ + sweep_start = time.monotonic() + viewer_id = _get_viewer_id() + if not viewer_id: + log("WARN", "linear_reactions: skipping sweep — could not resolve viewer id") + return + + viewer_ms = int((time.monotonic() - sweep_start) * 1000) + reactions_start = time.monotonic() + data = _graphql(_ISSUE_REACTIONS_QUERY, {"issueId": issue_id}) + reactions_ms = int((time.monotonic() - reactions_start) * 1000) + if not data: + log( + "TASK", + "linear_reactions: sweep skipped (reactions query failed) " + f"viewer={viewer_ms}ms reactions={reactions_ms}ms", + ) + return + + reactions = ((data.get("issue") or {}).get("reactions") or []) + deletes = 0 + deletes_start = time.monotonic() + for r in reactions: + if not isinstance(r, dict): + continue + emoji = r.get("emoji") + if emoji not in _BGAGENT_EMOJIS: + continue + user = r.get("user") or {} + if user.get("id") != viewer_id: + continue + rid = r.get("id") + if not rid: + continue + if exclude_id is not None and rid == exclude_id: + # The 👀 we just posted — skip, it's the new marker. + continue + _graphql(_DELETE_MUTATION, {"id": rid}) + deletes += 1 + deletes_ms = int((time.monotonic() - deletes_start) * 1000) + total_ms = int((time.monotonic() - sweep_start) * 1000) + log( + "TASK", + f"linear_reactions: sweep done total={total_ms}ms viewer={viewer_ms}ms " + f"reactions={reactions_ms}ms deletes={deletes}({deletes_ms}ms)", + ) + + def react_task_started( channel_source: str, channel_metadata: dict[str, str] | None, ) -> str | None: - """Post 👀 on the Linear issue. Return the reaction id (or None on failure/no-op).""" + """Post 👀 on the Linear issue. Return the reaction id (or None on failure/no-op). + + Order matters: the 👀 is posted *first*, then we sweep any stale + bgagent-owned 👀/✅/❌ from prior runs (excluding the one we just + posted). This keeps the user-visible signal fast — if Linear's API + is slow on a cold connection, the 5s timeout falls on a sweep call + and nobody waits, instead of falling on the 👀 post and gating it. + + Sweep is best-effort; failure leaves stale terminal markers next to + the new 👀 (the visual-duplication bug we set out to fix), but the + pipeline proceeds unaffected. + """ issue_id = _enabled(channel_source, channel_metadata) if not issue_id: return None + log("TASK", f"linear_reactions: react_task_started ENTER issue_id={issue_id}") + started_at = time.monotonic() + + # Post 👀 first — this is the user-visible signal. + create_start = time.monotonic() data = _graphql(_CREATE_MUTATION, {"issueId": issue_id, "emoji": EMOJI_STARTED}) + create_ms = int((time.monotonic() - create_start) * 1000) if not data: + total_ms = int((time.monotonic() - started_at) * 1000) + log( + "WARN", + "linear_reactions: react_task_started EXIT (👀 failed) " + f"total={total_ms}ms create={create_ms}ms", + ) return None - return (data.get("reactionCreate") or {}).get("reaction", {}).get("id") + rid = (data.get("reactionCreate") or {}).get("reaction", {}).get("id") + eyes_ms = int((time.monotonic() - started_at) * 1000) + log( + "TASK", + f"linear_reactions: 👀 posted reaction_id={rid} create={create_ms}ms " + f"(eyes-visible at +{eyes_ms}ms)", + ) + + # Sweep prior bgagent reactions in a background thread so the agent + # pipeline doesn't block on Linear API latency. Daemon=True so the + # thread doesn't keep the container alive past the agent's terminal + # status. The sweep filters out the just-posted reaction id so it + # never deletes itself. + threading.Thread( + target=_sweep_stale_reactions, + args=(issue_id,), + kwargs={"exclude_id": rid}, + daemon=True, + name="linear-reactions-sweep", + ).start() + + log( + "TASK", + f"linear_reactions: react_task_started EXIT (sweep dispatched) " + f"total={eyes_ms}ms create={create_ms}ms reaction_id={rid}", + ) + return rid def react_task_finished( diff --git a/agent/src/pipeline.py b/agent/src/pipeline.py index 9a20afe9..779af814 100644 --- a/agent/src/pipeline.py +++ b/agent/src/pipeline.py @@ -330,6 +330,11 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None: ) trajectory.set_truncation_callback(_on_trace_truncated) + # Declared up-front so the crash handler at the bottom of this `try` + # can reference it via a normal name rather than ``locals().get(...)`` + # — survives refactors and reads cleanly. Stays None until the Linear + # `react_task_started` call assigns the actual reaction id. + linear_eyes_reaction_id: str | None = None try: # Context hydration with task_span("task.context_hydration"): @@ -710,13 +715,14 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None: task_state.write_terminal(config.task_id, "FAILED", crash_result.model_dump()) # Best-effort ❌ on the Linear issue so the stale 👀 doesn't linger. # No-op for non-Linear tasks; network/GraphQL failures are swallowed. - # `linear_eyes_reaction_id` may be unbound if we crashed before the - # start-reaction call — guarded with locals() to stay safe. + # `linear_eyes_reaction_id` is initialized to None at the top of + # this try block, so it's always bound here even if we crashed + # before the start-reaction call assigned a real id. react_task_finished( config.channel_source, config.channel_metadata, success=False, - started_reaction_id=locals().get("linear_eyes_reaction_id"), + started_reaction_id=linear_eyes_reaction_id, ) raise diff --git a/agent/src/prompt_builder.py b/agent/src/prompt_builder.py index f530a925..c1f5fca2 100644 --- a/agent/src/prompt_builder.py +++ b/agent/src/prompt_builder.py @@ -98,17 +98,23 @@ def _channel_prompt_addendum(config: TaskConfig) -> str: return ( "\n\n## Linear issue progress updates (REQUIRED)\n\n" f"This task was submitted from Linear issue{issue_ref}. The Linear MCP " - "server is loaded. You MUST perform these three updates; they are part " - "of the task contract, not optional:\n\n" + "server is loaded. You MUST perform these updates; they are part of " + "the task contract, not optional:\n\n" "1. **At start** — call `mcp__linear-server__save_comment` with a short " - '"🤖 Starting on this issue…" message.\n' + '"🤖 Starting on this issue…" message, then call ' + "`mcp__linear-server__save_issue` to transition the issue state. Use " + "`mcp__linear-server__list_issue_statuses` first if you don't already " + "know the state ids; pick the one named `In Progress` (fall back to " + "`Todo` if that state doesn't exist). If the issue is already in " + "`In Progress` or any later state (`In Review`, `Done`), skip the " + "transition. If neither exists, skip — the comment alone is enough. " + "Do not invent state names or loop on `list_issue_statuses`.\n" "2. **When you open the PR** — call `mcp__linear-server__save_comment` " "with the PR URL, then call `mcp__linear-server__save_issue` to " - "transition the issue state. Use `mcp__linear-server__list_issue_statuses` " - "first if you don't already know the state ids; pick the one named " - "`In Review` (fall back to `In Progress` if that state doesn't exist). " - "If neither exists, skip the state transition — the PR comment alone " - "is enough. Do not invent state names or loop on `list_issue_statuses`.\n" + "transition the issue state to `In Review` (fall back to `In Progress` " + "if that state doesn't exist). If neither exists, skip the state " + "transition — the PR comment alone is enough. Do not invent state " + "names or loop on `list_issue_statuses`.\n" "3. **On completion or failure** — call `mcp__linear-server__save_comment` " "with the final status (succeeded / failed + short reason).\n\n" "Keep comments concise. Do not mirror the full agent transcript back to " diff --git a/agent/tests/test_linear_reactions.py b/agent/tests/test_linear_reactions.py index 57b33e25..8fe3b00e 100644 --- a/agent/tests/test_linear_reactions.py +++ b/agent/tests/test_linear_reactions.py @@ -2,8 +2,12 @@ from __future__ import annotations +import threading from unittest.mock import MagicMock, patch +import pytest + +import linear_reactions from linear_reactions import ( EMOJI_FAILURE, EMOJI_STARTED, @@ -14,6 +18,69 @@ ) +@pytest.fixture(autouse=True) +def _reset_viewer_cache(): + """Reset the module-level viewer-id cache between tests so one test's + successful viewer fetch doesn't leak into another test that asserts the + sweep no-ops on viewer-fetch failure.""" + linear_reactions._viewer_id_cache = None + yield + linear_reactions._viewer_id_cache = None + + +def _viewer_response(viewer_id: str = "viewer-bot") -> MagicMock: + resp = MagicMock() + resp.status_code = 200 + payload = {"data": {"viewer": {"id": viewer_id}}} + resp.content = b'{"ok": true}' + resp.json.return_value = payload + return resp + + +def _empty_reactions_response() -> MagicMock: + resp = MagicMock() + resp.status_code = 200 + payload = {"data": {"issue": {"reactions": []}}} + resp.content = b'{"ok": true}' + resp.json.return_value = payload + return resp + + +def _reactions_response(reactions: list[dict]) -> MagicMock: + resp = MagicMock() + resp.status_code = 200 + payload = {"data": {"issue": {"reactions": reactions}}} + resp.content = b'{"ok": true}' + resp.json.return_value = payload + return resp + + +def _clean_start_calls(reaction_id: str = "r-new") -> list[MagicMock]: + """Side-effect list for a typical react_task_started call where the + issue has no prior bgagent reactions to sweep. Order matches the + runtime call sequence: 👀 is posted first; the sweep (viewer + + reactions queries) runs after on a background thread. + 1. reactionCreate → returns the new 👀 id + 2. viewer query → returns the bot's id (sweep) + 3. issue reactions query → returns empty list (sweep) + """ + return [ + _ok_response(reaction_id=reaction_id), + _viewer_response("viewer-bot"), + _empty_reactions_response(), + ] + + +def _join_sweep_thread(timeout: float = 2.0) -> None: + """Block until the daemonized sweep thread (started by react_task_started) + finishes. Tests that assert on call counts must call this after the + function returns — otherwise the sweep may still be in flight and + `requests.post` mock counts will race.""" + for t in threading.enumerate(): + if t.name == "linear-reactions-sweep": + t.join(timeout=timeout) + + def _ok_response(reaction_id: str = "r-1") -> MagicMock: resp = MagicMock() resp.status_code = 200 @@ -62,20 +129,25 @@ class TestLinearPath: """channel='linear' with issue id → correct GraphQL shape per hook.""" def test_start_posts_eyes_and_returns_reaction_id(self, monkeypatch): + """Happy path: 👀 posted first → sweep dispatched on background thread.""" monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") with patch( "linear_reactions.requests.post", - return_value=_ok_response(reaction_id="react-42"), + side_effect=_clean_start_calls(reaction_id="react-42"), ) as post: rid = react_task_started("linear", {"linear_issue_id": "issue-123"}) assert rid == "react-42" - post.assert_called_once() - args, kwargs = post.call_args - assert args[0] == LINEAR_GRAPHQL_URL - assert kwargs["headers"]["Authorization"] == "lin_api_test" - vars_ = kwargs["json"]["variables"] + # First call is the user-visible reactionCreate for 👀; this lands + # before the sweep starts so the agent never blocks on Linear API. + create_call = post.call_args_list[0] + assert create_call.args[0] == LINEAR_GRAPHQL_URL + assert create_call.kwargs["headers"]["Authorization"] == "lin_api_test" + vars_ = create_call.kwargs["json"]["variables"] assert vars_["issueId"] == "issue-123" assert vars_["emoji"] == EMOJI_STARTED + # Wait for sweep to finish so we can assert it ran. + _join_sweep_thread() + assert post.call_count == 3 def test_finish_success_deletes_eyes_then_posts_check(self, monkeypatch): """✅ path: delete the 👀 reaction first, then post the success emoji.""" @@ -133,6 +205,7 @@ class TestFailureIsSwallowed: """Reactions are advisory — network/API failures never propagate.""" def test_http_error_does_not_raise(self, monkeypatch): + """All three calls (viewer, reactions, create) return 500 — must not raise.""" monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") resp = MagicMock() resp.status_code = 500 @@ -151,6 +224,7 @@ def test_request_exception_does_not_raise(self, monkeypatch): react_task_finished("linear", {"linear_issue_id": "issue-1"}, success=True) def test_graphql_errors_do_not_raise(self, monkeypatch): + """All three calls return GraphQL `errors` envelopes — must not raise.""" monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") resp = MagicMock() resp.status_code = 200 @@ -164,3 +238,138 @@ def test_missing_token_does_not_raise(self, monkeypatch): with patch("linear_reactions.requests.post") as post: react_task_started("linear", {"linear_issue_id": "issue-1"}) post.assert_not_called() + + +class TestSweepStaleReactions: + """react_task_started sweeps prior bgagent-owned 👀/✅/❌ before posting + the new 👀, so re-runs (label removed and re-applied; or pre-container ❌ + from the orchestrator/processor followed by a successful retry) don't + show stale terminal markers next to the new 👀. + + Scoping rules the tests pin: + - delete only the 3 bgagent emojis (eyes, white_check_mark, x) + - delete only reactions owned by the API-token viewer + - never touch human-added reactions, even if they happen to use the + same emoji + - never touch bot reactions of OTHER emojis (defensive) + - sweep failures don't block the 👀 post that follows + """ + + def test_sweep_deletes_only_viewer_owned_bgagent_emojis(self, monkeypatch): + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + prior_reactions = [ + # Bot's own prior 👀, ✅, ❌ — should all be deleted. + {"id": "r-bot-eyes", "emoji": EMOJI_STARTED, "user": {"id": "viewer-bot"}}, + {"id": "r-bot-check", "emoji": EMOJI_SUCCESS, "user": {"id": "viewer-bot"}}, + {"id": "r-bot-x", "emoji": EMOJI_FAILURE, "user": {"id": "viewer-bot"}}, + # Human-added 👀 — must NOT be deleted (different user id). + {"id": "r-human-eyes", "emoji": EMOJI_STARTED, "user": {"id": "user-alice"}}, + # Bot's reaction of a non-bgagent emoji — must NOT be deleted (defensive). + {"id": "r-bot-thumbsup", "emoji": "thumbsup", "user": {"id": "viewer-bot"}}, + # Human reaction with a non-bgagent emoji — must NOT be deleted. + {"id": "r-human-rocket", "emoji": "rocket", "user": {"id": "user-bob"}}, + ] + delete_resp = _ok_delete_response() + with patch( + "linear_reactions.requests.post", + side_effect=[ + _ok_response(reaction_id="r-new-eyes"), # new 👀 (first, user-visible) + _viewer_response("viewer-bot"), # sweep: viewer fetch + _reactions_response(prior_reactions), # sweep: reactions query + delete_resp, # delete r-bot-eyes + delete_resp, # delete r-bot-check + delete_resp, # delete r-bot-x + ], + ) as post: + rid = react_task_started("linear", {"linear_issue_id": "issue-1"}) + assert rid == "r-new-eyes" + _join_sweep_thread() + + # First call is the reactionCreate for the new 👀. + assert post.call_args_list[0].kwargs["json"]["variables"]["emoji"] == EMOJI_STARTED + + # Pull out the reactionDelete calls and assert they are exactly + # the 3 bot-owned bgagent reactions, no more, no less. + delete_ids = [ + call.kwargs["json"]["variables"]["id"] + for call in post.call_args_list + if "reactionDelete" in call.kwargs["json"]["query"] + ] + assert sorted(delete_ids) == sorted(["r-bot-eyes", "r-bot-check", "r-bot-x"]) + + def test_sweep_noop_when_issue_has_no_prior_reactions(self, monkeypatch): + """Empty reactions list → no deletes, just the new 👀.""" + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + with patch( + "linear_reactions.requests.post", + side_effect=_clean_start_calls(), + ) as post: + react_task_started("linear", {"linear_issue_id": "issue-1"}) + _join_sweep_thread() + # 3 calls total: create (👀), viewer, reactions (empty). No deletes. + queries = [c.kwargs["json"]["query"] for c in post.call_args_list] + assert sum("reactionDelete" in q for q in queries) == 0 + + def test_sweep_noop_when_viewer_fetch_fails(self, monkeypatch): + """Viewer fetch returning errors short-circuits the sweep — we + can't safely filter without knowing the viewer id, so skip rather + than risk deleting a human's reaction. The 👀 post still runs + (and succeeds first, before the sweep even starts).""" + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + viewer_err = MagicMock() + viewer_err.status_code = 200 + viewer_err.content = b'{"errors":[{"message":"auth"}]}' + viewer_err.json.return_value = {"errors": [{"message": "auth"}]} + with patch( + "linear_reactions.requests.post", + side_effect=[_ok_response(reaction_id="r-new"), viewer_err], + ) as post: + rid = react_task_started("linear", {"linear_issue_id": "issue-1"}) + assert rid == "r-new" + _join_sweep_thread() + # 2 calls: 👀 first (success), then failed viewer. No reactions query, no deletes. + assert post.call_count == 2 + assert post.call_args_list[0].kwargs["json"]["variables"]["emoji"] == EMOJI_STARTED + + def test_sweep_failure_does_not_block_eyes_post(self, monkeypatch): + """Reactions query fails → sweep gives up but the 👀 already landed + (it posts first, before the sweep starts).""" + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + reactions_err = MagicMock() + reactions_err.status_code = 500 + reactions_err.content = b"err" + reactions_err.json.return_value = {} + with patch( + "linear_reactions.requests.post", + side_effect=[ + _ok_response(reaction_id="r-new"), + _viewer_response("viewer-bot"), + reactions_err, + ], + ) as post: + rid = react_task_started("linear", {"linear_issue_id": "issue-1"}) + assert rid == "r-new" + _join_sweep_thread() + assert post.call_count == 3 + + def test_viewer_id_cached_across_calls(self, monkeypatch): + """Second call within the same container reuses the cached viewer id — + no second viewer query, just create + reactions.""" + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + with patch( + "linear_reactions.requests.post", + side_effect=[ + _ok_response(reaction_id="r-1"), # 1st call's 👀 + _viewer_response("viewer-bot"), # 1st call's viewer fetch (sweep) + _empty_reactions_response(), # 1st call's reactions query (sweep) + _ok_response(reaction_id="r-2"), # 2nd call's 👀 + _empty_reactions_response(), # 2nd call's reactions only (cached viewer) + ], + ) as post: + react_task_started("linear", {"linear_issue_id": "issue-1"}) + _join_sweep_thread() + react_task_started("linear", {"linear_issue_id": "issue-1"}) + _join_sweep_thread() + queries = [c.kwargs["json"]["query"] for c in post.call_args_list] + # Only one viewer query across both calls. + assert sum("Viewer" in q for q in queries) == 1 diff --git a/cli/src/commands/linear.ts b/cli/src/commands/linear.ts index 93d4eeab..99f34b45 100644 --- a/cli/src/commands/linear.ts +++ b/cli/src/commands/linear.ts @@ -358,7 +358,13 @@ export async function autoLinkTokenOwner(args: { organization = body.data.organization; } catch (err) { console.log(` ⚠ Could not auto-link token owner: ${err instanceof Error ? err.message : String(err)}`); - console.log(' Run `bgagent linear link ` manually after generating a code.'); + console.log(' The Linear API token is stored, but you are not yet linked as a platform user.'); + console.log(' Workarounds:'); + console.log(' • Re-run `bgagent linear setup` once Linear API is reachable (most common — transient failures).'); + console.log(' • If the failure persists, an admin can insert your linked identity directly into the'); + console.log(` ${args.userMappingTable} DynamoDB table (linear_identity = "#",`); + console.log(' platform_user_id = your Cognito sub). See docs/guides/LINEAR_SETUP_GUIDE.md.'); + console.log(' `bgagent linear link ` is a v3 feature that requires Linear OAuth bot install (not in v1).'); return; } diff --git a/docs/guides/LINEAR_SETUP_GUIDE.md b/docs/guides/LINEAR_SETUP_GUIDE.md index 85ffbc09..64d221bc 100644 --- a/docs/guides/LINEAR_SETUP_GUIDE.md +++ b/docs/guides/LINEAR_SETUP_GUIDE.md @@ -55,7 +55,28 @@ Back in your terminal at the paused `bgagent linear setup` prompt: Both are stored in Secrets Manager (`LinearWebhookSecret` and `LinearApiTokenSecret`). The wizard validates that the personal API key starts with `lin_api_`. Full authentication is verified the first time a webhook arrives or the agent calls the Linear MCP. -As a final step, `setup` calls the Linear API with the token you just stored, looks up the token owner, and auto-links that Linear identity to the Cognito user currently logged in to the CLI. This skips the code-exchange ceremony for the common case where one person installs ABCA for their own workspace. If the auto-link fails (token invalid, not logged in, etc.) setup prints a warning and continues — you can always fall back to the manual link flow in Step 6. +As a final step, `setup` calls the Linear API with the token you just stored, looks up the token owner, and auto-links that Linear identity to the Cognito user currently logged in to the CLI. This skips the code-exchange ceremony for the common case where one person installs ABCA for their own workspace. If the auto-link fails (token invalid, not logged in, etc.) setup prints a warning and continues. + +**If auto-link fails persistently** (rare — usually transient Linear API hiccups, just re-run `bgagent linear setup`), an admin can insert the mapping directly into the `LinearUserMappingTable` DynamoDB table: + +```bash +aws dynamodb put-item \ + --table-name -LinearIntegrationUserMappingTable... \ + --item '{ + "linear_identity": {"S": "#"}, + "platform_user_id": {"S": ""}, + "status": {"S": "active"}, + "linked_at": {"S": "2026-05-14T00:00:00Z"} + }' +``` + +To find the right values: + +- **`workspaceId`**: from Linear API `viewer { organization { id } }` or the URL `https://linear.app//...` +- **`viewerId`**: from Linear API `viewer { id }` +- **`platform_user_id`**: your Cognito `sub` claim — `cat ~/.bgagent/credentials.json | jq -r .id_token | cut -d. -f2 | base64 -d 2>/dev/null | jq -r .sub` + +The CLI command `bgagent linear link ` exists in v1 but is **non-functional** without a Linear-side code generator (planned for v3 OAuth bot install). Do not rely on it. ### Step 5: Onboard a Linear project diff --git a/docs/src/content/docs/using/Linear-setup-guide.md b/docs/src/content/docs/using/Linear-setup-guide.md index 3566aebe..eedc9c8e 100644 --- a/docs/src/content/docs/using/Linear-setup-guide.md +++ b/docs/src/content/docs/using/Linear-setup-guide.md @@ -59,7 +59,28 @@ Back in your terminal at the paused `bgagent linear setup` prompt: Both are stored in Secrets Manager (`LinearWebhookSecret` and `LinearApiTokenSecret`). The wizard validates that the personal API key starts with `lin_api_`. Full authentication is verified the first time a webhook arrives or the agent calls the Linear MCP. -As a final step, `setup` calls the Linear API with the token you just stored, looks up the token owner, and auto-links that Linear identity to the Cognito user currently logged in to the CLI. This skips the code-exchange ceremony for the common case where one person installs ABCA for their own workspace. If the auto-link fails (token invalid, not logged in, etc.) setup prints a warning and continues — you can always fall back to the manual link flow in Step 6. +As a final step, `setup` calls the Linear API with the token you just stored, looks up the token owner, and auto-links that Linear identity to the Cognito user currently logged in to the CLI. This skips the code-exchange ceremony for the common case where one person installs ABCA for their own workspace. If the auto-link fails (token invalid, not logged in, etc.) setup prints a warning and continues. + +**If auto-link fails persistently** (rare — usually transient Linear API hiccups, just re-run `bgagent linear setup`), an admin can insert the mapping directly into the `LinearUserMappingTable` DynamoDB table: + +```bash +aws dynamodb put-item \ + --table-name -LinearIntegrationUserMappingTable... \ + --item '{ + "linear_identity": {"S": "#"}, + "platform_user_id": {"S": ""}, + "status": {"S": "active"}, + "linked_at": {"S": "2026-05-14T00:00:00Z"} + }' +``` + +To find the right values: + +- **`workspaceId`**: from Linear API `viewer { organization { id } }` or the URL `https://linear.app//...` +- **`viewerId`**: from Linear API `viewer { id }` +- **`platform_user_id`**: your Cognito `sub` claim — `cat ~/.bgagent/credentials.json | jq -r .id_token | cut -d. -f2 | base64 -d 2>/dev/null | jq -r .sub` + +The CLI command `bgagent linear link ` exists in v1 but is **non-functional** without a Linear-side code generator (planned for v3 OAuth bot install). Do not rely on it. ### Step 5: Onboard a Linear project From b047b1e5df3434a0208ce96e8566372ac41b2906 Mon Sep 17 00:00:00 2001 From: bgagent Date: Thu, 14 May 2026 13:46:44 -0700 Subject: [PATCH 3/8] chore(format): apply ruff format Whitespace-only changes flagged by CI's self-mutation guard. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) --- agent/src/linear_reactions.py | 2 +- agent/tests/test_linear_reactions.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/agent/src/linear_reactions.py b/agent/src/linear_reactions.py index f130486e..3ed0adeb 100644 --- a/agent/src/linear_reactions.py +++ b/agent/src/linear_reactions.py @@ -234,7 +234,7 @@ def _sweep_stale_reactions(issue_id: str, exclude_id: str | None = None) -> None ) return - reactions = ((data.get("issue") or {}).get("reactions") or []) + reactions = (data.get("issue") or {}).get("reactions") or [] deletes = 0 deletes_start = time.monotonic() for r in reactions: diff --git a/agent/tests/test_linear_reactions.py b/agent/tests/test_linear_reactions.py index 8fe3b00e..2c65d4fe 100644 --- a/agent/tests/test_linear_reactions.py +++ b/agent/tests/test_linear_reactions.py @@ -274,8 +274,8 @@ def test_sweep_deletes_only_viewer_owned_bgagent_emojis(self, monkeypatch): "linear_reactions.requests.post", side_effect=[ _ok_response(reaction_id="r-new-eyes"), # new 👀 (first, user-visible) - _viewer_response("viewer-bot"), # sweep: viewer fetch - _reactions_response(prior_reactions), # sweep: reactions query + _viewer_response("viewer-bot"), # sweep: viewer fetch + _reactions_response(prior_reactions), # sweep: reactions query delete_resp, # delete r-bot-eyes delete_resp, # delete r-bot-check delete_resp, # delete r-bot-x @@ -359,11 +359,11 @@ def test_viewer_id_cached_across_calls(self, monkeypatch): with patch( "linear_reactions.requests.post", side_effect=[ - _ok_response(reaction_id="r-1"), # 1st call's 👀 - _viewer_response("viewer-bot"), # 1st call's viewer fetch (sweep) - _empty_reactions_response(), # 1st call's reactions query (sweep) - _ok_response(reaction_id="r-2"), # 2nd call's 👀 - _empty_reactions_response(), # 2nd call's reactions only (cached viewer) + _ok_response(reaction_id="r-1"), # 1st call's 👀 + _viewer_response("viewer-bot"), # 1st call's viewer fetch (sweep) + _empty_reactions_response(), # 1st call's reactions query (sweep) + _ok_response(reaction_id="r-2"), # 2nd call's 👀 + _empty_reactions_response(), # 2nd call's reactions only (cached viewer) ], ) as post: react_task_started("linear", {"linear_issue_id": "issue-1"}) From dd5474e8cac1cdba817102e695205973153bee7d Mon Sep 17 00:00:00 2001 From: bgagent Date: Fri, 15 May 2026 09:01:39 -0700 Subject: [PATCH 4/8] fix(linear): address PR #87 must-fix review items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - linear_reactions: guard auth-circuit globals with `_auth_state_lock` so the daemon sweep thread and the main thread can't race the read-modify-write on `_consecutive_auth_failures` / `_auth_circuit_open`. - linear_reactions: wrap the daemon sweep target in `_sweep_stale_reactions_safe` so an unexpected exception logs at ERROR instead of dying silently (stderr from a daemon thread doesn't reliably reach CloudWatch). - linear_reactions: only increment the sweep delete counter when `_graphql(_DELETE_MUTATION, ...)` actually returns a non-None response — previously the summary log overstated success. - config: hoist `import boto3` out of the catch-narrowed try/except so an `ImportError` (boto3 missing from the image) degrades to a WARN log instead of crashing the agent. - orchestrate-task: wrap `notifyLinearOnConcurrencyCap` in a defensive try/catch — durable-execution retries the entire admission-control step on throw, which would re-fire `failTask` + `emitTaskEvent` and produce duplicate events. - tests: 1 new throw-propagation test for `notifyLinearOnConcurrencyCap`, 3 new tests for `resolve_linear_api_token` (cached env, no-arn, ImportError fallback). Auto-reset fixture in `test_linear_reactions.py` now also resets the circuit-breaker globals between tests so future cases don't leak state. Co-Authored-By: Claude Opus 4.7 (1M context) --- agent/src/config.py | 7 +++ agent/src/linear_reactions.py | 49 ++++++++++++++----- agent/tests/test_config.py | 37 +++++++++++++- agent/tests/test_linear_reactions.py | 12 +++-- cdk/src/handlers/orchestrate-task.ts | 11 ++++- .../orchestrate-task-feedback.test.ts | 15 ++++++ 6 files changed, 114 insertions(+), 17 deletions(-) diff --git a/agent/src/config.py b/agent/src/config.py index d3c9e745..fda2f4ed 100644 --- a/agent/src/config.py +++ b/agent/src/config.py @@ -60,7 +60,14 @@ def resolve_linear_api_token() -> str: try: import boto3 from botocore.exceptions import BotoCoreError, ClientError + except ImportError as e: + # boto3 missing from the container image — degrade gracefully rather + # than hard-crashing the agent. The Linear MCP will fail on first + # call with a clear auth error. + log("WARN", f"resolve_linear_api_token: boto3 unavailable ({e}); skipping") + return "" + try: region = os.environ.get("AWS_REGION") or os.environ.get("AWS_DEFAULT_REGION") client = boto3.client("secretsmanager", region_name=region) resp = client.get_secret_value(SecretId=secret_arn) diff --git a/agent/src/linear_reactions.py b/agent/src/linear_reactions.py index 3ed0adeb..8763c1f1 100644 --- a/agent/src/linear_reactions.py +++ b/agent/src/linear_reactions.py @@ -95,10 +95,12 @@ #: Linear's quota. After ``_AUTH_FAILURE_THRESHOLD`` consecutive 401/403 #: responses, ``_auth_circuit_open`` flips to True and all later calls #: short-circuit (return None) without hitting the network. A successful -#: 2xx response resets the counter. +#: 2xx response resets the counter. The lock guards the read-modify-write +#: against the daemon sweep thread. _AUTH_FAILURE_THRESHOLD = 3 _consecutive_auth_failures = 0 _auth_circuit_open = False +_auth_state_lock = threading.Lock() def _enabled(channel_source: str, channel_metadata: dict[str, str] | None) -> str | None: @@ -126,8 +128,9 @@ def _graphql(query: str, variables: dict[str, Any]) -> dict[str, Any] | None: """ global _consecutive_auth_failures, _auth_circuit_open - if _auth_circuit_open: - return None + with _auth_state_lock: + if _auth_circuit_open: + return None token = os.environ.get("LINEAR_API_TOKEN", "") if not token: @@ -149,13 +152,19 @@ def _graphql(query: str, variables: dict[str, Any]) -> dict[str, Any] | None: return None if resp.status_code in (401, 403): - _consecutive_auth_failures += 1 - if _consecutive_auth_failures >= _AUTH_FAILURE_THRESHOLD and not _auth_circuit_open: - _auth_circuit_open = True + with _auth_state_lock: + _consecutive_auth_failures += 1 + opened = ( + _consecutive_auth_failures >= _AUTH_FAILURE_THRESHOLD and not _auth_circuit_open + ) + if opened: + _auth_circuit_open = True + failures = _consecutive_auth_failures + if opened: log( "ERROR", "linear_reactions: auth circuit OPEN after " - f"{_consecutive_auth_failures} consecutive {resp.status_code}s — " + f"{failures} consecutive {resp.status_code}s — " "API token likely revoked. Suppressing further Linear calls " "for this container.", ) @@ -169,7 +178,8 @@ def _graphql(query: str, variables: dict[str, Any]) -> dict[str, Any] | None: # Successful 2xx — reset the auth failure counter so transient blips don't # accumulate toward the threshold. - _consecutive_auth_failures = 0 + with _auth_state_lock: + _consecutive_auth_failures = 0 body = resp.json() if resp.content else {} if body.get("errors"): @@ -199,6 +209,23 @@ def _get_viewer_id() -> str | None: return None +def _sweep_stale_reactions_safe(issue_id: str, exclude_id: str | None = None) -> None: + """Top-level wrapper for the sweep daemon thread. + + Catches everything so an unexpected ``TypeError`` / ``AttributeError`` + inside ``_sweep_stale_reactions`` doesn't kill the thread silently — + stderr from a daemon thread may not reach CloudWatch in containerized + environments. + """ + try: + _sweep_stale_reactions(issue_id, exclude_id=exclude_id) + except Exception as e: + log( + "ERROR", + f"linear_reactions: sweep thread crashed ({type(e).__name__}): {e}", + ) + + def _sweep_stale_reactions(issue_id: str, exclude_id: str | None = None) -> None: """Delete bgagent-owned 👀/✅/❌ reactions on the issue. @@ -252,8 +279,8 @@ def _sweep_stale_reactions(issue_id: str, exclude_id: str | None = None) -> None if exclude_id is not None and rid == exclude_id: # The 👀 we just posted — skip, it's the new marker. continue - _graphql(_DELETE_MUTATION, {"id": rid}) - deletes += 1 + if _graphql(_DELETE_MUTATION, {"id": rid}) is not None: + deletes += 1 deletes_ms = int((time.monotonic() - deletes_start) * 1000) total_ms = int((time.monotonic() - sweep_start) * 1000) log( @@ -311,7 +338,7 @@ def react_task_started( # status. The sweep filters out the just-posted reaction id so it # never deletes itself. threading.Thread( - target=_sweep_stale_reactions, + target=_sweep_stale_reactions_safe, args=(issue_id,), kwargs={"exclude_id": rid}, daemon=True, diff --git a/agent/tests/test_config.py b/agent/tests/test_config.py index d9e32c84..aa01fe12 100644 --- a/agent/tests/test_config.py +++ b/agent/tests/test_config.py @@ -1,8 +1,11 @@ """Unit tests for config.py — build_config and constants.""" +import sys +from unittest.mock import patch + import pytest -from config import PR_TASK_TYPES, build_config +from config import PR_TASK_TYPES, build_config, resolve_linear_api_token from models import TaskConfig @@ -85,3 +88,35 @@ def test_auto_generated_task_id(self): ) assert config.task_id assert len(config.task_id) == 12 + + +class TestResolveLinearApiToken: + """Coverage for the secrets-manager + boto3 fallback paths.""" + + def test_returns_cached_env_var_without_calling_boto(self, monkeypatch): + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_cached") + monkeypatch.setenv("LINEAR_API_TOKEN_SECRET_ARN", "arn:aws:sm:::secret/linear") + # boto3 must not be touched if the env var is already set. + with patch("config.log") as mock_log: + assert resolve_linear_api_token() == "lin_cached" + mock_log.assert_not_called() + + def test_returns_empty_when_no_secret_arn(self, monkeypatch): + monkeypatch.delenv("LINEAR_API_TOKEN", raising=False) + monkeypatch.delenv("LINEAR_API_TOKEN_SECRET_ARN", raising=False) + assert resolve_linear_api_token() == "" + + def test_import_error_degrades_gracefully(self, monkeypatch): + """If boto3 is missing from the container image, log WARN and return '' + rather than crashing the agent.""" + monkeypatch.delenv("LINEAR_API_TOKEN", raising=False) + monkeypatch.setenv("LINEAR_API_TOKEN_SECRET_ARN", "arn:aws:sm:::secret/linear") + # Force `import boto3` (executed inside resolve_linear_api_token) to + # raise ImportError by removing it from sys.modules and shadowing it. + monkeypatch.setitem(sys.modules, "boto3", None) + with patch("config.log") as mock_log: + assert resolve_linear_api_token() == "" + # WARN logged, no exception escaped. + assert mock_log.call_count == 1 + assert mock_log.call_args[0][0] == "WARN" + assert "boto3 unavailable" in mock_log.call_args[0][1] diff --git a/agent/tests/test_linear_reactions.py b/agent/tests/test_linear_reactions.py index 2c65d4fe..270e4c0e 100644 --- a/agent/tests/test_linear_reactions.py +++ b/agent/tests/test_linear_reactions.py @@ -19,13 +19,17 @@ @pytest.fixture(autouse=True) -def _reset_viewer_cache(): - """Reset the module-level viewer-id cache between tests so one test's - successful viewer fetch doesn't leak into another test that asserts the - sweep no-ops on viewer-fetch failure.""" +def _reset_module_state(): + """Reset module-level caches and the auth circuit breaker between tests + so one test's state never leaks into another (viewer cache, consecutive + auth-failure counter, circuit-open flag).""" linear_reactions._viewer_id_cache = None + linear_reactions._consecutive_auth_failures = 0 + linear_reactions._auth_circuit_open = False yield linear_reactions._viewer_id_cache = None + linear_reactions._consecutive_auth_failures = 0 + linear_reactions._auth_circuit_open = False def _viewer_response(viewer_id: str = "viewer-bot") -> MagicMock: diff --git a/cdk/src/handlers/orchestrate-task.ts b/cdk/src/handlers/orchestrate-task.ts index 1f7ed054..fe244559 100644 --- a/cdk/src/handlers/orchestrate-task.ts +++ b/cdk/src/handlers/orchestrate-task.ts @@ -75,7 +75,16 @@ const durableHandler: DurableExecutionHandler = asyn if (!result) { await failTask(taskId, current.status, 'User concurrency limit reached', task.user_id, false); await emitTaskEvent(taskId, 'admission_rejected', { reason: 'concurrency_limit' }); - await notifyLinearOnConcurrencyCap(task); + // Linear feedback is non-fatal: a throw here would re-run failTask + + // emitTaskEvent on the durable-execution retry, producing duplicate events. + try { + await notifyLinearOnConcurrencyCap(task); + } catch (err) { + logger.warn('Linear concurrency-cap feedback failed (non-fatal)', { + task_id: taskId, + error: err instanceof Error ? err.message : String(err), + }); + } } return result; }); diff --git a/cdk/test/handlers/orchestrate-task-feedback.test.ts b/cdk/test/handlers/orchestrate-task-feedback.test.ts index 2b7b6662..88776b19 100644 --- a/cdk/test/handlers/orchestrate-task-feedback.test.ts +++ b/cdk/test/handlers/orchestrate-task-feedback.test.ts @@ -128,4 +128,19 @@ describe('notifyLinearOnConcurrencyCap', () => { process.env.LINEAR_API_TOKEN_SECRET_ARN = saved; } }); + + test('reportIssueFailure rejection propagates (caller must catch)', async () => { + // The helper itself swallows network errors internally, but we contract + // for callers to wrap the call defensively because durable-execution + // retries the entire step on throw, producing duplicate failTask + + // emitTaskEvent. This test asserts the rejection actually propagates so + // the orchestrate-task try-catch is load-bearing, not redundant. + reportIssueFailureMock.mockRejectedValue(new Error('boom')); + await expect( + notifyLinearOnConcurrencyCap(task({ + channel_source: 'linear', + channel_metadata: { linear_issue_id: 'lin-issue-1' }, + })), + ).rejects.toThrow('boom'); + }); }); From 420fc1116d2c984fb1d30db3c8aaa2db717fa6c1 Mon Sep 17 00:00:00 2001 From: bgagent Date: Fri, 15 May 2026 09:04:58 -0700 Subject: [PATCH 5/8] chore(linear): address PR #87 nice-to-have review items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - linear_reactions: log a single DEBUG line when the auth circuit breaker short-circuits a call, so the path isn't zero-trace once open. - config: split the `(BotoCoreError, ClientError)` catch so `AccessDeniedException` logs at ERROR instead of WARN — IAM misconfig is persistent and should page someone, not blend into transient warnings. Also drop the personal name from the inline reference to the #63 review. - linear-webhook-processor: tighten `buildCreateTaskFailureMessage` param types to `number` / `string` (no `| undefined`) — the only caller passes `APIGatewayProxyResult` fields which are always defined. Removes dead fallback-to-`'unknown'` branches. - test_config: 2 new tests covering the split exception path (AccessDenied → ERROR; ResourceNotFound → WARN). Co-Authored-By: Claude Opus 4.7 (1M context) --- agent/src/config.py | 15 +++++--- agent/src/linear_reactions.py | 6 ++-- agent/tests/test_config.py | 38 +++++++++++++++++++- cdk/src/handlers/linear-webhook-processor.ts | 6 ++-- 4 files changed, 55 insertions(+), 10 deletions(-) diff --git a/agent/src/config.py b/agent/src/config.py index fda2f4ed..dc52c01e 100644 --- a/agent/src/config.py +++ b/agent/src/config.py @@ -75,11 +75,18 @@ def resolve_linear_api_token() -> str: if token: os.environ["LINEAR_API_TOKEN"] = token return token - except (BotoCoreError, ClientError) as e: + except ClientError as e: + # Narrowed from a broader `except` per #63 review — broader catches + # hid genuine bugs in the Secrets Manager call shape. AccessDenied + # is logged at ERROR because it's a persistent IAM misconfig that + # should page someone, not a transient blip. + code = e.response.get("Error", {}).get("Code", "") + severity = "ERROR" if code == "AccessDeniedException" else "WARN" + log(severity, f"resolve_linear_api_token failed: {type(e).__name__}: {e}") + return "" + except BotoCoreError as e: # Never let a Secrets Manager outage crash the agent. The Linear MCP - # will simply fail on first call with a clear auth error. Narrowed - # to botocore exceptions per Alain's #63 review — broader `except` - # hid genuine bugs in the Secrets Manager call shape. + # will simply fail on first call with a clear auth error. log("WARN", f"resolve_linear_api_token failed: {type(e).__name__}: {e}") return "" diff --git a/agent/src/linear_reactions.py b/agent/src/linear_reactions.py index 8763c1f1..3f8fb3f7 100644 --- a/agent/src/linear_reactions.py +++ b/agent/src/linear_reactions.py @@ -129,8 +129,10 @@ def _graphql(query: str, variables: dict[str, Any]) -> dict[str, Any] | None: global _consecutive_auth_failures, _auth_circuit_open with _auth_state_lock: - if _auth_circuit_open: - return None + circuit_open = _auth_circuit_open + if circuit_open: + log("DEBUG", "linear_reactions: auth circuit still open; short-circuiting call") + return None token = os.environ.get("LINEAR_API_TOKEN", "") if not token: diff --git a/agent/tests/test_config.py b/agent/tests/test_config.py index aa01fe12..42054dfe 100644 --- a/agent/tests/test_config.py +++ b/agent/tests/test_config.py @@ -1,7 +1,7 @@ """Unit tests for config.py — build_config and constants.""" import sys -from unittest.mock import patch +from unittest.mock import MagicMock, patch import pytest @@ -120,3 +120,39 @@ def test_import_error_degrades_gracefully(self, monkeypatch): assert mock_log.call_count == 1 assert mock_log.call_args[0][0] == "WARN" assert "boto3 unavailable" in mock_log.call_args[0][1] + + def test_access_denied_logged_at_error(self, monkeypatch): + """Persistent IAM misconfig should page someone — escalate from WARN + to ERROR so alerts fire.""" + monkeypatch.delenv("LINEAR_API_TOKEN", raising=False) + monkeypatch.setenv("LINEAR_API_TOKEN_SECRET_ARN", "arn:aws:sm:::secret/linear") + + from botocore.exceptions import ClientError + + err = ClientError( + {"Error": {"Code": "AccessDeniedException", "Message": "no access"}}, + "GetSecretValue", + ) + fake_client = MagicMock() + fake_client.get_secret_value.side_effect = err + with patch("boto3.client", return_value=fake_client), patch("config.log") as mock_log: + assert resolve_linear_api_token() == "" + assert mock_log.call_count == 1 + assert mock_log.call_args[0][0] == "ERROR" + + def test_other_client_error_logged_at_warn(self, monkeypatch): + monkeypatch.delenv("LINEAR_API_TOKEN", raising=False) + monkeypatch.setenv("LINEAR_API_TOKEN_SECRET_ARN", "arn:aws:sm:::secret/linear") + + from botocore.exceptions import ClientError + + err = ClientError( + {"Error": {"Code": "ResourceNotFoundException", "Message": "missing"}}, + "GetSecretValue", + ) + fake_client = MagicMock() + fake_client.get_secret_value.side_effect = err + with patch("boto3.client", return_value=fake_client), patch("config.log") as mock_log: + assert resolve_linear_api_token() == "" + assert mock_log.call_count == 1 + assert mock_log.call_args[0][0] == "WARN" diff --git a/cdk/src/handlers/linear-webhook-processor.ts b/cdk/src/handlers/linear-webhook-processor.ts index 4ba5f66a..b9df404a 100644 --- a/cdk/src/handlers/linear-webhook-processor.ts +++ b/cdk/src/handlers/linear-webhook-processor.ts @@ -273,7 +273,7 @@ function shouldTrigger(payload: LinearIssueEvent, labelFilter: string): boolean * * Falls back to a generic message if the body fails to parse — best-effort, never throws. */ -function buildCreateTaskFailureMessage(statusCode: number | undefined, rawBody: string | undefined): string { +function buildCreateTaskFailureMessage(statusCode: number, rawBody: string): string { let detail = ''; try { if (rawBody) { @@ -296,9 +296,9 @@ function buildCreateTaskFailureMessage(statusCode: number | undefined, rawBody: return `❌ ABCA is temporarily unavailable (status ${statusCode}). Please re-apply the trigger label in a few minutes.`; } if (detail) { - return `❌ ABCA couldn't create this task (status ${statusCode ?? 'unknown'}): ${detail}`; + return `❌ ABCA couldn't create this task (status ${statusCode}): ${detail}`; } - return `❌ ABCA couldn't create this task (status ${statusCode ?? 'unknown'}). Check the ABCA admin logs for details.`; + return `❌ ABCA couldn't create this task (status ${statusCode}). Check the ABCA admin logs for details.`; } function buildTaskDescription(issue: LinearIssueEvent['data']): string { From f4633be9bc74acf24b3382603abdc7021b559eb9 Mon Sep 17 00:00:00 2001 From: bgagent Date: Sun, 17 May 2026 23:19:44 -0700 Subject: [PATCH 6/8] fix(linear): address PR #87 re-review must-fix items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - linear-webhook-processor: extracted `safeReportIssueFailure` helper and routed all 5 bare `await reportIssueFailure(...)` call sites through it. The helper is uniformly non-throwing — wraps the call in try/catch to defend against a future signature refactor that could break the helper's `Promise.allSettled` never-throw contract. A synchronous throw would otherwise propagate, fail the Lambda, and trigger SQS retries on a poison message. - linear-webhook-processor: dropped the `!` non-null assertion on `process.env.LINEAR_API_TOKEN_SECRET_ARN` at module scope. The helper now guards on `!API_TOKEN_SECRET_ARN` and logs a single clear `Skipping Linear feedback: LINEAR_API_TOKEN_SECRET_ARN not set` diagnostic per skip — matches the orchestrator pattern in `notifyLinearOnConcurrencyCap`. - test_linear_reactions: new `TestAuthCircuitBreaker` class with 5 tests covering the previously-untested circuit: * 3 consecutive 401/403s open the circuit * Once open, calls short-circuit without hitting the network * A 2xx between failures resets the counter * Non-auth status (500) doesn't increment the counter * 401 and 403 are both treated as auth failures - test_linear-webhook-processor: 2 new tests assert `safeReportIssueFailure` swallows both synchronous throws and async rejections from the underlying helper. Co-Authored-By: Claude Opus 4.7 (1M context) --- agent/tests/test_linear_reactions.py | 110 ++++++++++++++++++ cdk/src/handlers/linear-webhook-processor.ts | 50 ++++++-- .../handlers/linear-webhook-processor.test.ts | 32 +++++ 3 files changed, 181 insertions(+), 11 deletions(-) diff --git a/agent/tests/test_linear_reactions.py b/agent/tests/test_linear_reactions.py index 270e4c0e..c2cdb2a8 100644 --- a/agent/tests/test_linear_reactions.py +++ b/agent/tests/test_linear_reactions.py @@ -377,3 +377,113 @@ def test_viewer_id_cached_across_calls(self, monkeypatch): queries = [c.kwargs["json"]["query"] for c in post.call_args_list] # Only one viewer query across both calls. assert sum("Viewer" in q for q in queries) == 1 + + +class TestAuthCircuitBreaker: + """The circuit breaker in `_graphql` flips open after + ``_AUTH_FAILURE_THRESHOLD`` consecutive 401/403s and short-circuits all + subsequent calls until container restart. Auto-reset fixture clears + ``_consecutive_auth_failures`` and ``_auth_circuit_open`` between tests. + + These tests target `_graphql` directly via the public `react_task_started` + entry point — by exhausting the auth-failure budget on early sweeps and + asserting later calls don't hit the network. + """ + + def _auth_response(self, status: int = 401) -> MagicMock: + resp = MagicMock() + resp.status_code = status + resp.content = b'{"errors":[{"message":"auth"}]}' + resp.json.return_value = {"errors": [{"message": "auth"}]} + return resp + + def test_three_consecutive_401s_open_the_circuit(self, monkeypatch): + """Threshold is 3 — call `_graphql` directly via the private API to + exercise the increment + open logic without depending on the + public-API call shape.""" + import linear_reactions + + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + with patch( + "linear_reactions.requests.post", + side_effect=[self._auth_response(401)] * 3, + ) as post: + assert linear_reactions._graphql("query Q { x }", {}) is None + assert linear_reactions._graphql("query Q { x }", {}) is None + assert linear_reactions._graphql("query Q { x }", {}) is None + assert post.call_count == 3 + assert linear_reactions._consecutive_auth_failures == 3 + assert linear_reactions._auth_circuit_open is True + + def test_open_circuit_short_circuits_subsequent_calls(self, monkeypatch): + """Once the circuit is open, no network calls — the function returns + None immediately.""" + import linear_reactions + + linear_reactions._auth_circuit_open = True + with patch("linear_reactions.requests.post") as post: + assert linear_reactions._graphql("query Q { x }", {}) is None + assert linear_reactions._graphql("query Q { x }", {}) is None + post.assert_not_called() + + def test_2xx_response_resets_failure_counter(self, monkeypatch): + """A 200 between 401s clears the counter — the circuit only trips on + consecutive failures, not cumulative.""" + import linear_reactions + + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + ok = MagicMock() + ok.status_code = 200 + ok.content = b'{"data":{"x":1}}' + ok.json.return_value = {"data": {"x": 1}} + with patch( + "linear_reactions.requests.post", + side_effect=[ + self._auth_response(401), + self._auth_response(401), + ok, # resets counter + self._auth_response(401), + self._auth_response(401), + ], + ): + for _ in range(5): + linear_reactions._graphql("query Q { x }", {}) + # 2 failures after the reset, threshold=3 → circuit still closed. + assert linear_reactions._consecutive_auth_failures == 2 + assert linear_reactions._auth_circuit_open is False + + def test_non_auth_errors_do_not_increment_failure_counter(self, monkeypatch): + """A 500 is a server problem, not an auth problem — must not + contribute to the auth-failure budget.""" + import linear_reactions + + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + five_hundred = MagicMock() + five_hundred.status_code = 500 + five_hundred.content = b"server error" + five_hundred.json.return_value = {} + with patch( + "linear_reactions.requests.post", + side_effect=[five_hundred] * 5, + ): + for _ in range(5): + linear_reactions._graphql("query Q { x }", {}) + assert linear_reactions._consecutive_auth_failures == 0 + assert linear_reactions._auth_circuit_open is False + + def test_403_treated_same_as_401(self, monkeypatch): + """Both auth-failure status codes count toward the threshold.""" + import linear_reactions + + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + with patch( + "linear_reactions.requests.post", + side_effect=[ + self._auth_response(401), + self._auth_response(403), + self._auth_response(401), + ], + ): + for _ in range(3): + linear_reactions._graphql("query Q { x }", {}) + assert linear_reactions._auth_circuit_open is True diff --git a/cdk/src/handlers/linear-webhook-processor.ts b/cdk/src/handlers/linear-webhook-processor.ts index b9df404a..3aecc4b0 100644 --- a/cdk/src/handlers/linear-webhook-processor.ts +++ b/cdk/src/handlers/linear-webhook-processor.ts @@ -28,9 +28,42 @@ const ddb = DynamoDBDocumentClient.from(new DynamoDBClient({})); const PROJECT_MAPPING_TABLE = process.env.LINEAR_PROJECT_MAPPING_TABLE_NAME!; const USER_MAPPING_TABLE = process.env.LINEAR_USER_MAPPING_TABLE_NAME!; -const API_TOKEN_SECRET_ARN = process.env.LINEAR_API_TOKEN_SECRET_ARN!; +const API_TOKEN_SECRET_ARN = process.env.LINEAR_API_TOKEN_SECRET_ARN; const DEFAULT_LABEL_FILTER = 'bgagent'; +/** + * Post a Linear comment + ❌ reaction without ever propagating an error. + * + * Wraps `reportIssueFailure` so each call site is one line and uniformly + * non-throwing. Two failure modes handled here: + * + * - `LINEAR_API_TOKEN_SECRET_ARN` env var unset (deploy misconfig) — log a + * single clear diagnostic and skip, instead of letting `resolveToken` log + * a cryptic "could not resolve API token" warning on every feedback call. + * Mirrors the orchestrator's `notifyLinearOnConcurrencyCap` guard. + * - `reportIssueFailure` throws synchronously (today impossible thanks to the + * helper's internal `Promise.allSettled`, but a future refactor could + * break that contract). Catching here means a synchronous throw can't + * bubble up and fail the Lambda — which would trigger SQS retries on a + * poison message. + */ +async function safeReportIssueFailure(issueId: string, message: string): Promise { + if (!API_TOKEN_SECRET_ARN) { + logger.warn('Skipping Linear feedback: LINEAR_API_TOKEN_SECRET_ARN not set', { + issue_id: issueId, + }); + return; + } + try { + await reportIssueFailure(API_TOKEN_SECRET_ARN, issueId, message); + } catch (err) { + logger.warn('Linear feedback failed (non-fatal)', { + issue_id: issueId, + error: err instanceof Error ? err.message : String(err), + }); + } +} + /** Shape of Linear `Issue` webhook payloads we care about. Undocumented fields are tolerated. */ interface LinearIssueEvent { readonly action: 'create' | 'update' | 'remove' | string; @@ -102,8 +135,7 @@ export async function handler(event: ProcessorEvent): Promise { logger.info('Linear Issue has no projectId — skipping (cannot route to a repo)', { issue_id: issue.id, }); - await reportIssueFailure( - API_TOKEN_SECRET_ARN, + await safeReportIssueFailure( issue.id, "❌ This Linear issue isn't in a project — ABCA needs a Linear project to route the task to a repo. Move the issue into a project and re-apply the trigger label.", ); @@ -120,8 +152,7 @@ export async function handler(event: ProcessorEvent): Promise { linear_project_id: projectId, issue_id: issue.id, }); - await reportIssueFailure( - API_TOKEN_SECRET_ARN, + await safeReportIssueFailure( issue.id, "❌ This Linear project isn't onboarded to ABCA. An admin can onboard it with `bgagent linear onboard-project --repo / --label `.", ); @@ -157,8 +188,7 @@ export async function handler(event: ProcessorEvent): Promise { organization_id: workspaceId, actor_id: actorId, }); - await reportIssueFailure( - API_TOKEN_SECRET_ARN, + await safeReportIssueFailure( issue.id, "❌ Linear webhook is missing the organization or actor field — ABCA can't attribute this task to a user. This is unusual; please report it to your ABCA admin.", ); @@ -172,8 +202,7 @@ export async function handler(event: ProcessorEvent): Promise { linear_user_id: actorId, issue_id: issue.id, }); - await reportIssueFailure( - API_TOKEN_SECRET_ARN, + await safeReportIssueFailure( issue.id, "❌ This Linear user isn't linked to a platform user. In v1 only the API-token owner can submit tasks from Linear; multi-user OAuth support is on the v3 roadmap.", ); @@ -214,8 +243,7 @@ export async function handler(event: ProcessorEvent): Promise { body: result.body, issue_id: issue.id, }); - await reportIssueFailure( - API_TOKEN_SECRET_ARN, + await safeReportIssueFailure( issue.id, buildCreateTaskFailureMessage(result.statusCode, result.body), ); diff --git a/cdk/test/handlers/linear-webhook-processor.test.ts b/cdk/test/handlers/linear-webhook-processor.test.ts index 346c4b0e..cbc48ff2 100644 --- a/cdk/test/handlers/linear-webhook-processor.test.ts +++ b/cdk/test/handlers/linear-webhook-processor.test.ts @@ -329,5 +329,37 @@ describe('linear-webhook-processor handler', () => { // dropping a comment/❌ here would be noisy and misleading. expect(reportIssueFailureMock).not.toHaveBeenCalled(); }); + + test('safeReportIssueFailure: synchronous throw from reportIssueFailure does not propagate', async () => { + // Defends against a future signature refactor that breaks the helper's + // never-throw contract. Today `Promise.allSettled` guarantees this; if + // someone removes that, the surrounding catch keeps the Lambda from + // failing and triggering SQS retries on a poison message. + reportIssueFailureMock.mockImplementationOnce(() => { + throw new Error('synthetic synchronous throw'); + }); + const payload = issue(); + const data = { ...(payload.data as Record) }; + delete data.projectId; + payload.data = data; + + await expect(handler(eventWith(payload))).resolves.toBeUndefined(); + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + }); + + test('safeReportIssueFailure: async rejection from reportIssueFailure does not propagate', async () => { + // The helper's internal `Promise.allSettled` already guarantees this, + // but the orchestrator path's parallel catch motivated adding the same + // belt-and-suspenders here. This test locks in the contract so a + // refactor of either helper layer can't reintroduce the failure mode. + reportIssueFailureMock.mockRejectedValueOnce(new Error('async failure')); + const payload = issue(); + const data = { ...(payload.data as Record) }; + delete data.projectId; + payload.data = data; + + await expect(handler(eventWith(payload))).resolves.toBeUndefined(); + expect(reportIssueFailureMock).toHaveBeenCalledTimes(1); + }); }); }); From a713c62cb249ed483746dbdfb458bd4c20a422f0 Mon Sep 17 00:00:00 2001 From: bgagent Date: Sun, 17 May 2026 23:20:53 -0700 Subject: [PATCH 7/8] test(linear): address PR #87 re-review nice-to-have items MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - test_config: cover the BotoCoreError branch of `resolve_linear_api_token` with an `EndpointConnectionError` case. The PR-#87 split into ClientError + BotoCoreError branches previously had no test on the BotoCoreError path. - test_linear_reactions: new `test_sweep_preserves_just_posted_eyes_via_exclude_id` exercises the `exclude_id` filter — the existing sweep test never collided prior reaction ids with the newly posted one, so the branch was effectively dead code in tests. The new test plants the just- posted 👀 in the prior reactions list and asserts it survives the sweep while an older ❌ is deleted. Co-Authored-By: Claude Opus 4.7 (1M context) --- agent/tests/test_config.py | 19 ++++++++++++++++ agent/tests/test_linear_reactions.py | 33 ++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/agent/tests/test_config.py b/agent/tests/test_config.py index 42054dfe..4421945e 100644 --- a/agent/tests/test_config.py +++ b/agent/tests/test_config.py @@ -156,3 +156,22 @@ def test_other_client_error_logged_at_warn(self, monkeypatch): assert resolve_linear_api_token() == "" assert mock_log.call_count == 1 assert mock_log.call_args[0][0] == "WARN" + + def test_botocore_error_logged_at_warn(self, monkeypatch): + """The handler is split into ClientError + BotoCoreError branches. + BotoCoreError covers transient connectivity / endpoint problems — + log WARN and degrade gracefully rather than crashing the agent.""" + monkeypatch.delenv("LINEAR_API_TOKEN", raising=False) + monkeypatch.setenv("LINEAR_API_TOKEN_SECRET_ARN", "arn:aws:sm:::secret/linear") + + from botocore.exceptions import EndpointConnectionError + + fake_client = MagicMock() + fake_client.get_secret_value.side_effect = EndpointConnectionError( + endpoint_url="https://secretsmanager.us-east-1.amazonaws.com", + ) + with patch("boto3.client", return_value=fake_client), patch("config.log") as mock_log: + assert resolve_linear_api_token() == "" + assert mock_log.call_count == 1 + assert mock_log.call_args[0][0] == "WARN" + assert "EndpointConnectionError" in mock_log.call_args[0][1] diff --git a/agent/tests/test_linear_reactions.py b/agent/tests/test_linear_reactions.py index c2cdb2a8..9b47f9ce 100644 --- a/agent/tests/test_linear_reactions.py +++ b/agent/tests/test_linear_reactions.py @@ -378,6 +378,39 @@ def test_viewer_id_cached_across_calls(self, monkeypatch): # Only one viewer query across both calls. assert sum("Viewer" in q for q in queries) == 1 + def test_sweep_preserves_just_posted_eyes_via_exclude_id(self, monkeypatch): + """If Linear's reactions query happens to return our just-posted 👀 + (e.g. on a tight retry where the prior run's reaction id was reused + by Linear), the sweep MUST NOT delete it — exclude_id is what keeps + the new marker safe. Tests would otherwise pass with exclude_id=None + because the prior reaction ids never collide with the new one.""" + monkeypatch.setenv("LINEAR_API_TOKEN", "lin_api_test") + new_rid = "r-new-eyes" + prior_reactions = [ + # The just-posted 👀 — sweep must SKIP this one. + {"id": new_rid, "emoji": EMOJI_STARTED, "user": {"id": "viewer-bot"}}, + # An older bgagent ❌ from a prior run — sweep MUST delete this. + {"id": "r-old-x", "emoji": EMOJI_FAILURE, "user": {"id": "viewer-bot"}}, + ] + with patch( + "linear_reactions.requests.post", + side_effect=[ + _ok_response(reaction_id=new_rid), + _viewer_response("viewer-bot"), + _reactions_response(prior_reactions), + _ok_delete_response(), # only one delete expected + ], + ) as post: + react_task_started("linear", {"linear_issue_id": "issue-1"}) + _join_sweep_thread() + delete_ids = [ + call.kwargs["json"]["variables"]["id"] + for call in post.call_args_list + if "reactionDelete" in call.kwargs["json"]["query"] + ] + # The new 👀 must NOT be deleted; only the old ❌ should be. + assert delete_ids == ["r-old-x"] + class TestAuthCircuitBreaker: """The circuit breaker in `_graphql` flips open after From 0a3711c95c38116d1c8b4f8f7f9de3fd285cdb27 Mon Sep 17 00:00:00 2001 From: bgagent Date: Mon, 18 May 2026 09:20:13 -0700 Subject: [PATCH 8/8] fix(linear): annotate circuit breaker globals so ty doesn't narrow `ty check` infers `_consecutive_auth_failures = 0` as `Literal[0]` and `_auth_circuit_open = False` as `Literal[False]`, which then rejects the legitimate runtime flips (and the test fixture that resets them between cases). Adding explicit `int` / `bool` annotations widens the inferred type and fixes the CI typecheck failure introduced in `f4633be`. Co-Authored-By: Claude Opus 4.7 (1M context) --- agent/src/linear_reactions.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/agent/src/linear_reactions.py b/agent/src/linear_reactions.py index 3f8fb3f7..c9f430cc 100644 --- a/agent/src/linear_reactions.py +++ b/agent/src/linear_reactions.py @@ -98,8 +98,11 @@ #: 2xx response resets the counter. The lock guards the read-modify-write #: against the daemon sweep thread. _AUTH_FAILURE_THRESHOLD = 3 -_consecutive_auth_failures = 0 -_auth_circuit_open = False +# Annotated explicitly so ty doesn't narrow the initial values to +# `Literal[0]` / `Literal[False]` — that narrowing would reject the +# legitimate flips below (and any test that resets them). +_consecutive_auth_failures: int = 0 +_auth_circuit_open: bool = False _auth_state_lock = threading.Lock()