Support async rules in eager enforce(value) chains with awaitable chains and type updates#1261
Open
Support async rules in eager enforce(value) chains with awaitable chains and type updates#1261
enforce(value) chains with awaitable chains and type updates#1261Conversation
Review Summary by QodoSupport async rules in eager enforce chains with promise queuing and type updates
WalkthroughsDescription• Add async rule support to eager enforce(value) chains with Promise handling • Implement internal pendingPromise tracking to queue rules after async execution • Extend type system with AsyncMode generics to reflect async transitions • Preserve fail-fast sync behavior while enabling awaitable chains • Add comprehensive tests for async custom rules and type safety Diagramflowchart LR
A["enforce(value)"] --> B["Sync Rule Execution"]
B --> C{Rule Returns Promise?}
C -->|No| D["Continue Sync Chain"]
C -->|Yes| E["Create pendingPromise"]
E --> F["Queue Subsequent Rules"]
F --> G["Return Awaitable Chain"]
D --> H["Return Proxy"]
G --> I["Implement then/catch"]
File Changes1. packages/n4s/src/eager.ts
|
📝 WalkthroughWalkthroughThis pull request adds asynchronous enforcement support to the N4S eager enforce API. A new AsyncMode generic parameter is introduced across type definitions to track async state, and runtime implementation adds pendingPromise chaining to defer and queue rule execution when promises are encountered. Tests and documentation demonstrate async matcher registration via enforce.extend and type augmentation. Changes
Sequence DiagramsequenceDiagram
participant User
participant Enforce as Enforce<br/>(eager)
participant RuleCall as Rule Execution<br/>Pipeline
participant Promise as Pending Promise<br/>Queue
participant CustomRule as Custom Rule<br/>(Async)
User->>Enforce: enforce(value).rule1().rule2Async()
Enforce->>RuleCall: Execute rule1 (sync)
RuleCall->>RuleCall: processRuleResult
RuleCall->>Enforce: pass, return proxy
Enforce->>RuleCall: Execute rule2Async
RuleCall->>CustomRule: Invoke async rule
CustomRule-->>RuleCall: Return Promise
RuleCall->>Promise: setPendingPromise
Promise->>Promise: Queue rule execution
RuleCall->>Enforce: Return proxy (awaitable)
User->>Enforce: await result
Promise->>CustomRule: Execute queued rule
CustomRule-->>Promise: Resolve/Reject
Promise-->>User: Result void or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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)
📝 Coding Plan
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 |
Code Review by Qodo
1. Context lost during messages
|
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces asynchronous rule support to the eager enforce(value) API, a significant enhancement. The changes are well-implemented across the board, including runtime logic for promise-based rule execution, corresponding TypeScript type propagation to make chains awaitable, and comprehensive tests for the new async behaviors. My review found the code to be of high quality. I have one suggestion in ruleCallGenerator.ts to simplify a promise chain for better readability and maintainability.
Comment on lines
+55
to
+59
| pendingPromise.then(() => { | ||
| return Promise.resolve(runRule()).then(ruleResult => { | ||
| processRuleResult(ruleResult, ruleName, value, customMessage, args); | ||
| }); | ||
| }), |
Contributor
There was a problem hiding this comment.
The nested .then() and Promise.resolve() call are unnecessary here. The .then() method of a promise automatically handles promise unwrapping (or 'thenable assimilation'), so you can chain the calls directly. This simplifies the code and reduces nesting, improving readability.
pendingPromise.then(() => runRule()).then(ruleResult => {
processRuleResult(ruleResult, ruleName, value, customMessage, args);
}),![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+19
to
+50
| function processRuleResult( | ||
| result: any, | ||
| ruleName: string, | ||
| value: any, | ||
| customMessage: string | undefined, | ||
| args: any[], | ||
| ): void { | ||
| const transformedResult = transformResult(result, ruleName, value, ...args); | ||
|
|
||
| invariant( | ||
| transformedResult.pass, | ||
| enforceMessage(ruleName, transformedResult, value, customMessage), | ||
| ); | ||
| } | ||
|
|
||
| export function createRuleCall(config: RuleCallConfig) { | ||
| const { target, rule, ruleName, value, customMessage, clearMessage } = config; | ||
| const { | ||
| target, | ||
| rule, | ||
| ruleName, | ||
| value, | ||
| customMessage, | ||
| clearMessage, | ||
| getPendingPromise, | ||
| setPendingPromise, | ||
| } = config; | ||
|
|
||
| return function ruleCall(...args: any[]): any { | ||
| const transformedResult = ctx.run({ value }, () => | ||
| transformResult( | ||
| const runRule = () => | ||
| ctx.run({ value }, () => | ||
| (rule as (...args: any[]) => any)(value, ...args), | ||
| ruleName, | ||
| value, | ||
| ...args, | ||
| ), | ||
| ); | ||
| ); |
There was a problem hiding this comment.
1. Context lost during messages 🐞 Bug ✓ Correctness
transformResult() now runs outside ctx.run(), so rule-returned message functions that rely on enforce.context() will see no context and can produce incorrect/missing messages. This affects both sync rules and queued async rules because the context implementation resets immediately after the ctx.run callback returns.
Agent Prompt
### Issue description
`transformResult()` is executed outside `ctx.run()`, so message functions (`result.message`) evaluated via `dynamicValue` cannot read `enforce.context()` (the context is reset immediately after `ctx.run` returns).
### Issue Context
`packages/context/src/context.ts` shows `ctx.run` restores the parent context right after the callback returns, so any result/message processing must happen inside the callback if it needs context.
### Fix Focus Areas
- packages/n4s/src/eager/ruleCallGenerator.ts[19-95]
Suggested approach:
- Move `processRuleResult(...)` invocation inside `ctx.run({ value }, ...)`.
- For async rules, when the promise resolves, wrap the `processRuleResult(resolvedResult, ...)` call in a fresh `ctx.run({ value }, ...)` so message functions can still access context at evaluation time.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+44
to
53
| type IsPromiseLike<T> = T extends PromiseLike<any> ? true : false; | ||
| type NextAsyncMode< | ||
| CurrentAsync extends boolean, | ||
| RuleReturn, | ||
| > = CurrentAsync extends true ? true : IsPromiseLike<RuleReturn>; | ||
|
|
||
| export type EnforceCustomMatcher<F extends CB> = ( | ||
| ...args: CustomMatcherArgs<F extends keyof n4s.EnforceMatchers ? F : never> | ||
| ) => boolean | RuleRunReturn<any>; | ||
| ) => boolean | RuleRunReturn<any> | Promise<boolean | RuleRunReturn<any>>; | ||
|
|
There was a problem hiding this comment.
2. Async rules break lazy api 🐞 Bug ✓ Correctness
Promise-returning custom rules registered via enforce.extend(...) are also registered into the lazy RuleInstance API, but lazy execution wraps results with RuleRunReturn.create, which treats a Promise as an invalid object and returns pass: false. This makes enforce.myAsyncRule().run(value) silently fail even when the async rule would pass in eager mode.
Agent Prompt
### Issue description
Async (Promise-returning) custom rules are now allowed/encouraged for eager enforcement, but `enforce.extend` also wires those rules into the lazy `RuleInstance` API where Promises are not supported, leading to silent incorrect failures.
### Issue Context
`extendEnforce` registers rules into both eager and lazy APIs. Lazy `RuleRunReturn.create` cannot interpret a Promise result.
### Fix Focus Areas
- packages/n4s/src/extendLogic.ts[48-64]
- packages/n4s/src/utils/RuleRunReturn.ts[44-65]
- packages/n4s/src/n4sTypes.ts[50-53]
Suggested approach options (pick one):
1) **Guard/throw in lazy wrapper**: In `ruleWrapper`, if `res` is promise-like, throw an explicit error (e.g., "Async custom rules are not supported in lazy RuleInstance API; use eager enforce(value) and await the chain").
2) **Do not register lazy rules** for promise-returning matchers (hard to detect reliably without calling the function; you can at least block reserved `async` declarations in user typing or provide a separate `extendAsync` that only extends eager).
3) Implement an **AsyncRuleInstance** (larger change) so lazy `.run/.test` can return promises.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/n4s/src/eager.ts (1)
127-128: Settingproxy.pass = trueunconditionally may be misleading.Setting
pass = trueat initialization implies the validation has passed before any rules have run. While this may be intentional (empty chains pass), it could cause confusion if users check.passbefore calling any validation rules. Consider adding a comment explaining this design decision.📝 Add clarifying comment
+ // An empty chain (no rules called) is considered passing proxy.pass = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/n4s/src/eager.ts` around lines 127 - 128, The assignment proxy.pass = true sets the validation as "passed" at initialization which can be misleading; update the code around the proxy.pass assignment to include a concise comment explaining the design decision (e.g., that empty/absent rule chains are considered passing and therefore .pass defaults true) and warn consumers not to rely on .pass before any rules execute; reference the proxy.pass initialization in the eager.ts file (the proxy.pass assignment) so future readers understand why the default is true.packages/n4s/src/eager/ruleCallGenerator.ts (1)
52-65: Consider clarifying the error propagation behavior in async chains.When
processRuleResultthrows inside the promise chain (line 57), the error propagates as a rejection. However,target.pass = trueis set unconditionally on line 63 before the promise settles. This is correct for the async model (callers must await to see failures), but could be confusing.A brief comment would help clarify that
pass = trueis a placeholder until the promise resolves/rejects.📝 Add clarifying comment
+ // pass is set optimistically; actual result is determined when the promise settles clearMessage(); target.pass = true; return target;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/n4s/src/eager/ruleCallGenerator.ts` around lines 52 - 65, Add a short clarifying comment near the async branch that handles pending promises (around getPendingPromise / setPendingPromise) explaining that target.pass is set to true immediately as a placeholder for the asynchronous result and that any error thrown inside the chained promise (from runRule or processRuleResult) will propagate as a rejection which callers must await to observe; mention the specific symbols getPendingPromise, setPendingPromise, runRule, processRuleResult and target.pass so reviewers can find the spot.packages/n4s/src/__tests__/extendAsync.test.ts (1)
6-17: Consider movingenforce.extendtobeforeAllfor test isolation.Other async test files in this PR (e.g.,
asyncEnforce.test.ts) usebeforeAllfor extending enforce. Moving the extension outside the test case ensures consistent setup patterns across the test suite and avoids potential issues if more tests are added to this file later.♻️ Suggested refactor
describe('enforce.extend with async rules', () => { + beforeAll(() => { + enforce.extend({ + myCustomAsyncRule: async (val: number) => { + return { pass: val > 5 }; + }, + }); + }); + it('should register and execute an async custom rule properly', async () => { - enforce.extend({ - myCustomAsyncRule: async (val: number) => { - return { pass: val > 5 }; - }, - }); - // `@ts-expect-error` - testing dynamic rule await expect(enforce(10).myCustomAsyncRule()).resolves.toBeUndefined(); // `@ts-expect-error` - testing dynamic rule await expect(enforce(2).myCustomAsyncRule()).rejects.toThrow(); }); });You'll also need to add
beforeAllto the import on line 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/n4s/src/__tests__/extendAsync.test.ts` around lines 6 - 17, Move the enforce.extend call out of the test body into a beforeAll block so the async rule registration runs once per suite; add beforeAll to the test imports (as suggested) and place the existing enforce.extend({ myCustomAsyncRule: async (val: number) => ({ pass: val > 5 }) }) inside that beforeAll, then remove the extend call from the 'should register and execute an async custom rule properly' test so the test simply invokes enforce(10).myCustomAsyncRule() and enforce(2).myCustomAsyncRule() as before.packages/n4s/src/eager/eagerTypes.ts (1)
14-16: Consider addingfinallyfor full Promise ergonomics.The
ThenableEnforcetype omitsfinally, which is commonly used for cleanup in async flows (e.g.,await enforce(x).asyncRule().finally(() => cleanup())). While not required for basicawaitsupport, adding it would provide a more complete Promise-like experience.♻️ Suggested enhancement
type ThenableEnforce = PromiseLike<void> & { catch: (onrejected?: ((reason: unknown) => unknown) | null) => Promise<void>; + finally: (onfinally?: (() => void) | null) => Promise<void>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/n4s/src/eager/eagerTypes.ts` around lines 14 - 16, The ThenableEnforce type currently models PromiseLike<void> with a custom catch but omits finally; update the ThenableEnforce declaration (the type named ThenableEnforce) to include a finally method signature (e.g., finally(onfinally?: (() => void) | null): Promise<void>) so it exposes Promise-style cleanup chaining alongside the existing catch, preserving PromiseLike<void> behavior and return type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/n4s/src/eager.ts`:
- Around line 84-95: The switch in getReservedProperty calls
ensurePendingPromise() twice for THEN_KEY and CATCH_KEY which can create
confusing/fragile promise references and eagerly create a pending promise;
change each branch to call ensurePendingPromise() exactly once, store it in a
local const (e.g., const p = ensurePendingPromise()) and then return
p.then.bind(p) for THEN_KEY and p.catch.bind(p) for CATCH_KEY so the same
promise reference is used and the call site is clearer (refer to
getReservedProperty, THEN_KEY, CATCH_KEY, and ensurePendingPromise).
---
Nitpick comments:
In `@packages/n4s/src/__tests__/extendAsync.test.ts`:
- Around line 6-17: Move the enforce.extend call out of the test body into a
beforeAll block so the async rule registration runs once per suite; add
beforeAll to the test imports (as suggested) and place the existing
enforce.extend({ myCustomAsyncRule: async (val: number) => ({ pass: val > 5 })
}) inside that beforeAll, then remove the extend call from the 'should register
and execute an async custom rule properly' test so the test simply invokes
enforce(10).myCustomAsyncRule() and enforce(2).myCustomAsyncRule() as before.
In `@packages/n4s/src/eager.ts`:
- Around line 127-128: The assignment proxy.pass = true sets the validation as
"passed" at initialization which can be misleading; update the code around the
proxy.pass assignment to include a concise comment explaining the design
decision (e.g., that empty/absent rule chains are considered passing and
therefore .pass defaults true) and warn consumers not to rely on .pass before
any rules execute; reference the proxy.pass initialization in the eager.ts file
(the proxy.pass assignment) so future readers understand why the default is
true.
In `@packages/n4s/src/eager/eagerTypes.ts`:
- Around line 14-16: The ThenableEnforce type currently models PromiseLike<void>
with a custom catch but omits finally; update the ThenableEnforce declaration
(the type named ThenableEnforce) to include a finally method signature (e.g.,
finally(onfinally?: (() => void) | null): Promise<void>) so it exposes
Promise-style cleanup chaining alongside the existing catch, preserving
PromiseLike<void> behavior and return type.
In `@packages/n4s/src/eager/ruleCallGenerator.ts`:
- Around line 52-65: Add a short clarifying comment near the async branch that
handles pending promises (around getPendingPromise / setPendingPromise)
explaining that target.pass is set to true immediately as a placeholder for the
asynchronous result and that any error thrown inside the chained promise (from
runRule or processRuleResult) will propagate as a rejection which callers must
await to observe; mention the specific symbols getPendingPromise,
setPendingPromise, runRule, processRuleResult and target.pass so reviewers can
find the spot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbc5bdb4-f56d-45e1-972e-ab4d02c11256
📒 Files selected for processing (9)
packages/n4s/src/__tests__/asyncEnforce.types.test.tspackages/n4s/src/__tests__/extend.test.tspackages/n4s/src/__tests__/extendAsync.test.tspackages/n4s/src/eager.tspackages/n4s/src/eager/eagerTypes.tspackages/n4s/src/eager/ruleCallGenerator.tspackages/n4s/src/n4sTypes.tspackages/n4s/src/rules/__tests__/asyncEnforce.test.tsplans/async_enforcements_summary.md
Comment on lines
+84
to
+95
| const getReservedProperty = (key: string) => { | ||
| switch (key) { | ||
| case MESSAGE_KEY: | ||
| return setMessage; | ||
| case THEN_KEY: | ||
| return ensurePendingPromise().then.bind(ensurePendingPromise()); | ||
| case CATCH_KEY: | ||
| return ensurePendingPromise().catch.bind(ensurePendingPromise()); | ||
| default: | ||
| return undefined; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Potential bug: ensurePendingPromise() is called twice, potentially creating inconsistent promise references.
In the THEN_KEY and CATCH_KEY cases, ensurePendingPromise() is invoked twice per access — once to get .then/.catch and again inside .bind(). Since ensurePendingPromise may create a new promise on the first call if pendingPromise is null, the second call will return the same reference, but this pattern is confusing and could break if the implementation changes.
More importantly, the current pattern evaluates ensurePendingPromise() eagerly when .then or .catch is accessed, which creates a resolved promise even if no async rule has run. This might cause unexpected behavior where a purely synchronous chain becomes thenable.
🔧 Suggested fix to clarify intent
const getReservedProperty = (key: string) => {
+ const p = ensurePendingPromise();
switch (key) {
case MESSAGE_KEY:
return setMessage;
case THEN_KEY:
- return ensurePendingPromise().then.bind(ensurePendingPromise());
+ return p.then.bind(p);
case CATCH_KEY:
- return ensurePendingPromise().catch.bind(ensurePendingPromise());
+ return p.catch.bind(p);
default:
return undefined;
}
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/n4s/src/eager.ts` around lines 84 - 95, The switch in
getReservedProperty calls ensurePendingPromise() twice for THEN_KEY and
CATCH_KEY which can create confusing/fragile promise references and eagerly
create a pending promise; change each branch to call ensurePendingPromise()
exactly once, store it in a local const (e.g., const p = ensurePendingPromise())
and then return p.then.bind(p) for THEN_KEY and p.catch.bind(p) for CATCH_KEY so
the same promise reference is used and the call site is clearer (refer to
getReservedProperty, THEN_KEY, CATCH_KEY, and ensurePendingPromise).
🚀 Benchmark Results
Raw Output |
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.
Motivation
enforce(value)API while preserving existing fail-fast sync behavior.await enforce(x).rule()) so mixed sync/async validation flows are ergonomic.Description
pendingPromisetracking ineager.tsand exposed reservedthen/catchhandlers soenforce(value)becomes awaitable when an async rule is used.ruleCallGenerator.tsto detect Promise results (viaisPromise), queue subsequent rule execution behindpendingPromise, and unify sync/async result processing viaprocessRuleResult.AsyncModegenerics andThenableEnforceineager/eagerTypes.ts, and updatedn4sTypes.tsso custom matchers may returnPromiseand the eager return type reflects async transitions.extendAsync.test.ts,asyncEnforce.types.test.ts,rules/__tests__/asyncEnforce.test.ts, and adjustments toextend.test.ts; also addedplans/async_enforcements_summary.mddocumenting the approach.Testing
vitestsuite including the new tests underpackages/n4s/src/__tests__andpackages/n4s/src/rules/__tests__. All tests passed.asyncEnforce.types.test.ts), async custom rules execution and rejection (extendAsync.test.tsandrules/__tests__/asyncEnforce.test.ts), and updated behavior inextend.test.ts.Codex Task
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation