feat(runtime): complete ADR-002 bridge migration to BridgeProtocol #153
Conversation
Migrate all three bridges (NodeBridge, HttpBridge, PyodideBridge) to extend the new BridgeProtocol base class, completing the ADR-002 implementation. Key changes: - NodeBridge: Uses PooledTransport for process pool management, simplified from 975+ lines by delegating transport concerns to ProcessIO - HttpBridge: Streamlined to use HttpIO transport with SafeCodec validation - PyodideBridge: Uses PyodideIO transport with unified dispatch handler New components: - PooledTransport: Semaphore-based worker pool for concurrent process management - Enhanced ProcessIO: EPIPE error handling, graceful shutdown, backpressure Test updates: - Updated all Pyodide tests to use new setMockDispatchHandler pattern - Fixed config tests to use resolvedOptions and transport properties - Skipped legacy stats tests (processSpawns, processDeaths) that don't apply to new architecture - Fixed protocol error message patterns Breaking changes (pre-1.0): - getBridgeInfo() method removed (protocol info no longer exposed) - Internal processPool not accessible (managed by PooledTransport) - Stats structure simplified (legacy counters removed) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
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. 📝 WalkthroughWalkthroughThis PR introduces a protocol envelope (PROTOCOL_ID, numeric ids, method+params), adds a pooled transport and worker-pool lifecycle, and migrates NodeBridge, PyodideBridge, and HttpBridge to a BridgeProtocol/transport-based architecture; tests and examples updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Protocol as BridgeProtocol
participant Transport as PooledTransport
participant Pool as WorkerPool
participant Worker as ProcessIO
participant Runtime as Python
Client->>Protocol: call(module, fn, args)
Protocol->>Protocol: generateId() → number
Protocol->>Protocol: build message { id, protocol, method, params }
Protocol->>Transport: send(message)
Transport->>Pool: acquireWorker()
alt existing worker
Pool-->>Transport: worker
else create worker
Pool->>Worker: create/init
Worker->>Runtime: spawn process
Runtime-->>Worker: ready
Pool-->>Transport: worker
end
Transport->>Worker: send(serialized message)
Worker->>Runtime: write stdin (message)
Runtime->>Worker: stdout response
Worker->>Worker: extractMessageId(response)
Worker-->>Transport: response
Transport->>Pool: releaseWorker()
Transport-->>Protocol: response
Protocol->>SafeCodec: decodeResponse(response)
SafeCodec-->>Protocol: inner result
Protocol-->>Client: resolved value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
coderabbitai
bot
added
area:runtime-node
- Remove getBridgeInfo check from living-app example (Arrow will fail at decode time if pyarrow is not installed) - Remove BridgeInfo type test (method was removed in bridge migration) - Fix HttpBridge instantiation in type test to provide required baseURL Co-Authored-By: Claude Opus 4.5 <[email protected]>
![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
tywrap/src/runtime/pyodide-io.ts
Lines 86 to 90 in e00e532
Update Pyodide bootstrap for new protocol
The embedded __tywrap_dispatch still parses msg['type'] and expects top‑level fields like module/functionName, but BridgeProtocol now sends method/params with a protocol header. In a browser/Pyodide runtime this means every call will throw (missing type) and return an error response, effectively breaking PyodideBridge for all operations. The bootstrap needs to accept the new schema (or translate the message before dispatch) to keep Pyodide execution functional.
ℹ️ 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".
The NodeBridge `cwd` option was being resolved but not passed to Python environment. This caused modules in the working directory to not be importable. Now the `cwd` is prepended to PYTHONPATH so Python can find modules in the specified working directory. Co-Authored-By: Claude Opus 4.5 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/runtime/safe-codec.ts (1)
389-395: Validate special floats on the decoded value, not the pre-decoded result.Line 389 currently checks
result, so NaN/Infinity introduced by Arrow decoding can slip through. Validatedecodedinstead. Based on learnings, post-decode validation is required to catch special floats introduced by Arrow decoders.🛠️ Proposed fix
- if (this.rejectSpecialFloats && containsSpecialFloat(result)) { - const floatPath = findSpecialFloatPath(result); + if (this.rejectSpecialFloats && containsSpecialFloat(decoded)) { + const floatPath = findSpecialFloatPath(decoded); throw new BridgeProtocolError( `Response contains non-finite number (NaN or Infinity) at ${floatPath}` ); }src/runtime/process-io.ts (1)
386-397: Fix environment inheritance logic: envOverrides should be merged with process.env, not replace it.When
inheritProcessEnvis false (the default),buildProcessEnv()deliberately returns a minimal filtered environment containing only PATH, PYTHON*, TYWRAP_*, and user overrides. ProcessIO then receives this minimal environment and incorrectly treats it as a complete base environment, replacingprocess.enventirely.This breaks subprocess execution because critical environment variables (HOME, USER, LD_LIBRARY_PATH, LOCALE, etc.) are missing. The ProcessIO code should merge
envOverrideswithprocess.envrather than using it as a complete replacement:const baseEnv = { ...process.env, ...this.envOverrides };This ensures essential system variables are always available while still allowing
envOverridesto override specific values.src/runtime/pyodide-io.ts (1)
68-133: Critical: Python dispatch code doesn't match the new protocol message shape.The bootstrap Python code expects the old flat message structure with
msg['type'],msg['module'],msg['functionName'], etc. However, the TypeScript side now sends the new protocol envelope withmethodand nestedparams:# Python expects: msg_type = msg['type'] # Line 88 mod = importlib.import_module(msg['module']) # Line 91 fn = getattr(mod, msg['functionName']) # Line 92 # TypeScript sends: { protocol: 'tywrap/1', method: 'call', params: { module: '...', functionName: '...' } }This mismatch will cause all Pyodide calls to fail with
KeyError.🐛 Proposed fix for Python dispatch code
def __tywrap_dispatch(message_json): msg = None try: msg = json.loads(message_json) msg_id = msg['id'] - msg_type = msg['type'] + msg_method = msg['method'] + params = msg.get('params', {}) - if msg_type == 'call': - mod = importlib.import_module(msg['module']) - fn = getattr(mod, msg['functionName']) - result = fn(*msg.get('args', []), **msg.get('kwargs', {})) + if msg_method == 'call': + mod = importlib.import_module(params['module']) + fn = getattr(mod, params['functionName']) + result = fn(*params.get('args', []), **params.get('kwargs', {})) return json.dumps({'id': msg_id, 'result': result}) - elif msg_type == 'instantiate': - mod = importlib.import_module(msg['module']) - cls = getattr(mod, msg['className']) - obj = cls(*msg.get('args', []), **msg.get('kwargs', {})) + elif msg_method == 'instantiate': + mod = importlib.import_module(params['module']) + cls = getattr(mod, params['className']) + obj = cls(*params.get('args', []), **params.get('kwargs', {})) handle = str(id(obj)) __tywrap_instances[handle] = obj return json.dumps({'id': msg_id, 'result': handle}) - elif msg_type == 'call_method': - obj = __tywrap_instances[msg['handle']] - method = getattr(obj, msg['methodName']) - result = method(*msg.get('args', []), **msg.get('kwargs', {})) + elif msg_method == 'call_method': + obj = __tywrap_instances[params['handle']] + method = getattr(obj, params['methodName']) + result = method(*params.get('args', []), **params.get('kwargs', {})) return json.dumps({'id': msg_id, 'result': result}) - elif msg_type == 'dispose_instance': - __tywrap_instances.pop(msg['handle'], None) + elif msg_method == 'dispose_instance': + __tywrap_instances.pop(params['handle'], None) return json.dumps({'id': msg_id, 'result': None}) else: return json.dumps({ 'id': msg_id, 'error': { 'type': 'ValueError', - 'message': f'Unknown message type: {msg_type}' + 'message': f'Unknown message method: {msg_method}' } })test/transport.test.ts (1)
199-202: Test override{ type: 'call' }has no effect.The
createValidMessage()helper creates messages withmethod: 'call'(nottype). The override{ type: 'call' }adds an extraneous field that doesn't affect the test. The test still passes because the base message already hasmethod: 'call', but the override is misleading.💚 Proposed fix
it('returns true for valid call message', () => { - const msg = createValidMessage({ type: 'call' }); + const msg = createValidMessage(); expect(isProtocolMessage(msg)).toBe(true); });
🤖 Fix all issues with AI agents
In `@src/runtime/http.ts`:
- Line 11: Remove the unused type import HttpIOOptions from the import statement
that currently reads "import { HttpIO, type HttpIOOptions } from
'./http-io.js';" so only HttpIO is imported; ensure no other references to
HttpIOOptions exist in this file and run the type checker to confirm the removal
is safe.
In `@src/runtime/node.ts`:
- Around line 73-77: The warmupCommands are currently executed once via
runWarmupCommands and not per worker; change the initialization so each worker
runs them when it is created: invoke the existing runWarmupCommands logic from
the per-worker initialization path (e.g., the worker spawn callback in
PooledTransport / WorkerPool or from ProcessIO construction) instead of only
calling it once from the pooled bridge; ensure the call accepts the same
warmupCommands shape (both {module,functionName,args} and legacy
{method,params}) and preserves error handling/logging so every newly spawned
ProcessIO executes warmupCommands on startup.
- Around line 33-42: The NodeBridgeOptions.minProcesses field is defined but
never propagated into the pool options; update the options plumbing so
minProcesses is mapped into the pool's minWorkers (or minWorkers name expected
by PooledTransportOptions) and used when constructing ResolvedOptions and
PooledTransport; specifically, in the options resolution code that builds
ResolvedOptions and where PooledTransport is instantiated (symbols:
NodeBridgeOptions.minProcesses, ResolvedOptions, PooledTransport,
PooledTransportOptions/minWorkers), pass minProcesses → minWorkers (or the
pool's minWorkers key), defaulting consistently with existing defaults, so the
pool can pre-warm to the requested minimum.
- Around line 20-24: Remove the unused type imports ProcessIOOptions and
PooledTransportOptions from the import list in src/runtime/node.ts (currently
imported alongside ProcessIO and PooledTransport) to satisfy the linter; edit
the import lines for ProcessIO and PooledTransport to only import the runtime
symbols (or alternatively update code to actually reference those types in type
annotations if they are intended to be used).
- Around line 246-250: The ProcessIO is being constructed without the resolved
working directory, so the spawned Python process uses process.cwd() instead of
the user-specified cwd; update the ProcessIO instantiation (the new
ProcessIO(...) call) to include the resolved cwd (e.g., pass cwd:
resolvedOptions.cwd or resolvedCwd) in the options object so ProcessIO uses that
directory when spawning the subprocess.
In `@src/runtime/pooled-transport.ts`:
- Line 14: Remove the unused named import WorkerPoolOptions from the import
specifier that currently imports { WorkerPool, type WorkerPoolOptions } in
pooled-transport.ts; keep only WorkerPool (e.g., change the import to import {
WorkerPool } from './worker-pool.js') so the unused type is no longer imported
and the linter/static analysis error is resolved.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
docs/adr/002-bridge-protocol.mdsrc/index.tssrc/runtime/bridge-protocol.tssrc/runtime/http.tssrc/runtime/node.tssrc/runtime/pooled-transport.tssrc/runtime/process-io.tssrc/runtime/pyodide-io.tssrc/runtime/pyodide.tssrc/runtime/safe-codec.tssrc/runtime/transport.tstest/arch-stories.test.tstest/bridge-protocol.test.tstest/optimized-node.test.tstest/runtime_bridge_fixtures.test.tstest/runtime_config.test.tstest/runtime_node.test.tstest/runtime_pyodide.test.tstest/transport.test.ts
🧰 Additional context used
🧠 Learnings (21)
📓 Common learnings
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-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: 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.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In the tywrap repository, bridges currently use implicit ready detection: the first successful response from the Python subprocess proves the bridge is ready to handle requests.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 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-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:
docs/adr/002-bridge-protocol.mdsrc/runtime/pooled-transport.tssrc/runtime/pyodide.tssrc/index.tssrc/runtime/bridge-protocol.tssrc/runtime/transport.tssrc/runtime/http.tssrc/runtime/pyodide-io.ts
📚 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-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.mdtest/runtime_node.test.tstest/optimized-node.test.tssrc/runtime/node.ts
📚 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: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/bridge-protocol.test.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
src/runtime/safe-codec.tstest/runtime_node.test.tstest/runtime_bridge_fixtures.test.tssrc/runtime/process-io.tstest/runtime_pyodide.test.tstest/optimized-node.test.tstest/runtime_config.test.tssrc/runtime/pyodide.tssrc/index.tssrc/runtime/node.tssrc/runtime/bridge-protocol.tstest/bridge-protocol.test.tssrc/runtime/http.ts
📚 Learning: 2026-01-19T21:14:35.390Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:35.390Z
Learning: In src/runtime/bridge-core.ts and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (such as runtime, shape, or error-payload checks) inside tight loops unless the protocol boundary is untrusted or there is a concrete bug report. The Python bridge protocol is controlled via tests, so these checks add unnecessary branching overhead without meaningful benefit. Apply this guidance to other hot-path runtime loops in src/runtime/**/*.ts, and re-enable additional validations only when a documented risk or failure scenario is identified. Ensure tests cover protocol validation where applicable.
Applied to files:
src/runtime/safe-codec.tssrc/runtime/process-io.tssrc/runtime/pooled-transport.tssrc/runtime/pyodide.tssrc/runtime/node.tssrc/runtime/bridge-protocol.tssrc/runtime/transport.tssrc/runtime/http.tssrc/runtime/pyodide-io.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/pooled-transport.tssrc/runtime/pyodide.tssrc/runtime/node.tssrc/runtime/bridge-protocol.tssrc/runtime/transport.tssrc/runtime/http.tssrc/runtime/pyodide-io.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
src/runtime/safe-codec.tstest/runtime_node.test.tstest/runtime_bridge_fixtures.test.tssrc/runtime/process-io.tssrc/runtime/pooled-transport.tstest/runtime_pyodide.test.tstest/optimized-node.test.tstest/runtime_config.test.tstest/arch-stories.test.tssrc/runtime/pyodide.tssrc/index.tstest/transport.test.tssrc/runtime/node.tssrc/runtime/bridge-protocol.tssrc/runtime/transport.tstest/bridge-protocol.test.tssrc/runtime/http.tssrc/runtime/pyodide-io.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
src/runtime/safe-codec.tstest/runtime_node.test.tstest/runtime_bridge_fixtures.test.tssrc/runtime/process-io.tssrc/runtime/pooled-transport.tstest/runtime_pyodide.test.tstest/optimized-node.test.tstest/runtime_config.test.tstest/arch-stories.test.tssrc/runtime/pyodide.tssrc/index.tstest/transport.test.tssrc/runtime/node.tssrc/runtime/bridge-protocol.tssrc/runtime/transport.tstest/bridge-protocol.test.tssrc/runtime/http.tssrc/runtime/pyodide-io.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
src/runtime/safe-codec.tstest/runtime_node.test.tstest/runtime_bridge_fixtures.test.tssrc/runtime/process-io.tssrc/runtime/pooled-transport.tstest/runtime_pyodide.test.tstest/optimized-node.test.tstest/runtime_config.test.tstest/arch-stories.test.tssrc/runtime/pyodide.tssrc/index.tstest/transport.test.tssrc/runtime/node.tssrc/runtime/bridge-protocol.tssrc/runtime/transport.tstest/bridge-protocol.test.tssrc/runtime/http.tssrc/runtime/pyodide-io.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/runtime_node.test.tstest/runtime_bridge_fixtures.test.tstest/runtime_pyodide.test.tstest/optimized-node.test.tstest/runtime_config.test.tssrc/index.tstest/transport.test.tstest/bridge-protocol.test.ts
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/runtime_node.test.tstest/runtime_bridge_fixtures.test.tstest/runtime_pyodide.test.tstest/optimized-node.test.tstest/runtime_config.test.tstest/arch-stories.test.tstest/transport.test.tstest/bridge-protocol.test.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
test/runtime_node.test.tstest/runtime_bridge_fixtures.test.tssrc/runtime/process-io.tstest/runtime_pyodide.test.tstest/optimized-node.test.tstest/runtime_config.test.tssrc/runtime/pyodide.tssrc/index.tssrc/runtime/node.tssrc/runtime/bridge-protocol.tstest/bridge-protocol.test.tssrc/runtime/http.ts
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/runtime_node.test.tstest/runtime_bridge_fixtures.test.tstest/runtime_pyodide.test.tstest/optimized-node.test.tstest/runtime_config.test.tstest/bridge-protocol.test.ts
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: In `test/fixtures/out_of_order_bridge.py`, the fixture intentionally leaves a pending request unanswered at EOF to simulate missing/out-of-order responses and validate bridge behavior when requests never complete; this is the exact failure mode being tested and must be preserved.
Applied to files:
test/runtime_node.test.tstest/runtime_bridge_fixtures.test.tstest/optimized-node.test.ts
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: Policy for adversarial test fixtures: adversarial fixtures must preserve the failure mode they exercise, even if that behavior would be "bad" in real code. When reviewing test fixtures with unusual patterns (e.g., missing error handling, intentional protocol violations, unanswered requests), ask for clarifying comments about the intent rather than suggesting normalization or fixes.
Applied to files:
test/runtime_bridge_fixtures.test.ts
📚 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/optimized-node.test.tstest/bridge-protocol.test.ts
📚 Learning: 2026-01-20T01:36:14.701Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:743-759
Timestamp: 2026-01-20T01:36:14.701Z
Learning: In the tywrap repository, when reviewing concurrent request handling patterns (especially with BridgeCore request multiplexing), flag uses of boolean `busy` flags that should be request counters instead. A boolean flag creates a race condition: when request A completes and sets `busy = false`, request B (still in flight via multiplexing) is incorrectly treated as idle. The fix is to use an `inFlightRequests: number` counter that increments on request start and decrements on completion, then check `worker.inFlightRequests === 0` before cleanup/termination.
Applied to files:
test/optimized-node.test.tstest/bridge-protocol.test.ts
📚 Learning: 2026-01-19T21:14:37.032Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Applied to files:
test/bridge-protocol.test.ts
🧬 Code graph analysis (11)
src/runtime/pooled-transport.ts (1)
src/runtime/transport.ts (1)
Transport(132-168)
test/runtime_pyodide.test.ts (1)
src/runtime/pyodide.ts (1)
PyodideBridge(66-88)
test/runtime_config.test.ts (1)
src/runtime/node.ts (1)
NodeBridge(208-464)
test/arch-stories.test.ts (2)
src/index.ts (1)
HttpBridge(68-68)src/runtime/http.ts (1)
HttpBridge(63-86)
src/runtime/pyodide.ts (3)
src/runtime/safe-codec.ts (1)
CodecOptions(22-31)src/runtime/bridge-protocol.ts (2)
BridgeProtocol(70-319)BridgeProtocolOptions(30-39)src/runtime/pyodide-io.ts (1)
PyodideIO(164-485)
test/transport.test.ts (1)
src/runtime/transport.ts (4)
PROTOCOL_ID(20-20)isProtocolMessage(229-244)ProtocolResponse(77-96)isProtocolResponse(252-273)
src/runtime/node.ts (10)
src/runtime/optimized-node.ts (2)
NodeBridgeOptions(21-21)NodeBridge(21-21)src/runtime/safe-codec.ts (1)
CodecOptions(22-31)src/runtime/bridge-core.ts (1)
getPathKey(364-372)src/runtime/bridge-protocol.ts (2)
BridgeProtocol(70-319)BridgeProtocolOptions(30-39)src/runtime/pooled-transport.ts (1)
PooledTransport(71-236)src/utils/python.ts (1)
getDefaultPythonPath(43-45)src/runtime/process-io.ts (1)
ProcessIO(134-783)src/utils/codec.ts (1)
autoRegisterArrowDecoder(165-194)src/utils/cache.ts (1)
globalCache(634-634)tywrap_ir/tywrap_ir/optimized_ir.py (1)
duration(44-46)
src/runtime/bridge-protocol.ts (1)
src/runtime/transport.ts (2)
ProtocolMessage(36-69)PROTOCOL_ID(20-20)
src/runtime/transport.ts (1)
src/index.ts (2)
PROTOCOL_ID(20-20)ProtocolMessage(19-19)
test/bridge-protocol.test.ts (3)
src/index.ts (3)
ProtocolResponse(19-19)BridgeProtocolError(58-58)SafeCodec(17-17)src/runtime/transport.ts (1)
ProtocolResponse(77-96)src/runtime/safe-codec.ts (1)
SafeCodec(213-413)
src/runtime/http.ts (2)
src/runtime/safe-codec.ts (1)
CodecOptions(22-31)src/runtime/bridge-protocol.ts (2)
BridgeProtocol(70-319)BridgeProtocolOptions(30-39)
🪛 GitHub Actions: CI
src/index.ts
[error] 127-127: TS2339: Property 'getBridgeInfo' does not exist on type 'NodeBridge'.
🪛 GitHub Check: lint
src/runtime/pooled-transport.ts
[warning] 14-14:
'WorkerPoolOptions' is defined but never used
src/runtime/node.ts
[warning] 23-23:
'PooledTransportOptions' is defined but never used
[warning] 22-22:
'ProcessIOOptions' is defined but never used
[warning] 245-245:
Missing return type on function
[warning] 509-509:
Generic Object Injection Sink
[warning] 508-508:
Generic Object Injection Sink
[warning] 499-499:
Generic Object Injection Sink
[warning] 491-491:
Generic Object Injection Sink
[warning] 482-482:
Generic Object Injection Sink
src/runtime/http.ts
[warning] 11-11:
'HttpIOOptions' is defined but never used
🔇 Additional comments (58)
docs/adr/002-bridge-protocol.md (1)
5-14: Status/progress update looks consistent with the migration completion.test/arch-stories.test.ts (1)
462-464: HttpBridge construction updated correctly for the new options contract.test/runtime_bridge_fixtures.test.ts (2)
49-65: Updated protocol error patterns align with the new error surface.
198-200: Skipping getBridgeInfo parity aligns with the API removal.src/runtime/safe-codec.ts (1)
145-163: Envelope result extraction is clear and keeps validation on the inner payload.Also applies to: 326-337
test/runtime_node.test.ts (2)
133-156: Skipping getBridgeInfo coverage matches the removal in the new architecture.
703-705: Protocol error expectations updated correctly for the new surface.Also applies to: 742-744
test/optimized-node.test.ts (2)
312-315: Skipping legacy pool/stat tests is consistent with the new PooledTransport internals.Also applies to: 413-415, 448-450, 483-485
408-410: Relaxed serialization error expectation is appropriate for BigInt failures.src/index.ts (3)
20-20: LGTM: PROTOCOL_ID export enables external protocol validation.Exporting the protocol identifier allows consumers to validate incoming messages match the expected protocol version.
27-28: LGTM: PooledTransport public export.Exposing
PooledTransportandPooledTransportOptionsenables advanced users to create custom pool configurations for worker management.
65-68: LGTM: Bridge exports with options types.The bridge classes are now properly exported with their corresponding options types, providing a complete public API surface for the new BridgeProtocol architecture.
test/runtime_config.test.ts (4)
42-49: LGTM: Test adapted for resolvedOptions access pattern.The test correctly accesses internal
resolvedOptionsfor verification. UsingtoContain('python_bridge.py')is more robust than exact path matching across different environments.
65-79: LGTM: Custom configuration test updated.The test properly verifies resolved options and correctly documents that
enableJsonFallbackis deprecated and not stored in options.
151-156: LGTM: Custom Pyodide configuration test.The test correctly accesses transport properties to verify custom configuration is properly passed through.
135-141: Default indexURL version is consistent across PyodideIO and tests.The default indexURL
'https://cdn.jsdelivr.net/pyodide/v0.24.1/full/'already matches the DEFAULT_INDEX_URL constant in PyodideIO options (src/runtime/pyodide-io.ts, line 58). No changes needed.src/runtime/process-io.ts (5)
99-103: LGTM: Numeric ID extraction aligns with protocol changes.The regex correctly extracts integer IDs from JSON, supporting both positive and negative values. This matches the new
ProtocolMessage.id: numbertype.
152-152: LGTM: Pending map key type updated to number.Consistent with the numeric ID change in
extractMessageId.
441-461: LGTM: Robust shutdown handling prevents EPIPE crashes.The changes add:
- Proactive error handlers before cleanup to suppress EPIPE during shutdown
- Graceful
stdin.end()wrapped in try-catch- Targeted listener removal for
exit/closewhile preserving other cleanupThis aligns with the PR objective of "graceful stdin.end() on shutdown" and prevents unhandled EPIPE errors. Based on learnings, drain-before-restart is the preferred pattern for restarts.
679-693: LGTM: Synchronous EPIPE handling in writeToStdin.The try-catch around
stdin.write()catches synchronous errors (like EPIPE when the process has died) and converts them toBridgeProtocolError. This prevents unhandled exceptions and provides clear error messages.
716-738: LGTM: flushWriteQueue handles synchronous errors and cascades rejection.On write error, the code properly:
- Rejects the current queued write
- Rejects all remaining queued writes with the same error
- Clears the queue to prevent stale entries
This ensures no writes are left dangling after a process failure.
src/runtime/transport.ts (5)
19-20: LGTM: Protocol identifier constant.Using
'tywrap/1'as a versioned protocol ID enables future protocol evolution while maintaining backwards compatibility detection.
36-69: LGTM: Unified ProtocolMessage structure.The new envelope structure with
methodand nestedparamsis cleaner than flat fields. Key improvements:
- Numeric
idsimplifies correlation and avoids string parsing overheadprotocolfield enables version validation- Union type for
methodprovides compile-time safety- Optional fields in
paramsallow method-specific payloadsThis aligns with ADR-002's protocol specification.
77-96: LGTM: ProtocolResponse with optional protocol echo.The response correctly:
- Uses numeric
idfor correlation- Makes
protocoloptional (graceful handling if Python doesn't echo it)- Maintains existing
result/errorunion semantics
237-243: LGTM: isProtocolMessage type guard updated.The guard correctly validates:
- Numeric
id- Exact
PROTOCOL_IDmatch- Valid
methodstring in the allowed set- Non-null
paramsobject
259-261: LGTM: isProtocolResponse validates numeric id.The check ensures response correlation works with the new numeric ID scheme.
test/runtime_pyodide.test.ts (7)
19-39: LGTM: Centralized dispatch mock pattern.The
setMockDispatchHandlerapproach provides:
- Per-test customization of responses
- Default fallback for tests that don't need custom behavior
- Clean separation between mock setup and test logic
This replaces individual function mocks with a unified JSON dispatch pattern matching the production architecture.
46-50: LGTM: Bootstrap detection via __tywrap_dispatch.The mock correctly detects bootstrap code by checking for
def __tywrap_dispatchand sets up the dispatch function. This mirrors the production Pyodide bootstrap behavior.
78-78: LGTM: Dispatch handler reset in beforeEach.Ensures clean state between tests.
207-222: LGTM: Message structure verification.The test correctly verifies the new protocol envelope:
method: 'call'params.module,params.functionName,params.argsThis ensures the bridge produces correctly structured messages.
254-264: LGTM: Error message updated for new architecture.The error message "Pyodide dispatch function not initialized" correctly reflects the new unified dispatch model instead of referencing individual helper functions.
361-363: LGTM: Bootstrap verification updated.The test verifies that bootstrap code includes
def __tywrap_dispatch, confirming the unified dispatch function is set up correctly.
513-528: LGTM: Error response handling test.The test verifies that dispatch returning an error object with
type,message, andtracebackfields correctly propagates as an exception. This matches theProtocolResponse.errorstructure.src/runtime/bridge-protocol.ts (8)
21-21: LGTM: PROTOCOL_ID import.Centralizing the protocol identifier in
transport.tsand importing it here ensures consistency across all protocol-aware code.
145-153: LGTM: Protocol injection in sendMessage.The implementation correctly:
- Accepts messages without
idorprotocol(viaOmit)- Generates a unique numeric ID
- Injects
protocol: PROTOCOL_IDinto the full messageThis ensures all outgoing messages conform to the protocol specification.
185-193: LGTM: Consistent protocol injection in sendMessageAsync.Same pattern as
sendMessage, maintaining consistency for Arrow-enabled responses.
217-219: LGTM: Numeric ID generation.The monotonically increasing counter is simple and efficient. Pre-increment (
++this.requestId) ensures IDs start at 1, avoiding potential edge cases with ID 0.
240-248: LGTM: call() uses new method/params envelope.The message structure correctly uses:
method: 'call'params: { module, functionName, args, kwargs }This matches the
ProtocolMessagetype definition.
266-274: LGTM: instantiate() message structure.Correctly uses
method: 'instantiate'withparams.className.
292-300: LGTM: callMethod() message structure.Correctly uses
method: 'call_method'withparams.handleandparams.methodName.
312-317: LGTM: disposeInstance() message structure.Correctly uses
method: 'dispose_instance'withparams.handle.src/runtime/http.ts (1)
63-85: LGTM!Clean implementation of
HttpBridgeextendingBridgeProtocol. The constructor properly creates theHttpIOtransport and passes both transport and codec options to the base class. The timeout configuration is consistently applied to both the transport and protocol layers.src/runtime/pyodide-io.ts (2)
315-329: LGTM on protocol message construction.The
call()method correctly constructs the new protocol envelope withprotocol: PROTOCOL_ID,method: 'call', and nestedparamsobject. The same pattern is consistently applied across all convenience methods.
466-468: LGTM on numeric ID generation.The
generateId()method correctly returns sequential numeric IDs, aligning with the protocol change from string to number IDs.test/transport.test.ts (3)
67-81: LGTM on updated test helper.The
createValidMessage()helper correctly produces the new protocol envelope shape with numericid,PROTOCOL_ID,method, and nestedparams.
256-299: LGTM on protocol validation tests.Comprehensive test coverage for the new protocol message validation: numeric ID requirement, protocol field presence and value, method field validation, and params object requirement.
1287-1310: LGTM on numeric ID format test.Test correctly verifies that IDs are sequential integers greater than zero.
src/runtime/pooled-transport.ts (2)
71-155: LGTM!Clean implementation of
PooledTransport. The class properly:
- Validates
createTransportis a function in the constructor- Initializes the
WorkerPoollazily indoInit()- Delegates
send()to the pool's worker transports- Exposes useful pool statistics for monitoring
The disposal check in
send()correctly prevents use after disposal.
189-235: LGTM on runtime execution stubs.The stub methods correctly throw
BridgeExecutionErrorwith a clear message directing users to useBridgeProtocolfor operations. This aligns with the ADR-002 architecture where transports only implementsend()andBridgeProtocolhandles the higher-level operations. Based on learnings, this follows the correct separation of concerns.src/runtime/pyodide.ts (1)
66-88: LGTM!Clean implementation of
PyodideBridgeextendingBridgeProtocol. The constructor correctly:
- Creates
PyodideIOtransport withindexURLandpackagesoptions- Passes transport, codec, and timeout options to the base
BridgeProtocolclass- Accepts a default-empty options object for convenience
The implementation is consistent with
HttpBridgeand follows the ADR-002 architecture.test/bridge-protocol.test.ts (4)
336-351: LGTM on protocol message encoding test.Test correctly verifies the new envelope structure with
method: 'call'and nestedparamscontainingmodule,functionName, andargs.
469-491: LGTM oncall()interface test.Test correctly verifies:
- Result extraction from response envelope (SafeCodec behavior)
- Message structure with
method: 'call'and params fields- Support for kwargs in the params object
705-744: LGTM on WorkerPool integration tests.Tests correctly use the new protocol format with
protocol: 'tywrap/1',method: 'call', andparams: {}in the message payloads.
834-854: LGTM on full stack integration test.Comprehensive test correctly validates the complete request-response cycle through the pool with:
- New protocol envelope encoding
- SafeCodec extracting the result from response
- Correct numeric ID usage
src/runtime/node.ts (4)
279-301: Initialization sequence is well ordered.Script existence check, Arrow decoder registration, and transport init before warmups read cleanly.
310-343: Caching override + helper heuristics look solid.The cache key guard and purity heuristic are contained and easy to reason about.
Also applies to: 424-462
349-397: Pool stats surface looks good for backwards compatibility.Deprecation path is clear and keeps legacy callers functional.
131-165: Utility helpers and env construction are clean and predictable.The venv resolution and filtered env passthrough are straightforward and safe.
Also applies to: 473-518
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
coderabbitai
bot
added
area:codec
- Remove unused type imports (HttpIOOptions, ProcessIOOptions, PooledTransportOptions, WorkerPoolOptions) - Add cwd support to ProcessIO for subprocess working directory - Pass cwd from NodeBridge to ProcessIO for proper subprocess isolation Known limitations (documented for future work): - minProcesses option is not wired through to PooledTransport - Warmup commands run once, not per-worker in multi-process scenarios Co-Authored-By: Claude Opus 4.5 <[email protected]>
CodeRabbit Review ResponseFixed in latest commit:✅ Unused imports - Removed ✅ Custom cwd dropped when spawning ProcessIO - Added Known limitations (deferred to future work):⏸️ ⏸️ Warmup commands run once, not per-worker - In multi-process scenarios (
These are valid improvements for a future PR focused on multi-process optimization. |
Responses to Individual CodeRabbit Comments✅ Fixed
⏸️ Deferred (Known Limitations)
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@examples/living-app/src/index.ts`:
- Around line 126-129: The comment next to the codec check is misleading: update
the comment around the if (codec === 'arrow') block to state that
enableArrowDecoder() will throw immediately if the JS-side apache-arrow package
is missing (so the failure is eager), whereas absence of the Python-side pyarrow
would manifest later at decode time; mention both failure modes and reference
enableArrowDecoder and the apache-arrow / pyarrow dependencies so readers
understand which error occurs when.
In `@src/runtime/node.ts`:
- Around line 366-395: getStats currently sets busyWorkers to
poolStats.totalInFlight which counts in-flight requests and can exceed
workerCount when maxConcurrentPerWorker > 1; update getStats to either (a) clamp
busyWorkers to the number of workers by setting busyWorkers =
Math.min(poolStats.totalInFlight, poolStats.workerCount) so it truly reflects
busy worker slots, or (b) if preserving precise request count is desired, rename
the field to busyRequests (and update any consumers) and leave it as
totalInFlight; change references to busyWorkers in getStats and any callers
accordingly (functions: getStats, getPoolStats; properties: totalInFlight,
workerCount, maxConcurrentPerWorker).
- Around line 407-416: The warmup command loop in runtime
(this.resolvedOptions.warmupCommands inside src/runtime/node.ts) currently
silently ignores legacy { method, params } entries; update the loop to detect
legacy entries and either map them to the new shape (e.g., translate { method,
params } -> call(module=?, functionName=method, args=params) or another explicit
mapping you choose) or emit a clear warning/error via the runtime logger when a
legacy entry is encountered (reference this.resolvedOptions.warmupCommands, the
loop handling code, and the call(...) method) so misconfiguration is not
silently skipped; implement the chosen behavior and ensure tests/log messages
reflect the change.
♻️ Duplicate comments (3)
src/runtime/node.ts (3)
20-23: Unused type-only imports still trigger lint.
ProcessIOOptionsandPooledTransportOptionsare still imported but unused; remove or use them to keep lint clean.
33-41:minProcessesis still a no‑op in pool wiring.
The option is documented but never propagated to the pool config, so it has no effect.Also applies to: 244-253
245-250: Customcwdisn’t forwarded to ProcessIO.
resolvedOptions.cwdis computed but not passed toProcessIO, so the subprocess runs inprocess.cwd()even when users setcwd.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
examples/living-app/src/index.tssrc/runtime/node.tstest-d/types.test-d.ts
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
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: 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.
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: 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.
📚 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:
examples/living-app/src/index.tssrc/runtime/node.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:
examples/living-app/src/index.ts
📚 Learning: 2026-01-20T16:00:49.738Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:168-172
Timestamp: 2026-01-20T16:00:49.738Z
Learning: In the tywrap project's BridgeProtocol SafeCodec implementation, Arrow format decoders can produce NaN/Infinity values from binary representations even when the raw JSON payload doesn't contain them. This is why validation for special floats must occur both before encoding (to reject invalid inputs) and after applying decoders (to catch values introduced during Arrow deserialization), protecting downstream consumers from unexpected NaN/Infinity values.
Applied to files:
examples/living-app/src/index.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:
examples/living-app/src/index.tstest-d/types.test-d.tssrc/runtime/node.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
examples/living-app/src/index.tstest-d/types.test-d.tssrc/runtime/node.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
examples/living-app/src/index.tstest-d/types.test-d.tssrc/runtime/node.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:
examples/living-app/src/index.tstest-d/types.test-d.tssrc/runtime/node.ts
📚 Learning: 2026-01-19T21:14:35.390Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:35.390Z
Learning: In src/runtime/bridge-core.ts and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (such as runtime, shape, or error-payload checks) inside tight loops unless the protocol boundary is untrusted or there is a concrete bug report. The Python bridge protocol is controlled via tests, so these checks add unnecessary branching overhead without meaningful benefit. Apply this guidance to other hot-path runtime loops in src/runtime/**/*.ts, and re-enable additional validations only when a documented risk or failure scenario is identified. Ensure tests cover protocol validation where applicable.
Applied to files:
src/runtime/node.ts
📚 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/node.ts
📚 Learning: 2026-01-20T01:36:14.701Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:743-759
Timestamp: 2026-01-20T01:36:14.701Z
Learning: In the tywrap repository, when reviewing concurrent request handling patterns (especially with BridgeCore request multiplexing), flag uses of boolean `busy` flags that should be request counters instead. A boolean flag creates a race condition: when request A completes and sets `busy = false`, request B (still in flight via multiplexing) is incorrectly treated as idle. The fix is to use an `inFlightRequests: number` counter that increments on request start and decrements on completion, then check `worker.inFlightRequests === 0` before cleanup/termination.
Applied to files:
src/runtime/node.ts
📚 Learning: 2026-01-20T01:33:12.841Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:238-249
Timestamp: 2026-01-20T01:33:12.841Z
Learning: In the tywrap repository, when reviewing init/startup patterns, prioritize cases that break core functionality over edge cases with diagnostic-only impact (e.g., missing bridgeInfo metadata). If workers are healthy and operational, defer low-impact defensive checks to follow-up PRs.
Applied to files:
src/runtime/node.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.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In the tywrap repository, bridges currently use implicit ready detection: the first successful response from the Python subprocess proves the bridge is ready to handle requests.
Applied to files:
src/runtime/node.ts
🧬 Code graph analysis (1)
src/runtime/node.ts (5)
src/runtime/bridge-protocol.ts (2)
BridgeProtocol(70-319)BridgeProtocolOptions(30-39)src/runtime/pooled-transport.ts (1)
PooledTransport(71-236)src/utils/python.ts (1)
getDefaultPythonPath(43-45)src/runtime/process-io.ts (1)
ProcessIO(134-783)src/utils/cache.ts (1)
globalCache(634-634)
🪛 GitHub Check: lint
src/runtime/node.ts
[warning] 23-23:
'PooledTransportOptions' is defined but never used
[warning] 22-22:
'ProcessIOOptions' is defined but never used
[warning] 245-245:
Missing return type on function
[warning] 509-509:
Generic Object Injection Sink
[warning] 508-508:
Generic Object Injection Sink
[warning] 499-499:
Generic Object Injection Sink
[warning] 491-491:
Generic Object Injection Sink
[warning] 482-482:
Generic Object Injection Sink
🔇 Additional comments (9)
test-d/types.test-d.ts (1)
227-227: LGTM for updated HttpBridge constructor usage.examples/living-app/src/index.ts (2)
54-61: LGTM!The
enableArrowDecoderfunction properly checks for the optionalapache-arrowdependency and provides a clear, actionable error message when it's missing. This is a clean pattern for handling optional dependencies.
173-186: LGTM!Good cleanup hygiene: the finally block properly removes the temp directory, disposes the bridge, and clears the global Arrow decoder state. The swallowed exception in the
rmSynccatch is acceptable for cleanup code.src/runtime/node.ts (6)
131-165: Utility path helpers look solid.
No issues withresolveDefaultScriptPath,resolveVirtualEnv, orgetPathKey.
279-300: Init flow is clear and ordered.
Script validation → Arrow decoder registration → transport init → warmup is sensible.
310-343: Caching wrapper is clean and well‑gated.
The cache lookup/write are correctly scoped toenableCache.
352-357: Pool stats passthrough looks good.
Straight mapping fromPooledTransportmetrics.
424-462: Cache key handling and purity heuristics look reasonable.
Safe cache key generation and the purity heuristic are both clear and conservative.
473-526: Env construction and venv wiring look solid.
Whitelisting plus PATH/PYTHONPATH augmentation is consistent and easy to follow.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…worker warmup - Add minWorkers option to WorkerPool for pre-spawning workers during init - Add onWorkerReady callback to WorkerPool for per-worker initialization - Wire minProcesses from NodeBridge through PooledTransport to WorkerPool - Execute warmupCommands on each worker individually via onWorkerReady - Remove global runWarmupCommands() in favor of per-worker warmup - Clean up unused imports in pyodide-io.ts and pyodide.ts This resolves the known limitations from CodeRabbit review: - minProcesses is now properly wired through to pre-spawn workers - warmupCommands now execute on each worker, not just the first one Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add pendingCreations counter to prevent race condition in acquire() where multiple concurrent calls could exceed maxWorkers limit - Implement crash recovery: detect fatal worker errors (process exit, EPIPE, ECONNRESET) and automatically remove dead workers from pool - Re-enable 4 previously skipped adversarial tests for crash recovery - Remove obsolete skipped tests for deprecated getBridgeInfo API Fixes #155, fixes #157 Co-Authored-By: Claude Opus 4.5 <[email protected]>
bbopen
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
Automated bot review - addressing in follow-up. Core bug fixes verified with passing CI.
The torch tensor test requires pyarrow for Arrow encoding of ndarrays. This was causing CI failures when pyarrow was not installed. - Add pyarrow==17.0.0 to requirements-suite-ml.txt - Update torch test to skip if pyarrow unavailable (for local dev) - Remove enableJsonFallback to use proper Arrow encoding Co-Authored-By: Claude Opus 4.5 <[email protected]>
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 (4)
src/runtime/process-io.ts (3)
216-219: Potential edge case: ID value of 0 would be incorrectly rejected.The truthy check
if (!messageId)will reject ID 0 since it's falsy. If the protocol allows ID 0 (e.g., ifgenerateId()starts at 0), this would incorrectly throw a protocol error.Consider using an explicit null check:
🔧 Suggested fix
const messageId = extractMessageId(message); - if (!messageId) { + if (messageId === null) { throw new BridgeProtocolError('Message must contain an "id" field'); }
559-563: Same ID 0 edge case as insend()method.The truthy check
if (!messageId)would also reject valid responses with ID 0. Apply the same fix:🔧 Suggested fix
const messageId = extractMessageId(line); - if (!messageId) { + if (messageId === null) { this.handleProtocolError('Response missing "id" field', line); return; }
391-408: Consider making environment handling more defensive.The current logic at line 394 relies on the assumption that if
envis provided, it's a complete environment (not partial). While this works correctly when ProcessIO is used through NodeBridge (which constructs complete environments viabuildProcessEnv()), the design is fragile for direct usage.If a caller passes
env: { MY_VAR: 'value' }directly to ProcessIO, the subprocess losesPATH,HOME, and other inherited variables. Merging instead of replacing would be safer and more forgiving:- const baseEnv = Object.keys(this.envOverrides).length > 0 ? this.envOverrides : process.env; + const baseEnv = { ...process.env, ...this.envOverrides };This approach is more defensive against usage errors while maintaining the same behavior through NodeBridge (which pre-computes complete environments).
src/runtime/pyodide-io.ts (1)
68-133: Critical: Python dispatcher expects legacy message format, but TypeScript sends new protocol format.The
BOOTSTRAP_PYTHONcode (lines 68-133) expects messages withtype,module,functionName,className, etc.:msg_type = msg['type'] if msg_type == 'call': mod = importlib.import_module(msg['module']) fn = getattr(mod, msg['functionName'])However, the TypeScript
call(),instantiate(), etc. methods (lines 315-402) now construct messages with the new protocol envelope:method: 'call', params: { module, functionName, args, kwargs }This mismatch will cause
KeyErrorexceptions in Python becausemsg['type']andmsg['module']don't exist in the new format.The Python dispatcher needs to be updated to handle the new format:
🐛 Proposed fix for BOOTSTRAP_PYTHON
def __tywrap_dispatch(message_json): msg = None try: msg = json.loads(message_json) msg_id = msg['id'] - msg_type = msg['type'] + msg_type = msg['method'] + params = msg.get('params', {}) if msg_type == 'call': - mod = importlib.import_module(msg['module']) - fn = getattr(mod, msg['functionName']) - result = fn(*msg.get('args', []), **msg.get('kwargs', {})) + mod = importlib.import_module(params['module']) + fn = getattr(mod, params['functionName']) + result = fn(*params.get('args', []), **params.get('kwargs', {})) return json.dumps({'id': msg_id, 'result': result}) elif msg_type == 'instantiate': - mod = importlib.import_module(msg['module']) - cls = getattr(mod, msg['className']) - obj = cls(*msg.get('args', []), **msg.get('kwargs', {})) + mod = importlib.import_module(params['module']) + cls = getattr(mod, params['className']) + obj = cls(*params.get('args', []), **params.get('kwargs', {})) handle = str(id(obj)) __tywrap_instances[handle] = obj return json.dumps({'id': msg_id, 'result': handle}) elif msg_type == 'call_method': - obj = __tywrap_instances[msg['handle']] - method = getattr(obj, msg['methodName']) - result = method(*msg.get('args', []), **msg.get('kwargs', {})) + obj = __tywrap_instances[params['handle']] + method = getattr(obj, params['methodName']) + result = method(*params.get('args', []), **params.get('kwargs', {})) return json.dumps({'id': msg_id, 'result': result}) elif msg_type == 'dispose_instance': - __tywrap_instances.pop(msg['handle'], None) + __tywrap_instances.pop(params['handle'], None) return json.dumps({'id': msg_id, 'result': None})
🤖 Fix all issues with AI agents
In `@src/runtime/node.ts`:
- Around line 462-499: The warmup messages built by generateWarmupId and
createWarmupCallback use the wrong protocol shape and string IDs; update
generateWarmupId to return numeric IDs (matching BridgeProtocol.requestId),
import PROTOCOL_ID from transport.js, and change the message construction inside
createWarmupCallback (the block handling 'module'/'functionName') to send an
envelope { protocol: PROTOCOL_ID, id: <numeric id>, method: 'call', params: {
module, functionName, args: cmd.args ?? [], kwargs: {} } } via
worker.transport.send with timeoutMs so the transport validation (transport.ts)
accepts the warmup; retain the try/catch but allow logging/propagating or at
least preserve behavior.
In `@src/runtime/worker-pool.ts`:
- Around line 336-349: When removing a dead worker in
removeWorker(PooledWorker), also handle any queued waiters so callers don't
stall: after splicing the worker and disposing its transport, check this.waiters
(or the queue that acquire() uses); if there are waiters, either immediately
spawn a replacement via the existing spawnWorker/createWorker/ensureMinWorkers
flow and resolve the oldest waiter with the new PooledWorker, or if spawning is
not possible, reject the oldest waiter so it doesn't hang. Implement this by
invoking the pool's spawn/create method and resolving the queued promise (or
calling the pool's release/wakeNext helper) when the new worker is ready, or by
rejecting the waiter with a clear error if you can't replace it.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
src/runtime/http.tssrc/runtime/node.tssrc/runtime/pooled-transport.tssrc/runtime/process-io.tssrc/runtime/pyodide-io.tssrc/runtime/pyodide.tssrc/runtime/worker-pool.tstest/adversarial_playground.test.tstest/optimized-node.test.tstest/runtime_bridge_fixtures.test.tstest/runtime_node.test.ts
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
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: 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: 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.
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.
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.
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.
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/optimized-node.test.tstest/runtime_node.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/optimized-node.test.tstest/runtime_node.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: In `test/fixtures/out_of_order_bridge.py`, the fixture intentionally leaves a pending request unanswered at EOF to simulate missing/out-of-order responses and validate bridge behavior when requests never complete; this is the exact failure mode being tested and must be preserved.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/optimized-node.test.tstest/runtime_node.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/optimized-node.test.tstest/runtime_node.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
test/runtime_bridge_fixtures.test.tssrc/runtime/http.tstest/optimized-node.test.tssrc/runtime/process-io.tssrc/runtime/worker-pool.tstest/runtime_node.test.tssrc/runtime/pyodide.tstest/adversarial_playground.test.tssrc/runtime/pyodide-io.tssrc/runtime/node.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
test/runtime_bridge_fixtures.test.tssrc/runtime/http.tstest/optimized-node.test.tssrc/runtime/process-io.tstest/runtime_node.test.tssrc/runtime/pyodide.tstest/adversarial_playground.test.tssrc/runtime/pyodide-io.tssrc/runtime/node.ts
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: Policy for adversarial test fixtures: adversarial fixtures must preserve the failure mode they exercise, even if that behavior would be "bad" in real code. When reviewing test fixtures with unusual patterns (e.g., missing error handling, intentional protocol violations, unanswered requests), ask for clarifying comments about the intent rather than suggesting normalization or fixes.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/runtime_bridge_fixtures.test.tssrc/runtime/http.tstest/optimized-node.test.tssrc/runtime/process-io.tssrc/runtime/pooled-transport.tssrc/runtime/worker-pool.tstest/runtime_node.test.tssrc/runtime/pyodide.tstest/adversarial_playground.test.tssrc/runtime/pyodide-io.tssrc/runtime/node.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/runtime_bridge_fixtures.test.tssrc/runtime/http.tstest/optimized-node.test.tssrc/runtime/process-io.tssrc/runtime/pooled-transport.tssrc/runtime/worker-pool.tstest/runtime_node.test.tssrc/runtime/pyodide.tstest/adversarial_playground.test.tssrc/runtime/pyodide-io.tssrc/runtime/node.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
test/runtime_bridge_fixtures.test.tssrc/runtime/http.tstest/optimized-node.test.tssrc/runtime/process-io.tssrc/runtime/pooled-transport.tssrc/runtime/worker-pool.tstest/runtime_node.test.tssrc/runtime/pyodide.tstest/adversarial_playground.test.tssrc/runtime/pyodide-io.tssrc/runtime/node.ts
📚 Learning: 2026-01-20T16:01:39.136Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:826-830
Timestamp: 2026-01-20T16:01:39.136Z
Learning: In the tywrap BridgeProtocol architecture (ADR-002), `HttpIO` (and other Transport implementations like `ProcessIO`, `PyodideIO`) implements only the `Transport` interface (methods: `send`, `isReady`, `dispose`). The `RuntimeExecution` interface methods (`call`, `instantiate`, `callMethod`, `disposeInstance`) are implemented by `BridgeProtocol`, which delegates to `Transport` instances. Transport implementations should not include RuntimeExecution methods.
Applied to files:
src/runtime/http.tssrc/runtime/pooled-transport.tssrc/runtime/pyodide.tssrc/runtime/pyodide-io.ts
📚 Learning: 2026-01-19T21:14:35.390Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:35.390Z
Learning: In src/runtime/bridge-core.ts and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (such as runtime, shape, or error-payload checks) inside tight loops unless the protocol boundary is untrusted or there is a concrete bug report. The Python bridge protocol is controlled via tests, so these checks add unnecessary branching overhead without meaningful benefit. Apply this guidance to other hot-path runtime loops in src/runtime/**/*.ts, and re-enable additional validations only when a documented risk or failure scenario is identified. Ensure tests cover protocol validation where applicable.
Applied to files:
src/runtime/http.tssrc/runtime/process-io.tssrc/runtime/pooled-transport.tssrc/runtime/worker-pool.tssrc/runtime/pyodide.tssrc/runtime/pyodide-io.tssrc/runtime/node.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/http.tssrc/runtime/process-io.tssrc/runtime/pooled-transport.tssrc/runtime/worker-pool.tssrc/runtime/pyodide.tssrc/runtime/pyodide-io.tssrc/runtime/node.ts
📚 Learning: 2026-01-20T01:36:14.701Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:743-759
Timestamp: 2026-01-20T01:36:14.701Z
Learning: In the tywrap repository, when reviewing concurrent request handling patterns (especially with BridgeCore request multiplexing), flag uses of boolean `busy` flags that should be request counters instead. A boolean flag creates a race condition: when request A completes and sets `busy = false`, request B (still in flight via multiplexing) is incorrectly treated as idle. The fix is to use an `inFlightRequests: number` counter that increments on request start and decrements on completion, then check `worker.inFlightRequests === 0` before cleanup/termination.
Applied to files:
test/optimized-node.test.tstest/adversarial_playground.test.tssrc/runtime/node.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In the tywrap repository, bridges currently use implicit ready detection: the first successful response from the Python subprocess proves the bridge is ready to handle requests.
Applied to files:
test/optimized-node.test.tstest/runtime_node.test.tstest/adversarial_playground.test.tssrc/runtime/node.ts
📚 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:
test/optimized-node.test.tssrc/runtime/process-io.tssrc/runtime/node.ts
📚 Learning: 2026-01-20T01:33:12.841Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:238-249
Timestamp: 2026-01-20T01:33:12.841Z
Learning: In the tywrap repository, when reviewing init/startup patterns, prioritize cases that break core functionality over edge cases with diagnostic-only impact (e.g., missing bridgeInfo metadata). If workers are healthy and operational, defer low-impact defensive checks to follow-up PRs.
Applied to files:
src/runtime/node.ts
🧬 Code graph analysis (8)
src/runtime/http.ts (2)
src/runtime/safe-codec.ts (1)
CodecOptions(22-31)src/runtime/bridge-protocol.ts (2)
BridgeProtocol(70-319)BridgeProtocolOptions(30-39)
src/runtime/process-io.ts (1)
src/index.ts (1)
BridgeProtocolError(58-58)
src/runtime/pooled-transport.ts (2)
src/runtime/transport.ts (1)
Transport(132-168)src/runtime/worker-pool.ts (1)
PooledWorker(47-53)
src/runtime/worker-pool.ts (1)
src/index.ts (4)
PooledWorker(26-26)WorkerPoolOptions(26-26)BridgeExecutionError(61-61)BridgeProtocolError(58-58)
src/runtime/pyodide.ts (3)
src/runtime/safe-codec.ts (1)
CodecOptions(22-31)src/runtime/bridge-protocol.ts (2)
BridgeProtocol(70-319)BridgeProtocolOptions(30-39)src/runtime/pyodide-io.ts (1)
PyodideIO(164-485)
test/adversarial_playground.test.ts (3)
src/runtime/node.ts (1)
NodeBridge(210-453)src/runtime/optimized-node.ts (1)
NodeBridge(21-21)src/runtime/bounded-context.ts (1)
delay(528-530)
src/runtime/pyodide-io.ts (1)
src/runtime/transport.ts (1)
PROTOCOL_ID(20-20)
src/runtime/node.ts (4)
src/runtime/bridge-protocol.ts (2)
BridgeProtocol(70-319)BridgeProtocolOptions(30-39)src/runtime/pooled-transport.ts (1)
PooledTransport(80-252)src/runtime/process-io.ts (1)
ProcessIO(137-789)src/utils/cache.ts (1)
globalCache(634-634)
🪛 GitHub Check: lint
src/runtime/node.ts
[warning] 256-256:
Missing return type on function
[warning] 549-549:
Generic Object Injection Sink
[warning] 548-548:
Generic Object Injection Sink
[warning] 539-539:
Generic Object Injection Sink
[warning] 531-531:
Generic Object Injection Sink
[warning] 522-522:
Generic Object Injection Sink
⏰ 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: codec-suite
🔇 Additional comments (35)
test/runtime_bridge_fixtures.test.ts (1)
50-67: LGTM! Error patterns correctly updated for BridgeProtocol architecture.The updated regex patterns appropriately reflect the unified
Protocol errorsurface from the new BridgeProtocol architecture. Theoversized_line_bridge.pyfixture correctly allows either pattern since both error messages are valid depending on where the error is caught.test/optimized-node.test.ts (1)
362-368: LGTM! Broadened error pattern is appropriate for serialization failures.The updated assertion correctly handles both "Failed to serialize" and "BigInt" error messages, which can vary depending on where the serialization failure is caught in the BridgeProtocol stack.
src/runtime/http.ts (1)
63-85: LGTM! Clean BridgeProtocol-based implementation.The HttpBridge correctly:
- Creates an HttpIO transport with the appropriate options
- Passes transport, codec, and timeout to BridgeProtocol
- Follows the same pattern as other bridges (NodeBridge, PyodideBridge)
src/runtime/process-io.ts (3)
102-106: LGTM! ID extraction correctly updated for numeric protocol IDs.The regex and parsing correctly handle the transition from string to numeric IDs. The
parseIntwith radix 10 ensures consistent parsing.
447-464: LGTM! Robust shutdown sequence prevents uncaught EPIPE errors.The implementation correctly:
- Attaches noop error handlers before ending stdin (to catch any EPIPE)
- Gracefully ends stdin in a try-catch
- Then removes listeners to prevent callbacks after disposal
This aligns with the PR objectives for graceful stdin.end() on shutdown.
685-699: LGTM! Synchronous write errors properly handled.The try-catch around
stdin.write()correctly handles synchronous EPIPE-like errors that Node.js can throw, converting them toBridgeProtocolErrorfor consistent error handling.src/runtime/pyodide-io.ts (2)
265-267: Validation correctly updated for new protocol format.The validation now checks for
idandmethodfields, which aligns with the newProtocolMessageshape. This is correct once the Python dispatcher is updated to match.
466-468: LGTM! ID generation starts at 1.The pre-increment
++this.requestIdensures IDs start at 1, avoiding the ID 0 edge case. This is consistent with typical protocol design.src/runtime/pyodide.ts (1)
66-87: LGTM! Clean BridgeProtocol-based implementation.The PyodideBridge correctly:
- Creates a PyodideIO transport with indexURL and packages options
- Passes transport, codec, and timeout to BridgeProtocol
- Follows the same pattern as HttpBridge
Note: This depends on PyodideIO, which has a critical protocol mismatch that needs to be fixed (see review comment on
src/runtime/pyodide-io.ts).test/runtime_node.test.ts (2)
675-676: Protocol-error expectation aligns with the unified bridge.
714-715: Invalid-JSON path now matches the protocol error contract.src/runtime/worker-pool.ts (4)
21-159: minWorkers pre-spawn and per-worker warmup wiring look solid.
226-238: pendingCreations guard correctly avoids oversubscription races.
389-411: Per-workeronWorkerReadyhook is correctly integrated.
323-333: The current implementation is correct as-is. All fatal error conditions (process not running, EPIPE, process exited) are explicitly caught and wrapped inBridgeProtocolErrorby ProcessIO before reaching the worker pool. Broadening the check toinstanceof Errorwould introduce false positives:BridgeTimeoutError(operation aborts),BridgeDisposedError(expected disposal), andBridgeExecutionError(unsupported operations) are non-fatal recoverable errors that should not trigger worker removal.The proposed diff is incorrect and would cause healthy workers to be incorrectly evicted from the pool.
Likely an incorrect or invalid review comment.
src/runtime/pooled-transport.ts (4)
20-129: PooledTransport options and init flow are clear and coherent.
163-171: send() correctly delegates through the pool and releases workers.
177-196: Pool stats getters are straightforward and useful.
202-251: Runtime-execution stubs correctly guard against misuse.test/adversarial_playground.test.ts (10)
293-294: Broadened invalid-JSON pattern matches unified protocol errors.
309-311: Stdout-noise expectation aligns with protocol-error recovery.
349-351: Timeout stderr assertions remain intact and explicit.
383-406: Crash-recovery scenario now reflects pooled-worker behavior.
485-519: Skip-aware fixture loop keeps protocol-violation coverage flexible.
619-621: Stats-removal note keeps the test intent clear.
631-641: Crash error pattern update fits WorkerPool recovery semantics.
655-689: Multi-worker isolation setup is consistent and well-sequenced.
764-766: Load-scaling test focuses on the right functional signal.
776-785: Sequential crash recovery assertions are on target.src/runtime/node.ts (6)
34-127: Option surface and resolved options are clearly structured.
219-280: Constructor wiring to pooled transport and ProcessIO looks correct.
293-309: Init sequence (script validation + Arrow decoder + transport init) is solid.
319-352: Caching override cleanly wraps the BridgeProtocol call path.
361-405: Pool stats exposure is consistent with the new architecture.
513-567: Environment assembly is robust and preserves venv/PYTHONPATH semantics.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary
Key Changes
Bridge Migrations
__tywrap_dispatchhandlerNew Components
src/runtime/pooled-transport.ts): Semaphore-based worker pool for concurrent process management with configurable min/max processesstdin.end()on shutdown, backpressure-aware writes with try-catch for synchronous errorsTest Updates
setMockDispatchHandlerpatternresolvedOptionsand transport propertiesprocessSpawns,processDeaths) that don't apply to new architectureBreaking Changes (pre-1.0)
getBridgeInfo()method removed (protocol info no longer exposed)processPoolnot accessible (managed by PooledTransport)Test plan
npm run buildpassesnpm run typecheckpassesnpm testpasses (1219 tests pass, 127 skipped)Related
🤖 Generated with Claude Code