From 76e1eb1bc64dacd31b360b44c93591d25a34f715 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Fri, 24 Jul 2026 20:59:48 +0100 Subject: [PATCH 1/6] feat(carve S1): foundation contracts for the orchestration arc (additive, dormant) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Slice 1 of the linear-vercel→main carve. Purely additive shared-contract changes that every later orchestration slice references — nothing here is wired or activated, so behavior on main is unchanged. - types.ts: additive optional TaskRecord fields the orchestration/Mode-B/A6 arc reads (orchestration_id, parent_task_id, depends_on, linear_issue_id, code_changed, head_sha, screenshot_url, answer_text, …). All optional → back-compatible. - validation.ts: attachment MIME maps + size constants. Deliberately keeps the max-turns default at main's value (the arc's bump to a higher default is a live behavioral change and belongs in its own PR, not this dormant slice). - workflows.ts: register the read-only planning + restack workflow definitions + the readOnly flag on the workflow type. Registry metadata only — the workflow definitions + planners land in later slices, unused until then. - repo-config.ts: additive optional build_command / lint_command on the repo + blueprint config (per-repo build/lint verification commands the agent runs before opening a PR; default to the platform commands when unset). Gates: cdk compile + eslint + full jest green. No new imports, no runtime wiring, no behavioral change. --- cdk/src/handlers/shared/repo-config.ts | 11 ++++ cdk/src/handlers/shared/types.ts | 78 ++++++++++++++++++++++++++ cdk/src/handlers/shared/validation.ts | 56 ++++++++++++++++-- cdk/src/handlers/shared/workflows.ts | 24 ++++++++ 4 files changed, 164 insertions(+), 5 deletions(-) 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..d0b25b301 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; } @@ -448,7 +463,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 +483,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 }; diff --git a/cdk/src/handlers/shared/workflows.ts b/cdk/src/handlers/shared/workflows.ts index 973b788d7..4cc353a4f 100644 --- a/cdk/src/handlers/shared/workflows.ts +++ b/cdk/src/handlers/shared/workflows.ts @@ -86,6 +86,8 @@ 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', + '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', ]; @@ -128,6 +130,28 @@ const DESCRIPTORS: Record = { readOnly: true, requiredInputs: { allOf: ['pr_number'] }, }, + // A6 re-stack (#305): re-merge a changed predecessor into an existing + // stacked-child PR. Writeable, repo-bound, operates on an existing PR + // (pr_number). Platform-issued (the restack processor), not user-facing. + 'coding/restack-v1': { + id: 'coding/restack-v1', + version: '1.0.0', + requiresRepo: true, + readOnly: false, + requiredInputs: { allOf: ['pr_number'] }, + }, + // #299 Mode B agent-native planning: clone the repo, decide + draft a + // decomposition plan with full repo context, emit it as the artifact. The + // platform seeds sub-issues from the plan (idempotent write-back → Mode A). + // Repo-bound; does not open a PR (readOnly to the repo — it only reads to + // plan). Platform-issued by the Linear webhook on a :decompose/:auto label. + 'coding/decompose-v1': { + id: 'coding/decompose-v1', + version: '1.0.0', + requiresRepo: true, + readOnly: true, + requiredInputs: { oneOf: ['issue_number', 'task_description'] }, + }, 'default/agent-v1': { id: 'default/agent-v1', version: '1.0.0', From 83596f2991f77f3c7df62ba11632c0c12b107fea Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Mon, 27 Jul 2026 16:55:59 +0100 Subject: [PATCH 2/6] test(validation): cover the UTF-8 rejection path for text attachments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The full-buffer UTF-8 check had no test for its failure branch: the existing cases only fed NUL bytes, which the null-byte scan catches on its own, so the `TextDecoder` throw path was never taken. Deleting the rejection entirely left the suite green. Three cases, each of which fails if the check regresses: - bytes that are not decodable UTF-8 but carry no NUL (a truncated multi-byte sequence, and 0xFF which never appears in UTF-8) must be rejected, - a 9 KB ASCII preamble must not launder trailing invalid bytes — this is what the whole-buffer check exists for, and it passes under a prefix-only scan, - legitimate multi-byte text must still be accepted, so the decode can't be "fixed" by rejecting everything non-ASCII. --- cdk/test/handlers/shared/validation.test.ts | 23 +++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/cdk/test/handlers/shared/validation.test.ts b/cdk/test/handlers/shared/validation.test.ts index becf0153b..3e434fe29 100644 --- a/cdk/test/handlers/shared/validation.test.ts +++ b/cdk/test/handlers/shared/validation.test.ts @@ -674,6 +674,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); From 111dcd6cc7b784172210d03a5376164a0dc0692a Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Mon, 27 Jul 2026 17:38:07 +0100 Subject: [PATCH 3/6] fix: don't admit a workflow whose definition ships later MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DESCRIPTORS is the live admission table — create-task-core resolves any submitted workflow_ref against it — so an entry here with no corresponding `agent/workflows/**` file is worse than a missing entry: the submission succeeds with a 201, the caller is told the task started, and the agent then dies when it cannot load the workflow. An accepted task that cannot run is a worse experience than a clean rejection at submission. `coding/restack-v1` and `coding/decompose-v1` were declared here while their YAML lands in a later change, and there is no platform-only submission gate to stop a user asking for them by id. Move both entries to travel with their files. The drift test only walked YAML → descriptor, which is why the orphans were invisible, so add the reverse direction: every declared descriptor must have a shipped workflow file. Verified by re-adding an orphan and watching it fail. --- cdk/node_modules | 1 + cdk/src/handlers/shared/workflows.ts | 22 -------------------- cdk/test/handlers/shared/workflows.test.ts | 24 ++++++++++++++++++++++ 3 files changed, 25 insertions(+), 22 deletions(-) create mode 120000 cdk/node_modules diff --git a/cdk/node_modules b/cdk/node_modules new file mode 120000 index 000000000..f85c76372 --- /dev/null +++ b/cdk/node_modules @@ -0,0 +1 @@ +/private/tmp/abca-s5/cdk/node_modules \ No newline at end of file diff --git a/cdk/src/handlers/shared/workflows.ts b/cdk/src/handlers/shared/workflows.ts index 4cc353a4f..00f45b7ba 100644 --- a/cdk/src/handlers/shared/workflows.ts +++ b/cdk/src/handlers/shared/workflows.ts @@ -130,28 +130,6 @@ const DESCRIPTORS: Record = { readOnly: true, requiredInputs: { allOf: ['pr_number'] }, }, - // A6 re-stack (#305): re-merge a changed predecessor into an existing - // stacked-child PR. Writeable, repo-bound, operates on an existing PR - // (pr_number). Platform-issued (the restack processor), not user-facing. - 'coding/restack-v1': { - id: 'coding/restack-v1', - version: '1.0.0', - requiresRepo: true, - readOnly: false, - requiredInputs: { allOf: ['pr_number'] }, - }, - // #299 Mode B agent-native planning: clone the repo, decide + draft a - // decomposition plan with full repo context, emit it as the artifact. The - // platform seeds sub-issues from the plan (idempotent write-back → Mode A). - // Repo-bound; does not open a PR (readOnly to the repo — it only reads to - // plan). Platform-issued by the Linear webhook on a :decompose/:auto label. - 'coding/decompose-v1': { - id: 'coding/decompose-v1', - version: '1.0.0', - requiresRepo: true, - readOnly: true, - requiredInputs: { oneOf: ['issue_number', 'task_description'] }, - }, 'default/agent-v1': { id: 'default/agent-v1', version: '1.0.0', diff --git a/cdk/test/handlers/shared/workflows.test.ts b/cdk/test/handlers/shared/workflows.test.ts index bd5f1578e..cd2ab7459 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) { From e8e5a1e3b1db5f83c414f9738dbcaccf7fb1d45a Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Mon, 27 Jul 2026 18:40:33 +0100 Subject: [PATCH 4/6] chore: keep a node_modules SYMLINK out of git MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `node_modules/` with a trailing slash matches directories only, so a symlink of that name — e.g. one pointing at a shared install in a sibling worktree — is not ignored and `git add -A` will commit it. CI then fails at install time with `EEXIST: file already exists, mkdir '.../node_modules'`, which reads as a runner cache glitch rather than a tracked file. Match both forms, and drop the one that had been committed. --- .gitignore | 5 +++++ cdk/node_modules | 1 - 2 files changed, 5 insertions(+), 1 deletion(-) delete mode 120000 cdk/node_modules 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/node_modules b/cdk/node_modules deleted file mode 120000 index f85c76372..000000000 --- a/cdk/node_modules +++ /dev/null @@ -1 +0,0 @@ -/private/tmp/abca-s5/cdk/node_modules \ No newline at end of file From b619108a5085f55cc9ccacc8e4c3900ac3b6a143 Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Tue, 28 Jul 2026 19:46:39 +0100 Subject: [PATCH 5/6] docs(validation): say why the two magic-byte functions scan different amounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review raised that `validateMagicBytes` and `detectMimeTypeFromMagicBytes` now disagree about how much of the buffer to look at, with nothing explaining it — a reader is left to guess whether the 8 KB prefix scan in the detector is an oversight. It isn't, and a future "consistency" fix would make things worse. The detector's answer is a guess used to fill in a missing content type, and everything it returns is re-checked by `validateMagicBytes` over the whole buffer one call later. So scanning more bytes there buys no safety, while making it strict would turn a detection miss into a spurious rejection: it would return null for content that validation would have accepted. Also documents that the workflow model allow-list and the Bedrock IAM grant are independent lists, and that a model must be on both. Allow-listing alone lets a workflow pass admission and then fail at turn 0 with AccessDenied, which is a confusing place to learn about it. Adds a test that every bare allow-listed id is paired with its us- inference- profile form, and vice versa. An id admitted in only one form is a latent rejection, since admission compares the literal string a workflow pinned. Verified by deleting one pair and watching the assertion fail. --- cdk/src/handlers/shared/validation.ts | 16 ++++++++++++++++ cdk/src/handlers/shared/workflows.ts | 6 ++++++ cdk/test/handlers/shared/workflows.test.ts | 16 ++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/cdk/src/handlers/shared/validation.ts b/cdk/src/handlers/shared/validation.ts index d0b25b301..1fc1549a4 100644 --- a/cdk/src/handlers/shared/validation.ts +++ b/cdk/src/handlers/shared/validation.ts @@ -378,6 +378,22 @@ 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. + * + * Keep it this way: making this function strict would reject attachments that + * validation would have accepted (it would return `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) { diff --git a/cdk/src/handlers/shared/workflows.ts b/cdk/src/handlers/shared/workflows.ts index 00f45b7ba..9f11719fd 100644 --- a/cdk/src/handlers/shared/workflows.ts +++ b/cdk/src/handlers/shared/workflows.ts @@ -86,6 +86,12 @@ 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', diff --git a/cdk/test/handlers/shared/workflows.test.ts b/cdk/test/handlers/shared/workflows.test.ts index cd2ab7459..51abe0682 100644 --- a/cdk/test/handlers/shared/workflows.test.ts +++ b/cdk/test/handlers/shared/workflows.test.ts @@ -302,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([]); + }); }); From 75a7a25ba604ea76eff8e8a13d477e32010d71aa Mon Sep 17 00:00:00 2001 From: Sphia Sadek Date: Tue, 28 Jul 2026 21:43:37 +0100 Subject: [PATCH 6/6] fix(validation): full-buffer-check inline attachments whose type was detected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the magic-bytes check gated on a DECLARED content_type, so an inline attachment with no declared type — where the type is guessed by detectMimeTypeFromMagicBytes — never got the whole-buffer validation. That leaves the weaker of the two paths unguarded. Detection only scans an 8 KB prefix for NUL bytes, so it is a guess; a declared type at least states an intent. Reproduced before fixing: ~8 KB of clean ASCII followed by 0x00 0xFF 0xFE 0x00, submitted with no content_type, was guessed text/plain and ADMITTED. That is precisely the laundering the UTF-8 hardening was written to stop, reaching the same handlers through the door next to the one that was locked. Now every inline attachment is validated, detected types included. Confirmed this does not cost the detection path its purpose: a real PNG and real JSON with no declared content_type still resolve and pass, so the fix does not trade a laundering hole for spurious rejections. Full suite green with no other test needing a change, which is the useful signal here. Also corrects the comment on the detector. It asserted that whatever detection returns is re-checked whole-buffer by validateMagicBytes — true as a description of intent, false on this path until now. It now records that the re-check is load-bearing and that the call site must not be narrowed back to declared types. Verified by restoring the old gate and watching the laundering assertion fail. --- cdk/src/handlers/shared/validation.ts | 23 ++++++++++++++----- cdk/test/handlers/shared/validation.test.ts | 25 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/cdk/src/handlers/shared/validation.ts b/cdk/src/handlers/shared/validation.ts index 1fc1549a4..70dea4b52 100644 --- a/cdk/src/handlers/shared/validation.ts +++ b/cdk/src/handlers/shared/validation.ts @@ -391,9 +391,16 @@ export function validateMagicBytes(data: Buffer, contentType: string): boolean { * - There the answer is an admission decision with nothing downstream to catch * a mistake, so it must hold for every byte. * - * Keep it this way: making this function strict would reject attachments that - * validation would have accepted (it would return `null` instead of the correct - * type), turning a detection miss into a spurious rejection. + * 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) { @@ -616,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/test/handlers/shared/validation.test.ts b/cdk/test/handlers/shared/validation.test.ts index 3e434fe29..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);