Fix bytes envelope round-trip across JS-Python boundary#194
Merged
Conversation
|
Caution Review failedThe pull request is closed. 📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded bidirectional bytes handling at the JS↔Python bridge: JS byte envelopes are deserialized into Python Changes
Sequence Diagram(s)sequenceDiagram
participant Client as JS Client
participant Codec as SafeCodec
participant IO as Process I/O
participant Bridge as Python Bridge
participant Python as Python Runtime
rect rgba(100,150,200,0.5)
Note over Client,Python: Request flow (JS bytes -> Python bytes)
Client->>Codec: prepare args (Uint8Array)
Codec->>Codec: encode as __tywrap_bytes__ envelope
Codec->>IO: send JSON payload
IO->>Bridge: deliver request
Bridge->>Bridge: deserialize(...) -> detect envelope
Bridge->>Bridge: _deserialize_bytes_envelope() -> Python bytes
Bridge->>Python: invoke function with native bytes
end
rect rgba(200,150,100,0.5)
Note over Python,Client: Response flow (Python bytes -> JS Uint8Array)
Python->>Bridge: return result with bytes envelope (__type__: 'bytes' or __tywrap_bytes__)
Bridge->>IO: send JSON response
IO->>Codec: pass payload
Codec->>Codec: JSON.parse(..., reviveValue) -> fromBase64 -> Uint8Array
Codec->>Client: deliver typed result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
area:codec
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/cli.ts (1)
316-316:⚠️ Potential issue | 🔴 CriticalBug:
res.warnings.lengthwill throw ifres.warningsisundefined.Line 290 defensively assigns
const warnings = res.warnings ?? [], but line 316 (and line 358) accessesres.warnings.lengthdirectly, bypassing the guard. If thegenerate()result omitswarnings, this is a runtimeTypeError.Use the local
warningsvariable consistently.🐛 Proposed fix
- if (!(argv.failOnWarn && res.warnings.length > 0)) { + if (!(argv.failOnWarn && warnings.length > 0)) { process.exit(3); }And at line 358:
- if (argv.failOnWarn && res.warnings.length > 0) { + if (argv.failOnWarn && warnings.length > 0) { log.error( - `Warnings encountered (count ${res.warnings.length}). Failing due to --fail-on-warn.` + `Warnings encountered (count ${warnings.length}). Failing due to --fail-on-warn.` );src/core/generator.ts (1)
157-384: 🛠️ Refactor suggestion | 🟠 MajorSignificant DRY violation: kwargs/varargs generation logic is triplicated.
The same ~100-line pattern (compute params → build kwargs type → render overloads → build call prelude → build guards) is copy-pasted across:
generateFunctionWrapper(lines 157–384)- Class method bodies in
generateClassWrapper(lines 478–651)- Constructor spec in
generateClassWrapper(lines 657–833)Each copy differs only in indentation depth and error-message identifiers. A bug fix or feature addition to one copy must be manually replicated to the other two. Consider extracting the shared logic into a private helper that accepts the parameter list and context (function name, indent level) and returns the generated fragments (overloads, prelude, guards, param declaration).
Also applies to: 478-651, 657-833
docs/getting-started.md (1)
143-176: 🧹 Nitpick | 🔵 TrivialHelpful addition for non-importable module guidance.
The
pythonImportPathdocumentation and the fallback example are clear and actionable.One minor note: consider adding a brief comment or note that
"pythonImportPath": []is the default and can be omitted, to avoid confusion for new users who might think it's required.src/runtime/safe-codec.ts (1)
601-613:⚠️ Potential issue | 🟠 Major
String.fromCharCode(...bytes)will throwRangeErrorfor largeUint8Arrayinputs in the browser path.The spread operator passes each byte as a separate argument to
fromCharCode, which is limited by the JS engine's maximum call stack / argument count (typically ~65 536 on V8). For byte arrays larger than ~64 KB, this will crash in theatob/browser fallback path. ThefromBase64method (line 343) avoids this issue by usingArray.from.Consider chunking the conversion:
🔧 Proposed fix
private toBase64(bytes: Uint8Array): string { if (typeof Buffer !== 'undefined') { return Buffer.from(bytes).toString('base64'); } if (globalThis.btoa) { - const binary = String.fromCharCode(...bytes); + let binary = ''; + for (let i = 0; i < bytes.length; i++) { + binary += String.fromCharCode(bytes[i]); + } return globalThis.btoa(binary); }
🤖 Fix all issues with AI agents
In `@runtime/python_bridge.py`:
- Around line 161-168: The guard rejecting empty base64 strings prevents
zero-length bytes from decoding; in runtime/python_bridge.py update the checks
that validate the "b64" field (the block that runs when
value.get('__tywrap_bytes__') is True and the parallel legacy envelope block) to
only assert the type (e.g., isinstance(b64, str)) and not reject empty strings,
and keep the existing base64.b64decode try/except to raise ProtocolError on
invalid base64; make the same change in the legacy envelope branch so valid b""
round-trips successfully.
In `@src/core/annotation-parser.ts`:
- Around line 238-260: The Callable parsing block uses tab indentation causing a
style inconsistency; locate the block that begins with the
raw.startsWith('typing.Callable[') || raw.startsWith('Callable[') check (inside
the parse function where splitTopLevel and parse are used) and replace the
tab-based indentation with the file's standard space-based indentation to match
surrounding lines and satisfy linting/style rules.
In `@src/core/generator.ts`:
- Around line 533-537: The variable requiredPosCount declared inside the
overload-generation blocks shadows an outer variable of the same name; update
the inner declaration(s) (the one inside the block that uses needsKwargsParam,
requiredKwOnlyNames and positionalParams, and the analogous declaration in the
constructor spec overload-generation) to a distinct name such as
overloadRequiredPosCount to avoid shadowing and lint warnings, and update all
references within those blocks to use the new name.
- Around line 647-651: The generated method body contains a closing brace line
that uses tabs instead of spaces (e.g., the template string that returns
`${overloadDecl} async ${mname}(${paramsDecl}): Promise<${returnType}> { ...
}`), causing mixed tabs/spaces at the end (` }`); update the template in
generator.ts to replace the tab-indented closing line with spaces so its
indentation matches the surrounding lines (adjust the same pattern where the
template emits the closing brace around callPrelude/guards and
getRuntimeBridge().callMethod(...), and also fix the similar occurrences around
lines that generate other method bodies noted in the diff).
In `@src/runtime/safe-codec.ts`:
- Around line 508-517: The code currently calls JSON.parse(payload,
this.reviveValue.bind(this)) inside the decode path which allocates a new bound
function on every call; bind reviveValue once in the class constructor and store
it to a private field (e.g., this._reviveValue) and then use that field in the
JSON.parse call (replace this.reviveValue.bind(this) with this._reviveValue) so
reviveValue remains correctly bound without repeated allocations; update the
constructor to set this._reviveValue = this.reviveValue.bind(this) and change
the JSON.parse usage in the decode/JSON parse block accordingly.
In `@src/types/global.d.ts`:
- Around line 47-55: Update the kill() signature in the Subprocess return type
to match Bun's actual API: change the parameter name from signal? to exitCode?
and its type from number | string to number | NodeJS.Signals so the method reads
like kill(exitCode?: number | NodeJS.Signals): void; update any related
declarations that reference kill() in this type block to ensure tooling and
autocompletion use NodeJS.Signals.
In `@src/tywrap.ts`:
- Around line 215-229: The current filter in moduleModel.functions applies
exclude precedence over an explicit allow-list (moduleConfig.functions) by
checking allow.has(f.name) && !shouldExclude(f.name, false); decide the intended
precedence and update accordingly: if allow-list should win, remove the exclude
check so the filter becomes solely based on allow.has(f.name); if exclude should
win, keep the logic but add documentation explaining this precedence in
docs/configuration.md; ensure references to moduleConfig.functions,
moduleModel.functions, allow, and shouldExclude reflect the chosen behavior and
adjust any related warnings/messages if needed.
- Around line 190-200: The RegExp construction for user-supplied patterns
(excludeRegexes created from moduleConfig.excludePatterns in tywrap.ts) can be
vulnerable to ReDoS; update the logic to guard against catastrophic patterns by
validating or limiting patterns before calling new RegExp: e.g., enforce a max
pattern length, reject patterns with known exponential constructs (like nested
quantifiers) or run them through a safe-regex / regex-sanitizer helper, and if a
pattern is rejected push a warning into warnings with moduleKey explaining
rejection; keep the existing try/catch as a fallback and ensure any helper used
is referenced (e.g., safeRegexCheck or isComplexRegex) so reviewers can locate
the new validation step.
In `@src/utils/ir-cache.ts`:
- Around line 44-49: computeIrCacheFilename is currently using digest.slice(0,
32) which collapses to all zeros when hashUtils.sha256Hex falls back to the
32-bit DJB2 implementation (padded to 64 hex chars); update
computeIrCacheFilename to use the meaningful portion of the digest (for example
digest.slice(-32) to take the last 32 hex chars) so filenames differ under the
DJB2 fallback, ensuring the change is applied where computeIrCacheFilename
constructs the filename and that it still produces a stable ASCII filename like
`ir_<hex>.json`.
In `@src/utils/runtime.ts`:
- Around line 718-727: Replace tab characters in the DJB2 fallback block inside
the sha256Hex function with the project's space-based indentation to match the
rest of the file; specifically update the lines that initialize hash, the
for-loop, the hash calculations, and the return so they use spaces, and verify
the closing braces for sha256Hex and hashUtils align with surrounding code so
the function and object close visually and syntactically consistent with other
methods.
In `@test/parallel-processor.test.ts`:
- Around line 696-720: The test's fixed 50ms sleep is flaky; replace it with a
small polling loop that waits until processor.getStats().activeTasks > 0 (or a
short timeout like 1s) before calling processor.dispose(), so you ensure the
task is actually in-flight; update the it block around the executeTasks/await
processor.dispose() sequence to poll getStats().activeTasks (or fall back to a
longer delay with a comment) to avoid CI timing issues while preserving the
existing test timeout.
In `@test/process_utils_timeout.test.ts`:
- Around line 7-19: The availability probe uses spawnSync(pythonPath,
['--version'], { encoding: 'utf-8' }) without a timeout, so if the Python binary
hangs the test blocks; update that probe call in
test/process_utils_timeout.test.ts (the spawnSync check near
getDefaultPythonPath() and timeoutMs) to include a short timeout option (e.g.,
~1000ms) in its options so the guard self-contains its failure path and returns
early instead of relying on the outer Vitest timeout.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (55)
docs/configuration.mddocs/getting-started.mddocs/plans/2026-01-22-tywrapped-ecosystem-vision.mdruntime/python_bridge.pysrc/cli.tssrc/config/index.tssrc/core/analyzer.tssrc/core/annotation-parser.tssrc/core/discovery.tssrc/core/generator.tssrc/index.tssrc/runtime/errors.tssrc/runtime/http-io.tssrc/runtime/http.tssrc/runtime/node.tssrc/runtime/optimized-node.tssrc/runtime/process-io.tssrc/runtime/pyodide-io.tssrc/runtime/safe-codec.tssrc/runtime/worker-pool.tssrc/types/global.d.tssrc/types/index.tssrc/tywrap.tssrc/utils/cache.tssrc/utils/codec.tssrc/utils/ir-cache.tssrc/utils/parallel-processor.tssrc/utils/runtime.tstest/adversarial_playground.test.tstest/analyzer.test.tstest/annotation-parser.test.tstest/arch-stories.test.tstest/bounded-context.test.tstest/bridge-protocol.test.tstest/cache_generateKey.test.tstest/cli.test.tstest/code_generation_quality.test.tstest/config_loading.test.tstest/discovery.test.tstest/disposable.test.tstest/generator.test.tstest/integration.test.tstest/ir_cache_filename.test.tstest/optimized-node.test.tstest/parallel-processor.test.tstest/process_utils_timeout.test.tstest/runtime_codec_scientific.test.tstest/runtime_http.test.tstest/runtime_node.test.tstest/runtime_pyodide.test.tstest/safe-codec-arrow-special-floats.test.tstest/safe-codec.test.tstest/transport.test.tstest/validators.test.tstest/worker-pool.test.ts
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
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.
📚 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/adversarial_playground.test.tstest/arch-stories.test.tstest/safe-codec-arrow-special-floats.test.tstest/bounded-context.test.tstest/code_generation_quality.test.tstest/parallel-processor.test.tstest/transport.test.tstest/integration.test.tstest/runtime_node.test.tstest/safe-codec.test.tstest/optimized-node.test.tstest/worker-pool.test.tstest/runtime_http.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: 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/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/adversarial_playground.test.tstest/code_generation_quality.test.tstest/runtime_codec_scientific.test.tssrc/index.tsdocs/plans/2026-01-22-tywrapped-ecosystem-vision.mdtest/integration.test.tstest/runtime_node.test.tstest/safe-codec.test.tstest/optimized-node.test.tstest/runtime_http.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/adversarial_playground.test.tstest/bounded-context.test.tstest/discovery.test.tstest/code_generation_quality.test.tstest/runtime_pyodide.test.tsdocs/plans/2026-01-22-tywrapped-ecosystem-vision.mdtest/transport.test.tstest/integration.test.tstest/runtime_node.test.tstest/optimized-node.test.tstest/runtime_http.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/adversarial_playground.test.tstest/transport.test.tstest/runtime_node.test.tstest/runtime_http.test.tstest/bridge-protocol.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/adversarial_playground.test.tstest/config_loading.test.tstest/arch-stories.test.tssrc/utils/ir-cache.tstest/process_utils_timeout.test.tstest/safe-codec-arrow-special-floats.test.tstest/generator.test.tstest/bounded-context.test.tssrc/core/annotation-parser.tssrc/runtime/errors.tssrc/types/global.d.tstest/discovery.test.tssrc/core/discovery.tstest/code_generation_quality.test.tstest/runtime_codec_scientific.test.tssrc/types/index.tssrc/index.tssrc/utils/cache.tstest/parallel-processor.test.tssrc/utils/codec.tssrc/runtime/http.tstest/runtime_pyodide.test.tssrc/runtime/optimized-node.tssrc/runtime/http-io.tssrc/runtime/worker-pool.tstest/transport.test.tstest/analyzer.test.tssrc/runtime/process-io.tstest/integration.test.tstest/disposable.test.tssrc/runtime/node.tssrc/runtime/pyodide-io.tstest/validators.test.tstest/runtime_node.test.tstest/safe-codec.test.tssrc/runtime/safe-codec.tssrc/cli.tstest/annotation-parser.test.tstest/optimized-node.test.tstest/worker-pool.test.tstest/ir_cache_filename.test.tssrc/core/generator.tstest/cache_generateKey.test.tssrc/config/index.tstest/runtime_http.test.tssrc/utils/parallel-processor.tstest/cli.test.tssrc/utils/runtime.tssrc/core/analyzer.tstest/bridge-protocol.test.tssrc/tywrap.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/adversarial_playground.test.tstest/config_loading.test.tstest/arch-stories.test.tstest/process_utils_timeout.test.tstest/safe-codec-arrow-special-floats.test.tstest/generator.test.tstest/bounded-context.test.tstest/discovery.test.tstest/code_generation_quality.test.tstest/runtime_codec_scientific.test.tstest/parallel-processor.test.tstest/runtime_pyodide.test.tstest/transport.test.tstest/analyzer.test.tstest/integration.test.tstest/disposable.test.tstest/validators.test.tstest/runtime_node.test.tstest/safe-codec.test.tstest/annotation-parser.test.tstest/optimized-node.test.tstest/worker-pool.test.tstest/ir_cache_filename.test.tstest/cache_generateKey.test.tstest/runtime_http.test.tstest/cli.test.tstest/bridge-protocol.test.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/adversarial_playground.test.tstest/config_loading.test.tstest/arch-stories.test.tssrc/utils/ir-cache.tstest/process_utils_timeout.test.tstest/safe-codec-arrow-special-floats.test.tstest/generator.test.tstest/bounded-context.test.tssrc/core/annotation-parser.tssrc/runtime/errors.tssrc/types/global.d.tstest/discovery.test.tssrc/core/discovery.tstest/code_generation_quality.test.tstest/runtime_codec_scientific.test.tssrc/types/index.tssrc/index.tssrc/utils/cache.tstest/parallel-processor.test.tssrc/utils/codec.tssrc/runtime/http.tstest/runtime_pyodide.test.tssrc/runtime/optimized-node.tssrc/runtime/http-io.tssrc/runtime/worker-pool.tstest/transport.test.tstest/analyzer.test.tssrc/runtime/process-io.tstest/integration.test.tstest/disposable.test.tssrc/runtime/node.tssrc/runtime/pyodide-io.tstest/validators.test.tstest/runtime_node.test.tstest/safe-codec.test.tssrc/runtime/safe-codec.tssrc/cli.tstest/annotation-parser.test.tstest/optimized-node.test.tstest/worker-pool.test.tstest/ir_cache_filename.test.tssrc/core/generator.tstest/cache_generateKey.test.tssrc/config/index.tstest/runtime_http.test.tssrc/utils/parallel-processor.tstest/cli.test.tssrc/utils/runtime.tssrc/core/analyzer.tstest/bridge-protocol.test.tssrc/tywrap.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/adversarial_playground.test.tstest/config_loading.test.tstest/arch-stories.test.tssrc/utils/ir-cache.tstest/process_utils_timeout.test.tstest/safe-codec-arrow-special-floats.test.tstest/generator.test.tstest/bounded-context.test.tssrc/core/annotation-parser.tssrc/runtime/errors.tssrc/types/global.d.tstest/discovery.test.tssrc/core/discovery.tstest/code_generation_quality.test.tstest/runtime_codec_scientific.test.tssrc/types/index.tssrc/index.tssrc/utils/cache.tstest/parallel-processor.test.tssrc/utils/codec.tssrc/runtime/http.tstest/runtime_pyodide.test.tssrc/runtime/optimized-node.tssrc/runtime/http-io.tssrc/runtime/worker-pool.tstest/transport.test.tstest/analyzer.test.tssrc/runtime/process-io.tstest/integration.test.tstest/disposable.test.tssrc/runtime/node.tssrc/runtime/pyodide-io.tstest/validators.test.tstest/runtime_node.test.tstest/safe-codec.test.tssrc/runtime/safe-codec.tssrc/cli.tstest/annotation-parser.test.tstest/optimized-node.test.tstest/worker-pool.test.tstest/ir_cache_filename.test.tssrc/core/generator.tstest/cache_generateKey.test.tssrc/config/index.tstest/runtime_http.test.tssrc/utils/parallel-processor.tstest/cli.test.tssrc/utils/runtime.tssrc/core/analyzer.tstest/bridge-protocol.test.tssrc/tywrap.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/arch-stories.test.tstest/safe-codec-arrow-special-floats.test.tssrc/runtime/errors.tssrc/index.tssrc/runtime/worker-pool.tsdocs/plans/2026-01-22-tywrapped-ecosystem-vision.mdsrc/runtime/process-io.tstest/integration.test.tssrc/runtime/pyodide-io.tstest/runtime_node.test.tstest/safe-codec.test.tssrc/runtime/safe-codec.tstest/optimized-node.test.tstest/runtime_http.test.tssrc/utils/runtime.tstest/bridge-protocol.test.ts
📚 Learning: 2026-01-20T16:00:49.738Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:168-172
Timestamp: 2026-01-20T16:00:49.738Z
Learning: In the tywrap project's BridgeProtocol SafeCodec implementation, Arrow format decoders can produce NaN/Infinity values from binary representations even when the raw JSON payload doesn't contain them. This is why validation for special floats must occur both before encoding (to reject invalid inputs) and after applying decoders (to catch values introduced during Arrow deserialization), protecting downstream consumers from unexpected NaN/Infinity values.
Applied to files:
test/safe-codec-arrow-special-floats.test.tstest/safe-codec.test.tssrc/runtime/safe-codec.tstest/runtime_http.test.tstest/bridge-protocol.test.ts
📚 Learning: 2026-01-19T21:14:29.869Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
Learning: In the runtime env-var parsing (e.g., in src/runtime/bridge-core.ts and similar modules), adopt a tolerant, best-effort policy: numeric env values should parse by taking the leading numeric portion (e.g., TYWRAP_CODEC_MAX_BYTES=1024abc -> 1024). Only reject clearly invalid values (non-numeric start or <= 0). This reduces surprising failures from minor typos. Add tests to cover partial numeric prefixes, and ensure downstream logic documents the trusted range; consider a small helper to extract a positive integer from a string with a safe fallback.
Applied to files:
src/runtime/errors.tssrc/runtime/http.tssrc/runtime/optimized-node.tssrc/runtime/http-io.tssrc/runtime/worker-pool.tssrc/runtime/process-io.tssrc/runtime/node.tssrc/runtime/pyodide-io.tssrc/runtime/safe-codec.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/errors.tssrc/runtime/http.tssrc/runtime/optimized-node.tssrc/runtime/http-io.tssrc/runtime/worker-pool.tssrc/runtime/process-io.tssrc/runtime/node.tssrc/runtime/pyodide-io.tssrc/runtime/safe-codec.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/index.tsdocs/plans/2026-01-22-tywrapped-ecosystem-vision.mdtest/transport.test.tssrc/runtime/pyodide-io.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:
src/utils/codec.tstest/safe-codec.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:
src/runtime/worker-pool.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:
docs/plans/2026-01-22-tywrapped-ecosystem-vision.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/plans/2026-01-22-tywrapped-ecosystem-vision.mdtest/runtime_http.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/bridge-protocol.test.ts
🧬 Code graph analysis (24)
test/config_loading.test.ts (1)
src/utils/runtime.ts (1)
writeFile(386-416)
test/arch-stories.test.ts (2)
src/index.ts (2)
BridgeProtocolError(73-73)BridgeExecutionError(76-76)src/runtime/errors.ts (2)
BridgeProtocolError(13-13)BridgeExecutionError(31-33)
src/utils/ir-cache.ts (1)
src/utils/runtime.ts (1)
hashUtils(699-727)
test/safe-codec-arrow-special-floats.test.ts (1)
src/runtime/safe-codec.ts (1)
SafeCodec(317-614)
test/bounded-context.test.ts (3)
src/utils/runtime.ts (1)
resolve(302-320)src/index.ts (2)
BridgeTimeoutError(74-74)BridgeDisposedError(75-75)src/runtime/errors.ts (2)
BridgeTimeoutError(14-14)BridgeDisposedError(15-15)
src/core/annotation-parser.ts (1)
src/types/index.ts (1)
PythonType(70-82)
src/runtime/errors.ts (1)
src/index.ts (2)
BridgeCodecError(72-72)BridgeError(71-71)
test/parallel-processor.test.ts (1)
src/utils/parallel-processor.ts (1)
ParallelProcessor(126-782)
src/runtime/http.ts (1)
src/utils/codec.ts (1)
autoRegisterArrowDecoder(165-194)
src/runtime/process-io.ts (2)
src/index.ts (2)
BridgeProtocolError(73-73)BridgeTimeoutError(74-74)src/runtime/errors.ts (2)
BridgeProtocolError(13-13)BridgeTimeoutError(14-14)
test/integration.test.ts (1)
src/tywrap.ts (1)
generate(112-323)
src/runtime/node.ts (2)
src/index.ts (1)
BridgeProtocolError(73-73)src/runtime/errors.ts (1)
BridgeProtocolError(13-13)
test/validators.test.ts (1)
src/runtime/validators.ts (2)
assertFiniteNumber(69-75)containsSpecialFloat(206-208)
test/runtime_node.test.ts (1)
src/runtime/errors.ts (1)
BridgeExecutionError(31-33)
test/safe-codec.test.ts (2)
src/runtime/errors.ts (1)
BridgeCodecError(17-29)src/runtime/safe-codec.ts (1)
SafeCodec(317-614)
src/runtime/safe-codec.ts (3)
src/index.ts (3)
BridgeCodecError(72-72)containsSpecialFloat(59-59)BridgeProtocolError(73-73)src/runtime/errors.ts (2)
BridgeCodecError(17-29)BridgeProtocolError(13-13)src/runtime/validators.ts (1)
containsSpecialFloat(206-208)
test/annotation-parser.test.ts (1)
src/core/annotation-parser.ts (1)
parseAnnotationToPythonType(7-295)
test/optimized-node.test.ts (2)
src/index.ts (1)
BridgeCodecError(72-72)src/runtime/errors.ts (1)
BridgeCodecError(17-29)
test/worker-pool.test.ts (3)
src/index.ts (3)
WorkerPoolOptions(40-40)WorkerPool(40-40)BridgeExecutionError(76-76)src/runtime/worker-pool.ts (2)
WorkerPoolOptions(21-42)WorkerPool(101-504)src/runtime/errors.ts (1)
BridgeExecutionError(31-33)
test/ir_cache_filename.test.ts (1)
src/utils/ir-cache.ts (1)
computeIrCacheFilename(44-50)
test/runtime_http.test.ts (3)
src/runtime/errors.ts (3)
BridgeCodecError(17-29)BridgeExecutionError(31-33)BridgeTimeoutError(14-14)src/runtime/http.ts (1)
HttpBridge(64-93)src/utils/codec.ts (2)
clearArrowDecoder(120-122)hasArrowDecoder(127-129)
test/cli.test.ts (1)
tools/matrix.js (1)
config(120-127)
test/bridge-protocol.test.ts (2)
src/runtime/errors.ts (2)
BridgeDisposedError(15-15)BridgeCodecError(17-29)src/runtime/safe-codec.ts (1)
SafeCodec(317-614)
src/tywrap.ts (3)
src/utils/runtime.ts (3)
processUtils(422-617)isWindows(626-632)pathUtils(261-342)src/core/annotation-parser.ts (1)
parseAnnotationToPythonType(7-295)src/utils/ir-cache.ts (1)
computeIrCacheFilename(44-50)
🪛 GitHub Check: lint
src/core/generator.ts
[warning] 122-122:
Missing return type on function
[warning] 170-170:
Missing return type on function
[warning] 249-249:
Missing return type on function
[warning] 721-721:
'requiredPosCount' is already declared in the upper scope on line 677 column 13
[warning] 690-690:
Missing return type on function
[warning] 657-657:
Missing return type on function
[warning] 535-535:
'requiredPosCount' is already declared in the upper scope on line 486 column 15
[warning] 499-499:
Missing return type on function
src/core/analyzer.ts
[warning] 663-663:
Variable Assigned to Object Injection Sink
src/tywrap.ts
[warning] 193-193:
Found non-literal argument to RegExp Constructor
🪛 Ruff (0.15.0)
runtime/python_bridge.py
[warning] 147-147: Missing return type annotation for private function _deserialize_bytes_envelope
(ANN202)
[warning] 164-164: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 168-168: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 173-173: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 177-177: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 662-662: Avoid specifying long messages outside the exception class
(TRY003)
Comment on lines
238
to
260
| if (raw.startsWith('typing.Callable[') || raw.startsWith('Callable[')) { | ||
| const inner = raw.slice(raw.indexOf('[') + 1, raw.lastIndexOf(']')); | ||
| const parts = splitTopLevel(inner, ','); | ||
| if (parts.length >= 2) { | ||
| const paramsPart = (parts[0] ?? '').trim(); | ||
| const returnPart = parts.slice(1).join(',').trim(); | ||
| const paramInner = | ||
| paramsPart.startsWith('[') && paramsPart.endsWith(']') ? paramsPart.slice(1, -1) : ''; | ||
| const paramTypes = ((): PythonType[] => { | ||
| // Callable[..., R] uses a top-level Ellipsis. | ||
| if (paramsPart === '...' || paramsPart === 'Ellipsis') { | ||
| return [{ kind: 'custom', name: '...' } as PythonType]; | ||
| } | ||
| const trimmed = paramInner.trim(); | ||
| if (trimmed === '...' || trimmed === 'Ellipsis') { | ||
| return [{ kind: 'custom', name: '...' } as PythonType]; | ||
| } | ||
| return trimmed ? splitTopLevel(trimmed, ',').map(p => parse(p.trim(), depth + 1)) : []; | ||
| })(); | ||
| const returnType = parse(returnPart, depth + 1); | ||
| return { kind: 'callable', parameters: paramTypes, returnType } as PythonType; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent indentation in the Callable parsing block.
Lines 238–260 use tab-based indentation while the rest of the file uses spaces. This won't affect behavior but is a formatting inconsistency that linting should catch.
Proposed fix (align with file's space-based indentation)
- if (raw.startsWith('typing.Callable[') || raw.startsWith('Callable[')) {
- const inner = raw.slice(raw.indexOf('[') + 1, raw.lastIndexOf(']'));
- const parts = splitTopLevel(inner, ',');
- if (parts.length >= 2) {
- const paramsPart = (parts[0] ?? '').trim();
- const returnPart = parts.slice(1).join(',').trim();
- const paramInner =
- paramsPart.startsWith('[') && paramsPart.endsWith(']') ? paramsPart.slice(1, -1) : '';
- const paramTypes = ((): PythonType[] => {
- // Callable[..., R] uses a top-level Ellipsis.
- if (paramsPart === '...' || paramsPart === 'Ellipsis') {
- return [{ kind: 'custom', name: '...' } as PythonType];
- }
- const trimmed = paramInner.trim();
- if (trimmed === '...' || trimmed === 'Ellipsis') {
- return [{ kind: 'custom', name: '...' } as PythonType];
- }
- return trimmed ? splitTopLevel(trimmed, ',').map(p => parse(p.trim(), depth + 1)) : [];
- })();
- const returnType = parse(returnPart, depth + 1);
- return { kind: 'callable', parameters: paramTypes, returnType } as PythonType;
- }
- }
+ if (raw.startsWith('typing.Callable[') || raw.startsWith('Callable[')) {
+ const inner = raw.slice(raw.indexOf('[') + 1, raw.lastIndexOf(']'));
+ const parts = splitTopLevel(inner, ',');
+ if (parts.length >= 2) {
+ const paramsPart = (parts[0] ?? '').trim();
+ const returnPart = parts.slice(1).join(',').trim();
+ const paramInner =
+ paramsPart.startsWith('[') && paramsPart.endsWith(']') ? paramsPart.slice(1, -1) : '';
+ const paramTypes = ((): PythonType[] => {
+ // Callable[..., R] uses a top-level Ellipsis.
+ if (paramsPart === '...' || paramsPart === 'Ellipsis') {
+ return [{ kind: 'custom', name: '...' } as PythonType];
+ }
+ const trimmed = paramInner.trim();
+ if (trimmed === '...' || trimmed === 'Ellipsis') {
+ return [{ kind: 'custom', name: '...' } as PythonType];
+ }
+ return trimmed ? splitTopLevel(trimmed, ',').map(p => parse(p.trim(), depth + 1)) : [];
+ })();
+ const returnType = parse(returnPart, depth + 1);
+ return { kind: 'callable', parameters: paramTypes, returnType } as PythonType;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (raw.startsWith('typing.Callable[') || raw.startsWith('Callable[')) { | |
| const inner = raw.slice(raw.indexOf('[') + 1, raw.lastIndexOf(']')); | |
| const parts = splitTopLevel(inner, ','); | |
| if (parts.length >= 2) { | |
| const paramsPart = (parts[0] ?? '').trim(); | |
| const returnPart = parts.slice(1).join(',').trim(); | |
| const paramInner = | |
| paramsPart.startsWith('[') && paramsPart.endsWith(']') ? paramsPart.slice(1, -1) : ''; | |
| const paramTypes = ((): PythonType[] => { | |
| // Callable[..., R] uses a top-level Ellipsis. | |
| if (paramsPart === '...' || paramsPart === 'Ellipsis') { | |
| return [{ kind: 'custom', name: '...' } as PythonType]; | |
| } | |
| const trimmed = paramInner.trim(); | |
| if (trimmed === '...' || trimmed === 'Ellipsis') { | |
| return [{ kind: 'custom', name: '...' } as PythonType]; | |
| } | |
| return trimmed ? splitTopLevel(trimmed, ',').map(p => parse(p.trim(), depth + 1)) : []; | |
| })(); | |
| const returnType = parse(returnPart, depth + 1); | |
| return { kind: 'callable', parameters: paramTypes, returnType } as PythonType; | |
| } | |
| } | |
| if (raw.startsWith('typing.Callable[') || raw.startsWith('Callable[')) { | |
| const inner = raw.slice(raw.indexOf('[') + 1, raw.lastIndexOf(']')); | |
| const parts = splitTopLevel(inner, ','); | |
| if (parts.length >= 2) { | |
| const paramsPart = (parts[0] ?? '').trim(); | |
| const returnPart = parts.slice(1).join(',').trim(); | |
| const paramInner = | |
| paramsPart.startsWith('[') && paramsPart.endsWith(']') ? paramsPart.slice(1, -1) : ''; | |
| const paramTypes = ((): PythonType[] => { | |
| // Callable[..., R] uses a top-level Ellipsis. | |
| if (paramsPart === '...' || paramsPart === 'Ellipsis') { | |
| return [{ kind: 'custom', name: '...' } as PythonType]; | |
| } | |
| const trimmed = paramInner.trim(); | |
| if (trimmed === '...' || trimmed === 'Ellipsis') { | |
| return [{ kind: 'custom', name: '...' } as PythonType]; | |
| } | |
| return trimmed ? splitTopLevel(trimmed, ',').map(p => parse(p.trim(), depth + 1)) : []; | |
| })(); | |
| const returnType = parse(returnPart, depth + 1); | |
| return { kind: 'callable', parameters: paramTypes, returnType } as PythonType; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/core/annotation-parser.ts` around lines 238 - 260, The Callable parsing
block uses tab indentation causing a style inconsistency; locate the block that
begins with the raw.startsWith('typing.Callable[') ||
raw.startsWith('Callable[') check (inside the parse function where splitTopLevel
and parse are used) and replace the tab-based indentation with the file's
standard space-based indentation to match surrounding lines and satisfy
linting/style rules.
Comment on lines
533
to
537
| if (needsKwargsParam && requiredKwOnlyNames.length > 0) { | ||
| const firstOptionalIndex = positionalParams.findIndex(p => p.optional); | ||
| const requiredPosCount = | ||
| firstOptionalIndex >= 0 ? firstOptionalIndex : positionalParams.length; | ||
| for (let i = requiredPosCount; i <= positionalParams.length; i++) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Variable requiredPosCount shadows outer declaration (static analysis warning).
requiredPosCount on line 535 shadows the declaration on line 486. Same issue occurs at line 721 vs 677 in the constructor spec. While not a bug (different block scopes), it hurts readability and triggers lint warnings. Use a distinct name (e.g., overloadRequiredPosCount) inside the overload-generation blocks.
🧰 Tools
🪛 GitHub Check: lint
[warning] 535-535:
'requiredPosCount' is already declared in the upper scope on line 486 column 15
🤖 Prompt for AI Agents
In `@src/core/generator.ts` around lines 533 - 537, The variable requiredPosCount
declared inside the overload-generation blocks shadows an outer variable of the
same name; update the inner declaration(s) (the one inside the block that uses
needsKwargsParam, requiredKwOnlyNames and positionalParams, and the analogous
declaration in the constructor spec overload-generation) to a distinct name such
as overloadRequiredPosCount to avoid shadowing and lint warnings, and update all
references within those blocks to use the new name.
Comment on lines
44
to
49
| export async function computeIrCacheFilename(keyObject: IrCacheKeyObject): Promise<string> { | ||
| // Canonicalize to avoid cache misses when key object property insertion order differs. | ||
| const normalized = JSON.stringify(stableSortForJson(keyObject)); | ||
| const digest = await hashUtils.sha256Hex(normalized); | ||
| // Hash-only filename: safe ASCII, no separators, stable length. | ||
| return `ir_${digest.slice(0, 32)}.json`; |
There was a problem hiding this comment.
digest.slice(0, 32) would collapse to all zeros under the DJB2 fallback in hashUtils.sha256Hex.
The DJB2 fallback (in src/utils/runtime.ts) produces a 32-bit hash left-padded to 64 hex chars. Slicing the first 32 characters yields "00000000000000000000000000000000" for every input, causing total cache-key collisions. In practice this path is very unlikely (requires no Node crypto and no Web Crypto), but if it's ever hit, every IR file would map to the same filename.
Consider slicing the last 32 chars (the meaningful portion) or, better, always slicing a centered/tail window:
Proposed fix
- return `ir_${digest.slice(0, 32)}.json`;
+ // Use the tail of the digest so that the DJB2 non-crypto fallback
+ // (which left-pads with '0') still yields distinct filenames.
+ return `ir_${digest.slice(-32)}.json`;🤖 Prompt for AI Agents
In `@src/utils/ir-cache.ts` around lines 44 - 49, computeIrCacheFilename is
currently using digest.slice(0, 32) which collapses to all zeros when
hashUtils.sha256Hex falls back to the 32-bit DJB2 implementation (padded to 64
hex chars); update computeIrCacheFilename to use the meaningful portion of the
digest (for example digest.slice(-32) to take the last 32 hex chars) so
filenames differ under the DJB2 fallback, ensuring the change is applied where
computeIrCacheFilename constructs the filename and that it still produces a
stable ASCII filename like `ir_<hex>.json`.
Comment on lines
718
to
727
| // Fallback to DJB2 (non-crypto) for unknown runtimes | ||
| let hash = 5381; | ||
| for (let i = 0; i < text.length; i++) { | ||
| hash = (hash << 5) + hash + text.charCodeAt(i); | ||
| hash |= 0; | ||
| } | ||
| // Match sha256 hex shape (64 chars) so callers can rely on fixed-length output. | ||
| return Math.abs(hash).toString(16).padStart(64, '0'); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tab/space indentation mismatch in the DJB2 fallback block.
Lines 718–727 use tab characters while the rest of the file uses spaces. This won't break functionality but will cause issues if a formatter or linter enforces consistent indentation. The closing braces on lines 726–727 appear to close correctly (sha256Hex method then hashUtils object), but the tab indentation makes this hard to verify visually.
🤖 Prompt for AI Agents
In `@src/utils/runtime.ts` around lines 718 - 727, Replace tab characters in the
DJB2 fallback block inside the sha256Hex function with the project's space-based
indentation to match the rest of the file; specifically update the lines that
initialize hash, the for-loop, the hash calculations, and the return so they use
spaces, and verify the closing braces for sha256Hex and hashUtils align with
surrounding code so the function and object close visually and syntactically
consistent with other methods.
Comment on lines
696
to
720
| it('dispose() rejects pending tasks', async () => { | ||
| const workerScript = await writeWorkerScript(); | ||
| processor = new ParallelProcessor({ | ||
| maxWorkers: 1, | ||
| workerScript, | ||
| enableCaching: false, | ||
| enableMemoryMonitoring: false, | ||
| retryAttempts: 1, | ||
| taskTimeout: 10_000, | ||
| }); | ||
|
|
||
| const promise = processor.executeTasks([ | ||
| { id: 'hang', type: 'custom', data: { action: 'hang' } }, | ||
| ]); | ||
|
|
||
| // Give the worker a moment to receive the task. | ||
| await new Promise(resolve => setTimeout(resolve, 50)); | ||
| await processor.dispose(); | ||
|
|
||
| const results = await promise; | ||
| expect(results).toHaveLength(1); | ||
| expect(results[0]?.success).toBe(false); | ||
| expect(String(results[0]?.error ?? '').toLowerCase()).toContain('disposed'); | ||
| expect(processor.getStats().activeTasks).toBe(0); | ||
| }, 10_000); |
There was a problem hiding this comment.
Timing-dependent test: the 50ms delay may be flaky under CI load.
Line 712 uses a fixed 50ms delay to ensure the worker receives the task before dispose() is called. Under heavy CI load, this could be insufficient, causing the task to be rejected before it reaches the worker (different failure mode than intended).
Consider polling processor.getStats().activeTasks to confirm the task is in-flight before disposing, or increase the delay with a comment explaining the rationale.
🤖 Prompt for AI Agents
In `@test/parallel-processor.test.ts` around lines 696 - 720, The test's fixed
50ms sleep is flaky; replace it with a small polling loop that waits until
processor.getStats().activeTasks > 0 (or a short timeout like 1s) before calling
processor.dispose(), so you ensure the task is actually in-flight; update the it
block around the executeTasks/await processor.dispose() sequence to poll
getStats().activeTasks (or fall back to a longer delay with a comment) to avoid
CI timing issues while preserving the existing test timeout.
Comment on lines
7
to
19
| describe('processUtils.exec', () => { | ||
| it('rejects with a clear timeout error when the subprocess hangs', async () => { | ||
| const pythonPath = getDefaultPythonPath(); | ||
| const check = spawnSync(pythonPath, ['--version'], { encoding: 'utf-8' }); | ||
| if (check.error || check.status !== 0) { | ||
| return; // Python not available | ||
| } | ||
| const timeoutMs = 200; | ||
|
|
||
| await expect( | ||
| processUtils.exec(pythonPath, ['-c', 'import time; time.sleep(999)'], { timeoutMs }) | ||
| ).rejects.toThrow(`timed out after ${timeoutMs}ms`); | ||
| }, 10_000); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Well-structured test, consistent with project conventions for Python-optional guards.
The early return on missing Python aligns with the project's soft-skip approach. One minor hardening suggestion: the spawnSync availability check on line 10 has no timeout of its own—if the Python binary were present but hung, this blocks until the outer 10 s Vitest timeout kicks in. Adding a short timeout makes the guard self-contained.
🛡️ Optional: add timeout to the availability probe
- const check = spawnSync(pythonPath, ['--version'], { encoding: 'utf-8' });
+ const check = spawnSync(pythonPath, ['--version'], { encoding: 'utf-8', timeout: 5_000 });Based on learnings, the early-return pattern for missing Python is the preferred convention ("prefer early returns when Python or fixture files are unavailable … a silent pass is intentional for environments lacking Python/fixtures").
🤖 Prompt for AI Agents
In `@test/process_utils_timeout.test.ts` around lines 7 - 19, The availability
probe uses spawnSync(pythonPath, ['--version'], { encoding: 'utf-8' }) without a
timeout, so if the Python binary hangs the test blocks; update that probe call
in test/process_utils_timeout.test.ts (the spawnSync check near
getDefaultPythonPath() and timeoutMs) to include a short timeout option (e.g.,
~1000ms) in its options so the guard self-contains its failure path and returns
early instead of relying on the outer Vitest timeout.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f479efc79f
ℹ️ 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".
runtime/python_bridge.py
Outdated
Comment on lines
163
to
164
| if not isinstance(b64, str) or not b64: | ||
| raise ProtocolError('Invalid bytes envelope: missing b64') |
There was a problem hiding this comment.
The new envelope validator treats b64: "" / data: "" as missing (or not b64 / or not data), but empty base64 is the correct encoding for b'' / new Uint8Array(0). Calls that pass zero-length binary arguments now raise ProtocolError instead of reaching the target Python function, so bytes round-trip still fails for a valid and common edge case.
Useful? React with 👍 / 👎.
runtime/python_bridge.py
Outdated
| if not isinstance(b64, str) or not b64: | ||
| raise ProtocolError('Invalid bytes envelope: missing b64') | ||
| try: | ||
| return base64.b64decode(b64) |
There was a problem hiding this comment.
base64.b64decode(...) is called without strict validation, so malformed envelopes (for example b64: "%%%") can decode to empty bytes instead of erroring. This silently corrupts binary inputs at the transport boundary (the same decode pattern is also used in the legacy data branch), making payload corruption difficult to detect.
Useful? React with 👍 / 👎.
(cherry picked from commit f479efc)
bbopen
force-pushed
the
codex/tywrap-open-issue-reliability-pass
branch
from
f479efc to
7b5137b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/safe-codec.test.ts`:
- Around line 360-368: Add a companion test that covers the alternative envelope
shape handled by the reviver: create a payload JSON with result: {
__tywrap_bytes__: true, b64: 'SGVsbG8=' }, call
codec.decodeResponse<Uint8Array>(payload), and assert the return is a Uint8Array
and its bytes equal [72,101,108,108,111]; this mirrors the existing test that
uses { __type__: 'bytes' } and ensures encodeRequest/decoding paths for
__tywrap_bytes__ are verified.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
runtime/python_bridge.pysrc/runtime/process-io.tssrc/runtime/safe-codec.tstest/runtime_node.test.tstest/safe-codec.test.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/safe-codec.test.tstest/runtime_node.test.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/safe-codec.test.tssrc/runtime/process-io.tssrc/runtime/safe-codec.tstest/runtime_node.test.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
test/safe-codec.test.tssrc/runtime/process-io.tssrc/runtime/safe-codec.tstest/runtime_node.test.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/safe-codec.test.tssrc/runtime/process-io.tssrc/runtime/safe-codec.tstest/runtime_node.test.ts
📚 Learning: 2026-01-19T21:14:29.869Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
Learning: In the runtime env-var parsing (e.g., in src/runtime/bridge-core.ts and similar modules), adopt a tolerant, best-effort policy: numeric env values should parse by taking the leading numeric portion (e.g., TYWRAP_CODEC_MAX_BYTES=1024abc -> 1024). Only reject clearly invalid values (non-numeric start or <= 0). This reduces surprising failures from minor typos. Add tests to cover partial numeric prefixes, and ensure downstream logic documents the trusted range; consider a small helper to extract a positive integer from a string with a safe fallback.
Applied to files:
src/runtime/process-io.tssrc/runtime/safe-codec.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/process-io.tssrc/runtime/safe-codec.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.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
test/runtime_node.test.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/runtime_node.test.tsruntime/python_bridge.py
🧬 Code graph analysis (3)
test/safe-codec.test.ts (1)
test/python/test_safe_codec.py (1)
codec(35-37)
src/runtime/safe-codec.ts (1)
src/runtime/errors.ts (1)
BridgeCodecError(17-29)
test/runtime_node.test.ts (1)
scripts/act.mjs (1)
output(59-59)
🪛 Ruff (0.15.0)
runtime/python_bridge.py
[warning] 147-147: Missing return type annotation for private function _deserialize_bytes_envelope
(ANN202)
[warning] 164-164: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 168-168: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 173-173: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 177-177: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (6)
src/runtime/process-io.ts (1)
736-739: LGTM — defensiveunref()guard.The runtime check ensures compatibility with environments where
setTimeoutdoesn't return a Node.jsTimeoutobject with anunrefmethod.runtime/python_bridge.py (2)
182-198: LGTM — recursive deserialization is clean and correct.The sentinel-based approach avoids ambiguity between "decoded to
None" and "not a bytes envelope". The recursive walk over lists and dicts correctly propagates deserialization to nested values. Since JSON-parsed data is acyclic, no cycle guard is needed.
707-708: LGTM — consistent integration across all three handler entry points.
deserialize(coerce_list(...))anddeserialize(coerce_dict(...))are applied uniformly tohandle_call,handle_instantiate, andhandle_call_method, matching the PR objective.Also applies to: 718-719, 731-732
src/runtime/safe-codec.ts (2)
336-350: LGTM —fromBase64correctly handles both Node and browser runtimes.The
Buffer.frompath correctly slices withbyteOffsetandlengthto avoid issues with Node's pooledArrayBuffer. Theatobfallback covers non-Node environments, and the error for unsupported runtimes is clear.
359-374: Asymmetry is intentional but deserves a comment explaining the design.
bytesHandlinggates encoding inencodeRequestbutreviveValueunconditionally decodes bytes envelopes during response parsing. This is by design—bytesHandling: 'reject'prevents us from sending binary data, but we must always accept bytes from responses since we cannot control what the remote side returns.However, this differs from
rejectSpecialFloats, which is validated on both encode and decode. Consider adding a comment toreviveValueexplaining why decode ignoresbytesHandlingto clarify this intentional asymmetry. Also add a test that verifies a codec withbytesHandling: 'reject'can still decode bytes from responses.test/runtime_node.test.ts (1)
115-131: Good end-to-end regression test for the bytes round-trip.This exercises the full pipeline: JS
Uint8Array→ base64 envelope → Pythonbytes→ response envelope → JSUint8Array. Thelenassertion confirms Python receives realbytes(not a dict), and thebytescall confirms the return path.
Owner
Author
|
Requesting full automated reviews on the cleaned PR history after the branch reset. @codex review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b5137b489
ℹ️ 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".
runtime/python_bridge.py
Outdated
Comment on lines
163
to
164
| if not isinstance(b64, str) or not b64: | ||
| raise ProtocolError('Invalid bytes envelope: missing b64') |
There was a problem hiding this comment.
The new bytes deserializer rejects empty strings (not b64 / not data) as “missing”, but empty binary payloads are valid and are encoded as "" by both JS (Uint8Array(0) via toString('base64')) and Python (base64.b64encode(b'').decode('ascii')). This makes legitimate zero-length byte arguments fail with ProtocolError instead of round-tripping, so calls that pass empty blobs now break at the JS↔Python boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/safe-codec.ts (1)
508-517:⚠️ Potential issue | 🟡 MinorBase64 decode errors will surface as misleading "JSON parse failed" messages.
If
fromBase64throws inside the reviver (e.g., malformed base64 in a bytes envelope), the error propagates out ofJSON.parseand is caught here as a "JSON parse failed" error. The JSON itself was valid — the real issue is base64 decoding. Consider catching base64 errors inreviveValueand wrapping them with a clearer message.Proposed fix
private reviveValue(_key: string, value: unknown): unknown { if (value === null || typeof value !== 'object' || Array.isArray(value)) { return value; } const obj = value as Record<string, unknown>; if (obj.__tywrap_bytes__ === true && typeof obj.b64 === 'string') { - return this.fromBase64(obj.b64); + try { + return this.fromBase64(obj.b64); + } catch (err) { + throw new BridgeCodecError( + `Failed to decode bytes envelope: ${err instanceof Error ? err.message : String(err)}`, + { codecPhase: 'decode', valueType: 'bytes' } + ); + } } if (obj.__type__ === 'bytes' && obj.encoding === 'base64' && typeof obj.data === 'string') { - return this.fromBase64(obj.data); + try { + return this.fromBase64(obj.data); + } catch (err) { + throw new BridgeCodecError( + `Failed to decode bytes envelope: ${err instanceof Error ? err.message : String(err)}`, + { codecPhase: 'decode', valueType: 'bytes' } + ); + } } return value; }The
BridgeCodecErrorthrown here would then be caught by the outer catch block but re-thrown as-is (line 463-464 pattern), providing a clear error rather than "JSON parse failed".Wait — actually, looking at the catch block (lines 511-517), it does not re-throw
BridgeCodecError— it catches all errors uniformly and wraps them. This differs fromencodeRequest(lines 462-468) which checks forBridgeCodecErrorandBridgeProtocolErrorfirst. The decode catch block should do the same.Additional fix in decodeResponse catch block (and same for decodeResponseAsync)
} catch (err) { + if (err instanceof BridgeCodecError) { + throw err; + } const errorMessage = err instanceof Error ? err.message : String(err); throw new BridgeCodecError(🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@src/runtime/safe-codec.ts` around lines 508 - 517, The JSON parse catch is masking errors thrown by the reviver (e.g., fromBase64) as "JSON parse failed"; update the decode path to preserve and rethrow existing BridgeCodecError/BridgeProtocolError like encodeRequest does: in the try/catch around JSON.parse (used with reviveValue) detect if the caught error is already a BridgeCodecError or BridgeProtocolError and rethrow it, otherwise wrap non-bridge errors in a new BridgeCodecError with the "JSON parse failed" message and payload snippet; additionally, adjust reviveValue to catch base64/fromBase64 failures and throw a BridgeCodecError with a clear message identifying base64 decode failure so decodeResponse and decodeResponseAsync receive a specific error to propagate.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@src/runtime/safe-codec.ts`:
- Around line 508-517: The JSON parse catch is masking errors thrown by the
reviver (e.g., fromBase64) as "JSON parse failed"; update the decode path to
preserve and rethrow existing BridgeCodecError/BridgeProtocolError like
encodeRequest does: in the try/catch around JSON.parse (used with reviveValue)
detect if the caught error is already a BridgeCodecError or BridgeProtocolError
and rethrow it, otherwise wrap non-bridge errors in a new BridgeCodecError with
the "JSON parse failed" message and payload snippet; additionally, adjust
reviveValue to catch base64/fromBase64 failures and throw a BridgeCodecError
with a clear message identifying base64 decode failure so decodeResponse and
decodeResponseAsync receive a specific error to propagate.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
runtime/python_bridge.pysrc/runtime/process-io.tssrc/runtime/safe-codec.tstest/runtime_node.test.tstest/safe-codec.test.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: runtime/python_bridge.py:99-123
Timestamp: 2026-01-19T21:49:05.612Z
Learning: In the tywrap repository, TYWRAP_REQUEST_MAX_BYTES uses strict integer parsing that rejects values with trailing characters (e.g., "1024abc"). This differs from TYWRAP_CODEC_MAX_BYTES, which uses tolerant/best-effort parsing that accepts numeric prefixes. The strict policy for REQUEST_MAX_BYTES ensures explicit integer values and consistent parse behavior across Node/Python implementations.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:238-249
Timestamp: 2026-01-20T01:33:12.841Z
Learning: In the tywrap repository, when reviewing init/startup patterns, prioritize cases that break core functionality over edge cases with diagnostic-only impact (e.g., missing bridgeInfo metadata). If workers are healthy and operational, defer low-impact defensive checks to follow-up PRs.
📚 Learning: 2026-01-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/process-io.tstest/safe-codec.test.tstest/runtime_node.test.tssrc/runtime/safe-codec.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/process-io.tssrc/runtime/safe-codec.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/process-io.tssrc/runtime/safe-codec.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/process-io.tstest/safe-codec.test.tstest/runtime_node.test.tssrc/runtime/safe-codec.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/process-io.tstest/safe-codec.test.tstest/runtime_node.test.tssrc/runtime/safe-codec.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/safe-codec.test.tstest/runtime_node.test.tsruntime/python_bridge.py
📚 Learning: 2026-01-19T21:14:37.032Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:37.032Z
Learning: In tywrap (src/runtime/bridge-core.ts and similar), environment variable parsing follows a tolerant/best-effort policy. For example, `TYWRAP_CODEC_MAX_BYTES=1024abc` should be accepted as 1024. Only reject clearly invalid values (non-numeric start or <=0). This avoids surprising failures from minor typos.
Applied to files:
test/safe-codec.test.ts
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/safe-codec.test.tstest/runtime_node.test.ts
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/safe-codec.test.tstest/runtime_node.test.ts
📚 Learning: 2026-01-20T16:01:14.323Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:599-602
Timestamp: 2026-01-20T16:01:14.323Z
Learning: In `src/runtime/node-bridge.ts` (NodeBridge), a test message is sent to the Python subprocess to confirm the bridge is responsive before marking it as ready.
Applied to files:
test/runtime_node.test.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
test/runtime_node.test.tssrc/runtime/safe-codec.ts
🧬 Code graph analysis (2)
test/runtime_node.test.ts (1)
scripts/act.mjs (1)
output(59-59)
src/runtime/safe-codec.ts (1)
src/runtime/errors.ts (1)
BridgeCodecError(17-29)
🪛 Ruff (0.15.0)
runtime/python_bridge.py
[warning] 147-147: Missing return type annotation for private function _deserialize_bytes_envelope
(ANN202)
[warning] 164-164: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 168-168: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 173-173: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 177-177: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
src/runtime/process-io.ts (1)
736-739: LGTM — safe guard for cross-runtime compatibility.The
typeofcheck before callingunref()is the correct defensive pattern for runtimes that don't expose Node-style timer handles. This is outside the hot request/response loop, so the extra branch has no meaningful overhead.runtime/python_bridge.py (2)
144-198: Well-structured sentinel + recursive deserialization pattern.The
_NO_DESERIALIZEsentinel cleanly distinguishes "not a bytes envelope" from a decodedbytesvalue, and the recursivedeserializecorrectly walks only JSON-parsed structures (no cycle risk). Consistent application across all three handler boundaries (lines 707–708, 718–719, 731–732) is good.
707-708: Deserialization correctly applied at all three request handler boundaries.
handle_call,handle_instantiate, andhandle_call_methodall usedeserialize(coerce_list(...))/deserialize(coerce_dict(...))consistently, ensuring byte envelopes in args and kwargs are decoded before invocation.Also applies to: 718-719, 731-732
src/runtime/safe-codec.ts (2)
330-350: LGTM —fromBase64correctly handles both Node and browser runtimes.The
Buffer.frompath correctly extractsbyteOffsetandlengthfrom the NodeBuffer, and theatobpath usesArray.fromwith a mapping function (no spread-operator stack limit concern). Empty base64 strings are handled correctly on the JS side.
359-374: LGTM — reviver handles both envelope shapes cleanly.The bottom-up evaluation order of JSON.parse revivers means nested bytes envelopes are correctly decoded before their parent objects are processed. The type checks are minimal and appropriate for the controlled protocol.
test/safe-codec.test.ts (1)
360-368: Good regression test for the legacy bytes envelope path.The test correctly validates that the
__type__: 'bytes'envelope with base64 data decodes to aUint8Arraywith the expected byte values throughdecodeResponse. A companion test for the__tywrap_bytes__envelope shape would complete coverage of both reviver branches, as noted in a prior review.test/runtime_node.test.ts (1)
115-131: Solid end-to-end regression test for the bytes round-trip.This test exercises both directions of the fix: JS
Uint8Array→ Pythonbytes(viabuiltins.len) and Pythonbytes→ JSUint8Array(viabuiltins.bytes). The Python-availability guard follows the established repo pattern.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes byte-boundary interoperability between JavaScript and Python for Tywrap runtime calls.
bytesin:handle_callhandle_instantiatehandle_call_methodSafeCodecnow revives Python/JS bytes envelopes in both:decodeResponsedecodeResponseAsyncunref()inProcessIOto avoid runtime crashes on runtimes without Node timer unref semantics.Uint8Arrayround-trip through Pythonlen/bytesSafeCodecValidation
npx vitest --run test/safe-codec.test.ts test/runtime_node.test.tsCloses #193.