docs(adr): add ADR-002 BridgeProtocol architecture #152
Conversation
Design unified BridgeProtocol abstraction combining: - BoundedContext (lifecycle, error classification, bounded execution) - SafeCodec (validation layer for NaN/Infinity/serialization) - Transport (abstract I/O: ProcessIO, HttpIO, PyodideIO) - WorkerPool (concurrency management with semaphores) Maps ~30 open issues to specific architectural components. Includes TypeScript and Python SafeCodec implementations. Defines 5-week migration plan with backward compatibility. Closes #146, #148 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new BridgeProtocol architecture and runtime: a three-layer boundary-crossing design (SafeCodec, Transport, WorkerPool) with TypeScript transports (ProcessIO, HttpIO, PyodideIO), a BridgeProtocol orchestrator, a Python SafeCodec implementation, and extensive tests and documentation (ADR + migration plan). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Bridge as BridgeProtocol
participant Codec as SafeCodec
participant Pool as WorkerPool
participant Worker as Transport
participant Python as PythonWorker
Client->>Bridge: call/instantiate/callMethod(args)
Bridge->>Codec: encodeRequest(payload)
Codec-->>Bridge: encodedMessage
Bridge->>Pool: acquire/send(encodedMessage)
Pool->>Worker: send(message, timeout, signal)
Worker->>Python: deliver(message)
Python-->>Worker: response(encodedResponse)
Worker-->>Pool: deliverResponse(encodedResponse)
Pool->>Codec: decodeResponse(encodedResponse)
Codec-->>Bridge: decodedResult
Bridge-->>Client: return result / throw error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/adr/002-bridge-protocol.md`:
- Around line 31-66: The fenced ASCII diagram block lacks a language tag; edit
the block that begins with the BridgeProtocol diagram (the triple-backtick
fenced block containing "BridgeProtocol" and the ASCII diagram) and change the
opening fence from ``` to ```text so markdownlint no longer flags it and the
diagram remains literal.
- Around line 1163-1206: Add missing blank lines before and after each section
heading in the "Migration Path" and "Consequences" areas of the ADR so
Markdownlint stops flagging them; specifically ensure there is an empty line
above and below headings like "Phase 1: SafeCodec (Week 1)", "Phase 2: Transport
(Week 2)", "Phase 3: WorkerPool (Week 3)", "Phase 4: BridgeProtocol Integration
(Week 4)", "Phase 5: Cleanup (Week 5)", and the "Consequences" subheadings
(Positive, Negative, Neutral) by inserting a single blank line both preceding
and following each heading in docs/adr/002-bridge-protocol.md.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/adr/002-bridge-protocol.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T01:33:12.841Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:238-249
Timestamp: 2026-01-20T01:33:12.841Z
Learning: In the tywrap repository, when reviewing init/startup patterns, prioritize cases that break core functionality over edge cases with diagnostic-only impact (e.g., missing bridgeInfo metadata). If workers are healthy and operational, defer low-impact defensive checks to follow-up PRs.
Applied to files:
docs/adr/002-bridge-protocol.md
🪛 markdownlint-cli2 (0.18.1)
docs/adr/002-bridge-protocol.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1163-1163: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1169-1169: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1175-1175: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1180-1180: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1187-1187: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1194-1194: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1201-1201: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
1206-1206: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add `text` language identifier to ASCII diagram code block - Add blank lines after section headings in Migration Path - Add blank lines after section headings in Consequences Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@docs/adr/002-bridge-protocol.md`:
- Around line 826-830: Clarify and fix the stub RuntimeExecution methods (call,
instantiate, callMethod, disposeInstance) on HttpIO: update the ADR to state
explicitly which interface (RuntimeExecution) requires these methods and why
HttpIO must implement them (or that it also implements Transport), and replace
the unsafe returns ({} as T) with either proper delegation to the underlying
transport or explicit runtime-safe stubs that throw a clear "NotSupportedError"
/ "MethodNotImplemented" with explanatory message; mention the exact symbols
RuntimeExecution, Transport, HttpIO, and the methods call, instantiate,
callMethod, disposeInstance so reviewers can locate and update the class
structure and documentation accordingly.
- Around line 168-172: Add a brief explanatory comment near the post-decoder
special-float check to clarify why validation runs after applyDecoders():
explain that decoders (e.g., Arrow format decoders) can introduce NaN/Infinity
into the result even if the raw JSON lacked them, and note that this protects
downstream consumers; reference the existing symbols: the conditional using
this.options.rejectSpecialFloats, the containsSpecialFloat check, the
applyDecoders() call, and the BridgeProtocolError thrown so maintainers
understand why the guard exists in that location.
- Around line 203-211: The Map key-type check inside the BridgeProtocol
validation is ineffective because Maps aren't preserved by JSON.stringify;
update validation to explicitly reject Map and Set instances earlier (e.g., in
safeStringify or just prior to serialization) by detecting value instanceof Map
|| value instanceof Set and throwing a BridgeProtocolError with a clear message
like "Bridge protocol does not support Map/Set values" instead of the non-string
key loop; remove the unreachable key-type iteration (the for...of over
value.keys()) from the current handler to avoid dead code and centralize the
rejection logic in safeStringify or the serialization entrypoint.
- Around line 636-637: The current increment of this.requestCount followed by
this.maybeRestartProcess() can kill the process immediately when requestCount >=
restartAfterRequests and cause handleProcessExit to reject all pendingRequests;
update the code so maybeRestartProcess checks the pendingRequests collection
(pendingRequests.size) and only triggers the immediate kill if
pendingRequests.size === 0, otherwise schedule a deferred restart (e.g.,
setTimeout or a drain watcher that restarts when pendingRequests becomes zero),
or alternatively add a clear documentation comment near the request-count/check
code explaining that the current behavior rejects all in-flight requests and
advising raising restartAfterRequests to avoid mid-flight restarts; reference
the methods/fields this.requestCount, maybeRestartProcess(),
restartAfterRequests, pendingRequests, and handleProcessExit when making the
change.
- Around line 599-602: Implement a clear ready-signal contract for the bridge by
updating the waitForReady method: either (A) implement it to listen on the
existing transport for an explicit "ready" message from the Python process (with
a configurable timeout and clear error on timeout) so callers only send requests
after that signal, or (B) document explicitly in the ADR and in waitForReady
that the transport assumes readiness immediately after spawn and add a short
grace period/queuing behavior or immediate rejection of requests until imports
complete; reference the waitForReady function and the transport message handling
logic so the chosen behavior is enforced and timeouts/errors are surfaced to
callers.
- Around line 1059-1061: The generateId method on BridgeProtocol currently
returns `req_${Date.now()}_${++this.requestId}` which can collide across
instances, on clock skew, or after restarts; update BridgeProtocol.generateId to
use a stronger source of entropy (e.g., `crypto.randomUUID()` if available) or
append process-specific and extra entropy like `process.pid` and additional
random bytes (e.g.,
`req_${Date.now()}_${process.pid}_${++this.requestId}_${randomSuffix}`) so IDs
are unique across instances and restarts; ensure you reference the class field
requestId and add any required import/feature-detection for crypto.randomUUID()
or a fallback random generator.
- Around line 615-643: In handleResponseLine, avoid running the full
/"id"\s*:\s*"([^"]+)"/ regex over the entire potentially very large line;
instead, search only a truncated prefix (e.g., first ~1KB) for the id before
looking up pendingRequests and rejecting; keep the subsequent JSON.parse on the
full line for valid responses, and ensure you still extract the id from the
truncated slice to perform pendingRequests.get(id) and pending.reject/ delete as
currently implemented.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/adr/002-bridge-protocol.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T01:33:12.841Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:238-249
Timestamp: 2026-01-20T01:33:12.841Z
Learning: In the tywrap repository, when reviewing init/startup patterns, prioritize cases that break core functionality over edge cases with diagnostic-only impact (e.g., missing bridgeInfo metadata). If workers are healthy and operational, defer low-impact defensive checks to follow-up PRs.
Applied to files:
docs/adr/002-bridge-protocol.md
🪛 LanguageTool
docs/adr/002-bridge-protocol.md
[grammar] ~25-~25: Use a hyphen to join words.
Context: ...ion layer that standardizes all boundary crossing concerns by combining: 1. **Bo...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (5)
docs/adr/002-bridge-protocol.md (5)
257-378: SafeCodec Python implementation looks solid.The
_default_encoderprovides comprehensive coverage of special Python types (numpy scalars, datetime, Decimal, UUID, Path, bytes, Pydantic models), and the explicit NaN/Infinity checks (lines 328-334) provide clearer error messages than relying onjson.dumpsfailures. The edge-case handling aligns well with the TypeScript side.
679-716: Backpressure handling looks correct.The
queueWriteimplementation properly respects Node.js stream backpressure by:
- Checking the return value of
write()(line 700)- Waiting for the
drainevent before continuing (line 712)- Processing writes sequentially with the
isWritingflagThis aligns with Node.js best practices for writable streams.
901-977: WorkerPool concurrency logic looks solid.The acquire/release pattern correctly manages worker availability:
acquire()tries available → create → wait (lines 903-917)release()decrements count and serves waiting requests (lines 923-933)waitForWorker()usestimer.unref()to prevent keeping the process alive (lines 970-973)- Cleanup properly rejects waiters and disposes workers (lines 883-896)
This addresses the issues mentioned in the mapping table (
#139,#138,#99,#92,#60).
1122-1196: Issue mappings and migration plan are well-structured.The tables clearly show how 30+ runtime issues map to the three architectural layers (SafeCodec, Transport, WorkerPool), and the 5-phase migration plan provides a logical progression:
- Week 1-2: Build core layers (SafeCodec, Transport)
- Week 3: Add WorkerPool for concurrency
- Week 4: Integrate into BridgeProtocol and migrate existing bridges
- Week 5: Cleanup and documentation
This phased approach allows for incremental testing and reduces migration risk.
7-22: The ADR document does not reference issue#146. The ADR explicitly addresses ~30 boundary crossing issues (Data Validation, Transport Reliability, Resource Management) and references issue#149(BoundedContext implementation), not compile-time type mapping. If the PR summary claims to close#146, that appears to be a documentation error in the PR description itself, not an issue with the ADR content.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Implement the core infrastructure for ADR-002 BridgeProtocol, which standardizes all JS↔Python boundary crossing concerns. New components: - SafeCodec: Validation and serialization layer (TS + Python) - Rejects NaN/Infinity by default - Rejects non-string object keys - Configurable size limits - Python side handles numpy, datetime, Decimal, UUID, Path, bytes - Transport interface with three implementations: - ProcessIO: Subprocess JSONL protocol with backpressure handling - HttpIO: Stateless HTTP transport with timeout/abort support - PyodideIO: Browser WASM in-memory transport - WorkerPool: Semaphore-based concurrent transport management - Configurable max workers and queue timeout - Lazy worker creation - Proper cleanup on dispose - BridgeProtocol: Base class combining BoundedContext + SafeCodec + Transport - Unified sendMessage API - RuntimeExecution interface implementation - Resource tracking integration Tests added: - SafeCodec: 64 TS tests, 73 Python tests - Transport: 126 tests - WorkerPool: 61 tests - BridgeProtocol: 49 integration tests Bridge migrations (NodeBridge, HttpBridge, PyodideBridge) deferred to future PRs for incremental adoption. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
coderabbitai
bot
added
area:codec
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 Fix all issues with AI agents
In `@docs/adr/002-bridge-protocol.md`:
- Around line 988-993: Update the ADR sketch's WorkerPool RuntimeExecution
method stubs (the async methods call, instantiate, callMethod, and
disposeInstance) so they mirror the real implementation by throwing
BridgeExecutionError instead of returning "{} as T"; locate the RuntimeExecution
class in the ADR content and replace the placeholder returns with code that
throws a BridgeExecutionError (including a descriptive message) for each of
those methods to keep the documentation consistent with the implementation.
In `@runtime/safe_codec.py`:
- Around line 19-22: Remove the unnecessary pass inside the CodecError exception
class: locate the class CodecError(Exception) in runtime/safe_codec.py and
delete the redundant `pass` statement so the class body only contains the
docstring, leaving the class definition empty otherwise.
In `@src/runtime/bridge-protocol.ts`:
- Around line 216-218: The generateId() method currently builds IDs from
Date.now() and a per-instance counter (requestId), which risks collisions across
instances or clock changes; replace its body to return a
cryptographically-unique UUID via crypto.randomUUID(), e.g. have generateId()
call crypto.randomUUID() (ensure the Node global crypto is used or import crypto
from 'node:crypto' if your linter prefers), remove reliance on this.requestId
for ID uniqueness, and update any tests or callers expecting the old prefix
format (req_...) to accept UUID strings; keep the method signature generateId():
string unchanged.
In `@src/runtime/http-io.ts`:
- Around line 180-209: The caught-error handler in http-io.ts defines a local
variable named `message` which shadows the function parameter `message` (from
the surrounding function), causing confusion; rename the local `message` (e.g.,
to `errMsg` or `errorMessage`) in the catch block where you build the fallback
error (currently used in the BridgeExecutionError throw) and update its usage in
the template string, leaving the outer `message` parameter untouched; ensure
references in the final BridgeExecutionError construction and the cause
assignment use the new local name.
- Around line 130-131: The current effectiveTimeout calculation treats 0 like a
negative value and falls back to this.defaultTimeoutMs, which violates the
Transport interface JSDoc that says "timeoutMs: 0 = no timeout"; change the
logic so that if timeoutMs === 0 you preserve “no timeout” (e.g., set
effectiveTimeout to 0 or a sentinel used by the caller), if timeoutMs > 0 use
timeoutMs, otherwise use this.defaultTimeoutMs; update the assignment that
defines effectiveTimeout (and any downstream use of effectiveTimeout) to follow
this three-way branch using the existing variable names timeoutMs,
effectiveTimeout, and this.defaultTimeoutMs.
- Around line 126-128: The disposed-state check in the Transport implementation
uses the wrong error type: replace the thrown BridgeExecutionError with
BridgeDisposedError (preserve the existing message 'Transport has been
disposed'), update the import to reference BridgeDisposedError where
BridgeExecutionError was referenced in this module, and run a quick compile to
ensure any other references to BridgeExecutionError in this file are adjusted or
left intact as appropriate; specifically change the throw in the block checking
this._isDisposed to throw BridgeDisposedError instead of BridgeExecutionError.
In `@src/runtime/process-io.ts`:
- Around line 216-219: The restart logic currently calls this.restartProcess()
as soon as this.requestCount >= this.restartAfterRequests; change it to gate the
restart until in-flight work is drained by checking pendingRequests.size === 0
and any queuedWrites/queue.length === 0 (or call an existing
drainPendingRequests() helper) before invoking restartProcess(); if the pipeline
is not empty, set a restartPending flag and await/subscribe to the
pendingRequests/queue drain (or poll with a short delay and optional timeout) so
restartProcess() only runs once pendingRequests are zero and all queued writes
are flushed.
- Around line 37-40: Replace the regex literal definitions for ANSI_ESCAPE_RE
and CONTROL_CHARS_RE with RegExp constructor calls that use escaped hex
sequences (e.g., "\\uXXXX" escapes) instead of raw control characters;
specifically update the constants ANSI_ESCAPE_RE and CONTROL_CHARS_RE to be
created via new RegExp(...) with the same patterns and the "g" flag so behavior
is preserved while avoiding Biome lint flags.
- Around line 224-242: The timeout handler currently rejects but never removes
the abort listener, leaking listeners; update the timeout callback (where
setTimeout is used) to remove the abort listener (call
signal.removeEventListener('abort', abortHandler)) before rejecting and deleting
this.pending for messageId, and likewise ensure abortHandler also clears the
timer and removes itself from the signal (removeEventListener) before deleting
this.pending and rejecting with BridgeTimeoutError; reference the existing
timer, abortHandler, messageId, this.pending and BridgeTimeoutError to locate
the changes.
In `@src/runtime/pyodide-io.ts`:
- Around line 303-390: The Transport implementation (PyodideIO) contains
RuntimeExecution convenience methods (call, instantiate, callMethod,
disposeInstance) which duplicates BridgeProtocol responsibilities; remove these
methods from PyodideIO and either implement them on BridgeProtocol to delegate
to the Transport.send(...) path or extract them into a separate standalone
helper class used only when PyodideIO is intentionally used without
BridgeProtocol; ensure PyodideIO only implements Transport methods (send,
isReady, dispose), update any callers/tests to use BridgeProtocol.<init> or the
new helper, and keep SafeCodec/WorkerPool protocol-level concerns within
BridgeProtocol.
- Around line 17-19: The import list in pyodide-io.ts includes an unused symbol
BridgeDisposedError; remove BridgeDisposedError from the import statement that
currently reads "import { BridgeDisposedError, BridgeExecutionError,
BridgeProtocolError } from './errors.js'" so that only used errors (e.g.,
BridgeExecutionError and BridgeProtocolError) remain imported and no unused
import lints remain.
- Around line 454-456: The generateId method (generateId) can collide under high
concurrency; replace the simple Date.now()+Math.random approach with a robust
unique generator—either call crypto.randomUUID() when available, or add a
class-level monotonic counter (e.g., this._idCounter++) combined with
timestamp/instance prefix to ensure uniqueness across calls; update generateId
to use the chosen strategy and initialize the counter on the class so concurrent
calls cannot produce duplicate IDs.
In `@src/runtime/safe-codec.ts`:
- Around line 252-258: The catch block in safe-codec.ts is shadowing the outer
method parameter named "message"; in the catch for JSON serialization failure
(inside the function that throws BridgeProtocolError) rename the local variable
currently called "message" to something like "errMessage" or
"serializationError" and use that new identifier in the BridgeProtocolError
interpolation so the thrown error no longer shadows the parameter and the lint
warning is resolved (reference the catch block that throws new
BridgeProtocolError(`JSON serialization failed: ${...}`)).
- Around line 354-371: The special float validation in the decoding logic
currently checks the `parsed` value (before Arrow decoding), but Arrow decoders
can introduce NaN/Infinity during the `decodeArrowValue` call. Change the
post-decode validation block to validate the `decoded` value instead of `parsed`
in both the `containsSpecialFloat` and `findSpecialFloatPath` calls. This
ensures that special floats introduced during Arrow deserialization are properly
detected and rejected when the `rejectSpecialFloats` flag is enabled, rather
than allowing them to pass through undetected.
- Around line 379-386: The current toBase64(bytes: Uint8Array) uses
String.fromCharCode(...bytes) which can exceed JS engine argument limits; change
the globalThis.btoa branch to build the binary string in safe chunks (e.g.,
iterate over bytes with a chunk size like 0x8000 or 32768, convert each chunk
via String.fromCharCode(...chunk) and append to a binary accumulator) before
calling globalThis.btoa(binary). Update the implementation inside toBase64 to
use subarray/chunking on the bytes Uint8Array and concatenate chunk results to
avoid RangeError/stack overflow for large buffers.
- Around line 75-227: The code must reject Map and Set at the serialization
boundary to avoid silent data loss: update assertStringKeys to immediately throw
a BridgeProtocolError if value is an instance of Map or Set (with a clear path
message using buildPath/path) instead of attempting to permit or recurse into
Maps, and also add an explicit pre-serialization check in
SafeCodec.encodeRequest (before calling assertStringKeys/JSON.stringify) that
throws a BridgeProtocolError when the top-level message is a Map or Set;
reference assertStringKeys, SafeCodec.encodeRequest, and BridgeProtocolError
when implementing these changes so Maps/Sets are unconditionally rejected with a
clear error.
In `@src/runtime/transport.ts`:
- Around line 195-204: The isTransport type guard currently only checks for the
presence of isReady but not its type; update the guard (function isTransport) to
also verify that (value as Transport).isReady is a boolean by adding a typeof
check (e.g., typeof (value as Transport).isReady === 'boolean') alongside the
existing checks so only objects with a boolean isReady pass the guard.
In `@src/runtime/worker-pool.ts`:
- Around line 300-313: createWorker currently creates a transport via
this.options.createTransport() and calls await transport.init(), but if init()
throws the created transport may leak; wrap the init call in try/catch and on
error run a defensive async cleanup (e.g., if transport.dispose/close/destroy
exist, await the available one) before rethrowing the error, and only push the
PooledWorker into this.workers after init succeeds; reference createWorker,
transport.init, this.options.createTransport, and PooledWorker when making the
change.
In `@test/safe-codec.test.ts`:
- Around line 159-196: Tests currently allow Maps/Sets to pass through (relying
on lossy JSON.stringify) but the protocol requires rejecting these types; update
tests that call codec.encodeRequest and new SafeCodec({ rejectNonStringKeys:
false }) so they expect a BridgeProtocolError for Map and Set inputs (including
Map with string keys, nested Maps in objects/arrays, and Sets), and assert the
error message mentions the location (e.g., "Map value found" or path like "at
outer.inner") or at minimum that BridgeProtocolError is thrown; keep tests for
plain objects unchanged and ensure a separate test verifies that when
rejectNonStringKeys is false the codec still rejects Map/Set only if that option
is true (or adjust expectations accordingly for permissiveCodec to accept
non-string keys but still reject Map/Set unless explicitly allowed).
In `@test/transport.test.ts`:
- Around line 513-521: Replace the dynamic Vitest skip by making the test an
ordinary it(...) and add an early-return guard at the top of the test body that
checks the pythonAvailable boolean and returns immediately if false; update the
test that currently uses it.skipIf (the "init spawns process and becomes ready
with real bridge script" test) to keep its assertion logic but start with if
(!pythonAvailable) return; so the test silently passes when Python/fixtures are
unavailable.
- Around line 485-511: The tests are asserting Transport implementations (e.g.,
ProcessIO) implement BridgeProtocol methods (call, instantiate, callMethod,
disposeInstance); update the tests to keep the Transport suite focused only on
the Transport interface by either moving these assertions into the
BridgeProtocol test file or replacing them with assertions that transports do
NOT expose those methods (i.e., remove the expect(() => transport.call...) /
instantiate / callMethod / disposeInstance checks from the ProcessIO/Transport
tests and instead test only send, isReady, and dispose on ProcessIO); reference
ProcessIO, call, instantiate, callMethod, disposeInstance, BridgeProtocol and
the Transport interface (send/isReady/dispose) when making the change.
♻️ Duplicate comments (2)
docs/adr/002-bridge-protocol.md (2)
211-226: Map/Set validation note should reflect implementation decision.Per previous discussion and learnings, Map/Set instances should be explicitly rejected before serialization rather than checking keys. The code sketch at lines 212-220 still shows the key iteration approach. Consider adding a comment noting the actual implementation will reject Map/Set instances directly.
834-839: Stub RuntimeExecution methods in HttpIO should be removed or noted.Per the learning and previous discussion, Transport implementations should not include RuntimeExecution methods. These stubs returning
{} as Tare unsafe and the ADR notes they are "Required by RuntimeExecution but not used directly" which is misleading since HttpIO only implements Transport.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
docs/adr/002-bridge-protocol.mdruntime/safe_codec.pysrc/index.tssrc/runtime/bridge-protocol.tssrc/runtime/http-io.tssrc/runtime/process-io.tssrc/runtime/pyodide-io.tssrc/runtime/safe-codec.tssrc/runtime/transport.tssrc/runtime/worker-pool.tstest/bridge-protocol.test.tstest/python/test_safe_codec.pytest/safe-codec.test.tstest/transport.test.tstest/worker-pool.test.ts
🧰 Additional context used
🧠 Learnings (22)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:826-830
Timestamp: 2026-01-20T16:01:39.136Z
Learning: In the tywrap BridgeProtocol architecture (ADR-002), `HttpIO` (and other Transport implementations like `ProcessIO`, `PyodideIO`) implements only the `Transport` interface (methods: `send`, `isReady`, `dispose`). The `RuntimeExecution` interface methods (`call`, `instantiate`, `callMethod`, `disposeInstance`) are implemented by `BridgeProtocol`, which delegates to `Transport` instances. Transport implementations should not include RuntimeExecution methods.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:636-637
Timestamp: 2026-01-20T16:01:29.276Z
Learning: In the tywrap BridgeProtocol ProcessIO transport (ADR-002), when implementing `restartAfterRequests` functionality, use drain-before-restart as the default: wait for `pendingRequests.size === 0` before killing and restarting the process. This prevents rejecting in-flight requests during restart. Alternative graceful restart (spawn new process, drain old) adds complexity with two concurrent processes.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In the tywrap repository, bridges currently use implicit ready detection: the first successful response from the Python subprocess proves the bridge is ready to handle requests.
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/worker-pool.test.tstest/bridge-protocol.test.tstest/transport.test.tsdocs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/worker-pool.test.tssrc/runtime/safe-codec.tstest/bridge-protocol.test.tstest/safe-codec.test.tssrc/runtime/process-io.tstest/transport.test.tssrc/runtime/transport.tssrc/runtime/http-io.tssrc/index.tssrc/runtime/bridge-protocol.tssrc/runtime/pyodide-io.tssrc/runtime/worker-pool.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
test/worker-pool.test.tstest/bridge-protocol.test.tssrc/runtime/process-io.tstest/transport.test.tssrc/runtime/transport.tssrc/runtime/bridge-protocol.ts
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/worker-pool.test.tstest/bridge-protocol.test.tstest/safe-codec.test.tstest/transport.test.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/worker-pool.test.tssrc/runtime/safe-codec.tstest/bridge-protocol.test.tstest/safe-codec.test.tssrc/runtime/process-io.tstest/transport.test.tssrc/runtime/transport.tssrc/runtime/http-io.tssrc/index.tssrc/runtime/bridge-protocol.tssrc/runtime/pyodide-io.tssrc/runtime/worker-pool.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
src/runtime/safe-codec.tstest/bridge-protocol.test.tssrc/runtime/process-io.tssrc/runtime/transport.tssrc/index.tssrc/runtime/bridge-protocol.tssrc/runtime/pyodide-io.tsdocs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T16:00:49.738Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:168-172
Timestamp: 2026-01-20T16:00:49.738Z
Learning: In the tywrap project's BridgeProtocol SafeCodec implementation, Arrow format decoders can produce NaN/Infinity values from binary representations even when the raw JSON payload doesn't contain them. This is why validation for special floats must occur both before encoding (to reject invalid inputs) and after applying decoders (to catch values introduced during Arrow deserialization), protecting downstream consumers from unexpected NaN/Infinity values.
Applied to files:
src/runtime/safe-codec.tstest/safe-codec.test.tsruntime/safe_codec.pytest/python/test_safe_codec.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/safe-codec.tssrc/runtime/process-io.tssrc/runtime/transport.tssrc/runtime/http-io.tssrc/runtime/bridge-protocol.tssrc/runtime/pyodide-io.tssrc/runtime/worker-pool.ts
📚 Learning: 2026-01-19T21:14:29.869Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
Learning: In the runtime env-var parsing (e.g., in src/runtime/bridge-core.ts and similar modules), adopt a tolerant, best-effort policy: numeric env values should parse by taking the leading numeric portion (e.g., TYWRAP_CODEC_MAX_BYTES=1024abc -> 1024). Only reject clearly invalid values (non-numeric start or <= 0). This reduces surprising failures from minor typos. Add tests to cover partial numeric prefixes, and ensure downstream logic documents the trusted range; consider a small helper to extract a positive integer from a string with a safe fallback.
Applied to files:
src/runtime/safe-codec.tssrc/runtime/process-io.tssrc/runtime/transport.tssrc/runtime/http-io.tssrc/runtime/bridge-protocol.tssrc/runtime/pyodide-io.tssrc/runtime/worker-pool.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/bridge-protocol.test.tstest/transport.test.tsdocs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T16:01:39.136Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:826-830
Timestamp: 2026-01-20T16:01:39.136Z
Learning: In the tywrap BridgeProtocol architecture (ADR-002), `HttpIO` (and other Transport implementations like `ProcessIO`, `PyodideIO`) implements only the `Transport` interface (methods: `send`, `isReady`, `dispose`). The `RuntimeExecution` interface methods (`call`, `instantiate`, `callMethod`, `disposeInstance`) are implemented by `BridgeProtocol`, which delegates to `Transport` instances. Transport implementations should not include RuntimeExecution methods.
Applied to files:
test/bridge-protocol.test.tssrc/runtime/process-io.tstest/transport.test.tssrc/runtime/transport.tssrc/runtime/http-io.tssrc/index.tssrc/runtime/bridge-protocol.tssrc/runtime/pyodide-io.tsdocs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T16:01:29.276Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:636-637
Timestamp: 2026-01-20T16:01:29.276Z
Learning: In the tywrap BridgeProtocol ProcessIO transport (ADR-002), when implementing `restartAfterRequests` functionality, use drain-before-restart as the default: wait for `pendingRequests.size === 0` before killing and restarting the process. This prevents rejecting in-flight requests during restart. Alternative graceful restart (spawn new process, drain old) adds complexity with two concurrent processes.
Applied to files:
src/runtime/process-io.tstest/transport.test.tsdocs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T16:00:40.505Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:168-172
Timestamp: 2026-01-20T16:00:40.505Z
Learning: In docs/adr/002-bridge-protocol.md, add a guideline that the SafeCodec path must reject NaN and Infinity before encoding and also validate after Arrow-deserialization. Use explicit checks (e.g., Number.isFinite or equivalent) on all floating-point inputs before encoding and on values produced by decoders after deserialization. Include tests that cover both pre-encode validation and post-decode validation to prevent invalid values from propagating to downstream consumers.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T16:00:58.747Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:58.747Z
Learning: In the BridgeProtocol implementation for the tywrap repository, Map and Set instances should be explicitly rejected before serialization (e.g., in `safeStringify` or at the serialization entrypoint) with a clear error message like "Bridge protocol does not support Map/Set values", rather than checking for non-string keys after the fact, since Maps and Sets cannot round-trip through JSON.stringify.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In the tywrap repository, bridges currently use implicit ready detection: the first successful response from the Python subprocess proves the bridge is ready to handle requests.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T16:00:56.576Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:00:56.576Z
Learning: Before marking the NodeBridge as ready, validate the bridge by sending a test message to the Python subprocess and require a successful response (heartbeat/ACK) to confirm the bridge is responsive. Document this readiness handshake in the ADR and ensure tests cover the handshake scenario.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: In `test/fixtures/out_of_order_bridge.py`, the fixture intentionally leaves a pending request unanswered at EOF to simulate missing/out-of-order responses and validate bridge behavior when requests never complete; this is the exact failure mode being tested and must be preserved.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T01:36:14.701Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:743-759
Timestamp: 2026-01-20T01:36:14.701Z
Learning: In the tywrap repository, when reviewing concurrent request handling patterns (especially with BridgeCore request multiplexing), flag uses of boolean `busy` flags that should be request counters instead. A boolean flag creates a race condition: when request A completes and sets `busy = false`, request B (still in flight via multiplexing) is incorrectly treated as idle. The fix is to use an `inFlightRequests: number` counter that increments on request start and decrements on completion, then check `worker.inFlightRequests === 0` before cleanup/termination.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-19T21:14:37.032Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-20T01:33:12.841Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:238-249
Timestamp: 2026-01-20T01:33:12.841Z
Learning: In the tywrap repository, when reviewing init/startup patterns, prioritize cases that break core functionality over edge cases with diagnostic-only impact (e.g., missing bridgeInfo metadata). If workers are healthy and operational, defer low-impact defensive checks to follow-up PRs.
Applied to files:
docs/adr/002-bridge-protocol.md
📚 Learning: 2026-01-19T21:49:05.612Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: runtime/python_bridge.py:99-123
Timestamp: 2026-01-19T21:49:05.612Z
Learning: In the tywrap repository, TYWRAP_REQUEST_MAX_BYTES uses strict integer parsing that rejects values with trailing characters (e.g., "1024abc"). This differs from TYWRAP_CODEC_MAX_BYTES, which uses tolerant/best-effort parsing that accepts numeric prefixes. The strict policy for REQUEST_MAX_BYTES ensures explicit integer values and consistent parse behavior across Node/Python implementations.
Applied to files:
docs/adr/002-bridge-protocol.md
🧬 Code graph analysis (9)
test/worker-pool.test.ts (3)
src/index.ts (6)
Transport(19-19)WorkerPoolOptions(26-26)WorkerPool(26-26)BridgeExecutionError(59-59)BridgeTimeoutError(57-57)PooledWorker(26-26)src/runtime/transport.ts (1)
Transport(115-151)src/runtime/worker-pool.ts (3)
WorkerPoolOptions(21-33)WorkerPool(92-397)PooledWorker(38-44)
src/runtime/safe-codec.ts (1)
src/index.ts (5)
CodecOptions(17-17)isPlainObject(35-35)BridgeProtocolError(56-56)containsSpecialFloat(43-43)BridgeExecutionError(59-59)
test/bridge-protocol.test.ts (3)
src/runtime/transport.ts (3)
Transport(115-151)ProtocolMessage(28-55)ProtocolResponse(63-79)src/runtime/bridge-protocol.ts (2)
BridgeProtocol(70-311)BridgeProtocolOptions(30-39)src/runtime/safe-codec.ts (1)
CodecOptions(22-31)
test/safe-codec.test.ts (2)
src/index.ts (3)
SafeCodec(17-17)BridgeProtocolError(56-56)BridgeExecutionError(59-59)src/runtime/safe-codec.ts (1)
SafeCodec(194-389)
src/runtime/process-io.ts (1)
src/runtime/transport.ts (1)
Transport(115-151)
src/runtime/http-io.ts (1)
src/runtime/transport.ts (1)
Transport(115-151)
runtime/safe_codec.py (2)
test/fixtures/python/fastapi_models.py (1)
Path(22-22)src/runtime/safe-codec.ts (1)
SafeCodec(194-389)
src/runtime/pyodide-io.ts (1)
src/runtime/transport.ts (2)
ProtocolMessage(28-55)ProtocolResponse(63-79)
src/runtime/worker-pool.ts (1)
src/runtime/transport.ts (1)
Transport(115-151)
🪛 Biome (2.1.2)
src/runtime/process-io.ts
[error] 37-37: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 40-40: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 40-40: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 40-40: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 40-40: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 40-40: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 40-40: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🪛 GitHub Check: lint
src/runtime/safe-codec.ts
[warning] 256-256:
'message' is already declared in the upper scope on line 215 column 17
[warning] 163-163:
Function Call Object Injection Sink
[warning] 155-155:
Function Call Object Injection Sink
[warning] 125-125:
Function Call Object Injection Sink
[warning] 105-105:
Function Call Object Injection Sink
src/runtime/http-io.ts
[warning] 206-206:
'message' is already declared in the upper scope on line 125 column 14
src/runtime/pyodide-io.ts
[warning] 18-18:
'BridgeDisposedError' is defined but never used
🪛 LanguageTool
docs/adr/002-bridge-protocol.md
[grammar] ~34-~34: Use a hyphen to join words.
Context: ...ion layer that standardizes all boundary crossing concerns by combining: 1. **Bo...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.13)
runtime/safe_codec.py
22-22: Unnecessary pass statement
Remove unnecessary pass
(PIE790)
25-25: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
43-43: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
57-57: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
79-79: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
115-115: Boolean-typed positional argument in function definition
(FBT001)
115-115: Boolean default positional argument in function definition
(FBT002)
128-128: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
154-156: Avoid specifying long messages outside the exception class
(TRY003)
157-157: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Avoid specifying long messages outside the exception class
(TRY003)
164-164: Avoid specifying long messages outside the exception class
(TRY003)
168-168: Dynamically typed expressions (typing.Any) are disallowed in decode
(ANN401)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
191-191: Avoid specifying long messages outside the exception class
(TRY003)
193-193: Dynamically typed expressions (typing.Any) are disallowed in obj
(ANN401)
193-193: Dynamically typed expressions (typing.Any) are disallowed in _default_encoder
(ANN401)
215-217: Avoid specifying long messages outside the exception class
(TRY003)
284-286: Avoid specifying long messages outside the exception class
(TRY003)
289-291: Avoid specifying long messages outside the exception class
(TRY003)
311-311: Dynamically typed expressions (typing.Any) are disallowed in value
(ANN401)
331-331: Dynamically typed expressions (typing.Any) are disallowed in decode
(ANN401)
test/python/test_safe_codec.py
221-221: Boolean positional value in function call
(FBT003)
323-323: datetime.datetime() called without a tzinfo argument
(DTZ001)
330-330: datetime.datetime() called without a tzinfo argument
(DTZ001)
🔇 Additional comments (47)
src/runtime/worker-pool.ts (5)
1-12: LGTM - Clean module setup and imports.The file header, imports, and type definitions are well-structured. The module correctly imports from
bounded-context.js,errors.js, andtransport.js.
102-119: Constructor validation is solid.Proper validation of required options with clear error messages. Defaults for
queueTimeoutMs(30000) andmaxConcurrentPerWorker(1) are sensible.
140-166: doDispose handles cleanup correctly.The disposal logic properly:
- Clears all waiter timers before rejecting
- Uses AggregateError for multiple disposal errors
- Clears internal state arrays
318-340: Timer handling in waitForWorker is correct.The implementation properly:
- Removes the waiter from queue on timeout before rejecting
- Uses
timer.unref()to prevent keeping the Node.js process alive- Includes proper type checking for
unrefavailabilityThis aligns with the learning about clearing timers to prevent leaks.
346-397: RuntimeExecution stubs are intentional and correctly documented.Per the learnings,
WorkerPoolimplements only theTransportmanagement layer, notRuntimeExecution. These stub methods correctly throwBridgeExecutionErrorwith clear guidance to usewithWorker()instead. This aligns with the ADR-002 architecture.test/bridge-protocol.test.ts (5)
49-129: MockTransport is well-designed for testing.The mock properly implements all
Transportinterface methods and provides useful helpers (setDynamicResponse,setErrorResponse) for configuring test scenarios. TheisReadygetter correctly reflects lifecycle state.
173-235: Constructor tests provide good coverage.Tests verify required options, optional codec settings, and default timeout behavior. The verification through codec behavior (line 198-199) is a pragmatic approach.
596-617: Error response handling test validates BridgeExecutionError structure.The test at lines 596-608 correctly verifies that Python error responses are converted to
BridgeExecutionErrorwith the expected message format ({type}: {message}) and traceback preservation.
1003-1038: Concurrent worker isolation test is valuable.This test verifies that with
maxConcurrentPerWorker: 1, concurrent requests use separate transports. The use of identified transports and timing delays effectively demonstrates pool behavior under concurrent load.
1117-1135: Unicode handling test covers important i18n cases.Testing emoji, Chinese, and Arabic characters ensures the codec handles multi-byte UTF-8 correctly across the boundary.
docs/adr/002-bridge-protocol.md (3)
40-75: Architecture diagram is clear and well-formatted.The
textlanguage identifier addresses the previous markdown lint issue. The ASCII diagram effectively illustrates the layered architecture with SafeCodec, Transport implementations, and optional WorkerPool.
1128-1169: Issue mapping tables are comprehensive and valuable.The tables clearly link issues to their solutions in each layer (SafeCodec, Transport, WorkerPool). This provides excellent traceability for the ~30 issues being addressed.
1206-1226: Consequences section provides balanced assessment.The positive, negative, and neutral consequences are realistic and help stakeholders understand the trade-offs of this architectural decision.
test/worker-pool.test.ts (6)
25-53: MockTransport implementation is appropriate for testing.The mock properly tracks init/dispose state, supports configurable delays and failures, and implements the full Transport interface including
send().
560-583: Important distinction documented for sequential vs concurrent acquires.The test and comments at lines 560-583 correctly document that with sequential
awaitcalls, inFlightCount is visible to subsequent acquires, enabling proper worker reuse. This contrasts with the concurrent test at lines 648-674 which documents race condition behavior.
648-674: Concurrent acquire race condition is properly documented and tested.The test acknowledges that
Promise.allconcurrent acquires may see stale state before any inFlightCount increments, potentially creating more workers than strictly necessary. The assertions correctly usetoBeGreaterThanOrEqual(2)andtoBeLessThanOrEqual(6)to account for this expected behavior.
691-715: Timeout test correctly uses fake timers.The test properly:
- Uses
vi.useFakeTimers()andvi.useRealTimers()in afterEach- Advances time past the configured timeout
- Verifies
BridgeTimeoutErroris thrown- Releases the worker to allow proper cleanup
999-1027: Edge case: release before dispose resolves waiter.This test documents important timing behavior - if
release()is called beforedispose()completes, the waiting acquire resolves successfully rather than being rejected. The test correctly handles this scenario.
1245-1289: Runtime execution stub tests verify correct error behavior.Tests confirm that
call(),instantiate(),callMethod(), anddisposeInstance()all throwBridgeExecutionErroras expected, since WorkerPool doesn't implement RuntimeExecution directly.runtime/safe_codec.py (7)
1-17: Module docstring and imports are clean.The imports cover all necessary standard library modules and type hints. The docstring clearly explains the module's purpose.
210-218: Post-extraction NaN/Inf validation for numpy scalars is correct.Per the learnings, Arrow format decoders (and numpy scalar extraction via
.item()) can produce NaN/Infinity values that weren't in the original JSON. This validation after extraction (lines 214-218) correctly protects downstream consumers.
220-234: Pandas scalar handling covers common cases.The handling of
pd.NaT,pd.Timestamp, andpd.Timedeltais appropriate:
- NaT →
None(consistent with null representation)- Timestamp → ISO format string
- Timedelta → total seconds (consistent with Python timedelta)
262-268: Bytes encoding with type marker enables round-trip.The
__type__: 'bytes'marker with base64 encoding allows the JS side to detect and decode binary data correctly.
270-276: Pydantic model_dump with fallback for older versions.The try/except pattern handles both Pydantic v2 (
mode='json') and older versions gracefully.
278-286: Sets and complex numbers handled correctly.
- Sets/frozensets converted to lists (JSON-compatible)
- Complex numbers explicitly rejected with clear error message
311-328: Module-level encode() creates new codec when allow_nan differs.The
encode()function correctly creates a newSafeCodecinstance whenallow_nan=Trueis requested, rather than using the default codec which hasallow_nan=False. This ensures correct behavior without mutating the shared default.src/runtime/bridge-protocol.ts (5)
70-96: BridgeProtocol constructor properly composes components.The constructor correctly:
- Initializes SafeCodec with provided options
- Stores transport reference
- Sets default timeout (30000ms)
- Tracks transport as a resource for automatic cleanup
108-122: Lifecycle methods are correctly structured.
doInit()properly delegates totransport.init()doDispose()is intentionally empty since transport is tracked and disposed by BoundedContext- Comments guide subclass implementations
145-168: sendMessage implements the core request-response flow.The method correctly:
- Generates unique ID
- Encodes via SafeCodec (with validation)
- Sends via transport with timeout support
- Decodes response synchronously
- Wraps in
execute()for bounded execution semantics
184-207: sendMessageAsync provides Arrow decoding support.This method mirrors
sendMessagebut usesdecodeResponseAsyncfor Arrow-encoded data structures (DataFrames, ndarrays). This is the correct pattern for responses that may contain binary-encoded data.
233-310: RuntimeExecution interface implementation is complete.All four methods (
call,instantiate,callMethod,disposeInstance) correctly:
- Use
sendMessageAsyncfor Arrow support- Pass appropriate protocol message structure
- Support kwargs for all applicable operations
- Include proper JSDoc documentation
test/python/test_safe_codec.py (1)
57-116: Solid baseline round‑trip coverage.The basic round‑trip and JSON validity checks look comprehensive and align well with expected SafeCodec behavior.
src/index.ts (1)
12-26: Export surface looks consistent.The new BridgeProtocol, SafeCodec, Transport, and WorkerPool exports are clear and align with the intended public API.
src/runtime/http-io.ts (4)
1-29: LGTM!Clean module header and well-documented
HttpIOOptionsinterface. The optional properties with sensible defaults (30s timeout) align with the Transport contract.
61-75: LGTM!Constructor correctly normalizes URL and merges headers with appropriate defaults. The implementation properly follows the Transport pattern from learnings.
144-158: LGTM!Correct handling of external abort signal with proper early-abort check and cleanup registration. The
{ once: true }option is a good practice.
210-220: LGTM!Proper cleanup in
finallyblock ensures timeout and abort listener are always cleaned up, preventing timer leaks. This aligns with the learned pattern from PR#136about clearing timers in all paths.src/runtime/transport.ts (4)
1-55: LGTM!Well-documented protocol types with clear discriminated union for message types. The optional fields are appropriately marked based on operation type.
57-79: LGTM!Clean response structure with proper documentation. The error shape with optional traceback provides good debugging capability while keeping the contract simple.
115-151: LGTM!The Transport interface is well-designed with clear lifecycle semantics and comprehensive JSDoc. The contract aligns with the ADR-002 architecture where Transport handles only I/O while BridgeProtocol handles RuntimeExecution methods. Based on learnings, this separation is intentional.
233-254: LGTM!The
isProtocolResponseguard correctly validates the response structure, including proper validation of the nested error object when present. The comment on line 244 accurately describes the valid states.src/runtime/pyodide-io.ts (6)
68-133: LGTM!The bootstrap Python code is well-structured with proper error handling and traceback capture. The dispatch function correctly routes all protocol message types and handles edge cases (unknown type, missing id).
164-178: LGTM!Constructor correctly initializes with defaults and freezes the packages array to prevent mutation. Extending
BoundedContextprovides proper lifecycle management.
195-211: LGTM!Initialization sequence is correct: resolve loader → load Pyodide → load packages → bootstrap dispatcher. The error message for missing Pyodide is clear and actionable.
248-297: LGTM!The
sendmethod correctly usesBoundedContext.execute()for lifecycle protection. Good validation of JSON parsing and required fields. The proxy cleanup infinallyis essential for preventing Pyodide memory leaks.
402-421: LGTM!Good fallback strategy: check global first (browser script tag), then try dynamic import. Silent catch for import failure is appropriate since Pyodide may not be available in all environments.
464-472: LGTM!Proper defensive proxy cleanup with try-catch to ignore failures. This prevents memory leaks from Pyodide proxy objects.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Throw BridgeDisposedError (not BridgeExecutionError) when transport is disposed - Fix timeout semantics: 0 = no timeout (per interface contract), negative = use default - Rename shadowed variable to errorMessage for clarity - Update test to expect BridgeDisposedError Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
Summary
Adds Architecture Decision Record for BridgeProtocol - a unified abstraction combining:
Issue Mapping
The ADR maps ~30 open issues to specific architectural components:
Migration Plan
5-week phased approach:
Test plan
Closes #146, #148
🤖 Generated with Claude Code