Extract shared task-graph coordination base class from FlowExecution / TaskExecution#33
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedPull request was closed or merged during review WalkthroughThis pull request refactors the executor architecture by introducing Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/executors/shared.ts (1)
72-200: Let TypeScript infer these internal helper return types.Most of these new
protectedhelpers are implementation details, so the added: void,: boolean,: Error, and: Promise<unknown>annotations add maintenance noise without strengthening the public surface.As per coding guidelines,
**/*.{ts,tsx}: Prefer type inference whenever possible in TypeScript. Do not add explicit return types unless required by tooling, declaration emit, or a public API contract. Do not use explicit return types when inference already produces the correct result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/shared.ts` around lines 72 - 200, Remove redundant explicit return-type annotations on internal helper methods so TypeScript can infer them: drop the explicit return types for getTaskHandler (keep param types), createResultProxy, waitForResult, storeResult, storeError, resolveWaiters, rejectWaiters, abortInternal, onTaskResult, onTaskError, mapStoredError, shouldAbortOnTaskError, and shouldRethrowTaskError; leave public/abstract API signatures that need a declared type (e.g., createTaskContext) intact. Ensure only implementation-internal helpers lose the ": void", ": boolean", ": Error", and ": Promise<unknown>" annotations referenced above.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/executors/flow.ts`:
- Around line 18-29: The execute() method starts all tasks but uses await
Promise.allSettled(promises) which prevents returning early on a FlowExitSignal;
change execute() to short-circuit when this.firstRejection is set (especially if
it's a FlowExitSignal) by racing the full-settle promise against a
"firstRejection" notifier: create a promise (or helper like
waitForFirstRejection) that resolves as soon as this.firstRejection is assigned
and then use Promise.race([Promise.allSettled(promises),
waitForFirstRejection()]); after the race, if this.firstRejection is a
FlowExitSignal return its value (as FlowResult<T>) otherwise throw the rejection
— reference execute(), this.firstRejection, FlowExitSignal, runTask(), and
Promise.allSettled() in the change.
In `@src/lib/executors/shared.ts`:
- Around line 136-138: storeError currently stores the raw unknown and calls
rejectWaiters with that value, causing inconsistent rejections vs waitForResult;
change storeError to normalize the error first by calling
this.mapStoredError(error), then this.errors.set(taskName, mappedError) and
this.rejectWaiters(taskName, mappedError). Apply the same change to the other
place in the file where an error is set and waiters are rejected (the second
occurrence that mirrors storeError).
- Around line 249-261: The execute() path currently always awaits
Promise.allSettled(promises), which prevents fail-fast behavior and can lead to
throwing undefined; change the logic so that when this.#mode !== "settled"
(i.e., fail-fast) you await Promise.all(promises) to let the first rejection
propagate immediately, while keeping Promise.allSettled(promises) for the
settled path; additionally, when checking this.firstRejection ensure you never
throw undefined by throwing this.firstRejection ?? new Error("Task failed")
(refer to execute, this.#mode, this.runTask, this.firstRejection, and
this.#returnValue).
---
Nitpick comments:
In `@src/lib/executors/shared.ts`:
- Around line 72-200: Remove redundant explicit return-type annotations on
internal helper methods so TypeScript can infer them: drop the explicit return
types for getTaskHandler (keep param types), createResultProxy, waitForResult,
storeResult, storeError, resolveWaiters, rejectWaiters, abortInternal,
onTaskResult, onTaskError, mapStoredError, shouldAbortOnTaskError, and
shouldRethrowTaskError; leave public/abstract API signatures that need a
declared type (e.g., createTaskContext) intact. Ensure only
implementation-internal helpers lose the ": void", ": boolean", ": Error", and
": Promise<unknown>" annotations referenced above.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9489db9-e67e-41fd-afac-4bc805140aaa
📒 Files selected for processing (3)
.plans/01-pr-18-review-triage-checklist.mdsrc/lib/executors/flow.tssrc/lib/executors/shared.ts
💤 Files with no reviewable changes (1)
- .plans/01-pr-18-review-triage-checklist.md
adelrodriguez
force-pushed
the
03-07-extract_shared_task-graph_coordination_base_class_from_flowexecution___taskexecution_
branch
2 times, most recently
from
4835355 to
006edfb
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 006edfb3dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
adelrodriguez
force-pushed
the
03-07-extract_shared_task-graph_coordination_base_class_from_flowexecution___taskexecution_
branch
from
006edfb to
1b7c4ec
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/executors/shared.ts (1)
255-270: Unnecessaryasyncwrapper on line 256.Since
runTaskis already an async method returningPromise<void>, theasync (name) =>wrapper is redundant and adds a slight overhead.🔧 Suggested simplification
async execute(): Promise<Record<string, unknown>> { - const promises = this.taskNames.map(async (name) => this.runTask(name)) + const promises = this.taskNames.map((name) => this.runTask(name)) if (this.#mode === "settled") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/shared.ts` around lines 255 - 270, The map callback in execute() is using an unnecessary async wrapper (async (name) => this.runTask(name)); since runTask(name) already returns a Promise, replace the wrapper with a direct call to runTask to avoid the extra microtask: e.g. create promises with this.taskNames.map(name => this.runTask(name)) (or this.taskNames.map(this.runTask.bind(this)) if you prefer passing the method directly) so Promise.all/Promise.allSettled receive the original promises from runTask.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/executors/shared.ts`:
- Around line 255-270: The map callback in execute() is using an unnecessary
async wrapper (async (name) => this.runTask(name)); since runTask(name) already
returns a Promise, replace the wrapper with a direct call to runTask to avoid
the extra microtask: e.g. create promises with this.taskNames.map(name =>
this.runTask(name)) (or this.taskNames.map(this.runTask.bind(this)) if you
prefer passing the method directly) so Promise.all/Promise.allSettled receive
the original promises from runTask.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e94d24c9-7daa-45d3-a742-64ae4763f105
📒 Files selected for processing (8)
.plans/01-pr-18-review-triage-checklist.mdsrc/__tests__/index.test.tssrc/index.tssrc/lib/executors/__tests__/all.test.tssrc/lib/executors/__tests__/flow.test.tssrc/lib/executors/__tests__/shared.test.tssrc/lib/executors/flow.tssrc/lib/executors/shared.ts
💤 Files with no reviewable changes (2)
- .plans/01-pr-18-review-triage-checklist.md
- src/index.ts
adelrodriguez
force-pushed
the
03-07-extract_shared_task-graph_coordination_base_class_from_flowexecution___taskexecution_
branch
from
1b7c4ec to
f945f82
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b7c4ecc69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f945f82520
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
adelrodriguez
force-pushed
the
03-07-extract_shared_task-graph_coordination_base_class_from_flowexecution___taskexecution_
branch
from
f945f82 to
a90f927
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a90f927ac9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…` / `TaskExecution`
adelrodriguez
force-pushed
the
03-07-extract_shared_task-graph_coordination_base_class_from_flowexecution___taskexecution_
branch
from
a90f927 to
088550a
Compare
Merge activity
|
adelrodriguez
deleted the
03-07-extract_shared_task-graph_coordination_base_class_from_flowexecution___taskexecution_
branch
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 088550a82d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (catchFn) { | ||
| const context = { | ||
| failedTask: execution.failedTask, | ||
| partial: execution.returnValue as Partial<AllValue<T>>, |
There was a problem hiding this comment.
ctx.partial now points at the live execution.returnValue object while sibling tasks are still running, so an async catch handler can observe different values depending on timing (e.g., ctx.partial.a is undefined before an await and populated after). This makes catch-mapping nondeterministic in fail-fast runs introduced by this refactor, because the handler no longer receives a stable failure-time view of partial results.
Useful? React with 👍 / 👎.
1 participant
No description provided.