Feat/script stack#55
Merged
Merged
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces work-item handler dispatch with script and decorator stack execution. WorkItems use ChangesScript stack migration
Dynamic package publishing
Sequence Diagram(s)sequenceDiagram
participant Runner
participant DispatchHandler
participant ScriptStack
participant ScriptContext
participant Script
Runner->>DispatchHandler: receive WorkItem dispatch
DispatchHandler->>ScriptStack: resolve kind, flow, and conventions
DispatchHandler->>ScriptContext: create live WorkItem context
DispatchHandler->>ScriptStack: execute composed stack
ScriptStack->>Script: invoke decorators and script
Script->>ScriptContext: persist state updates
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
orchestrator-v2/publish.js (1)
47-69: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against directories without a
package.json(and packages without aname).
discoverPublishablePackagesunconditionally readsjoin(dir, "package.json")for every subdirectory underpackagesDir. Any non-package folder (build output, tooling,.turbo, etc.) makesreadFilethrow and aborts the entire publish. Additionally, apackage.jsonwithout anamepasses the filter on Line 61 (pkg.name?.includes("example")is falsy) and gets pushed withname: undefined, which later becomes anundefinedkey inpackageByNameand a bogus entry in the topo sort.♻️ Proposed hardening
const dir = join(packagesDir, entry.name); const pkgPath = join(dir, "package.json"); + let pkg; + try { // oxlint-disable-next-line no-await-in-loop - const pkg = JSON.parse(await readFile(pkgPath, "utf-8")); + pkg = JSON.parse(await readFile(pkgPath, "utf-8")); + } catch { + continue; // not a publishable package directory + } - if (pkg.private || pkg.name?.includes("example")) { + if (!pkg.name || pkg.private || pkg.name.includes("example")) { continue; }🤖 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 `@orchestrator-v2/publish.js` around lines 47 - 69, Harden discoverPublishablePackages by checking whether each directory contains package.json before reading it, skipping directories where the file is absent or unreadable instead of aborting discovery. Also skip parsed packages whose pkg.name is missing or not a valid publishable name, then apply the existing private/example filters before pushing entries so packageByName and dependency sorting never receive undefined names.orchestrator-v2/packages/runner/src/script-stack.spec.ts (1)
114-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGiven functions duplicate when-function setup and fire off unawaited promises.
Three
givenfunctions (nested_decorators,skip_decorator,flaky_script_and_retry_decorator) fully duplicate the registry/work-item setup found in their correspondingwhenfunctions, then firecomposeStack(...).then(...)orexecuteScriptStack(...).then(...)as unawaited promises. These fire-and-forget calls race with thewhenfunctions and can overwritethis.resultnon-deterministically. Thegivenfunctions should only set up preconditions; execution should live exclusively in thewhenfunctions.♻️ Proposed refactor: move execution out of given functions
Example for
nested_decorators/executing_nested_stack:function nested_decorators(this: Context) { const order: string[] = []; this.workItem = { ...baseWorkItem(), flow: ["outer", "inner"] }; this.scripts = new Registry<ScriptFn>(); this.decorators = new Registry<DecoratorFn>(); this.scripts.register("hunt", async () => { order.push("script"); return "done"; }); this.decorators.register("outer", async (_wi, _ctx, next) => { order.push("outer-before"); await next(); order.push("outer-after"); }); this.decorators.register("inner", async (_wi, _ctx, next) => { order.push("inner-before"); await next(); order.push("inner-after"); }); - this.result = order; - const stack = resolveStack(this.workItem, this.scripts, this.decorators, []); - void composeStack(this.workItem, scriptContext, stack.script, stack.decorators)().then(() => { - this.result = order; - }); + this.result = order; // store the order array reference for the when function } async function executing_nested_stack(this: Context) { - const order: string[] = []; - this.workItem = { ...baseWorkItem(), flow: ["outer", "inner"] }; - this.scripts = new Registry<ScriptFn>(); - this.decorators = new Registry<DecoratorFn>(); - - this.scripts.register("hunt", async () => { - order.push("script"); - return "done"; - }); - - this.decorators.register("outer", async (_wi, _ctx, next) => { - order.push("outer-before"); - await next(); - order.push("outer-after"); - }); - - this.decorators.register("inner", async (_wi, _ctx, next) => { - order.push("inner-before"); - await next(); - order.push("inner-after"); - }); - const stack = resolveStack(this.workItem, this.scripts, this.decorators, []); await executeScriptStack(this.workItem, scriptContext, stack); - this.result = order; }Apply the same pattern to
skip_decorator/executing_short_circuitandflaky_script_and_retry_decorator/executing_with_retry.Also applies to: 182-201, 224-256
🤖 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 `@orchestrator-v2/packages/runner/src/script-stack.spec.ts` around lines 114 - 142, Remove the duplicated registry and work-item setup and all fire-and-forget execution from the given functions nested_decorators, skip_decorator, and flaky_script_and_retry_decorator. Keep those functions limited to establishing preconditions, and move composeStack/executeScriptStack invocation and result assignment exclusively into executing_nested_stack, executing_short_circuit, and executing_with_retry, awaiting completion before setting this.result.
🤖 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 `@orchestrator-v2/packages/interfaces-work/src/types.ts`:
- Around line 68-69: Update the flow-entry validation in isWorkItem to require
each entry to be a non-empty string, matching missingWorkItemFields by checking
typeof entry === "string" and entry.length > 0; this ensures empty flow values
are rejected before dispatch reaches resolveStack.
In `@orchestrator-v2/packages/runner/src/dispatch-handler.ts`:
- Around line 26-31: Update the dispatch subscription callback and the
post-acceptance flow in handleDispatch so errors are not silently swallowed.
Preserve the initial accepted response, then wrap createScriptContext,
executeScriptStack, and related stack.rpc.call execution in try/catch; on
failure, send a terminal workItem.fail or workItem.pause outcome through the
existing RPC mechanism. Add a default case to the execution switch, and ensure
the outer catch logs or reports failures rather than using catch(() =>
undefined).
---
Nitpick comments:
In `@orchestrator-v2/packages/runner/src/script-stack.spec.ts`:
- Around line 114-142: Remove the duplicated registry and work-item setup and
all fire-and-forget execution from the given functions nested_decorators,
skip_decorator, and flaky_script_and_retry_decorator. Keep those functions
limited to establishing preconditions, and move composeStack/executeScriptStack
invocation and result assignment exclusively into executing_nested_stack,
executing_short_circuit, and executing_with_retry, awaiting completion before
setting this.result.
In `@orchestrator-v2/publish.js`:
- Around line 47-69: Harden discoverPublishablePackages by checking whether each
directory contains package.json before reading it, skipping directories where
the file is absent or unreadable instead of aborting discovery. Also skip parsed
packages whose pkg.name is missing or not a valid publishable name, then apply
the existing private/example filters before pushing entries so packageByName and
dependency sorting never receive undefined names.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ff16c326-c8f8-475f-a2c8-a95e58003a87
📒 Files selected for processing (30)
orchestrator-v2/README.mdorchestrator-v2/docs/runner.mdorchestrator-v2/docs/temporal/script-stack-caveman.mdorchestrator-v2/docs/temporal/script-stack-eli5.mdorchestrator-v2/docs/temporal/script-stack.mdorchestrator-v2/examples/lvl3/doSomething.tsorchestrator-v2/examples/lvl3/runner.tsorchestrator-v2/packages/agent-3-task/src/augment.spec.tsorchestrator-v2/packages/agent-3-task/src/augment.tsorchestrator-v2/packages/agent-3-task/src/create-task-agent.tsorchestrator-v2/packages/agent-3-task/src/run-task-agent.spec.tsorchestrator-v2/packages/agent-3-task/src/run-task-agent.tsorchestrator-v2/packages/interfaces-work/src/index.tsorchestrator-v2/packages/interfaces-work/src/types.tsorchestrator-v2/packages/orchestrator/src/test-helpers.tsorchestrator-v2/packages/runner/README.mdorchestrator-v2/packages/runner/src/conventions/fail-on-error.tsorchestrator-v2/packages/runner/src/dispatch-handler.tsorchestrator-v2/packages/runner/src/execute-work-item.tsorchestrator-v2/packages/runner/src/index.tsorchestrator-v2/packages/runner/src/runner.spec.tsorchestrator-v2/packages/runner/src/runner.tsorchestrator-v2/packages/runner/src/script-agent.tsorchestrator-v2/packages/runner/src/script-context.tsorchestrator-v2/packages/runner/src/script-stack.spec.tsorchestrator-v2/packages/runner/src/script-stack.tsorchestrator-v2/packages/runner/src/work-item-execution-context.tsorchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.spec.tsorchestrator-v2/packages/work-item-source-bifrost/src/bifrost-work-item-source.tsorchestrator-v2/publish.js
💤 Files with no reviewable changes (2)
- orchestrator-v2/packages/runner/src/execute-work-item.ts
- orchestrator-v2/packages/runner/src/work-item-execution-context.ts
…covery. Reject empty flow entries before resolveStack, report dispatch failures via workItem.fail instead of swallowing errors, dedupe script-stack test setup, and skip invalid package directories during publish.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@orchestrator-v2/publish.js`:
- Around line 63-72: Update the error handlers surrounding package reads and
JSON.parse in the package discovery flow to log failures before continuing,
ensuring parse errors include the affected package path and error details.
Preserve the existing continue behavior for unreadable or invalid package files
while making skipped packages distinguishable from directories without
package.json.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 736d2ce5-2832-44ac-bdaf-b5969ce96236
📒 Files selected for processing (4)
orchestrator-v2/packages/interfaces-work/src/types.tsorchestrator-v2/packages/runner/src/dispatch-handler.tsorchestrator-v2/packages/runner/src/script-stack.spec.tsorchestrator-v2/publish.js
🚧 Files skipped from review as they are similar to previous changes (3)
- orchestrator-v2/packages/runner/src/script-stack.spec.ts
- orchestrator-v2/packages/interfaces-work/src/types.ts
- orchestrator-v2/packages/runner/src/dispatch-handler.ts
Comment on lines
+63
to
+72
| } catch { | ||
| continue; | ||
| } | ||
|
|
||
| let pkg; | ||
| try { | ||
| pkg = JSON.parse(contents); | ||
| } catch { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Log when packages are skipped due to read or parse failures.
Both catch blocks silently continue, making it impossible to distinguish "no package.json here (expected)" from "package.json is corrupted (unexpected)." A corrupted package.json in a real package would silently drop it from the publish order, potentially breaking dependent packages. At minimum, log the JSON parse failure.
🛡️ Suggested improvement
try {
// oxlint-disable-next-line no-await-in-loop
contents = await readFile(pkgPath, "utf-8");
} catch {
continue;
}
let pkg;
try {
pkg = JSON.parse(contents);
} catch {
+ console.warn(`Skipping ${entry.name}: package.json is not valid JSON`);
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch { | |
| continue; | |
| } | |
| let pkg; | |
| try { | |
| pkg = JSON.parse(contents); | |
| } catch { | |
| continue; | |
| } | |
| } catch { | |
| continue; | |
| } | |
| let pkg; | |
| try { | |
| pkg = JSON.parse(contents); | |
| } catch { | |
| console.warn(`Skipping ${entry.name}: package.json is not valid JSON`); | |
| continue; | |
| } |
🤖 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 `@orchestrator-v2/publish.js` around lines 63 - 72, Update the error handlers
surrounding package reads and JSON.parse in the package discovery flow to log
failures before continuing, ensuring parse errors include the affected package
path and error details. Preserve the existing continue behavior for unreadable
or invalid package files while making skipped packages distinguishable from
directories without package.json.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Motivation
Type of change
Checklist
go/npxwere not used — everything went throughmake/npm runmake lintpassesmake testpassesmake buildpassesmain; rebased on latestmainNotes for reviewers