feat(carve S6): auto-decomposition planning and iteration feedback (dormant)

[isadeks]

Slice 6 of the staged carve. Stacked on #656 (carve/s5-dag-core-channel) — review that first; this PR's diff is only its own commit.

What this adds

Two capabilities, both inert until a later slice wires them up.

Auto-decomposition — plan, review, revise, then run

Turns one plain issue into a reviewable sub-issue graph instead of one giant task.

• decomposition-mode — pure label parsing. The plain trigger label runs an issue as one task (or runs its existing sub-issue graph); …:decompose plans first and waits for approval; …:auto plans and starts immediately. A decompose suffix on an issue that already has sub-issues is a no-op — you can't decompose what's already a graph.
• decomposition-planner — parse and validate the plan a coding/decompose-v1 agent emits as an artifact. Two design choices worth calling out: budget is derived from each piece's S/M/L size rather than asked of the model (so the Σ is a stable, explainable ceiling), and dependency edges are indices into the plan's own list, validated as a DAG before anything is created.
• decomposition-caps — per-project limits. Over-cap rejects with a message and never trims: trimming a graph can silently drop a node others depend on, producing a broken partial result.
• decomposition-render — the proposal comment and every note around it. Deliberately plain English: no "critical path" or "cost ceiling" jargon, the spend figure is framed as the safety limit it is (not a forecast that anchors the reviewer at the ceiling), and each decline path says what actually happened rather than a plausible-sounding stand-in.
• decomposition-store — the pending plan has to survive between the propose webhook and the approve one, so it's one TTL'd row. put is create-once (a redelivery is a no-op); consume is a conditional delete-and-return, so two racing approvals can't both write back.
• decomposition-writeback — create the real sub-issues and their blockedBy relations. Idempotent and resumable: a planned node whose title already exists is reused, so a retry after a partial write-back doesn't double-create.
• decomposition-flow — ties the above into caps → propose-or-seed, plus the approve/reject verdict path. All I/O is injected, so the control flow is testable without Linear or DynamoDB.
• plan-commands / plan-revise / plan-revise-interpret — a reviewer can edit the plan directly (`drop 3`, `merge 1 and 2`) or in prose. Edits apply deterministically to the current plan, and the "What changed" line is a computed before→after diff — never a model self-report, which previously let a dropped piece quietly reappear with a fabricated justification for it.

Iteration feedback

• iteration-heartbeat (+ construct and scheduled sweep) — a long iteration used to show "starting on this" and then nothing at all until it finished. The sweep edits the same maturing reply in place with elapsed time and the latest progress note; it never posts new comments. Eligibility keys on the reply-routing fields, so standalone iterations are covered too, not just orchestrated ones.
• iteration-reply-claim — claim a reply so two concurrent writers converge on one comment instead of overwriting each other.
• failure-reply — failure is answerable. A red build points at the build log by task id (the agent runs the build itself, and the target repo may have no CI at all); an agent crash gives the classified reason, a truncated excerpt, and whether to retry or escalate. Never the raw build output — that's untrusted repo code.
• clarify-resume — resume a task that stopped to ask a question before spending.

Why this is safe to merge early

Every file here is net-new. No handler calls into any of it, the heartbeat construct is not instantiated in any stack, and no schedule is created — the wiring lands in a later slice behind an env-var gate. Synth and unit tests only; no deployed resource or behaviour changes.

Verification

• tsc --noEmit clean
• CDK: 3213 tests / 164 suites passing (up from 2917/150 on the base)
• eslint clean

🤖 Generated with Claude Code

---

Tracking

Slice S6 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.

Stacked on #656 — review that first; this PR's diff is against its branch.

[isadeks]

Automated review — carve S6 (#657)

Reviewed as its own diff only (carve/s5-dag-core-channel..carve/s6-decompose-iteration, 31 files, +8299/−0). Governance (branch naming / no linked issue) is noted once for the whole stack, not repeated here.

Slice standalone-ness: PASS

Typecheck: PASS. tsc --noEmit clean at this slice's tip on its own base (verified by running the compiler against this slice's own install, not inferred). Every symbol it references resolves on the base branch — TaskTable.STATUS_INDEX, makeLinearChannel, upsertThreadedReply, deriveOrchestrationId, validateDag, renderMaturingReply, classifyError/retryGuidance, SubIssueNode are all defined in #656.

Zero deletions, so there is no revert risk in this slice — confirmed via --numstat.

Dormancy claim: CONFIRMED

Worth stating explicitly, because the premise in the PR description does not hold: ORCHESTRATION_TABLE_NAME does not exist anywhere in the tree at this slice, or at #656. It first appears at #659 (reconciler handlers) and only reaches linear-webhook-processor.ts at #662. So dormancy here cannot rest on that env-var gate — it rests entirely on non-wiring. All three checks pass on that basis:

1. No handler imports the new modules. The only cross-file importers of the 15 new shared modules are other files added by this same PR. No pre-existing handler (linear-webhook-processor.ts, orchestration-reconciler.ts, fanout-task-events.ts) references any of them at this slice.
2. No construct is instantiated. IterationHeartbeat does not reach cdk/src/stacks/agent.ts until feat(carve S8): wire the orchestration arc into the stack and activate it #662.
3. Nothing in agent/src imports it — the diff touches no agent/, docs/ or cli/ file at all.

The dormancy claim is accurate. The PR description's reason for it is not.

---

1. MAJOR — a private commit sha is the stated justification for bypassing a prompt-injection guardrail

cdk/src/handlers/shared/orchestration-plan-revise-interpret.ts:46:

it does NOT flow through the task-creation guardrail that screens task_description for PROMPT_ATTACK (the bfc57c5 trap)

bfc57c5 is not an ancestor of upstream/main or of any slice in this stack. It is a private-fork commit, unresolvable to any public reader.

This is not an ordinary shorthand leak. It is the single sentence explaining why a PROMPT_ATTACK screen is deliberately skipped on a path that embeds a user-authored instruction into a model prompt — precisely the sentence a security reviewer needs to be able to follow. State the trap in words ("a plan-revision instruction reaching the planner unscreened would let a reviewer's comment steer the model; it is embedded as delimited data, not instructions") and drop the sha.

2. MINOR — private shorthand T1 used as the reason a code path behaves as it does

orchestration-plan-commands.ts:111 and orchestration-plan-commands.test.ts:59 both reference T1 as if the reader knows it ("preserving T1's revise routing", "The T1 revise phrase must NOT be captured as a command"). T1 appears nowhere else in the tree. Same class as A1-A6/B1-B8/UX.3. The rest of this slice's prose is clean of Mode A/Mode B, K1-K15 and ABCA-###, so this is a small residue of the cleanup pass rather than a systemic gap.

3. MINOR — looksMultiPart conjunction regex cannot match its own intended phrasings

orchestration-decomposition-mode.ts:200 — /\b(and also|as well as|in addition|plus,|;)\b/gi. The trailing \b after a punctuation alternative requires a word character to follow, which inverts the intent for two alternatives. Executed:

input	matches
Build the login form, plus, a signup page	0
Build the login form, plus,a signup page	1
do a; b; c	0
do a;b;c	2
this and also that	1 (correct)

So the natural, correctly-punctuated enumerations score zero while the unnatural no-space variants score. Minor because the consequence is a mis-scored decomposition hint, not a wrong outcome — but the two punctuation alternatives are currently dead weight as written.

4. MINOR — HeartbeatTaskView.latestProgressNote is never populated anywhere in the stack

iteration-heartbeat.ts:79 declares it and planHeartbeat renders a progressNote from it; both the module and construct docs advertise "elapsed + an optional progress note". The only producer of a HeartbeatTaskView in the entire stack is toView() in iteration-heartbeat-sweep.ts, and at #657, #659 and #662 that function never sets the field — nor is there a persisted progress_note-shaped attribute to read. So the progress half of the heartbeat is permanently undefined in production and exercised only by its unit test. Either wire it or drop it before publishing; a documented feature that cannot fire is worse than an absent one.

5. MINOR — stale absolute line reference dangles on this PR's own tree

iteration-heartbeat.ts:69 justifies a load-bearing design claim by citing linear-webhook-processor.ts ~1317. At this slice that file is 463 lines — the cited site does not exist until #662. The one pointer a reader would follow to verify the claim goes nowhere. Line-number references across files are fragile generally; name the function instead.

6. MINOR — DEFAULT_REVISE_MODEL_ID hardcodes a model id, bypassing the repo's own resolver

orchestration-plan-revise-interpret.ts:56 hardcodes us.anthropic.claude-sonnet-4-6, while the base slice already establishes bedrock-models.ts (DEFAULT_BEDROCK_MODEL_IDS + resolveBedrockModelIds) as the single source of truth — its own doc says both grant sites derive from that list "so the two backends can never drift". This is a hand-synced third copy of exactly the value that resolver exists to centralise. Given #654 already contains a live model/grant drift bug in this stack, this is worth fixing on principle.

7. MINOR — new IterationHeartbeat construct ships with no construct test

The slice adds 11 test files, none under cdk/test/constructs/. IterationHeartbeat creates a Lambda, an EventBridge schedule, a grantReadData, and two cdk-nag suppressions, and its own comment makes a least-privilege claim ("No write…") that nothing asserts. The repo convention is a synth assertion per construct (26 such files exist, including closely-analogous scheduled Lambdas). This is the one place in the slice where source and test coverage part company.

---

Reviewed with Claude Code. Verification: tsc --noEmit on this slice's own base; dormancy checked by import-graph and stack-instantiation grep at this tip rather than via the env-var gate; the looksMultiPart regex and the bfc57c5 ancestry claim both checked by execution.

[isadeks]

Rendered user-facing copy — output read, not source

Per the "render it, don't read it" rule, I compiled failure-reply.ts at the stack tip and executed renderFailureReply / renderPanelFailureReason across ~20 input permutations drawn from the classifier's real PATTERNS (not invented error strings). Everything below is quoted rendered output.

Two notes first, because my initial pass got them wrong and rendering is what corrected it:

• classifyError does classify correctly — transient/service/user all resolve, with the right retryGuidance per class. My first probe used raw AWS exception strings (ThrottlingException, AccessDeniedException), which the classifier does not key on; with real ABCA markers it works.
• The [auto-retried] breadcrumb does change the copy. Confirmed:
  • without: …This is usually a temporary infrastructure issue, not a problem with your request — reply here to try again.
  • with: …This looks like a temporary infrastructure issue — I automatically tried again and it still failed. Reply here to retry…

So the classifier arc is sound. What rendering did surface:

---

1. The error excerpt runs into the next sentence — every agent-failure reply

There is no terminating punctuation between the excerpt and see CloudWatch. All 9 realistic classifier cases produce this:

❌ Couldn't start — the compute environment was mid-update — TaskDefinition is inactive see CloudWatch for task `01JXKQ…`. This is usually a temporary infrastructure issue…

❌ Insufficient GitHub permissions — INSUFFICIENT_GITHUB_REPO_PERMISSIONS for repo acme/app see CloudWatch for task `01JXKQ…`. Retrying as-is won't fix this…

❌ Exceeded max turns — agent_status='error_max_turns' see CloudWatch for task `01JXKQ…`. Reply here with any extra guidance…

…is inactive see CloudWatch… reads as one broken sentence. The excerpt is arbitrary-length user/infra text, so it can end in any character. One-line fix: emit ${detail}. (or wrap the excerpt in backticks) so the sentence closes before see CloudWatch.

This is exactly the class of defect that is invisible in source — the template reads fine as `❌ ${title} —${detail} see CloudWatch…` because the space is there; what is missing is a period.

2. A double space and dangling em-dash when the excerpt is empty but present

❌ Unexpected error —  see CloudWatch for task `01JXKQ…`. A retry may not resolve this on its own…

Reproduced from errorMessage: '[auto-retried]' (the marker is stripped, leaving '') and from errorMessage: ' '. Both are reachable: the orchestrator stamps [auto-retried] onto a session-start error, and if the underlying message is empty the reply renders with — followed by nothing. Guard on the post-excerpt() value rather than on input.errorMessage being truthy.

3. A classified reason echoes a secret ARN with the account id into a user-visible tracker comment

❌ Blocked: environmental fault — BLOCKED[secret] resource=arn:aws:secretsmanager:us-east-1:1234:secret:foo see CloudWatch for task `01JXKQ…`. A retry may not resolve this on its own…

The BLOCKED[<kind>] resource=<arn> marker is surfaced verbatim in the excerpt, so an AWS account id and secret name land in a comment on the user's issue. The rest of this module is careful about exactly this ("No raw output dump (untrusted repo code)", "classified reason + task id only, never the raw build output") — the blocker path bypasses that stance. Since blockerClassification already extracts the resource for the remedy, render the name (secret \foo``) rather than the full ARN, or omit the excerpt entirely for the blocker class.

4. Stuttering when the classified title and the excerpt say the same thing

❌ Concurrency limit reached — concurrency limit reached for this account see CloudWatch…

Cosmetic, but it makes the reply look machine-assembled. When the excerpt is a case-insensitive superset of the title, dropping the excerpt would read better.

---

Verified good

The panel renderer (renderPanelFailureReason) is clean across the same permutations, and the integration-node wording genuinely differentiates:

Build/tests failed — see the build log in CloudWatch for task `01JXKQ…`.
Combined build failed after merging the sub-issue branches — see the build log in CloudWatch for task `01JXKQ…`.

It correctly returns null when there is no task id to point at. Build-failure and build-timeout are properly distinguished on the live gating shapes (build_ok=False vs build_ok=timeout), and the timeout copy says the right thing ("didn't finish in time", not "didn't pass").

Why the test suite does not catch #1–#4

failure-reply.test.ts passes (80 tests green across it and orchestration-rollup.test.ts). Every assertion is a fragment-level toMatch on an individual piece:

expect(body).toMatch(/^❌/);
expect(body).toMatch(/build\/tests didn't pass/i);
expect(body).toMatch(/build log in CloudWatch for task `t1`/);
expect(body).toMatch(/reply with guidance/i);

Nothing asserts how the pieces join, which is where all four defects live. A single full-string snapshot per representative case would have caught every one of them.

---

Verification: compiled at the s8 tip in a scratch worktree with the stack's own yarn.lock installed, driven via ts-node; ~20 permutations per renderer (each classifier class, empty/whitespace/marker-only excerpts, 400-char truncation, multiline tracebacks, integration vs non-integration panel, missing task id). All quoted strings are program output.

[isadeks]

Review findings addressed

Thank you for checking dormancy by import graph and stack instantiation rather than taking the PR description's word for it — you are right that the description's stated reason was wrong. ORCHESTRATION_TABLE_NAME does not exist anywhere in the tree at this slice; dormancy here rests entirely on non-wiring. I have corrected the description.

MAJOR #1 — a private commit sha justifying a PROMPT_ATTACK bypass. Fixed. This was the right one to lead with: it is the single sentence a security reviewer needs to follow, and it pointed at a commit that is not an ancestor of anything public. Now stated in words — the instruction is embedded as delimited data rather than instructions, and why that is the boundary.

MINOR #2 — T1. Fixed, along with the wider shorthand sweep across the stack.

MINOR #3 — looksMultiPart conjunction regex cannot match its own phrasings. Confirmed by execution, fixed. The trailing \b after plus, and ; requires a word character next, which inverts the intent — do a; b; c scored 0 while do a;b;c scored 2. Both punctuation alternatives were dead weight as written. Fixed and tested with correctly-spaced input.

MINOR #4 — latestProgressNote is never populated anywhere in the stack. Confirmed; documentation corrected rather than the code. You are right that toView() never sets it at any slice and there is no persisted attribute to read. I have not wired it, because doing so means adding a progress-note write path that belongs with the agent-side progress work, not here. Instead the module and construct docs no longer advertise a progress note as something the heartbeat shows — the field is documented as reserved for a future producer. That keeps the "documented feature that cannot fire" problem you identified from shipping, without inventing a half-feature.

MINOR #5 — stale absolute line reference. Fixed, replaced with the function name. Agreed on the general point; cross-file line numbers were used in several places in this arc and I have removed the ones I found.

MINOR #6 — DEFAULT_REVISE_MODEL_ID hardcodes a model, bypassing the repo's own resolver. Confirmed, not changed here. It is a genuine third copy of a centralised value, and your "worth fixing on principle" framing is fair given the drift bug in this same stack. I have left it because the resolver is a CDK-side construct and this is an agent-invoked classification call whose model is deliberately not the per-task override — pointing it at resolveBedrockModelIds would couple it to the workload model set, which is the opposite of the intent. What it should probably have is its own named constant in the shared models module. Tracking that rather than doing it mid-carve.

MINOR #7 — IterationHeartbeat ships with no construct test. Fixed, following the repo's per-construct synth-assertion convention, including the least-privilege claim its comment makes.

[isadeks]

Second-round review: the regex over-corrected, and my guardrail comment overclaimed

Two findings on my own fixes, both confirmed by execution.

The conjunction regex now false-positived on pasted code. Dropping the trailing \b fixed the punctuation alternatives, but a bare ; with no boundary matches every semicolon — and an issue description written for a coding agent routinely contains a snippet, a stack trace or CSS. Four realistic single-task bug reports flipped to "multi-part":

description	old	with my fix
bug report + 2-line TS snippet	single	multi (false positive)
bug report + CSS paste	single	multi (false positive)
bug report + stack trace	single	multi (false positive)
prose ; + const base = 250;	single	multi (false positive)

Each would have nagged the user to decompose a plain bug report. My own previous commit message argued for avoiding exactly that direction of error, and the new test set couldn't catch it — it only asserted positives, and its one negative case was 73 chars, below the 80-char length floor, so it returned false without ever reaching the conjunction count.

The bare ; is dropped entirely. It never earned its place in either form: with the boundary it matched only unnatural no-space input, without one it matched everything. plus, keeps the boundaryless form, the word alternatives keep theirs, and both real multi-part prose shapes still score; a numbered list is caught by the separate list-item path. A test now pins that a code paste stays single-task.

My guardrail comment asserted a safety property the code did not have — which is worse than the private commit sha it replaced, because a reviewer will trust prose. Three claims failed, all shown by execution:

• the instruction was interpolated raw, so reviewer text containing the """ fence could close the data block and continue at prompt level (the reviewer produced a prompt with 6 fence occurrences and an injected directive sitting at prompt level);
• title/description were type-checked but unbounded — 5000-char values passed;
• the computed diff keys on title identity, so an edit reusing an existing title reports as "modified" rather than "added", and a drop-then-re-add under the same title reports as neither. That was the load-bearing claim in my comment and it is false.

Rather than soften the prose, the first two are now true: the delimiter is neutralized in reviewer text and both the instruction and the returned fields are bounded. The comment now names the real backstop — the downstream guardrail screen at task creation, which the reviewer correctly identified as what actually catches content — and says plainly that the diff is a reviewer-facing summary, not a security control.

All three new guards are mutation-verified.

Verified and not changed: latestProgressNote really has no producer (the RESERVED note is accurate), and the new IterationHeartbeat construct test is not a rubber stamp — flipping grantReadData to grantReadWriteData fails its least-privilege assertion.

[scottschreckengaust]

Verdict: Approve (with non-blocking notes)

Reviewing as a Principal AWS Solutions Architect. This is Slice 6 of the linear-vercel carve, stacked on #656 (carve/s5-dag-core-channel) — I reviewed only the incremental diff vs that base, not code introduced by earlier slices. Every file is net-new and provably dormant: I verified there are no production (non-test) importers of any new module, IterationHeartbeat is instantiated in no stack, and iteration-heartbeat-sweep.ts is referenced by no stack. Synth + unit tests only, no deployed-resource or behaviour change. Given the strong test coverage, the exceptional inline documentation of the design rationale, and the safe-by-construction dormancy, this is a clear approve.

Vision alignment

Strongly aligned with the VISION tenets. Fire-and-forget, escalate by policy: :decompose gates spend behind an explicit @bgagent approve (spend-safe default) while :auto opts out — the precedence rule in parseDecompositionMode deterministically makes the spend-safe choice win on ambiguous label sets. Bounded blast radius & cost: per-project caps (applyPlanCaps) reject-never-trim over-cap plans, budget is derived from S/M/L sizing rather than model-asserted (a stable, explainable ceiling), and the sweep is capped at MAX_EDITS_PER_SWEEP. Reviewable outcomes: the proposal comment, the computed before→after diff, and the idempotent Linear write-back keep the graph inspectable and human-editable. No tenet is traded; the work lands part of #299/#247 (both approved P0) and is tracked by epic #668.

Governance

PASS. Backing issues #299 and #247 both carry approved + P0. The carve/s6-decompose-iteration branch name does not follow (feat|fix|...)/<issue>-..., but per the review contract a carve/* branch is a de-facto-waived nit for the staged carve, not a blocker.

Blocking issues

None.

Non-blocking suggestions / nits

1. Latent wiring gap for single-kind approval via runPlanVerdict (cdk/src/handlers/shared/orchestration-decomposition-flow.ts:106,353). The consumePendingPlan effect is typed as returning only { nodes }, discarding the row's pending_kind and single_task_description. The approve branch then unconditionally calls finalizeWriteBack(..., { nodes: taken.nodes }). For a pending_kind:'single' pending plan (nodes is []), that reaches writeBackPlan which returns {kind:'error', message:'No sub-issues to create.'} — the single-task approval would never run the coding task. This is inert today (dormant, and the reconciler wiring lands in a later slice), so it is not blocking, but the S8 wiring slice must route a single-kind approval down a distinct path (run the persisted single_task_description) rather than through finalizeWriteBack. Consider widening the effect's return type now so the gap can't be missed at wire-up time.

2. looksMultiPart conjunction heuristic (cdk/src/handlers/shared/orchestration-decomposition-mode.ts:213). The regex has been iterated twice in this PR's own commits and is now conservative (word alternatives + boundaryless plus,). It is a pure hint that never changes what runs, so correctness risk is nil; I only flag that a heuristic this fiddly may be worth a small table-driven test matrix of realistic issue bodies (code pastes, stack traces, prose lists) to lock the false-positive boundary — the current tests cover the fixed cases but this class of regex tends to regress.

3. extractJsonObject divergence between the two parsers. The planner's extractor (orchestration-decomposition-planner.ts:221) scans all top-level objects and prefers the last plan-shaped one (robust against prose that quotes CSS/JSON); the interpret extractor (orchestration-plan-revise-interpret.ts:348) takes the first balanced object. The interpret prompt demands "ONE JSON object, no prose", so first-match is acceptable, but the two "same idea, slightly different" implementations are a mild coherence smell — a shared helper would prevent them drifting.

Documentation

Acceptable for a dormant slice. No docs/guides/ or docs/design/ prose changed and no Starlight mirror is touched, so the "Fail build on mutation" step is not at risk. The user-facing label grammar (:decompose/:auto/:help), the caps, and the new env vars will need a guide update when the wiring slice activates them — I'd expect that documentation to land with S8 (activation), not here, and #668 already carries the slice/review guidance. No missing ADR: the design decisions (derived budget, reject-never-trim, deterministic diff, claim protocol) are extensively documented inline and none constitutes a new architectural tenet trade requiring an ADR.

Tests & CI

Strong. Every source module has a matching */test/ file; the PR adds ~4,900 lines of tests against ~4,600 lines of source. Failure paths are covered, not just happy paths (DAG-invalid plans, over-cap rejection, redelivery no-ops, race-losing consume, partial write-back resume, throttle/backoff, claim exhaustion, build-gate vs agent-crash classification). The new IterationHeartbeat construct test asserts the least-privilege claim its own comment makes (read-only on TaskTable; explicitly not PutItem/UpdateItem/DeleteItem/BatchWriteItem) and the schedule/rate — good discipline. CI is green (build (agentcore) pass, secrets/deps/workflow scan pass, dead-code advisory pass). Test-performance guidance (#366): the construct test uses per-test synth() helpers but the suite is tiny (1 construct) and bundling is globally disabled, so no concern.

Bootstrap synth-coverage: not applicable. The diff introduces no new CloudFormation resource types — the heartbeat construct uses Lambda + AWS::Events::Rule (already in resource-action-map.ts:77 → events:PutRule) + a DynamoDB read grant, and the construct is not instantiated in any deployed stack. No cdk/src/bootstrap/* change is required for this PR; the bootstrap bundle must be revisited in the slice that actually instantiates the construct.

Review agents run

The pr-review-toolkit agent plugins were not available as invocable subagents in this environment, so I applied their review lenses directly over the diff and state which lens covered what:

• code-reviewer (guidelines/style) — applied: MIT headers present, L2 constructs, cdk-nag suppressions justified, Node 24 runtime matches the repo norm (44/46 constructs), pure/effect separation is consistent.
• silent-failure-hunter (error handling) — applied: this is the highest-risk surface and it is handled well. writeBackPlan/linearGraphqlFn never throw and surface resumable errors; interpretRevise returns {kind:'error'} on model/parse failure (caller falls back, no silent no-op); the sweep is best-effort and never wedges; releaseReplyClaim bounds retries to prevent the observed 900-iteration spin. No swallowed failures that lose user intent found.
• type-design-analyzer (new types) — applied: discriminated unions (DecompositionResult, PlanCapResult, PlanEdit, ReviseInterpretation, DecompositionFlowResult) are well-modelled and make illegal states unrepresentable; the one nit is the over-narrow consumePendingPlan return (item 1).
• comment-analyzer (comment accuracy) — applied: comments are unusually load-bearing and accurate — the PR's own follow-up commits fixed two over-claiming comments (the guardrail-exception note and a dead regex). The latestProgressNote field is honestly documented as RESERVED/no-producer rather than over-sold.
• pr-test-analyzer (coverage gaps) — applied: see Tests section; coverage is thorough. Only gap is the multi-part heuristic boundary (item 2).
• /security-review — applied (prompt-injection + IAM + secrets surface): the interpret prompt is the sensitive path. It sanitizes the """ delimiter in reviewer text, bounds instruction + returned-field length, validates the model output against a closed edit vocabulary before applying, and correctly names the real backstop (the downstream createTaskCore guardrail screen at task creation) rather than claiming the diff is a security control. IAM is least-privilege (read-only TaskTable, per-workspace secret grant deferred to the stack). No secrets logged. Fail-closed posture throughout.

Human heuristics

• Proportionality — Pass. The module split (mode / planner / caps / render / store / writeback / flow / revise / revise-interpret / plan-commands) maps to genuinely distinct responsibilities with injected I/O, not speculative abstraction; each is independently unit-testable. orchestration-decomposition-render.ts is the one large file (674 lines) but it is a flat catalog of ~30 small render functions, not accreted complexity.
• Coherence — Pass (minor). Same concepts use the same terms across modules (PlannedSubIssue, depends_on indices, SIZE_DEFAULT_BUDGET_USD). The two extractJsonObject variants (item 3) are the only mild parallel-but-not-shared smell.
• Clarity — Pass. Names communicate intent; error handling surfaces failures rather than hiding them behind plausible defaults (the renderPlannerErrorNote path deliberately refuses to falsely claim "single cohesive change"). Magic values are named constants with rationale.
• Appropriateness — Pass. The Linear GraphQL integration encodes real API behaviour verified against linear-feedback.ts (e.g. the IssueRelationType enum-vs-String note at writeback.ts:138 documents a real 400-causing footgun), not just self-written mocks. Tests assert what the code should do (idempotent reuse-by-title, DAG validity, spend-safe precedence), not merely what it does.

---

🤖 Reviewed by Claude (Principal AWS Solutions Architect persona) via the ABCA review_pr workflow.