feat(runtime): implement BoundedContext abstraction #150
Conversation
This PR introduces a unified BoundedContext base class that provides consistent handling of cross-boundary concerns across all runtime bridges. Key features: - Lifecycle management (idle → initializing → ready → disposing → disposed) - Init deduplication and retry on failure - Validation helpers (validateNumeric, validatePositive, etc.) - Error classification (pattern-matching to BridgeError subtypes) - Bounded execution with timeout, retry, and abort signal support - Resource tracking for automatic cleanup on dispose All three bridges (NodeBridge, PyodideBridge, HttpBridge) have been migrated to extend BoundedContext. RuntimeBridge is now deprecated but remains as an alias for backwards compatibility. Closes #149 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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. 📝 WalkthroughWalkthroughAdds a new BoundedContext abstraction centralizing lifecycle, validation, bounded execution, and resource ownership; introduces Disposable utilities and validators; migrates runtime bridges to extend BoundedContext; preserves RuntimeBridge as a deprecated compatibility export; adds comprehensive tests. Changes
Sequence DiagramsequenceDiagram
participant Consumer as Consumer
participant Bounded as BoundedContext
participant Subclass as Subclass(doInit/doDispose)
participant Resource as ResourceTracker
participant ErrorH as ErrorHandler
Consumer->>Bounded: init()
activate Bounded
Bounded->>Bounded: state = initializing
Bounded->>Subclass: doInit()
activate Subclass
Subclass-->>Bounded: init complete
deactivate Subclass
Bounded->>Bounded: state = ready
deactivate Bounded
Consumer->>Bounded: execute(operation, {timeoutMs,retries,signal})
activate Bounded
Bounded->>Bounded: auto-init if needed
loop retries
Bounded->>Bounded: withTimeout(operation, timeoutMs)
alt success
Bounded->>Bounded: validate output
Bounded-->>Consumer: result
else timeout / abort
Bounded->>ErrorH: classifyError()
ErrorH-->>Bounded: BridgeTimeoutError
Bounded->>Bounded: delay(retryDelayMs)
else non-retryable
Bounded-->>Consumer: throw error
end
end
deactivate Bounded
Consumer->>Bounded: trackResource(resource)
activate Bounded
Bounded->>Resource: add resource
Resource-->>Consumer: tracked
deactivate Bounded
Consumer->>Bounded: dispose()
activate Bounded
Bounded->>Bounded: state = disposing
Bounded->>Resource: disposeAll(tracked)
activate Resource
Resource-->>Bounded: aggregated errors[]
deactivate Resource
Bounded->>Subclass: doDispose()
activate Subclass
Subclass-->>Bounded: dispose complete
deactivate Subclass
Bounded->>Bounded: state = disposed
Bounded-->>Consumer: resolved / throws AggregateError
deactivate Bounded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
bot
added
area:runtime-node
![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: cc4f5bf207
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/node.ts (1)
428-446: Timer leak in recursive polling pattern.When the timeout fires and rejects the promise, any pending recursive
setTimeout(checkWorker, 10)is not cleared. This can causecheckWorkerto continue executing after the promise has settled, leading to timer leaks.Based on learnings, track and clear the recursive timer on all settlement paths.
🔧 Proposed fix to track and clear the recursive timer
private async waitForAvailableWorker(timeoutMs: number = 5000): Promise<WorkerProcess> { return new Promise((resolvePromise, reject) => { + let checkTimer: NodeJS.Timeout | undefined; + const timeout = setTimeout(() => { + if (checkTimer) clearTimeout(checkTimer); reject(new Error('Timeout waiting for available worker')); }, timeoutMs); const checkWorker = (): void => { const worker = this.selectOptimalWorker(); if (worker) { clearTimeout(timeout); resolvePromise(worker); } else { // Check again in 10ms - setTimeout(checkWorker, 10); + checkTimer = setTimeout(checkWorker, 10); } }; checkWorker(); }); }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
src/index.tssrc/runtime/base.tssrc/runtime/bounded-context.tssrc/runtime/disposable.tssrc/runtime/http.tssrc/runtime/node.tssrc/runtime/pyodide.tssrc/runtime/validators.tstest/bounded-context.test.tstest/disposable.test.tstest/validators.test.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
📚 Learning: 2026-01-19T21: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/validators.test.tstest/disposable.test.tstest/bounded-context.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/validators.test.tstest/disposable.test.tssrc/runtime/disposable.tssrc/runtime/base.tssrc/runtime/node.tssrc/runtime/http.tssrc/runtime/validators.tstest/bounded-context.test.tssrc/runtime/pyodide.tssrc/runtime/bounded-context.tssrc/index.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/disposable.tssrc/runtime/base.tssrc/runtime/node.tssrc/runtime/http.tssrc/runtime/validators.tssrc/runtime/pyodide.tssrc/runtime/bounded-context.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/disposable.tssrc/runtime/base.tssrc/runtime/node.tssrc/runtime/http.tssrc/runtime/validators.tssrc/runtime/pyodide.tssrc/runtime/bounded-context.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/base.tssrc/runtime/node.tssrc/runtime/http.tssrc/runtime/pyodide.tssrc/runtime/bounded-context.tssrc/index.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/bounded-context.test.ts
🧬 Code graph analysis (9)
test/validators.test.ts (1)
src/runtime/validators.ts (17)
isFiniteNumber(25-27)isPositiveNumber(32-34)isNonNegativeNumber(39-41)isNonEmptyString(46-48)isPlainObject(53-55)assertFiniteNumber(69-75)ValidationError(11-16)assertPositive(85-91)assertNonNegative(101-107)assertString(117-122)assertNonEmptyString(132-138)assertArray(148-153)assertObject(163-169)containsSpecialFloat(186-197)assertNoSpecialFloats(206-212)sanitizeForFilename(225-231)containsPathTraversal(239-241)
test/disposable.test.ts (1)
src/runtime/disposable.ts (4)
Disposable(11-17)isDisposable(25-31)safeDispose(40-44)disposeAll(53-65)
src/runtime/disposable.ts (1)
src/index.ts (4)
Disposable(14-14)isDisposable(15-15)safeDispose(15-15)disposeAll(15-15)
src/runtime/base.ts (1)
src/index.ts (2)
RuntimeBridge(39-39)BoundedContext(13-13)
src/runtime/node.ts (2)
src/index.ts (2)
NodeBridge(51-51)BoundedContext(13-13)src/runtime/base.ts (1)
BoundedContext(24-24)
src/runtime/validators.ts (1)
src/index.ts (17)
ValidationError(17-17)isFiniteNumber(18-18)isPositiveNumber(19-19)isNonNegativeNumber(20-20)isNonEmptyString(21-21)isPlainObject(22-22)assertFiniteNumber(23-23)assertPositive(24-24)assertNonNegative(25-25)assertString(26-26)assertNonEmptyString(27-27)assertArray(28-28)assertObject(29-29)containsSpecialFloat(30-30)assertNoSpecialFloats(31-31)sanitizeForFilename(32-32)containsPathTraversal(33-33)
test/bounded-context.test.ts (2)
src/runtime/bounded-context.ts (2)
ExecuteOptions(52-63)ContextState(47-47)src/runtime/disposable.ts (1)
Disposable(11-17)
src/runtime/pyodide.ts (1)
src/runtime/base.ts (1)
BoundedContext(24-24)
src/runtime/bounded-context.ts (3)
src/types/index.ts (1)
RuntimeExecution(373-398)src/runtime/disposable.ts (2)
Disposable(11-17)disposeAll(53-65)src/runtime/validators.ts (5)
assertFiniteNumber(69-75)assertPositive(85-91)assertString(117-122)assertNonEmptyString(132-138)ValidationError(11-16)
🔇 Additional comments (53)
src/runtime/disposable.ts (4)
1-17: Clear Disposable contract and idempotency guidance.
19-31:isDisposableguard is precise and safe.
33-44:safeDisposebehavior is consistent and predictable.
46-65:disposeAllbest-effort disposal and error aggregation look good.test/validators.test.ts (5)
33-141: Type-guard coverage is thorough across numbers, strings, and objects.
143-263: Assertion tests validate both success paths and error messaging well.
264-337: Special-float detection tests cover nested and mixed structures.
339-397: Path traversal and filename sanitization tests are comprehensive.
399-418: ValidationError behavior is correctly verified.test/disposable.test.ts (4)
19-35: Fixtures cover both success and failure disposal paths.
41-83:isDisposabletests cover positive/negative cases well.
89-107:safeDisposetests correctly exercise no-op and error propagation.
113-204:disposeAlltests cover ordering, aggregation, and collection types.src/runtime/http.ts (2)
7-65: Lifecycle hooks and BoundedContext inheritance are clean and consistent.
105-139: Updated HTTP error mapping is clear and improves error typing.test/bounded-context.test.ts (6)
27-117: Test fixtures expose the protected surface cleanly for coverage.
123-299: Lifecycle tests cover state transitions, idempotency, and failure recovery.
304-361: Validation helper tests are solid and targeted.
367-463: Error classification tests are comprehensive and well-scoped.
469-659: Bounded execution tests cover timeout, retry, abort, and validation paths.
665-739: Resource ownership tests validate tracking/untracking and disposal.src/runtime/base.ts (1)
1-24: No changes needed. RuntimeBridge correctly inherits allRuntimeExecutioncontract methods through its parent classBoundedContext, which explicitly implements theRuntimeExecutioninterface. The class declares all required methods (call,instantiate,callMethod,disposeInstance) as abstract, and inheriting from it preserves the full type contract for consumers. This is not a breaking change.src/runtime/node.ts (6)
24-25: LGTM!Import changes correctly bring in
BoundedContextas the new base class andBridgeProtocolErrorfor error handling, aligning with the bounded-context lifecycle model.
184-234: LGTM!The class properly extends
BoundedContext, callssuper()in the constructor, and defers initialization todoInit(). The cleanup scheduler is appropriately started in the constructor for pool management.
236-259: LGTM!The
doInit()implementation correctly:
- Validates the Python bridge script exists before proceeding
- Registers the Arrow decoder for value decoding
- Spawns the minimum required processes
- Validates protocol version via
refreshBridgeInfo()- Executes warmup commands if configured
485-491: LGTM!Correct use of
this.isDisposedto guard against spawning replacement workers after the bridge has been disposed. This integrates properly with theBoundedContextlifecycle.
614-618: LGTM!The
isDisposedcheck correctly prevents spawning replacement workers when the bridge is being or has been disposed.
803-820: LGTM!The
doDispose()implementation correctly:
- Clears the cleanup interval timer to prevent post-disposal callbacks
- Force-terminates all worker processes
- Waits for all terminations to complete
- Clears the process pool and removes event listeners
src/runtime/pyodide.ts (8)
5-6: LGTM!Import changes correctly bring in
BoundedContextandBridgeProtocolErrorfor the new lifecycle and error handling model.
23-32: LGTM!The class properly extends
BoundedContext, callssuper(), and stores configuration as readonly properties.
34-44: LGTM!The
doInit()implementation correctly:
- Resolves the
loadPyodidefunction from global or dynamic import- Throws
BridgeProtocolErrorfor environment unavailability- Initializes Pyodide and loads configured packages
- Bootstraps the Python helper functions
95-121: LGTM!The
call()method correctly:
- Ensures initialization via
await this.init()- Guards against uninitialized Pyodide or missing helpers
- Uses
BridgeProtocolErrorfor protocol/environment issues- Properly cleans up PyProxy objects in the
finallyblock
123-149: LGTM!The
instantiate()method follows the same correct pattern: init check, guards, and cleanup.
151-177: LGTM!The
callMethod()method correctly follows the established pattern with proper initialization, guards, and cleanup.
179-195: LGTM!The
disposeInstance()method correctly handles instance disposal with proper initialization and cleanup.
197-200: LGTM!The
doDispose()correctly clears the Pyodide reference, allowing garbage collection. The comment appropriately documents that Pyodide lacks an explicit disposal API.src/index.ts (2)
12-34: LGTM!The new exports are well-organized:
BoundedContextwith related types for cross-boundary concernsDisposabletype and utilities for resource management- Comprehensive validator functions for runtime value checking
This provides a clean public API for the new abstractions.
36-39: LGTM!The deprecation notice for
RuntimeBridgeis clear and provides migration guidance toBoundedContext. Keeping the export maintains backward compatibility during the transition period.src/runtime/validators.ts (5)
11-16: LGTM!The
ValidationErrorclass properly extendsErrorand sets thenameproperty for better error identification in catch blocks and logs.
22-55: LGTM!The type guards are well-implemented:
isFiniteNumbercorrectly usesNumber.isFinite()to excludeNaNandInfinity- Numeric guards compose properly
isPlainObjectcorrectly handlesnulland array edge cases
61-169: LGTM!The assertion functions are well-designed:
- Return typed values enabling assignment patterns like
const num = assertPositive(value, 'count')- Provide descriptive error messages with actual values for debugging
- Properly compose to avoid code duplication
175-212: LGTM!The special float detection correctly:
- Recursively traverses arrays and plain objects
- Detects
NaN,Infinity, and-Infinitythat would cause JSON serialization issues- Provides a clear error message explaining the JSON serialization problem
218-241: LGTM!The path validation functions provide appropriate security measures:
sanitizeForFilenameneutralizes path traversal patterns and invalid filename characterscontainsPathTraversaldetects common directory traversal attemptsThe strict detection of any
/or\is appropriate for filename-only contexts.src/runtime/bounded-context.ts (10)
1-33: LGTM!The module documentation clearly explains the five unified concerns (lifecycle, validation, error classification, bounded execution, resource tracking). Imports are appropriate for the functionality provided.
38-63: LGTM!The
ContextStatetype andExecuteOptionsinterface are well-designed:
- State machine transitions are clearly documented
- Options provide comprehensive execution control (timeout, retries, validation, abort)
- Generic type parameter on
validateensures type safety
121-145: LGTM!The
init()method correctly implements:
- Idempotent behavior (returns immediately if ready)
- Concurrent call deduplication (returns the same promise)
- Retry-on-failure semantics (resets to idle on error)
- Error classification before propagating failures
159-195: LGTM!The
dispose()method correctly:
- Provides idempotent disposal (safe to call multiple times)
- Disposes tracked resources before subclass cleanup
- Always transitions to the terminal
disposedstate- Properly aggregates multiple disposal errors using
AggregateError
318-359: LGTM!The
classifyError()method provides sensible error classification:
- Preserves existing
BridgeErrorinstances to avoid double-wrapping- Uses context state for disposed-related errors
- Pattern-matches common timeout and protocol error messages
- Falls back to
BridgeExecutionErrorfor unclassified errors
382-425: LGTM!The
execute()method provides robust bounded execution:
- Auto-initialization for lazy startup
- Disposed check prevents operations on disposed contexts
- Retry loop with configurable attempts and linear backoff
- Abort signal checked before each attempt
- Optional result validation callback
- Error classification integrated into the flow
436-469: LGTM!The
withTimeout()method correctly manages resources:
- Disables timeout when
ms <= 0or non-finite- Properly clears the timer on both success and failure paths
- Handles abort signals with proper listener cleanup
- Uses
{ once: true }for the abort listener as a safety measure
478-491: LGTM!The
isRetryable()method provides sensible defaults:
- Timeout errors are transient and worth retrying
- Connection errors (
ECONNRESET,ECONNREFUSED,EPIPE) indicate transient failures- Protected visibility allows subclasses to customize retry logic
515-538: LGTM!The resource tracking implementation is clean:
trackResourcereturns the resource for convenient chaininguntrackResourceallows manual cleanup before automatic disposalresourceCountgetter provides visibility into tracked resources- Protected visibility allows subclasses to manage their own resources
544-577: LGTM!The abstract method declarations correctly mirror the
RuntimeExecutioninterface, ensuring all bridge implementations provide the required Python interop operations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Guard init completion from reviving disposed context: If dispose() is called during init(), the .then() handler now checks state before setting to 'ready', preventing the disposed state from being overwritten. - Honor abort signals even when timeouts are disabled: When timeoutMs=0, the abort signal is now properly honored by wrapping the promise with an abort handler. Fixes P2 issues from PR review. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@codex full review of the PR with the latest changes please |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/runtime/bounded-context.ts`:
- Around line 121-145: The init() path currently allows starting while the
context is 'disposing' and its catch handler unconditionally resets _state to
'idle', reviving a disposed context; change init() to guard against running
during disposal by throwing BridgeDisposedError if this._state === 'disposing'
before setting this._state = 'initializing', and in the .catch handler only
reset this._state = 'idle' and this._initPromise = undefined when this._state
=== 'initializing' (leave it untouched if it became 'disposing' or 'disposed');
keep references to init(), dispose(), _state, _initPromise, doInit(), and
classifyError() when making these checks.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/runtime/bounded-context.tstest/bounded-context.test.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
📚 Learning: 2026-01-19T21: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/bounded-context.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/bounded-context.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/bounded-context.test.tssrc/runtime/bounded-context.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/bounded-context.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/bounded-context.ts
🧬 Code graph analysis (2)
test/bounded-context.test.ts (2)
src/runtime/bounded-context.ts (2)
ExecuteOptions(52-63)ContextState(47-47)src/runtime/disposable.ts (1)
Disposable(11-17)
src/runtime/bounded-context.ts (3)
src/types/index.ts (1)
RuntimeExecution(373-398)src/runtime/disposable.ts (2)
Disposable(11-17)disposeAll(53-65)src/runtime/validators.ts (5)
assertFiniteNumber(69-75)assertPositive(85-91)assertString(117-122)assertNonEmptyString(132-138)ValidationError(11-16)
🔇 Additional comments (1)
test/bounded-context.test.ts (1)
298-316: Good regression coverage for dispose‑during‑init race.The test directly asserts the non‑revival invariant, which is exactly the failure mode we want to prevent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41df35980a
ℹ️ 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".
…revival - Guard against 'disposing' state in init() by throwing BridgeDisposedError when this._state === 'disposing' (in addition to 'disposed') - Only reset state to 'idle' in catch handler if this._state is still 'initializing' - prevents reviving a disposed/disposing context when init fails after dispose was called Addresses codex/coderabbit review comments. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/runtime/bounded-context.ts`:
- Around line 407-427: The retry loop in bounded-context.ts currently uses
linear backoff (retryDelayMs * (attempt + 1)) but tests/docs claim exponential
backoff; change the delay calculation in the retry block of the loop that calls
this.delay(...) to exponential (e.g., retryDelayMs * 2 ** attempt) so attempt
0→retryDelayMs, attempt 1→2×, attempt 2→4×, and keep existing checks
(signal?.aborted, withTimeout, classifyError, isRetryable) intact;
alternatively, if you prefer to keep linear behavior, update the
test/documentation that references exponential backoff to reflect the linear
formula instead.
- Around line 166-202: The dispose() method should deduplicate concurrent calls
by caching a promise (e.g., add a private _disposePromise: Promise<void>), so
when _state === 'disposing' callers return await this._disposePromise instead of
returning immediately; set _disposePromise at the start of disposal, perform
existing logic (disposeAll(this._resources), await this.doDispose(), collect
errors into allErrors, set _state = 'disposed' and this._initPromise =
undefined), then resolve or reject _disposePromise accordingly and finally clear
_disposePromise; ensure errors are thrown as before (single error or
AggregateError) and that _resources.clear() still occurs.
In `@test/bounded-context.test.ts`:
- Around line 635-652: The test title is misleading—implementation in execute()
(invoked via context.testExecute) uses linear backoff (retryDelayMs * (attempt +
1)) not exponential; change the test name from "applies exponential backoff" to
"applies linear backoff" (or alternatively update execute() to use exponential
math if that was intended) so the test name matches behavior, and keep
references to context.testExecute and the execute retry logic consistent.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/runtime/bounded-context.tstest/bounded-context.test.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
📚 Learning: 2026-01-19T21: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/bounded-context.test.tssrc/runtime/bounded-context.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/bounded-context.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/bounded-context.test.tssrc/runtime/bounded-context.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/bounded-context.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/bounded-context.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/bounded-context.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/bounded-context.ts
🧬 Code graph analysis (1)
test/bounded-context.test.ts (2)
src/runtime/bounded-context.ts (2)
ExecuteOptions(52-63)ContextState(47-47)src/runtime/disposable.ts (1)
Disposable(11-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: os (windows-latest)
🔇 Additional comments (13)
src/runtime/bounded-context.ts (8)
1-63: LGTM!The types and imports are well-structured. The
ContextStateunion clearly documents the lifecycle states, andExecuteOptionsprovides a clean interface for bounded execution configuration with sensible defaults.
75-103: LGTM!State encapsulation is clean with appropriate getters. The
isDisposedcorrectly reflects only the terminaldisposedstate, distinguishing it from the transitionaldisposingstate.
121-152: LGTM!The
init()implementation correctly addresses the previously raised concerns:
- Guards against both
disposedanddisposingstates.- The
.then()handler only transitions toreadyif stillinitializing, preventing revival after disposal.- The
.catch()handler only resets toidleif stillinitializing, avoiding resurrection of a disposed context.
220-308: LGTM!The validation helpers follow a consistent pattern: wrap assertion utilities, catch errors, and reclassify through
classifyError(). The pass-throughvalidateInput/validateOutputhooks provide clean extension points for subclasses.
325-366: LGTM!The error classification logic is well-structured with clear precedence: existing
BridgeErrorpass-through → disposed state → pattern matching → default. The cause preservation ensures debuggability.
443-499: LGTM!The
withTimeout()implementation correctly handles all edge cases:
- Already-aborted signals are detected immediately.
- When timeout is disabled (
ms <= 0), abort signals are still honored via the dedicated wrapper (lines 455-469).- Timer and event listener cleanup is thorough on all paths.
This addresses the previously raised concern about honoring abort signals even when timeouts are disabled.
508-530: LGTM!The
isRetryable()logic correctly identifies transient errors (timeouts, connection resets) as retryable while allowing non-transient errors to fail fast. Thedelay()helper is appropriately simple.
536-608: LGTM!Resource tracking is clean with proper add/remove semantics and the fluent
trackResource()return. The abstract methods correctly define theRuntimeExecutioninterface contract for subclasses.test/bounded-context.test.ts (5)
27-117: LGTM!The
TestContextfixture is well-designed with configurable init behavior (delay, failure, custom error) and proper exposure of protected methods for testing. TheMockResourceprovides clean disposal verification with optional error injection.
123-357: LGTM!Excellent lifecycle test coverage including:
- Idempotency and concurrent init deduplication
- Retry-after-failure semantics
- Guards against
disposedanddisposingstates- Race condition handling (dispose during init, both success and failure paths)
- Resource disposal ordering and error aggregation
The inline subclass pattern for slow dispose testing (lines 188-193) is a clean approach.
364-421: LGTM!Validation tests cover the essential cases for each validator type with appropriate error type assertions (
BridgeProtocolError).
427-523: LGTM!Error classification tests thoroughly cover all pattern-matching cases, state-based classification, pass-through behavior, default fallback, and cause preservation.
749-823: LGTM!Resource ownership tests verify tracking, untracking, chaining, count accuracy, and proper disposal behavior. Good coverage of the resource management API.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
|
All review feedback has been addressed:
Please re-review @coderabbitai |
|
I'll perform a full re-review of the PR with the latest changes. 🧠 Learnings used✅ Actions performedReview triggered.
|
Summary
BoundedContextbase class - a unified abstraction for cross-boundary concernsRuntimeBridge(remains as alias for backwards compatibility)Features
Lifecycle Management
idle → initializing → ready → disposing → disposedValidation Helpers
validateNumeric(),validatePositive(),validateString(),validateNonEmptyString()Error Classification
Bounded Execution
execute()method with timeout, retry, and abort signal supportResource Tracking
trackResource()/untrackResource()for automatic cleanup on disposeNew Files
src/runtime/bounded-context.tssrc/runtime/disposable.tssrc/runtime/validators.tstest/bounded-context.test.tstest/disposable.test.tstest/validators.test.tsTest plan
Closes #149
🤖 Generated with Claude Code