feat(runtime): add getBridgeInfo() meta call#188
Merged
Conversation
|
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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a public getBridgeInfo(options?: GetBridgeInfoOptions) method to BridgeProtocol that fetches, validates, caches, and optionally refreshes bridge diagnostic info; introduced validateBridgeInfoPayload validator, GetBridgeInfoOptions interface, and bridgeInfoCache private field; added a test covering info reporting and instance counting. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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:runtime-node
![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
🤖 Fix all issues with AI agents
In `@src/runtime/bridge-protocol.ts`:
- Around line 53-60: The code in validateBridgeInfoPayload unsafely casts value
to BridgeInfo before validating fields; instead, treat value as Record<string,
unknown> (or const obj = value as Record<string, unknown>) and use indexed
access (obj['protocol'], obj['protocolVersion'], etc.) to validate types and
values against PROTOCOL_ID and TYWRAP_PROTOCOL_VERSION, then construct and
return a properly typed BridgeInfo object; remove the premature cast and only
cast/return after all runtime checks pass, throwing BridgeProtocolError where
checks fail.
- Around line 53-93: The validator validateBridgeInfoPayload currently enforces
a hardcoded bridge identifier ('python-subprocess') while the BridgeInfo type is
a literal for that value; if you intend to support multiple bridge identifiers
you must first update the BridgeInfo type to a union (e.g., bridge:
'python-subprocess' | 'python-http' | 'pyodide') and then relax the check in
validateBridgeInfoPayload to accept that union (use PROTOCOL_ID and
TYWRAP_PROTOCOL_VERSION checks as-is); additionally replace the repeated generic
'Invalid bridge info payload' throws with field-discriminated error messages
that include the failing field name and value (e.g., include candidate.bridge,
candidate.protocol, candidate.pythonVersion, candidate.pid,
candidate.codecFallback, etc.) to aid debugging while validating each property.
In `@test/runtime_node.test.ts`:
- Around line 141-142: The test currently asserts info.protocolVersion with
toBeGreaterThan(0), which is too loose; import the authoritative constant
TYWRAP_PROTOCOL_VERSION (from ../src/runtime/protocol.js or the module exporting
it) and change the assertion to
expect(info.protocolVersion).toBe(TYWRAP_PROTOCOL_VERSION) so the test fails on
any protocol mismatch between the TypeScript and Python bridge; reference the
test variable info and the constant TYWRAP_PROTOCOL_VERSION when making the
change.
- Around line 134-159: The test misses asserting the cache-hit path for
bridge.getBridgeInfo; after the initial info = await bridge.getBridgeInfo()
call, call bridge.getBridgeInfo() again without { refresh: true } and assert the
returned value is the same cached object (use strict reference equality) to
ensure caching is used (reference symbols: bridge.getBridgeInfo, info, cached);
keep the existing refresh checks (mid/after) unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/runtime/bridge-protocol.tstest/runtime_node.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:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In the tywrap repository, bridges currently use implicit ready detection: the first successful response from the Python subprocess proves the bridge is ready to handle requests.
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.
📚 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/bridge-protocol.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:
src/runtime/bridge-protocol.tstest/runtime_node.test.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/bridge-protocol.ts
📚 Learning: 2026-01-20T16:01:39.136Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:826-830
Timestamp: 2026-01-20T16:01:39.136Z
Learning: In the tywrap BridgeProtocol architecture (ADR-002), `HttpIO` (and other Transport implementations like `ProcessIO`, `PyodideIO`) implements only the `Transport` interface (methods: `send`, `isReady`, `dispose`). The `RuntimeExecution` interface methods (`call`, `instantiate`, `callMethod`, `disposeInstance`) are implemented by `BridgeProtocol`, which delegates to `Transport` instances. Transport implementations should not include RuntimeExecution methods.
Applied to files:
src/runtime/bridge-protocol.ts
📚 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/bridge-protocol.tstest/runtime_node.test.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/bridge-protocol.tstest/runtime_node.test.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/bridge-protocol.tstest/runtime_node.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_node.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/runtime_node.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/runtime_node.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/runtime_node.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 the tywrap repository, bridges currently use implicit ready detection: the first successful response from the Python subprocess proves the bridge is ready to handle requests.
Applied to files:
test/runtime_node.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_node.test.ts
🧬 Code graph analysis (1)
src/runtime/bridge-protocol.ts (3)
src/index.ts (3)
BridgeInfo(98-98)BridgeProtocolError(58-58)PROTOCOL_ID(20-20)src/runtime/transport.ts (1)
PROTOCOL_ID(20-20)src/runtime/protocol.ts (1)
TYWRAP_PROTOCOL_VERSION(2-2)
⏰ 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 (4)
src/runtime/bridge-protocol.ts (3)
31-37: LGTM!Clean interface definition. The
refreshflag provides a clear opt-in mechanism for bypassing the cache.
137-139: LGTM on the cache field.Private optional field with no default — appropriate for lazy population via
getBridgeInfo().
377-400:getBridgeInfo— clean implementation; cache behavior confirmed.The method correctly uses
sendMessage(notsendMessageAsync) since meta payloads won't contain Arrow-encoded data, and thevalidatecallback integrates well with theexecutepipeline.One behavior to be aware of: after
dispose(), a call togetBridgeInfo()withoutrefresh: truewill return cached data without attempting to contact the bridge. This happens because the cache check at line 385 bypasses the disposed guard inexecute(). This is acceptable if cached metadata is meant to remain readable post-disposal; if not, consider adding a disposed check before the cache return or document this as intended behavior.test/runtime_node.test.ts (1)
506-508: LGTM — cosmetic paren removal.Removing the parentheses from a single arrow-function parameter is a stylistic normalization with no behavioral change.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
53
to
93
| function validateBridgeInfoPayload(value: unknown): BridgeInfo { | ||
| if (!value || typeof value !== 'object' || Array.isArray(value)) { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
|
|
||
| const candidate = value as BridgeInfo; | ||
| if (candidate.protocol !== PROTOCOL_ID || candidate.protocolVersion !== TYWRAP_PROTOCOL_VERSION) { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
|
|
||
| if (candidate.bridge !== 'python-subprocess') { | ||
| throw new BridgeProtocolError(`Unexpected bridge identifier: ${candidate.bridge}`); | ||
| } | ||
|
|
||
| if (typeof candidate.pythonVersion !== 'string' || candidate.pythonVersion.length === 0) { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
| if (typeof candidate.pid !== 'number' || !Number.isFinite(candidate.pid)) { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
| if (candidate.codecFallback !== 'json' && candidate.codecFallback !== 'none') { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
| if (typeof candidate.arrowAvailable !== 'boolean') { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
| if (typeof candidate.scipyAvailable !== 'boolean') { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
| if (typeof candidate.torchAvailable !== 'boolean') { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
| if (typeof candidate.sklearnAvailable !== 'boolean') { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
| if (typeof candidate.instances !== 'number' || !Number.isFinite(candidate.instances)) { | ||
| throw new BridgeProtocolError('Invalid bridge info payload'); | ||
| } | ||
|
|
||
| return candidate; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# First, find the BridgeInfo type definition
fd src/runtime/bridge-protocol.ts -x head -100 {} | rg -A 20 "type BridgeInfo|interface BridgeInfo"Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
# Search for HttpBridge and PyodideBridge classes
rg -l "class HttpBridge|class PyodideBridge" src/Repository: bbopen/tywrap
Length of output: 99
🏁 Script executed:
# Check what bridges exist and their getBridgeInfo implementations
rg "getBridgeInfo" -A 5 src/runtime/Repository: bbopen/tywrap
Length of output: 824
🏁 Script executed:
# Look at the file header and class-level docs mentioned at line 11
head -20 src/runtime/bridge-protocol.tsRepository: bbopen/tywrap
Length of output: 811
🏁 Script executed:
# Check what HttpBridge returns for bridge identifier
cat -n src/runtime/http.ts | head -150Repository: bbopen/tywrap
Length of output: 3227
🏁 Script executed:
# Check what PyodideBridge returns
cat -n src/runtime/pyodide.ts | head -150Repository: bbopen/tywrap
Length of output: 3289
🏁 Script executed:
# Get the full BridgeInfo type definition
rg "type BridgeInfo|interface BridgeInfo" -A 30 src/types/Repository: bbopen/tywrap
Length of output: 1337
🏁 Script executed:
# Check if there are transport-specific implementations that return different bridge values
rg "bridge.*http|bridge.*pyodide" -i src/runtime/Repository: bbopen/tywrap
Length of output: 1462
🏁 Script executed:
# Check what each transport returns in its response handling
rg "BridgeInfo|bridge.*:" src/runtime/http-io.ts src/runtime/pyodide-io.ts | head -30Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
# Check if there are tests showing what getBridgeInfo returns for different bridges
fd test src/ | xargs rg "getBridgeInfo|BridgeInfo" | grep -E "http|pyodide" | head -20Repository: bbopen/tywrap
Length of output: 39
🏁 Script executed:
# Look at the full BridgeInfo type definition to understand the design intent
cat -n src/types/index.ts | rg -A 15 "export interface BridgeInfo"Repository: bbopen/tywrap
Length of output: 553
Consider whether BridgeInfo should support multiple bridge identifiers before extending the validator.
The hardcoded 'python-subprocess' check aligns with the BridgeInfo type definition, which explicitly declares bridge: 'python-subprocess' as a literal. HttpBridge and PyodideBridge inherit getBridgeInfo() from the base class and don't override the response format. If these bridges ever need to report different identifiers, the BridgeInfo type itself would need to change to a union type (e.g., bridge: 'python-subprocess' | 'python-http' | 'pyodide'). Extending the validator before updating the type contract could mask type safety issues. If supporting multiple identifiers is the intended design, start by updating the type definition.
Adding field discriminators to error messages would improve debugging:
♻️ Suggested error message improvement
- throw new BridgeProtocolError('Invalid bridge info payload');
+ throw new BridgeProtocolError('Invalid bridge info payload: missing or invalid pythonVersion');Similar discriminators for each field would aid debugging on this non-hot path.
🤖 Prompt for AI Agents
In `@src/runtime/bridge-protocol.ts` around lines 53 - 93, The validator
validateBridgeInfoPayload currently enforces a hardcoded bridge identifier
('python-subprocess') while the BridgeInfo type is a literal for that value; if
you intend to support multiple bridge identifiers you must first update the
BridgeInfo type to a union (e.g., bridge: 'python-subprocess' | 'python-http' |
'pyodide') and then relax the check in validateBridgeInfoPayload to accept that
union (use PROTOCOL_ID and TYWRAP_PROTOCOL_VERSION checks as-is); additionally
replace the repeated generic 'Invalid bridge info payload' throws with
field-discriminated error messages that include the failing field name and value
(e.g., include candidate.bridge, candidate.protocol, candidate.pythonVersion,
candidate.pid, candidate.codecFallback, etc.) to aid debugging while validating
each property.
![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: 9aa3657e5e
ℹ️ 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".
Comment on lines
+388
to
+391
| const info = await this.sendMessage<BridgeInfo>( | ||
| { | ||
| method: 'meta', | ||
| params: {}, |
There was a problem hiding this comment.
getBridgeInfo() sends a single meta request and caches that one response, but in Node pooled mode each request is executed by one worker (PooledTransport.send delegates to withWorker) and Python meta reports instances from that process-local map (len(instances) in runtime/python_bridge.py). After concurrent traffic has used multiple workers, this method can report a single worker’s pid/instances as if it were the whole bridge, which makes diagnostics and leak checks incorrect unless callers build their own aggregation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/runtime/bridge-protocol.ts`:
- Around line 154-159: The instances validation currently allows negatives and
fractions; update the check in the bridge-protocol (the block that reads const
instances = obj.instances) to require a non-negative integer (use
Number.isInteger(instances) && instances >= 0) instead of Number.isFinite, and
update the thrown BridgeProtocolError message to reflect "non-negative integer"
while continuing to include formatValue(instances) for context.
- Around line 112-117: The pid validation currently allows fractional and
negative numbers; update the check in the block around const pid = obj.pid so it
requires an integer greater than zero (use Number.isInteger(pid) && pid > 0) and
throw the same BridgeProtocolError with the existing formatted message
(formatValue(pid)) when the check fails to ensure only valid positive process
IDs are accepted.
- Around line 218-219: The bridgeInfoCache field holds stale BridgeInfo after
disposal; update the doDispose() method to clear this cache by setting
bridgeInfoCache = undefined when disposing the bridge so subsequent calls to
getBridgeInfo() without { refresh: true } cannot return stale data; locate the
bridgeInfoCache private field and modify doDispose() to explicitly
null/undefined that cache as part of the disposal sequence.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/runtime/bridge-protocol.tstest/runtime_node.test.ts
🧰 Additional context used
🧠 Learnings (19)
📓 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.
📚 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_node.test.tssrc/runtime/bridge-protocol.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/runtime_node.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/runtime_node.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/runtime_node.test.tssrc/runtime/bridge-protocol.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/runtime_node.test.tssrc/runtime/bridge-protocol.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/runtime_node.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_node.test.ts
📚 Learning: 2026-01-20T01:36:14.701Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:743-759
Timestamp: 2026-01-20T01:36:14.701Z
Learning: In the tywrap repository, when reviewing concurrent request handling patterns (especially with BridgeCore request multiplexing), flag uses of boolean `busy` flags that should be request counters instead. A boolean flag creates a race condition: when request A completes and sets `busy = false`, request B (still in flight via multiplexing) is incorrectly treated as idle. The fix is to use an `inFlightRequests: number` counter that increments on request start and decrements on completion, then check `worker.inFlightRequests === 0` before cleanup/termination.
Applied to files:
test/runtime_node.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: Policy for adversarial test fixtures: adversarial fixtures must preserve the failure mode they exercise, even if that behavior would be "bad" in real code. When reviewing test fixtures with unusual patterns (e.g., missing error handling, intentional protocol violations, unanswered requests), ask for clarifying comments about the intent rather than suggesting normalization or fixes.
Applied to files:
test/runtime_node.test.ts
📚 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:
test/runtime_node.test.tssrc/runtime/bridge-protocol.ts
📚 Learning: 2026-01-20T01:33:12.841Z
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.
Applied to files:
test/runtime_node.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 the tywrap repository, bridges currently use implicit ready detection: the first successful response from the Python subprocess proves the bridge is ready to handle requests.
Applied to files:
test/runtime_node.test.tssrc/runtime/bridge-protocol.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:
test/runtime_node.test.tssrc/runtime/bridge-protocol.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:
test/runtime_node.test.tssrc/runtime/bridge-protocol.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/bridge-protocol.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:
src/runtime/bridge-protocol.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/bridge-protocol.ts
📚 Learning: 2026-01-20T16:01:39.136Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:826-830
Timestamp: 2026-01-20T16:01:39.136Z
Learning: In the tywrap BridgeProtocol architecture (ADR-002), `HttpIO` (and other Transport implementations like `ProcessIO`, `PyodideIO`) implements only the `Transport` interface (methods: `send`, `isReady`, `dispose`). The `RuntimeExecution` interface methods (`call`, `instantiate`, `callMethod`, `disposeInstance`) are implemented by `BridgeProtocol`, which delegates to `Transport` instances. Transport implementations should not include RuntimeExecution methods.
Applied to files:
src/runtime/bridge-protocol.ts
🧬 Code graph analysis (1)
test/runtime_node.test.ts (1)
src/runtime/protocol.ts (1)
TYWRAP_PROTOCOL_VERSION(2-2)
🔇 Additional comments (5)
src/runtime/bridge-protocol.ts (3)
31-37: LGTM!Clean interface definition with good JSDoc. The optional
refreshflag with a clear description is a nice API design choice.
53-80: Good validation structure; addresses prior feedback.The
BridgeInfoWireintermediary type avoids premature casting toBridgeInfo, and each field now gets a discriminated error message. TheformatValuehelper correctly handlesJSON.stringifyreturningundefinedfor non-serializable values. This is appropriate validation for a non-hot-path meta call. Based on learnings, the subprocess boundary is effectively untrusted for response validation, and since this is not a hot-path loop, the overhead is justified.
464-481: [No changes needed] Thevalidatecallback's return value is already correctly propagated byexecute()at line 415 (return validate ? validate(result) : result;). The reconstructed object fromvalidateBridgeInfoPayloadis returned to the caller, not discarded. The validation serves its intended purpose.Likely an incorrect or invalid review comment.
test/runtime_node.test.ts (2)
135-163: Solid integration test covering the full getBridgeInfo lifecycle.The test covers initial fetch, cache-hit (reference equality), refresh after instance creation, and refresh after disposal — all key behaviors described in the PR objectives. The
TYWRAP_PROTOCOL_VERSIONimport tightens the protocol version assertion compared to the earliertoBeGreaterThan(0)approach.One small gap:
arrowAvailableandcodecFallbackare validated byvalidateBridgeInfoPayloadbut not asserted in the test. This is fine since the validator would throw on invalid values, but explicit assertions would document the expected shape more clearly if desired.
510-512: LGTM — minor style change only.Removing parentheses around a single arrow function parameter. No logic change.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
154
to
159
| const instances = obj.instances; | ||
| if (typeof instances !== 'number' || !Number.isFinite(instances)) { | ||
| throw new BridgeProtocolError( | ||
| `Invalid bridge info payload: instances expected finite number, got ${formatValue(instances)}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
instances validation allows fractional and negative numbers.
Same concern as pid — an instance count should be a non-negative integer. Number.isFinite permits -1 and 2.5.
🛡️ Proposed fix
const instances = obj.instances;
- if (typeof instances !== 'number' || !Number.isFinite(instances)) {
+ if (typeof instances !== 'number' || !Number.isInteger(instances) || instances < 0) {
throw new BridgeProtocolError(
- `Invalid bridge info payload: instances expected finite number, got ${formatValue(instances)}`
+ `Invalid bridge info payload: instances expected non-negative integer, got ${formatValue(instances)}`
);🤖 Prompt for AI Agents
In `@src/runtime/bridge-protocol.ts` around lines 154 - 159, The instances
validation currently allows negatives and fractions; update the check in the
bridge-protocol (the block that reads const instances = obj.instances) to
require a non-negative integer (use Number.isInteger(instances) && instances >=
0) instead of Number.isFinite, and update the thrown BridgeProtocolError message
to reflect "non-negative integer" while continuing to include
formatValue(instances) for context.
bbopen
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
Addressed requested changes (pid/instances integer validation + clear cache on dispose). Dismissing stale bot review to unblock merge.
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.
Adds BridgeProtocol.getBridgeInfo() backed by Python bridge meta. Includes payload validation + caching (refresh optional) and a Node integration test.