Repository navigation
Split references, run control, checkout guard and provider resolution out of dag-executor.ts - #4516
Conversation
Step D4 of the dag-executor split (part of #1140). The declarations move verbatim into two owners under packages/workflows/src/dag/: - references.ts: run inputs, node bindings, value templates, shell quoting and spill files, output-reference errors, template substitution, loop-body scopes and attention options. Bindings and templates call each other, so they are one module. - node-output.ts: the persisted-output byte cap and spill, the execution output a node persists, and the artifact-pointer rejection. dry-run.ts now imports the text functions from dag/references.ts. The five reference suites move to dag/references.test.ts with their 63 tests unchanged. No behavior change.
Step D5 of the dag-executor split (part of #1140). The declarations move verbatim into two owners under packages/workflows/src/dag/: - run-control.ts: the cancel-check and activity-heartbeat throttles, the streaming status policy, the retry wait that watches for a stopped run, and the subprocess runner that ends a node's process tree when its run stops. - retry.ts: the node retry defaults and config, retryable-failure classification, the budgeted attempt, and the retry loops for AI and deterministic nodes. The streaming-policy and stopped-subprocess suites move to dag/run-control.test.ts (9 tests) and the two retry suites to dag/retry.test.ts (33 tests), unchanged apart from the streaming suite's dynamic import now naming run-control. No behavior change.
Step D6 of the dag-executor split (part of #1140). The declarations move verbatim into two owners under packages/workflows/src/dag/: - checkout-guard.ts: the checkout snapshot and its engine-write exclusions, and the mutates_checkout: false assertion. - trigger-rule.ts: trigger-rule decisions for a node's upstreams and the composed-block boundary check. dry-run.ts now imports checkTriggerRule and checkComposedBlockBoundaries from dag/trigger-rule.ts. The three checkTriggerRule suites move to dag/trigger-rule.test.ts (18 tests) and the mutates_checkout suite to dag/checkout-guard.test.ts (13 tests), unchanged. No behavior change.
….ts; one provider-invocation classifier Step D7 of the dag-executor split (part of #1140). The declarations move into two owners under packages/workflows/src/dag/: - provider-resolution.ts: preset options, node MCP resolution, per-node provider and model resolution, and the scoped-capability checks. - capability-preflight.ts: the provider-invoking node walk and the container, scoped-capability and strict-schema preflights. resolveNodeProviderAndModel now takes (ctx, node, { resolveAiText, warnedProviderConflicts }) instead of fifteen positionals; the unused cwd is gone. The approval rework call still passes text => text and no conflict set. The capability walker and the credential walker in run-preflight.ts each kept their own list of node kinds that reach a provider, in sync by hand. Both now select nodes through providerInvocation, an exhaustive switch on node.kind, so a new node kind does not compile until it is classified. The selected set is unchanged: agent, loop, loop_group, and a gate with rework. Each walker keeps its own resolution scope. Twelve suites move to dag/provider-resolution.test.ts (62 tests) and dag/capability-preflight.test.ts (35 tests) unchanged apart from resolveNodeMcp's dynamic import; a new providerInvocation suite feeds every node kind through the classifier. The stale resolveNodeModel docstring now says it is the one chain execution, dry run and run preflight share. No behavior change.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 76.47% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 68 functions across 24 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main change: moving references, run control, checkout guarding, and provider resolution out of dag-executor.ts. Description check The description includes the required Problem and outcome, Review guidance, Solution, and Validation sections. It also states scope, invariants, changed seams, test results, and remaining verification… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description includes the required Problem and outcome, Review guidance, Solution, and Validation sections. It also states scope, invariants, changed seams, test results, and remaining verification limits.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Reviewed commit: Review reportVerdictReady: true. Action: Accepted contractSource: issue #4505 (part of #1140), PR #4516, base Reviewed head SHAContract coverage
FindingsNone. No blocking findings, no notes. Rejected findingsNone raised. Discoveries
Review coverageRoute reason: first round; the diff moves retry, checkout-guard, trigger-rule and provider-resolution code and adds a shared classifier, so seams, tests and errors were selected alongside code; docs was off because there is no user-facing change. Selected: seams, code, tests, errors. All four ran, each has a completed |
Problem and outcome
packages/workflows/src/dag-executor.tsis still a 12,000-line file that mixes reference resolution, retry, checkout guarding, trigger rules and provider resolution with the executor itself. This is steps D4–D7 of lane D of #1140, landed together, one commit per step.packages/workflows/src/dag/with its tests beside it.dag-executor.tsshrinks from 12,370 to 10,115 lines anddag-executor.test.tsfrom 43,258 to 38,453.executeDagWorkflow) stays where it is. Importers point at the new owners;dag-executor.tskeeps no re-exports.packages/workflows.Review guidance
resolveNodeProviderAndModelsignature. D4–D6 are mechanical.packages/workflows/src/dag/capability-preflight.ts(providerInvocation). It is the only non-verbatim design change.a7d4fb8fd(D7), then skim D435edb2c7d, D5ee41f14d2and D67087f7911.export, re-wrapped signatures, and oneimport('../schemas/node-execution')type path. Tests moved the same way.bun run test <file>), as the package runner does. Therun-controlstopped-subprocess test times out ifretry.test.tsshares its process.Solution
Each step moves its declarations into the modules the plan names:
dag/references.tsanddag/node-output.ts.dry-run.tsnow imports the pure text functions fromreferencesand no longer loads them through the executor.dag/run-control.tsanddag/retry.ts.dag/checkout-guard.tsanddag/trigger-rule.ts.dag/provider-resolution.tsanddag/capability-preflight.ts. Two things change beyond the move:resolveNodeProviderAndModel(ctx, node, { resolveAiText, warnedProviderConflicts })replaces fifteen positional arguments, and the unused_cwdis gone. The approval rework call still passestext => textand no conflict set.run-preflight.tseach listed the node kinds that reach a provider, kept in sync by hand. Both now select nodes through one exhaustiveswitch (node.kind)classifier,providerInvocation. A new node kind no longer compiles until it is classified. The selected set is unchanged: agent, loop, loop_group and a gate with rework. Each walker keeps its own traversal and resolution scope.resolveNodeModeldocstring now says it is the one chain that execution, dry run and run preflight share.Changed seams
capability-preflight/run-preflight→ node-kind selectiondag/capability-preflight.tsproviderInvocation; newproviderInvocationsuite feeds every node kind through itrunLayers/executeApprovalNode→resolveNodeProviderAndModel(ctx, node, { resolveAiText, warnedProviderConflicts })dag-executor.tsValidation
bun x tsc --noEmit -p .inpackages/workflows— clean.bun x biome lint --error-on-warnings packages/workflows/src— clean.bun run check:ratchet— passed, with noRatchet-Allowtrailer.bun run test <file>per file inpackages/workflows— 0 failures:dag-executor.test.ts(817),dag/references(63),dag/run-control(9),dag/retry(33),dag/trigger-rule(18),dag/checkout-guard(13),dag/provider-resolution(62),dag/capability-preflight(37),run-preflight(29),dry-run(115).packages/coreprocess-preflight.test.ts(1) andprocess-registration.test.ts(85) — pass; they exercise credential requirements.dag-executor.test.tswent from 1,050 to 817 tests, and 233 tests moved unchanged into the new files (817 + 233 = 1,050). No test was deleted.capability-preflight.test.tsadds 2 newproviderInvocationtests: one feeds everyDagNode['kind']through the classifier and asserts the selected set, the other covers a gate without rework.caseis a compile error. Classifyingworkflowasinvokesfails the new test.Links
Summary by CodeRabbit
New Features
Bug Fixes