Feat/script stack#55
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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}
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
| !Array.isArray(record.flow) || | ||
| !record.flow.every((entry) => typeof entry === "string") || |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
isWorkItem allows empty-string flow entries while missingWorkItemFields rejects them.
isWorkItem at line 69 only checks typeof entry === "string" but missingWorkItemFields at line 113 checks typeof entry === "string" && entry.length > 0. A work item with flow: [""] passes isWorkItem (used as validateWorkItem in dispatch), proceeds to resolveStack, and throws "Unknown decorator: " — an unhelpful error. The validation gate should reject empty flow entries consistently.
🐛 Proposed fix to reject empty strings in `isWorkItem`
!Array.isArray(record.flow) ||
- !record.flow.every((entry) => typeof entry === "string") ||
+ !record.flow.every((entry) => typeof entry === "string" && entry.length > 0) ||📝 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.
| !Array.isArray(record.flow) || | |
| !record.flow.every((entry) => typeof entry === "string") || | |
| !Array.isArray(record.flow) || | |
| !record.flow.every((entry) => typeof entry === "string" && entry.length > 0) || |
🤖 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/interfaces-work/src/types.ts` around lines 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.
| return peer.subscribe( | ||
| (payload) => payload.kind === "rpc.request" && payload.method === "dispatch", | ||
| (payload) => { | ||
| void handleDispatch(peer, handlers, data, rpc, payload).catch(() => undefined); | ||
| void handleDispatch(peer, stack, payload).catch(() => undefined); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Errors after accepted: true are silently swallowed, orphaning work items.
The .catch(() => undefined) at line 29 swallows all errors from handleDispatch. After sendRpcResponse(peer, payload.id, { accepted: true }) at line 65, if createScriptContext, executeScriptStack, or stack.rpc.call throws, the orchestrator receives accepted: true but never gets a terminal outcome (workItem.complete/fail/pause). The work item is stuck in limbo.
The default failOnError convention catches script-thrown errors, but does not cover createScriptContext failures, RPC network errors, or scenarios where failOnError is removed from conventions. The switch also lacks a default case.
🔒 Proposed fix: wrap post-acceptance execution in try/catch
sendRpcResponse(peer, payload.id, { accepted: true });
- const { workItem: liveWorkItem, ctx } = createScriptContext(workItem, stack.rpc, stack.data);
- const result = await executeScriptStack(liveWorkItem, ctx, resolved);
-
- switch (result.outcome) {
- case "completed":
- await stack.rpc.call("workItem.complete", { workItemId: workItem.workItemId });
- break;
- case "failed":
- await stack.rpc.call("workItem.fail", {
- workItemId: workItem.workItemId,
- message: result.message ?? "failed",
- });
- break;
- case "paused":
- await stack.rpc.call("workItem.pause", { workItemId: workItem.workItemId });
- break;
+ try {
+ const { workItem: liveWorkItem, ctx } = createScriptContext(workItem, stack.rpc, stack.data);
+ const result = await executeScriptStack(liveWorkItem, ctx, resolved);
+
+ switch (result.outcome) {
+ case "completed":
+ await stack.rpc.call("workItem.complete", { workItemId: workItem.workItemId });
+ break;
+ case "failed":
+ await stack.rpc.call("workItem.fail", {
+ workItemId: workItem.workItemId,
+ message: result.message ?? "failed",
+ });
+ break;
+ case "paused":
+ await stack.rpc.call("workItem.pause", { workItemId: workItem.workItemId });
+ break;
+ default:
+ await stack.rpc.call("workItem.fail", {
+ workItemId: workItem.workItemId,
+ message: `Unknown outcome: ${String(result.outcome)}`,
+ });
+ }
+ } catch (error) {
+ const message = error instanceof Error ? error.message : String(error);
+ await stack.rpc.call("workItem.fail", {
+ workItemId: workItem.workItemId,
+ message,
+ });
}Also applies to: 65-83
🤖 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/dispatch-handler.ts` around lines 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).
1 participant
Summary
Motivation
Type of change
Checklist
go/npxwere not used — everything went throughmake/npm runmake lintpassesmake testpassesmake buildpassesmain; rebased on latestmainNotes for reviewers