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, restack workflow #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). Deliberately not Closes — no single slice closes it; it closes when S8 activates the arc. The auto-decomposition issue (#299) is no longer part of this carve — that feature stays on the development branch as experimental.

[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@4596b66). 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.15%           
=======================================
  Files           ?      241           
  Lines           ?    59008           
  Branches        ?     5477           
=======================================
  Hits            ?    53199           
  Misses          ?     5809           
  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]

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.

[isadeks]

@scottschreckengaust — your approval at f2d38ff5 was auto-dismissed by the ruleset when I pushed the fix for the finding you left in that same review. Flagging what changed so a re-look is quick rather than a fresh review.

Exactly one commit since your approval: 75a7a25b, which fixes the inline-detected-type finding from your validation.ts:389 thread (now resolved). Nothing else moved — the main merge you reviewed at f2d38ff5 is still the tip beneath it.

I reproduced your case before changing anything: 8192 A bytes + 00 FF FE 00, type: file, no content_type → VALID: true. Admitted, exactly as you described. Dropped && att.content_type at line 620 as you suggested.

Two things I checked that seemed worth more than taking the fix on faith:

• It does not cost the detection path its purpose. A real PNG and real JSON with no declared content_type still resolve and pass. The full 2531-test suite went green with no other test needing a change — nothing in the tree depended on detected types skipping validation.
• Mutation-verified. Restored the old gate; the laundering assertion fails, the valid-detected-type assertion keeps passing.

The comment that claimed the whole-buffer re-check happens was mine from the previous round, and you were right that it was false on that path. It now records that the re-check is load-bearing and that the call site must not be narrowed back to declared-types-only, since that is the shape the bug had.

State: 0 unresolved threads, Secrets, deps, and workflow scan green, build (agentcore) running. This is the front of the 8-slice stack and the only one based on main, so it is the one PR where a push costs the approval — I will not push again before it merges.

[scottschreckengaust]

Verdict: Approve ✅

Re-review at head 75a7a25b. Every substantive concern from the prior three rounds is now resolved in code — including the last one (the inline-attachment-without-content_type re-validation gap). I verified the fix independently and ran all in-scope review agents against this head. This slice is a net security improvement with strong tests, clean governance, and green CI. It belongs.

Prior thread — verified resolved

• Inline detected-type re-validation gap (validation.ts:623): the guard is now if (decoded) (was if (decoded && att.content_type)), so detected MIME types are full-buffer validated too. I traced every path: whenever decoded is truthy, resolvedContentType is always assigned (declared or detected branch) before validateMagicBytes is called — no undefined-type path. The laundering payload (8 KB ASCII + trailing binary, no declared type) is now rejected; genuinely-correct detected PNG/JSON still admit. Both directions are tested.

Vision alignment

Advances the staged carve landing #247/#299 (both approved, P0) under epic #668 (approved, P1). Bounded blast radius respected: the ~11 new TaskRecord fields are optional/back-compatible and provably dormant (they do not cross the toTaskDetail API projection), and the validateMagicBytes change is fail-closed. Slicing keeps outcomes reviewable.

Blocking issues

None.

Non-blocking nits

1. Stale comment example (repeat nit, still unfixed) — cheap to fix here. The JSDoc above SUPPORTED_ATTACHMENT_EXTENSIONS_LABEL says the label is "PNG, JPG, JPEG, TXT, CSV, MD, JSON, PDF, LOG", but the actual runtime value is "PNG, JPG, TXT, CSV, MD, JSON, PDF, LOG, JPEG" — the jpeg alias is spread in last, so JPEG lands at the tail, not position 3. Code is correct; only the example is wrong.
2. No duplicate-extension collision guard. EXTENSION_TO_MIME inverts MIME_TO_EXTENSION via Object.fromEntries; a future second MIME mapping to an existing extension would last-write-win silently with no compile error or test failure. A one-line invariant test (Object.values(MIME_TO_EXTENSION).length === new Set(...).size) would convert that latent drift into a build-time signal.
3. Coupled optionals on TaskRecord (screenshot_url/screenshot_preview_url, code_changed/answer_text) admit illegal combinations. Appropriate for a dormant slice — defer tightening to a discriminated union until the writer slices land; a one-line "tighten when wired in S_n" breadcrumb would preserve the intent.
4. Benign empty-buffer case: validateMagicBytes(Buffer.alloc(0), 'text/plain') returns true. Pre-existing, unreachable via detection, not a binary bypass (empty is valid text). No action needed.

Documentation

No obligation — the diff touches no docs/guides/, docs/design/, or CONTRIBUTING.md. Internal contracts, dormant fields, a validation tightening, and allowlist metadata; no Starlight mirror sync owed. Slice rationale captured in #668. The .gitignore symlink note is accurate.

Tests & CI

CI green (4/4). CDK bootstrap synth-coverage: N/A — no new CloudFormation resource types (additive fields, allowlist strings, derived maps only). New tests are behavioral and cover the real risk paths: UTF-8 continuation-byte reject, 0xFF reject, >8 KB preamble laundering reject, multi-byte UTF-8 accept, detected-path laundering reject + correct-detected-type admit, and the bare↔us. allowlist pairing invariant in both directions.

Review agents run (against head 75a7a25b)

• code-reviewer — routing correct; style consistent; confirmed no cli/src/types.ts sync owed (TaskRecord on CDK_ONLY_ALLOWLIST).
• silent-failure-hunter (security lens) — catch → return false is correct fail-closed; fatal:true decoder rejects overlong-NUL smuggling; resolvedContentType always assigned. No blockers.
• type-design-analyzer — new fields back-compatible; flagged derived-map collision (nit 2) and coupled optionals (nit 3); no API leakage.
• comment-analyzer — 3 of 4 scrutinized comments accurate; caught the label-ordering inaccuracy (nit 1).
• pr-test-analyzer — no blocking coverage gaps; two optional nits.
• /security-review — folded into the admission-path pass; no IAM/Cedar/network/secrets surface touched.
• Omitted: none in scope.

Human heuristics

• Proportionality — pass. Small dormant slice; complexity matches the carve strategy.
• Coherence — pass. Consistent terms; derived maps rather than duplicated lists.
• Clarity — minor concern (nit 1): one JSDoc example is stale. The safety-property comments that failed prior rounds are now backed by the code.
• Appropriateness — pass. Maintainable; tests assert intended contract, not merely current behavior.

Net: safe to merge as part of the stack. Approving; the remaining nit is a trivial comment fix that needn't gate the merge.

[isadeks]

Scope change: auto-decomposition is no longer part of this carve

Heads-up before you re-read this — every slice was force-pushed, so a review in progress is against a stale diff. Sorry for the churn; the reason is a scope decision, not a fix.

Auto-decomposition is staying on the development branch as experimental and is not landing on main. It presupposes one way of working: an issue split into a sub-issue graph, with a human approval gate on a proposed plan. main should not presuppose that. Everything else in this arc is workstyle-neutral — running a graph a human already authored, iterating on a PR from a comment, heartbeats, clarify-resume, the Channel abstraction, the compute plane — and those all stay.

What moved

~7,900 lines removed. 23 dedicated files, the agent workflow and its prompt, the admission-table entry, the CLI flags, and about 314 lines embedded in files that survive.

slice	effect
#647 S1	unchanged
#653 S2	comments now describe artifact workflows by their contract, not by one workflow
#654 S3	workflow YAML, prompt, and admission entry removed
#655 S4	--decompose-allowed / --max-sub-issues / --max-parent-budget-usd removed from linear onboard-project
#656 S5	note-sweep renamed for what it does
#657 S6	8538 → ~2000 lines. Now purely the iteration slice; retitled
#659 S7	graph CREATION removed from the reconciler; graph EXECUTION kept
#662 S8	the plan-proposal surface removed; webhook 4502 → 3156 lines

Three things worth knowing as a reviewer

Some of the review fixes from the last round are gone with the feature. The verdict-marker fix (B1 on #662) and the retry-hint trigger_label fix (#659) were both decomposition-only. They were real fixes and they live on the development branch — they are not being dropped as wrong.

Two things looked decomposition-only and were not. DEFAULT_LABEL_FILTER and hasHelpLabel lived in a decompose-named module but are plain label plumbing, so they moved to trigger-label.ts rather than being deleted. And the note sweep is prefix-based and general, so it was renamed, not removed. Both would have been silent breakage.

Removing the feature exposed dead IAM. The reconciler held an s3:GetObject on the artifacts prefix plus put/delete on the attachments bucket, all for plan seeding. They had no caller left. Removed — a net least-privilege gain, and there is now a test asserting the reconciler has no object grant at all, which is the stronger claim since S3 does not normalize keys.

Verification

• All 8 slices CI-green, chain integrity confirmed (each slice's base is its parent's tip)
• 3436 CDK / 681 CLI / 1436 agent tests at the tip
• Zero decompose references in every slice individually, not just at the tip — an earlier pass looked clean at the tip while intermediate slices still carried them, which would have landed decompose text on main temporarily during the serial merge
• Deployed to dev and live-verified by artifact: the deployed webhook bundle contains no plan surface; coding/decompose-v1 is now rejected at admission (Unknown workflow_ref, 400); a normal coding task still runs end to end (PR opened with the requested file, exact contents); coding/pr-review-v1 still admits; the reconciler's IAM shows no S3 statements