Replace ConfigurationError with coded Panic system#22
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
WalkthroughThis PR replaces ConfigurationError with a new Panic error model (class Panic, PanicCode, PanicMessages) and updates error constructors to use message+options. It adds invariant() and changes assertUnreachable to accept PanicCode. Many runtime checks and tests were converted to throw/expect Panic codes (e.g., WRAP_UNAVAILABLE, RUN_SYNC_TRY_PROMISE, FLOW_NO_EXIT, TASK_UNKNOWN_REFERENCE). The retry API was renamed: normalizeRetryPolicy/retryOptions → createRetryPolicy. Public exports updated accordingly (added Panic/PanicCode/createRetryPolicy; removed ConfigurationError/retryOptions). Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
264-311:⚠️ Potential issue | 🟡 MinorRemove returned
ConfigErrorsfrom the documentedrunsignatures.The new Panic notes are correct, but the
Common signaturessection still saysrun(...)returnsConfigErrors. That leaves the public contract internally inconsistent after the Panic migration.📝 Suggested doc fix
-- `run(tryFn)` -> `Promise<T | UnhandledException | ConfigErrors>` -- `run({ try, catch })` -> `Promise<T | C | ConfigErrors>` +- `run(tryFn)` -> `Promise<T | UnhandledException>` +- `run({ try, catch })` -> `Promise<T | C>`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 264 - 311, Update the "Common signatures" docs to remove any reference to returned `ConfigErrors` from the `run` signatures: change the listed signatures so `run(tryFn)` returns `Promise<T | UnhandledException>` and `run({ try, catch })` returns `Promise<T | C | UnhandledException>` (or equivalent generic `C` for catch results), leaving `runSync`, `all`, `settled().all`, and `flow` unchanged; update any nearby lines in the same "Common signatures" block that still mention `ConfigErrors` so the public contract is consistent with the Panic migration.
🧹 Nitpick comments (1)
src/lib/executors/__tests__/retry.test.ts (1)
175-206: Use the sharedexpectPanichelper for these cases.Both tests duplicate the same
try/catch+Panic.codeassertion flow. Switching them to the helper added in this PR would keep the panic assertions shorter and centralized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/__tests__/retry.test.ts` around lines 175 - 206, Replace the duplicated try/catch panic assertions in the two tests that call executeRunSync with the shared expectPanic helper: call expectPanic(() => executeRunSync(...), "RUN_SYNC_ASYNC_RETRY_POLICY") (or the helper's signature used in the test suite) in the tests titled "throws Panic when sync runner is used with async-required retry policy" and "throws Panic when sync runner is used with jittered constant retry" so both assertions use expectPanic instead of manual try/catch and explicit Panic.code checks.
🤖 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/__tests__/types.test.ts`:
- Around line 153-169: Add a type-regression test to cover the renamed
retry-policy export: in src/__tests__/types.test.ts add a new conditional
typecheck block (similar to the existing "panic" block) that references
createRetryPolicy to assert its exported type (e.g., declare a const rp =
try$.createRetryPolicy(...) and use Expect/Equal to pin the expected
return/parameter types) and include a line with // `@ts-expect-error` to assert
that try$.retryOptions no longer exists; use the unique symbols
createRetryPolicy and retryOptions so the test fails if the public API drifts.
In `@src/index.ts`:
- Around line 27-29: Add compile-time pins for the new root exports by updating
src/__tests__/types.test.ts to import createRetryPolicy and the PanicCode type
from the package root and assert their presence at type-check time;
specifically, import createRetryPolicy (value) and PanicCode (type) and create
minimal type-level assertions or dummy assignments that will fail TS compilation
if those exports are missing or change signature, ensuring the public API change
is covered by the test suite.
In `@src/lib/errors.ts`:
- Around line 50-76: The constructors for CancellationError, TimeoutError,
RetryExhaustedError, and UnhandledException must remain backward-compatible with
the legacy call shape where the first argument may be an options object (e.g.,
new UnhandledException({ cause })); update each constructor (e.g., in class
UnhandledException and RetryExhaustedError) to accept (messageOrOptions?: string
| ErrorOptions, options?: ErrorOptions), detect if the first arg is an object
and no message string was provided, then normalize to message and options (use
the default message when message is omitted) before calling super and setting
name; this preserves JS/TS compatibility without forcing consumers to change,
and if you instead keep the new strict signature remember to pin the API types
in src/__tests__/types.test.ts.
In `@src/lib/executors/all.ts`:
- Around line 48-64: The async branch currently awaits the catch handler promise
(mapped) directly which allows long-running catch handlers to ignore the
builder's timeout/abort and masks timeout/cancellation as
ALL_CATCH_HANDLER_REJECT; in the checkIsPromiseLike(mapped) branch wrap mapped
with the executor's existing timeout/cancellation mechanism (race mapped against
the builder's timeout and abort signal or use the helper that enforces the same
AbortSignal/timeout used for the rest of all(...)), so that a TimeoutError or
CancellationError is surfaced instead of ALL_CATCH_HANDLER_REJECT; preserve the
existing Panic("ALL_CATCH_HANDLER_THROW", ...) for synchronous throwers and only
convert mapped rejections that are real handler errors, while letting
timeout/cancellation propagate from the new wrapper (refer to catchFn, mapped,
checkIsPromiseLike, Panic, ALL_CATCH_HANDLER_REJECT).
In `@src/lib/executors/flow.ts`:
- Around line 203-208: The current code inspects the resolved value from
this.#executeAttempt() with instanceof to detect cancellation/timeout, which
misclassifies legitimate $exit(new TimeoutError()/CancellationError()) as
failures; instead, await this.#executeAttempt() inside a try/catch and only
rethrow TimeoutError or CancellationError if they were thrown (i.e., caught),
not when returned as a value. Remove the instanceof checks on the resolved value
in the shown block (and the similar checks around the other occurrence near the
242-244 region) so that returned TimeoutError/CancellationError values are
treated as normal successful results and only thrown exceptions are propagated
as aborts.
In `@src/lib/executors/gen.ts`:
- Around line 28-29: The catch blocks in gen currently wrap all throwables into
UnhandledException, which loses control-flow errors; update each catch (the ones
returning UnhandledException at the spots shown) to first detect control errors
(if error is instance of Panic, CancellationError, or TimeoutError) and
return/propagate that error as-is cast to GenErrors<TYield>, and only construct
new UnhandledException for unknown throwables; apply the same change to all
similar catch sites in gen.ts (the returns at the indicated locations).
In `@src/lib/executors/run-sync.ts`:
- Around line 214-219: The try/catch around invoking catchFn currently wraps any
thrown Panic into a new Panic with code RUN_SYNC_CATCH_HANDLER_THROW, which
hides the original RUN_SYNC_CATCH_PROMISE; update the catch block in the
run-sync path so that if the caught error (catchError) is an instance of Panic
with the code "RUN_SYNC_CATCH_PROMISE" it is rethrown unchanged, otherwise wrap
it into new Panic("RUN_SYNC_CATCH_HANDLER_THROW", { cause: catchError }); keep
references to catchFn, assertNotPromiseLike, mapped, Panic,
RUN_SYNC_CATCH_PROMISE and RUN_SYNC_CATCH_HANDLER_THROW to locate and implement
this conditional rethrow.
---
Outside diff comments:
In `@README.md`:
- Around line 264-311: Update the "Common signatures" docs to remove any
reference to returned `ConfigErrors` from the `run` signatures: change the
listed signatures so `run(tryFn)` returns `Promise<T | UnhandledException>` and
`run({ try, catch })` returns `Promise<T | C | UnhandledException>` (or
equivalent generic `C` for catch results), leaving `runSync`, `all`,
`settled().all`, and `flow` unchanged; update any nearby lines in the same
"Common signatures" block that still mention `ConfigErrors` so the public
contract is consistent with the Panic migration.
---
Nitpick comments:
In `@src/lib/executors/__tests__/retry.test.ts`:
- Around line 175-206: Replace the duplicated try/catch panic assertions in the
two tests that call executeRunSync with the shared expectPanic helper: call
expectPanic(() => executeRunSync(...), "RUN_SYNC_ASYNC_RETRY_POLICY") (or the
helper's signature used in the test suite) in the tests titled "throws Panic
when sync runner is used with async-required retry policy" and "throws Panic
when sync runner is used with jittered constant retry" so both assertions use
expectPanic instead of manual try/catch and explicit Panic.code checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20537298-3590-45e3-9065-ef830290b8c3
📒 Files selected for processing (26)
README.mdsrc/__tests__/index.test.tssrc/__tests__/types.test.tssrc/index.tssrc/lib/__tests__/utils.test.tssrc/lib/builder.tssrc/lib/errors.tssrc/lib/executors/__tests__/all.test.tssrc/lib/executors/__tests__/flow.test.tssrc/lib/executors/__tests__/retry.test.tssrc/lib/executors/__tests__/run-sync.test.tssrc/lib/executors/__tests__/run.test.tssrc/lib/executors/__tests__/shared.test.tssrc/lib/executors/all.tssrc/lib/executors/base.tssrc/lib/executors/flow.tssrc/lib/executors/gen.tssrc/lib/executors/run-sync.tssrc/lib/executors/run.tssrc/lib/executors/shared.tssrc/lib/modifiers/__tests__/retry.test.tssrc/lib/modifiers/retry.tssrc/lib/modifiers/signal.tssrc/lib/modifiers/timeout.tssrc/lib/types/builder.tssrc/lib/utils.ts
💤 Files with no reviewable changes (1)
- src/lib/types/builder.ts
adelrodriguez
force-pushed
the
03-06-replace_configurationerror_with_coded_panic_for_better_diagnostics
branch
from
1ae39b1 to
9713e41
Compare
|
@codex review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/executors/run-sync.ts (1)
214-219:⚠️ Potential issue | 🟠 Major
RUN_SYNC_CATCH_PROMISEpanic is incorrectly wrapped asRUN_SYNC_CATCH_HANDLER_THROW.The
assertNotPromiseLikecall at line 216 is inside the try block, so ifcatchFnreturns a Promise, the resultingPanic("RUN_SYNC_CATCH_PROMISE")is caught and rewrapped asRUN_SYNC_CATCH_HANDLER_THROW, losing the more specific error code.🛠️ Suggested fix
try { const mapped = catchFn(error) - assertNotPromiseLike(mapped, "RUN_SYNC_CATCH_PROMISE") - return mapped } catch (catchError) { + if (catchError instanceof Panic) { + throw catchError + } throw new Panic("RUN_SYNC_CATCH_HANDLER_THROW", { cause: catchError }) } + + assertNotPromiseLike(mapped, "RUN_SYNC_CATCH_PROMISE") + return mapped🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/run-sync.ts` around lines 214 - 219, The try-block currently encloses both calling catchFn and assertNotPromiseLike so a RUN_SYNC_CATCH_PROMISE Panic (thrown by assertNotPromiseLike) is caught and rewrapped as RUN_SYNC_CATCH_HANDLER_THROW; fix by limiting the try to only the call to catchFn: call const mapped = catchFn(error) inside try and catch only that throw (rethrow as Panic("RUN_SYNC_CATCH_HANDLER_THROW", { cause: catchError })), then call assertNotPromiseLike(mapped, "RUN_SYNC_CATCH_PROMISE") outside the try so a promise-like detection raises the correct Panic and is not rewrapped.
🧹 Nitpick comments (1)
src/lib/modifiers/timeout.ts (1)
71-75: Consider extracting the repeatedTimeoutErrorconstruction pattern.The same conditional pattern for creating
TimeoutErrorwith or without a cause appears in three places (#abort,checkDidTimeout, andrace). While the current approach works, extracting a small private helper could reduce duplication.♻️ Optional refactor
+ `#createTimeoutError`(cause?: unknown): TimeoutError { + return cause === undefined + ? new TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`) + : new TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`, { cause }) + } + `#abort`(cause?: unknown): TimeoutError { - const timeoutError = - cause === undefined - ? new TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`) - : new TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`, { cause }) + const timeoutError = this.#createTimeoutError(cause)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/modifiers/timeout.ts` around lines 71 - 75, Extract the repeated TimeoutError construction into a private helper (e.g., a method like createTimeoutError(cause?: unknown)) and replace the three duplicated conditional constructions in `#abort`, checkDidTimeout, and race to call this helper; the helper should return new TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`) when no cause is provided or new TimeoutError(..., { cause }) when a cause/reason is passed, keeping existing behavior and message formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/executors/run-sync.ts`:
- Around line 214-219: The try-block currently encloses both calling catchFn and
assertNotPromiseLike so a RUN_SYNC_CATCH_PROMISE Panic (thrown by
assertNotPromiseLike) is caught and rewrapped as RUN_SYNC_CATCH_HANDLER_THROW;
fix by limiting the try to only the call to catchFn: call const mapped =
catchFn(error) inside try and catch only that throw (rethrow as
Panic("RUN_SYNC_CATCH_HANDLER_THROW", { cause: catchError })), then call
assertNotPromiseLike(mapped, "RUN_SYNC_CATCH_PROMISE") outside the try so a
promise-like detection raises the correct Panic and is not rewrapped.
---
Nitpick comments:
In `@src/lib/modifiers/timeout.ts`:
- Around line 71-75: Extract the repeated TimeoutError construction into a
private helper (e.g., a method like createTimeoutError(cause?: unknown)) and
replace the three duplicated conditional constructions in `#abort`,
checkDidTimeout, and race to call this helper; the helper should return new
TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`) when no cause
is provided or new TimeoutError(..., { cause }) when a cause/reason is passed,
keeping existing behavior and message formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8148f013-9e37-468e-a1ec-486f918682fa
📒 Files selected for processing (26)
README.mdsrc/__tests__/index.test.tssrc/__tests__/types.test.tssrc/index.tssrc/lib/__tests__/utils.test.tssrc/lib/builder.tssrc/lib/errors.tssrc/lib/executors/__tests__/all.test.tssrc/lib/executors/__tests__/flow.test.tssrc/lib/executors/__tests__/retry.test.tssrc/lib/executors/__tests__/run-sync.test.tssrc/lib/executors/__tests__/run.test.tssrc/lib/executors/__tests__/shared.test.tssrc/lib/executors/all.tssrc/lib/executors/base.tssrc/lib/executors/flow.tssrc/lib/executors/gen.tssrc/lib/executors/run-sync.tssrc/lib/executors/run.tssrc/lib/executors/shared.tssrc/lib/modifiers/__tests__/retry.test.tssrc/lib/modifiers/retry.tssrc/lib/modifiers/signal.tssrc/lib/modifiers/timeout.tssrc/lib/types/builder.tssrc/lib/utils.ts
💤 Files with no reviewable changes (1)
- src/lib/types/builder.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/executors/tests/run.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/executors/tests/shared.test.ts
- src/lib/modifiers/signal.ts
- src/lib/executors/gen.ts
- src/tests/types.test.ts
- src/lib/executors/tests/all.test.ts
- src/lib/utils.ts
- src/lib/executors/shared.ts
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ 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". |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/executors/run-sync.ts (1)
214-219:⚠️ Potential issue | 🟠 MajorPreserve
RUN_SYNC_CATCH_PROMISEinstead of rewrapping it.The
assertNotPromiseLikecall at line 216 can throwPanic("RUN_SYNC_CATCH_PROMISE"), but it's inside the same try block that catches and rewraps all errors asRUN_SYNC_CATCH_HANDLER_THROW. This causes the more specific panic code to be lost.💡 Suggested fix
try { const mapped = catchFn(error) - assertNotPromiseLike(mapped, "RUN_SYNC_CATCH_PROMISE") - return mapped } catch (catchError) { + if (catchError instanceof Panic) { + throw catchError + } throw new Panic("RUN_SYNC_CATCH_HANDLER_THROW", { cause: catchError }) } + + assertNotPromiseLike(mapped, "RUN_SYNC_CATCH_PROMISE") + return mapped🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/run-sync.ts` around lines 214 - 219, The current try/catch wraps both the call to catchFn and the assertNotPromiseLike check so any Panic thrown by assertNotPromiseLike (RUN_SYNC_CATCH_PROMISE) gets rewrapped as RUN_SYNC_CATCH_HANDLER_THROW; fix it by narrowing the try to only invoke catchFn: call const mapped = catchFn(error) inside the try and let the catch block rewrap errors from catchFn only, then call assertNotPromiseLike(mapped, "RUN_SYNC_CATCH_PROMISE") outside that try (so its Panic escapes intact) and finally return mapped; reference functions/symbols: catchFn, assertNotPromiseLike, Panic, RUN_SYNC_CATCH_PROMISE, RUN_SYNC_CATCH_HANDLER_THROW.
🧹 Nitpick comments (1)
src/lib/modifiers/timeout.ts (1)
41-44: Consider extracting a local helper to reduce duplication.The same conditional TimeoutError construction pattern appears three times. A small private helper could improve maintainability.
♻️ Optional refactor to reduce duplication
+ `#createTimeoutError`(cause?: unknown): TimeoutError { + const message = `Execution exceeded timeout of ${this.#timeoutMs}ms` + return cause === undefined + ? new TimeoutError(message) + : new TimeoutError(message, { cause }) + } + `#abort`(cause?: unknown): TimeoutError { - const timeoutError = - cause === undefined - ? new TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`) - : new TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`, { cause }) + const timeoutError = this.#createTimeoutError(cause)Then use
this.#createTimeoutError(cause)incheckDidTimeoutandraceas well.Also applies to: 71-75, 107-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/modifiers/timeout.ts` around lines 41 - 44, Extract a private helper that centralizes TimeoutError construction to eliminate the repeated conditional pattern: create a method (e.g., `#createTimeoutError`(cause?: Error)) that returns new TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`) or new TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`, { cause }) depending on whether cause is provided, then replace the three duplicated constructions in checkDidTimeout, race, and the other location with calls to this.#createTimeoutError(cause).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/executors/run-sync.ts`:
- Around line 214-219: The current try/catch wraps both the call to catchFn and
the assertNotPromiseLike check so any Panic thrown by assertNotPromiseLike
(RUN_SYNC_CATCH_PROMISE) gets rewrapped as RUN_SYNC_CATCH_HANDLER_THROW; fix it
by narrowing the try to only invoke catchFn: call const mapped = catchFn(error)
inside the try and let the catch block rewrap errors from catchFn only, then
call assertNotPromiseLike(mapped, "RUN_SYNC_CATCH_PROMISE") outside that try (so
its Panic escapes intact) and finally return mapped; reference
functions/symbols: catchFn, assertNotPromiseLike, Panic, RUN_SYNC_CATCH_PROMISE,
RUN_SYNC_CATCH_HANDLER_THROW.
---
Nitpick comments:
In `@src/lib/modifiers/timeout.ts`:
- Around line 41-44: Extract a private helper that centralizes TimeoutError
construction to eliminate the repeated conditional pattern: create a method
(e.g., `#createTimeoutError`(cause?: Error)) that returns new
TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`) or new
TimeoutError(`Execution exceeded timeout of ${this.#timeoutMs}ms`, { cause })
depending on whether cause is provided, then replace the three duplicated
constructions in checkDidTimeout, race, and the other location with calls to
this.#createTimeoutError(cause).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5c0691d-0ac6-4ba6-9209-8b6a3774e7b1
📒 Files selected for processing (26)
README.mdsrc/__tests__/index.test.tssrc/__tests__/types.test.tssrc/index.tssrc/lib/__tests__/utils.test.tssrc/lib/builder.tssrc/lib/errors.tssrc/lib/executors/__tests__/all.test.tssrc/lib/executors/__tests__/flow.test.tssrc/lib/executors/__tests__/retry.test.tssrc/lib/executors/__tests__/run-sync.test.tssrc/lib/executors/__tests__/run.test.tssrc/lib/executors/__tests__/shared.test.tssrc/lib/executors/all.tssrc/lib/executors/base.tssrc/lib/executors/flow.tssrc/lib/executors/gen.tssrc/lib/executors/run-sync.tssrc/lib/executors/run.tssrc/lib/executors/shared.tssrc/lib/modifiers/__tests__/retry.test.tssrc/lib/modifiers/retry.tssrc/lib/modifiers/signal.tssrc/lib/modifiers/timeout.tssrc/lib/types/builder.tssrc/lib/utils.ts
💤 Files with no reviewable changes (1)
- src/lib/types/builder.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/lib/executors/tests/run.test.ts
- src/lib/tests/utils.test.ts
- README.md
- src/lib/executors/all.ts
- src/lib/modifiers/tests/retry.test.ts
- src/index.ts
- src/lib/executors/tests/shared.test.ts
- src/lib/executors/tests/flow.test.ts
- src/lib/executors/run.ts
adelrodriguez
force-pushed
the
03-06-replace_configurationerror_with_coded_panic_for_better_diagnostics
branch
from
9713e41 to
86502b7
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: 86502b7dde
ℹ️ 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".
| assertNotPromiseLike(mapped, "RUN_SYNC_CATCH_PROMISE") | ||
| return mapped | ||
| } catch (catchError) { | ||
| throw new Panic({ cause: catchError }) | ||
| throw new Panic("RUN_SYNC_CATCH_HANDLER_THROW", { cause: catchError }) |
There was a problem hiding this comment.
When runSync({ try, catch }) handles an error and the provided catch returns a Promise (via unsafe cast), assertNotPromiseLike throws Panic("RUN_SYNC_CATCH_PROMISE"), but that panic is immediately caught by the surrounding try/catch and rethrown as RUN_SYNC_CATCH_HANDLER_THROW. This misclassifies a non-throwing catch handler as a throw, so consumers relying on Panic.code get incorrect diagnostics for this input case.
Useful? React with 👍 / 👎.
adelrodriguez
force-pushed
the
03-06-replace_configurationerror_with_coded_panic_for_better_diagnostics
branch
2 times, most recently
from
5f0170c to
ae1c392
Compare
adelrodriguez
force-pushed
the
03-06-replace_configurationerror_with_coded_panic_for_better_diagnostics
branch
from
ae1c392 to
4e6307b
Compare
adelrodriguez
deleted the
03-06-replace_configurationerror_with_coded_panic_for_better_diagnostics
branch
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e6307b2b8
ℹ️ 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".
| if (raced instanceof CancellationError || raced instanceof TimeoutError) { | ||
| throw raced |
There was a problem hiding this comment.
In executeAll's async catch path, this branch treats any CancellationError/TimeoutError result as a control failure, but raced can also be the user-provided mapped value from options.catch. That means a handler like catch: async () => new TimeoutError("fallback") now rejects instead of resolving with the mapped value, which is a behavioral regression introduced by this change. To preserve valid catch mappings, control precedence should be based on controller state (e.g., post-race control checks), not on the mapped value's runtime class.
Useful? React with 👍 / 👎.
1 participant
This pull request replaces the generic
ConfigurationErrorwith a more structuredPanicerror system that includes machine-readable error codes.Key Changes
Error System Overhaul:
ConfigurationErrorexport and replaced withPanicclassPanicCodetype with 20+ specific error codes (e.g.,FLOW_NO_EXIT,TASK_SELF_REFERENCE,RUN_SYNC_TRY_PROMISE)Panicnow exposes acodeproperty for programmatic error handlingPanicMessagesAPI Changes:
retryOptionsexport tocreateRetryPolicyPanicinstead of returning error valuesErrorOptionspatternEnhanced Error Handling:
invariantutility function for assertion-style error checkingassertUnreachableto accept a panic code parameterTest Updates:
expectPanichelper function for consistent panic testingThe new system provides better debugging capabilities through structured error codes while maintaining backward compatibility for runtime error handling.