Skip to content

feat(carve S7): the orchestration compute plane — reconciler, release, rollup - #659

Open
isadeks wants to merge 3 commits into
carve/s6-decompose-iterationfrom
carve/s7-orchestration-plane
Open

feat(carve S7): the orchestration compute plane — reconciler, release, rollup#659
isadeks wants to merge 3 commits into
carve/s6-decompose-iterationfrom
carve/s7-orchestration-plane

Conversation

@isadeks

@isadeks isadeks commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Slice 7 of the staged carve. Stacked on #657 (carve/s6-decompose-iteration) — review that first; this PR's diff is only its own commit.

What this adds

The engine that actually runs a sub-issue graph. It watches child tasks reach a terminal state, releases whatever they unblocked, keeps the parent's status panel current, and re-stacks dependents when a branch moves.

  • orchestration-discovery — the composer that turns a trigger into a seeded run: read the graph, validate it, append the integration node if it fans out, persist it, and hand back the root set to start.
  • orchestration-reconcile — pure gating. Given a child that just went terminal, decide which dependents are now releasable, which must be skipped, and when the parent itself is done. Two recovery shapes are covered too: comment-fixing a failed child un-fails it and re-releases the dependents that were skipped behind it, and re-triggering a finished epic retries only its failed/skipped children.
  • orchestration-release — create a child's task, behind a claim so two concurrent releasers can't both launch it. A guardrail rejection or an un-onboarded repo is terminal with a reason the user can act on; a 5xx or a duplicate replay rolls back to ready so the sweep retries.
  • orchestration-rollup — the parent's single maturing panel: one comment that matures in place rather than a stream of updates. A failed row carries an indented sub-line saying what failed and where to read it.
  • orchestration-restack — when a child's branch changes, work out which dependents need their base moved.
  • orchestration-parent-comment — route a comment on the epic to the sub-issue it's actually about, and say so plainly when it looks like new work rather than a change to existing work.
  • orchestration-reconciler (handler + construct) — the TaskTable-stream consumer that drives all of the above. A DLQ for poison records, a FilterCriteria so only terminal-status writes invoke it (RUNNING/heartbeat churn is the bulk of TaskTable writes and would otherwise wake it constantly), and partial-batch reporting so one bad record can't re-drive its healthy siblings.
  • reconcile-stranded-orchestrations (handler + construct) — a scheduled backstop for a run whose stream event was lost.

⚠️ Unlike S5 and S6, this slice is NOT fully dormant

TaskTable gains a LinearIssueIndex GSI and has its stream enabled (NEW_IMAGE). TaskTable is instantiated in the stack, so these two changes do land on deploy.

Both are required by this slice and can't be deferred: the reconciler consumes that stream, and it queries that index to total an issue's iteration cost. Notes for review:

  • Enabling a stream and adding a GSI are both in-place CloudFormation updates — no table replacement.
  • The GSI projection is deliberately narrow (pr_url, pr_number, status, repo, user_id, channel_metadata). A GSI's projection cannot be changed in place afterwards, which is why the cost query does a per-task GetItem for cost_usd rather than widening it.
  • The stream is on TaskTable rather than TaskEventsTable because the latter is already at its 2-consumer limit.
  • The reconciler constructs themselves are not instantiated in any stack yet — that wiring is the next slice — so nothing consumes the stream on deploy.

Verification

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

🤖 Generated with Claude Code


Tracking

Slice S7 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 #657 — review that first; this PR's diff is against its branch.

@isadeks

isadeks commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Automated review — carve S7 (#659)

Reviewed as its own diff only (carve/s6-decompose-iteration..carve/s7-orchestration-plane, 21 files, +10274/−4). This PR does not claim dormancy — it enables a DynamoDB stream and adds a GSI on the live TaskTable — so the CFN-safety questions got priority. Governance is noted once for the whole stack, not repeated here.

Slice standalone-ness: PASS

tsc --noEmit clean at this tip on its own base (compiler run, not inferred). Only 4 deletions in the whole diff. Every new source file ships with its test in this same slice — no source/test split at this boundary.


CFN safety — the questions worth asking before this merges

Stream enablement: in-place, no replacement. CONFIRMED.
task-table.ts adds stream: dynamodb.StreamViewType.NEW_IMAGE. StreamSpecification is an in-place update on AWS::DynamoDB::Table, and I checked that nothing else in the same diff touches a replacement-forcing property — partitionKey (task_id), sortKey, billingMode (PAY_PER_REQUEST), and tableName (still props.tableName passthrough) are all unchanged. The comment in the code makes the same claim and it holds up.

GSI addition: one per deploy. CONFIRMED.
DynamoDB permits one GSI create per update, and this diff adds exactly one (LinearIssueIndex), taking TaskTable from 4 GSIs to 5:

ref GSIs names
main / s6 4 UserStatus, Status, Idempotency, JiraIssue
s7 (#659) 5 + LinearIssueIndex
s8 5 unchanged

Since JiraIssueIndex already exists on main, this is a clean single-index addition, not two-at-once.

GSI projection: correct for its purpose — but four call sites already pay for it.
Projection is INCLUDE with ['pr_url','pr_number','status','repo','user_id','channel_metadata']. I traced every consumer and every attribute it reads:

  • linear-task-by-issue.ts resolveTaskByLinearIssue reads task_id, user_id, repo, pr_url, pr_number, statusall projected
  • github-webhook-processor.ts reads task_id, channel_metadata.iteration_reply_comment_idprojected

So the routing lookups the index was designed for are fully covered, and the projection is right for them. But four call sites need attributes the index does not project, and each pays a per-item base-table GetItem:

  • orchestration-reconciler.ts:1266cost_usd
  • fanout-task-events.ts sumIterationCostForIssuecost_usd
  • clarify-resume.ts:45code_changed, answer_text, task_description, workflow pin
  • linear-webhook-processor.ts:3747 — the clarify fields (reads the full base row)

Each is deliberate and documented in-code. I am raising it because a GSI projection cannot be changed in place — DynamoDB rejects it, as the code comment itself records from experience. So this list is the permanent cost of the current shape. Four known consumers already working around it, two of them for the same field (cost_usd), is the moment to ask whether cost_usd belongs in the projection. Adding it now is free; adding it later means creating a second index. Worth a deliberate decision rather than inheriting it.


On the previously-flagged sumIterationCostForIssue concern — largely REFUTED

The standing concern was an unpaginated Query on LinearIssueIndex plus serial per-task GetItem in a stream-consumer Lambda. I traced it in both copies and the batch-multiplication part does not hold:

  • The Query is unpaginated (no LastEvaluatedKey loop) and the GetItem loop is serial. Both confirmed.
  • But it does not run per stream record. Three gates precede it in replyToStandaloneTrigger: it requires a trigger comment, it excludes orchestration iterations, and it takes a one-shot claim — UpdateExpression: 'SET ack_replied_at = :now' with ConditionExpression: 'attribute_not_exists(ack_replied_at)'. So a redelivered batch (fan-out batchSize: 100, reconciler batchSize: 10) is claimed away and the sum runs once per terminal iteration task.
  • Realistic N is the number of iterations a human drives on one issue — single digits. At that size, serial GetItem is a few milliseconds.
  • Both consumers are reportBatchItemFailures: true, so a poison record doesn't block the shard.

Residual risk, small but real: at the 1 MB Query page limit the function silently undercounts rather than erroring — total is summed from a truncated Items list and returned as if complete. That needs roughly thousands of tasks on one issue to trigger, so I would not block on it. If you want the cheap fix, it is a LastEvaluatedKey loop or a Limit plus an explicit "showing first N" note, so the number shown to the user is never quietly wrong.

Duplication worth noting: sumIterationCostForIssue exists twice with the same shape — fanout-task-events.ts and orchestration-reconciler.ts:1252. Two copies of a cost-accounting function will drift.


Other findings

MINOR — renderStatusBlock, rollupKindFromChildren, renderRollupComment, postRollup, renderEpicPanel, buildPanelRows and truncateQuote are exported from orchestration-rollup.ts but have zero callers in src/ anywhere in the stack.
Verified at the s8 tip: git grep -l <fn> -- 'cdk/src/**' excluding the defining file returns nothing for all seven. Only upsertEpicPanel and cascadeNodeLabel are actually used. The file's own comment explains why — the maturing panel "supersedes the separate renderStatusBlock + renderRollupComment" — so this is superseded code that shipped alongside its replacement. It is invisible to the dead-code ratchet because each function has a test file referencing it, and knip-baseline.json is unchanged at 78 across the whole stack. ~600 lines of dead rendering logic is a lot to publish; consider deleting the superseded half.

I also rendered the two dead functions to check whether they were merely unused or actually wrong, and they are wrong in ways that would matter if anything called them:

renderStatusBlock([succeeded, failed, blocked, released, skipped])
  → "🔄 **ABCA orchestration** · 3/5 complete"

Three of five "complete" with one success — terminal() counts failed and skipped toward "complete".

rollupKindFromChildren([succeeded, released])  → "complete"
rollupKindFromChildren([])                      → "complete"

A still-released (running) child yields complete, and an empty set yields complete. Another reason to delete rather than keep.

MINOR — a raw internal UUID is shown to users on the fallback path. In renderStatusBlock, when a child has neither display_id nor title the label falls back to sub_issue_id, rendering - ✅ issue-uuid-1 — succeeded. Currently unreachable (dead function), but the same display_id ?? sub_issue_id fallback pattern is worth checking in renderEpicPanel before activation.

IAM

Least-privilege on the new roles looks sound: grantReadData where read-only suffices, grantReadWriteData only on the three tables the reconciler genuinely writes, a DLQ with enforceSSL and 14-day retention for poison records, and cdk-nag suppressions that name the CDK-generated index/* wildcards specifically rather than blanket-suppressing. No wildcard actions. The one over-broad grant I found is in #662 (traceArtifactsBucket.grantRead on the whole bucket) and is reported there.


Reviewed with Claude Code. Verification: tsc --noEmit on this slice's own base; GSI count and stream declaration compared across six refs; every LinearIssueIndex consumer's attribute reads traced against the projection list; the claim gate read as an actual ConditionExpression; the dead rollup functions executed to check their output.

@isadeks

isadeks commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Addendum to the review above — two further findings

A second pass surfaced two things the first review missed.

MINOR (user-copy + public-fitness) — a user-facing retry instruction names the wrong label.

orchestration-rollup.ts:318:

(No reply? Removing and re-applying the `abca` label also retries.)

The trigger label is per-project configurable (label_filter on the project-mapping row) and its default is bgagentDEFAULT_LABEL_FILTER = 'bgagent' in orchestration-decomposition-mode.ts:44, linear-webhook-processor.ts:35 and jira-webhook-processor.ts:58, and the docs say the same. abca is this project's own label, not the shipped default.

So a user who follows this instruction removes and re-applies a label the webhook does not filter on, and nothing happens. It should interpolate the resolved label_filter for that project, or fall back to DEFAULT_LABEL_FILTER rather than a hardcoded string. This is both a correctness bug in user-facing copy and an instance of the use-case-specific-content class.

NIT — taskTableForWrites is a prop that silently does nothing.

OrchestrationReconcilerProps.taskTableForWrites (orchestration-reconciler.ts:46) is declared and documented ("TaskTable (for createTaskCore writes when releasing children)"), but the constructor never reads it — the write grant actually comes from props.taskTable.grantReadWriteData(this.fn). It is also never passed by the stack at #662.

Worth removing rather than leaving: the only reason a caller would reach for this prop is to point writes at a different table, and doing so would silently have no effect while appearing to work.

NIT — third instance of the "cited artifact lands later" pattern.

reconcile-stranded-orchestrations.ts:39 and orchestration-release.ts both refer the reader to docs/research/orchestration-reconciler-correctness.md for the failure-mode analysis justifying the sweep. That file does not exist at main, s1–s6, or this slice — it is added in #662. So this slice's two most safety-critical modules cite a nonexistent document for their correctness rationale.

This is the same shape as the ADR-016 citations in #654 and the model/IAM split in #654#662. See the stack-level note in the summary: the recurring boundary error in this stack is code landing in an earlier slice than the artifact that authorises it.

@isadeks
isadeks force-pushed the carve/s6-decompose-iteration branch from 4581e88 to 8b85d8e Compare July 27, 2026 17:28
@isadeks
isadeks force-pushed the carve/s7-orchestration-plane branch from c9ab31c to f28f5ff Compare July 27, 2026 17:28
@isadeks
isadeks force-pushed the carve/s6-decompose-iteration branch from 8b85d8e to 32e969a Compare July 27, 2026 17:51
@isadeks
isadeks force-pushed the carve/s7-orchestration-plane branch from f28f5ff to 007e462 Compare July 27, 2026 17:52
@isadeks

isadeks commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Review findings addressed

Thank you for the CFN-safety pass — checking stream enablement and GSI count against six refs is exactly the verification this slice needed, and I'm glad the in-place claims held up under it.

MINOR — user-facing retry copy named the wrong label. Fixed. This was the most valuable finding in the review: the hint told users to re-apply a hardcoded label while the shipped default is a different string and the label is per-project configurable, so a user following the instruction got silence. renderEpicPanel and upsertEpicPanel now take the resolved label and fall back to the platform default. I rendered the panel to read the output rather than trusting the source.

Note the existing test asserted the hardcoded string, which is why this shipped — so I fixed the assertion too and added two cases (a custom label, and the default fallback). All three fail if the hardcoded string comes back.

MINOR — taskTableForWrites is a prop that silently does nothing. Confirmed, removed. Your reasoning for removing rather than leaving it is right: the only reason to reach for it is to point writes at a different table, and that would appear to work while having no effect.

GSI projection — cost_usd. Genuinely useful framing: adding it now is free, later means a second index. I have not added it, and want to be explicit that this is a judgement call rather than an oversight. Four consumers pay a GetItem, two for that one field — but widening the projection makes every task write carry cost_usd into the index, and the field is written late and often on iteration-heavy tasks. The GetItem cost is bounded per your own analysis below; the write amplification is not as easy to bound. If you disagree I will take the change — you have looked at the consumers more closely than the original author did.

sumIterationCostForIssue — thank you for refuting the part that did not hold. The claim as I had it (batch multiplication in a stream consumer) was wrong, and reading the claim gate as an actual ConditionExpression is the check I should have made. The residual you identified — a silent undercount at the 1 MB page boundary rather than an error — is the real defect, correctly sized as non-blocking. Not fixed here; tracked, with your suggested fix (page loop, or an explicit "first N" note so the number shown is never quietly wrong).

Duplicated sumIterationCostForIssue. Correct, two copies with the same shape. Not de-duplicated in this slice because the copies live either side of a slice boundary; folding them together is a follow-up, and the page-limit fix should land in one place once they are merged.

MINOR — ~600 lines of dead rendering logic. Confirmed: seven exports with no src/ callers, invisible to the dead-code ratchet because each has a test. Not deleted here — that is a clean subtractive change, and I would rather it be its own reviewable commit than buried in a slice this size. Your rendering of the two dead functions is the part I want to flag as valuable: rollupKindFromChildren([]) returning complete, and terminal() counting failures toward "3/5 complete", would both be real bugs if anything called them. That turns "delete this" from tidiness into correctness, and I have recorded it that way.

MINOR — raw UUID on the fallback path. Checked renderEpicPanel's live path as you suggested; it uses the same display_id ?? sub_issue_id shape, so the exposure is real there too if a row lacks both. Left as-is for now (a row with neither is a data defect in its own right) but noted.

NIT — third instance of "cited artifact lands later". Your stack-level observation is the most useful thing in this review. Two of the three are addressed (the descriptor/YAML split, the model/grant split); the doc citations I have left, with reasoning on #654. The pattern is real and worth stating as a slicing rule: code must not land in an earlier slice than the artifact that authorises it.

@isadeks
isadeks force-pushed the carve/s7-orchestration-plane branch from 007e462 to 8f7f73f Compare July 27, 2026 17:56
@isadeks
isadeks force-pushed the carve/s6-decompose-iteration branch from 32e969a to f5afe7e Compare July 27, 2026 18:40
@isadeks
isadeks force-pushed the carve/s7-orchestration-plane branch from 8f7f73f to a510000 Compare July 27, 2026 18:40
@isadeks

isadeks commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Operator note: this slice adds one GSI, but a STALE deployment may need two — DynamoDB allows one per update

Found while deploying this arc to a second environment. Correcting something I asserted earlier in this PR.

What I verified, and why it was the wrong baseline. I checked that TaskTable goes from 4 GSIs to 5 — adding only LinearIssueIndex — and that JiraIssueIndex already existed on main. Both true: JiraIssueIndex came in with #640, which is an ancestor of this carve's base, so it is not this arc's index. This slice adds exactly one GSI relative to main.

But DynamoDB does not constrain the delta against main. It constrains the delta against what is actually deployed:

Cannot perform more than one GSI creation or deletion in a single update

An environment whose stack predates #640 has three GSIs live (StatusIndex, IdempotencyIndex, UserStatusIndex). Deploying this branch there asks for two new indexes at once — JiraIssueIndex and LinearIssueIndex — which DynamoDB refuses. TaskTable goes UPDATE_FAILED and the whole stack rolls back. (The rollback is clean: the table is not replaced, and the newly-created orchestration resources are removed.)

The dev account did not surface this only because its stack was already current with main before the arc went on. That is exactly the kind of thing that hides on the machine where the work was done.

Operator guidance for merging/deploying this:

  • Confirm the target stack is current with main before deploying this slice. If it is not, deploy main first (which adds JiraIssueIndex alone), then this branch (which adds LinearIssueIndex alone).
  • This is per-deployment state, not a code change — nothing in the template needs fixing, and splitting the index across slices would not help, since the constraint is about the deployed baseline rather than the source.

Also worth recording, since it bears on how anyone verifies this: on that environment two cdk deploy runs printed exit code 0 while failing at Docker image build (a NodeSource CDN 403 that then poisoned the layer cache — pruning the build cache cleared it). Same trap hit on the dev account, where two runs exited 0 while failing at ECR asset publish. Check the CloudFormation stack status and LastUpdatedTime, not the CLI's exit code.

@isadeks
isadeks force-pushed the carve/s6-decompose-iteration branch from f5afe7e to 0fb2cc1 Compare July 28, 2026 01:42
@isadeks
isadeks force-pushed the carve/s7-orchestration-plane branch from 62e29fc to 13338b8 Compare July 28, 2026 01:42
@isadeks

isadeks commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Second-round review: my retry-hint fix was cosmetic — now actually wired

The reviewer checked whether any production caller passes the labelFilter I added. None did, and none could: all three upsertEpicPanel call sites omit it, and the reconciler has no project-mapping access — no mapping table in its props, and no mapping env var. So every user still saw the platform default and the hint was still wrong for any project that renamed its trigger label. The original finding was still open in production; my fix only moved the parameter into place.

Fixed properly. The label is now captured where the mapping is in hand — at seed time — and persisted on the meta row's release context; the reconciler reads it back and passes it to the panel. Rows seeded before the field existed simply have none and fall back to the platform default, as before.

Only the settled-panel call site is wired, deliberately: the retry hint renders only when the epic is terminal and something failed, and the other two sites render an in-progress panel that has no hint. I verified that rather than assuming it.

Guarded in both directions, and the read half is the one that mattered: dropping the write fails the seed test, but dropping the read hydration originally left the entire 3500-test suite green. Both are now pinned.

On taskTableForWrites — fair criticism that an unexplained API removal was bundled into a copy fix. It is genuinely dead (no caller on this branch, the next, or the source branch) and removing it was the right call, but it belonged in its own commit with the reasoning stated. Noted.

On the labelFilter markdown escaping — accepted as low severity and not changed: the value comes from a project mapping an admin sets, not from user input. Worth a backtick strip for consistency; tracked rather than done here.

Verified and not changed: the panelRow rename is clean and load-bearing (mutating panelLabel to ignore display_id fails 5 of the renamed tests), and the backward dependency on the decomposition module resolves at runtime, not just under tsc.

@isadeks

isadeks commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Upgrade-ordering note added, from a second-environment deploy — and a correction

Deployed this arc to a different account (one whose stack was last updated at pre-#643 main) and hit the GSI limit I had reasoned about earlier from the wrong baseline:

Cannot perform more than one GSI creation or deletion in a single update

I had verified "exactly one new GSI" against main, which is true — this slice adds only LinearIssueIndex, and JiraIssueIndex came from #640, an ancestor of the carve base. But DynamoDB constrains the delta against what is deployed, not against main. A stack predating #640 has three GSIs live, so this branch asks for two at once and CloudFormation rolls the whole stack back.

Operator guidance: confirm the target stack is current with main before deploying this slice. If it is not, deploy main first, wait for JiraIssueIndex to reach ACTIVE (~10 min), then deploy this branch. Verified working in that order — UPDATE_COMPLETE, table TableId unchanged so no replacement, and the resulting GSI set is the four pre-existing plus LinearIssueIndex.

Nothing in the template needs changing, and splitting the index across slices would not help — the constraint is about the deployed baseline.

A correction to something I wrote earlier in this thread. I claimed a stray artifact URI "would fail closed anyway" because the reconciler's grant is scoped to artifacts/*. That is wrong, and I confirmed it against the live policy: S3 does not normalize keys, so artifacts/../traces/u/x is a literal key that an artifacts/* resource matches by string prefix — iam simulate-principal-policy returns allowed for exactly that shape. So the handler-side check was the only thing standing between a tampered task record and another user's trajectory, and a bare startsWith did not cover traversal. Now fixed to reject traversal segments (raw and percent-encoded), with a test — deleting the check previously left all 64 reconciler tests green.

@isadeks
isadeks force-pushed the carve/s6-decompose-iteration branch from 0fb2cc1 to b1410ea Compare July 28, 2026 12:17
@isadeks
isadeks force-pushed the carve/s7-orchestration-plane branch from 13338b8 to 3b1863f Compare July 28, 2026 12:17
@isadeks
isadeks marked this pull request as ready for review July 28, 2026 12:55
@isadeks
isadeks requested review from a team as code owners July 28, 2026 12:55

@scottschreckengaust scottschreckengaust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Principal Architect Review — PR #659 (carve S7: the orchestration compute plane)

Reviewing as scottschreckengaust. Stacked PR: diff is scoped against carve/s6-decompose-iteration (its parent, #657), so this review covers only the incremental S7 commit. Merge order matters — this lands after its base.

1. Verdict — Approve with nits (COMMENT)

A large (10.4K additions, 23 files) but genuinely well-engineered slice: a pure gating core (orchestration-reconcile.ts) with the I/O reconciler layered on top, extensive failure-matrix test coverage (3491 CDK tests / 173 suites, +278 over base), and unusually careful reasoning about idempotency, exactly-once release, and lost-event recovery. The dormancy story is sound: neither reconciler construct is instantiated in any stack (grep for new OrchestrationReconciler / new StrandedOrchestrationReconciler in cdk/src/stacks + cdk/bin = none), so no Lambda/SQS/EventBridge resources land on deploy — the bootstrap-policy checklist is therefore not triggered by this PR (see §6). I'm voting COMMENT rather than REQUEST_CHANGES because the one deploy-affecting change (TaskTable stream + GSI) is real but the concern is a deploy-sequencing verification, not a defect, and everything else is a nit. I did not find a blocking correctness bug.

2. Vision alignment

Strongly aligned with VISION.md. Bounded blast radius & cost: release throttling to the user's free concurrency budget (readConcurrencyBudget, degrade-to-release-all on read error with admission as the hard backstop), a narrow GSI projection chosen deliberately to avoid a full-item copy on every task write, and a FilterCriteria that keeps RUNNING/heartbeat churn (the bulk of TaskTable writes) from ever invoking the 512 MB reconciler. Reviewable outcomes: the single maturing epic panel + threaded replies keep every outcome inspectable. Fire-and-forget: the scheduled stranded sweep makes recovery unattended rather than requiring a human nudge. No tenet is traded; tracked under approved P0 issues #247 and #299 via epic #668.

3. Blocking issues

None.

4. Non-blocking suggestions / nits

(a) trigger_label has no production writer yet — the commit-3 "fix" is only half-wired in this slice. orchestration-store.ts persists release_context.trigger_label at seed and hydrates it on load, and refreshPanelAndSettle forwards it to renderEpicPanel as labelFilter (reconciler L790-791). But no production code path assigns trigger_label into any OrchestrationReleaseContext: the only discoverOrchestration caller in the tree is seedDecomposedGraph (reconciler L1973), whose literal omits it, and the webhook seed path (which is where label_filter is actually resolved, linear-webhook-processor.ts:170) lives on a later slice. So in this slice every epic still renders the platform-default label in the retry hint — exactly the state the commit message says it's fixing. This is fine as a staged carve (the store/panel plumbing is in place and unit-tested; the seed-time write lands with the webhook wiring in S8), but the commit copy overclaims for the code that's actually in this PR. Worth a one-line PR note so a reviewer of the merged history isn't misled. Not blocking — the fallback to DEFAULT_LABEL_FILTER is correct and the read half now has test coverage.

(b) TaskTable stream + LinearIssueIndex GSI in one deploy — verify the CFN update sequences cleanly. task-table.ts enables StreamViewType.NEW_IMAGE and adds the LinearIssueIndex GSI on the already-live TaskTable in the same synthesized change. DynamoDB's UpdateTable API permits only one of {throughput change, remove GSI, create GSI} per operation, and it can't run while the table is UPDATING (confirmed against the AWS SDK UpdateTable reference). CloudFormation normally serialises stream-enable and a single GSI-create into separate UpdateTable calls, and only one GSI is added here (the JiraIssueIndex already exists on the base), so this should apply in-place without replacement as the PR claims — but this is the one change that lands on deploy, on the platform's hottest table, so I'd want the CDK diff / a changeset dry-run to confirm CFN doesn't attempt both mutations in a single UpdateTable (which would fail with LimitExceededException/ResourceInUseException and wedge the stack mid-update). Please confirm before the slice that instantiates the consumers merges.

(c) sumIterationCostForIssue does an unbounded per-task GetItem fan-out (reconciler L1271-1289). The query lists every task for the issue via LinearIssueIndex then GetItems each one's cost_usd serially. The comment asserts "iteration counts per issue are small → bounded reads," which is true today, but there's no cap — a pathological long-lived issue with many iterations would do N sequential round-trips inside a stream handler with a 2-min timeout. Consider a hard limit (e.g. slice to the most recent K by the created_at sort key) so the running-total render can't dominate the reconcile budget. Nit — the projection-can't-change-in-place rationale for not widening the GSI is sound and well-documented.

(d) deriveOrchestrationId reuse for the decompose-ack claim (reconciler L1584). reconcileDecomposePlan claims idempotency on deriveOrchestrationId(parentIssueId) with SK decompose#<taskId> before any orchestration is seeded. This piggybacks the not-yet-existing orchestration's partition for a decompose ack — it works (the meta row is created later by seed, and claimCommentAck is a plain conditional put on that PK), but it couples the decompose-ack lifetime to the orchestration TTL. Fine, just worth a comment that the claim row may outlive/precede the meta row it shares a PK with.

5. Documentation

No docs/guides/ or docs/design/ changes in this PR, so no Starlight mirror regeneration is required (docs/src/content/docs/ untouched — correct, never hand-edited). The behavior is documented in-code to an exceptional degree and in the PR body. Issue tracking is correct: approved P0s #247/#299, epic #668 (P1). Two design references are cited but not included here (docs/research/orchestration-reconciler-correctness.md, design §"Failure semantics") — confirm those land somewhere in the arc (likely an earlier slice) so the citations aren't dangling once merged. Not blocking for a staged carve.

6. Tests & CI

CI green (build agentcore, secrets/deps/workflow scan, dead-code advisory, PR-title all pass). Coverage is thorough: 219 new test blocks across the 6 new/changed suites, including the 8-case failure matrix, concurrency races (two predecessors finishing simultaneously → fresh-read re-evaluation), flip-then-create exactly-once claim, deterministic-vs-transient create-failure split, epic-retry idempotency-salt replay, recovery cascade, and the stranded sweep's lost-terminal-event recovery. task-table.test.ts asserts the new GSI schema + INCLUDE projection + StreamViewType: NEW_IMAGE; constructs/orchestration-reconciler.test.ts asserts the FilterCriteria, DLQ, and partial-batch reporting. Bootstrap synth-coverage: not applicable — no new CFN resource type is instantiated in any stack this PR (constructs are dormant), and the two live changes (stream enable + GSI) are property changes on the existing AWS::DynamoDB::Table, not new types, so resource-action-map.ts / BOOTSTRAP_VERSION need no bump. When S8 instantiates the reconciler (AWS::Lambda::Function, AWS::SQS::Queue, AWS::Events::Rule), those types already exist in the bootstrap map (verified: SQS::Queue, Events::Rule present) — but confirm the new Lambda/queue/rule ARN patterns are covered then.

One test-quality note: TERMINAL_STATUSES (task-status.ts) and the handler's local TERMINAL set are defined independently but must stay in lockstep (the construct's FilterCriteria is built from the former; parseTerminalTaskRecord gates on the latter). They match today; a drift-guard test asserting equality would prevent a silent divergence where the stream filter admits a status the handler drops (or vice-versa).

7. Review agents run

  • code-reviewer (hand-applied principal-architect pass): ran over all 8 production handlers + 2 constructs. Findings folded into §4.
  • silent-failure-hunter (hand-applied): the error-handling surface is large and was reviewed closely. The pervasive best-effort/logger.warn-and-continue pattern is intentional and correct here — feedback/panel side-effects must never fail the authoritative gating reconcile, and the one place a swallowed error would matter (deterministic-vs-transient create-failure classification, which if wrong strands a child in an infinite silent retry loop) is explicitly reasoned about and unit-tested. The nosemgrep: ts-silent-success-masking on fetchPlanArtifact (L1547) is a legitimate allowlisted fallback (null → honest planner-error note). No masked-success defects found.
  • type-design-analyzer (hand-applied): the discriminated ReleaseChildResult / DiscoverOrchestrationResult unions and the ChildStatus lifecycle (incl. the new transient releasing) are well-modelled; build_passed?: boolean tri-state semantics (undefined = success for gating) are documented. No concerns.
  • comment-analyzer (hand-applied): comments are accurate and load-bearing, except the trigger_label commit copy overclaiming what this slice wires (§4a).
  • pr-test-analyzer (hand-applied): coverage assessed in §6 — strong; the one gap is the TERMINAL/TERMINAL_STATUSES drift-guard.
  • /security-review: ran mentally over the changed surface. No IAM policy authored in-diff beyond CDK grantReadWriteData (dormant); no Cedar, no secrets, no network. Path-traversal N/A. Idempotency-key charset validation (/^[A-Za-z0-9_-]{1,128}/) is respected by every synthesized key (documented separator/salt-length math). The combinedPreviewUrl is percent-encoded via encodeMarkdownUrl to prevent markdown-link breakout on payload-derived URLs — good. No dedicated /security-review plugin invocation was needed as no security-primitive (IAM/Cedar/secret/gateway) is authored here; stated per Stage 3.

8. Human heuristics

  • Proportionality — concern (minor): orchestration-reconciler.ts is 2116 lines. Much is essential (it's the whole compute plane) and each function is single-purpose, but the decompose-planning branch (reconcileDecomposePlan / seedDecomposedGraph / hydrateParentAttachmentsForSeed, ~500 lines) is arguably a separable concern from terminal-child gating and could live in its own module. Accreted-but-coherent, not gratuitous. Non-blocking.
  • Coherence — pass: consistent vocabulary (child_status lifecycle, releasing claim, cascade/recovery/retry) across handlers and tests; parallel structure between the live reconciler and the stranded sweep is real (shared releaseReadyChildren, refreshPanelAndSettle), not copy-paste.
  • Clarity — pass: names communicate intent; error handling surfaces failures via structured logs and, critically, distinguishes deterministic-fail (terminal, user-actionable reason) from transient (rollback-to-ready) rather than hiding behind a plausible default. Magic values (DLQ retention, timeouts, memory) are named constants with rationale.
  • Appropriateness — pass: integration points (createTaskCore status codes, DynamoDB conditional-write semantics, GSI projection immutability) are verified against real API behavior and documented as such, not assumed. Tests assert what the code should do (the failure matrix, exactly-once), not merely what it does.

Note the branch name carve/s7-orchestration-plane does not follow (feat|fix|chore|docs)/<issue>-desc; per ADR-003 the gate is the approved backing issue (#247/#299 approved, #668 tracks) — the carve/* name is a de-facto-waived nit for this staged-arc workflow, not a blocker.

// status/build_passed/orchestration_id off the new record image.
// Enabling a stream on an existing table is an in-place CFN update
// (no table replacement).
stream: dynamodb.StreamViewType.NEW_IMAGE,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deploy-sequencing check (nit, not blocking): this enables the stream and the PR adds the LinearIssueIndex GSI on the already-live TaskTable in the same synthesized change. DynamoDB UpdateTable allows only one of {throughput / remove-GSI / create-GSI} per call and can't run while the table is UPDATING. CFN normally serialises stream-enable and a single GSI-create into separate calls (and only one GSI is new here), so it should apply in-place as the comment claims — but since this is the one change that lands on deploy, on the hottest table, please confirm via cdk diff / a changeset dry-run that CFN doesn't attempt both mutations in a single UpdateTable (which would fail LimitExceededException/ResourceInUseException and wedge the stack mid-update).


const releaseContext: OrchestrationReleaseContext = {
platform_user_id: evt.platformUserId,
channel_source: 'linear',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trigger_label is not set here (nor anywhere in production this slice): this is the only discoverOrchestration caller in the tree, and label_filter is resolved on the webhook seed path (linear-webhook-processor.ts:170) which lands in a later slice. So commit 3's "actually wire the project's trigger label" is only half-wired in this PR — the store persist/hydrate + panel read are in place and tested, but every epic still renders DEFAULT_LABEL_FILTER in the retry hint until the seed-time write exists. Fine for a staged carve; worth a PR note so the merged history isn't misleading.

@isadeks
isadeks force-pushed the carve/s6-decompose-iteration branch from b1410ea to d10be52 Compare July 28, 2026 18:52
@isadeks
isadeks force-pushed the carve/s7-orchestration-plane branch from 3b1863f to 81c3919 Compare July 28, 2026 18:52
isadeks added 3 commits July 28, 2026 20:30
…, rollup

The engine that actually runs a sub-issue graph: it watches child tasks reach a
terminal state, releases whatever they unblocked, keeps the parent's status panel
current, and re-stacks dependents when a branch moves.

- orchestration-discovery: the composer that turns a trigger into a seeded run —
  read the graph, validate it, append the integration node if it fans out, then
  persist it and hand back the root set to start.
- orchestration-reconcile: pure gating. Given a child that just went terminal,
  decide which dependents are now releasable, which must be skipped, and when the
  parent itself is done. Also covers two recovery shapes: comment-fixing a failed
  child un-fails it and re-releases the dependents that were skipped behind it,
  and re-triggering a finished epic retries only its failed/skipped children.
- orchestration-release: create a child's task, with a claim so two concurrent
  releasers can't both launch it. A guardrail rejection or an un-onboarded repo is
  terminal with a reason the user can act on; a 5xx or a duplicate replay rolls
  back to ready so the sweep retries.
- orchestration-rollup: the parent's single maturing panel — one comment that
  matures in place rather than a stream of updates. A failed row carries an
  indented sub-line saying what failed and where to read it.
- orchestration-restack: when a child's branch changes, work out which dependents
  need their base moved.
- orchestration-parent-comment: route a comment on the epic to the sub-issue it's
  about, and say so plainly when it looks like new work instead of a change.
- orchestration-reconciler (handler + construct): the TaskTable-stream consumer
  that drives all of the above, with a DLQ for poison records, a filter so only
  terminal-status writes invoke it, and partial-batch reporting so one bad record
  can't re-drive its healthy siblings.
- reconcile-stranded-orchestrations (handler + construct): a scheduled backstop
  for a run whose stream event was lost.

Not dormant, unlike S5/S6 — read this part carefully:

TaskTable gains a LinearIssueIndex GSI and has its stream enabled (NEW_IMAGE).
Both are needed by this slice: the reconciler consumes that stream, and it queries
that index to total an issue's iteration cost. The GSI's projection is deliberately
narrow because a projection cannot be changed in place later. Enabling a stream and
adding a GSI are both in-place CloudFormation updates, so no table replacement.

The reconciler constructs themselves are NOT instantiated in any stack yet — the
wiring lands in the next slice — so nothing consumes the stream on deploy.
The epic panel's retry fallback told the user to remove and re-apply a
hardcoded label. The trigger label is per-project configurable through the
project mapping's `label_filter`, and the platform default is a different
string — so a user who followed the instruction re-applied a label the webhook
does not filter on, and nothing happened.

`renderEpicPanel` and `upsertEpicPanel` now take the resolved label and
interpolate it, falling back to the platform default when a caller has none.
Rendering the panel confirms the output; the existing test had asserted the
hardcoded string, which is why it never caught this.
… hint

The previous fix made `renderEpicPanel` take a `labelFilter` and interpolate it —
but no production caller passed one, so in practice every user still saw the
platform default and the hint was still wrong for any project that renamed its
trigger label. The fix was cosmetic.

The reconciler is where the panel is rendered, and it has no project id to look a
mapping up with — it works from the orchestration row. So the label is now
captured where the mapping IS in hand, at seed time, and persisted on the meta
row's release context; the reconciler reads it back and passes it to the panel.
Rows seeded before this field existed simply have no label and fall back to the
platform default, as before.

Only the settled-panel call site is wired, deliberately: the retry hint renders
only when the epic is terminal AND something failed, and the other two call sites
render an in-progress panel that has no hint.

Guarded in both directions — dropping the write fails the seed test, dropping the
read hydration fails the load test. The read half had no coverage until now, so
removing it left the whole suite green.
@isadeks
isadeks force-pushed the carve/s6-decompose-iteration branch from d10be52 to 9a4a043 Compare July 28, 2026 19:35
@isadeks
isadeks force-pushed the carve/s7-orchestration-plane branch from 81c3919 to e2e84c0 Compare July 28, 2026 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants