Merged
Conversation
Owner
Author
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR introduces the foundational architecture for hardtry, a TypeScript-based retry/timeout/abort-control flow library. It includes design and implementation documentation, core type definitions, an error hierarchy, an immutable fluent builder API (TryBuilder) for configuring operations, and synchronous execution logic with structured error handling and context management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
src/__tests__/index.test.ts (1)
1-42: LGTM — integration tests correctly cover the sync contract.Side note:
TryBuilder.runinsrc/lib/builder.ts(lines 56–62) has a redundantif (typeof input === "function")— both branches callexecuteRun(this.#config, input)identically. The branch adds no runtime behavior and can be removed.♻️ Suggested simplification in builder.ts
- run<T, E>(input: RunTryFn<T> | RunWithCatchOptions<T, E>) { - if (typeof input === "function") { - return executeRun(this.#config, input) - } - - return executeRun(this.#config, input) - } + run<T, E>(input: RunTryFn<T> | RunWithCatchOptions<T, E>) { + return executeRun(this.#config, input) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/index.test.ts` around lines 1 - 42, Remove the redundant branch in TryBuilder.run: the if (typeof input === "function") check is unnecessary because both branches call executeRun(this.#config, input) identically; simplify the method by calling executeRun(this.#config, input) unconditionally and removing the conditional and its duplicate call, keeping the method signature and preserving use of this.#config and the executeRun helper.src/lib/__tests__/runner.test.ts (1)
20-32: Optional: add a happy-path test for object form wheretrysucceeds.The current object-form tests only cover the
try-throws path. The success path (no throw fromtry) is not exercised — it falls through toreturn input.try(ctx)inexecuteRunwithout any assertion.✅ Suggested additional test
+ it("returns success value in object form", () => { + const result = executeRun( + {}, + { + try: () => "ok" as const, + catch: () => "mapped", + } + ) + + expect(result).toBe("ok") + }) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/runner.test.ts` around lines 20 - 32, Add a happy-path test for the object form to assert executeRun returns the value from the provided try function when it does not throw; specifically, in src/lib/__tests__/runner.test.ts add a test that calls executeRun with an object having try: () => "ok" and catch: () => "mapped" (or a spy) and assert the result is "ok" (and optionally that catch was not invoked), targeting the executeRun function behavior.src/lib/retry.ts (1)
19-21:retryOptionshas no dedicated tests andnormalizeRetryPolicyis untested for its object branch.The runner tests exercise context building with
{ limit: 3 }, but there are no tests directly coveringnormalizeRetryPolicywith a full policy object (non-number path), including default application forbackoff/delayMs.✅ Suggested tests for retry.ts
import { describe, expect, it } from "bun:test" import { normalizeRetryPolicy, retryOptions } from "../retry" describe("normalizeRetryPolicy", () => { it("converts a number to a RetryPolicy with linear defaults", () => { expect(normalizeRetryPolicy(3)).toEqual({ backoff: "linear", delayMs: 0, limit: 3, }) }) it("applies defaults for missing backoff and delayMs in object form", () => { expect(normalizeRetryPolicy({ limit: 5 })).toEqual({ backoff: "linear", delayMs: 0, limit: 5, }) }) it("preserves explicit backoff and delayMs in object form", () => { expect(normalizeRetryPolicy({ limit: 3, backoff: "exponential", delayMs: 500 })).toEqual({ backoff: "exponential", delayMs: 500, limit: 3, }) }) }) describe("retryOptions", () => { it("delegates to normalizeRetryPolicy", () => { expect(retryOptions({ limit: 2 })).toEqual(normalizeRetryPolicy({ limit: 2 })) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/retry.ts` around lines 19 - 21, Add unit tests covering normalizeRetryPolicy's object branch and retryOptions delegation: create tests that assert numeric input (e.g., 3) returns { backoff: "linear", delayMs: 0, limit: 3 }, that an object with only limit (e.g., { limit: 5 }) applies defaults for backoff and delayMs, and that an object with explicit backoff and delayMs (e.g., { limit: 3, backoff: "exponential", delayMs: 500 }) is preserved; also add a test that retryOptions({ limit: 2 }) simply returns the same value as normalizeRetryPolicy({ limit: 2 }). Ensure tests import normalizeRetryPolicy and retryOptions and use assertions for equality to cover the previously untested branches.src/lib/builder.ts (2)
57-63: Redundant runtime branch — both arms call the same code.The
if/elseexists solely to narrow types for overload resolution. Since both branches are identical at runtime, consider collapsing them with a type assertion, or adding a brief comment so future readers don't wonder about the duplication.Option: collapse with a comment
run<T, E>(input: RunTryFn<T> | RunWithCatchOptions<T, E>) { - if (typeof input === "function") { - return executeRun(this.#config, input) - } - - return executeRun(this.#config, input) + // Branch narrows `input` so TS selects the correct executeRun overload. + if (typeof input === "function") { + return executeRun(this.#config, input) + } + return executeRun(this.#config, input) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/builder.ts` around lines 57 - 63, The run method in class Builder redundantly branches even though both branches call executeRun(this.#config, input); simplify by removing the if/else and calling executeRun once, using a type assertion to satisfy overloads (e.g., assert input as RunTryFn<T> | RunWithCatchOptions<T,E>) or add an inline comment explaining the runtime duplication is deliberate for type narrowing; update the run<T,E>(input: ...) implementation around the run method to either collapse to a single return executeRun(this.#config, input as any) with a clarifying comment or keep the current signature but eliminate the duplicate branch while preserving type correctness for executeRun and `#config` references.
14-20:normalizeTimeoutOptionsmirrorsnormalizeRetryPolicy— consider co-locating or keeping the pattern consistent.
normalizeRetryPolicylives inretry.tsand is exported, whilenormalizeTimeoutOptionsis a file-private function inbuilder.ts. Whentimeout.tsis implemented (step 5 in the implementation order), this helper will likely need to move there. Not blocking, just a heads-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/builder.ts` around lines 14 - 20, normalizeTimeoutOptions is currently a private helper in builder.ts but mirrors normalizeRetryPolicy's exported pattern; move and export normalizeTimeoutOptions to the new timeout module (e.g., timeout.ts) so it follows the same public helper pattern as normalizeRetryPolicy, then update builder.ts to import { normalizeTimeoutOptions } from timeout.ts (or export it from a central index) to keep helper placement and visibility consistent when the timeout feature is implemented..context/design.md (2)
302-303: Minor naming drift between design doc and implementation.The design doc references
RunOptions<T, E>here, but the actual type insrc/lib/types.tsisRunWithCatchOptions<T, E>(with a separateRunTryFn<T>for the function form). Worth aligning the doc to avoid confusion as the team grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.context/design.md around lines 302 - 303, The doc refers to RunOptions<T, E> but the code defines RunWithCatchOptions<T, E> (and a separate RunTryFn<T> for the try-function form); update the design document to use the actual type names (replace RunOptions<T, E> with RunWithCatchOptions<T, E> and mention RunTryFn<T> where the function form is described) so the doc matches the implementation and avoids naming drift.
65-93: Add a language identifier to the fenced code block.The directory tree block at line 65 is missing a language specifier, which triggers the MD040 lint warning. Use
```textor```plaintextto silence it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.context/design.md around lines 65 - 93, The fenced code block that shows the project directory tree (the triple-backtick block containing the "src/" tree) lacks a language identifier and triggers MD040; update the opening fence from ``` to a specific specifier such as ```text or ```plaintext so the block is treated as plain text and the lint warning is silenced.src/lib/errors.ts (1)
14-24: Consider exportingExecutionErrorfor catch-allinstanceofchecks.
ExecutionErroris not exported, so consumers cannot write a singleinstanceof ExecutionErrorguard to match any framework error. If the intent is to keep it internal and have users rely on thecodediscriminant instead, that's a valid choice — just worth making explicit in the public docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/errors.ts` around lines 14 - 24, ExecutionError is currently declared as an internal abstract class and not exported, preventing consumers from using instanceof ExecutionError to catch any framework error; change its declaration to export abstract class ExecutionError so it becomes part of the public API, update any local imports/usages that relied on the non-exported symbol, and add it to your library's public exports/barrel (or relevant export list) so consumers can import it for catch-all instanceof checks (if you prefer to keep it internal instead, explicitly document that consumers should use the error code discriminant instead).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.context/api-proposal.md:
- Around line 10-13: The example passed to .run has a syntax error: the object
literal with properties try and catch is missing a comma; update the .run
invocation (the object containing try: () => myFn() and catch: (e) => new
MyErr()) to include a comma after the try property so the code becomes a valid
object literal and the .run({ try: () => myFn(), catch: (e) => new MyErr() })
form is correct.
- Around line 104-116: The examples for try$.all / try$.allSettled have trailing
commas after return expressions (e.g., in the async functions a, b, c and the
b(conn) functions) which are syntax errors; edit the example function bodies
(the async a(), async b(), async c(), and their counterparts) to remove the
trailing commas after the return expressions so each return line ends with the
expression only (e.g., return getA() not return getA(),) and ensure dependent
uses like getC(await this.$result.a) remain unchanged.
In `@src/index.ts`:
- Around line 25-26: Replace the direct constant assignments for gen and dispose
with re-exports to avoid the need for explicit type annotations: instead of
"export const gen = executeGen" and "export const dispose = createDisposer",
re-export the existing symbols so that executeGen is exported as gen and
createDisposer is exported as dispose; this removes the declaration-emit typing
requirement while preserving the public API.
In `@src/lib/retry.ts`:
- Around line 12-16: The return object currently spreads ...policy after setting
backoff and delayMs which allows undefined values in policy to clobber the
defaults; change the order so ...policy is spread first and then set the
defaults using policy.backoff ?? "linear" and policy.delayMs ?? 0 (i.e., put the
spread before the explicit backoff and delayMs assignments) so the defaults
always apply even when policy.backoff or policy.delayMs are undefined; target
the function that returns this object and the local variable policy and the
properties backoff/delayMs.
---
Nitpick comments:
In @.context/design.md:
- Around line 302-303: The doc refers to RunOptions<T, E> but the code defines
RunWithCatchOptions<T, E> (and a separate RunTryFn<T> for the try-function
form); update the design document to use the actual type names (replace
RunOptions<T, E> with RunWithCatchOptions<T, E> and mention RunTryFn<T> where
the function form is described) so the doc matches the implementation and avoids
naming drift.
- Around line 65-93: The fenced code block that shows the project directory tree
(the triple-backtick block containing the "src/" tree) lacks a language
identifier and triggers MD040; update the opening fence from ``` to a specific
specifier such as ```text or ```plaintext so the block is treated as plain text
and the lint warning is silenced.
In `@src/__tests__/index.test.ts`:
- Around line 1-42: Remove the redundant branch in TryBuilder.run: the if
(typeof input === "function") check is unnecessary because both branches call
executeRun(this.#config, input) identically; simplify the method by calling
executeRun(this.#config, input) unconditionally and removing the conditional and
its duplicate call, keeping the method signature and preserving use of
this.#config and the executeRun helper.
In `@src/lib/__tests__/runner.test.ts`:
- Around line 20-32: Add a happy-path test for the object form to assert
executeRun returns the value from the provided try function when it does not
throw; specifically, in src/lib/__tests__/runner.test.ts add a test that calls
executeRun with an object having try: () => "ok" and catch: () => "mapped" (or a
spy) and assert the result is "ok" (and optionally that catch was not invoked),
targeting the executeRun function behavior.
In `@src/lib/builder.ts`:
- Around line 57-63: The run method in class Builder redundantly branches even
though both branches call executeRun(this.#config, input); simplify by removing
the if/else and calling executeRun once, using a type assertion to satisfy
overloads (e.g., assert input as RunTryFn<T> | RunWithCatchOptions<T,E>) or add
an inline comment explaining the runtime duplication is deliberate for type
narrowing; update the run<T,E>(input: ...) implementation around the run method
to either collapse to a single return executeRun(this.#config, input as any)
with a clarifying comment or keep the current signature but eliminate the
duplicate branch while preserving type correctness for executeRun and `#config`
references.
- Around line 14-20: normalizeTimeoutOptions is currently a private helper in
builder.ts but mirrors normalizeRetryPolicy's exported pattern; move and export
normalizeTimeoutOptions to the new timeout module (e.g., timeout.ts) so it
follows the same public helper pattern as normalizeRetryPolicy, then update
builder.ts to import { normalizeTimeoutOptions } from timeout.ts (or export it
from a central index) to keep helper placement and visibility consistent when
the timeout feature is implemented.
In `@src/lib/errors.ts`:
- Around line 14-24: ExecutionError is currently declared as an internal
abstract class and not exported, preventing consumers from using instanceof
ExecutionError to catch any framework error; change its declaration to export
abstract class ExecutionError so it becomes part of the public API, update any
local imports/usages that relied on the non-exported symbol, and add it to your
library's public exports/barrel (or relevant export list) so consumers can
import it for catch-all instanceof checks (if you prefer to keep it internal
instead, explicitly document that consumers should use the error code
discriminant instead).
In `@src/lib/retry.ts`:
- Around line 19-21: Add unit tests covering normalizeRetryPolicy's object
branch and retryOptions delegation: create tests that assert numeric input
(e.g., 3) returns { backoff: "linear", delayMs: 0, limit: 3 }, that an object
with only limit (e.g., { limit: 5 }) applies defaults for backoff and delayMs,
and that an object with explicit backoff and delayMs (e.g., { limit: 3, backoff:
"exponential", delayMs: 500 }) is preserved; also add a test that retryOptions({
limit: 2 }) simply returns the same value as normalizeRetryPolicy({ limit: 2 }).
Ensure tests import normalizeRetryPolicy and retryOptions and use assertions for
equality to cover the previously untested branches.
Comment on lines
+12
to
+16
| return { | ||
| backoff: policy.backoff ?? "linear", | ||
| delayMs: policy.delayMs ?? 0, | ||
| ...policy, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for tsconfig files and search for exactOptionalPropertyTypes
find . -name "tsconfig*.json" -type f | head -20Repository: adelrodriguez/hardtry
Length of output: 80
🏁 Script executed:
#!/bin/bash
# Read the retry.ts file to see the current code
wc -l src/lib/retry.tsRepository: adelrodriguez/hardtry
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Read the retry.ts file around lines 12-16
sed -n '1,25p' src/lib/retry.tsRepository: adelrodriguez/hardtry
Length of output: 527
🏁 Script executed:
#!/bin/bash
# Check tsconfig.json for exactOptionalPropertyTypes
cat tsconfig.jsonRepository: adelrodriguez/hardtry
Length of output: 420
🏁 Script executed:
#!/bin/bash
# Find and read the types file to understand RetryPolicy
find . -name "types.ts" -o -name "types.d.ts" | head -5Repository: adelrodriguez/hardtry
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Read the types.ts file to see RetryPolicy definition
cat src/lib/types.tsRepository: adelrodriguez/hardtry
Length of output: 1178
Move spread operator before defaults to prevent undefined clobber.
When spreading two objects with the same properties, the last assignment wins—even if it's undefined. The current code places ...policy after the ?? defaults:
return {
backoff: policy.backoff ?? "linear",
delayMs: policy.delayMs ?? 0,
...policy,
}Since exactOptionalPropertyTypes is not enabled in tsconfig, callers can pass { limit: 3, backoff: undefined } (valid RetryPolicy), which would clobber the defaults and return backoff: undefined instead of "linear".
Move the spread before the defaults so defaults always apply:
🐛 Proposed fix
return {
- backoff: policy.backoff ?? "linear",
- delayMs: policy.delayMs ?? 0,
...policy,
+ backoff: policy.backoff ?? "linear",
+ delayMs: policy.delayMs ?? 0,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/retry.ts` around lines 12 - 16, The return object currently spreads
...policy after setting backoff and delayMs which allows undefined values in
policy to clobber the defaults; change the order so ...policy is spread first
and then set the defaults using policy.backoff ?? "linear" and policy.delayMs ??
0 (i.e., put the spread before the explicit backoff and delayMs assignments) so
the defaults always apply even when policy.backoff or policy.delayMs are
undefined; target the function that returns this object and the local variable
policy and the properties backoff/delayMs.
adelrodriguez
force-pushed
the
02-22-feat_add_execution_runner
branch
2 times, most recently
from
cdedcdd to
fe74bbf
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
adelrodriguez
force-pushed
the
02-22-feat_add_execution_runner
branch
from
fe74bbf to
d03bd39
Compare
adelrodriguez
force-pushed
the
02-22-feat_add_execution_runner
branch
from
d03bd39 to
bb72fd6
Compare
This was referenced Feb 22, 2026
This was referenced Feb 23, 2026
This was referenced Feb 27, 2026
Merged
This was referenced Mar 6, 2026
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 by CodeRabbit
Release Notes
run()function for executing code with configurable retry and timeout policiesretry(),timeout(),signal(), andwrap()methods for fine-grained control flowCancellationError,TimeoutError,RetryExhaustedError,UnhandledException, andPanicGreptile Summary
Implements Phase 0 and Phase 1 of the
hardtryexecution runner library, establishing foundation types, error classes, and synchronousrun()execution. The PR adds a fluent builder API (retry,timeout,signal,wrap) and core execution logic with proper error handling forUnhandledExceptionandPaniccases.Key Changes:
TryCtx,RunOptions, andBuilderConfigTryBuilderwith chainable configuration methodsexecuteRunfunction supporting both function and object formsgen,dispose,all,allSettled,flow)Issues Found:
src/lib/retry.ts:12-16where spread operator placement allows undefined values to override defaultsConfidence Score: 4/5
src/lib/retry.tsbefore merging to prevent potential undefined override issuesImportant Files Changed
Last reviewed commit: bb72fd6