feat(local-runner): add recovery policies and reusable pipelines - #436
Conversation
📝 WalkthroughWalkthroughThe orchestration runtime adds validated retry and fallback policies, attempt metadata, policy graph nodes, reusable nested pipelines, scoped hierarchical IDs, persistence fingerprints, and pipeline lifecycle events. Public types and helpers are re-exported. Tests cover retries, fallbacks, idempotency, resumption, nested pipeline scoping, and validation. The regional example now uses a reusable preparation pipeline. Gallery normalization now handles concluding snapshot nodes and traverses graph edges for layout. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OrchestrationContext
participant invokePipelineNode
participant prepareRegions
participant runWithPolicyNode
participant NotebookRunner
OrchestrationContext->>invokePipelineNode: invoke regional-preparation
invokePipelineNode->>prepareRegions: execute scoped pipeline
prepareRegions->>runWithPolicyNode: run regional step with policy
runWithPolicyNode->>NotebookRunner: execute retryable attempt
NotebookRunner-->>runWithPolicyNode: prepared regional text
runWithPolicyNode-->>prepareRegions: resolved step result
prepareRegions-->>invokePipelineNode: notes and preparation state
invokePipelineNode-->>OrchestrationContext: pipeline output
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/local-orchestration #436 +/- ##
============================================================
- Coverage 87.48% 87.46% -0.03%
============================================================
Files 184 184
Lines 9926 10082 +156
Branches 2825 2882 +57
============================================================
+ Hits 8684 8818 +134
- Misses 1241 1263 +22
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
examples/local-runner/gallery/pipeline-data.js (1)
58-77: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse an indexed queue instead of
shift().
Array.prototype.shift()can be O(n), so wide graphs can still make this traversal quadratic.Proposed fix
const ready = nodes.filter(node => incoming.get(node.id) === 0).map(node => node.id) + let readyIndex = 0 let visited = 0 - while (ready.length > 0) { - const id = ready.shift() + while (readyIndex < ready.length) { + const id = ready[readyIndex++]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/local-runner/gallery/pipeline-data.js` around lines 58 - 77, Replace the shift-based queue consumption in the graph traversal while loop with an indexed queue position, advancing the index as each ready node is processed. Keep appending newly ready child IDs to ready and preserve the existing traversal, column calculation, and cycle detection behavior.examples/local-runner/orchestration/run.mjs (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
targetshadows the module-leveltarget.Same value today, but renaming the destructured field (e.g.
runTarget) keeps the example unambiguous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/local-runner/orchestration/run.mjs` at line 25, Rename the destructured target parameter in the run method to runTarget to avoid shadowing the module-level target, and update all references within run to use the new name.packages/local-runner/src/orchestrate.ts (1)
796-798: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRethrown
OrchestrationStepErrorcarries the attempt ID, not the policy ID.Infrastructure errors from the last attempt propagate as-is, so
error.stepIdis"<id>-attempt-N"while docs tell consumers to key off the stable policy node ID. Consider wrapping with the policyidandcausepreserved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-runner/src/orchestrate.ts` around lines 796 - 798, Update the last-error rethrow in the orchestration flow to wrap an OrchestrationStepError with the stable policy id rather than rethrowing the attempt-scoped error directly. Preserve the original error as the cause and retain the existing behavior for non-Error values.packages/local-runner/src/orchestrate.test.ts (1)
318-321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering backoff delays.
Nothing exercises
initialDelayMs/backoffMultiplier/maxDelayMs; fake timers would pin the computed delay sequence and the cap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-runner/src/orchestrate.test.ts` around lines 318 - 321, Add a test around defineRunPolicy that configures initialDelayMs, backoffMultiplier, and maxDelayMs, uses fake timers to control retry scheduling, and asserts the computed delay sequence includes the configured cap. Keep the existing idempotent and maxAttempts behavior covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/local-runner/src/orchestrate.ts`:
- Around line 1009-1022: The qualifyDependencies function currently forces every
unprefixed dependency ID into the current scope, preventing references to
outer-scope nodes. Add and consistently apply an explicit absolute-ID escape
hatch for dependency strings and object IDs, while retaining current scoping for
ordinary IDs and preserving already-qualified IDs.
---
Nitpick comments:
In `@examples/local-runner/gallery/pipeline-data.js`:
- Around line 58-77: Replace the shift-based queue consumption in the graph
traversal while loop with an indexed queue position, advancing the index as each
ready node is processed. Keep appending newly ready child IDs to ready and
preserve the existing traversal, column calculation, and cycle detection
behavior.
In `@examples/local-runner/orchestration/run.mjs`:
- Line 25: Rename the destructured target parameter in the run method to
runTarget to avoid shadowing the module-level target, and update all references
within run to use the new name.
In `@packages/local-runner/src/orchestrate.test.ts`:
- Around line 318-321: Add a test around defineRunPolicy that configures
initialDelayMs, backoffMultiplier, and maxDelayMs, uses fake timers to control
retry scheduling, and asserts the computed delay sequence includes the
configured cap. Keep the existing idempotent and maxAttempts behavior covered.
In `@packages/local-runner/src/orchestrate.ts`:
- Around line 796-798: Update the last-error rethrow in the orchestration flow
to wrap an OrchestrationStepError with the stable policy id rather than
rethrowing the attempt-scoped error directly. Preserve the original error as the
cause and retain the existing behavior for non-Error values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34eb9ccd-e66a-4065-86a9-abecf28fa207
📒 Files selected for processing (8)
examples/local-runner/gallery/pipeline-data.jsexamples/local-runner/gallery/pipeline.test.tsexamples/local-runner/orchestration/README.mdexamples/local-runner/orchestration/run.mjspackages/local-runner/README.mdpackages/local-runner/src/index.tspackages/local-runner/src/orchestrate.test.tspackages/local-runner/src/orchestrate.ts
| function qualifyDependencies( | ||
| scopeId: string, | ||
| dependencies: OrchestrationDependencyInput[] | undefined | ||
| ): OrchestrationDependencyInput[] | undefined { | ||
| const qualify = (id: string): string => (id.startsWith(`${scopeId}/`) ? id : `${scopeId}/${id}`) | ||
| return dependencies?.map(dependency => | ||
| typeof dependency === 'string' | ||
| ? qualify(dependency) | ||
| : { | ||
| ...dependency, | ||
| id: qualify(dependency.id), | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No way to depend on an outer-scope node from inside a pipeline.
Every dependency ID that isn't already prefixed gets scoped, so a child step depending on a parent-level node (e.g. 'inputs') fails with "depends on unknown or not-yet-started node". Fine if intentional isolation, but consider documenting it or allowing an absolute-ID escape hatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/local-runner/src/orchestrate.ts` around lines 1009 - 1022, The
qualifyDependencies function currently forces every unprefixed dependency ID
into the current scope, preventing references to outer-scope nodes. Add and
consistently apply an explicit absolute-ID escape hatch for dependency strings
and object IDs, while retaining current scoping for ordinary IDs and preserving
already-qualified IDs.
1 participant
Summary
Stacked on #435. Review the incremental diff only; this PR adds the next two orchestration capabilities:
Validation
pnpm test— 2,699 passed, 1 skippedpnpm typecheckpnpm buildpnpm biome:check— passes with 7 existing warningspnpm prettier:checkpnpm spell-checkpnpm example:orchestration— completed against Deepnote Cloud, including both regional notebook runs and the concluding agent runSummary by CodeRabbit
New Features
Bug Fixes
Documentation