Conversation
When parsing fails in runWith() or runWithSync() and a source context's disposal also throws, the disposal error used to silently replace the original parse error due to JavaScript's try/finally semantics. The disposal error now wraps both failures in a SuppressedError (following TC39 conventions) instead of discarding the parse error: - .error = the disposal error (the newer error) - .suppressed = the original parse error The try/finally blocks were restructured into helper functions (runWithBody, runWithSyncBody) with explicit error handling in the callers, avoiding the no-unsafe-finally lint issue entirely. #246 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SuppressedError is only a global starting from Node 24, but the package supports Node >= 20. Add a local polyfill so the double-failure disposal path works correctly on all supported runtimes instead of throwing ReferenceError. #246 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #771 +/- ##
==========================================
+ Coverage 91.07% 91.14% +0.07%
==========================================
Files 43 43
Lines 25249 25317 +68
Branches 6473 6493 +20
==========================================
+ Hits 22995 23075 +80
+ Misses 2175 2165 -10
+ Partials 79 77 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 797077471d
ℹ️ 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.
Pull request overview
This PR fixes an error-reporting edge case in @optique/core’s runWith() / runWithSync() where a context disposal failure could overwrite (and effectively hide) the original parse failure. It restructures the control flow to dispose contexts outside of try/finally so both failures can be preserved via SuppressedError.
Changes:
- Refactors parsing logic into
runWithBody()/runWithSyncBody()and performs disposal in an explicit post-parse step. - Wraps parse+dispose double-failures using
SuppressedErrorsemantics (error= disposal error,suppressed= parse error), with a local constructor fallback. - Adds regression tests for suppressed disposal errors and updates
CHANGES.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/core/src/facade.ts | Refactors runWith* flow and introduces SuppressedError constructor fallback + explicit suppression behavior. |
| packages/core/src/facade.test.ts | Adds tests asserting SuppressedError behavior for parse failure + disposal failure combinations. |
| CHANGES.md | Documents the fix and links the issue/PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where parse errors were discarded if context disposal also failed. It introduces a SuppressedError polyfill for compatibility with older runtimes and refactors runWith and runWithSync to ensure all failures are preserved. The review feedback suggests simplifying the code by removing redundant type assertions when accessing contextOptions.
Replace `instanceof SuppressedError` with duck-typing on `.name`/`.error`/`.suppressed` so the assertions don't depend on a global `SuppressedError` constructor. #771 (comment) #771 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove redundant casts; RunWithOptions already declares contextOptions as an optional property. #771 (comment) #771 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors parsing/annotation flow into new helpers ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 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/core/src/facade.test.ts`:
- Line 6327: Replace uses of "error instanceof SuppressedError" in the tests
with a shape/name-based assertion so tests don't rely on the global
SuppressedError; for each occurrence (e.g. the assert.ok(error instanceof
SuppressedError) checks) assert that error is an object, error.name ===
'SuppressedError' (or error?.name === 'SuppressedError') and that the expected
suppressed property exists (e.g. Array.isArray(error.suppressed) or 'suppressed'
in error) — update all occurrences noted (the assert.ok(...) checks at the
listed locations) to use this guard pattern.
In `@packages/core/src/facade.ts`:
- Around line 3047-3057: The JSDoc for the public API was left on the private
helper runWithBody; move that docblock back onto the exported runWith function,
ensuring the exported runWith has full description, `@param`, `@returns` and the
`@since` tag (remove `@since` from runWithBody). Update the `@throws` block on runWith
to mention the new SuppressedError path (describe when SuppressedError is thrown
and include it alongside existing error cases). Repeat the same
relocation/update for the other occurrence referenced (lines around the
runWith/runWithBody pair at 3202-3214) so only exported symbols carry `@since` and
public docs.
- Around line 3229-3255: The current flow only treats thrown exceptions from
runWithBody/runWithSyncBody as "primary" errors, so parse failures returned via
runParser's configured onError are treated as success and can be overwritten by
disposal errors; update the body helpers (runWithBody and runWithSyncBody) to
return a discriminated outcome that distinguishes a successful result, a thrown
exception, and a "handled parse failure" (the onError return), then in this code
use that discriminator instead of hasPrimaryError: capture the
handled-parse-failure outcome (e.g., hasHandledParseFailure + handledValue)
alongside thrown primaryError, call disposeContexts, and if dispose throws
prefer wrapping the disposeError with SuppressedErrorCtor using the original
primary/handled outcome as the suppressed value so the configured parse-failure
result is not lost; adjust downstream logic that currently checks
hasPrimaryError to also consider the handled-parse-failure branch.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ce8ec5bf-4360-44bc-841b-2b919a255ff1
📒 Files selected for processing (3)
CHANGES.mdpackages/core/src/facade.test.tspackages/core/src/facade.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/facade.ts (1)
3359-3365: 🛠️ Refactor suggestion | 🟠 MajorUpdate
@throwsto includeSuppressedError.The function can now throw
SuppressedErrorwhen both parsing and disposal fail. The JSDoc@throwssection should document this new error type for completeness.📝 Suggested documentation update
* `@throws` {TypeError} If an async parser is passed at runtime. Use * {`@link` runWith} or {`@link` runWithAsync} for async parsers. * `@throws` {TypeError} If two or more contexts share the same * {`@link` SourceContext.id}. * `@throws` {Error} If any context returns a Promise or if a context's * `[Symbol.asyncDispose]` returns a Promise. + * `@throws` {SuppressedError} If parsing fails and context disposal also + * throws. The disposal error is in `.error` and the parse error in + * `.suppressed`.As per coding guidelines, functions that throw exceptions must include the
@throwstag documenting all thrown error types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/facade.ts` around lines 3359 - 3365, Update the JSDoc `@throws` block in the affected facade function to list SuppressedError in addition to the existing error types: add a line like "@throws {SuppressedError} If both parsing and disposal fail and an aggregate suppressed error is produced" so the documentation reflects that the function can throw SuppressedError when a parser error and a disposal ([Symbol.asyncDispose]) error occur (alongside the existing TypeError and Error notes referencing async parsers, SourceContext.id collisions, and Promise-returning contexts). Ensure the new `@throws` entry appears with the other `@throws` tags near the runWith/runWithAsync related JSDoc.
♻️ Duplicate comments (2)
packages/core/src/facade.ts (2)
3047-3057: 🛠️ Refactor suggestion | 🟠 MajorJSDoc is on the internal helper instead of the exported function.
The JSDoc block (including
@since 0.10.0,@example, and@throws) is currently documentingrunWithBody()rather than the exportedrunWith(). Per coding guidelines, exported APIs must have JSDoc comments, and based on learnings, only publicly exported API symbols should carry the@sincetag.Additionally, the
@throwsdocumentation should be updated to includeSuppressedErrorfor the new disposal error handling path.Also applies to: 3200-3214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/facade.ts` around lines 3047 - 3057, The JSDoc currently annotates the internal helper runWithBody instead of the exported runWith; move the full JSDoc block (including `@since` 0.10.0, `@example` and `@throws`) from runWithBody to the exported runWith function, remove the `@since` tag from any non-exported/internal symbols, and update the `@throws` list to mention both the original errors and SuppressedError to cover the new disposal error path; also apply the same relocation/update for the duplicate JSDoc instance that documents the internal helper referenced around the other occurrence (the later block covering the same behavior).
3227-3255:⚠️ Potential issue | 🟡 MinorParse failures handled via
onErrorcallback return are not preserved when disposal throws.When
runParser()reports a parse failure by returning the configuredonErrorvalue (rather than throwing),runWithBody()returns normally andhasPrimaryErrorstaysfalse. If disposal subsequently throws, only the disposal error propagates—the parse-failure return value is lost.This is primarily an edge case (common patterns like
onError: process.exitterminate before disposal), but for handlers that return values, the original outcome is overwritten.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/facade.ts` around lines 3227 - 3255, The code only treats thrown errors from runWithBody as the "primary" outcome; to preserve parse-failure returns you must mark and preserve the successful result as a primary outcome as well: after calling runWithBody set a hasPrimaryResult (or reuse hasPrimaryError semantics inverted) and store result; in the disposeContexts catch, if a primary result exists then do not let the disposal error overwrite it—return the preserved result (or wrap the disposal error as suppressed but keep returning the original result) instead of throwing; update the logic around runWithBody, result, hasPrimaryError, and disposeContexts to implement this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/facade.ts`:
- Around line 3359-3365: Update the JSDoc `@throws` block in the affected facade
function to list SuppressedError in addition to the existing error types: add a
line like "@throws {SuppressedError} If both parsing and disposal fail and an
aggregate suppressed error is produced" so the documentation reflects that the
function can throw SuppressedError when a parser error and a disposal
([Symbol.asyncDispose]) error occur (alongside the existing TypeError and Error
notes referencing async parsers, SourceContext.id collisions, and
Promise-returning contexts). Ensure the new `@throws` entry appears with the other
`@throws` tags near the runWith/runWithAsync related JSDoc.
---
Duplicate comments:
In `@packages/core/src/facade.ts`:
- Around line 3047-3057: The JSDoc currently annotates the internal helper
runWithBody instead of the exported runWith; move the full JSDoc block
(including `@since` 0.10.0, `@example` and `@throws`) from runWithBody to the exported
runWith function, remove the `@since` tag from any non-exported/internal symbols,
and update the `@throws` list to mention both the original errors and
SuppressedError to cover the new disposal error path; also apply the same
relocation/update for the duplicate JSDoc instance that documents the internal
helper referenced around the other occurrence (the later block covering the same
behavior).
- Around line 3227-3255: The code only treats thrown errors from runWithBody as
the "primary" outcome; to preserve parse-failure returns you must mark and
preserve the successful result as a primary outcome as well: after calling
runWithBody set a hasPrimaryResult (or reuse hasPrimaryError semantics inverted)
and store result; in the disposeContexts catch, if a primary result exists then
do not let the disposal error overwrite it—return the preserved result (or wrap
the disposal error as suppressed but keep returning the original result) instead
of throwing; update the logic around runWithBody, result, hasPrimaryError, and
disposeContexts to implement this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1520ae69-ecd7-4d9e-9e10-4eb907ad7482
📒 Files selected for processing (2)
packages/core/src/facade.test.tspackages/core/src/facade.ts
The extraction left the public API docblock on the private runWithBody helper while the exported runWith was undocumented. Move it back and add @throws {SuppressedError} to both runWith and runWithSync. #771 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
/gemini review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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.
Code Review
This pull request updates runWith and runWithSync to use SuppressedError when both parsing and context disposal fail, ensuring the original parse error is preserved. It introduces a SuppressedError polyfill for compatibility with Node.js versions older than 24 and refactors the core logic into runWithBody and runWithSyncBody helpers. Review feedback identifies a regression in these new helper functions where the removal of optional chaining on the options parameter could cause crashes if the options object is omitted by the user.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/src/facade.ts`:
- Around line 3182-3184: Add a matching `@throws` {SuppressedError} JSDoc entry to
runWithAsync() describing the rejection path where parsing fails and a context
disposal also throws (original parse error available via .suppressed and
disposal error via .error), mirroring the existing `@throws` text used for
runWith()/runWithSync(); also update the other runWithAsync JSDoc occurrence
that exposes the same path so both declarations document this SuppressedError
behavior.
- Around line 69-83: The polyfill uses an unsafe `as unknown as typeof
SuppressedError`; replace that with a local structural constructor type (e.g.
SuppressedErrorConstructor) that models the polyfill shape — a `new(error:
unknown, suppressed: unknown, message?: string)` returning an object with
`.error` and `.suppressed` and extends Error — then type the polyfill factory
and the exported constant `SuppressedErrorCtor` with that structural type
instead of double-casting; update the inner class `SuppressedErrorPolyfill` to
satisfy that type so no `as unknown as` assertions are needed.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e6d64aef-7796-45a8-b5e0-1f40cad1f14a
📒 Files selected for processing (1)
packages/core/src/facade.ts
Replace the `as unknown as typeof SuppressedError` double assertion with a local SuppressedErrorConstructor type that describes the polyfill's actual shape. Also add the missing @throws {SuppressedError} to runWithAsync() JSDoc. #771 (comment) #771 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
/gemini review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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.
Code Review
This pull request updates the runWith, runWithSync, and runWithAsync functions to prevent the loss of original parse errors when context disposal also fails. It implements a SuppressedError mechanism, including a polyfill for older runtimes, to wrap both failures according to TC39 conventions. The core logic has been refactored into internal functions to manage disposal sequences more effectively, and new test cases verify the error wrapping behavior across different execution modes. I have no feedback to provide.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/src/facade.ts`:
- Around line 3188-3190: Update the JSDoc `@throws` description for
SuppressedError to reflect that any exception thrown by runWithBody() or
runWithSyncBody() (not just parse failures) may be wrapped when a context
disposal also throws; reference SuppressedError and explain that the original
error (whatever its origin: parse errors, duplicate context ID, annotation
collection errors, etc.) is available via `.suppressed` and the disposal error
via `.error`. Apply the same broader wording to the other occurrences noted
(around the documentation blocks for the other two locations).
- Around line 3074-3127: The code currently treats any thrown exception during
the first pass like a parse failure and falls back to runParser(), which causes
the parser to be re-run; change the behavior so thrown exceptions are not
swallowed or retried: in the try/catch around parseAsync/parseSync do not set
firstPassFailed on catch — rethrow the caught error (or remove the catch) so
exceptions bubble out — keep the existing fallback path that calls
injectAnnotationsIntoParser and runParser only for actual Result objects where
success === false (i.e. when firstPassFailed is set due to a non-success Result
from parseAsync/parseSync, not due to an exception). Ensure you update the catch
handling around parseAsync/parseSync and references to
firstPassFailed/firstPassResult so runParser is only invoked for genuine
success:false results.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 77056049-3fe6-41d1-9001-056eac9464ee
📒 Files selected for processing (1)
packages/core/src/facade.ts
The SuppressedError wraps any exception from the body (not just parse failures), so say "the runner throws" instead of "parsing fails". #771 (comment) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
|
/gemini review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ 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.
Code Review
This pull request fixes an issue where runWith and runWithSync would discard the original parse error if a source context's disposal also threw an error. The changes introduce a SuppressedError to wrap both failures, including a polyfill for environments lacking native support. The core logic in facade.ts was refactored to separate the execution body from the disposal phase, ensuring that primary errors are preserved and wrapped if disposal fails. Additionally, comprehensive tests were added to cover various error scenarios, including multiple disposal failures using AggregateError. I have no feedback to provide as there were no review comments.
When parsing fails in
runWith()orrunWithSync()and a source context also throws during disposal, the disposal error silently replaces the original parse error. This happens because JavaScript'stry/finallysemantics cause thefinallyerror to take precedence over thetryerror.For example, given a context whose
Symbol.disposethrows and a parser that receives invalid input:This PR restructures the error handling in both
runWith()andrunWithSync()to preserve the original parse error when disposal also throws. The parsing body is extracted into helper functions (runWithBodyandrunWithSyncBody) in packages/core/src/facade.ts, and the callers handle disposal explicitly outsidetry/finallyblocks. When both parsing and disposal fail, the errors are wrapped in aSuppressedErrorfollowing TC39 conventions:.errorcontains the disposal error (the newer error) and.suppressedcontains the original parse error.Since
SuppressedErroris only available as a global starting from Node 24, but the package supportsnode >=20, a local polyfill is included that provides the same interface on older runtimes.Closes #246