Unify Node bridge core and harden guardrails #127
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces a shared BridgeCore abstraction for RPC over subprocesses, migrates NodeBridge and OptimizedNodeBridge to use it, adds request-size limits and diagnostics to the Python bridge, documents new options/env vars, and adds extensive adversarial tests and fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant NodeBridge as NodeBridge
participant BridgeCore as BridgeCore
participant PythonProc as Python Process
participant SubprocIO as Subprocess Stdout/Stderr
Client->>NodeBridge: send(payload)
NodeBridge->>BridgeCore: send(payload)
activate BridgeCore
BridgeCore->>BridgeCore: serialize request, assign id, start timer
BridgeCore->>PythonProc: write(JSONL line)
deactivate BridgeCore
PythonProc->>SubprocIO: emit JSONL response line
SubprocIO->>BridgeCore: handleStdoutData(chunk)
activate BridgeCore
BridgeCore->>BridgeCore: buffer/extract lines, enforce maxLineLength
BridgeCore->>BridgeCore: parse & validate protocol, decode result
BridgeCore->>BridgeCore: resolve matching pending request, clear timer
deactivate BridgeCore
BridgeCore->>NodeBridge: deliver result
NodeBridge->>Client: return result
alt Timeout
BridgeCore->>BridgeCore: timer fires → mark request failed
BridgeCore->>BridgeCore: capture bounded stderr tail
BridgeCore->>NodeBridge: reject with timeout/BridgeExecutionError
NodeBridge->>Client: throw timeout error
end
alt Protocol error (e.g., oversized line)
BridgeCore->>BridgeCore: detect protocol violation → mark fatal
BridgeCore->>BridgeCore: reject all pending requests
BridgeCore->>NodeBridge: onFatalError callback
NodeBridge->>Client: throw BridgeProtocolError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
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
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (7)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: 2026-01-19T21:00:35.449ZApplied to files:
📚 Learning: 2026-01-19T21:14:35.390ZApplied to files:
📚 Learning: 2026-01-19T21:00:35.449ZApplied to files:
📚 Learning: 2026-01-19T21:00:22.461ZApplied to files:
📚 Learning: 2026-01-19T21:00:35.449ZApplied to files:
📚 Learning: 2026-01-19T21:14:29.869ZApplied to files:
🧬 Code graph analysis (4)test/fixtures/unexpected_id_bridge.py (2)
test/fixtures/out_of_order_bridge.py (4)
test/fixtures/wrong_protocol_bridge.py (2)
test/fixtures/missing_id_bridge.py (1)
🪛 Ruff (0.14.13)test/fixtures/unexpected_id_bridge.py30-30: Do not catch blind exception: (BLE001) 39-39: Do not catch blind exception: (BLE001) test/fixtures/out_of_order_bridge.py37-37: Do not catch blind exception: (BLE001) test/fixtures/wrong_protocol_bridge.py30-30: Do not catch blind exception: (BLE001) test/fixtures/missing_id_bridge.py30-30: Do not catch blind exception: (BLE001) test/fixtures/string_id_bridge.py30-30: Do not catch blind exception: (BLE001) 🔇 Additional comments (10)
✏️ Tip: You can disable this entire section by setting Comment |
![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: ca4ea0fdbb
ℹ️ 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".
coderabbitai
bot
added
area:ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@src/runtime/node.ts`:
- Around line 252-284: The buildEnv function currently filters process.env down
to only keys in allowedKeys or with allowedPrefixes (TYWRAP_), which can break
setups that depend on other variables (cert paths, locale, LD_LIBRARY_PATH,
etc.); update buildEnv to either (a) respect a new option like
options.inheritProcessEnv (boolean) that when true merges the entire process.env
into baseEnv before normalization, or (b) expand the allowlist via a documented
configuration option so callers can opt-in to inheriting specific additional
keys; ensure references to allowedPrefixes, allowedKeys, options.env, and
options.virtualEnv remain correct and document the new option or behavior change
in the runtime API docs.
In `@src/runtime/optimized-node.ts`:
- Around line 190-200: The cached-return path in optimized-node.ts increments
this.stats.cacheHits but skips the aggregated update logic, so totalRequests and
derived metrics (cacheHitRate) are not updated; before returning the cached
value from the branch that uses this.safeCacheKey(...) and
globalCache.get<T>(cacheKey), invoke the same aggregate update routine (e.g.
call this.updateStats or the existing stats-aggregation helper) to increment
totalRequests and account for the cache hit—ensure you reference the same
updateStats mechanism used elsewhere so cacheHits and totalRequests remain
consistent when returning cached results.
In `@test/adversarial_playground.test.ts`:
- Around line 42-62: The local variable pythonPath returned from
resolvePythonForTests can be null but isn't narrowed after calling
pythonAvailable(pythonPath); update createBridge to ensure a non-null string is
passed to the NodeBridge constructor by either adding an explicit
null-check/guard (e.g., if (!pythonPath) return null) or using a non-null
assertion when supplying pythonPath, referencing the createBridge function and
the pythonAvailable/resolvePythonForTests flow so the NodeBridge instantiation
receives a correctly typed string.
- Around line 339-360: The test "includes recent stderr in timeout errors"
currently performs recovery assertions (the post-timeout call to callAdversarial
and expect(result).toBe('post-timeout')) inside the finally block which can mask
the original timeout error; refactor so the finally block only performs cleanup
(await bridge.dispose()) and move the recovery sequence (await delay(600); const
result = await callAdversarial(bridge, 'echo', ['post-timeout']);
expect(result).toBe('post-timeout')) outside the finally after the try/catch so
the catch can assert the timeout message from error and any failures in recovery
do not overwrite the original test failure; act on the existing variables
bridge, callAdversarial, delay and error handling in the test to implement this
change.
In `@test/codec-performance.test.ts`:
- Around line 68-84: The memory measurement is asymmetric because runGc() is
only called before startMem but again after the timed loop, which can make
deltaMem unreliable; update the test in test/codec-performance.test.ts so GC
handling is consistent — either remove the post-loop runGc() call or invoke
runGc() both before recording startMem and again immediately before recording
endMem; adjust the block containing runGc(), startMem, start,
decodeValue(sparseEnvelope/torchEnvelope/sklearnEnvelope) loop, and
endMem/deltaMem assertions accordingly (symbols: runGc, startMem, endMem,
deltaMem, decodeValue, iterations, timeBudgetMs, memoryBudgetBytes).
In `@test/fixtures/out_of_order_bridge.py`:
- Around line 29-48: The main() function can leave a pending message unanswered
if stdin ends while pending is non-None; add logic at EOF to flush pending by
calling send_response(pending, 1) if pending is not None so every stored request
is responded to; update the end of main() (after the for line in sys.stdin loop)
to check pending and call send_response(pending, 1), referencing the existing
pending variable and send_response function to implement the EOF flush.
In `@test/fixtures/oversized_line_bridge.py`:
- Around line 1-15: Add a short inline comment in the test fixture near the
payload creation (in function main where payload = "x" * 1100000) explaining
that 1,100,000 bytes intentionally exceeds typical 1MB line-buffer limits for
IPC/line-based readers, and note why that size was chosen (e.g., to test
oversized-line handling); place the comment directly above the payload variable
so it's obvious to future readers and reviewers.
In `@test/optimized-node.test.ts`:
- Around line 400-403: The two chained awaits on the same promise (the variable
"promise" used with BridgeProtocolError) should be consolidated into a single
assertion that verifies both the error type and the message to avoid relying on
repeated evaluation of the same settled promise; update the test in
optimized-node.test.ts to replace the two awaits with one assertion that checks
the rejected error is an instance of BridgeProtocolError and that its message
matches /Failed to serialize request/ (e.g. using a single
expect(...).rejects.toSatisfy or by awaiting the rejection into an error
variable and asserting both instanceof BridgeProtocolError and the regex against
error.message).
- Around line 417-434: The test accesses the private processPool via a type
assertion on bridge (used in the block that assigns pool and worker and calls
worker.process.stdin?.destroy()) which is fragile; add an inline comment above
the access explaining that this is intentional coupling for integration tests
and may break if internal implementation changes, and reference the specific
symbols processPool, bridge, worker, and worker.process.stdin?.destroy() so
future maintainers understand why the private field is used and that changes to
the internal API will require updating this test.
In `@test/runtime_bridge_fixtures.test.ts`:
- Around line 59-80: The test gates on a detected pythonPath but doesn't pass it
into the bridge constructors, so NodeBridge and OptimizedNodeBridge may spawn
the wrong python binary; update the two constructor calls (NodeBridge({...}) and
new OptimizedNodeBridge({...})) to include the detected pythonPath option (e.g.,
pythonPath: pythonPath) alongside scriptPath and timeoutMs, then keep the
existing init and call assertions unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/ci.ymlREADME.mdROADMAP.mddocs/api/README.mddocs/configuration.mddocs/runtimes/nodejs.mdruntime/python_bridge.pysrc/config/index.tssrc/runtime/bridge-core.tssrc/runtime/node.tssrc/runtime/optimized-node.tssrc/utils/codec.tssrc/utils/logger.tssrc/utils/python.tstest/adversarial_playground.test.tstest/bridge-core.test.tstest/codec-performance.test.tstest/fixtures/missing_id_bridge.pytest/fixtures/out_of_order_bridge.pytest/fixtures/oversized_line_bridge.pytest/fixtures/python/adversarial_module.pytest/fixtures/string_id_bridge.pytest/fixtures/unexpected_id_bridge.pytest/fixtures/wrong_protocol_bridge.pytest/optimized-node.test.tstest/performance-budgets.test.tstest/runtime_bridge_fixtures.test.ts
💤 Files with no reviewable changes (1)
- src/utils/python.ts
🧰 Additional context used
🧬 Code graph analysis (7)
test/runtime_bridge_fixtures.test.ts (1)
src/utils/runtime.ts (1)
getPythonExecutableName(568-570)
test/optimized-node.test.ts (2)
src/index.ts (2)
isNodejs(68-68)BridgeProtocolError(15-15)src/utils/runtime.ts (1)
isNodejs(146-148)
test/fixtures/wrong_protocol_bridge.py (2)
test/fixtures/missing_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/string_id_bridge.py (2)
meta_payload(10-20)main(23-37)
src/runtime/bridge-core.ts (2)
src/runtime/timed-out-request-tracker.ts (1)
TimedOutRequestTracker(10-58)src/runtime/protocol.ts (2)
TYWRAP_PROTOCOL(1-1)TYWRAP_PROTOCOL_VERSION(2-2)
test/fixtures/unexpected_id_bridge.py (3)
test/fixtures/missing_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/string_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/wrong_protocol_bridge.py (2)
meta_payload(10-20)main(23-37)
test/bridge-core.test.ts (1)
src/runtime/bridge-core.ts (2)
normalizeEnv(356-393)BridgeCore(52-316)
test/adversarial_playground.test.ts (2)
src/utils/python.ts (1)
resolvePythonExecutable(47-78)src/runtime/node.ts (1)
NodeBridge(76-308)
🪛 Ruff (0.14.13)
test/fixtures/string_id_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
test/fixtures/out_of_order_bridge.py
37-37: Do not catch blind exception: Exception
(BLE001)
test/fixtures/wrong_protocol_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
test/fixtures/python/adversarial_module.py
15-15: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
15-15: Dynamically typed expressions (typing.Any) are disallowed in echo
(ANN401)
23-23: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
23-23: Dynamically typed expressions (typing.Any) are disallowed in sleep_and_return
(ANN401)
40-40: Dynamically typed expressions (typing.Any) are disallowed in return_unserializable
(ANN401)
66-66: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
66-66: Dynamically typed expressions (typing.Any) are disallowed in print_to_stdout
(ANN401)
75-75: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
75-75: Dynamically typed expressions (typing.Any) are disallowed in write_to_stderr
(ANN401)
test/fixtures/unexpected_id_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
38-38: Do not catch blind exception: Exception
(BLE001)
test/fixtures/missing_id_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
runtime/python_bridge.py
145-147: try-except-pass detected, consider logging the exception
(S110)
145-145: Do not catch blind exception: Exception
(BLE001)
687-687: Avoid specifying long messages outside the exception class
(TRY003)
705-705: Avoid specifying long messages outside the exception class
(TRY003)
718-718: Avoid specifying long messages outside the exception class
(TRY003)
731-731: Avoid specifying long messages outside the exception class
(TRY003)
757-757: Avoid specifying long messages outside the exception class
(TRY003)
800-800: Consider moving this statement to an else block
(TRY300)
816-816: Abstract raise to an inner function
(TRY301)
852-852: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (56)
src/utils/codec.ts (3)
174-182: LGTM!The CJS-first with ESM fallback pattern is appropriate for apache-arrow loading across different Node.js module environments. The silent catch before fallback is the correct approach here.
431-433: LGTM!Formatting change only; validation semantics unchanged.
428-454: LGTM!The
sklearn.estimatorhandling correctly validates theversionfield when present and propagates it in the returned object. The ternary on line 447 is necessary for TypeScript type narrowing despite the earlier validation guard..github/workflows/ci.yml (1)
162-167: LGTM! Expanded codec-suite triggers look correct.The condition properly enables codec tests on pushes to main, scheduled runs, manual dispatches, and PRs labeled
area:codec. The YAML multiline syntax and label check are both valid.src/utils/logger.ts (1)
137-138: LGTM! Lint suppression is appropriate.The
levelparameter is constrained to theLogLevelunion type, making object injection impossible here. The inline comment accurately documents the rationale.src/config/index.ts (1)
317-318: LGTM! Lint suppression is appropriate for temporary file write.The temp path is safely derived from the already-validated config file location using
dirname(resolved)combined withrandomUUID()for uniqueness. The file is also properly cleaned up in thefinallyblock.test/performance-budgets.test.ts (2)
1-1: LGTM! Explicit import is cleaner.Adding the explicit
performanceimport fromnode:perf_hooksis better practice than relying on global availability.
13-14: LGTM! Simplified gating logic.The test is now controlled by the
TYWRAP_PERF_BUDGETSenvironment variable, which aligns with the CI workflow configuration and provides consistent behavior across environments.ROADMAP.md (1)
1-7: Verify roadmap reflects current PR progress.Based on the PR summary, this PR "extracts a shared JSONL bridge core for NodeBridge and OptimizedNodeBridge," which appears to address the first roadmap item. Consider updating the checklist to mark completed items:
## Runtime Bridges -- [ ] Unify NodeBridge and OptimizedNodeBridge behind a shared JSONL transport core. +- [x] Unify NodeBridge and OptimizedNodeBridge behind a shared JSONL transport core.docs/configuration.md (1)
299-310: LGTM!The new environment variable documentation is well-organized, follows the existing style, and provides clear defaults. The grouping of codec-specific performance variables together improves discoverability.
README.md (1)
57-68: LGTM!The documentation accurately reflects the API change (
timeout→timeoutMs) and clearly explains the relationship betweenNodeBridgeandOptimizedNodeBridge, including the shared JSONL core and payload size limits.test/codec-performance.test.ts (1)
25-66: LGTM!The test envelope definitions are well-structured and representative of the codec paths being exercised. The use of
as constassertions is appropriate for type narrowing.runtime/python_bridge.py (8)
57-71: LGTM!The new
RequestMaxBytesParseErrorandRequestTooLargeErrorclasses follow the established pattern fromCodecMaxBytesParseErrorandPayloadTooLargeError, maintaining consistency in the error hierarchy.
99-122: LGTM!The
get_request_max_bytes()implementation is consistent with the existingget_codec_max_bytes()pattern and handles edge cases appropriately (None, empty string, non-positive values).
129-147: LGTM!The
emit_protocol_diagnosticfunction correctly bounds output to prevent stderr flooding and silently catches exceptions to avoid breaking the main loop. The static analysis hints (S110, BLE001) are acceptable here since diagnostic code should never interrupt protocol handling.
697-732: LGTM!The validation helpers (
require_str,coerce_list,coerce_dict) centralize parameter validation, reducing duplication across handlers. The inline error messages (flagged by TRY003) are appropriately short and contextual for validation errors.
685-694: LGTM!The addition of the
isinstance(msg, dict)check improves protocol validation by rejecting malformed payloads early.
790-802: LGTM!The
write_payloadfunction correctly centralizes stdout writes and handlesBrokenPipeErrorgracefully, allowing the main loop to exit cleanly when the parent process terminates.
813-853: LGTM!The main loop improvements are well-structured:
- Request size validation occurs before JSON parsing, preventing memory exhaustion from oversized payloads
- Request ID preservation ensures error responses can be correlated with requests
- Comprehensive error handling maintains protocol compliance throughout
619-661: LGTM!The handler functions now use the centralized validation helpers consistently, improving code quality and ensuring uniform error messages across all request types.
test/fixtures/python/adversarial_module.py (6)
1-21: LGTM!The module docstring and basic
echofunction are well-documented. The static analysis warnings aboutAnytypes (ANN401) are acceptable for test fixtures that intentionally handle arbitrary values to exercise the bridge.
23-29: LGTM!The timing functions correctly use
time.sleepfor delay testing and includefloat()coercion for type safety.Also applies to: 85-93
32-63: LGTM!The payload generation functions correctly produce their intended edge cases:
- Large strings for size limit testing
- Sets for non-JSON-serializable values
- Circular references for recursion detection
- NaN/Infinity for invalid JSON number handling
66-82: LGTM!The I/O functions correctly test the protocol's handling of stdout noise (which should cause errors) and stderr output (which should not break protocol parsing).
96-109: LGTM!The error and crash functions are correctly implemented:
raise_errortests exception propagation through the bridgecrash_processusesos._exit()appropriately to simulate hard crashes without cleanup
112-219: LGTM!The malformed envelope functions provide comprehensive coverage of decoder validation paths:
- Unsupported codec versions and encodings
- Missing required fields (
b64,data)- Invalid sparse matrix format and shape
- Invalid nested types for torch and sklearn envelopes
Each function is well-documented with its testing purpose.
src/runtime/bridge-core.ts (9)
7-43: LGTM!The type definitions are well-structured:
RpcMethodcovers all supported bridge operationsRpcRequestandRpcResponseaccurately model the JSONL protocolBridgeCoreOptionsprovides sensible defaults and hooks for customizationPendingRequestcorrectly captures the promise lifecycle with timer
52-77: LGTM!The constructor properly initializes state with sensible defaults:
- 1MB max line length prevents unbounded memory growth
- 8KB stderr buffer is sufficient for diagnostics
TimedOutRequestTrackerTTL ofmax(1000, timeoutMs * 2)ensures late responses are properly discarded
100-143: LGTM!The
send()method correctly manages the request lifecycle:
- JSON serialization errors are caught and wrapped in
BridgeProtocolError- Timeout handling includes stderr context for debugging
- Transport write failures properly reject the pending request and trigger fatal error handling
- The promise is correctly wired to the pending request before any operations that could fail
145-181: LGTM!The
handleStdoutDatamethod implements robust line buffering:
- Pre-newline buffer size check prevents DoS from extremely long lines
- Individual line length validation provides defense in depth
- Protocol errors properly terminate further processing
183-194: LGTM!The stderr buffering correctly keeps the most recent bytes (tail) when the buffer exceeds the limit, which is appropriate for diagnostic purposes. The silent error handling prevents stderr processing from breaking the main protocol.
211-262: LGTM!The
handleResponseLinemethod provides comprehensive response handling:
- JSON parsing errors are caught and reported as protocol errors
- Protocol and ID validation ensures response integrity
- Late responses (after timeout) are silently consumed via
TimedOutRequestTrackerinstead of triggering spurious protocol errors- Decode errors are properly wrapped and propagated
264-315: LGTM!The error handling methods are well-designed:
errorFrompreserves Python traceback for debuggingfailRequestandrejectAllproperly clean up timers before rejecting promiseshandleProtocolErrorincludes a helpful hint about stdout pollution and guards against duplicate invocationshandleFatalErrorsafely invokes the callback without propagating exceptions
318-401: LGTM!The utility functions are well-implemented:
validateBridgeInfoensures protocol compatibility before proceedinggetPathKeyandnormalizeEnvcorrectly handle Windows PATH case-insensitivityensurePythonEncodingsets bothPYTHONUTF8andPYTHONIOENCODINGfor robust cross-platform UTF-8 handling- The
eslint-disablecomments fordetect-object-injectionare appropriate since environment keys are inherently dynamic
79-98: LGTM!The accessor and cleanup methods are correctly implemented:
getStderrTail()trims whitespace for cleaner diagnostic outputgetPendingCount()enables external monitoring of in-flight requestsclear()properly resets all state including pending request timerssrc/runtime/node.ts (1)
211-243: BridgeCore wiring looks solid.Centralized transport + lifecycle hooks keep stdout/stderr and teardown behavior consistent.
docs/runtimes/nodejs.md (1)
14-21: Docs additions are clear and helpful.Bridge selection guidance and the request size cap explanation match the new behavior well.
Also applies to: 233-242
docs/api/README.md (1)
70-72: Good API clarification.The experimental status note helps prevent accidental reliance on non‑public exports.
src/runtime/optimized-node.ts (1)
443-458: BridgeCore worker wiring is clean.Good use of
onFatalError/onTimeoutto quarantine unhealthy workers and keep the pool stable.test/bridge-core.test.ts (1)
14-66: Solid guardrail coverage.These tests directly validate the new env normalization and protocol/line‑length safeguards.
test/fixtures/missing_id_bridge.py (1)
1-41: LGTM! Test fixture correctly simulates missing ID responses.The fixture intentionally omits the
idfield in non-meta responses (line 35) to test protocol resilience. The broadExceptioncatch on line 30 is acceptable here since this is a test fixture designed to be robust against malformed input.test/fixtures/string_id_bridge.py (1)
1-41: LGTM! Correctly simulates string ID responses for protocol testing.The fixture converts numeric IDs to strings (line 35) to test the bridge's handling of non-integer ID types.
test/optimized-node.test.ts (2)
18-32: LGTM! Well-implemented polling helper.The
waitForutility correctly implements async polling with configurable timeout and interval. The error message on timeout is clear.
493-499: LGTM! Precise assertions for timeout behavior.Using exact equality (
toBe) instead oftoBeGreaterThanOrEqualcorrectly validates that exactly one worker death occurs after timeout, ensuring no double-kills or missed cleanups.test/fixtures/wrong_protocol_bridge.py (1)
32-35: LGTM! Correctly simulates protocol version mismatch.The fixture returns
"tywrap/0"for non-meta messages while using the correct protocol for meta responses. This allows the handshake to succeed but triggers protocol error handling on subsequent calls.test/fixtures/unexpected_id_bridge.py (1)
34-40: LGTM! Correctly simulates unexpected ID responses.The fixture returns
id + 1instead of the original request ID, testing the bridge's handling of ID mismatches. The innertry/except(lines 36-39) properly handles non-integer IDs.test/adversarial_playground.test.ts (10)
1-16: LGTM!Clean imports and sensible test gating. The conditional
describeAdversarialpattern properly isolates these tests behind theTYWRAP_ADVERSARIALflag, preventing accidental execution in standard test runs.
18-29: LGTM!The Python resolution fallback chain is sensible: explicit env var → project resolver → null. Error swallowing is appropriate here since
pythonAvailable()performs the actual validation.
31-40: LGTM!Straightforward helper implementations. The
spawnSyncavailability check is reliable, andbuildPythonPathcorrectly preserves existingPYTHONPATHentries.
64-82: LGTM!Clean fixture bridge factory following the same pattern as
createBridge. The emptyenvdefault is appropriate since fixture scripts don't need the adversarial module path.
84-139: Well-documented known limitation withit.fails.Good use of
it.failsto document the serial bridge concurrency limitation. The comments clearly explain why the fast call may also timeout when a slow call precedes it.
141-178: LGTM!Good coverage of payload size guardrails. The env-based limit injection is clean, and the flexible regex patterns accommodate variations in error message formatting.
180-249: LGTM!Appropriate use of type casting to bypass TypeScript for adversarial protocol validation. These tests verify that the Python bridge correctly rejects malformed payloads at the protocol level.
362-415: LGTM!Solid coverage of Python exception surfacing, crash recovery, and module import errors. The recovery test after
crash_processvalidates the bridge's resilience to unexpected subprocess termination.
417-469: LGTM!Clean table-driven test pattern for decoder validation. The explicit
patternmatchers make failure diagnosis straightforward.
471-550: LGTM!Excellent protocol resilience coverage. The fixture-based approach for testing contract violations (wrong protocol, missing/invalid IDs, invalid JSON) provides clear separation between test logic and malicious bridge behavior. The fragmented frame and out-of-order response tests validate robustness of the JSONL framing implementation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Addressed review notes:
Tests:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@codex please review incrementally |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6daa5704f
ℹ️ 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/configuration.md (1)
291-319: Fix markdownlint blank-line rules around the env vars fence.Markdownlint flags MD031/MD022 in this block. Add blank lines around the fenced code block (and before the next heading) to satisfy the linter.
🧹 Suggested markdown spacing fix
Override configuration with environment variables: ```bash # Runtime configuration export TYWRAP_PYTHON_PATH="/usr/local/bin/python3.11" @@ export TYWRAP_CODEC_PERF_MEMORY_BUDGET_MB="32" # Development export TYWRAP_VERBOSE="true" export TYWRAP_HOT_RELOAD="true"Advanced Configuration Patterns
</details> </blockquote></details> <details> <summary>src/runtime/node.ts (1)</summary><blockquote> `197-233`: **Validate `maxLineLength` to avoid zero/NaN configs.** If callers pass `0`, negative, or `NaN`, BridgeCore will treat it literally and immediately trigger protocol errors. Consider sanitizing to a positive finite value and falling back to env/default. <details> <summary>🛠️ Suggested guard</summary> ```diff - const env = this.buildEnv(); - const maxLineLength = this.options.maxLineLength ?? getMaxLineLengthFromEnv(env); + const env = this.buildEnv(); + const configuredMaxLineLength = + this.options.maxLineLength ?? getMaxLineLengthFromEnv(env); + const maxLineLength = + typeof configuredMaxLineLength === 'number' && configuredMaxLineLength > 0 + ? configuredMaxLineLength + : undefined;src/runtime/optimized-node.ts (1)
424-482: ValidatemaxLineLengthbefore passing to BridgeCore.Same risk as NodeBridge: zero/negative/NaN values will immediately trip protocol errors. Sanitizing avoids surprising failures.
🛠️ Suggested guard
- env = normalizeEnv(env, {}); - const maxLineLength = this.options.maxLineLength ?? getMaxLineLengthFromEnv(env); + env = normalizeEnv(env, {}); + const configuredMaxLineLength = + this.options.maxLineLength ?? getMaxLineLengthFromEnv(env); + const maxLineLength = + typeof configuredMaxLineLength === 'number' && configuredMaxLineLength > 0 + ? configuredMaxLineLength + : undefined;
🤖 Fix all issues with AI agents
In `@src/runtime/bridge-core.ts`:
- Around line 44-50: handleStderrData is currently appending raw stderr bytes
into the bounded tail (DEFAULT_MAX_STDERR_BYTES) and must sanitize output first;
update the logic in handleStderrData to run incoming stderr chunks through a
sanitizer that strips ANSI escape sequences and non‑printable/control characters
(allowing printable UTF‑8 and sensible whitespace like newline/tab), then append
the sanitized string to the stderr buffer and trim to DEFAULT_MAX_STDERR_BYTES;
ensure the same sanitization is applied in the other occurrence noted (lines
~183–194) so only cleaned, printable text is stored for diagnostics.
- Around line 318-325: validateBridgeInfo currently assumes its input is a
BridgeInfo object and will throw a TypeError if the Python subprocess returns
null or a non-object; add a runtime type guard at the start of
validateBridgeInfo to first verify that info is a non-null object and that
typeof info.protocol, info.protocolVersion, and info.bridge are strings (or
otherwise present) before accessing them, and if the check fails throw
BridgeProtocolError; keep the existing protocol/protocolVersion/bridge value
comparisons (TYWRAP_PROTOCOL, TYWRAP_PROTOCOL_VERSION, 'python-subprocess')
after the guard so all malformed payloads result in BridgeProtocolError instead
of a TypeError.
♻️ Duplicate comments (2)
test/optimized-node.test.ts (2)
400-403: Consolidate the tworejectsassertions into one.Chaining two
await expect(promise).rejectson the same promise is brittle; a single assertion is clearer.🔧 Suggested consolidation
- await expect(promise).rejects.toBeInstanceOf(BridgeProtocolError); - await expect(promise).rejects.toThrow(/Failed to serialize request/); + await expect(promise).rejects.toSatisfy( + (err: Error) => + err instanceof BridgeProtocolError && + /Failed to serialize request/.test(err.message) + );
417-423: Add a note for the privateprocessPoolaccess in the test.This is intentional coupling to internals; a short comment will prevent future confusion.
📝 Suggested note
- const pool = (bridge as unknown as { processPool: Array<{ id: string; process: any }> }) - .processPool; + // NOTE: Accessing internal processPool to simulate worker failure in integration tests. + const pool = (bridge as unknown as { processPool: Array<{ id: string; process: any }> }) + .processPool;
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
README.mddocs/api/README.mddocs/configuration.mddocs/runtimes/nodejs.mdsrc/runtime/bridge-core.tssrc/runtime/node.tssrc/runtime/optimized-node.tstest/optimized-node.test.tstest/runtime_bridge_fixtures.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
test/optimized-node.test.ts (2)
src/utils/runtime.ts (1)
isNodejs(146-148)src/runtime/optimized-node.ts (1)
OptimizedNodeBridge(131-763)
src/runtime/node.ts (1)
src/runtime/bridge-core.ts (7)
RpcRequest(9-14)getMaxLineLengthFromEnv(356-366)normalizeEnv(368-405)getPathKey(327-335)ensurePythonEncoding(337-348)ensureJsonFallback(350-354)validateBridgeInfo(318-325)
src/runtime/optimized-node.ts (5)
src/runtime/bridge-core.ts (5)
BridgeCore(52-316)ensurePythonEncoding(337-348)getPathKey(327-335)ensureJsonFallback(350-354)getMaxLineLengthFromEnv(356-366)src/utils/runtime.ts (1)
resolve(302-320)src/utils/cache.ts (1)
globalCache(634-634)src/utils/logger.ts (2)
error(174-176)log(141-160)src/utils/codec.ts (1)
decodeValueAsync(481-483)
src/runtime/bridge-core.ts (1)
src/runtime/protocol.ts (2)
TYWRAP_PROTOCOL(1-1)TYWRAP_PROTOCOL_VERSION(2-2)
🪛 markdownlint-cli2 (0.18.1)
docs/configuration.md
304-304: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
311-311: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (27)
docs/api/README.md (1)
67-95: Doc update matches the new bridge status and options.test/runtime_bridge_fixtures.test.ts (2)
12-25: Python detection helper is solid and portable.
27-83: Fixture parity tests and cleanup look good.README.md (1)
52-74: Runtime bridge guidance is clear and helpful.docs/runtimes/nodejs.md (2)
14-22: Bridge selection section is concise and accurate.
84-251: Advanced options and payload/request limits are documented well.test/optimized-node.test.ts (1)
11-32: Helper utilities are straightforward and platform-safe.src/runtime/node.ts (5)
16-106: BridgeCore integration and option surfacing look good.Exposing
maxLineLengthandinheritProcessEnvwhile wiring the constructor to BridgeCore keeps the public API aligned with the new guardrails.
173-187: Dispose/send flow is consistent with the new core.Guarding sends on missing core and clearing on dispose looks correct.
261-300: Env assembly is solid.The inherit/full-env toggle plus venv PATH injection and encoding/json-fallback normalization looks correct.
302-317: ResetProcess cleanup is thorough.Clearing core state, terminating the child, and resetting init/bridgeInfo keeps the bridge consistent across failures.
319-323: Bridge info validation is a good guardrail.src/runtime/optimized-node.ts (7)
18-175: Options & worker metadata updates align well with BridgeCore.The new
maxLineLength/enableCacheoptions and workercore/quarantinedfields make the pool ready for the shared core flow.
210-219: Cache hits now update aggregate stats — nice.
324-338: Worker selection correctly skips quarantined/exited processes.
367-390: Per-worker stats aroundcore.sendlook correct.
392-414: Quarantine flow is clear and self-healing.
500-525: Lifecycle cleanup and termination paths are consistent.Routing stdout/stderr through BridgeCore, clearing core on exit, and force termination during dispose all look coherent.
Also applies to: 530-551, 688-704, 752-756
573-579: Safe cache keying and pending-count stats are helpful.Also applies to: 632-652
src/runtime/bridge-core.ts (8)
7-98: Core types and initialization look solid.Option defaulting, pending tracking, and buffer resets are well structured.
100-143: Request send path is robust.Serialization failures and transport write errors are handled cleanly with timeouts wired in.
145-181: Stdout framing and max-line enforcement look correct.
196-209: Process exit/error propagation is appropriate.
211-268: Response parsing and decode error wrapping look good.
270-291: Pending cleanup helpers are solid.
293-315: Protocol error handling is clear and centralized.
327-414: Env helpers and normalization utilities look solid.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Addressed the latest CodeRabbit items:
Tests:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/runtime/bridge-core.ts`:
- Around line 51-54: Replace the regex literals used by ANSI_ESCAPE_RE and
CONTROL_CHARS_RE with RegExp constructors using string-escaped sequences (e.g.,
"\\u001b" and "\\u0000-\\u001F") to satisfy Biome's noControlCharactersInRegex
rule; update the CONTROL_CHARS_RE pattern to also include the C1 control range
"\\u0080-\\u009F" so sanitizeStderr covers both C0 and C1 control characters;
keep sanitizeStderr implementation the same but point it at the newly
constructed RegExp instances.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/runtime/bridge-core.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/runtime/bridge-core.ts (2)
src/runtime/timed-out-request-tracker.ts (1)
TimedOutRequestTracker(10-58)src/runtime/protocol.ts (2)
TYWRAP_PROTOCOL(1-1)TYWRAP_PROTOCOL_VERSION(2-2)
🪛 Biome (2.1.2)
src/runtime/bridge-core.ts
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (1)
src/runtime/bridge-core.ts (1)
322-335: Good runtime guard on bridge info.This prevents
TypeErroron malformed payloads and keeps errors consistent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/runtime/bridge-core.ts`:
- Around line 108-110: The current construction of message allows payload to
override id/protocol because it spreads payload after setting id and protocol;
change the order so payload is spread first and then id and protocol are
assigned (i.e., build RpcRequest with ...payload first, then id: this.nextId++
and protocol: this.options.protocol) so id and protocol in the message object
are authoritative; update the code around the message variable and ensure types
still satisfy RpcRequest (message: RpcRequest = { ...payload, id, protocol } as
RpcRequest).
- Around line 218-236: In handleResponseLine, add a runtime shape guard after
parsing to ensure msg is a non-null object before accessing properties; if
JSON.parse returns null or a primitive, call this.handleProtocolError with a
message like "Invalid JSON RPC response type" (including the raw line) and
return. Specifically update the parsed value check around the local variable msg
in the handleResponseLine method to verify typeof msg === 'object' && msg !==
null (and optionally Array.isArray(msg) === false) before reading msg.protocol,
then proceed with the existing protocol and id checks using
this.options.protocol and handleProtocolError.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/runtime/bridge-core.ts
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
CodeRabbit notes we’re declining (intentional test design):
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/runtime/bridge-core.ts`:
- Around line 375-385: The current getMaxLineLengthFromEnv function uses
Number.parseInt which accepts trailing non-digit characters (e.g., "1024abc") —
tighten validation by ensuring the env string is purely numeric before parsing:
check raw against a digit-only pattern (e.g., /^\d+$/) or validate that parsing
consumes the whole string, then convert to a number and keep the existing
Number.isFinite/positive checks; update getMaxLineLengthFromEnv to return
undefined for any non‑pure numeric or malformed TYWRAP_CODEC_MAX_BYTES values.
- Around line 260-263: When handling an incoming error (the block checking
msg.error) validate the shape before passing to errorFrom: ensure msg.error is
an object with string properties type and message (optional traceback as
string); if validation fails, construct a safe fallback like { type:
"UnknownError", message: String(msg.error) } and pass that to errorFrom, then
call pending.reject with the resulting Error. Update the logic around msg.error,
errorFrom, and pending.reject to perform this defensive check and fallback so
malformed payloads don't produce "undefined: undefined" errors.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/runtime/bridge-core.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:22.461Z
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.
📚 Learning: 2026-01-19T21:00:22.461Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:22.461Z
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:
src/runtime/bridge-core.ts
🧬 Code graph analysis (1)
src/runtime/bridge-core.ts (2)
src/runtime/timed-out-request-tracker.ts (1)
TimedOutRequestTracker(10-58)src/runtime/protocol.ts (2)
TYWRAP_PROTOCOL(1-1)TYWRAP_PROTOCOL_VERSION(2-2)
🔇 Additional comments (9)
src/runtime/bridge-core.ts (9)
1-57: LGTM: Type definitions and sanitization utilities are well-structured.The type definitions provide clear contracts for the RPC protocol. The sanitizer correctly handles both ANSI escape sequences and control characters (C0 and C1 ranges), meeting the PR's guardrail requirements for safe stderr embedding.
59-84: LGTM: Constructor properly initializes defaults and state.Options normalization with nullish coalescing provides clean defaults. The
TimedOutRequestTrackerTTL calculation (max(1000, timeoutMs * 2)) appropriately handles late-arriving responses without unbounded memory growth.
86-105: LGTM: Clean accessor and reset methods.The
clear()method properly clears timeout timers before removing pending entries, preventing potential timer-related memory leaks. ResettingprotocolErrorenables bridge recovery scenarios.
107-150: LGTM: Robust request lifecycle management.The
sendmethod correctly:
- Prevents payload from overriding
id/protocolvia spread order- Wraps serialization failures in
BridgeProtocolError- Manages timeout with cleanup and stderr diagnostics
- Handles IPC failures with proper request cleanup and fatal error propagation
152-201: LGTM: Properly bounded I/O handling with sanitization.The stdout handler implements two-level bounds checking:
- Pre-newline buffer overflow detection (Lines 155-166)
- Per-line size validation (Lines 177-184)
The stderr handler correctly sanitizes content before buffering and maintains a bounded tail, meeting the PR objective for safe error message embedding.
203-216: LGTM: Correct process termination handling.Both handlers properly reject all pending requests with descriptive errors including stderr context.
handleProcessErroradditionally triggers the fatal error callback for upstream recovery handling.
276-327: LGTM: Robust error handling infrastructure.Key strengths:
handleProtocolErrordebounces viaprotocolErrorflag, preventing cascading errors- Error messages include actionable hints for common issues (stdout pollution)
handleFatalErrorsafely wraps callback invocation- Line snippets are truncated to 500 chars to prevent log flooding
330-344: LGTM: Runtime type guard properly validates bridge info.The function correctly handles malformed payloads by checking for null/non-object before accessing properties, preventing
TypeErrorand ensuring consistentBridgeProtocolErrorhandling.
387-433: LGTM: Cross-platform environment normalization.The
normalizeEnvfunction correctly handles Windows PATH casing issues by:
- Preserving the base environment's key casing
- Using override values when present
- Dropping undefined values to avoid
spawnsurprisesThe
eslint-disablecomments forsecurity/detect-object-injectionare appropriate since dynamic env key access is intentional.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Remaining nitpicks we’re not changing:
Policy: adversarial fixtures/tests preserve failure modes and prioritize resilience validation, even if structure is atypical. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
bbopen
force-pushed
the
feat/adversarial-playground
branch
from
8e6e29e to
0e68e0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
runtime/python_bridge.py (1)
805-853: Preserve request id when rejecting oversized requests.The size check happens before JSON parsing, so
midremainsNoneonRequestTooLargeError, yieldingid: -1. The JS bridge treats unmatched ids as protocol errors, which can tear down the session instead of returning a per-callRequestTooLargeError. Extract the id before raising to keep the response correlated.🔧 Suggested fix (lightweight id extraction)
+def extract_request_id(line: str): + try: + key = '"id"' + start = line.find(key) + if start == -1: + return None + colon = line.find(':', start + len(key)) + if colon == -1: + return None + idx = colon + 1 + while idx < len(line) and line[idx].isspace(): + idx += 1 + end = idx + while end < len(line) and line[end].isdigit(): + end += 1 + if end == idx: + return None + return int(line[idx:end]) + except Exception: + return None + def main(): for line in sys.stdin: line = line.strip() if not line: continue mid = None out = None try: if REQUEST_MAX_BYTES is not None: payload_bytes = len(line.encode('utf-8')) if payload_bytes > REQUEST_MAX_BYTES: + mid = extract_request_id(line) raise RequestTooLargeError(payload_bytes, REQUEST_MAX_BYTES) msg = json.loads(line)
🤖 Fix all issues with AI agents
In `@ROADMAP.md`:
- Line 3: The heading "## Runtime Bridges" in ROADMAP.md lacks surrounding blank
lines; update the file so there is one blank line before and one blank line
after the "## Runtime Bridges" heading to satisfy markdownlint MD022 and ensure
proper spacing.
In `@runtime/python_bridge.py`:
- Around line 99-123: get_request_max_bytes currently fails on values with
trailing characters (e.g. "1024abc"); change it to be tolerant by extracting a
leading numeric prefix instead of strict int(raw). In get_request_max_bytes,
after stripping raw, verify it begins with a digit, use a regex or scanning to
capture the leading digits (group 1), convert that substring to int, raise
RequestMaxBytesParseError if there is no leading digit or parsing fails, and
return None for non-positive values; keep the startup cached REQUEST_MAX_BYTES
assignment as-is.
In `@test/adversarial_playground.test.ts`:
- Around line 339-357: Add a short inline comment above the recovery assertions
in the finally block of the "includes recent stderr in timeout errors" test to
explain why we validate post-timeout behavior there (we purposely keep recovery
checks adjacent to cleanup to ensure the bridge recovers and isn't removed by
refactors); reference the surrounding calls so it's obvious what to document:
callAdversarial('echo', ['post-timeout']), delay(600), and bridge.dispose(), and
mention that these assertions confirm the bridge can still respond after the
timeout induced by callAdversarial('write_stderr_then_sleep', ...).
In `@test/fixtures/missing_id_bridge.py`:
- Around line 23-36: In main(), document the intentional missing "id" in
non-meta responses by adding a brief inline comment near the branch that
constructs out (the else that sets out = {"protocol": PROTOCOL, "result": 1})
explaining this fixture purposely omits the id to simulate an
adversarial/malformed response for testing; reference the msg variable and the
meta branch (msg.get("method") == "meta") so reviewers understand only meta
replies include id via msg.get("id", -1).
In `@test/fixtures/out_of_order_bridge.py`:
- Around line 29-48: The test fixture's main() intentionally leaves the variable
pending set when stdin ends to simulate an adversarial "pending at EOF"
behavior; add a short clarifying comment above the pending/EOF behavior (near
the pending variable and where pending is left unset at the end of the loop)
explaining that leaving pending set at exit is intentional and used to test
out-of-order/unfinished requests, referencing the pending variable and the
send_response calls so future maintainers don't normalize this failure mode.
In `@test/fixtures/unexpected_id_bridge.py`:
- Around line 35-40: Add a concise inline comment in the fixture explaining that
the returned "id" is intentionally skewed to trigger the "unexpected id" error
path: annotate the block that computes req_id, next_id and constructs out
(variables req_id, next_id, out and constant PROTOCOL) with a one-line comment
like "Intentionally return a different id to provoke the ‘unexpected id’ error
path in tests" so future contributors don't normalize the behavior.
In `@test/runtime_bridge_fixtures.test.ts`:
- Around line 59-66: Replace the silent early returns with explicit test skips
so missing Python or fixture files are reported as skipped instead of passing;
locate the test creating NodeBridge (the it(`NodeBridge handles fixture
${fixture.script}`...) block) and change the checks that use pythonPath and
existsSync(scriptPath) to call test.skip / it.skip with a clear message (e.g.,
"skipped: no pythonPath" or "skipped: fixture missing") before constructing
NodeBridge or calling nodeBridge.call, referencing NodeBridge, nodeBridge.call,
fixture.script, pythonPath, scriptPath and existsSync to find where to insert
the skips.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/ci.ymlREADME.mdROADMAP.mddocs/api/README.mddocs/configuration.mddocs/runtimes/nodejs.mdruntime/python_bridge.pysrc/config/index.tssrc/runtime/bridge-core.tssrc/runtime/node.tssrc/runtime/optimized-node.tssrc/utils/codec.tssrc/utils/logger.tssrc/utils/python.tstest/adversarial_playground.test.tstest/bridge-core.test.tstest/codec-performance.test.tstest/fixtures/missing_id_bridge.pytest/fixtures/out_of_order_bridge.pytest/fixtures/oversized_line_bridge.pytest/fixtures/python/adversarial_module.pytest/fixtures/string_id_bridge.pytest/fixtures/unexpected_id_bridge.pytest/fixtures/wrong_protocol_bridge.pytest/optimized-node.test.tstest/performance-budgets.test.tstest/runtime_bridge_fixtures.test.ts
💤 Files with no reviewable changes (1)
- src/utils/python.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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 (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: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
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: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:22.461Z
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.
📚 Learning: 2026-01-19T21:00:22.461Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:22.461Z
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/bridge-core.test.tstest/runtime_bridge_fixtures.test.tstest/adversarial_playground.test.tstest/codec-performance.test.tstest/optimized-node.test.tstest/fixtures/out_of_order_bridge.pysrc/runtime/bridge-core.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 (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/bridge-core.test.tsROADMAP.mdtest/runtime_bridge_fixtures.test.tstest/adversarial_playground.test.tsREADME.mdtest/optimized-node.test.tstest/fixtures/wrong_protocol_bridge.pytest/fixtures/missing_id_bridge.pydocs/runtimes/nodejs.mdruntime/python_bridge.pytest/fixtures/out_of_order_bridge.py
📚 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 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/bridge-core.test.tstest/adversarial_playground.test.tstest/codec-performance.test.tsREADME.mddocs/configuration.mddocs/runtimes/nodejs.md
📚 Learning: 2026-01-19T21:00:35.449Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:35.449Z
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_bridge_fixtures.test.tstest/adversarial_playground.test.tstest/optimized-node.test.tstest/fixtures/wrong_protocol_bridge.pytest/fixtures/missing_id_bridge.pytest/fixtures/string_id_bridge.pytest/fixtures/unexpected_id_bridge.pytest/fixtures/out_of_order_bridge.pytest/fixtures/oversized_line_bridge.pysrc/runtime/bridge-core.ts
📚 Learning: 2026-01-19T21:00:35.449Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:35.449Z
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_bridge_fixtures.test.tstest/adversarial_playground.test.tssrc/runtime/bridge-core.ts
📚 Learning: 2026-01-19T21:00:35.449Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:35.449Z
Learning: Maintain adversarial test fixtures by preserving the failure mode they exercise, even if that behavior would be considered bad in production code. When reviewing test fixtures with unusual patterns (e.g., missing error handling, protocol violations, unanswered requests), require clarifying comments that explain the intent rather than suggesting normalization or fixes. This helps ensure tests accurately validate failure modes and that intent is documented for future maintenance.
Applied to files:
test/fixtures/wrong_protocol_bridge.pytest/fixtures/missing_id_bridge.pytest/fixtures/string_id_bridge.pytest/fixtures/unexpected_id_bridge.pytest/fixtures/out_of_order_bridge.pytest/fixtures/python/adversarial_module.pytest/fixtures/oversized_line_bridge.py
📚 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/node.tssrc/runtime/optimized-node.tssrc/runtime/bridge-core.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/node.tssrc/runtime/optimized-node.tssrc/runtime/bridge-core.ts
🧬 Code graph analysis (12)
test/bridge-core.test.ts (1)
src/runtime/bridge-core.ts (2)
normalizeEnv(387-424)BridgeCore(59-328)
test/runtime_bridge_fixtures.test.ts (2)
tools/matrix.js (1)
candidates(38-40)src/utils/runtime.ts (1)
getPythonExecutableName(568-570)
test/adversarial_playground.test.ts (1)
src/runtime/node.ts (1)
NodeBridge(81-324)
test/optimized-node.test.ts (1)
src/utils/runtime.ts (1)
isNodejs(146-148)
test/fixtures/wrong_protocol_bridge.py (2)
test/fixtures/missing_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/string_id_bridge.py (2)
meta_payload(10-20)main(23-37)
test/fixtures/missing_id_bridge.py (3)
test/fixtures/string_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/unexpected_id_bridge.py (2)
meta_payload(10-20)main(23-42)test/fixtures/wrong_protocol_bridge.py (2)
meta_payload(10-20)main(23-37)
test/fixtures/string_id_bridge.py (1)
test/fixtures/missing_id_bridge.py (2)
meta_payload(10-20)main(23-37)
src/runtime/node.ts (4)
src/runtime/bridge-core.ts (6)
RpcRequest(9-14)getMaxLineLengthFromEnv(375-385)getPathKey(346-354)ensurePythonEncoding(356-367)ensureJsonFallback(369-373)validateBridgeInfo(330-344)src/index.ts (3)
BridgeProtocolError(15-15)decodeValueAsync(71-71)BridgeInfo(55-55)src/utils/codec.ts (1)
decodeValueAsync(481-483)src/types/index.ts (1)
BridgeInfo(312-324)
test/fixtures/unexpected_id_bridge.py (2)
test/fixtures/missing_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/string_id_bridge.py (2)
meta_payload(10-20)main(23-37)
test/fixtures/out_of_order_bridge.py (2)
test/fixtures/missing_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/wrong_protocol_bridge.py (2)
meta_payload(10-20)main(23-37)
test/fixtures/oversized_line_bridge.py (2)
runtime/python_bridge.py (1)
main(805-853)test/bridge-core.test.ts (1)
write(9-11)
src/runtime/bridge-core.ts (2)
src/runtime/timed-out-request-tracker.ts (1)
TimedOutRequestTracker(10-58)src/runtime/protocol.ts (2)
TYWRAP_PROTOCOL(1-1)TYWRAP_PROTOCOL_VERSION(2-2)
🪛 markdownlint-cli2 (0.18.1)
ROADMAP.md
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 Ruff (0.14.13)
test/fixtures/wrong_protocol_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
test/fixtures/missing_id_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
test/fixtures/string_id_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
test/fixtures/unexpected_id_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
38-38: Do not catch blind exception: Exception
(BLE001)
runtime/python_bridge.py
145-147: try-except-pass detected, consider logging the exception
(S110)
145-145: Do not catch blind exception: Exception
(BLE001)
687-687: Avoid specifying long messages outside the exception class
(TRY003)
705-705: Avoid specifying long messages outside the exception class
(TRY003)
718-718: Avoid specifying long messages outside the exception class
(TRY003)
731-731: Avoid specifying long messages outside the exception class
(TRY003)
757-757: Avoid specifying long messages outside the exception class
(TRY003)
800-800: Consider moving this statement to an else block
(TRY300)
816-816: Abstract raise to an inner function
(TRY301)
852-852: Do not catch blind exception: Exception
(BLE001)
test/fixtures/out_of_order_bridge.py
37-37: Do not catch blind exception: Exception
(BLE001)
test/fixtures/python/adversarial_module.py
15-15: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
15-15: Dynamically typed expressions (typing.Any) are disallowed in echo
(ANN401)
23-23: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
23-23: Dynamically typed expressions (typing.Any) are disallowed in sleep_and_return
(ANN401)
40-40: Dynamically typed expressions (typing.Any) are disallowed in return_unserializable
(ANN401)
66-66: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
66-66: Dynamically typed expressions (typing.Any) are disallowed in print_to_stdout
(ANN401)
75-75: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
75-75: Dynamically typed expressions (typing.Any) are disallowed in write_to_stderr
(ANN401)
🔇 Additional comments (72)
src/utils/logger.ts (1)
133-138: LGTM: scoped lint suppression is reasonable here.
The indexed access is constrained byLogLevel, so the suppression stays safe and minimal..github/workflows/ci.yml (1)
161-167: LGTM: codec-suite trigger expansion is consistent with broader CI coverage.src/config/index.ts (1)
315-318: LGTM: lint suppression is appropriately scoped.test/performance-budgets.test.ts (1)
1-15: LGTM: explicit perf-budget gating is clear.test/codec-performance.test.ts (1)
1-85: LGTM: codec performance budget test is well-scoped.test/fixtures/unexpected_id_bridge.py (1)
10-20: Meta payload aligns with existing fixtures.Matches the standard bridge metadata shape used by other fixtures.
test/adversarial_playground.test.ts (10)
18-87: Helper setup is clean and deterministic.Resolution + availability checks, fixture path wiring, and bridge creation look solid for adversarial gating.
89-109: Timeout recovery coverage is strong.The test asserts timeout behavior and verifies the bridge remains usable afterward.
112-139: Concurrency edge-case coverage looks solid.This captures the serial-bridge limitation and verifies recovery after the timeout.
141-178: Payload limit enforcement tests are clear.Good coverage for both response-size and request-size caps.
180-247: Input-shape validation tests are comprehensive.Covers invalid args/kwargs and missing module/function names cleanly.
251-299: Serialization failure handling is well covered.Non-serializable, circular, and invalid JSON payload paths are asserted explicitly.
302-337: Stdout/stderr noise resilience is well exercised.Good differentiation between stdout protocol errors and stderr tolerance.
362-415: Process-exit and exception propagation coverage is solid.These cases validate that bridge recovery works after hard failures.
417-468: Decoder validation matrix looks thorough.The malformed-envelope cases are enumerated and asserted cleanly.
471-550: Protocol-violation fixture coverage looks good.Fixtures for wrong protocol/id, invalid JSON, and noisy output are exercised appropriately.
runtime/python_bridge.py (5)
57-72: New request-size error types are clear and targeted.Naming and error messages make the failure mode explicit.
129-148: Bounded protocol diagnostics are a good safety valve.Keeps stderr informative without flooding logs or violating JSONL framing.
619-733: Centralized request validation reads well.The helper-based schema checks improve consistency across handlers.
735-758: Dispatch now correctly reports unknown methods.Routing and error surfacing are explicit and consistent.
790-803:write_payloadabstraction is a good reliability win.Centralizing BrokenPipe handling keeps the main loop simpler and safer.
test/fixtures/python/adversarial_module.py (1)
1-219: Fixture helpers are well documented and purpose-built.Clear, deterministic adversarial cases with explicit intent in docstrings.
docs/api/README.md (1)
70-95: Docs correctly reflect the new NodeBridge options and status note.The experimental note and options list match the runtime changes.
test/fixtures/oversized_line_bridge.py (1)
5-13: Fixture is clear and purpose-driven.The inline note makes the oversized payload intent explicit.
README.md (2)
57-57: LGTM!The option name change from
timeouttotimeoutMsimproves clarity by explicitly indicating the unit.
61-74: LGTM!Clear documentation of the bridge architecture, environment handling defaults, and payload size controls. This helps users understand the differences between NodeBridge and OptimizedNodeBridge while providing guidance on environment inheritance and JSONL limits.
test/bridge-core.test.ts (4)
6-12: LGTM!Simple and effective mock transport for capturing BridgeCore writes during tests.
14-36: LGTM!Good coverage of the
normalizeEnvfunction: validates PATH case normalization (Windows-stylePathto override), dropping ofundefinedvalues, and preservation of other environment variables.
38-52: LGTM!Correctly tests that circular references (non-serializable payloads) are rejected with
BridgeProtocolErrorduring the send phase.
54-67: LGTM!Validates that oversized stdout lines are properly rejected. The test correctly initiates a pending request before feeding the oversized data to trigger the protocol error.
test/runtime_bridge_fixtures.test.ts (2)
27-45: LGTM!Good test lifecycle management with
beforeAllfor Python detection andafterEachfor bridge cleanup. The null checks beforedispose()prevent errors if bridge initialization failed.
73-82: LGTM!The pythonPath is now correctly passed to OptimizedNodeBridge, addressing the previous review feedback. The init/call/dispose sequence is properly structured.
docs/configuration.md (1)
299-312: LGTM!Clear documentation of the new environment variables for request size limits and performance budgets. The note about
TYWRAP_CODEC_MAX_BYTESdefaultingmaxLineLengthis helpful for understanding the relationship between these settings.docs/runtimes/nodejs.md (3)
14-22: LGTM!Clear differentiation between NodeBridge (stable, recommended) and OptimizedNodeBridge (experimental), with appropriate caveats about the experimental nature and reference to the roadmap.
94-95: LGTM!The new
maxLineLengthandinheritProcessEnvoptions are well-documented with accurate defaults and clear descriptions. The note about minimal subprocess environment is helpful for security-conscious deployments.Also applies to: 115-122
239-250: LGTM!The Request Size Limit section follows the same pattern as Payload Size Limit, maintaining consistency in the documentation. Clear explanation of the environment variable and its effect.
src/utils/codec.ts (3)
174-182: LGTM!Clean simplification of the async wrapper. The require-with-fallback-to-import pattern for apache-arrow loading is preserved.
443-453: LGTM!Good addition of runtime validation for the
versionfield. The check correctly allowsundefinedwhile rejecting non-string values, ensuring type safety for theSklearnEstimatorinterface.
431-441: LGTM!Multiline formatting improves readability of error messages without changing behavior.
test/fixtures/wrong_protocol_bridge.py (1)
1-41: LGTM! Clear adversarial fixture for protocol mismatch testing.The fixture correctly simulates a bridge that returns the correct protocol (
tywrap/1) formetarequests but an incorrect protocol (tywrap/0) for all other messages. This is the exact failure mode needed to test protocol validation in BridgeCore.The broad
except Exceptionat line 30 is acceptable here—test fixtures should gracefully handle any malformed input to focus on the specific protocol behavior being tested. Based on learnings, adversarial fixtures should preserve their failure modes rather than add defensive handling.test/optimized-node.test.ts (5)
18-32: Well-implemented polling utility with sensible defaults.The
waitForhelper is clean and provides reasonable defaults (1500ms timeout, 25ms interval). The async polling pattern is appropriate for waiting on process lifecycle changes in tests.
386-407: Good regression test for serialization error handling.The test correctly verifies that BigInt (which can't be JSON serialized) surfaces as a
BridgeProtocolErrorwith the expected message pattern. The consolidatedtoSatisfyassertion addresses the previous review feedback about chained rejects.
409-440: Effective test for stdin failure handling.The test properly exercises the worker quarantine/replacement path by destroying stdin and verifying both the error and the pool update. The internal field access comment (line 421) addresses the previous review feedback.
442-471: Solid crash recovery test.The test gates appropriately on fixture availability, triggers a crash via the adversarial module, waits for processDeaths to increment, and then verifies recovery by making a successful call. This validates the resilience path effectively.
494-503: Timeout handling assertions correctly updated.The
processDeathsexpectation change from 0 to +1 correctly reflects the new quarantine-on-timeout behavior where timed-out workers are terminated. This aligns with theonTimeoutcallback in BridgeCore triggeringquarantineWorker.test/fixtures/string_id_bridge.py (1)
1-41: LGTM! Clear adversarial fixture for ID type validation.The fixture correctly returns the ID as a string (
str(msg.get("id", "1"))) for non-meta messages while properly handling meta requests. This tests thetypeof msg.id !== 'number'validation in BridgeCore (line 241 of bridge-core.ts).The broad
except Exceptionis acceptable for test fixtures that need to gracefully handle malformed input.src/runtime/node.ts (7)
16-26: Clean import structure for BridgeCore integration.The imports properly bring in the BridgeCore class along with the RPC types and environment utilities needed for the refactored implementation.
34-35: New options properly surfaced.The
maxLineLengthandinheritProcessEnvoptions are correctly added to bothNodeBridgeOptionsandResolvedNodeBridgeOptions, with appropriate defaults (inheritProcessEnv: false). This addresses the previous review feedback about exposing the line length limit.Also applies to: 53-54, 102-103
173-177: Dispose properly notifies core before cleanup.Calling
core.handleProcessExit()beforeresetProcess()ensures pending requests are rejected with proper error messages before the core is cleared.
179-187: Send delegation is clean and correct.The
sendmethod properly guards against disposed state and missing core, then delegates tocore.send(). This centralizes the IPC logic in BridgeCore.
220-235: BridgeCore construction is well-structured.The BridgeCore is initialized with:
- A write callback that guards against missing stdin
- Timeout from options
- maxLineLength from options or environment
- Value decoder for async decoding
- Fatal error callback that triggers process reset
This correctly centralizes the IPC handling while maintaining proper error propagation.
261-300: Environment building logic is correct.The
buildEnvmethod properly:
- Filters process.env based on
inheritProcessEnvsetting- Preserves PATH, PYTHONPATH, VIRTUAL_ENV, PYTHONHOME, and TYWRAP_* vars when not inheriting
- Applies virtual environment PATH adjustments
- Ensures Python encoding settings
- Applies JSON fallback configuration
This addresses the previous review about env allowlist behavior by providing the
inheritProcessEnvopt-in.
302-317: Reset process handles cleanup correctly.The
resetProcessmethod:
- Clears core state (timers, pending requests)
- Unsets core reference
- Attempts graceful SIGTERM if process still running
- Clears child/init/bridgeInfo references
The try-catch around kill is appropriate since the process may have already exited.
src/runtime/optimized-node.ts (8)
49-64: Well-defined resolved options interface.The
ResolvedProcessPoolOptionsinterface properly types all the internal options with required fields where appropriate. The newmaxLineLengthandenableCacheoptions are correctly included.
66-80: Worker structure properly extended for BridgeCore.The
quarantinedflag andcorefield are appropriate additions for the new architecture. Quarantine tracking enables safe worker replacement without race conditions.
210-221: Cache handling correctly updates aggregate stats.The cache hit path now calls
this.updateStats(performance.now() - startTime)before returning, addressing the previous review feedback. This ensurestotalRequestsand derived metrics are accurate.
325-327: Worker selection correctly excludes quarantined workers.The filter now checks
!w.quarantinedin addition to!w.busyand exit status, preventing requests from being routed to workers being terminated.
367-390: Worker request handling properly delegates to core.The
sendToWorkermethod:
- Marks worker busy and updates metadata
- Delegates to
worker.core.send()- Updates per-worker stats on success/failure
- Properly clears busy flag in finally block
The timing and stats tracking is appropriate.
392-414: Quarantine logic is robust and handles edge cases.The
quarantineWorkermethod:
- Guards against double-quarantine
- Logs with structured context
- Force-terminates the worker
- Spawns replacement if below minimum (with error handling)
The async/await pattern with
.then()/.catch()is acceptable here since we don't need to block on termination.
449-481: Worker initialization with BridgeCore is well-structured.The temporary
null as unknown as BridgeCoreassignment (line 456) is necessary due to the circular reference (worker needs core, core callbacks need worker). The core is immediately assigned after construction.The callbacks properly:
- Guard stdin writability before writing
- Trigger quarantine on fatal errors and timeouts
573-579: Safe cache key generation handles serialization failures.The
safeCacheKeymethod wrapsglobalCache.generateKeyin a try-catch, returningnullon failure. This prevents cache key generation errors (e.g., from BigInt) from bubbling up as request failures—aligning with the regression test in optimized-node.test.ts.src/runtime/bridge-core.ts (11)
1-6: Clean imports and type dependencies.The imports properly bring in the necessary types and error classes. Using the existing
TimedOutRequestTrackerfor tracking timed-out request IDs is a good reuse of existing code.
7-42: Well-defined type exports for the RPC protocol.The
RpcMethod,RpcRequest,RpcResponse,BridgeCoreTransport, andBridgeCoreOptionstypes provide a clear contract for the IPC protocol. The options interface properly includes hooks for fatal errors and timeouts.
44-57: Stderr sanitization meets guardrail requirements.The implementation:
- Uses
RegExpconstructors to avoid Biome lint errors (addresses past review)- Strips ANSI escape sequences with
ANSI_ESCAPE_RE- Strips C0 and C1 control characters with
CONTROL_CHARS_RE(U+0000-U+001F, U+007F, U+0080-U+009F)This fulfills the PR objective of sanitizing stderr before buffering.
59-84: BridgeCore constructor properly initializes state.The constructor:
- Stores transport reference
- Applies defaults for all optional options
- Initializes
TimedOutRequestTrackerwith TTL based on timeoutThe TTL of
max(1000, timeoutMs * 2)provides a reasonable window for late responses.
107-150: Send implementation is correct and robust.The
sendmethod:
- Generates unique ID and constructs message with
id/protocolafter spreading payload (addresses past review about override prevention)- Wraps JSON.stringify in try-catch for serialization errors
- Sets up timeout with proper cleanup and error notification
- Handles transport write failures with fatal error propagation
The promise lifecycle is correctly managed.
152-188: Stdout handling with line length limits is well-implemented.The
handleStdoutDatamethod:
- Buffers incoming chunks
- Detects oversized lines before newline (prevents unbounded growth)
- Processes complete lines
- Validates line length after extraction
- Delegates to
handleResponseLineThis meets the guardrail requirements for bounded I/O.
190-201: Stderr handling with sanitization and bounding.The method:
- Sanitizes incoming chunks before buffering
- Keeps only the most recent
maxStderrBytes(rolling tail)- Silently ignores buffering errors
This fulfills the objective of safe stderr handling.
218-274: Response line handling is robust.The
handleResponseLinemethod:
- Parses JSON with proper error handling
- Guards against non-object payloads (addresses past review)
- Validates protocol match
- Validates numeric ID type
- Handles pending request lookup with timed-out request tracking
- Properly clears timer before resolving/rejecting
- Uses async decode with error wrapping
Per learnings, no extra defensive validation is added for error payload shape in the hot path.
330-344: validateBridgeInfo includes runtime type guard.The function now:
- Checks for null/non-object input first (addresses past review)
- Validates protocol and version match
- Validates bridge identifier
This prevents TypeError on malformed payloads.
375-385: Environment parsing follows tolerant policy.The
getMaxLineLengthFromEnvfunction accepts leading numeric portions (e.g.,1024abc→ 1024) per the established learning. Only non-numeric starts or non-positive values are rejected.
387-424: Environment normalization handles PATH casing correctly.The
normalizeEnvfunction:
- Finds existing PATH key casing in both base and overrides
- Preserves the correct casing for cross-platform compatibility (Windows uses
Path)- Filters out undefined values
- Properly merges overrides
This is important for Windows compatibility where PATH casing varies.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/optimized-node.ts (1)
32-64: AddinheritProcessEnvoption to match node.ts runtime.The
inheritProcessEnvoption is missing fromProcessPoolOptionsandResolvedProcessPoolOptionsinterfaces, and the env handling at line 424 unconditionally inheritsprocess.envwithout respecting a controlled default. This creates an inconsistency with the standard node.ts runtime (which defaultsinheritProcessEnvtofalse) and risks leaking environment variables.Add the option to both interfaces, default it to
falsein the constructor, and wire it into env selection:Suggested fix
interface ProcessPoolOptions { + inheritProcessEnv?: boolean; minProcesses?: number;In the constructor, add to
this.options:+ inheritProcessEnv: options.inheritProcessEnv ?? false, minProcesses: options.minProcesses ?? 2,At line 424, replace:
- let env = normalizeEnv(process.env as Record<string, string | undefined>, this.options.env); + const baseEnv = this.options.inheritProcessEnv + ? (process.env as Record<string, string | undefined>) + : {}; + let env = normalizeEnv(baseEnv, this.options.env);
🤖 Fix all issues with AI agents
In `@src/runtime/bridge-core.ts`:
- Around line 260-298: The hot path currently calls normalizeErrorPayload for
every error response which adds unnecessary overhead; remove
normalizeErrorPayload and its call, and instead pass the raw msg.error into
errorFrom (e.g., pending.reject(this.errorFrom(msg.error as
{type:string;message:string;traceback?:string}))), then simplify errorFrom to
assume the protocol-provided shape and only minimally coerce/attach traceback
(or gate full validation behind a debug/strict flag). Update/remove the
normalizeErrorPayload method and ensure errorFrom (and its signature) references
remain consistent.
- Around line 393-405: The current getMaxLineLengthFromEnv function rejects
values like "1024abc" due to the strict /^\d+$/ check; update it to accept a
leading numeric prefix by trimming env.TYWRAP_CODEC_MAX_BYTES, matching only the
leading digits (e.g., via /^\s*(\d+)/ or similar), parse that captured prefix
(use parseInt or Number on the match), validate it is finite and > 0, and return
that number; if no leading digits are found or validation fails, return
undefined. Ensure you continue to reference getMaxLineLengthFromEnv and
env.TYWRAP_CODEC_MAX_BYTES when locating the change.
In `@test/fixtures/string_id_bridge.py`:
- Around line 34-35: Add a short inline comment next to the construction of the
response object (the out = {"id": str(msg.get("id", "1")), "protocol": PROTOCOL,
"result": 1} line) clarifying that converting msg.get("id") to a string is
intentional: this fixture purposely returns an id as a string to exercise
type-coercion and adversarial handling in the bridge. Keep the comment concise
and mention the intent to test string-id handling and bridge coercion behavior.
In `@test/fixtures/wrong_protocol_bridge.py`:
- Around line 34-35: Add a brief inline comment next to the branch that sets out
= {"id": msg.get("id", -1), "protocol": "tywrap/0", "result": 1} explaining that
the protocol mismatch is intentional for this adversarial fixture to trigger
protocol validation errors (i.e., non-meta messages should purposely return
"tywrap/0"). Mention that this is deliberate to prevent future contributors from
"fixing" the behavior and reference the variable name out and the
msg["protocol"] value in the comment for clarity.
♻️ Duplicate comments (8)
test/fixtures/out_of_order_bridge.py (1)
42-48: Document the intentional "pending at EOF" behavior.Per learnings, this fixture intentionally leaves a pending request unanswered at EOF to simulate missing/out-of-order responses. Add a clarifying comment to preserve this adversarial intent and prevent accidental normalization.
📝 Suggested inline comment
if pending is None: + # Intentionally allow pending to remain set at EOF to simulate missing responses. pending = msg continuetest/fixtures/missing_id_bridge.py (1)
34-35: Document the intentional missing-id response.Per learnings, adversarial fixtures should have clarifying comments explaining their intent. The
idfield is intentionally omitted for non-meta responses to test bridge handling of missing IDs.📝 Suggested inline comment
else: + # Intentionally omit id to exercise missing-id handling in the bridge. out = {"protocol": PROTOCOL, "result": 1}ROADMAP.md (1)
3-7: Add a blank line after the heading.Markdownlint MD022 requires blank lines around headings. Add a blank line between line 3 and line 4.
📝 Proposed fix
## Runtime Bridges + - [ ] Unify NodeBridge and OptimizedNodeBridge behind a shared JSONL transport core.test/runtime_bridge_fixtures.test.ts (1)
58-66: Consider explicit test skips for better CI visibility.Silent early returns when
pythonPathis null or the fixture doesn't exist cause tests to pass silently, which could mask environment issues in CI. Using Vitest's skip mechanism would make test status more transparent.♻️ Optional improvement
for (const fixture of fixtures) { - it(`NodeBridge handles fixture ${fixture.script}`, async () => { - if (!pythonPath) return; + it(`NodeBridge handles fixture ${fixture.script}`, async ({ skip }) => { + if (!pythonPath) { + skip(); + return; + } const scriptPath = join(process.cwd(), 'test', 'fixtures', fixture.script); - if (!existsSync(scriptPath)) return; + if (!existsSync(scriptPath)) { + skip(); + return; + }test/fixtures/unexpected_id_bridge.py (1)
34-40: Document the intentional ID skew in this fixture.This fixture intentionally responds with
next_id = int(req_id) + 1to trigger the "unexpected id" error path. A brief inline comment will prevent future "cleanup" from breaking the failure mode. Based on learnings, adversarial fixtures should document their intent.📝 Suggested inline comment
else: + # Intentional: respond with a different id to trigger unexpected-id handling. req_id = msg.get("id", 0) try: next_id = int(req_id) + 1 except Exception: next_id = 1 out = {"id": next_id, "protocol": PROTOCOL, "result": 1}test/adversarial_playground.test.ts (1)
339-360: Add a brief note explaining the recovery assertions infinally.Per learnings, this test intentionally keeps recovery validation adjacent to cleanup to verify resilience under stress. Adding a brief inline comment will prevent future refactors from "fixing" it away.
📝 Suggested inline comment
} finally { + // Why: adversarial test verifies post-timeout recovery even if it masks the original error. await delay(600); const result = await callAdversarial(bridge, 'echo', ['post-timeout']); expect(result).toBe('post-timeout'); await bridge.dispose(); }runtime/python_bridge.py (2)
99-118: MakeTYWRAP_REQUEST_MAX_BYTESparsing tolerant of numeric prefixes.
int(raw)rejects values like1024abc, which currently raises at startup. The repo policy is best‑effort numeric prefix parsing.🛠️ Suggested tolerant parsing
- try: - value = int(raw) - except Exception as exc: - raise RequestMaxBytesParseError() from exc + idx = 0 + while idx < len(raw) and raw[idx].isdigit(): + idx += 1 + if idx == 0: + raise RequestMaxBytesParseError() + value = int(raw[:idx])Based on learnings, align env parsing with the tolerant prefix policy.
813-835: Preserve request id when rejecting oversized payloads.
The size check runs beforejson.loads, somidstaysNoneand the error payload usesid: -1. On the JS side this is treated as a protocol error and can tear down the worker, rather than returning a per‑callRequestTooLargeError. Consider extractingidfirst (even a minimal parse) or enforcing the limit client‑side.🛠️ Possible fix (parse id before size check)
- if REQUEST_MAX_BYTES is not None: - payload_bytes = len(line.encode('utf-8')) - if payload_bytes > REQUEST_MAX_BYTES: - raise RequestTooLargeError(payload_bytes, REQUEST_MAX_BYTES) - msg = json.loads(line) - if isinstance(msg, dict): - req_id = msg.get('id') - if isinstance(req_id, int): - # Why: preserve request ids even when handlers raise. - mid = req_id + msg = json.loads(line) + if isinstance(msg, dict): + req_id = msg.get('id') + if isinstance(req_id, int): + # Why: preserve request ids even when handlers raise. + mid = req_id + if REQUEST_MAX_BYTES is not None: + payload_bytes = len(line.encode('utf-8')) + if payload_bytes > REQUEST_MAX_BYTES: + raise RequestTooLargeError(payload_bytes, REQUEST_MAX_BYTES)
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (24)
README.mdROADMAP.mddocs/api/README.mddocs/configuration.mddocs/runtimes/nodejs.mdruntime/python_bridge.pysrc/config/index.tssrc/runtime/bridge-core.tssrc/runtime/node.tssrc/runtime/optimized-node.tssrc/utils/codec.tssrc/utils/logger.tssrc/utils/python.tstest/adversarial_playground.test.tstest/bridge-core.test.tstest/fixtures/missing_id_bridge.pytest/fixtures/out_of_order_bridge.pytest/fixtures/oversized_line_bridge.pytest/fixtures/python/adversarial_module.pytest/fixtures/string_id_bridge.pytest/fixtures/unexpected_id_bridge.pytest/fixtures/wrong_protocol_bridge.pytest/optimized-node.test.tstest/runtime_bridge_fixtures.test.ts
💤 Files with no reviewable changes (1)
- src/utils/python.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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 (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: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
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: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:22.461Z
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.
📚 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 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:
docs/configuration.mdtest/bridge-core.test.tsdocs/runtimes/nodejs.mdtest/adversarial_playground.test.tsREADME.mdruntime/python_bridge.py
📚 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 (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:
ROADMAP.mdtest/optimized-node.test.tstest/runtime_bridge_fixtures.test.tstest/bridge-core.test.tsdocs/runtimes/nodejs.mdtest/fixtures/wrong_protocol_bridge.pytest/fixtures/out_of_order_bridge.pytest/adversarial_playground.test.tsREADME.mdruntime/python_bridge.pytest/fixtures/missing_id_bridge.py
📚 Learning: 2026-01-19T21:00:22.461Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:22.461Z
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/optimized-node.test.tstest/runtime_bridge_fixtures.test.tstest/bridge-core.test.tstest/fixtures/out_of_order_bridge.pytest/adversarial_playground.test.tssrc/runtime/bridge-core.ts
📚 Learning: 2026-01-19T21:00:35.449Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:35.449Z
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/optimized-node.test.tstest/runtime_bridge_fixtures.test.tstest/fixtures/string_id_bridge.pytest/fixtures/unexpected_id_bridge.pytest/fixtures/wrong_protocol_bridge.pytest/fixtures/out_of_order_bridge.pytest/adversarial_playground.test.tstest/fixtures/missing_id_bridge.pytest/fixtures/oversized_line_bridge.pysrc/runtime/bridge-core.ts
📚 Learning: 2026-01-19T21:00:35.449Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:35.449Z
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_bridge_fixtures.test.tstest/adversarial_playground.test.tssrc/runtime/bridge-core.ts
📚 Learning: 2026-01-19T21:00:35.449Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:35.449Z
Learning: Maintain adversarial test fixtures by preserving the failure mode they exercise, even if that behavior would be considered bad in production code. When reviewing test fixtures with unusual patterns (e.g., missing error handling, protocol violations, unanswered requests), require clarifying comments that explain the intent rather than suggesting normalization or fixes. This helps ensure tests accurately validate failure modes and that intent is documented for future maintenance.
Applied to files:
test/fixtures/string_id_bridge.pytest/fixtures/unexpected_id_bridge.pytest/fixtures/wrong_protocol_bridge.pytest/fixtures/out_of_order_bridge.pytest/fixtures/python/adversarial_module.pytest/fixtures/missing_id_bridge.pytest/fixtures/oversized_line_bridge.py
📚 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/node.tssrc/runtime/optimized-node.tssrc/runtime/bridge-core.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/node.tssrc/runtime/optimized-node.tssrc/runtime/bridge-core.ts
🧬 Code graph analysis (10)
test/runtime_bridge_fixtures.test.ts (4)
tools/matrix.js (1)
candidates(38-40)src/utils/runtime.ts (1)
getPythonExecutableName(568-570)src/runtime/node.ts (1)
NodeBridge(81-324)src/runtime/optimized-node.ts (1)
OptimizedNodeBridge(131-763)
test/fixtures/string_id_bridge.py (1)
test/fixtures/missing_id_bridge.py (2)
meta_payload(10-20)main(23-37)
test/fixtures/unexpected_id_bridge.py (3)
test/fixtures/missing_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/string_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/wrong_protocol_bridge.py (2)
meta_payload(10-20)main(23-37)
test/bridge-core.test.ts (1)
src/runtime/bridge-core.ts (2)
normalizeEnv(408-445)BridgeCore(59-346)
src/runtime/node.ts (3)
src/runtime/bridge-core.ts (5)
BridgeCore(59-346)getPathKey(364-372)ensurePythonEncoding(374-385)ensureJsonFallback(387-391)validateBridgeInfo(348-362)src/utils/codec.ts (1)
decodeValueAsync(481-483)src/types/index.ts (1)
BridgeInfo(312-324)
test/fixtures/out_of_order_bridge.py (5)
test/fixtures/missing_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/string_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/unexpected_id_bridge.py (2)
meta_payload(10-20)main(23-42)test/fixtures/wrong_protocol_bridge.py (2)
meta_payload(10-20)main(23-37)runtime/python_bridge.py (1)
main(805-853)
test/adversarial_playground.test.ts (2)
src/utils/python.ts (1)
resolvePythonExecutable(47-78)src/runtime/node.ts (1)
NodeBridge(81-324)
test/fixtures/missing_id_bridge.py (2)
test/fixtures/string_id_bridge.py (2)
meta_payload(10-20)main(23-37)test/fixtures/wrong_protocol_bridge.py (2)
meta_payload(10-20)main(23-37)
test/fixtures/oversized_line_bridge.py (1)
test/fixtures/missing_id_bridge.py (1)
main(23-37)
src/runtime/bridge-core.ts (2)
src/runtime/timed-out-request-tracker.ts (1)
TimedOutRequestTracker(10-58)src/runtime/protocol.ts (2)
TYWRAP_PROTOCOL(1-1)TYWRAP_PROTOCOL_VERSION(2-2)
🪛 markdownlint-cli2 (0.18.1)
ROADMAP.md
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 Ruff (0.14.13)
test/fixtures/string_id_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
test/fixtures/unexpected_id_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
38-38: Do not catch blind exception: Exception
(BLE001)
test/fixtures/wrong_protocol_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
test/fixtures/out_of_order_bridge.py
37-37: Do not catch blind exception: Exception
(BLE001)
runtime/python_bridge.py
145-147: try-except-pass detected, consider logging the exception
(S110)
145-145: Do not catch blind exception: Exception
(BLE001)
687-687: Avoid specifying long messages outside the exception class
(TRY003)
705-705: Avoid specifying long messages outside the exception class
(TRY003)
718-718: Avoid specifying long messages outside the exception class
(TRY003)
731-731: Avoid specifying long messages outside the exception class
(TRY003)
757-757: Avoid specifying long messages outside the exception class
(TRY003)
800-800: Consider moving this statement to an else block
(TRY300)
816-816: Abstract raise to an inner function
(TRY301)
852-852: Do not catch blind exception: Exception
(BLE001)
test/fixtures/python/adversarial_module.py
15-15: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
15-15: Dynamically typed expressions (typing.Any) are disallowed in echo
(ANN401)
23-23: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
23-23: Dynamically typed expressions (typing.Any) are disallowed in sleep_and_return
(ANN401)
40-40: Dynamically typed expressions (typing.Any) are disallowed in return_unserializable
(ANN401)
66-66: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
66-66: Dynamically typed expressions (typing.Any) are disallowed in print_to_stdout
(ANN401)
75-75: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
75-75: Dynamically typed expressions (typing.Any) are disallowed in write_to_stderr
(ANN401)
test/fixtures/missing_id_bridge.py
30-30: Do not catch blind exception: Exception
(BLE001)
⏰ 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: required
🔇 Additional comments (63)
src/utils/logger.ts (1)
133-138: ESLint suppression looks appropriate here.
levelis constrained toLogLevel, so suppressing the object-injection rule is reasonable and keeps the intent clear.src/utils/codec.ts (3)
174-182: LGTM!The reformatting of the async arrow function improves readability without changing runtime behavior.
431-441: LGTM!Multiline reformatting improves readability while preserving the original error messages.
443-453: LGTM!The runtime type guard for
versioncorrectly validates that it must be a string when provided, preventing TypeErrors from malformed payloads. The returnedSklearnEstimatornow properly includes the optionalversionfield, matching the interface definition.src/config/index.ts (1)
317-318: LGTM!The ESLint disable is appropriate and consistent with similar suppressions elsewhere in this file (lines 81, 247, 331). The temp path is safely derived from the resolved config location.
test/fixtures/oversized_line_bridge.py (1)
1-16: LGTM!Well-structured adversarial fixture for testing oversized line handling. The comment at line 6 clearly documents the payload size intent, and the implementation correctly flushes after each write.
test/fixtures/python/adversarial_module.py (4)
1-12: Well-structured adversarial test fixture module.The module docstring and imports are appropriate. The
typing.Anyusage flagged by Ruff (ANN401) is intentional here—these adversarial helpers need to accept and return arbitrary values to exercise edge cases in the bridge protocol.
15-63: LGTM — baseline and serialization edge-case helpers.The explicit
float()andint()casts on lines 28 and 37 correctly handle potential string inputs from JSON deserialization. The unserializable/circular/NaN helpers properly exercise serialization failure paths.
66-109: LGTM — I/O noise and crash simulation helpers.Using
os._exit()at line 109 is the correct choice to simulate a hard crash that bypasses Python's normal cleanup, which is exactly what this fixture needs to test.
112-219: LGTM — comprehensive envelope validation fixtures.These helpers provide good coverage for decoder guard paths: unsupported codec versions, unknown encodings, missing required fields, and type mismatches. The envelope structures correctly target specific validation branches.
docs/configuration.md (1)
299-303: LGTM — environment variable documentation.The new
TYWRAP_REQUEST_MAX_BYTESand the note aboutTYWRAP_CODEC_MAX_BYTESdefaultingmaxLineLengthalign with the implementation changes in this PR.docs/api/README.md (2)
70-72: Clear experimental status disclaimer.Good addition clarifying that OptimizedNodeBridge is not yet part of the public API surface.
93-94: LGTM — interface documentation matches implementation.The new
maxLineLengthandinheritProcessEnvoptions are correctly documented, consistent with theNodeBridgeOptionsinterface insrc/runtime/node.ts.README.md (2)
57-57: Good — API consistency withtimeoutMs.The rename from
timeouttotimeoutMsimproves clarity by explicitly indicating the unit.
61-73: Comprehensive bridge behavior documentation.This section effectively summarizes the key differences between NodeBridge and OptimizedNodeBridge, environment inheritance defaults, and payload size caps. The reference to
ROADMAP.mdfor the unification plan is helpful.test/runtime_bridge_fixtures.test.ts (2)
12-25: LGTM — robust Python detection.The
checkPythonAvailablefunction correctly tries multiple candidates including the platform-appropriate name fromgetPythonExecutableName().
68-83: LGTM — OptimizedNodeBridge parity test.The test correctly passes
pythonPathto the constructor and validates error pattern matching against the same fixtures as NodeBridge.test/bridge-core.test.ts (4)
6-12: Clean minimal test transport.The
TestTransportclass provides a simple mock that captures writes for verification without introducing unnecessary complexity.
14-36: LGTM — environment normalization coverage.The test validates key behaviors: PATH case normalization (Windows
PathvsPATH), undefined value dropping, and override merging. The assertions on lines 29-34 correctly verify the expected outcome based on thenormalizeEnvimplementation.
38-52: LGTM — fail-fast validation for non-serializable payloads.This test correctly verifies that
BridgeCore.sendrejects immediately withBridgeProtocolErrorwhen the payload contains a circular reference, ensuring serialization failures are surfaced before IPC.
54-68: LGTM — oversized line buffering guard.The test validates that
BridgeCoreenforcesmaxLineLengthby rejecting when stdout data exceeds the configured limit (32 bytes) without a newline. This aligns with the protocol guardrails objective.test/fixtures/wrong_protocol_bridge.py (2)
28-31: BareExceptioncatch is acceptable for this test fixture.Static analysis flags
BLE001here, but for adversarial test fixtures, catching all exceptions ensures the fixture remains resilient and continues exercising the intended failure mode even with unexpected input. This is intentional.
1-41: LGTM overall.The fixture correctly implements a dual-mode protocol bridge that returns the correct protocol for meta requests but an incorrect protocol for all other requests, enabling protocol validation testing.
docs/runtimes/nodejs.md (3)
14-21: LGTM: Bridge selection guidance is clear.The documentation correctly distinguishes NodeBridge (default, correctness-first) from OptimizedNodeBridge (experimental, not public API) and notes both share the same JSONL core.
115-121: LGTM: Configuration reference accurately reflects new options.The table correctly documents
maxLineLengthdefaulting toTYWRAP_CODEC_MAX_BYTESor1048576, andinheritProcessEnvdefaulting tofalse. The follow-up text clarifies the minimal subprocess environment behavior.
242-250: LGTM: Request size limit documentation is accurate.The new section correctly describes
TYWRAP_REQUEST_MAX_BYTESbehavior and provides a clear example.test/fixtures/unexpected_id_bridge.py (1)
1-46: LGTM overall structure.The fixture correctly implements the unexpected-ID protocol violation scenario for testing bridge resilience. The bare
Exceptioncatches on lines 30 and 38 are acceptable for test fixtures to ensure resilience with malformed input.test/optimized-node.test.ts (5)
18-32: LGTM:waitForhelper is well-implemented.The helper correctly polls with configurable timeout and interval, throws on timeout, and avoids busy-waiting. This pattern is appropriate for async condition verification in tests.
386-407: LGTM: Serialization error regression test.The test correctly verifies that
BigIntserialization failures surface asBridgeProtocolErrorwith the expected message pattern, using the consolidatedtoSatisfyassertion pattern.
409-440: LGTM: Worker stdin destruction test.The test correctly verifies that destroying a worker's stdin triggers proper error handling and eventual worker pool cleanup. The internal
processPoolaccess is acceptable for integration testing, and the coupling is noted per past review feedback.
442-472: LGTM: Worker crash recovery test.The test correctly exercises the crash-and-recover path by triggering a crash, waiting for
processDeathsto increment, and verifying the bridge remains functional afterward. This aligns with the adversarial testing philosophy in the PR.
496-502: LGTM: Precise timeout death count assertions.Changing from
>=to strict equality (before.processDeaths + 1) makes the test more precise and easier to debug if the count drifts unexpectedly.test/adversarial_playground.test.ts (3)
42-62: LGTM: Bridge factory handles null pythonPath correctly.The factory now checks
!pythonPath || !pythonAvailable(pythonPath)before proceeding, addressing the type narrowing concern from past reviews.
417-469: LGTM: Comprehensive decoder validation failure coverage.The parameterized tests cover a good range of malformed envelope scenarios (bad codec version, bad encoding, missing fields, invalid formats). Each case correctly disposes the bridge in a finally block.
471-550: LGTM: Protocol contract violation tests are thorough.The fixture-based tests correctly exercise protocol errors (wrong protocol, missing/string/unexpected IDs, invalid JSON, noisy output) and edge cases (fragmented frames, out-of-order responses). The test structure with proper cleanup is appropriate.
src/runtime/node.ts (5)
16-26: LGTM: Clean import organization for BridgeCore integration.The imports correctly pull in
BridgeCore,RpcRequesttype, and the helper functions needed for environment handling and validation.
196-235: LGTM: BridgeCore integration is well-structured.The
startProcessmethod correctly:
- Derives
maxLineLengthfrom options or environment- Constructs
BridgeCorewith proper writer callback, timeout, and decoder- Wires up
onFatalErrorto trigger process reset- Routes stdout/stderr through core handlers
261-300: LGTM:buildEnvcorrectly implements the environment filtering policy.The method properly:
- Inherits full env when
inheritProcessEnv: true- Otherwise filters to allowed keys/prefixes (PATH, PYTHON*, TYWRAP_*)
- Applies virtualenv PATH adjustments
- Ensures Python encoding and JSON fallback settings
This addresses the past review concern about breaking existing setups by providing the
inheritProcessEnvopt-in.
302-317: LGTM:resetProcessprovides clean lifecycle teardown.The method correctly clears the core, kills the process if still running, and resets all state references. Catching and ignoring errors on kill is appropriate since the process may already be gone.
173-177: LGTM: Dispose correctly delegates to core and resets.The
disposemethod properly marks the bridge as disposed, triggers pending request rejection viahandleProcessExit, and cleans up viaresetProcess.runtime/python_bridge.py (7)
57-71: Clear error types for request limits.
Nice separation between parse and oversize errors, which should make diagnostics more actionable.
129-147: Bounded protocol diagnostics are solid.
The size cap and safe stderr writes keep the JSONL stream protected.
620-657: Handler arg validation is nicely centralized.
Usingrequire_str/coerce_*makes handler failures much clearer.
685-733: Protocol and param coercion helpers look good.
Consistent validation improves error reporting and reduces boilerplate.
741-758: Unknown methods now fail fast withProtocolError.
This keeps the response contract explicit.
790-803:write_payloadcleanly encapsulatesBrokenPipehandling.
Centralizing this is a good reliability improvement.
842-851: Fallback response path handlesBrokenPipegracefully.
Good use ofwrite_payloadin the error fallback.src/runtime/optimized-node.ts (7)
18-27: No review notes for the import updates.
66-74: Per‑workerBridgeCoreand quarantine flag are well integrated.
This keeps IO/state encapsulated and makes worker lifecycle clearer.Also applies to: 135-135, 465-481
210-220: Cache hits now update aggregate stats — nice catch.
This keepstotalRequestsand hit‑rate metrics accurate.Also applies to: 232-236
325-327: Quarantined workers are excluded and replaced cleanly.
The selection + replacement flow looks robust.Also applies to: 392-413
375-389:sendToWorkernow resetsbusyreliably and updates per‑worker stats.
Thetry/finallystructure is solid.
500-523: BridgeCore IO hooks are wired correctly.
Stdout/stderr/process errors are now consistently delegated.
530-536: Exit/terminate cleanup and pending metrics look good.
core.clear()andpendingRequestsreporting should improve operability.Also applies to: 651-651, 688-704, 752-755
src/runtime/bridge-core.ts (9)
44-57: Stderr sanitization and control‑char stripping look good.
Meets the guardrail objectives cleanly.
86-105:clear()correctly resets pending timers and buffers.
State cleanup looks complete and safe.
107-150:send()timeout and request tracking logic looks solid.
Good separation of transport failure vs. timeout paths.
152-188: Stdout line‑length guarding is robust.
The early cut‑off protects against malformed output.
190-200: Stderr tail buffering is bounded and sanitized.
This matches the guardrail requirement.
203-216: Process exit/error handling is consistent.
Reject‑all plus fatal callback keeps the state coherent.
300-345: Failure propagation helpers are clean and consistent.
The single‑error guard inhandleProtocolErroris especially helpful.
348-362: Runtime guard invalidateBridgeInfois a good hardening step.
PreventsTypeErroron malformed metadata.
364-392: Env helpers and normalization look good.
ensurePythonEncoding/ensureJsonFallback+ PATH normalization are tidy.Also applies to: 408-445
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary
Testing
Closes #125, #124, #123, #122, #121, #119, #115, #113, #112, #111, #110, #109, #105, #101, #98, #97, #96, #90, #80, #79, #78, #76, #64, #51, #46, #44, #39, #83, #42, #100, #88, #50