feat(carve S1): foundation contracts for the orchestration arc (additive, dormant)

[isadeks]

Slice 1 of a staged carve — foundation contracts

First slice landing the Linear sub-issue orchestration + Mode-B + iteration arc (developed on the linear-vercel line) onto main, in dependency-ordered, independently-reviewable pieces. This slice is mostly additive shared-contract changes that later slices reference. Two exceptions, both flagged in review and both live policy changes on main (see the corrected validation.ts / workflows.ts entries below): the text-attachment validation gate is tightened fail-closed, and the workflow model allow-list is widened. Everything else is unwired and inert.

Contents

• types.ts — additive optional TaskRecord fields the 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, plus a live security tightening of validateMagicBytes (not additive). main scanned only the first 8 KB for NUL bytes and never validated UTF-8 despite its comment claiming it did; text attachments must now decode as valid, NUL-free UTF-8 end to end. Both callers (jira-attachments.ts, resolve-url-attachments.ts) exist on main, so this is a live admission path: content main accepts (an 8 KB ASCII preamble followed by binary) is now rejected. detectMimeTypeFromMagicBytes deliberately keeps the cheap 8 KB prefix scan — it is a detection heuristic whose output validateMagicBytes re-checks in full; a comment there explains why the two differ. (Deliberately keeps the max-turns default unchanged; the arc's bump to a higher default is a live behavioral change that will come as its own PR.)
• workflows.ts — two model allow-list entries only (anthropic.claude-opus-4-8 and its us. inference-profile form). An earlier version of this description claimed it registered the planning/restack workflow definitions and the readOnly flag; that was wrong — those already exist on main. Adding to WORKFLOW_MODEL_ALLOWLIST is itself a live policy widening. Note the allow-list and the Bedrock IAM grant in bedrock-models.ts are independent lists and a model must be on both; the grant for this id lands in S3 (feat(carve S3): agent runtime — deterministic issue feedback, build/lint gate, plan and restack workflows #654), so on S1/S2 the id is admissible but not invocable. Not reachable (the allow-list gates only models a workflow YAML pins, and none pin it on S1–S3) and documented at the list, with a test pinning the bare/us. pairing invariant.
• repo-config.ts — additive optional build_command / lint_command on the repo/blueprint config (per-repo build/lint verification commands; default to the platform commands when unset).

Combination safety

Part of a staged sequence; every later slice is built + verified so that the union of all slices reproduces the deploy-verified linear-vercel tree. This slice's files are byte-identical to that source (comments aside).

Gates

cdk compile + eslint + full jest (135 suites / 2509 tests) green.

🤖 Generated with Claude Code

---

Tracking

Slice S1 of the linear-vercel → main carve. Tracking issue: #668 (slice table, why it is sliced rather than merged, and review guidance).

Lands part of #247 (parent/sub-issue orchestration) and #299 (auto-decomposition). Deliberately not Closes — no single slice closes either; they close when S8 activates the arc.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@32407d5). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #647   +/-   ##
=======================================
  Coverage        ?   90.10%           
=======================================
  Files           ?      241           
  Lines           ?    58995           
  Branches        ?     6321           
=======================================
  Hits            ?    53160           
  Misses          ?     5835           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[isadeks]

Automated review — carve S1 (#647)

Reviewed as its own diff only (main..carve/s1-foundation-contracts, 4 files, +164/−5). This is the base of the eight-PR stack, so I held it to a high bar despite its size. Governance (branch naming / no linked issue) is noted once for the whole stack, not repeated here.

Slice standalone-ness: typecheck PASS, but the "dormant" claim does not fully hold

tsc --noEmit is clean on main as base (verified by running the compiler, not inferred). Three of the four files are genuinely additive-and-dormant: the new optional fields on RepoConfig/BlueprintConfig (build_command, lint_command) and the ten new TaskRecord fields have no reader at this slice, and TaskRecord is on CDK_ONLY_ALLOWLIST so the CDK↔CLI drift gate stays green.

The other two files are not dormant, which is the substance of this review.

---

1. BLOCKING — two workflows are admitted here, but their agent YAML does not exist until #654

This PR adds coding/restack-v1 and coding/decompose-v1 to DESCRIPTORS (cdk/src/handlers/shared/workflows.ts). DESCRIPTORS is the live admission table, not a dormant registry — create-task-core.ts resolves any submitted workflow_ref against it (resolveWorkflowRef → getWorkflowDescriptor(resolvedWorkflow.id)!).

Verified per-slice:

	coding/restack-v1 in DESCRIPTORS	agent/workflows/coding/restack-v1.yaml
main	no	missing
#647 (s1)	yes	missing
#653 (s2)	yes	missing
#654 (s3)	yes	present

Same for coding/decompose-v1. I also checked for a platform-only submission gate — grep for USER_SUBMITTABLE / PLATFORM_ONLY / internalWorkflow / platformOnly across cdk/src at this slice returns nothing, so there is no barrier to a user submitting these ids directly.

Failure: merge #647 or #653 alone and deploy. A user submits workflow_ref: "coding/decompose-v1". Admission succeeds — they get a 201 and are told the task started. The agent then calls load_workflow, which raises WorkflowValidationError: workflow 'coding/decompose-v1' not found at <path>. The task is accepted and then dies, which is a worse user experience than a clean 400 at submission.

Fix: move the two DESCRIPTORS entries to #654 alongside their YAML, or add the YAML here.

2. MAJOR — a live, security-relevant validation tightening ships here with its tests in #655

validation.ts:351 — validateMagicBytes' text branch changes from "scan the first 8 KB for a NUL byte" to "strict whole-buffer TextDecoder({fatal:true}) plus whole-buffer NUL scan". This is the one genuinely non-dormant behaviour change in a PR described as additive and dormant, and it is live on three call paths that already exist on main (jira-attachments.ts, resolve-url-attachments.ts, and validation.ts's own caller).

The validateMagicBytes suite at this slice has 5 cases (PNG, …) and none exercise the new strict-UTF-8 path; the tests for it land in #655. So this PR tightens an input-validation rule on live paths and asserts nothing about the new behaviour. Given it can now reject files it previously accepted (any non-UTF-8 byte anywhere in the buffer, not just a NUL in the first 8 KB), the regression risk is real and untested at this boundary.

3. MAJOR — private work-item shorthand as the stated reason fields exist

The new doc comments carry explanation-bearing private labels that resolve nowhere in the public repo — not at this slice and not at #662:

• types.ts:141, :146 — A6/#299 given as the reason a field exists.
• types.ts:294, :305 — "Introduced in PR A1; unused at runtime until A2/A3". A1/A2/A3/A6 have zero definitions anywhere in the tree.
• Also present in this diff: Mode A/Mode B, UX.3/UX.17, ADR-016 P4.5.

This PR is 4 files — it is the cheapest place in the stack to get this perfect, and it sets the tone for the other seven. Replace with what the field is for ("set when a task is spawned as part of a multi-issue breakdown"), and drop the labels.

4. MINOR — TaskRecord.parent_task_id is never read or written anywhere in the stack

Of the ten TaskRecord fields added here, nine are eventually consumed. parent_task_id is not: at #662 — the final slice — its only occurrence in the entire tree is its own declaration. Its doc comment asserts a purpose ("for attribution and rollup") that nothing implements. Either drop it or wire it; a published contract field that no code touches invites someone to build on it.

5. MINOR — ATTACHMENTS.md still documents the contract this PR replaced

docs/design/ATTACHMENTS.md:465 and its published mirror docs/src/content/docs/architecture/Attachments.md:469 both still read | text/* | Valid UTF-8, no null bytes in first 8 KB |. After finding #2 that row is wrong in both directions, and it is still stale at #662. The code comment was updated; the shipped documentation was not.

6. NIT — WORKFLOW_MODEL_ALLOWLIST gains opus-4-8 entries that stay unreachable through the whole stack

disallowedWorkflowModel() returns early unless a DESCRIPTORS entry sets modelId, and no descriptor sets modelId at any slice (grep -c 'modelId:' on workflows.ts is 0 at s1 through s8), nor does any workflow YAML declare agent_config.model. So these entries are dead for the entire stack. Worth noting because #654 flips the agent's default to opus-4-8 while the Bedrock IAM grant for it does not land until #662 — see the review on #654. The allowlist addition here is harmless; the grant gap there is not.

---

Reviewed with Claude Code. Verification: tsc --noEmit on main as base; the workflow-descriptor gap checked by testing YAML existence at each slice tip and grepping for an admission gate; the agent-side failure mode traced to load_workflow's raise.

[isadeks]

Addressed the Codecov report above. The 2 uncovered lines were the catch { return false; } branch of the new full-buffer UTF-8 check in validateMagicBytes — i.e. the rejection half of the change, which is the security-relevant part.

I confirmed it was a genuine gap rather than a reporting artifact by mutation-testing it: deleting the rejection entirely left all 109 tests passing. The existing cases only fed NUL bytes, which the separate null-byte scan catches on its own, so the TextDecoder throw path was never taken.

Added three cases, each verified to fail when the check is broken:

• bytes that are not decodable UTF-8 but carry no NUL (a truncated 2-byte sequence, and 0xFF which never appears in UTF-8) must be rejected — this is the case the old null-only check let through;
• a 9 KB ASCII preamble must not launder trailing invalid bytes — this is precisely what whole-buffer validation exists for, and it passes under a prefix-only scan;
• legitimate multi-byte text (accents, CJK, emoji) must still be accepted, so the decode can't be "satisfied" by rejecting everything non-ASCII.

Re-running coverage, 367-368 no longer appears in the uncovered list.

Note the remaining uncovered lines in that file are pre-existing and unrelated to this diff. Codecov also flags a missing BASE report for main@32407d5 — the percentages here are patch-relative only.

[isadeks]

Addendum — a committed symlink to the author's local filesystem

Found while reviewing #653 (which deletes it). This is in this PR's diff and I missed it on the first pass.

#647 commits cdk/node_modules as a symlink pointing at an absolute path on the author's machine:

$ git ls-tree upstream/carve/s1-foundation-contracts cdk/node_modules
120000 blob f85c7637…    cdk/node_modules

$ git show upstream/carve/s1-foundation-contracts:cdk/node_modules
/private/tmp/abca-s5/cdk/node_modules

Traced across the stack:

ref	cdk/node_modules
main	absent
#647 (s1)	symlink → /private/tmp/abca-s5/cdk/node_modules
#653 (s2)	deleted
#662 (s8)	absent

Two things follow:

1. It leaks a local path (/private/tmp/abca-s5/…) into a public repo — the abca-s5 scratch directory from the carve work.
2. It is a functional hazard while it exists. Anyone who checks out feat(carve S1): foundation contracts for the orchestration arc (additive, dormant) #647 or builds on it gets a cdk/node_modules symlink into a nonexistent /private/tmp path. Node resolution against a dangling symlink fails in ways that do not obviously point at the cause, and it is exactly the sort of thing that makes "works on my machine" reviews.

.gitignore has node_modules/ with a trailing slash, which matches a directory but not a symlink — that is why this slipped past the ignore rule.

Fix: drop the symlink from this commit. Optionally tighten .gitignore to node_modules (no slash) so the same mistake can't recur. Since #653 already removes it, the stack self-heals from s2 onward — but #647 is the PR that would be merged and deployed first, and it should not carry this.

[isadeks]

Note: this PR was force-pushed during the review — status of each finding against the current head

My review above was written against 76e1eb1b. The current head is 111dcd6c. Restating each finding against the new head so the record is accurate:

finding	status at 111dcd6c
1. BLOCKING — coding/restack-v1 / coding/decompose-v1 admitted before their YAML exists	no longer in this PR — the two DESCRIPTORS entries were removed from #647. But they now land in #653, while the YAML still only arrives in #654, so the defect moved one slice rather than being resolved. Re-raised on #653.
2. MAJOR — validateMagicBytes tightening ships with tests in a later slice	partially addressed — the force-push added 23 lines to cdk/test/handlers/shared/validation.test.ts in this PR. Worth re-checking that the new cases cover the strict whole-buffer UTF-8 path specifically.
3. MAJOR — private shorthand (A6/#299, PR A1, Mode A/B, UX.3) as the reason fields exist	still present
4. MINOR — TaskRecord.parent_task_id never read or written anywhere in the stack	still present
5. MINOR — ATTACHMENTS.md documents the superseded "no null bytes in first 8 KB" contract	still present
6. NIT — WORKFLOW_MODEL_ALLOWLIST opus-4-8 entries unreachable	moot for this PR — workflows.ts changes were removed here
Addendum — committed cdk/node_modules symlink to /private/tmp/abca-s5/…	STILL PRESENT at 111dcd6c. Still the highest-priority item in this PR: it leaks a local path into a public repo and leaves anyone who checks out this slice with a dangling symlink. #653 still deletes it.

Everything else in the original review stands as written.

[isadeks]

Review findings addressed

BLOCKING #1 — two workflows admitted before their definitions exist. Fixed. Confirmed independently: coding/restack-v1 and coding/decompose-v1 were in DESCRIPTORS here and at the next slice, while their agent/workflows/coding/*.yaml only arrive with the agent-runtime slice, and a grep found no platform-only submission gate. Both entries moved to travel with their YAML.

The root cause was the drift test: it walked YAML → descriptor only, so an orphan descriptor was invisible to it. Added the reverse direction — every declared descriptor must have a shipped workflow file — and verified it by re-adding an orphan and watching it fail. That guard is what stops this recurring, not the move.

MAJOR #2 — the UTF-8 tightening shipped without its tests. Fixed (see the Codecov reply above). Three cases added, each mutation-verified to fail when the check is broken. Worth noting the coupling you identified was real in both directions: the later slice had its own equivalent tests, and its version covered a case mine missed (a multi-byte character straddling the old 8 KB cutoff), so I merged both rather than keeping just one.

MAJOR #3 — private work-item shorthand as the stated reason fields exist. Fixed across this slice and the rest of the stack: tracker ids, A1–A6, Mode A/Mode B, UX.n, PM-n and dated incident framing are gone from comments, docstrings, logger messages and test names. The reasoning is preserved in plain language — where a comment recorded a real defect it still records it, just without an id only this team can resolve. Where a private label was the only thing naming a concept, it was replaced with a description rather than deleted.

MINOR #4 — parent_task_id unread across the whole stack. Confirmed, keeping it, flagged as a known gap. It is part of the published record contract for a child task's provenance and the rollup work that consumes it is not in this arc. I would rather not churn the contract twice; if you would prefer it dropped until a reader exists, say so and I will remove it.

MINOR #5 — ATTACHMENTS.md still documents the replaced contract. Correct, and still stale at the last slice. Not fixed here: that file is not in any slice of this carve, so editing it would widen this PR's diff beyond its bucket. Tracking it separately.

NIT #6 — WORKFLOW_MODEL_ALLOWLIST entries unreachable through the stack. Confirmed harmless and left as-is. The related grant gap you pointed at from here was real and is fixed — see #654.

[scottschreckengaust]

Verdict: Approve with nits (posted as COMMENT — no code blockers, but the PR description materially misstates the diff)

Reviewing as a Principal AWS Solutions Architect. This is S1, the base of the 8-slice linear-vercel → main carve (tracking epic #668), a cross-repository (fork) PR from external author @isadeks. The code is clean, well-tested, and safe to land. My only substantive concerns are description/framing accuracy, not defects — hence COMMENT rather than a clean APPROVE.

---

Governance (ADR-003)

Gate is an approved backing issue. The feature issues this slice lands part of — #247 and #299 — both carry approved + P0. The tracking epic #668 is the correct place for the slice rationale. Backing-issue gate: satisfied. Branch name carve/s1-foundation-contracts does not match (feat|fix|chore|docs)/<issue>-..., but per the carve convention this is a de-facto-waived nit, not a blocker.

Vision alignment

Advances bounded blast radius: the slice is dormant-by-design (TypeScript compiles unimported library code; the orchestration path is env-gated in later slices), keeping main deployable while the arc lands reviewably. The validateMagicBytes change is a fail-closed tightening of the input gateway — squarely aligned with the admission-guard tenet. No tenet is traded away.

Blocking issues

None.

Non-blocking suggestions / nits

1. The PR body says behavior on main is unchanged and calls the slice purely additive … nothing is wired or activated — that is not accurate for two of the files, both of which are LIVE paths:

   • cdk/src/handlers/shared/validation.ts — validateMagicBytes is called from jira-attachments.ts and resolve-url-attachments.ts (live admission). Switching text validation from an 8 KB NUL-scan to a whole-buffer TextDecoder({fatal:true}) + full NUL scan is a real, live behavioral change. It is a strict tightening (a long ASCII preamble can no longer launder trailing binary; non-UTF-8 is now rejected), which is good — but it should be described as a live security fix, not a dormant contract. Note also that detectMimeTypeFromMagicBytes (validation.ts:390) still uses the 8 KB prefix scan (TEXT_MAGIC_BYTE_CHECK_BYTES); that is a detection heuristic rather than the validation gate, so it is defensible, but the two now use different rules — worth a one-line comment so a future reader does not "fix" the asymmetry.
   • cdk/src/handlers/shared/workflows.ts — the body claims this file register[s] the read-only planning + restack workflow definitions + the readOnly flag on the workflow type. The actual diff adds only two model-allowlist entries (anthropic.claude-opus-4-8, us.anthropic.claude-opus-4-8); readOnly and all five DESCRIPTORS already exist on main. Adding to WORKFLOW_MODEL_ALLOWLIST is itself a live policy change (it widens the set of models a submitted workflow may pin). Please correct the changelog so reviewers of later slices can trust the per-file descriptions.

2. Model-ID coverage (workflows.ts:89-90). The allowlist adds anthropic. and us.anthropic. forms of claude-opus-4-8 but not a global.anthropic. inference-profile form. If the deployment uses global cross-region inference profiles anywhere (the repo references a global. prefix elsewhere), a pinned global.anthropic.claude-opus-4-8 would be rejected. Confirm the intended prefix set. Non-blocking.

3. repo-config.ts build_command / lint_command. Additive optional fields, fine for a dormant slice. When the orchestrator actually threads these into the agent payload (later slice), the repo-config docs under docs/guides/ should gain these knobs. Flag for the activation slice, not this one.

Documentation

No docs change is required for S1: all additions are optional persistence fields, an internal validation tightening, and allowlist metadata — none change a user-facing command or env var yet. No docs/guides / docs/design / Starlight-mirror edits are in the diff, so there is no mirror-sync obligation to check. The slice rationale is well-captured in #668. OK.

Tests & CI

• Bootstrap synth-coverage: N/A — the diff adds no CDK constructs/stacks/handlers and introduces no new CloudFormation resource types, so the ADR-002 bootstrap-policy checklist does not apply.
• New tests are solid and target the right trees (cdk/test/handlers/shared/): the three validateMagicBytes cases cover the non-UTF-8 reject, the 9 KB-preamble laundering reject, and the multi-byte-UTF-8 accept — i.e. failure paths, not just happy path. The new every CDK descriptor has a shipped workflow file test closes a real orphan-admission gap (accepted task that the agent then cannot run). I verified all five declared descriptor ids resolve to shipped agent/workflows/** YAMLs, so the test passes as written. The id: '…' regex is anchored to 4-space indentation matching the current DESCRIPTORS block — a valid but slightly brittle source-scrape (it breaks if the table is ever reformatted or a nested 4-space id: appears); acceptable given DESCRIPTORS is intentionally unexported.
• I could not execute the jest suites in the review worktree (node_modules is not installed there and aws-cdk-lib/testhelpers/jest-autoclean is absent), so I verified the new tests by static analysis against source rather than by running them. CI on the PR is the authoritative gate.

Review agents run

• code-reviewer (style/guidelines): applied — code matches repo conventions; JSDoc is thorough; no routing issues (changes correctly land in cdk/src/handlers/shared/).
• silent-failure-hunter: applied — the one catch {} in validateMagicBytes deliberately maps a decode failure to return false (fail-closed), which is correct and commented. No swallowed errors.
• type-design-analyzer: applied — new TaskRecord fields are optional and back-compatible; EXTENSION_TO_MIME/SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL are derived from the canonical MIME_TO_EXTENSION (no drift by construction). Confirmed these TaskRecord additions are persistence-only and are not surfaced in any API response, so no cli/src/types.ts sync is owed for this slice.
• comment-analyzer: applied — this is where the PR-body/diff mismatch surfaced (nit #1). The in-file comments themselves are accurate.
• pr-test-analyzer: applied — see Tests section; coverage is appropriate for the live changes.
• /security-review: applied — validation.ts (input gateway) and the model allowlist (policy gate) are in scope. Both changes are fail-closed / tightening; no IAM, Cedar, network, or secrets surface is touched. No findings.

Human heuristics

• Proportionality: pass — small, dormant foundation slice; complexity matches the carve strategy.
• Coherence: concern — the PR description's per-file claims do not match the workflows.ts diff and overstate dormancy for validation.ts (nit #1). The code itself is coherent.
• Clarity: pass — names and JSDoc communicate intent well (orchestration_id, depends_on, head_sha, etc. are precisely documented).
• Appropriateness: pass — maintainable, derives reverse maps rather than duplicating, tests assert intended contract (reject laundered binary) not merely current behavior.

---

Net: safe to merge as part of the stack once the PR body is corrected. Approving in spirit; posting as COMMENT so the description-accuracy nits are addressed before the later, behavior-changing slices lean on this one.

[scottschreckengaust]

Verdict: Approve with nits ✅

Re-review at head b619108a (the branch was reset/rebased and the PR body rewritten since my earlier COMMENT). This slice is a net security improvement with excellent tests and clean governance — it belongs. Two prior threads I raised are resolved (see below), and I found one new in-diff nit worth fixing plus a latent (pre-existing) gap the new comment inadvertently papers over.

Vision alignment

Advances the staged carve landing #247/#299 (both approved, P0) tracked under epic #668 (approved, P1). Bounded blast radius respected: the validateMagicBytes change is fail-closed (tightens admission), and the ~11 new TaskRecord fields are optional/back-compatible and provably dormant. Slicing keeps outcomes reviewable — this is exactly the "reviewable outcomes" tenet in action.

Prior threads — verified resolved

• validation.ts (live admission path): ✅ addressed. PR body now correctly labels this a live security tightening, and the hardening (full-buffer fatal UTF-8 + end-to-end NUL scan) is the right fix, with tests for the invalid-continuation-byte, 0xFF, 9000-byte-preamble laundering, and multi-byte-UTF-8-accept cases.
• workflows.ts (2 allowlist entries only): ✅ addressed. Changelog corrected; readOnly/DESCRIPTORS confirmed already on main. The bare↔us. pairing invariant now has a test in both directions.

Nits (non-blocking)

1. Inaccurate new comment claims a re-validation backstop that doesn't cover one path — validation.ts (added lines ~388–390). The comment states a detected type is "handed straight to isAllowedMimeType and then to validateMagicBytes, which re-checks the WHOLE buffer. A wrong guess is caught one call later." That backstop does not run on the inline-attachment-without-content_type path: validateAttachments gates the full check on if (decoded && att.content_type) (validation.ts:620), so an inline attachment with no declared content_type gets its type from detectMimeTypeFromMagicBytes (8 KB-prefix NUL scan only, validation.ts:406) and is never run through validateMagicBytes. Net effect: the very "8 KB clean ASCII + trailing binary" case the hardening targets can still be admitted on that path (bounded — inline data is ≤500 KB and still guardrail-screened downstream).

   • This gap is pre-existing (line 620 is not in this diff), so it's not a regression and not a blocker on this PR. But the PR's new comment asserts a safety property that isn't true. Suggested fix (cheap, and it makes the comment honest): drop the && att.content_type condition at validation.ts:620 so inline detected types are also full-validated — closes the latent gap and makes the new comment accurate. Otherwise, soften the comment to stop claiming detection results are re-validated.

2. Doc example lists the wrong label order — validation.ts (SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL comment). Comment says "PNG, JPG, JPEG, TXT, ..."; the actual module-load value is "PNG, JPG, TXT, CSV, MD, JSON, PDF, LOG, JPEG" (the jpeg alias is spread in last). Label is computed correctly; only the example is wrong. The new Set(...) dedup is redundant (object keys are already unique) but harmless.

3. Newly-exported symbols have no consumers yet — EXTENSION_TO_MIME, SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL, and the newly-exported MIME_TO_EXTENSION (validation.ts) have zero importers in cdk/src, cli/src, or tests. Documented as scaffolding for later slices; acceptable for a staged carve, but it's speculative surface landing ahead of its code — and the derivation/label have no direct unit test (a future duplicate extension would silently collide in EXTENSION_TO_MIME with nothing to catch it).

4. linear_issue_id doc describes unwired behavior — types.ts documents it as "hoisted to top level ... at task-create time" for a GSI, but live code still reads/writes the nested channel_metadata.linear_issue_id (e.g. orchestrate-task.ts, linear-webhook-processor.ts); the top-level field is neither written nor read in this PR. Fine as forward-declaration, but the doc describes behavior that arrives in a later slice — consider a "wired in S_n" note like the orchestration fields have.

Documentation

No docs/guides or docs/design change required (internal contracts, dormant). .gitignore note (symlink node_modules) is well-explained. No Starlight mirror impact.

Tests & CI

CI green; PR reports 135 suites / 2509 tests. New tests are high-quality and cover the real risk paths (UTF-8 rejection, preamble laundering, multi-byte accept, orphan-descriptor admission, bare↔us. pairing). CDK bootstrap synth-coverage: not applicable — no new CloudFormation resource types (additive interface fields, allowlist strings, derived maps only). Minor gaps: empty-buffer text (currently returns true) and EXTENSION_TO_MIME/label are untested.

Review agents run

• code-reviewer / silent-failure-hunter (security lens on the admission path): run — surfaced the nit-1 re-validation gap; confirmed the decode is bounded by MAX_ATTACHMENT_SIZE_BYTES (size checks are upstream in both hydration callers) and fail-closed.
• type-design-analyzer: run — new optional fields are back-compatible; flagged depends_on self-admitted dup-of-OrchestrationTable (dormant) and the coupled-optionals (screenshot_url/_preview_url, code_changed/answer_text) not expressed as invariants.
• pr-test-analyzer: run — coverage assessed above.
• comment-analyzer: folded into nits 1, 2, 4 (comment accuracy).
• /security-review: covered by the admission-path pass above (no IAM/Cedar/network change in this diff).
• Omitted: none in scope.

Human heuristics

• Proportionality — pass. Change matches the problem; no over-abstraction.
• Coherence — pass. Fields/maps use consistent repo terms; parallel structure has real substance (tests, not boilerplate).
• Clarity — concern (nit 1): a security-path comment asserts a backstop that doesn't hold on one path. Names are otherwise clear.
• Appropriateness — pass. Maintainable; tests assert intended behavior (whole-buffer validation), not just current output.

[scottschreckengaust]

The re-validation this comment promises does not run on the inline-attachment-without-content_type path: validateAttachments gates validateMagicBytes on if (decoded && att.content_type) (validation.ts:620), so a detected (guessed) type is never re-checked whole-buffer. An inline payload of ~8 KB clean ASCII + trailing binary (no declared content_type) is guessed text/plain here (8 KB-prefix NUL scan only, line 406) and admitted without the full-buffer check — the exact laundering case the hardening targets. Pre-existing (line 620 is not in this diff), so non-blocking, but the comment claims a safety property that is not true on that path. Cheap fix: drop && att.content_type at line 620 so inline detected types are full-validated too, which also makes this comment accurate.

[scottschreckengaust]

Follow-up: cross-stack check of the new exports + the nit-1 validation gap (per reviewer request).

I traced how the S1 additions are used by the downstream stacked PRs (checked at the current #662 tip). Two things worth recording:

1. The new exports are real forward-declarations, not idle scaffolding. EXTENSION_TO_MIME and SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL are consumed by cdk/src/handlers/shared/linear-attachments.ts (introduced in S4 / feat(carve S4): Linear issue-context surface — attachments, PDF/image screening, auth health #655): the label backs the user-facing "unsupported file type (supported: …)" reject message (linear-attachments.ts:566), and EXTENSION_TO_MIME backs the text-only label→MIME fallback in inferMime (linear-attachments.ts:705). So landing them dormant in S1 is justified — the consumer arrives in the same arc. My "no consumers yet" nit stands only for the S1↔S3 window.

2. Good news on nit-1: the downstream consumer does NOT inherit the gap. My nit-1 flagged that validateAttachments skips validateMagicBytes on the inline-attachment-without-content_type path (gated by if (decoded && att.content_type) at validation.ts:620). The S4 Linear path does not route through that gate — linear-attachments.ts:572 calls validateMagicBytes(content, mimeType) unconditionally after resolving the type via inferMime (which treats bytes as authoritative and only trusts a text content-type/label, never promoting to a binary type). So the S1 whole-buffer UTF-8/NUL hardening pays off exactly as intended on the Linear surface regardless of a declared content_type.

Net: nit-1 remains a pre-existing, non-blocking gap localized to the legacy validateAttachments inline path; it does not propagate into the new stacked code. The cheap fix (drop && att.content_type at validation.ts:620) is still worth doing to (a) close that legacy path and (b) make the S1 comment on detectMimeTypeFromMagicBytes accurate — but it is not a blocker for this PR or the stack.