diff --git a/cdk/src/constructs/slack-integration.ts b/cdk/src/constructs/slack-integration.ts index 9b6d2e99..8e46a11c 100644 --- a/cdk/src/constructs/slack-integration.ts +++ b/cdk/src/constructs/slack-integration.ts @@ -47,6 +47,10 @@ export interface SlackIntegrationProps { /** The DynamoDB task events table (must have DynamoDB Streams enabled). */ readonly taskEventsTable: dynamodb.ITable; + /** The Cedar HITL approvals table (optional — enables Slack-button + * approve/deny on `approval_requested` messages; issue #112). */ + readonly taskApprovalsTable?: dynamodb.ITable; + /** The DynamoDB repo config table (optional — for repo onboarding checks). */ readonly repoTable?: dynamodb.ITable; @@ -305,6 +309,19 @@ export class SlackIntegration extends Construct { slackInteractionsFn.addToRolePolicy(readSlackSecretsPolicy); props.taskTable.grantReadWriteData(slackInteractionsFn); this.userMappingTable.grantReadData(slackInteractionsFn); + // Slack-button HITL approvals (issue #112): the interactions handler + // runs the same decision core as ApproveTaskFn/DenyTaskFn, which + // needs the approvals table (decision row + rate-limit counter) and + // the events table (approval_decision_recorded audit). Without the + // approvals table, approval clicks degrade to an ephemeral + // "not configured" message. + if (props.taskApprovalsTable) { + slackInteractionsFn.addEnvironment('TASK_APPROVALS_TABLE_NAME', props.taskApprovalsTable.tableName); + slackInteractionsFn.addEnvironment('TASK_EVENTS_TABLE_NAME', props.taskEventsTable.tableName); + slackInteractionsFn.addEnvironment('TASK_RETENTION_DAYS', String(props.taskRetentionDays ?? 90)); + props.taskApprovalsTable.grantReadWriteData(slackInteractionsFn); + props.taskEventsTable.grantWriteData(slackInteractionsFn); + } // --- Slash Command Acknowledger --- const slackCommandsFn = new lambda.NodejsFunction(this, 'SlackCommandsFn', { diff --git a/cdk/src/handlers/approve-task.ts b/cdk/src/handlers/approve-task.ts index 393ade46..c9e2d645 100644 --- a/cdk/src/handlers/approve-task.ts +++ b/cdk/src/handlers/approve-task.ts @@ -17,28 +17,21 @@ * SOFTWARE. */ -import { DynamoDBClient, TransactionCanceledException } from '@aws-sdk/client-dynamodb'; -import { DynamoDBDocumentClient, PutCommand, TransactWriteCommand, UpdateCommand } from '@aws-sdk/lib-dynamodb'; +import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; +import { DynamoDBDocumentClient } from '@aws-sdk/lib-dynamodb'; import type { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda'; import { ulid } from 'ulid'; +import { approvalDecisionConfigFromEnv, processApprovalDecision } from './shared/approval-decision'; import { VALID_APPROVAL_SCOPE_PREFIXES, parseApprovalScope } from './shared/approval-scope'; import { extractUserId } from './shared/gateway'; import { logger } from './shared/logger'; -import { formatMinuteBucket } from './shared/rate-limit'; import { ErrorCode, errorResponse, successResponse } from './shared/response'; import type { ApprovalRequest, ApprovalResponse, ApprovalScope } from './shared/types'; const ddb = DynamoDBDocumentClient.from(new DynamoDBClient({})); -const TASK_TABLE_NAME = process.env.TASK_TABLE_NAME; -const TASK_APPROVALS_TABLE_NAME = process.env.TASK_APPROVALS_TABLE_NAME; -const EVENTS_TABLE_NAME = process.env.TASK_EVENTS_TABLE_NAME; -if (!TASK_TABLE_NAME || !TASK_APPROVALS_TABLE_NAME || !EVENTS_TABLE_NAME) { - throw new Error( - 'approve-task handler requires TASK_TABLE_NAME, TASK_APPROVALS_TABLE_NAME, and TASK_EVENTS_TABLE_NAME env vars', - ); -} -const APPROVE_RATE_LIMIT_PER_MINUTE = Number(process.env.APPROVE_RATE_LIMIT_PER_MINUTE ?? '30'); -const AUDIT_EVENT_RETENTION_DAYS = Number(process.env.TASK_RETENTION_DAYS ?? '90'); +// Validates the required table env vars at module load so a broken +// deployment fails loudly at cold start, matching the old behavior. +const DECISION_CONFIG = approvalDecisionConfigFromEnv(); /** * POST /v1/tasks/{task_id}/approve — User-in-the-loop approval decision. @@ -46,17 +39,11 @@ const AUDIT_EVENT_RETENTION_DAYS = Number(process.env.TASK_RETENTION_DAYS ?? '90 * Flow (design §7.1): * 1. Auth — Cognito JWT `sub` → `caller_user_id` (verbatim). * 2. Parse + validate body (`request_id`, optional `scope`). - * 3. Per-user per-minute rate limit (30/min). - * 4. Atomic cross-table `TransactWriteItems`: - * - Update approval row: `status PENDING → APPROVED`, guarded by - * `user_id = :caller` (ownership) AND `status = :pending`. - * - No-op update on TaskTable guarded by - * `status = AWAITING_APPROVAL AND awaiting_approval_request_id = :rid`. - * 5. On `TransactionCanceledException`, inspect per-item reasons to - * distinguish 404 vs 409 variants (prevents enumeration via the - * 404 collapse on ownership mismatch — finding #6). - * 6. Write `approval_decision_recorded` audit event to - * `TaskEventsTable` (IMPL-6). + * 3–6. Rate limit, atomic transition, cancel classification, and the + * audit event live in `shared/approval-decision.ts` — + * `processApprovalDecision` — shared with the Slack-button + * approvals path (issue #112) so the trust-critical core cannot + * drift between surfaces. * * Returns 202 with `{task_id, request_id, status: APPROVED, scope, * decided_at}` on success. @@ -118,125 +105,47 @@ export async function handler(event: APIGatewayProxyEvent): Promise#MINUTE#` - // so the existing grantReadWriteData wiring carries forward; TTL - // reaps the counter after ~120s. - const minuteBucket = formatMinuteBucket(new Date()); - try { - await ddb.send(new UpdateCommand({ - TableName: TASK_APPROVALS_TABLE_NAME, - Key: { - task_id: `RATE#${callerUserId}#APPROVE`, - request_id: `MINUTE#${minuteBucket}`, - }, - UpdateExpression: 'ADD #count :one SET #ttl = :ttl', - ConditionExpression: 'attribute_not_exists(#count) OR #count < :max', - ExpressionAttributeNames: { - '#count': 'count', - '#ttl': 'ttl', - }, - ExpressionAttributeValues: { - ':one': 1, - ':max': APPROVE_RATE_LIMIT_PER_MINUTE, - ':ttl': nowEpoch + 120, - }, - })); - } catch (err: unknown) { - const name = (err as { name?: string })?.name; - if (name === 'ConditionalCheckFailedException') { - return errorResponse( - 429, - ErrorCode.RATE_LIMIT_EXCEEDED, - `Rate limit exceeded: at most ${APPROVE_RATE_LIMIT_PER_MINUTE} approve/deny decisions per minute.`, - requestId, - ); - } - throw err; + // 3–5. Shared decision core (rate limit + atomic transition + audit). + const outcome = await processApprovalDecision(ddb, DECISION_CONFIG, { + taskId, + requestId: request_id, + callerUserId, + decision: 'approve', + scope, + }); + if (outcome.kind === 'rate_limited') { + return errorResponse( + 429, + ErrorCode.RATE_LIMIT_EXCEEDED, + `Rate limit exceeded: at most ${outcome.limit} approve/deny decisions per minute.`, + requestId, + ); } - - // 4. Cross-table atomic transition (§7.1 pseudocode). - try { - await ddb.send(new TransactWriteCommand({ - TransactItems: [ - { - Update: { - TableName: TASK_APPROVALS_TABLE_NAME, - Key: { task_id: taskId, request_id }, - UpdateExpression: - 'SET #status = :approved, decided_at = :now, #scope = :scope', - ConditionExpression: - 'attribute_exists(request_id) AND #status = :pending AND user_id = :caller', - ExpressionAttributeNames: { - '#status': 'status', - '#scope': 'scope', - }, - ExpressionAttributeValues: { - ':approved': 'APPROVED', - ':pending': 'PENDING', - ':now': nowIso, - ':scope': scope, - ':caller': callerUserId, - }, - }, - }, - { - Update: { - TableName: TASK_TABLE_NAME, - Key: { task_id: taskId }, - // No-op update on TaskTable; the purpose is the condition guard. - UpdateExpression: 'SET last_decision_at = :now', - ConditionExpression: - '#status = :awaiting AND awaiting_approval_request_id = :rid', - ExpressionAttributeNames: { '#status': 'status' }, - ExpressionAttributeValues: { - ':awaiting': 'AWAITING_APPROVAL', - ':rid': request_id, - ':now': nowIso, - }, - }, - }, - ], - })); - } catch (err: unknown) { - if (err instanceof TransactionCanceledException) { - return classifyCancel(err, requestId); - } - throw err; + if (outcome.kind === 'not_found') { + return errorResponse( + 404, + ErrorCode.REQUEST_NOT_FOUND, + 'Approval request not found or not owned by caller.', + requestId, + ); } - - // 5. Audit event (IMPL-6). Failure to write the audit is logged - // but does not fail the request — the decision is already - // committed on TaskApprovalsTable and the agent will see it on its - // next poll regardless. - try { - await ddb.send(new PutCommand({ - TableName: EVENTS_TABLE_NAME, - Item: { - task_id: taskId, - event_id: ulid(), - event_type: 'approval_decision_recorded', - timestamp: nowIso, - ttl: nowEpoch + AUDIT_EVENT_RETENTION_DAYS * 86400, - metadata: { - request_id, - status: 'APPROVED', - scope, - decided_at: nowIso, - caller_user_id: callerUserId, - }, - }, - })); - } catch (auditErr) { - logger.warn('approval_decision_recorded audit write failed (decision already committed)', { - task_id: taskId, - request_id, - error: auditErr instanceof Error ? auditErr.message : String(auditErr), - }); + if (outcome.kind === 'not_awaiting') { + return errorResponse( + 409, + ErrorCode.TASK_NOT_AWAITING_APPROVAL, + 'Task is not currently awaiting approval for this request.', + requestId, + ); + } + if (outcome.kind === 'transaction_unknown') { + return errorResponse( + 503, + ErrorCode.SERVICE_UNAVAILABLE, + 'Approval transaction cancelled for unknown reason.', + requestId, + ); } + const nowIso = outcome.decidedAt; logger.info('Approval recorded', { task_id: taskId, @@ -262,62 +171,3 @@ export async function handler(event: APIGatewayProxyEvent): Promise = new Set(['pr_created']); +const ROUTABLE_MILESTONES: ReadonlySet = new Set([ + 'pr_created', + 'approval_requested', + 'approval_stranded', +]); /** * Unwrap ``agent_milestone`` events to their milestone name for diff --git a/cdk/src/handlers/shared/approval-decision.ts b/cdk/src/handlers/shared/approval-decision.ts new file mode 100644 index 00000000..c7f1a438 --- /dev/null +++ b/cdk/src/handlers/shared/approval-decision.ts @@ -0,0 +1,283 @@ +/** + * 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 { TransactionCanceledException } from '@aws-sdk/client-dynamodb'; +import { type DynamoDBDocumentClient, PutCommand, TransactWriteCommand, UpdateCommand } from '@aws-sdk/lib-dynamodb'; +import { ulid } from 'ulid'; +import { logger } from './logger'; +import { formatMinuteBucket } from './rate-limit'; +import type { ApprovalScope } from './types'; + +/** + * Core Cedar HITL approve/deny decision logic, shared by the HTTP + * handlers (`approve-task.ts` / `deny-task.ts`) and the Slack + * interactions handler (issue #112 — Slack-button approvals). + * + * Encapsulates exactly the parts that MUST behave identically no matter + * which surface the decision arrives from: + * + * 1. Per-user per-minute rate limit (shared approve+deny counter). + * 2. Atomic cross-table `TransactWriteItems`: + * - approval row `PENDING → APPROVED|DENIED`, guarded by + * ownership (`user_id = :caller`) AND `status = PENDING`; + * - no-op TaskTable update guarded by + * `status = AWAITING_APPROVAL AND awaiting_approval_request_id`. + * 3. `approval_decision_recorded` audit event (best-effort). + * + * Deliberately NOT here (caller-owned, surface-specific): + * - authentication (Cognito JWT vs Slack signature + user mapping), + * - body validation, scope parsing, deny-reason secret-scanning, + * - response rendering (HTTP status codes vs Slack ephemeral text), + * - severity gating (Slack-only constraint — §11.2 finding #4). + */ + +/** Outcome of a decision attempt, surface-agnostic. Callers map these + * to HTTP responses or Slack ephemeral messages. */ +export type ApprovalDecisionOutcome = + /** Decision committed; `decidedAt` is the transaction timestamp. */ + | { readonly kind: 'ok'; readonly decidedAt: string } + /** Per-user per-minute approve+deny budget exhausted. */ + | { readonly kind: 'rate_limited'; readonly limit: number } + /** + * Approval row missing, owned by a different user, or already + * decided. Collapsed into one outcome per §7.1 finding #6 — callers + * must NOT distinguish these (existence oracle). + */ + | { readonly kind: 'not_found' } + /** Task row condition failed — not awaiting approval for this request. */ + | { readonly kind: 'not_awaiting' } + /** Transaction cancelled for a reason we can't classify. */ + | { readonly kind: 'transaction_unknown' }; + +export interface ApprovalDecisionInput { + readonly taskId: string; + /** The approval request id (`request_id` on the approvals row). */ + readonly requestId: string; + /** Platform user id the decision is attributed to (ownership-checked). */ + readonly callerUserId: string; + readonly decision: 'approve' | 'deny'; + /** Approve-only. Callers validate via `parseApprovalScope` first. */ + readonly scope?: ApprovalScope; + /** Deny-only. Callers run `scanDenyReason` + truncate BEFORE passing. */ + readonly sanitizedReason?: string; +} + +export interface ApprovalDecisionConfig { + readonly taskTableName: string; + readonly approvalsTableName: string; + readonly eventsTableName: string; + readonly rateLimitPerMinute: number; + readonly auditRetentionDays: number; +} + +/** Build the config from the standard env vars the approval handlers + * already require. Throws when a required var is absent so a broken + * deployment fails loudly at first use. */ +export function approvalDecisionConfigFromEnv(): ApprovalDecisionConfig { + const taskTableName = process.env.TASK_TABLE_NAME; + const approvalsTableName = process.env.TASK_APPROVALS_TABLE_NAME; + const eventsTableName = process.env.TASK_EVENTS_TABLE_NAME; + if (!taskTableName || !approvalsTableName || !eventsTableName) { + throw new Error( + 'approval decision requires TASK_TABLE_NAME, TASK_APPROVALS_TABLE_NAME, and TASK_EVENTS_TABLE_NAME env vars', + ); + } + return { + taskTableName, + approvalsTableName, + eventsTableName, + rateLimitPerMinute: Number(process.env.APPROVE_RATE_LIMIT_PER_MINUTE ?? '30'), + auditRetentionDays: Number(process.env.TASK_RETENTION_DAYS ?? '90'), + }; +} + +/** + * Record an approve/deny decision atomically. See module docstring for + * the invariants. Never throws for the classified 4xx-equivalent + * outcomes; infra errors (DDB unavailable, throttle outside the + * transaction) propagate to the caller's 500 path. + */ +export async function processApprovalDecision( + ddb: DynamoDBDocumentClient, + config: ApprovalDecisionConfig, + input: ApprovalDecisionInput, +): Promise { + const { taskId, requestId, callerUserId, decision } = input; + const nowIso = new Date().toISOString(); + const nowEpoch = Math.floor(Date.now() / 1000); + + // 1. Per-user per-minute rate limit. Synthetic row in the approvals + // table keyed `RATE##APPROVE` / `MINUTE#` — approve + // and deny share the counter so the combined budget is the limit. + const minuteBucket = formatMinuteBucket(new Date()); + try { + await ddb.send(new UpdateCommand({ + TableName: config.approvalsTableName, + Key: { + task_id: `RATE#${callerUserId}#APPROVE`, + request_id: `MINUTE#${minuteBucket}`, + }, + UpdateExpression: 'ADD #count :one SET #ttl = :ttl', + ConditionExpression: 'attribute_not_exists(#count) OR #count < :max', + ExpressionAttributeNames: { + '#count': 'count', + '#ttl': 'ttl', + }, + ExpressionAttributeValues: { + ':one': 1, + ':max': config.rateLimitPerMinute, + ':ttl': nowEpoch + 120, + }, + })); + } catch (err: unknown) { + const name = (err as { name?: string })?.name; + if (name === 'ConditionalCheckFailedException') { + return { kind: 'rate_limited', limit: config.rateLimitPerMinute }; + } + throw err; + } + + // 2. Cross-table atomic transition (§7.1 / §7.2 pseudocode). + interface ApprovalUpdateShape { + readonly UpdateExpression: string; + readonly ExpressionAttributeNames: Record; + readonly ExpressionAttributeValues: Record; + } + const approvalUpdate: ApprovalUpdateShape = decision === 'approve' + ? { + UpdateExpression: 'SET #status = :decided, decided_at = :now, #scope = :scope', + ExpressionAttributeNames: { '#status': 'status', '#scope': 'scope' }, + ExpressionAttributeValues: { + ':decided': 'APPROVED', + ':pending': 'PENDING', + ':now': nowIso, + ':scope': input.scope ?? 'this_call', + ':caller': callerUserId, + }, + } + : { + UpdateExpression: 'SET #status = :decided, decided_at = :now, deny_reason = :reason', + ExpressionAttributeNames: { '#status': 'status' }, + ExpressionAttributeValues: { + ':decided': 'DENIED', + ':pending': 'PENDING', + ':now': nowIso, + ':reason': input.sanitizedReason ?? '', + ':caller': callerUserId, + }, + }; + + try { + await ddb.send(new TransactWriteCommand({ + TransactItems: [ + { + Update: { + TableName: config.approvalsTableName, + Key: { task_id: taskId, request_id: requestId }, + ConditionExpression: + 'attribute_exists(request_id) AND #status = :pending AND user_id = :caller', + ...approvalUpdate, + }, + }, + { + Update: { + TableName: config.taskTableName, + Key: { task_id: taskId }, + // No-op update on TaskTable; the purpose is the condition guard. + UpdateExpression: 'SET last_decision_at = :now', + ConditionExpression: + '#status = :awaiting AND awaiting_approval_request_id = :rid', + ExpressionAttributeNames: { '#status': 'status' }, + ExpressionAttributeValues: { + ':awaiting': 'AWAITING_APPROVAL', + ':rid': requestId, + ':now': nowIso, + }, + }, + }, + ], + })); + } catch (err: unknown) { + if (err instanceof TransactionCanceledException) { + return classifyCancel(err); + } + throw err; + } + + // 3. Audit event (IMPL-6). Failure to write the audit is logged but + // does not fail the decision — it is already committed and the agent + // will see it on its next poll regardless. + const auditMetadata: Record = decision === 'approve' + ? { + request_id: requestId, + status: 'APPROVED', + scope: input.scope ?? 'this_call', + decided_at: nowIso, + caller_user_id: callerUserId, + } + : { + request_id: requestId, + status: 'DENIED', + reason: input.sanitizedReason ?? '', + decided_at: nowIso, + caller_user_id: callerUserId, + }; + try { + await ddb.send(new PutCommand({ + TableName: config.eventsTableName, + Item: { + task_id: taskId, + event_id: ulid(), + event_type: 'approval_decision_recorded', + timestamp: nowIso, + ttl: nowEpoch + config.auditRetentionDays * 86400, + metadata: auditMetadata, + }, + })); + } catch (auditErr) { + logger.warn('approval_decision_recorded audit write failed (decision already committed)', { + task_id: taskId, + request_id: requestId, + error: auditErr instanceof Error ? auditErr.message : String(auditErr), + }); + } + + return { kind: 'ok', decidedAt: nowIso }; +} + +/** + * Map a `TransactionCanceledException` to the surface-agnostic outcome. + * + * Per §7.1, the cancellation reasons are per-item (index 0 is the + * approvals-row Update, index 1 is the task-row Update). DDB does not + * say which sub-clause of a `ConditionExpression` failed, so the + * approvals-row failure collapses {missing, wrong owner, already + * decided} into `not_found` to prevent the existence oracle + * (§7.1 finding #6). + */ +function classifyCancel(err: TransactionCanceledException): ApprovalDecisionOutcome { + const reasons = err.CancellationReasons ?? []; + if (reasons[0]?.Code === 'ConditionalCheckFailed') { + return { kind: 'not_found' }; + } + if (reasons[1]?.Code === 'ConditionalCheckFailed') { + return { kind: 'not_awaiting' }; + } + return { kind: 'transaction_unknown' }; +} diff --git a/cdk/src/handlers/shared/slack-blocks.ts b/cdk/src/handlers/shared/slack-blocks.ts index b717d59b..158cfdcf 100644 --- a/cdk/src/handlers/shared/slack-blocks.ts +++ b/cdk/src/handlers/shared/slack-blocks.ts @@ -118,11 +118,89 @@ export function renderSlackBlocks( return taskStrandedMessage(task, eventMetadata); case 'agent_error': return agentErrorMessage(task, eventMetadata); + case 'approval_requested': + return approvalRequestedMessage(task, eventMetadata); + case 'approval_stranded': + return approvalStrandedMessage(task, eventMetadata); default: return simpleStatusMessage(task, `Event: ${eventType}`); } } +/** + * Cedar HITL severities that may be decided via Slack buttons. + * `high` gates require a fresh CLI Cognito flow (§11.2 finding #4) — + * the message renders without buttons and points at the CLI instead. + */ +export const SLACK_APPROVABLE_SEVERITIES: ReadonlySet = new Set(['low', 'medium']); + +/** + * `approval_requested` — the Cedar HITL gate fired and the task is + * paused in AWAITING_APPROVAL (issue #112). + * + * The action_id carries `approve_action:{task_id}:{request_id}` (or + * `deny_action:…`) — the interactions handler splits on `:` to address + * the approval row. Both ids are ULIDs (no `:` in the alphabet), so the + * separator is unambiguous. The Slack-side identity → platform user + * mapping and the severity re-check happen in `slack-interactions.ts`; + * the buttons here are UX, not authorization. + */ +function approvalRequestedMessage( + task: Pick, + eventMetadata?: Record, +): SlackMessage { + const requestId = typeof eventMetadata?.request_id === 'string' ? eventMetadata.request_id : undefined; + const toolName = typeof eventMetadata?.tool_name === 'string' ? eventMetadata.tool_name : 'a tool'; + const inputPreview = typeof eventMetadata?.input_preview === 'string' ? eventMetadata.input_preview : ''; + const reason = typeof eventMetadata?.reason === 'string' ? eventMetadata.reason : ''; + const severity = typeof eventMetadata?.severity === 'string' ? eventMetadata.severity : 'medium'; + const timeoutS = typeof eventMetadata?.timeout_s === 'number' ? eventMetadata.timeout_s : undefined; + + const lines = [`:lock: *Approval required* for \`${task.repo}\``]; + lines.push(`_Tool:_ \`${toolName}\`${inputPreview ? ` — \`${truncate(inputPreview, 150)}\`` : ''}`); + if (reason) lines.push(`_Reason:_ ${truncate(reason, 200)}`); + lines.push(`_Severity:_ ${severity}${timeoutS ? ` · _times out in ${formatDuration(timeoutS)}_` : ''}`); + const text = lines.join('\n'); + + const blocks: SlackBlock[] = [section(text)]; + + if (requestId && SLACK_APPROVABLE_SEVERITIES.has(severity)) { + blocks.push(actions(task.task_id, [ + approveButton(task.task_id, requestId), + denyButton(task.task_id, requestId), + ])); + } else if (requestId) { + // High severity: CLI-only per §11.2 finding #4 — a compromised + // Slack identity must not be able to approve the riskiest gates. + blocks.push(section( + `:shield: High-severity gate — approve via CLI: \`bgagent approve ${task.task_id} ${requestId}\``, + )); + } + + return { + text: `Approval required for ${task.repo}`, + blocks, + }; +} + +/** + * `approval_stranded` — the reconciler found a PENDING gate whose task + * stopped heartbeating. Informational; nothing left to click. + */ +function approvalStrandedMessage( + task: Pick, + eventMetadata?: Record, +): SlackMessage { + const requestId = typeof eventMetadata?.request_id === 'string' + ? `\n_Request:_ \`${eventMetadata.request_id}\`` + : ''; + const text = `:warning: *Approval request stranded* for \`${task.repo}\` — the task stopped before a decision was recorded.${requestId}`; + return { + text: `Approval request stranded for ${task.repo}`, + blocks: [section(text)], + }; +} + function taskCreatedMessage( task: Pick, ): SlackMessage { @@ -288,6 +366,36 @@ function dangerButton(label: string, actionId: string): ActionButtonElement { }; } +function approveButton(taskId: string, requestId: string): ActionButtonElement { + return { + type: 'button', + text: { type: 'plain_text', text: '✅ Approve' }, + action_id: `approve_action:${taskId}:${requestId}`, + style: 'primary', + confirm: { + title: { type: 'plain_text', text: 'Approve this tool call?' }, + text: { type: 'mrkdwn', text: 'The agent will run the gated tool call and continue.' }, + confirm: { type: 'plain_text', text: 'Approve' }, + deny: { type: 'plain_text', text: 'Go back' }, + }, + }; +} + +function denyButton(taskId: string, requestId: string): ActionButtonElement { + return { + type: 'button', + text: { type: 'plain_text', text: '❌ Deny' }, + action_id: `deny_action:${taskId}:${requestId}`, + style: 'danger', + confirm: { + title: { type: 'plain_text', text: 'Deny this tool call?' }, + text: { type: 'mrkdwn', text: 'The agent will skip the gated tool call and adjust.' }, + confirm: { type: 'plain_text', text: 'Deny' }, + deny: { type: 'plain_text', text: 'Go back' }, + }, + }; +} + function prLabel(prUrl: string): string { const match = prUrl.match(/\/pull\/(\d+)$/); return match ? `#${match[1]}` : 'Pull Request'; diff --git a/cdk/src/handlers/slack-interactions.ts b/cdk/src/handlers/slack-interactions.ts index ac25ccff..78377403 100644 --- a/cdk/src/handlers/slack-interactions.ts +++ b/cdk/src/handlers/slack-interactions.ts @@ -20,7 +20,9 @@ import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; import { DynamoDBDocumentClient, GetCommand, UpdateCommand } from '@aws-sdk/lib-dynamodb'; import type { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda'; +import { approvalDecisionConfigFromEnv, processApprovalDecision } from './shared/approval-decision'; import { logger } from './shared/logger'; +import { SLACK_APPROVABLE_SEVERITIES } from './shared/slack-blocks'; import { getSlackSecret, SLACK_SECRET_PREFIX, verifySlackRequest } from './shared/slack-verify'; const ddb = DynamoDBDocumentClient.from(new DynamoDBClient({})); @@ -28,6 +30,7 @@ const ddb = DynamoDBDocumentClient.from(new DynamoDBClient({})); const SIGNING_SECRET_ARN = process.env.SLACK_SIGNING_SECRET_ARN!; const TASK_TABLE = process.env.TASK_TABLE_NAME!; const USER_MAPPING_TABLE = process.env.SLACK_USER_MAPPING_TABLE_NAME!; +const TASK_APPROVALS_TABLE = process.env.TASK_APPROVALS_TABLE_NAME; interface SlackInteractionPayload { readonly type: string; @@ -48,6 +51,14 @@ interface SlackInteractionPayload { * Slack sends interaction payloads as a URL-encoded `payload` field in the body. * Currently handles: * - `cancel_task:{task_id}` — Cancel a running task via the "Cancel Task" button. + * - `approve_action:{task_id}:{request_id}` / `deny_action:{task_id}:{request_id}` + * — Cedar HITL approval decisions via Slack buttons (issue #112). The + * Slack identity is mapped to a platform user via `SlackUserMappingTable` + * (user-initiated linking — §11.2 finding #4), the gate's severity is + * re-checked server-side (low/medium only; high is CLI-only), and the + * decision goes through the same `processApprovalDecision` core as the + * HTTP approve/deny handlers — including the ownership condition, so a + * linked-but-wrong user's click is rejected by the transaction itself. */ export async function handler(event: APIGatewayProxyEvent): Promise { try { @@ -83,6 +94,11 @@ export async function handler(event: APIGatewayProxyEvent): Promise { + const [verb, taskId, approvalRequestId] = actionId.split(':'); + const decision = verb === 'approve_action' ? 'approve' as const : 'deny' as const; + if (!taskId || !approvalRequestId) { + logger.warn('Slack approval action_id malformed', { action_id: actionId }); + await postToResponseUrl(payload.response_url, ':warning: Malformed approval action — please use the CLI.'); + return; + } + + if (!TASK_APPROVALS_TABLE) { + logger.error('TASK_APPROVALS_TABLE_NAME not set — Slack approvals disabled'); + await postToResponseUrl(payload.response_url, + ':warning: Slack approvals are not configured on this stack. Use `bgagent approve`/`deny` from the CLI.'); + return; + } + + // 1. Slack identity → platform user (user-initiated mapping only). + const teamId = payload.user.team_id; + const slackUserId = payload.user.id; + const mappingResult = await ddb.send(new GetCommand({ + TableName: USER_MAPPING_TABLE, + Key: { slack_identity: `${teamId}#${slackUserId}` }, + })); + if (!mappingResult.Item || mappingResult.Item.status === 'pending') { + await postToResponseUrl(payload.response_url, + ':link: Your Slack account is not linked. Run `bgagent slack link` first, then re-click.'); + return; + } + const platformUserId = mappingResult.Item.platform_user_id as string; + + // 2. Server-side severity gate (§11.2 finding #4). The buttons only + // render on low/medium gates, but the message is advisory — re-read + // the approvals row and refuse high severity regardless of what the + // action_id claims. Missing row falls through to the decision core, + // which collapses it into the no-oracle "not found" outcome. + const approvalRow = await ddb.send(new GetCommand({ + TableName: TASK_APPROVALS_TABLE, + Key: { task_id: taskId, request_id: approvalRequestId }, + })); + const severity = approvalRow.Item?.severity as string | undefined; + if (severity && !SLACK_APPROVABLE_SEVERITIES.has(severity)) { + logger.warn('Slack approval rejected: severity not Slack-approvable', { + task_id: taskId, + request_id: approvalRequestId, + severity, + slack_identity: `${teamId}#${slackUserId}`, + }); + await postToResponseUrl(payload.response_url, + `:shield: This is a *${severity}*-severity gate — it can only be decided from the CLI with a fresh login: ` + + `\`bgagent ${decision} ${taskId} ${approvalRequestId}\``); + return; + } + + // 3. Shared decision core — identical invariants to the HTTP path. + const outcome = await processApprovalDecision(ddb, approvalDecisionConfigFromEnv(), { + taskId, + requestId: approvalRequestId, + callerUserId: platformUserId, + decision, + ...(decision === 'approve' ? { scope: 'this_call' as const } : { sanitizedReason: 'Denied via Slack' }), + }); + + switch (outcome.kind) { + case 'ok': { + logger.info('Slack approval decision recorded', { + task_id: taskId, + request_id: approvalRequestId, + decision, + platform_user_id: platformUserId, + slack_identity: `${teamId}#${slackUserId}`, + }); + const emoji = decision === 'approve' ? ':white_check_mark:' : ':no_entry:'; + const label = decision === 'approve' ? 'Approved' : 'Denied'; + await postToResponseUrl(payload.response_url, + `${emoji} ${label} — the agent will pick the decision up on its next poll.`); + return; + } + case 'rate_limited': + await postToResponseUrl(payload.response_url, + `:hourglass: Rate limit exceeded (${outcome.limit} decisions/minute). Try again shortly.`); + return; + case 'not_found': + // Includes "owned by someone else" — same no-oracle collapse as + // the HTTP handlers (§7.1 finding #6). + await postToResponseUrl(payload.response_url, + ':mag: Approval request not found, already decided, or not yours to decide.'); + return; + case 'not_awaiting': + await postToResponseUrl(payload.response_url, + ':warning: The task is no longer awaiting this approval (it may have timed out).'); + return; + case 'transaction_unknown': + await postToResponseUrl(payload.response_url, + ':warning: Could not record the decision — please retry or use the CLI.'); + return; + } +} + async function updateSlackMessage(botToken: string, channel: string, ts: string, text: string, threadTs?: string): Promise { try { const payload: Record = { diff --git a/cdk/src/handlers/slack-notify.ts b/cdk/src/handlers/slack-notify.ts index 4b391ac7..c1223934 100644 --- a/cdk/src/handlers/slack-notify.ts +++ b/cdk/src/handlers/slack-notify.ts @@ -91,15 +91,27 @@ const SLACK_DEDUP_ATTRIBUTE: Record = { agent_error: 'slack_dispatched_agent_error', task_created: null, session_started: null, + // Approval gates are per-request_id, and a task can fire many gates + // over its lifetime — a per-task presence marker would suppress every + // gate after the first. A duplicate post on a partial-batch retry is + // self-limiting UX-wise: the buttons carry the same + // `{task_id}:{request_id}`, and the decision core's PENDING-only + // transaction makes the second click a no-op ("already decided"). + approval_requested: null, + approval_stranded: null, }; /** Event types this dispatcher renders. Must stay in sync with the * Slack entries in ``CHANNEL_DEFAULTS`` (see fanout-task-events.ts) — * drift means the router subscribes Slack to events that the * dispatcher silently ignores, which lies in batch telemetry - * (issue #64 review Cat 7). Forward-compat ``approval_required`` and - * ``status_response`` are deliberately absent until their emitters - * ship; until then they fall through and are dropped at this gate. + * (issue #64 review Cat 7). Forward-compat ``status_response`` is + * deliberately absent until its emitter ships; until then it falls + * through and is dropped at this gate. + * ``approval_requested`` / ``approval_stranded`` are the Cedar HITL + * gate signals (issue #112) — rendered with Approve/Deny buttons for + * low/medium severity (high severity points at the CLI per §11.2 + * finding #4). * ``pr_created`` is intentionally omitted from Slack — the * ``task_completed`` block already carries the View PR button, so a * separate "PR opened" message just produces visible duplication @@ -114,6 +126,8 @@ export const NOTIFIABLE_EVENTS = new Set([ 'task_timed_out', 'task_stranded', 'agent_error', + 'approval_requested', + 'approval_stranded', ]); /** diff --git a/cdk/src/stacks/agent.ts b/cdk/src/stacks/agent.ts index 750c178f..5da025e4 100644 --- a/cdk/src/stacks/agent.ts +++ b/cdk/src/stacks/agent.ts @@ -677,6 +677,7 @@ export class AgentStack extends Stack { userPool: taskApi.userPool, taskTable: taskTable.table, taskEventsTable: taskEventsTable.table, + taskApprovalsTable: taskApprovalsTable.table, repoTable: repoTable.table, orchestratorFunctionArn: orchestrator.alias.functionArn, guardrailId: inputGuardrail.guardrailId, diff --git a/cdk/test/constructs/slack-integration.test.ts b/cdk/test/constructs/slack-integration.test.ts index c78b4214..68efe50a 100644 --- a/cdk/test/constructs/slack-integration.test.ts +++ b/cdk/test/constructs/slack-integration.test.ts @@ -134,3 +134,67 @@ describe('SlackIntegration construct', () => { }); }); }); + +describe('SlackIntegration construct — Slack-button approvals wiring (issue #112)', () => { + let template: Template; + + beforeAll(() => { + const app = new App(); + const stack = new Stack(app, 'TestStack'); + + const api = new apigw.RestApi(stack, 'TestApi'); + const userPool = new cognito.UserPool(stack, 'TestUserPool'); + const taskTable = new dynamodb.Table(stack, 'TaskTable', { + partitionKey: { name: 'task_id', type: dynamodb.AttributeType.STRING }, + }); + const taskEventsTable = new dynamodb.Table(stack, 'TaskEventsTable', { + partitionKey: { name: 'task_id', type: dynamodb.AttributeType.STRING }, + sortKey: { name: 'event_id', type: dynamodb.AttributeType.STRING }, + stream: dynamodb.StreamViewType.NEW_IMAGE, + }); + const taskApprovalsTable = new dynamodb.Table(stack, 'TaskApprovalsTable', { + partitionKey: { name: 'task_id', type: dynamodb.AttributeType.STRING }, + sortKey: { name: 'request_id', type: dynamodb.AttributeType.STRING }, + }); + + new SlackIntegration(stack, 'SlackIntegration', { + api, + userPool, + taskTable, + taskEventsTable, + taskApprovalsTable, + }); + + template = Template.fromStack(stack); + }); + + test('interactions handler env wires the approvals + events tables when taskApprovalsTable is provided', () => { + template.hasResourceProperties('AWS::Lambda::Function', { + Environment: { + Variables: Match.objectLike({ + SLACK_USER_MAPPING_TABLE_NAME: Match.anyValue(), + TASK_APPROVALS_TABLE_NAME: Match.anyValue(), + TASK_EVENTS_TABLE_NAME: Match.anyValue(), + TASK_RETENTION_DAYS: Match.anyValue(), + }), + }, + }); + }); + + test('grants the interactions handler read/write on the approvals table', () => { + // The decision core needs Update (decision + rate-limit counter) + // and the transaction touches both tables. + const policies = template.findResources('AWS::IAM::Policy'); + const hasApprovalsGrant = Object.values(policies).some((p) => { + const statements = (p.Properties as { + PolicyDocument: { Statement: Array<{ Action: string | string[]; Resource: unknown }> }; + }).PolicyDocument.Statement; + return statements.some((s) => { + const actions = Array.isArray(s.Action) ? s.Action : [s.Action]; + return actions.some((a) => a.startsWith('dynamodb:')) + && JSON.stringify(s.Resource).includes('TaskApprovalsTable'); + }); + }); + expect(hasApprovalsGrant).toBe(true); + }); +}); diff --git a/cdk/test/handlers/fanout-task-events.test.ts b/cdk/test/handlers/fanout-task-events.test.ts index deecf066..02996923 100644 --- a/cdk/test/handlers/fanout-task-events.test.ts +++ b/cdk/test/handlers/fanout-task-events.test.ts @@ -247,22 +247,21 @@ describe('fanout-task-events: per-channel filter contract (design §6.2)', () => test('every Slack-default event the dispatcher actually renders today is in NOTIFIABLE_EVENTS (issue #64 review Cat 7 drift guard)', () => { // The router subscribes Slack to events the dispatcher must - // render. ``approval_requested``, ``approval_stranded``, and - // ``status_response`` are forward-compat (no Slack-side renderer - // today — the CLI surfaces approval UX; Slack is only in the - // channel-defaults set so a future Slack-button renderer can - // light up without changing the router filter). They're allowed - // to be in CHANNEL_DEFAULTS.slack but absent from - // NOTIFIABLE_EVENTS — when their emitters land, this test will - // start failing and force the dispatcher update at the same time. - // Every OTHER Slack default must be renderable, otherwise - // telemetry lies. Use ``requireActual`` to bypass the + // render. ``status_response`` is forward-compat (no emitter yet); + // it's allowed to be in CHANNEL_DEFAULTS.slack but absent from + // NOTIFIABLE_EVENTS — when its emitter lands, this test will start + // failing and force the dispatcher update at the same time. + // ``approval_requested`` / ``approval_stranded`` graduated from + // forward-compat in issue #112 (Slack-button approvals): they now + // have real renderers in slack-blocks.ts and MUST be in + // NOTIFIABLE_EVENTS. Every other Slack default must be renderable, + // otherwise telemetry lies. Use ``requireActual`` to bypass the // slack-notify mock and read the real exported NOTIFIABLE_EVENTS // set. const real = jest.requireActual( '../../src/handlers/slack-notify', ); - const forwardCompat = new Set(['approval_requested', 'approval_stranded', 'status_response']); + const forwardCompat = new Set(['status_response']); const expectedRenderable = [...CHANNEL_DEFAULTS.slack].filter( e => !forwardCompat.has(e), ); diff --git a/cdk/test/handlers/shared/approval-decision.test.ts b/cdk/test/handlers/shared/approval-decision.test.ts new file mode 100644 index 00000000..684f91ab --- /dev/null +++ b/cdk/test/handlers/shared/approval-decision.test.ts @@ -0,0 +1,228 @@ +/** + * 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. + */ + +/** + * Unit tests for the shared approve/deny decision core (issue #112). + * The HTTP handlers and the Slack interactions handler both delegate + * here; these tests pin the invariants that must hold for EVERY + * surface (the per-handler tests cover surface-specific mapping). + */ + +import { TransactionCanceledException } from '@aws-sdk/client-dynamodb'; +import type { DynamoDBDocumentClient } from '@aws-sdk/lib-dynamodb'; + +jest.mock('@aws-sdk/lib-dynamodb', () => ({ + PutCommand: jest.fn((input: unknown) => ({ _type: 'Put', input })), + TransactWriteCommand: jest.fn((input: unknown) => ({ _type: 'TransactWrite', input })), + UpdateCommand: jest.fn((input: unknown) => ({ _type: 'Update', input })), +})); + +import { + type ApprovalDecisionConfig, + approvalDecisionConfigFromEnv, + processApprovalDecision, +} from '../../../src/handlers/shared/approval-decision'; + +const CONFIG: ApprovalDecisionConfig = { + taskTableName: 'Tasks', + approvalsTableName: 'TaskApprovals', + eventsTableName: 'TaskEvents', + rateLimitPerMinute: 30, + auditRetentionDays: 90, +}; + +const INPUT = { + taskId: 'task-1', + requestId: 'req-1', + callerUserId: 'user-1', +} as const; + +function makeDdb(send: jest.Mock): DynamoDBDocumentClient { + return { send } as unknown as DynamoDBDocumentClient; +} + +describe('processApprovalDecision', () => { + test('approve: rate limit → transaction → audit, returns ok with decidedAt', async () => { + const send = jest.fn().mockResolvedValue({}); + const outcome = await processApprovalDecision(makeDdb(send), CONFIG, { + ...INPUT, + decision: 'approve', + scope: 'this_call', + }); + + expect(outcome.kind).toBe('ok'); + const types = send.mock.calls.map(([cmd]) => (cmd as { _type: string })._type); + expect(types).toEqual(['Update', 'TransactWrite', 'Put']); + + // Rate-limit row shape: shared approve+deny counter namespace. + const rate = send.mock.calls[0][0].input; + expect(rate.Key).toEqual({ + task_id: 'RATE#user-1#APPROVE', + request_id: expect.stringMatching(/^MINUTE#\d{12}$/), + }); + + // Transaction: approval row first (PENDING→APPROVED, ownership- + // guarded), task row second (AWAITING_APPROVAL guard). + const txn = send.mock.calls[1][0].input.TransactItems; + expect(txn[0].Update.ConditionExpression).toContain('user_id = :caller'); + expect(txn[0].Update.ConditionExpression).toContain('#status = :pending'); + expect(txn[0].Update.ExpressionAttributeValues[':decided']).toBe('APPROVED'); + expect(txn[0].Update.ExpressionAttributeValues[':scope']).toBe('this_call'); + expect(txn[1].Update.ConditionExpression).toContain('awaiting_approval_request_id = :rid'); + + // Audit event. + const audit = send.mock.calls[2][0].input.Item; + expect(audit.event_type).toBe('approval_decision_recorded'); + expect(audit.metadata.status).toBe('APPROVED'); + expect(audit.metadata.caller_user_id).toBe('user-1'); + }); + + test('approve without an explicit scope defaults to this_call', async () => { + const send = jest.fn().mockResolvedValue({}); + await processApprovalDecision(makeDdb(send), CONFIG, { ...INPUT, decision: 'approve' }); + const txn = send.mock.calls[1][0].input.TransactItems; + expect(txn[0].Update.ExpressionAttributeValues[':scope']).toBe('this_call'); + }); + + test('deny persists the sanitized reason on the row and the audit', async () => { + const send = jest.fn().mockResolvedValue({}); + const outcome = await processApprovalDecision(makeDdb(send), CONFIG, { + ...INPUT, + decision: 'deny', + sanitizedReason: 'too risky', + }); + expect(outcome.kind).toBe('ok'); + const txn = send.mock.calls[1][0].input.TransactItems; + expect(txn[0].Update.ExpressionAttributeValues[':decided']).toBe('DENIED'); + expect(txn[0].Update.ExpressionAttributeValues[':reason']).toBe('too risky'); + const audit = send.mock.calls[2][0].input.Item; + expect(audit.metadata.status).toBe('DENIED'); + expect(audit.metadata.reason).toBe('too risky'); + }); + + test('rate-limit conditional failure short-circuits before the transaction', async () => { + const limitErr = new Error('limit'); + limitErr.name = 'ConditionalCheckFailedException'; + const send = jest.fn().mockRejectedValueOnce(limitErr); + const outcome = await processApprovalDecision(makeDdb(send), CONFIG, { + ...INPUT, + decision: 'approve', + }); + expect(outcome).toEqual({ kind: 'rate_limited', limit: 30 }); + expect(send).toHaveBeenCalledTimes(1); + }); + + test('approvals-row condition failure collapses to not_found (no oracle — §7.1 finding #6)', async () => { + const send = jest.fn() + .mockResolvedValueOnce({}) // rate limit + .mockRejectedValueOnce(new TransactionCanceledException({ + message: 'cancelled', + CancellationReasons: [{ Code: 'ConditionalCheckFailed' }, { Code: 'None' }], + $metadata: {}, + })); + const outcome = await processApprovalDecision(makeDdb(send), CONFIG, { + ...INPUT, + decision: 'approve', + }); + expect(outcome).toEqual({ kind: 'not_found' }); + // Audit must NOT be written on a failed decision. + expect(send.mock.calls.find(([cmd]) => (cmd as { _type: string })._type === 'Put')).toBeFalsy(); + }); + + test('task-row condition failure maps to not_awaiting', async () => { + const send = jest.fn() + .mockResolvedValueOnce({}) + .mockRejectedValueOnce(new TransactionCanceledException({ + message: 'cancelled', + CancellationReasons: [{ Code: 'None' }, { Code: 'ConditionalCheckFailed' }], + $metadata: {}, + })); + const outcome = await processApprovalDecision(makeDdb(send), CONFIG, { + ...INPUT, + decision: 'deny', + }); + expect(outcome).toEqual({ kind: 'not_awaiting' }); + }); + + test('unexplained transaction cancellation maps to transaction_unknown', async () => { + const send = jest.fn() + .mockResolvedValueOnce({}) + .mockRejectedValueOnce(new TransactionCanceledException({ + message: 'cancelled', + CancellationReasons: [{ Code: 'TransactionConflict' }, { Code: 'None' }], + $metadata: {}, + })); + const outcome = await processApprovalDecision(makeDdb(send), CONFIG, { + ...INPUT, + decision: 'approve', + }); + expect(outcome).toEqual({ kind: 'transaction_unknown' }); + }); + + test('infra errors outside the transaction propagate (caller owns the 500 path)', async () => { + const send = jest.fn() + .mockResolvedValueOnce({}) + .mockRejectedValueOnce(new Error('DDB unavailable')); + await expect(processApprovalDecision(makeDdb(send), CONFIG, { + ...INPUT, + decision: 'approve', + })).rejects.toThrow('DDB unavailable'); + }); + + test('audit write failure does not fail the decision (already committed)', async () => { + const send = jest.fn() + .mockResolvedValueOnce({}) // rate limit + .mockResolvedValueOnce({}) // transaction + .mockRejectedValueOnce(new Error('events table throttled')); // audit + const outcome = await processApprovalDecision(makeDdb(send), CONFIG, { + ...INPUT, + decision: 'approve', + }); + expect(outcome.kind).toBe('ok'); + }); +}); + +describe('approvalDecisionConfigFromEnv', () => { + const saved = { ...process.env }; + afterEach(() => { + process.env = { ...saved }; + }); + + test('reads the standard env vars', () => { + process.env.TASK_TABLE_NAME = 'T'; + process.env.TASK_APPROVALS_TABLE_NAME = 'A'; + process.env.TASK_EVENTS_TABLE_NAME = 'E'; + process.env.APPROVE_RATE_LIMIT_PER_MINUTE = '10'; + process.env.TASK_RETENTION_DAYS = '30'; + expect(approvalDecisionConfigFromEnv()).toEqual({ + taskTableName: 'T', + approvalsTableName: 'A', + eventsTableName: 'E', + rateLimitPerMinute: 10, + auditRetentionDays: 30, + }); + }); + + test('throws when a required table env var is missing', () => { + process.env.TASK_TABLE_NAME = 'T'; + delete process.env.TASK_APPROVALS_TABLE_NAME; + process.env.TASK_EVENTS_TABLE_NAME = 'E'; + expect(() => approvalDecisionConfigFromEnv()).toThrow(/TASK_APPROVALS_TABLE_NAME/); + }); +}); diff --git a/cdk/test/handlers/shared/slack-blocks.test.ts b/cdk/test/handlers/shared/slack-blocks.test.ts index 6fc8eaad..03acb5a2 100644 --- a/cdk/test/handlers/shared/slack-blocks.test.ts +++ b/cdk/test/handlers/shared/slack-blocks.test.ts @@ -202,4 +202,77 @@ describe('renderSlackBlocks', () => { expect(text.length).toBeLessThan(400); expect(text).toContain('...'); }); + + describe('approval_requested (issue #112 — Slack-button approvals)', () => { + const gateMetadata = { + request_id: '01JREQ', + tool_name: 'Bash', + input_preview: 'git push --force', + reason: 'Force push to a protected branch', + severity: 'medium', + timeout_s: 3600, + }; + + test('renders the gate details with Approve/Deny buttons for medium severity', () => { + const msg = renderSlackBlocks('approval_requested', baseTask, gateMetadata); + expect(msg.text).toContain('Approval required'); + const text = sectionText(msg.blocks[0]); + expect(text).toContain(':lock:'); + expect(text).toContain('Bash'); + expect(text).toContain('git push --force'); + expect(text).toContain('Force push'); + expect(text).toContain('medium'); + expect(text).toContain('1h'); + + const actions = actionsBlock(msg.blocks[1]); + expect(actions.elements).toHaveLength(2); + const [approve, deny] = actions.elements; + if (!('action_id' in approve) || !('action_id' in deny)) throw new Error('expected action buttons'); + // action_id carries {task_id}:{request_id} — the interactions + // handler splits on ':' to address the approval row. + expect(approve.action_id).toBe('approve_action:01HXYZ123:01JREQ'); + expect(deny.action_id).toBe('deny_action:01HXYZ123:01JREQ'); + // Both buttons require a confirm dialog (fat-finger guard). + expect(approve.confirm).toBeDefined(); + expect(deny.confirm).toBeDefined(); + }); + + test('renders buttons for low severity', () => { + const msg = renderSlackBlocks('approval_requested', baseTask, { ...gateMetadata, severity: 'low' }); + expect(msg.blocks.some((b) => b.type === 'actions')).toBe(true); + }); + + test('high severity renders NO buttons and points at the CLI (§11.2 finding #4)', () => { + const msg = renderSlackBlocks('approval_requested', baseTask, { ...gateMetadata, severity: 'high' }); + expect(msg.blocks.some((b) => b.type === 'actions')).toBe(false); + const cliHint = sectionText(msg.blocks[1]); + expect(cliHint).toContain('bgagent approve 01HXYZ123 01JREQ'); + }); + + test('missing request_id renders the notification without buttons (nothing to address)', () => { + const { request_id: _rid, ...withoutRequestId } = gateMetadata; + const msg = renderSlackBlocks('approval_requested', baseTask, withoutRequestId); + expect(msg.blocks).toHaveLength(1); + expect(msg.blocks.some((b) => b.type === 'actions')).toBe(false); + }); + + test('long input_preview is truncated', () => { + const msg = renderSlackBlocks('approval_requested', baseTask, { + ...gateMetadata, + input_preview: 'Y'.repeat(500), + }); + const text = sectionText(msg.blocks[0]); + expect(text).toContain('...'); + expect(text.length).toBeLessThan(600); + }); + }); + + test('renders approval_stranded as an informational warning without buttons', () => { + const msg = renderSlackBlocks('approval_stranded', baseTask, { request_id: '01JREQ' }); + expect(msg.text).toContain('stranded'); + const text = sectionText(msg.blocks[0]); + expect(text).toContain(':warning:'); + expect(text).toContain('01JREQ'); + expect(msg.blocks.some((b) => b.type === 'actions')).toBe(false); + }); }); diff --git a/cdk/test/handlers/slack-interactions-approvals.test.ts b/cdk/test/handlers/slack-interactions-approvals.test.ts new file mode 100644 index 00000000..7142a9d8 --- /dev/null +++ b/cdk/test/handlers/slack-interactions-approvals.test.ts @@ -0,0 +1,398 @@ +/** + * 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. + */ + +/** + * Slack-button HITL approval tests for the interactions handler + * (issue #112). Lives in a separate file from + * `slack-interactions.test.ts` because the handler reads + * `TASK_APPROVALS_TABLE_NAME` at module-load time — setting it here + * before the import enables the approval path; the sibling file leaves + * it unset to exercise the not-configured degradation. + */ + +import * as crypto from 'crypto'; +import { TransactionCanceledException } from '@aws-sdk/client-dynamodb'; +import type { APIGatewayProxyEvent } from 'aws-lambda'; + +const ddbSend = jest.fn(); +jest.mock('@aws-sdk/client-dynamodb', () => { + class TransactionCanceledExceptionMock extends Error { + public CancellationReasons?: Array<{ Code?: string }>; + constructor(opts: { message: string; CancellationReasons?: Array<{ Code?: string }> }) { + super(opts.message); + this.name = 'TransactionCanceledException'; + this.CancellationReasons = opts.CancellationReasons; + } + } + return { + DynamoDBClient: jest.fn(() => ({})), + TransactionCanceledException: TransactionCanceledExceptionMock, + }; +}); +jest.mock('@aws-sdk/lib-dynamodb', () => ({ + DynamoDBDocumentClient: { from: jest.fn(() => ({ send: ddbSend })) }, + GetCommand: jest.fn((input: unknown) => ({ _type: 'Get', input })), + UpdateCommand: jest.fn((input: unknown) => ({ _type: 'Update', input })), + PutCommand: jest.fn((input: unknown) => ({ _type: 'Put', input })), + TransactWriteCommand: jest.fn((input: unknown) => ({ _type: 'TransactWrite', input })), +})); + +const smSend = jest.fn(); +jest.mock('@aws-sdk/client-secrets-manager', () => ({ + SecretsManagerClient: jest.fn(() => ({ send: smSend })), + GetSecretValueCommand: jest.fn((input: unknown) => ({ _type: 'GetSecretValue', input })), +})); + +const fetchMock = jest.fn(); +(global as unknown as { fetch: unknown }).fetch = fetchMock; + +process.env.SLACK_SIGNING_SECRET_ARN = 'arn:aws:secretsmanager:us-east-1:123:secret:bgagent/slack/signing-A'; +process.env.TASK_TABLE_NAME = 'Tasks'; +process.env.SLACK_USER_MAPPING_TABLE_NAME = 'SlackMap'; +process.env.TASK_APPROVALS_TABLE_NAME = 'TaskApprovals'; +process.env.TASK_EVENTS_TABLE_NAME = 'TaskEvents'; + +import { invalidateSlackSecretCache } from '../../src/handlers/shared/slack-verify'; +import { handler } from '../../src/handlers/slack-interactions'; + +const SIGNING_SECRET = 'test-signing'; +const TASK_ID = 'task-42'; +const REQUEST_ID = '01JXAPPROVALREQUESTULID00'; + +function sign(body: string, ts: string): string { + return 'v0=' + crypto.createHmac('sha256', SIGNING_SECRET).update(`v0:${ts}:${body}`).digest('hex'); +} + +function makeInteractionEvent(payload: object): APIGatewayProxyEvent { + const body = `payload=${encodeURIComponent(JSON.stringify(payload))}`; + const ts = String(Math.floor(Date.now() / 1000)); + return { + body, + headers: { + 'X-Slack-Signature': sign(body, ts), + 'X-Slack-Request-Timestamp': ts, + }, + multiValueHeaders: {}, + httpMethod: 'POST', + isBase64Encoded: false, + path: '/v1/slack/interactions', + pathParameters: null, + queryStringParameters: null, + multiValueQueryStringParameters: null, + stageVariables: null, + requestContext: {} as APIGatewayProxyEvent['requestContext'], + resource: '', + }; +} + +function interactionPayload(actionId: string, userId = 'U1', teamId = 'T1'): object { + return { + type: 'block_actions', + user: { id: userId, username: 'u', team_id: teamId }, + response_url: 'https://hooks.slack.com/response/xyz', + trigger_id: 't.1', + actions: [{ action_id: actionId, block_id: TASK_ID }], + channel: { id: 'C1' }, + }; +} + +/** Latest body posted to the interaction response_url, or undefined. */ +function lastEphemeralText(): string | undefined { + const call = [...fetchMock.mock.calls].reverse().find(([url]) => { + try { + return new URL(String(url)).hostname === 'hooks.slack.com'; + } catch { + return false; + } + }); + if (!call) return undefined; + return (JSON.parse((call[1] as { body: string }).body) as { text: string }).text; +} + +/** + * Default DDB routing for the happy path: + * - Get on SlackMap → linked platform user + * - Get on TaskApprovals → PENDING low-severity row + * - Update (rate limit) → ok + * - TransactWrite → ok + * - Put (audit) → ok + */ +function configureHappyPath(severity = 'low', platformUserId = 'user-42') { + ddbSend.mockImplementation((cmd: { _type?: string; input: Record }) => { + if (cmd._type === 'Get') { + const table = cmd.input.TableName; + if (table === 'SlackMap') { + return Promise.resolve({ Item: { platform_user_id: platformUserId, status: 'active' } }); + } + if (table === 'TaskApprovals') { + return Promise.resolve({ + Item: { + task_id: TASK_ID, + request_id: REQUEST_ID, + status: 'PENDING', + severity, + user_id: platformUserId, + }, + }); + } + return Promise.resolve({ Item: undefined }); + } + return Promise.resolve({}); + }); +} + +describe('slack-interactions — approve/deny buttons (issue #112)', () => { + beforeEach(() => { + ddbSend.mockReset(); + smSend.mockReset(); + fetchMock.mockReset(); + invalidateSlackSecretCache(process.env.SLACK_SIGNING_SECRET_ARN!); + smSend.mockResolvedValue({ SecretString: SIGNING_SECRET }); + fetchMock.mockResolvedValue({ ok: true, json: () => Promise.resolve({ ok: true }) }); + }); + + test('approve_action records the decision through the shared core and confirms ephemerally', async () => { + configureHappyPath('low'); + const event = makeInteractionEvent(interactionPayload(`approve_action:${TASK_ID}:${REQUEST_ID}`)); + const result = await handler(event); + expect(result.statusCode).toBe(200); + + // The decision transaction ran with the mapped platform user as the + // ownership caller and APPROVED as the target status. + const txn = ddbSend.mock.calls.find(([cmd]) => cmd._type === 'TransactWrite'); + expect(txn).toBeTruthy(); + const items = (txn![0].input as { TransactItems: Array<{ Update: Record }> }).TransactItems; + expect(items[0].Update.Key).toEqual({ task_id: TASK_ID, request_id: REQUEST_ID }); + expect(items[0].Update.ExpressionAttributeValues[':decided']).toBe('APPROVED'); + expect(items[0].Update.ExpressionAttributeValues[':caller']).toBe('user-42'); + // Scope is one-shot from Slack — no scoped blanket approvals via buttons. + expect(items[0].Update.ExpressionAttributeValues[':scope']).toBe('this_call'); + + // Audit event written. + const audit = ddbSend.mock.calls.find(([cmd]) => cmd._type === 'Put'); + expect(audit).toBeTruthy(); + expect((audit![0].input as { Item: { event_type: string } }).Item.event_type) + .toBe('approval_decision_recorded'); + + expect(lastEphemeralText()).toContain('Approved'); + }); + + test('deny_action records DENIED with the Slack-sourced reason', async () => { + configureHappyPath('medium'); + const event = makeInteractionEvent(interactionPayload(`deny_action:${TASK_ID}:${REQUEST_ID}`)); + const result = await handler(event); + expect(result.statusCode).toBe(200); + + const txn = ddbSend.mock.calls.find(([cmd]) => cmd._type === 'TransactWrite'); + expect(txn).toBeTruthy(); + const items = (txn![0].input as { TransactItems: Array<{ Update: Record }> }).TransactItems; + expect(items[0].Update.ExpressionAttributeValues[':decided']).toBe('DENIED'); + expect(items[0].Update.ExpressionAttributeValues[':reason']).toBe('Denied via Slack'); + expect(lastEphemeralText()).toContain('Denied'); + }); + + test('high-severity gate is refused server-side even though the action_id is well-formed (§11.2 finding #4)', async () => { + // The CRITICAL test: buttons never render on high-severity messages, + // but that's UX. A forged/replayed action_id must be stopped by the + // server-side severity re-check before any decision write. + configureHappyPath('high'); + const event = makeInteractionEvent(interactionPayload(`approve_action:${TASK_ID}:${REQUEST_ID}`)); + const result = await handler(event); + expect(result.statusCode).toBe(200); + + expect(ddbSend.mock.calls.find(([cmd]) => cmd._type === 'TransactWrite')).toBeFalsy(); + expect(lastEphemeralText()).toContain('high'); + expect(lastEphemeralText()).toContain('CLI'); + }); + + test('unlinked Slack account cannot decide (told to link first)', async () => { + ddbSend.mockImplementation((cmd: { _type?: string; input: Record }) => { + if (cmd._type === 'Get' && cmd.input.TableName === 'SlackMap') { + return Promise.resolve({ Item: undefined }); + } + return Promise.resolve({}); + }); + const event = makeInteractionEvent(interactionPayload(`approve_action:${TASK_ID}:${REQUEST_ID}`)); + const result = await handler(event); + expect(result.statusCode).toBe(200); + expect(ddbSend.mock.calls.find(([cmd]) => cmd._type === 'TransactWrite')).toBeFalsy(); + expect(lastEphemeralText()).toContain('not linked'); + }); + + test('pending (half-linked) mapping rows are treated as unlinked', async () => { + ddbSend.mockImplementation((cmd: { _type?: string; input: Record }) => { + if (cmd._type === 'Get' && cmd.input.TableName === 'SlackMap') { + return Promise.resolve({ Item: { status: 'pending' } }); + } + return Promise.resolve({}); + }); + const event = makeInteractionEvent(interactionPayload(`deny_action:${TASK_ID}:${REQUEST_ID}`)); + await handler(event); + expect(ddbSend.mock.calls.find(([cmd]) => cmd._type === 'TransactWrite')).toBeFalsy(); + expect(lastEphemeralText()).toContain('not linked'); + }); + + test("non-owner's click fails the ownership condition and reads as not-found (no oracle)", async () => { + configureHappyPath('low', 'mallory'); // mapping resolves, but the row belongs to user-42 + ddbSend.mockImplementation((cmd: { _type?: string; input: Record }) => { + if (cmd._type === 'Get') { + const table = cmd.input.TableName; + if (table === 'SlackMap') { + return Promise.resolve({ Item: { platform_user_id: 'mallory', status: 'active' } }); + } + if (table === 'TaskApprovals') { + return Promise.resolve({ + Item: { task_id: TASK_ID, request_id: REQUEST_ID, status: 'PENDING', severity: 'low', user_id: 'user-42' }, + }); + } + } + if (cmd._type === 'TransactWrite') { + // Ownership condition (user_id = :caller) fails on item 0. + return Promise.reject(new TransactionCanceledException({ + message: 'cancelled', + CancellationReasons: [{ Code: 'ConditionalCheckFailed' }, { Code: 'None' }], + } as ConstructorParameters[0])); + } + return Promise.resolve({}); + }); + + const event = makeInteractionEvent(interactionPayload(`approve_action:${TASK_ID}:${REQUEST_ID}`)); + const result = await handler(event); + expect(result.statusCode).toBe(200); + // Audit must not be written for a failed decision. + expect(ddbSend.mock.calls.find(([cmd]) => cmd._type === 'Put')).toBeFalsy(); + expect(lastEphemeralText()).toContain('not found'); + }); + + test('already-decided gate reads as not-found (same collapse as the HTTP path)', async () => { + ddbSend.mockImplementation((cmd: { _type?: string; input: Record }) => { + if (cmd._type === 'Get') { + const table = cmd.input.TableName; + if (table === 'SlackMap') { + return Promise.resolve({ Item: { platform_user_id: 'user-42', status: 'active' } }); + } + if (table === 'TaskApprovals') { + return Promise.resolve({ + Item: { task_id: TASK_ID, request_id: REQUEST_ID, status: 'APPROVED', severity: 'low', user_id: 'user-42' }, + }); + } + } + if (cmd._type === 'TransactWrite') { + return Promise.reject(new TransactionCanceledException({ + message: 'cancelled', + CancellationReasons: [{ Code: 'ConditionalCheckFailed' }, { Code: 'None' }], + } as ConstructorParameters[0])); + } + return Promise.resolve({}); + }); + + const event = makeInteractionEvent(interactionPayload(`approve_action:${TASK_ID}:${REQUEST_ID}`)); + await handler(event); + expect(lastEphemeralText()).toContain('not found'); + }); + + test('task no longer awaiting approval is reported distinctly', async () => { + ddbSend.mockImplementation((cmd: { _type?: string; input: Record }) => { + if (cmd._type === 'Get') { + const table = cmd.input.TableName; + if (table === 'SlackMap') { + return Promise.resolve({ Item: { platform_user_id: 'user-42', status: 'active' } }); + } + if (table === 'TaskApprovals') { + return Promise.resolve({ + Item: { task_id: TASK_ID, request_id: REQUEST_ID, status: 'PENDING', severity: 'low', user_id: 'user-42' }, + }); + } + } + if (cmd._type === 'TransactWrite') { + return Promise.reject(new TransactionCanceledException({ + message: 'cancelled', + CancellationReasons: [{ Code: 'None' }, { Code: 'ConditionalCheckFailed' }], + } as ConstructorParameters[0])); + } + return Promise.resolve({}); + }); + + const event = makeInteractionEvent(interactionPayload(`approve_action:${TASK_ID}:${REQUEST_ID}`)); + await handler(event); + expect(lastEphemeralText()).toContain('no longer awaiting'); + }); + + test('rate-limit trip is surfaced ephemerally', async () => { + ddbSend.mockImplementation((cmd: { _type?: string; input: Record }) => { + if (cmd._type === 'Get') { + const table = cmd.input.TableName; + if (table === 'SlackMap') { + return Promise.resolve({ Item: { platform_user_id: 'user-42', status: 'active' } }); + } + if (table === 'TaskApprovals') { + return Promise.resolve({ + Item: { task_id: TASK_ID, request_id: REQUEST_ID, status: 'PENDING', severity: 'low', user_id: 'user-42' }, + }); + } + } + if (cmd._type === 'Update') { + const err = new Error('limit'); + err.name = 'ConditionalCheckFailedException'; + return Promise.reject(err); + } + return Promise.resolve({}); + }); + + const event = makeInteractionEvent(interactionPayload(`approve_action:${TASK_ID}:${REQUEST_ID}`)); + await handler(event); + expect(ddbSend.mock.calls.find(([cmd]) => cmd._type === 'TransactWrite')).toBeFalsy(); + expect(lastEphemeralText()).toContain('Rate limit'); + }); + + test('malformed action_id (missing request id) is rejected without any DDB access', async () => { + const event = makeInteractionEvent(interactionPayload('approve_action:task-only')); + const result = await handler(event); + expect(result.statusCode).toBe(200); + expect(ddbSend.mock.calls.find(([cmd]) => cmd._type === 'TransactWrite')).toBeFalsy(); + expect(lastEphemeralText()).toContain('Malformed'); + }); + + test('missing approvals row still goes through the decision core (no existence oracle at the Get)', async () => { + // A Get miss on the approvals row must NOT short-circuit with a + // distinct message — the severity check simply has nothing to gate + // on, and the transaction's attribute_exists condition produces the + // same "not found" as ownership/decided failures. + ddbSend.mockImplementation((cmd: { _type?: string; input: Record }) => { + if (cmd._type === 'Get') { + const table = cmd.input.TableName; + if (table === 'SlackMap') { + return Promise.resolve({ Item: { platform_user_id: 'user-42', status: 'active' } }); + } + return Promise.resolve({ Item: undefined }); + } + if (cmd._type === 'TransactWrite') { + return Promise.reject(new TransactionCanceledException({ + message: 'cancelled', + CancellationReasons: [{ Code: 'ConditionalCheckFailed' }, { Code: 'None' }], + } as ConstructorParameters[0])); + } + return Promise.resolve({}); + }); + + const event = makeInteractionEvent(interactionPayload(`approve_action:${TASK_ID}:${REQUEST_ID}`)); + await handler(event); + expect(lastEphemeralText()).toContain('not found'); + }); +}); diff --git a/docs/design/CEDAR_HITL_GATES.md b/docs/design/CEDAR_HITL_GATES.md index 54134ad7..6b5512a5 100644 --- a/docs/design/CEDAR_HITL_GATES.md +++ b/docs/design/CEDAR_HITL_GATES.md @@ -1514,8 +1514,11 @@ Fan-out dispatch rules (extending Phase 1b stubs): - Lambda validates both tokens, writes `{slack_user_id, cognito_sub, created_at}` to `SlackUserMappingTable` - `MapSlackUserFn` IAM allows `PutItem` only — no admin bulk-write API 3. Slack app does **not** have `chat:write.admin` scope or any workspace-wide write capability. Only the per-user OAuth flow can create mappings. -4. When a Slack button fires: the fan-out Lambda receives the Slack `user_id` → looks up the mapping → obtains `cognito_sub` → impersonates the user for the approve/deny call. The fan-out Lambda's IAM role allows `sts:AssumeRole` into a dedicated `SlackApprovalProxyRole` that restricts approve/deny to ONLY `severity: low` or `severity: medium` approvals. `severity: high` gates **cannot** be approved via Slack — they require a CLI approve with a fresh Cognito JWT. -5. The Slack approval button payload is cryptographically signed (Slack's signing secret) and includes the `task_id`/`request_id`; the fan-out Lambda re-verifies on receipt. +4. When a Slack button fires: the interactions Lambda receives the Slack `user_id` → looks up the mapping → obtains the platform user id → records the decision **as that user**. `severity: high` gates **cannot** be approved via Slack — they require a CLI approve with a fresh Cognito JWT. + + > **Implementation note (issue #112).** The shipped implementation enforces these constraints directly in `SlackInteractionsFn` rather than via the `sts:AssumeRole` proxy this section originally sketched: (a) the severity cap is re-checked **server-side** from the approvals row before any write (the absence of buttons on high-severity messages is UX, not enforcement); (b) the decision runs through the same `processApprovalDecision` core as `ApproveTaskFn`/`DenyTaskFn` (`cdk/src/handlers/shared/approval-decision.ts`), so the ownership condition (`user_id = :caller`) inside the `TransactWriteItems` rejects a linked-but-wrong user at the transaction layer; (c) Slack approvals always record `scope: this_call` — blanket scoped approvals require the CLI. The IAM-role indirection added no enforcement the row conditions don't already provide, at the cost of a cross-role hop. + +5. The Slack approval button payload is cryptographically signed (Slack's signing secret) and includes the `task_id`/`request_id` in the `action_id` (`approve_action:{task_id}:{request_id}`); the interactions Lambda re-verifies the signature on receipt. ```mermaid flowchart TB diff --git a/docs/guides/ROADMAP.md b/docs/guides/ROADMAP.md index 2103180c..d4695d8e 100644 --- a/docs/guides/ROADMAP.md +++ b/docs/guides/ROADMAP.md @@ -77,7 +77,8 @@ What's shipped and what's coming next. - [x] **DDB Stream fanout** - FanOut Consumer Lambda on TaskEventsTable streams (ParallelizationFactor: 1 for per-task ordering) routes events to channel dispatchers - [x] **GitHub edit-in-place** - Single status comment per task on the target PR, edited in place as progress events fire (phase, milestone, cost, link) - [x] **Routable agent milestones** - Named checkpoints (`pr_created`, `nudge_acknowledged`) unwrapped against allowlist for channel filter matching -- [x] **Slack notification dispatcher** - FanOut Block Kit messages for Slack-origin tasks (lifecycle events, threaded replies, terminal dedup, in-thread cancel). Generic fallback text for unmapped event types (e.g. some milestones); richer milestone and approval-gate rendering is follow-up work +- [x] **Slack notification dispatcher** - FanOut Block Kit messages for Slack-origin tasks (lifecycle events, threaded replies, terminal dedup, in-thread cancel). Generic fallback text for unmapped event types (e.g. some milestones); richer milestone rendering is follow-up work +- [x] **Slack-button HITL approvals** - `approval_requested` renders with in-thread ✅ Approve / ❌ Deny buttons for low/medium-severity Cedar gates (high severity stays CLI-only); clicks map the Slack identity to the linked platform user and run the same atomic decision core as `bgagent approve`/`deny` (#112) - [x] **Deploy-preview screenshots** - Listens for GitHub `deployment_status: success` events from any provider (Vercel, Amplify Hosting, Netlify, GitHub Actions); captures the preview URL via AgentCore Browser; posts a markdown image comment on the open PR (and on the linked Linear issue if Linear is configured). Lambda-only, deterministic, ~10–15 s post-deploy. See [Deploy preview screenshots guide](./DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md). - [ ] **Email dispatcher** - Log-only stub; pending SES integration @@ -181,7 +182,7 @@ Planned capabilities, grouped by theme. Items are independent and may ship in an | Capability | Description | |------------|-------------| | **Additional git providers** | GitLab (and optionally Bitbucket). Same workflow, provider-specific API adapters. | -| **Slack notification polish** | Rich Block Kit for `agent_milestone` and `approval_requested` (today many map to generic fallback text); in-thread approve/deny buttons wired to HITL APIs. Should render **Smart progress updates** when that ships. | +| **Slack notification polish** | Rich Block Kit for `agent_milestone` (today many map to generic fallback text). Should render **Smart progress updates** when that ships. (`approval_requested` Block Kit + in-thread approve/deny buttons shipped — see **Slack-button HITL approvals** under What's ready.) | | **Control panel** | Web UI: task list, task detail with logs/traces, cancel, metrics dashboards, cost attribution. Task detail should show manager-style progress alongside raw events/traces. | | **Email notification dispatcher** | SES-based email notifications via the existing fanout pipeline. Log-only stub ships today (see unchecked **Email dispatcher** under What's ready). | | **Per-user notification preferences** | DynamoDB (or equivalent) store for preferred channels, per-channel config, and event filters (`INPUT_GATEWAY.md`). | diff --git a/docs/guides/SLACK_SETUP_GUIDE.md b/docs/guides/SLACK_SETUP_GUIDE.md index b56fc5c3..4bacf130 100644 --- a/docs/guides/SLACK_SETUP_GUIDE.md +++ b/docs/guides/SLACK_SETUP_GUIDE.md @@ -22,6 +22,7 @@ This single command handles everything: deploying the stack (if needed), generat - **@Shoof mentions**: `@Shoof fix the bug in org/repo#42` submits a task. Reactions on your message show progress: :eyes: (received) → :hourglass_flowing_sand: (working) → :white_check_mark: (done) - **DMs**: Message Shoof directly for private task submissions - **Notifications**: Threaded messages show task_created → completed (with PR link, duration, cost). The Cancel button lets you stop a running task. +- **HITL approvals**: When a Cedar approval gate fires, the thread gets an `:lock: Approval required` message with ✅ Approve / ❌ Deny buttons for low/medium-severity gates. Clicking requires a linked account (`bgagent slack link`) and decides the same approval the CLI's `bgagent approve`/`deny` would — high-severity gates are CLI-only by design. - **Multi-workspace**: Each workspace installs via OAuth and gets its own bot token ## Step-by-step setup diff --git a/docs/guides/USER_GUIDE.md b/docs/guides/USER_GUIDE.md index 8b870f20..2e709dd0 100644 --- a/docs/guides/USER_GUIDE.md +++ b/docs/guides/USER_GUIDE.md @@ -714,7 +714,7 @@ When a rule marked `@tier("soft")` matches a tool call: 1. The agent stops before invoking the tool. 2. A row is atomically written to the approvals table and the task status flips to `AWAITING_APPROVAL`. -3. A progress event (`approval_requested`) is emitted so `bgagent watch` shows the gate in real time. +3. A progress event (`approval_requested`) is emitted so `bgagent watch` shows the gate in real time. For Slack-origin tasks the thread also gets an `:lock: Approval required` message — with ✅ Approve / ❌ Deny buttons when the gate is low/medium severity and your Slack account is linked (`bgagent slack link`). High-severity gates are CLI-only. 4. The task waits for your decision up to the rule's timeout (default 300 s, configurable per-rule and per-task). 5. On approval, the agent proceeds; on denial, the deny reason is best-effort injected back into the agent's context so it can adapt; on timeout, the gate is treated as a denial with `timed_out` as the reason. diff --git a/docs/src/content/docs/architecture/Cedar-hitl-gates.md b/docs/src/content/docs/architecture/Cedar-hitl-gates.md index f52c1d8b..e28337af 100644 --- a/docs/src/content/docs/architecture/Cedar-hitl-gates.md +++ b/docs/src/content/docs/architecture/Cedar-hitl-gates.md @@ -1518,8 +1518,11 @@ Fan-out dispatch rules (extending Phase 1b stubs): - Lambda validates both tokens, writes `{slack_user_id, cognito_sub, created_at}` to `SlackUserMappingTable` - `MapSlackUserFn` IAM allows `PutItem` only — no admin bulk-write API 3. Slack app does **not** have `chat:write.admin` scope or any workspace-wide write capability. Only the per-user OAuth flow can create mappings. -4. When a Slack button fires: the fan-out Lambda receives the Slack `user_id` → looks up the mapping → obtains `cognito_sub` → impersonates the user for the approve/deny call. The fan-out Lambda's IAM role allows `sts:AssumeRole` into a dedicated `SlackApprovalProxyRole` that restricts approve/deny to ONLY `severity: low` or `severity: medium` approvals. `severity: high` gates **cannot** be approved via Slack — they require a CLI approve with a fresh Cognito JWT. -5. The Slack approval button payload is cryptographically signed (Slack's signing secret) and includes the `task_id`/`request_id`; the fan-out Lambda re-verifies on receipt. +4. When a Slack button fires: the interactions Lambda receives the Slack `user_id` → looks up the mapping → obtains the platform user id → records the decision **as that user**. `severity: high` gates **cannot** be approved via Slack — they require a CLI approve with a fresh Cognito JWT. + + > **Implementation note (issue #112).** The shipped implementation enforces these constraints directly in `SlackInteractionsFn` rather than via the `sts:AssumeRole` proxy this section originally sketched: (a) the severity cap is re-checked **server-side** from the approvals row before any write (the absence of buttons on high-severity messages is UX, not enforcement); (b) the decision runs through the same `processApprovalDecision` core as `ApproveTaskFn`/`DenyTaskFn` (`cdk/src/handlers/shared/approval-decision.ts`), so the ownership condition (`user_id = :caller`) inside the `TransactWriteItems` rejects a linked-but-wrong user at the transaction layer; (c) Slack approvals always record `scope: this_call` — blanket scoped approvals require the CLI. The IAM-role indirection added no enforcement the row conditions don't already provide, at the cost of a cross-role hop. + +5. The Slack approval button payload is cryptographically signed (Slack's signing secret) and includes the `task_id`/`request_id` in the `action_id` (`approve_action:{task_id}:{request_id}`); the interactions Lambda re-verifies the signature on receipt. ```mermaid flowchart TB diff --git a/docs/src/content/docs/roadmap/Roadmap.md b/docs/src/content/docs/roadmap/Roadmap.md index 8fcaf97c..4d691963 100644 --- a/docs/src/content/docs/roadmap/Roadmap.md +++ b/docs/src/content/docs/roadmap/Roadmap.md @@ -81,7 +81,8 @@ What's shipped and what's coming next. - [x] **DDB Stream fanout** - FanOut Consumer Lambda on TaskEventsTable streams (ParallelizationFactor: 1 for per-task ordering) routes events to channel dispatchers - [x] **GitHub edit-in-place** - Single status comment per task on the target PR, edited in place as progress events fire (phase, milestone, cost, link) - [x] **Routable agent milestones** - Named checkpoints (`pr_created`, `nudge_acknowledged`) unwrapped against allowlist for channel filter matching -- [x] **Slack notification dispatcher** - FanOut Block Kit messages for Slack-origin tasks (lifecycle events, threaded replies, terminal dedup, in-thread cancel). Generic fallback text for unmapped event types (e.g. some milestones); richer milestone and approval-gate rendering is follow-up work +- [x] **Slack notification dispatcher** - FanOut Block Kit messages for Slack-origin tasks (lifecycle events, threaded replies, terminal dedup, in-thread cancel). Generic fallback text for unmapped event types (e.g. some milestones); richer milestone rendering is follow-up work +- [x] **Slack-button HITL approvals** - `approval_requested` renders with in-thread ✅ Approve / ❌ Deny buttons for low/medium-severity Cedar gates (high severity stays CLI-only); clicks map the Slack identity to the linked platform user and run the same atomic decision core as `bgagent approve`/`deny` (#112) - [x] **Deploy-preview screenshots** - Listens for GitHub `deployment_status: success` events from any provider (Vercel, Amplify Hosting, Netlify, GitHub Actions); captures the preview URL via AgentCore Browser; posts a markdown image comment on the open PR (and on the linked Linear issue if Linear is configured). Lambda-only, deterministic, ~10–15 s post-deploy. See [Deploy preview screenshots guide](/using/deploy-preview-screenshots-guide). - [ ] **Email dispatcher** - Log-only stub; pending SES integration @@ -185,7 +186,7 @@ Planned capabilities, grouped by theme. Items are independent and may ship in an | Capability | Description | |------------|-------------| | **Additional git providers** | GitLab (and optionally Bitbucket). Same workflow, provider-specific API adapters. | -| **Slack notification polish** | Rich Block Kit for `agent_milestone` and `approval_requested` (today many map to generic fallback text); in-thread approve/deny buttons wired to HITL APIs. Should render **Smart progress updates** when that ships. | +| **Slack notification polish** | Rich Block Kit for `agent_milestone` (today many map to generic fallback text). Should render **Smart progress updates** when that ships. (`approval_requested` Block Kit + in-thread approve/deny buttons shipped — see **Slack-button HITL approvals** under What's ready.) | | **Control panel** | Web UI: task list, task detail with logs/traces, cancel, metrics dashboards, cost attribution. Task detail should show manager-style progress alongside raw events/traces. | | **Email notification dispatcher** | SES-based email notifications via the existing fanout pipeline. Log-only stub ships today (see unchecked **Email dispatcher** under What's ready). | | **Per-user notification preferences** | DynamoDB (or equivalent) store for preferred channels, per-channel config, and event filters (`INPUT_GATEWAY.md`). | diff --git a/docs/src/content/docs/using/Approval-gates-cedar-hitl.md b/docs/src/content/docs/using/Approval-gates-cedar-hitl.md index daa28cbc..cb32617f 100644 --- a/docs/src/content/docs/using/Approval-gates-cedar-hitl.md +++ b/docs/src/content/docs/using/Approval-gates-cedar-hitl.md @@ -14,7 +14,7 @@ When a rule marked `@tier("soft")` matches a tool call: 1. The agent stops before invoking the tool. 2. A row is atomically written to the approvals table and the task status flips to `AWAITING_APPROVAL`. -3. A progress event (`approval_requested`) is emitted so `bgagent watch` shows the gate in real time. +3. A progress event (`approval_requested`) is emitted so `bgagent watch` shows the gate in real time. For Slack-origin tasks the thread also gets an `:lock: Approval required` message — with ✅ Approve / ❌ Deny buttons when the gate is low/medium severity and your Slack account is linked (`bgagent slack link`). High-severity gates are CLI-only. 4. The task waits for your decision up to the rule's timeout (default 300 s, configurable per-rule and per-task). 5. On approval, the agent proceeds; on denial, the deny reason is best-effort injected back into the agent's context so it can adapt; on timeout, the gate is treated as a denial with `timed_out` as the reason. diff --git a/docs/src/content/docs/using/Slack-setup-guide.md b/docs/src/content/docs/using/Slack-setup-guide.md index 90909790..6f033967 100644 --- a/docs/src/content/docs/using/Slack-setup-guide.md +++ b/docs/src/content/docs/using/Slack-setup-guide.md @@ -26,6 +26,7 @@ This single command handles everything: deploying the stack (if needed), generat - **@Shoof mentions**: `@Shoof fix the bug in org/repo#42` submits a task. Reactions on your message show progress: :eyes: (received) → :hourglass_flowing_sand: (working) → :white_check_mark: (done) - **DMs**: Message Shoof directly for private task submissions - **Notifications**: Threaded messages show task_created → completed (with PR link, duration, cost). The Cancel button lets you stop a running task. +- **HITL approvals**: When a Cedar approval gate fires, the thread gets an `:lock: Approval required` message with ✅ Approve / ❌ Deny buttons for low/medium-severity gates. Clicking requires a linked account (`bgagent slack link`) and decides the same approval the CLI's `bgagent approve`/`deny` would — high-severity gates are CLI-only by design. - **Multi-workspace**: Each workspace installs via OAuth and gets its own bot token ## Step-by-step setup