feat(carve S7): the orchestration compute plane — reconciler, release, rollup - #659
feat(carve S7): the orchestration compute plane — reconciler, release, rollup#659isadeks wants to merge 3 commits into
Conversation
9302c20 to
59ba7e2
Compare
b51246e to
18bcf34
Compare
59ba7e2 to
bc71750
Compare
18bcf34 to
4581e88
Compare
bc71750 to
c9ab31c
Compare
Automated review — carve S7 (#659)Reviewed as its own diff only ( Slice standalone-ness: PASS
CFN safety — the questions worth asking before this mergesStream enablement: in-place, no replacement. CONFIRMED. GSI addition: one per deploy. CONFIRMED.
Since GSI projection: correct for its purpose — but four call sites already pay for it.
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
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 ( On the previously-flagged
|
Addendum to the review above — two further findingsA second pass surfaced two things the first review missed. MINOR (user-copy + public-fitness) — a user-facing retry instruction names the wrong label.
The trigger label is per-project configurable ( 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 NIT —
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.
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. |
4581e88 to
8b85d8e
Compare
c9ab31c to
f28f5ff
Compare
8b85d8e to
32e969a
Compare
f28f5ff to
007e462
Compare
Review findings addressedThank 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. 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 — GSI projection —
Duplicated MINOR — ~600 lines of dead rendering logic. Confirmed: seven exports with no MINOR — raw UUID on the fallback path. Checked 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. |
007e462 to
8f7f73f
Compare
32e969a to
f5afe7e
Compare
8f7f73f to
a510000
Compare
Operator note: this slice adds one GSI, but a STALE deployment may need two — DynamoDB allows one per updateFound 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 But DynamoDB does not constrain the delta against An environment whose stack predates #640 has three GSIs live ( The dev account did not surface this only because its stack was already current with Operator guidance for merging/deploying this:
Also worth recording, since it bears on how anyone verifies this: on that environment two |
f5afe7e to
0fb2cc1
Compare
62e29fc to
13338b8
Compare
Second-round review: my retry-hint fix was cosmetic — now actually wiredThe reviewer checked whether any production caller passes the 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 On the Verified and not changed: the |
Upgrade-ordering note added, from a second-environment deploy — and a correctionDeployed 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: I had verified "exactly one new GSI" against Operator guidance: confirm the target stack is current with 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 |
0fb2cc1 to
b1410ea
Compare
13338b8 to
3b1863f
Compare
scottschreckengaust
left a comment
There was a problem hiding this comment.
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. Thenosemgrep: ts-silent-success-maskingonfetchPlanArtifact(L1547) is a legitimate allowlisted fallback (null → honest planner-error note). No masked-success defects found. - type-design-analyzer (hand-applied): the discriminated
ReleaseChildResult/DiscoverOrchestrationResultunions and theChildStatuslifecycle (incl. the new transientreleasing) are well-modelled;build_passed?: booleantri-state semantics (undefined = success for gating) are documented. No concerns. - comment-analyzer (hand-applied): comments are accurate and load-bearing, except the
trigger_labelcommit 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). ThecombinedPreviewUrlis percent-encoded viaencodeMarkdownUrlto prevent markdown-link breakout on payload-derived URLs — good. No dedicated/security-reviewplugin 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.tsis 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_statuslifecycle,releasingclaim, cascade/recovery/retry) across handlers and tests; parallel structure between the live reconciler and the stranded sweep is real (sharedreleaseReadyChildren,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, |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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.
b1410ea to
d10be52
Compare
3b1863f to
81c3919
Compare
…, 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.
d10be52 to
9a4a043
Compare
81c3919 to
e2e84c0
Compare
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.TaskTablegains aLinearIssueIndexGSI and has its stream enabled (NEW_IMAGE).TaskTableis 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:
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-taskGetItemforcost_usdrather than widening it.TaskTablerather thanTaskEventsTablebecause the latter is already at its 2-consumer limit.Verification
tsc --noEmitcleaneslintclean🤖 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.