diff --git a/.gitignore b/.gitignore index 4165a1146..6d974c96b 100644 --- a/.gitignore +++ b/.gitignore @@ -38,6 +38,11 @@ junit.xml # Node / JS # ────────────────────────────────────────────── node_modules/ +# Also match a SYMLINK named node_modules. The trailing-slash form above matches +# directories only, so a symlink (e.g. one pointing at a shared install in +# another worktree) slips past it and can be committed by `git add -A`. A +# committed symlink then breaks CI at install time with EEXIST. +node_modules jspm_packages/ .eslintcache *.tgz diff --git a/cdk/src/handlers/shared/repo-config.ts b/cdk/src/handlers/shared/repo-config.ts index 1753d5685..a8303cd98 100644 --- a/cdk/src/handlers/shared/repo-config.ts +++ b/cdk/src/handlers/shared/repo-config.ts @@ -40,6 +40,14 @@ export interface RepoConfig { readonly system_prompt_overrides?: string; readonly github_token_secret_arn?: string; readonly poll_interval_ms?: number; + /** + * Per-repo build/lint verification commands (#1 build-gate fix). The agent + * runs these to gate build/lint regressions before opening a PR; default to + * ``mise run build`` / ``mise run lint`` when unset. Set for non-mise repos + * (e.g. ``npm run build``) so build-regression gating actually works. + */ + readonly build_command?: string; + readonly lint_command?: string; readonly egress_allowlist?: string[]; readonly cedar_policies?: string[]; /** @@ -67,6 +75,9 @@ export interface BlueprintConfig { readonly system_prompt_overrides?: string; readonly github_token_secret_arn?: string; readonly poll_interval_ms?: number; + /** Per-repo build/lint verification commands (#1). Default mise when unset. The orchestrator threads these into the agent payload. */ + readonly build_command?: string; + readonly lint_command?: string; readonly egress_allowlist?: string[]; readonly cedar_policies?: string[]; /** diff --git a/cdk/src/handlers/shared/types.ts b/cdk/src/handlers/shared/types.ts index df58f536d..bb3243e07 100644 --- a/cdk/src/handlers/shared/types.ts +++ b/cdk/src/handlers/shared/types.ts @@ -95,10 +95,38 @@ export interface TaskRecord { readonly agent_heartbeat_at?: string; readonly execution_id?: string; readonly pr_url?: string; + /** + * Public CloudFront URL of the deploy-preview screenshot captured for this + * task's PR (#247). Persisted best-effort by the screenshot pipeline + * (github-webhook-processor) keyed off the taskId in the deploy branch, so + * the orchestration reconciler can embed the INTEGRATION node's combined + * preview in the parent epic panel. Absent until a preview deploys (and for + * tasks with no UI to screenshot). + */ + readonly screenshot_url?: string; + /** + * Live deploy-preview URL the {@link screenshot_url} image was captured from + * (e.g. the Vercel/Netlify preview deploy). Persisted alongside + * ``screenshot_url`` so the orchestration reconciler can make the INTEGRATION + * node's combined preview in the parent epic panel a clickable deep-link to + * the running combined site, not just a static image (#247 UX.17). Absent + * when no preview deployed. + */ + readonly screenshot_preview_url?: string; readonly error_message?: string; readonly idempotency_key?: string; readonly channel_source: ChannelSource; readonly channel_metadata?: Record; + /** + * Linear issue UUID, hoisted to the top level from + * ``channel_metadata.linear_issue_id`` at task-create time (#247 UX.3). + * Top-level because a DynamoDB GSI (``LinearIssueIndex``) cannot key off a + * nested map field — the standalone ``@bgagent`` comment trigger queries + * this index to resolve a plain issue back to its newest ABCA task + PR. + * Present only for Linear-origin tasks; absent for GitHub/Slack/API tasks + * (which keeps the GSI sparse). + */ + readonly linear_issue_id?: string; /** Sparse JiraIssueIndex key (`{cloudId}#{issueKey}`); internal only. */ readonly jira_issue_identity?: string; readonly status_created_at: string; @@ -109,6 +137,21 @@ export interface TaskRecord { readonly cost_usd?: number; readonly duration_s?: number; readonly build_passed?: boolean; + /** + * A6/#299: whether a PR-iteration advanced the branch HEAD (a real commit) vs. + * ran with no change (a question-only ``@bgagent`` comment). Absent for + * pre-fix tasks / non-iterations → the settle reply defaults to "✅ Updated". + */ + readonly code_changed?: boolean; + /** A6/#299: the agent's answer, surfaced on a no-change iteration reply. */ + readonly answer_text?: string; + /** + * The branch HEAD sha this iteration pushed. The screenshot webhook matches a + * deploy's commit sha → the iteration task that pushed it, so the preview + * thumbnail lands on the right reply when iterations overlap on one PR. Absent + * on pre-fix / non-PR tasks → the webhook falls back to the newest reply. + */ + readonly head_sha?: string; /** Whether the post-run lint gate passed (#515). Written with `build_passed` * at terminal state; absent on tasks that predate the field. */ readonly lint_passed?: boolean; @@ -177,6 +220,14 @@ export interface TaskRecord { * record). Absent until the first successful post. */ readonly linear_final_comment_event_id?: string; + /** + * Event ID of the ``pr_created`` milestone whose Linear "PR opened" + * courtesy comment was successfully posted (fan-out plane, ADR-016 P4.5). + * Makes the first-run PR-opened comment post-once across partial-batch + * Lambda retries — the analogue of ``linear_final_comment_event_id`` for + * the mid-run PR notification. Absent until the first successful post. + */ + readonly linear_pr_comment_event_id?: string; /** * Event ID of the terminal event whose Jira final-status comment was * successfully posted (fan-out plane). Jira has no comment edit API, @@ -227,6 +278,33 @@ export interface TaskRecord { * atomically on resume (§10.2, §9). */ readonly awaiting_approval_request_id?: string; + /** + * Linear parent/sub-issue orchestration (issue #247, Mode A). + * ``orchestration_id`` PK of the row in ``OrchestrationTable`` whose + * DAG this task is a child of. Absent on ordinary (non-orchestrated) + * tasks. PR A1 introduces the field; graph discovery (A2) and the + * reconciler (A3) populate and read it. Until then it is always + * ``undefined`` at runtime. + */ + readonly orchestration_id?: string; + /** + * Linear orchestration (#247): the ``task_id`` of the parent task + * for attribution and rollup, when a parent task exists. Absent on + * non-orchestrated tasks and on root children whose parent is the + * Linear issue rather than an ABCA task. Introduced in PR A1; + * unused at runtime until A2/A3. + */ + readonly parent_task_id?: string; + /** + * Linear orchestration (#247): sibling ``sub_issue_id``s this child + * is blocked by — the predecessors that must reach terminal-success + * (``COMPLETED`` with ``build_passed !== false``) before the + * reconciler releases this child. Empty/absent for root children. + * Authoritative gating state lives on the ``OrchestrationTable`` row; + * this is the denormalized copy threaded onto the task record. + * Introduced in PR A1; unused at runtime until A3. + */ + readonly depends_on?: readonly string[]; } /** Per-channel override for one notification channel. See diff --git a/cdk/src/handlers/shared/validation.ts b/cdk/src/handlers/shared/validation.ts index e30405dd8..70dea4b52 100644 --- a/cdk/src/handlers/shared/validation.ts +++ b/cdk/src/handlers/shared/validation.ts @@ -348,11 +348,26 @@ export function validateMagicBytes(data: Buffer, contentType: string): boolean { return sig.bytes.every((b, i) => data[offset + i] === b); } - // Text types: valid UTF-8, no null bytes in first 8 KB + // Text types: the WHOLE buffer must be valid, null-free UTF-8 (review #3 — an + // 8 KB ASCII preamble followed by binary must NOT pass; validate everything, not + // just a prefix). Attachments are already size-capped (MAX_ATTACHMENT_SIZE_BYTES), + // and this runs once at admission, so full-buffer decode is affordable. + // + // What this does and does NOT guarantee: it proves the bytes are decodable UTF-8 + // text with no embedded NULs — enough to safely feed them to the text guardrail + // and store them. It does NOT prove the content is "harmless" — a valid-UTF-8 + // SVG or HTML file labeled .txt passes (it IS text), and is then SCREENED AS TEXT + // by the Bedrock guardrail like any other text attachment. That's the intended + // contract: text is screened as text; binary (non-UTF-8 / NUL-bearing) is rejected. if (contentType.startsWith('text/') || contentType === 'application/json') { - const check = data.subarray(0, TEXT_MAGIC_BYTE_CHECK_BYTES); - for (let i = 0; i < check.length; i++) { - if (check[i] === 0) return false; + try { + // fatal:true throws on any invalid UTF-8 sequence (no U+FFFD substitution). + new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }).decode(data); + } catch { + return false; // not valid UTF-8 end-to-end → not a text attachment + } + for (let i = 0; i < data.length; i++) { + if (data[i] === 0) return false; // embedded NUL → binary, not text } return true; } @@ -363,6 +378,29 @@ export function validateMagicBytes(data: Buffer, contentType: string): boolean { /** * Detect MIME type from magic bytes (for inline attachments without content_type). + * + * This is a DETECTION HEURISTIC, not a validation gate, and it deliberately + * keeps the cheap 8 KB prefix scan that {@link validateMagicBytes} no longer + * uses. The two differ on purpose: + * + * - Here the answer is only a GUESS at what the caller probably sent, used to + * fill in a missing `content_type`. Guessing `text/plain` grants nothing: + * whatever this returns is handed straight to `isAllowedMimeType` and then + * to `validateMagicBytes`, which re-checks the WHOLE buffer. A wrong guess + * is caught one call later, so scanning more bytes here buys no safety. + * - There the answer is an admission decision with nothing downstream to catch + * a mistake, so it must hold for every byte. + * + * That re-check is what makes this safe, so it is load-bearing rather than + * incidental: `validateAttachments` deliberately runs `validateMagicBytes` on + * every inline attachment, DETECTED types included. It used to run only on + * declared ones, which left this path's 8 KB guess as the final word and let + * ~8 KB of ASCII followed by binary through as `text/plain`. Do not narrow that + * call site back to declared types only. + * + * Keep this function itself lenient: making it strict would reject attachments + * validation would have accepted (returning `null` instead of the correct type), + * turning a detection miss into a spurious rejection. */ export function detectMimeTypeFromMagicBytes(data: Buffer): string | null { for (const sig of MAGIC_BYTES) { @@ -448,7 +486,16 @@ function filenameFromUrl(url: string, index: number): string { return `url_attachment_${index}`; } -const MIME_TO_EXTENSION: Record = { +/** + * Canonical MIME → file-extension map for the platform-allowed attachment types. + * This is the single source of truth for the type↔extension relationship: other + * modules that need the reverse (extension → MIME, e.g. to type a generic + * `application/octet-stream` download) derive it from {@link EXTENSION_TO_MIME} + * rather than re-listing the types — so adding a supported type is a one-line + * change here, inherited in both directions. Keep in step with + * ALLOWED_IMAGE_MIME_TYPES / ALLOWED_FILE_MIME_TYPES. + */ +export const MIME_TO_EXTENSION: Record = { 'image/png': 'png', 'image/jpeg': 'jpg', 'text/plain': 'txt', @@ -459,6 +506,28 @@ const MIME_TO_EXTENSION: Record = { 'text/x-log': 'log', }; +/** + * Reverse of {@link MIME_TO_EXTENSION}: file-extension → MIME, for typing a + * download whose HTTP content-type is generic. Derived (not hand-listed) so it + * can never drift from the canonical map. `jpg` → `image/jpeg` covers the common + * `.jpeg` alias too via the extra entry below. + */ +export const EXTENSION_TO_MIME: Readonly> = Object.freeze({ + ...Object.fromEntries(Object.entries(MIME_TO_EXTENSION).map(([mime, ext]) => [ext, mime])), + jpeg: 'image/jpeg', // `.jpeg` is a common alias MIME_TO_EXTENSION collapses to `jpg` +}); + +/** + * Human-friendly list of supported attachment file extensions, derived from + * {@link EXTENSION_TO_MIME}'s KEYS (deduped, upper-cased) — e.g. "PNG, JPG, JPEG, + * TXT, CSV, MD, JSON, PDF, LOG". Keyed off EXTENSION_TO_MIME (not MIME_TO_EXTENSION) + * so the accepted `.jpeg` alias is listed too — the label is exactly the set of + * extensions the type-inference will actually accept. For user-facing + * "unsupported file type" messages, so the list can never drift from the allowlist. + */ +export const SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL: string = + [...new Set(Object.keys(EXTENSION_TO_MIME))].map((e) => e.toUpperCase()).join(', '); + export type AttachmentValidationResult = | { readonly valid: true; readonly parsed: ValidatedAttachment[] } | { readonly valid: false; readonly error: string }; @@ -554,8 +623,14 @@ export function validateAttachments( return { valid: false, error: `attachments[${i}]: content_type is required for presigned uploads` }; } - // Magic bytes check against declared content_type (for inline data with declared type) - if (decoded && att.content_type) { + // Magic-bytes check for ALL inline data, whether the type was DECLARED or + // DETECTED. Gating this on `att.content_type` left the detected path + // unvalidated, which is the weaker of the two: `detectMimeTypeFromMagicBytes` + // only scans an 8 KB prefix for NULs, so ~8 KB of clean ASCII followed by + // binary was guessed `text/plain` and admitted with no whole-buffer check — + // exactly the laundering `validateMagicBytes` was hardened to stop. A + // detected type is a GUESS and needs verifying more than a declared one does. + if (decoded) { if (!validateMagicBytes(decoded, resolvedContentType)) { return { valid: false, error: `attachments[${i}]: content does not match declared type` }; } diff --git a/cdk/src/handlers/shared/workflows.ts b/cdk/src/handlers/shared/workflows.ts index 973b788d7..9f11719fd 100644 --- a/cdk/src/handlers/shared/workflows.ts +++ b/cdk/src/handlers/shared/workflows.ts @@ -86,6 +86,14 @@ export const WORKFLOW_MODEL_ALLOWLIST: readonly string[] = [ 'us.anthropic.claude-sonnet-4-6', 'anthropic.claude-opus-4-20250514-v1:0', 'us.anthropic.claude-opus-4-20250514-v1:0', + // Claude Opus 4.8. Admitting an id here does NOT grant permission to invoke + // it — this list and the IAM grant in `bedrock-models.ts` are independent, and + // a model must be on BOTH to be usable. A workflow pinning an allow-listed but + // un-granted id passes admission and then fails at turn 0 with AccessDenied, + // so keep the two in step: when adding an id here, add it there (or to the + // `bedrockModels` context) in the same change. + 'anthropic.claude-opus-4-8', + 'us.anthropic.claude-opus-4-8', 'anthropic.claude-haiku-4-5-20251001-v1:0', 'us.anthropic.claude-haiku-4-5-20251001-v1:0', ]; diff --git a/cdk/test/handlers/shared/validation.test.ts b/cdk/test/handlers/shared/validation.test.ts index becf0153b..fc4bd8952 100644 --- a/cdk/test/handlers/shared/validation.test.ts +++ b/cdk/test/handlers/shared/validation.test.ts @@ -470,6 +470,31 @@ describe('validateAttachments', () => { const jsonContent = Buffer.from('{"key": "value"}'); const jsonBase64 = jsonContent.toString('base64'); + test('full-buffer-validates an inline attachment whose type was DETECTED, not declared', () => { + // The magic-bytes check used to be gated on a DECLARED content_type, leaving + // the detected path unchecked — the weaker of the two, since a detected type is + // only a guess from an 8 KB prefix scan. So ~8 KB of clean ASCII followed by + // binary was guessed `text/plain` and admitted, laundering non-text bytes past + // the very check that was hardened to catch them. + const launder = Buffer.concat([ + Buffer.from('A'.repeat(8192)), + Buffer.from([0x00, 0xFF, 0xFE, 0x00]), + ]); + const result = validateAttachments([{ type: 'file', data: launder.toString('base64') }]); + expect(result.valid).toBe(false); + if (!result.valid) expect(result.error).toContain('does not match declared type'); + }); + + test('still admits inline content whose DETECTED type is genuinely correct', () => { + // The guard above must not cost the detection path its purpose: a real PNG and + // real JSON with no declared content_type still resolve and pass. Otherwise the + // fix trades a laundering hole for spurious rejections of valid uploads. + const detectedPng = validateAttachments([{ type: 'image', data: pngBase64 }]); + expect(detectedPng.valid).toBe(true); + const detectedJson = validateAttachments([{ type: 'file', data: jsonBase64 }]); + expect(detectedJson.valid).toBe(true); + }); + test('returns valid with empty parsed array for undefined input', () => { const result = validateAttachments(undefined); expect(result.valid).toBe(true); @@ -674,6 +699,29 @@ describe('validateMagicBytes', () => { expect(validateMagicBytes(binary, 'text/plain')).toBe(false); }); + test('rejects a text type whose bytes are not decodable UTF-8', () => { + // 0xC3 starts a 2-byte sequence but 0x28 is not a valid continuation byte, and + // 0xFF never appears in UTF-8 at all. Neither carries a NUL, so the null-byte + // scan below would pass them — only the decode catches this. + expect(validateMagicBytes(Buffer.from([0x48, 0xC3, 0x28]), 'text/plain')).toBe(false); + expect(validateMagicBytes(Buffer.from([0xFF, 0xFE, 0x41]), 'application/json')).toBe(false); + }); + + test('a long ASCII preamble does not launder trailing binary', () => { + // The check used to look at only the first 8 KB, so a benign preamble longer + // than that let arbitrary bytes through behind it. Validate the whole buffer. + const payload = Buffer.concat([ + Buffer.from('A'.repeat(9000)), + Buffer.from([0xC3, 0x28]), + ]); + expect(validateMagicBytes(payload, 'text/plain')).toBe(false); + }); + + test('accepts multi-byte UTF-8 text (not just ASCII)', () => { + // The decode must not reject legitimate non-ASCII text. + expect(validateMagicBytes(Buffer.from('héllo — 世界 🎉', 'utf8'), 'text/plain')).toBe(true); + }); + test('rejects mismatched signatures', () => { const jpeg = Buffer.from([0xFF, 0xD8, 0xFF, 0xE0]); expect(validateMagicBytes(jpeg, 'image/png')).toBe(false); diff --git a/cdk/test/handlers/shared/workflows.test.ts b/cdk/test/handlers/shared/workflows.test.ts index bd5f1578e..51abe0682 100644 --- a/cdk/test/handlers/shared/workflows.test.ts +++ b/cdk/test/handlers/shared/workflows.test.ts @@ -150,6 +150,30 @@ describe('CDK descriptors stay in sync with agent/workflows/**', () => { return out; })(); + // The reverse direction. The check above walks YAML → descriptor, so a + // descriptor with no workflow file is invisible to it. That orphan is worse + // than a missing descriptor: DESCRIPTORS is the live admission table, so a + // submitted ref resolves, the caller gets a 201, and the agent then dies when + // load_workflow can't find the file — an accepted task that cannot run. + test('every CDK descriptor has a shipped workflow file (no orphan admissions)', () => { + const shippedIds = new Set(yamlFiles.map((file) => { + const doc = yaml.load(fs.readFileSync(file, 'utf8')) as Record; + return doc.id as string; + })); + // DESCRIPTORS is intentionally not exported (it is internal to the resolver), + // so read the ids off the source the same way this file already cross-checks + // the agent's own constants. + const workflowsTs = fs.readFileSync( + path.resolve(__dirname, '../../../src/handlers/shared/workflows.ts'), 'utf8', + ); + const table = workflowsTs.slice(workflowsTs.indexOf('const DESCRIPTORS')); + const declaredIds = [...table.matchAll(/^\s{4}id: '([^']+)',$/gm)].map((m) => m[1]); + expect(declaredIds.length).toBeGreaterThan(0); + + const orphans = declaredIds.filter((id) => !shippedIds.has(id)); + expect(orphans).toEqual([]); + }); + test('every shipped workflow file has a matching CDK descriptor', () => { expect(yamlFiles.length).toBeGreaterThan(0); for (const file of yamlFiles) { @@ -278,4 +302,20 @@ describe('disallowedWorkflowModel (WORKFLOWS.md rule 13)', () => { expect(WORKFLOW_MODEL_ALLOWLIST).toContain('anthropic.claude-sonnet-4-6'); expect(WORKFLOW_MODEL_ALLOWLIST).toContain('us.anthropic.claude-sonnet-4-6'); }); + + test('every bare allow-listed id is paired with its us- inference-profile form', () => { + // An id admitted in only one of the two forms is a latent rejection: the + // workflow YAML may legitimately pin either, and admission compares the + // literal string. Pairing them is the invariant, so assert it for every + // entry rather than spot-checking one model. + const bare = WORKFLOW_MODEL_ALLOWLIST.filter(id => !id.startsWith('us.')); + expect(bare.length).toBeGreaterThan(0); + const missing = bare.filter(id => !WORKFLOW_MODEL_ALLOWLIST.includes(`us.${id}`)); + expect(missing).toEqual([]); + // ...and no us- entry is orphaned (its bare form must be admitted too). + const orphaned = WORKFLOW_MODEL_ALLOWLIST + .filter(id => id.startsWith('us.')) + .filter(id => !WORKFLOW_MODEL_ALLOWLIST.includes(id.slice('us.'.length))); + expect(orphaned).toEqual([]); + }); });