Conversation
bbopen
added
area:runtime-http
📝 WalkthroughWalkthroughThis PR adds cycle-aware traversal to prevent infinite recursion when validating and encoding values with circular references. Helper functions in safe-codec.ts and validators.ts now track visited objects via WeakSet, and tests verify proper error handling for non-serializable values in HttpBridge calls. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
added
area:codec
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/safe-codec.ts (1)
504-511:⚠️ Potential issue | 🟠 MajorPre-existing bug: post-decode float validation checks
resultinstead ofdecoded.In
decodeResponseAsync, the special-float check on line 506 inspectsresult(the pre-Arrow value), butdecoded(the Arrow-decoded value) is what's actually returned on line 513. Arrow decoders can introduce NaN/Infinity values from binary representations, so the validation should run againstdecoded.Not introduced by this PR, but directly relevant to the cycle-detection traversal being added.
🐛 Proposed fix
// Post-decode validation for special floats if enabled // Note: We check the result value since that's what we're returning - if (this.rejectSpecialFloats && containsSpecialFloat(result)) { - const floatPath = findSpecialFloatPath(result); + if (this.rejectSpecialFloats && containsSpecialFloat(decoded)) { + const floatPath = findSpecialFloatPath(decoded); throw new BridgeProtocolError( `Response contains non-finite number (NaN or Infinity) at ${floatPath}` ); } - return decoded as T; + return decoded as T;Based on learnings: "In the tywrap project's BridgeProtocol SafeCodec implementation, Arrow format decoders can produce NaN/Infinity values from binary representations even when the raw JSON payload doesn't contain them. This is why validation for special floats must occur both before encoding (to reject invalid inputs) and after applying decoders (to catch values introduced during Arrow deserialization)."
🤖 Fix all issues with AI agents
In `@test/runtime_http.test.ts`:
- Around line 14-32: Refactor the test that calls HttpBridge.call to use
Vitest's async assertion style instead of the nested try/catch: replace the
inner try/catch + expect.fail with an await
expect(bridge.call('math','sqrt',[1n])).rejects.toBeInstanceOf(BridgeProtocolError)
and assert the error message via .rejects.toThrow(/JSON serialization failed/i)
(or equivalent combined checks), keep the fetchMock assertion
(expect(fetchMock).not.toHaveBeenCalled()) and ensure bridge.dispose() still
runs in the finally block; locate the test by the it description and references
to HttpBridge, call, and BridgeProtocolError.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/runtime/safe-codec.tssrc/runtime/validators.tstest/runtime_http.test.tstest/safe-codec.test.tstest/validators.test.ts
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:58.747Z
Learning: In the BridgeProtocol implementation for the tywrap repository, Map and Set instances should be explicitly rejected before serialization (e.g., in `safeStringify` or at the serialization entrypoint) with a clear error message like "Bridge protocol does not support Map/Set values", rather than checking for non-string keys after the fact, since Maps and Sets cannot round-trip through JSON.stringify.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:168-172
Timestamp: 2026-01-20T16:00:49.738Z
Learning: In the tywrap project's BridgeProtocol SafeCodec implementation, Arrow format decoders can produce NaN/Infinity values from binary representations even when the raw JSON payload doesn't contain them. This is why validation for special floats must occur both before encoding (to reject invalid inputs) and after applying decoders (to catch values introduced during Arrow deserialization), protecting downstream consumers from unexpected NaN/Infinity values.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:238-249
Timestamp: 2026-01-20T01:33:12.841Z
Learning: In the tywrap repository, when reviewing init/startup patterns, prioritize cases that break core functionality over edge cases with diagnostic-only impact (e.g., missing bridgeInfo metadata). If workers are healthy and operational, defer low-impact defensive checks to follow-up PRs.
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
src/runtime/safe-codec.tstest/safe-codec.test.tstest/runtime_http.test.tstest/validators.test.tssrc/runtime/validators.ts
📚 Learning: 2026-01-19T21:14:35.390Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:35.390Z
Learning: In src/runtime/bridge-core.ts and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (such as runtime, shape, or error-payload checks) inside tight loops unless the protocol boundary is untrusted or there is a concrete bug report. The Python bridge protocol is controlled via tests, so these checks add unnecessary branching overhead without meaningful benefit. Apply this guidance to other hot-path runtime loops in src/runtime/**/*.ts, and re-enable additional validations only when a documented risk or failure scenario is identified. Ensure tests cover protocol validation where applicable.
Applied to files:
src/runtime/safe-codec.tssrc/runtime/validators.ts
📚 Learning: 2026-01-19T21:14:29.869Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
Learning: In the runtime env-var parsing (e.g., in src/runtime/bridge-core.ts and similar modules), adopt a tolerant, best-effort policy: numeric env values should parse by taking the leading numeric portion (e.g., TYWRAP_CODEC_MAX_BYTES=1024abc -> 1024). Only reject clearly invalid values (non-numeric start or <= 0). This reduces surprising failures from minor typos. Add tests to cover partial numeric prefixes, and ensure downstream logic documents the trusted range; consider a small helper to extract a positive integer from a string with a safe fallback.
Applied to files:
src/runtime/safe-codec.tssrc/runtime/validators.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
src/runtime/safe-codec.tstest/safe-codec.test.tstest/runtime_http.test.tstest/validators.test.tssrc/runtime/validators.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
src/runtime/safe-codec.tstest/safe-codec.test.tstest/runtime_http.test.tstest/validators.test.tssrc/runtime/validators.ts
📚 Learning: 2026-01-20T16:00:49.738Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:168-172
Timestamp: 2026-01-20T16:00:49.738Z
Learning: In the tywrap project's BridgeProtocol SafeCodec implementation, Arrow format decoders can produce NaN/Infinity values from binary representations even when the raw JSON payload doesn't contain them. This is why validation for special floats must occur both before encoding (to reject invalid inputs) and after applying decoders (to catch values introduced during Arrow deserialization), protecting downstream consumers from unexpected NaN/Infinity values.
Applied to files:
test/safe-codec.test.ts
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/safe-codec.test.tstest/runtime_http.test.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/safe-codec.test.tstest/runtime_http.test.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
test/safe-codec.test.tstest/runtime_http.test.ts
📚 Learning: 2026-01-19T21:14:37.032Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Applied to files:
test/safe-codec.test.ts
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/safe-codec.test.tstest/runtime_http.test.tstest/validators.test.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
test/runtime_http.test.ts
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: In `test/fixtures/out_of_order_bridge.py`, the fixture intentionally leaves a pending request unanswered at EOF to simulate missing/out-of-order responses and validate bridge behavior when requests never complete; this is the exact failure mode being tested and must be preserved.
Applied to files:
test/runtime_http.test.ts
🧬 Code graph analysis (3)
src/runtime/safe-codec.ts (1)
src/runtime/validators.ts (1)
isPlainObject(53-55)
test/safe-codec.test.ts (3)
test/python/test_safe_codec.py (1)
codec(35-37)src/runtime/safe-codec.ts (1)
SafeCodec(304-529)src/index.ts (2)
SafeCodec(17-17)BridgeProtocolError(58-58)
test/validators.test.ts (2)
src/runtime/validators.ts (3)
containsSpecialFloat(206-208)assertNoSpecialFloats(217-223)ValidationError(11-16)src/index.ts (3)
containsSpecialFloat(45-45)assertNoSpecialFloats(46-46)ValidationError(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: os (windows-latest)
🔇 Additional comments (8)
test/validators.test.ts (2)
318-330: Good coverage for circular reference handling incontainsSpecialFloat.Both cases are well-crafted: the first verifies cycle traversal terminates without overflow when no special float is present, and the second confirms detection still works when a special float exists within a cyclic graph.
350-356: LGTM — assertion wrapper correctly tested with circular structures.This complements the
containsSpecialFloatcircular tests by verifying theassertNoSpecialFloatspath throwsValidationErroras expected.test/runtime_http.test.ts (2)
8-12: Proper test isolation withafterEachcleanup.Saving the original
fetch, restoring mocks, and resettingglobalThis.fetchensures tests don't leak state. Thebridge.dispose()in each test'sfinallyblock is also appropriate.
34-55: Circular reference test correctly validates the pre-transport guardrail.The assertion that
fetchMockwas not called confirms the serialization error is caught before any network activity, which directly satisfies the acceptance criteria from issue#120.test/safe-codec.test.ts (1)
251-265: Good test refactoring — covers both default and guardrails-disabled paths for circular references.The first test (default
SafeCodec()) validates the common path, while the second test (guardrails disabled) ensures theJSON.stringifycatch block is the final safety net. Together they confirmBridgeProtocolErroris always raised regardless of configuration.src/runtime/safe-codec.ts (2)
93-104: Cycle detection inassertStringKeysis correctly implemented.The
visitedWeakSet parameter with a default value ensures backward compatibility for the top-level call, while early-return on revisited objects prevents infinite recursion. The null/primitive guard at line 98 correctly precedes the WeakSet check since primitives can't be stored in a WeakSet.
248-273: Cycle detection infindSpecialFloatPathlooks correct.Same sound pattern as
assertStringKeys: primitives checked first, then null/non-object guard, then visited check. Thevisitedset is properly threaded through all recursive calls.src/runtime/validators.ts (1)
186-208: Clean cycle-aware refactoring with correct encapsulation.The private
containsSpecialFloatRecursivehelper withWeakSet<object>follows the same sound pattern used insafe-codec.ts. The public API signature is unchanged, and using.some()for short-circuit traversal is idiomatic. Good separation between the internal recursive helper and the public wrapper.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+14
to
+32
| it('surfaces BigInt args serialization failures as BridgeProtocolError', async () => { | ||
| const fetchMock = vi.fn(); | ||
| globalThis.fetch = fetchMock as unknown as typeof fetch; | ||
|
|
||
| const bridge = new HttpBridge({ baseURL: 'http://localhost:8000' }); | ||
| try { | ||
| try { | ||
| await bridge.call('math', 'sqrt', [1n]); | ||
| expect.fail('Expected serialization failure'); | ||
| } catch (error) { | ||
| expect(error).toBeInstanceOf(BridgeProtocolError); | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| expect(message).toMatch(/JSON serialization failed/i); | ||
| } | ||
| expect(fetchMock).not.toHaveBeenCalled(); | ||
| } finally { | ||
| await bridge.dispose(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using Vitest's idiomatic rejects pattern for cleaner async assertions.
The nested try/catch + expect.fail works but is verbose. Vitest provides await expect(...).rejects.toThrow(...) which is more concise and conventional for async error assertions.
♻️ Proposed refactor using idiomatic Vitest assertions
it('surfaces BigInt args serialization failures as BridgeProtocolError', async () => {
const fetchMock = vi.fn();
globalThis.fetch = fetchMock as unknown as typeof fetch;
const bridge = new HttpBridge({ baseURL: 'http://localhost:8000' });
try {
- try {
- await bridge.call('math', 'sqrt', [1n]);
- expect.fail('Expected serialization failure');
- } catch (error) {
- expect(error).toBeInstanceOf(BridgeProtocolError);
- const message = error instanceof Error ? error.message : String(error);
- expect(message).toMatch(/JSON serialization failed/i);
- }
+ await expect(bridge.call('math', 'sqrt', [1n])).rejects.toThrow(BridgeProtocolError);
+ await expect(bridge.call('math', 'sqrt', [1n])).rejects.toThrow(/JSON serialization failed/i);
expect(fetchMock).not.toHaveBeenCalled();
} finally {
await bridge.dispose();
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('surfaces BigInt args serialization failures as BridgeProtocolError', async () => { | |
| const fetchMock = vi.fn(); | |
| globalThis.fetch = fetchMock as unknown as typeof fetch; | |
| const bridge = new HttpBridge({ baseURL: 'http://localhost:8000' }); | |
| try { | |
| try { | |
| await bridge.call('math', 'sqrt', [1n]); | |
| expect.fail('Expected serialization failure'); | |
| } catch (error) { | |
| expect(error).toBeInstanceOf(BridgeProtocolError); | |
| const message = error instanceof Error ? error.message : String(error); | |
| expect(message).toMatch(/JSON serialization failed/i); | |
| } | |
| expect(fetchMock).not.toHaveBeenCalled(); | |
| } finally { | |
| await bridge.dispose(); | |
| } | |
| }); | |
| it('surfaces BigInt args serialization failures as BridgeProtocolError', async () => { | |
| const fetchMock = vi.fn(); | |
| globalThis.fetch = fetchMock as unknown as typeof fetch; | |
| const bridge = new HttpBridge({ baseURL: 'http://localhost:8000' }); | |
| try { | |
| const promise = bridge.call('math', 'sqrt', [1n]); | |
| await expect(promise).rejects.toThrow(BridgeProtocolError); | |
| expect(fetchMock).not.toHaveBeenCalled(); | |
| } finally { | |
| await bridge.dispose(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@test/runtime_http.test.ts` around lines 14 - 32, Refactor the test that calls
HttpBridge.call to use Vitest's async assertion style instead of the nested
try/catch: replace the inner try/catch + expect.fail with an await
expect(bridge.call('math','sqrt',[1n])).rejects.toBeInstanceOf(BridgeProtocolError)
and assert the error message via .rejects.toThrow(/JSON serialization failed/i)
(or equivalent combined checks), keep the fetchMock assertion
(expect(fetchMock).not.toHaveBeenCalled()) and ensure bridge.dispose() still
runs in the finally block; locate the test by the it description and references
to HttpBridge, call, and BridgeProtocolError.
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
BridgeProtocolErrorbefore transport sendfetchTesting
Closes #120